Add new GPIO APIs for adding shared GPIO handlers, and improve docs #850
No reviewers
Labels
No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-raspberrypi/pico-sdk!850
Loading…
Reference in a new issue
No description provided.
Delete branch "gpio_irq_improvements"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Note this PR includes the get_core_num() change as they are dependent
@ -347,63 +353,288 @@ void gpio_set_drive_strength(uint gpio, enum gpio_drive_strength drive);*/nit: "an GPIO" -> "a GPIO"
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highnit: lowercase 'w' on "which"
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highbease -> based ?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is high"it callback is NULL" -> " if callback is NULL" ?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highTypo? https://www.doxygen.nl/manual/commands.html#cmdendcode
Not clear what this comment is signifying?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highThis function takes a single
uint gpio, so should "specified GPIOs" be "specified GPIO" ?@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is high"This configures affects the..." doesn't sound right?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highIs it worth clarifying that this is a bitmask of events, or is that fairly self-evident?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is high\endcode?(and also in multiple other places)
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is high"can be controller" -> "can be controlled"
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is high"can be controller" -> "can be controlled"
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highShould this have a
check_gpio_param(gpio)?Should this have been
GPIO_CALLBACK_IRQ_ORDER_PRIORITY, or is there a reason for it being different?@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highI wonder if it's worth mentioning (if I've understood things correctly) that only 'edge' events can be cleared / acknowledged, and 'level' events can't be cleared? (since the corresponding fields in the INTR registers are all RO).
@ -407,0 +500,4 @@* \code{.c}* void my_irq_handler(void) {* if (gpio_get_irq_event_mask(my_gpio_num) & my_gpio_event_mask) {* gpio_acknowledge_irq(my_gpio_num, my_gpio_event_mask);Could it be worth adding e.g. a
gpio_set_irq_enabled_with_callback_maskedfunction which would take a bitmask of GPIO numbers? (so that the user doesn't have to make multiple separategpio_set_irq_enabledcalls)@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highIRQs events -> IRQ events?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highnit: "a explicit" -> "an explicit"
@ -407,0 +583,4 @@** In addition to the default mechanism of a single GPIO IRQ event callback per core (see \ref gpio_set_irq_callback),* it is possible to add explicit GPIO IRQ handlers which are called independent of the default event callback.*This is probably a dumb question, but if the user has to manually check for GPIO numbers inside their callback-function themselves, what's the advantage of using these "raw handlers" over the "shared handler"? From looking at the implementation of
gpio_add_raw_irq_handler_with_order_priority_maskedit seems like if you added multiple raw-handlers (each with a separate callback), then each of the callbacks would get called for every GPIO IRQ that's registered to be handled by a raw-handler?So you could just do the same thing with the shared-handler, by calling a different function depending on the GPIO number that it was passed?
No doubt I'm missing something obvious / fundamental 😕
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highcurrent code -> current core
nope it is different, but i'm adding a another constant
If you use
gpio_add_raw_irq_handler_maskedto e.g. set up GPIOs 2, 3 and 4 to be handled bymy_callback1, is it possible that you might want to switch GPIOs 2 and 4 back to the default handler, but not remove themy_callback1handler that's still being used by GPIO 3?@ -407,0 +500,4 @@* \code{.c}* void my_irq_handler(void) {* if (gpio_get_irq_event_mask(my_gpio_num) & my_gpio_event_mask) {* gpio_acknowledge_irq(my_gpio_num, my_gpio_event_mask);the events may be different, and I was disfavoring the original method only, so don't think ill change it
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highspecified GPIOs -> specified GPIO
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highShould there be a PICO_CONFIG stanza for this, or is it something that you don't expect users to change?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highI have renamed params and methods to be more explicitly event_mask
sadly some of the enums and indeed function names were badly/imprecisely named from the start
What does "non-existent GPIO" mean here, given that you're iterating from
0toNUM_BANK0_GPIOS?@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highHmm; i wasn't event aware of that!!! cc @liamfraser, @Wren6991
I think it would probably be more confusing to change it; it is harmless as is.
@ -407,0 +583,4 @@** In addition to the default mechanism of a single GPIO IRQ event callback per core (see \ref gpio_set_irq_callback),* it is possible to add explicit GPIO IRQ handlers which are called independent of the default event callback.*Yes, you can, but only one person can set the shared calback function. Therefore if you have two independent/unrelated bits of code that want to respond to GPIO IRQ they would stomp on each other if they used the old API. Most people will continue to use the existing API, but libraries that need to hook a GPIO should add their own handler.
well i'm going in steps of 8, so the comment relates to what happens if NUM_BANK0_GPIOs is not divisible by 8 (which it might not be, and indeed it is 30)
@ -407,0 +583,4 @@** In addition to the default mechanism of a single GPIO IRQ event callback per core (see \ref gpio_set_irq_callback),* it is possible to add explicit GPIO IRQ handlers which are called independent of the default event callback.*Ahhh, "libraries that need to hook a GPIO should add their own handler" was the bit I was missing, thanks 😃
(is that worth clarifying in the doxygen?)
Ahhh, so I guess the fix for that would be to put a
if (i >= NUM_BANK0_GPIOS) break;inside the inner for-loop? (or even change the inner for-loop itself tofor(uint i=gpio;events8 && i<gpio+8 && i<NUM_BANK0_GPIOS;i++) {)@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highnit: I guess the full-stop here ought to be a colon?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highYes, the level IRQs are non-latching. If we did have a register you could clear, that register would immediately re-pend anyway.
Should we assert that there is not already a raw IRQ for this GPIO?
Reasoning is that we unconditionally clear the raw bit in the
removefunction, so goingadd->add->removeon the same GPIO leaves it marked as non-raw even though it does have a raw handler installed.@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highSo yes if you are interrupting on GPIO level, you "clear" the IRQ by changing the IRQ enable to interrupt on the opposite level.
Not convinced there is anything that needs fixing here. It's not totally clear your proposed change would make anything faster -- seems like six of one, half a dozen the other.
This assumption is correct btw, and I believe always will be.
Happy with this, other than discussion about whether to assert on double-hooking the same pin, and the comment nits.
At risk of bikeshedding, I'm not crazy about the name
raw, as opposed to e.g.per_pin, but I don't feel strongly enough about this to not approveGood point! I think this code works fine as long as you add/remove with the same masks, and some cases where you fail to do this will be caught by an assert on removing the same IRQ twice, but in this case you would actually just lock up the core because nobody would clear the IRQ status.
I don't see any way to fix this without excessive bookkeeping, so maybe we just need to document that add/remove must use the same mask?
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highah; probably worth mentioning that then
@ -124,7 +126,7 @@ enum gpio_slew_rate gpio_get_slew_rate(uint gpio) {// Enum encoding should match hardware encoding on RP2040oops; these are the wrong way around
Good point. And adding two handlers for the same pin is silly anyway, as you are unlikely to have two different event mask handlers for the same pin. I will add an assert
@ -368,0 +372,4 @@* bit | constant | interrupt* ----|----------------------------------------------------------* 0 | GPIO_IRQ_LEVEL_LOW | Continuously while level is low* 1 | GPIO_IRQ_LEVEL_HIGH | Continuously while level is highi have added some blurb
@ -407,0 +583,4 @@** In addition to the default mechanism of a single GPIO IRQ event callback per core (see \ref gpio_set_irq_callback),* it is possible to add explicit GPIO IRQ handlers which are called independent of the default event callback.*i think these comments are outdated as I don't see the text "libraries that need to"
fixed
ok really fixed
note it isn't per pin anyway, as it can be more than one pin :-) sticking with raw for now