You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/11/07 20:27:24 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
v01d opened a new pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240
## Summary
This change improves upon current support for pin interrupts. Before,
a pin interrupt was handled (with nrf52_gpiote_setevent) using one
of the eight available GPIOTE channels. Moreover, it didn't event let
the user specify which channel to use (simply tried to get a free one).
Also, it was buggy since it did not consider unsetting the callback.
Besides GPIOTE channels, there is another way to deal with pin interrupts.
The GPIO peripheral is capable of generating a PORT event
(for the whole GPIO port) depending on the pin SENSE configuration
(HIGH or LOW, or NONE) and GPIO DETECTMODE register
(latching or non-latching). This means that GPIOs can be made level
or edge sensitive by handling the ISR for the whole port.
This change then renames nrf52_gpiote_setevent into nrf52_gpio_set_ch_event,
maintaining functionality of original function, but now allows specifying
channel (and correctly handles unsetting the callback). Then, a
new nrf52_gpio_set_pin_event is added, which allows to set a callback
for a given pin. During initialization, interrupt for the PORT event is
enabled and handled in such way that for each pin whose corresponding
bit in LATCH register (indicates the result of pin SENSEing) the
callback for this pin will be invoked. This mechanism means that
every pin can get an ISR. It also avoids using GPIOTE channels for this
purpose which carry higher current consumption.
There was only one use of nrf52_gpio_setevent() which was migrated
into nrf52_gpio_set_pin_event().
## Impact
nRF52 pin interrupt handling
## Testing
Set pin interrupt using SENSE mechanism, works correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519805023
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -61,9 +61,14 @@ struct nrf52_gpiote_callback_s
* Private Data
****************************************************************************/
-/* Interrupt handlers attached to each GPIOTE */
+/* Callbacks attached to each GPIOTE channel */
-static struct nrf52_gpiote_callback_s g_gpiote_callbacks[GPIOTE_CHANNELS];
+static struct nrf52_gpiote_callback_s g_gpiote_ch_callbacks[GPIOTE_CHANNELS];
+
+/* Callbacks attached to each GPIO pin */
+
+static struct nrf52_gpiote_callback_s
Review comment:
I think this will really complicate Kconfig. You will have to know how many pins will require an interrupt, which is not something that is usually done on any other arch. The alternative you suggest means that you have to search for the complete list of callbacks for the matching pin (trades speed for memory). I don't think that the memory cost-saving of not having all pins be configurable with a callback (256 bytes for the whole port) is worth it for the added complexity of having to know before hand the number of pins you will want to set with callbacks.
There's an underlying issue here IMHO: if you look at stm32_gpiosetevent it makes you think you are setting the interrupt for the pin when you can only set interrupts by groups of pins sharing the interrupt (in nRF52 this group is the whole GPIO port). If you look at how this is used in NuttX, it is used as it would effectively let you set interrupts for pins individually, which means that setting the callback for another pin in the same group will unset the first one.
In this implementation I intended to explicitly support per-pin callbacks since otherwise you are simply setting one ISR for all GPIOs (assuming one port as in nRF52832). I think that this is the way we should handle pin interrupts to add some value over simply setting a callback for the ISR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724160645
> With an all-or-nothing approach, the whole idea of Kconfig makes no sense.
Why would it not make sense? I just changed it to support per-pin interrupts or just interrupts for the individual port event of each port. This is the bare minimum option for supporting the PORT event (we could even add an option to not support that either or not support channels events, but that is beyond this PR I think).
I'll push the change and you can see the idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519215478
##########
File path: arch/arm/src/nrf52/hardware/nrf52_gpiote.h
##########
@@ -67,7 +67,8 @@
#define GPIOTE_INT_IN_MASK (0xff << GPIOTE_INT_IN_SHIFT)
# define GPIOTE_INT_IN(i) ((1 << (i + GPIOTE_INT_IN_SHIFT)) & GPIOTE_INT_IN_MASK)
-#define GPIOTE_INT_PORT 31 /* Bit 31: Enable interrupt for event PORT */
+#define GPIOTE_INT_PORT_SHIFT 31 /* Bit 31: Enable interrupt for event PORT */
+#define GPIOTE_INT_PORT (1 << GPIOTE_INT_PORT_SHIFT)
Review comment:
this is a bug I fixed as part of this change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519807866
##########
File path: boards/arm/nrf52/nrf52840-dk/src/nrf52_sx127x.c
##########
@@ -88,7 +88,7 @@ static int sx127x_irq0_attach(xcpt_t isr, FAR void *arg)
/* IRQ on rising edge */
- nrf52_gpiosetevent(GPIO_SX127X_DIO0, true, false, false, isr, arg);
+ nrf52_gpiote_set_pin_event(GPIO_SX127X_DIO0, isr, arg);
Review comment:
Can you check if this pin is already HIGH on boot? In that case the interrupt will fire immediately as you say. In such case it may indeed be better to use a GPIOTE channel as it directly supports edge sensitive trigger.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519699907
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -61,9 +61,14 @@ struct nrf52_gpiote_callback_s
* Private Data
****************************************************************************/
-/* Interrupt handlers attached to each GPIOTE */
+/* Callbacks attached to each GPIOTE channel */
-static struct nrf52_gpiote_callback_s g_gpiote_callbacks[GPIOTE_CHANNELS];
+static struct nrf52_gpiote_callback_s g_gpiote_ch_callbacks[GPIOTE_CHANNELS];
+
+/* Callbacks attached to each GPIO pin */
+
+static struct nrf52_gpiote_callback_s
Review comment:
What if we introduce a new structure for pin events
```
struct nrf52_gpiote_pin_callback_s
{
xcpt_t callback;
FAR void *arg;
uint8_t pin;
#ifdef CONFIG_NRF52_HAVE_PORT1
uint8_t port;
#endif
}
```
and configure size of g_gpiote_pin_callbacks from Kconfig?
With this we can save some resources and eliminate the scanning of all pins.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519962460
##########
File path: boards/arm/nrf52/nrf52840-dk/src/nrf52_sx127x.c
##########
@@ -88,7 +88,7 @@ static int sx127x_irq0_attach(xcpt_t isr, FAR void *arg)
/* IRQ on rising edge */
- nrf52_gpiosetevent(GPIO_SX127X_DIO0, true, false, false, isr, arg);
+ nrf52_gpiote_set_pin_event(GPIO_SX127X_DIO0, isr, arg);
Review comment:
Ok, so this could then be done using a GPIOTE channel. I can change it if you prefer.
Note that it can be handled with a per-pin callback with the PORT event but requires more work to make it edge-sensitive, it would require:
- setting SENSE to LOW initially
- setup the pin callback to toggle the SENSE polarity on each call
- only react to relevant edge (in this case, low-to-high, so it would be done when entering callbacking with SENSE set to HIGH).
This toggling is annoying, but is actually considered in nordic SDK (you pass a NRF_GPIOTE_POLARITY_TOGGLE flag, and on each call it toggles the sense polarity) to handle this case. The motivation to do it this way is to not use GPIOTE unless necessary, since it is a scarce resource and it uses more current (if SPI is active, it is about 400uA due to chip errata present in all chips).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724177601
Now it looks much better for me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519687269
##########
File path: arch/arm/src/nrf52/nrf52_gpio.c
##########
@@ -204,7 +236,8 @@ int nrf52_gpio_config(nrf52_pinset_t cfgset)
switch (cfgset & GPIO_FUNC_MASK)
{
case GPIO_INPUT: /* GPIO input pin */
- break; /* Already configured */
+ nrf52_gpio_sense(cfgset, port, pin);
Review comment:
This triggers GPIOTE events even if we haven't enabled pin events yet.
As I understand, SENSE is only used for GPIOTE, then it should be configured when we enable the given pin event.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519989805
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -135,6 +141,61 @@ static int nrf52_gpiote_isr(int irq, FAR void *context, FAR void *arg)
}
}
+ /* Check for PORT event */
+
+ regval = nrf52_gpiote_getreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET);
+ if (regval)
+ {
+ uint32_t addr = 0;
+
+ /* Ack PORT event */
+
+ nrf52_gpiote_putreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET, 0);
+
+ /* For each GPIO port, get LATCH register */
+
+ for (i = 0; i < NRF52_GPIO_NPORTS; i++)
+ {
+ switch (i)
+ {
+ case 0:
+ addr = NRF52_GPIO_P0_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#ifdef CONFIG_NRF52_HAVE_PORT1
+ case 1:
+ addr = NRF52_GPIO_P1_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#endif
+ }
+
+ /* Retrieve LATCH register */
+
+ regval = getreg32(addr);
+
+ /* Clear LATCH register (this may set PORT again) */
+
+ putreg32(0xffffffff, addr);
+
+ /* Check for pins with DETECT bit high in LATCH register
+ * and dispatch callback if set
+ */
+
+ for (j = 0; j < NRF52_GPIO_NPINS; j++)
Review comment:
I didn't find a non-looping approach to only go over bits set. I will ultimately requires looping to find the position of next bit set so I don't think it helps much. I will just add a check to see if the register is zero and not look for bits set then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724159011
For nrf52840 if we don't want to use nrf52_gpiote_set_pin_event this adds 512 bytes for nothing.
Adding one or two options to Kconfig is not a problem. In return, we get a system adapted to our requirements.
With an all-or-nothing approach, the whole idea of Kconfig makes no sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519954635
##########
File path: arch/arm/src/nrf52/nrf52_gpio.c
##########
@@ -204,7 +236,8 @@ int nrf52_gpio_config(nrf52_pinset_t cfgset)
switch (cfgset & GPIO_FUNC_MASK)
{
case GPIO_INPUT: /* GPIO input pin */
- break; /* Already configured */
+ nrf52_gpio_sense(cfgset, port, pin);
Review comment:
OK, you're right. I didn't notice that DETECT has more functionality than just a PORT event.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-723491910
@btashton this is somewhat related to the pending fix/changes in nRF52 SPIM since there are some direct register accesses to PPI and GPIOTE in there which should somehow be made to ensure do not conflict with PPI/GPIOTE channels used via the low-level nRF52 api for these peripherals. I think a second step would be to add the notion of "reserved" PPI and GPIOTE channels and then add DEBUGASSERT() on PPI/GPIOTE for these channels to ensure the user does not attempt to use a "reserved" channel. Also, note that there's a specific errata for using SPIM and GPIOTE together, so we should consider that in the workaround for the other SPIM errata :facepalm:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519759945
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -135,6 +141,61 @@ static int nrf52_gpiote_isr(int irq, FAR void *context, FAR void *arg)
}
}
+ /* Check for PORT event */
Review comment:
I think this should be configurable with Kconfig. There is no point in including this logic if we don't want to use SENSE mechanism
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519694190
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -135,6 +141,61 @@ static int nrf52_gpiote_isr(int irq, FAR void *context, FAR void *arg)
}
}
+ /* Check for PORT event */
+
+ regval = nrf52_gpiote_getreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET);
+ if (regval)
+ {
+ uint32_t addr = 0;
+
+ /* Ack PORT event */
+
+ nrf52_gpiote_putreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET, 0);
+
+ /* For each GPIO port, get LATCH register */
+
+ for (i = 0; i < NRF52_GPIO_NPORTS; i++)
+ {
+ switch (i)
+ {
+ case 0:
+ addr = NRF52_GPIO_P0_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#ifdef CONFIG_NRF52_HAVE_PORT1
+ case 1:
+ addr = NRF52_GPIO_P1_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#endif
+ }
+
+ /* Retrieve LATCH register */
+
+ regval = getreg32(addr);
+
+ /* Clear LATCH register (this may set PORT again) */
+
+ putreg32(0xffffffff, addr);
+
+ /* Check for pins with DETECT bit high in LATCH register
+ * and dispatch callback if set
+ */
+
+ for (j = 0; j < NRF52_GPIO_NPINS; j++)
Review comment:
Scanning all pins is very ineffective, especially when we have more than 1 port.
It would be better to store the pin and port for a given event and compare only these.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519735883
##########
File path: boards/arm/nrf52/nrf52840-dk/src/nrf52_sx127x.c
##########
@@ -88,7 +88,7 @@ static int sx127x_irq0_attach(xcpt_t isr, FAR void *arg)
/* IRQ on rising edge */
- nrf52_gpiosetevent(GPIO_SX127X_DIO0, true, false, false, isr, arg);
+ nrf52_gpiote_set_pin_event(GPIO_SX127X_DIO0, isr, arg);
Review comment:
I checked it and it doesn't work.
`nrf52_gpiote_isr` is only called somewhere on startup where it shouldn't be (probably because of `nrf52_gpio_sense` in `nrf52_gpio_config`)
Changed to `nrf52_gpiote_set_ch_event(GPIO_SX127X_DIO0, 0, true, false, isr, arg);` and it triggers IRQ.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519955450
##########
File path: boards/arm/nrf52/nrf52840-dk/src/nrf52_sx127x.c
##########
@@ -88,7 +88,7 @@ static int sx127x_irq0_attach(xcpt_t isr, FAR void *arg)
/* IRQ on rising edge */
- nrf52_gpiosetevent(GPIO_SX127X_DIO0, true, false, false, isr, arg);
+ nrf52_gpiote_set_pin_event(GPIO_SX127X_DIO0, isr, arg);
Review comment:
Yes, this pin is HIGH on boot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724016178
To sum up:
- I can make the pin scanning mechanism more efficient by scanning only set bits (I think, I will try and report)
- We should not add partial support for pin callbacks: it should be all or nothing, otherwise it makes configuration much more complex. If it is not selected, set_pin_event would not be available. I could add a set_port_event in that case, so that you can still
set an ISR for the whole PORT event (as the ISR is the same as for GPIOTE channels).
- Indeed SENSE mechanism is not a direct replacement for most situations since it does not directly supports edge-sensitive and also only supports either HIGH or LOW. So most likely using a GPIOTE channel will be used in such case
- I will revert the use of old nrf52_gpiosetevent to use a GPIOTE channel
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724178032
> Now it looks much better for me.
Great, sorry if I wasn't clear when explained the idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl merged pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl merged pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519988744
##########
File path: boards/arm/nrf52/nrf52840-dk/src/nrf52_sx127x.c
##########
@@ -88,7 +88,7 @@ static int sx127x_irq0_attach(xcpt_t isr, FAR void *arg)
/* IRQ on rising edge */
- nrf52_gpiosetevent(GPIO_SX127X_DIO0, true, false, false, isr, arg);
+ nrf52_gpiote_set_pin_event(GPIO_SX127X_DIO0, isr, arg);
Review comment:
I changed this to use a channel
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-724169588
I just pushed what I mentioned, the feature is now optional and if disabled exposes a PORT event callback instead (one for each port). It also defaults to y or n depending on DEFAULT_SMALL.
I've updated the commit message to reflect the new code and the PR message as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519793826
##########
File path: arch/arm/src/nrf52/nrf52_gpio.c
##########
@@ -204,7 +236,8 @@ int nrf52_gpio_config(nrf52_pinset_t cfgset)
switch (cfgset & GPIO_FUNC_MASK)
{
case GPIO_INPUT: /* GPIO input pin */
- break; /* Already configured */
+ nrf52_gpio_sense(cfgset, port, pin);
Review comment:
I'm not sure what you mean by GPIOTE vs pin events.
SENSE is used for waking up the MCU from IDLE sleep (without an extra interrupt) but most importantly from system OFF.
SENSE generates an internal DETECT signal (depending on DETECTMODE register), which in turns generates the PORT event. What controls waking up is the DETECT signal and is independent of handling (or not) the PORT event via interrupt.
The thing about DETECT is that if you enable SENSE_HIGH when the pin is HIGH it will immediately interrupt (as expected). An option to not generate the interrupt is to make the interrupt enable be dependant on having the callback set (as an indication that the user really wants the interrupt to be generated).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519794685
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -135,6 +141,61 @@ static int nrf52_gpiote_isr(int irq, FAR void *context, FAR void *arg)
}
}
+ /* Check for PORT event */
+
+ regval = nrf52_gpiote_getreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET);
+ if (regval)
+ {
+ uint32_t addr = 0;
+
+ /* Ack PORT event */
+
+ nrf52_gpiote_putreg(NRF52_GPIOTE_EVENTS_PORT_OFFSET, 0);
+
+ /* For each GPIO port, get LATCH register */
+
+ for (i = 0; i < NRF52_GPIO_NPORTS; i++)
+ {
+ switch (i)
+ {
+ case 0:
+ addr = NRF52_GPIO_P0_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#ifdef CONFIG_NRF52_HAVE_PORT1
+ case 1:
+ addr = NRF52_GPIO_P1_BASE + NRF52_GPIO_LATCH_OFFSET;
+ break;
+#endif
+ }
+
+ /* Retrieve LATCH register */
+
+ regval = getreg32(addr);
+
+ /* Clear LATCH register (this may set PORT again) */
+
+ putreg32(0xffffffff, addr);
+
+ /* Check for pins with DETECT bit high in LATCH register
+ * and dispatch callback if set
+ */
+
+ for (j = 0; j < NRF52_GPIO_NPINS; j++)
Review comment:
Yes, I can improve this. I was looking for some non-looping code to only go through set bits in LATCH register. This way it does only what is needed and does nothing when LATCH is zero.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
raiden00pl commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519718376
##########
File path: arch/arm/src/nrf52/nrf52_gpio.h
##########
@@ -187,6 +187,12 @@
typedef uint32_t nrf52_pinset_t;
+enum nrf52_gpio_detectmode_e
Review comment:
This belongs to bit-encoded nrf52_pinset_t.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#issuecomment-723492344
Thanks @v01d I'm digging out of my backlog from the week right now, but I think I'll be able to look at the SPIM and this tonight or in the morning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2240: nrf52 GPIO/GPIOTE: better expose pin interrupt capability
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2240:
URL: https://github.com/apache/incubator-nuttx/pull/2240#discussion_r519809065
##########
File path: arch/arm/src/nrf52/nrf52_gpiote.c
##########
@@ -135,6 +141,61 @@ static int nrf52_gpiote_isr(int irq, FAR void *context, FAR void *arg)
}
}
+ /* Check for PORT event */
Review comment:
If we do so, we should disable the set_pin_event function as well, so that if one wants to use this mechanism it can depend on the corresponding config symbol. But as I said before, it may be better to always support it (once I make the change to only scan the set bits in LATCH).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org