gpio_set_irq_enabled() will not enable GPIO interrupt handling

[yoavct]

Hi,

Trying to enable interrupts for GPIO using the following sequence will not work:
gpio_set_irq_callback()
gpio_set_irq_enabled()

But using the gpio_set_irq_enabled_with_callback() does work.

The latter includes the two above that set the callback and enable IRQs, but also adds a call to irq_set_enabled(IO_IRQ_BANK0, enabled) without which interrupts will not work.

Unless the intention of the API is to force users to use gpio_set_irq_enabled_with_callback(), it seems that irq_set_enabled() should be moved to _gpio_set_irq_enabled().

[maqifrnswa]

Unless the intention of the API is to force users to use gpio_set_irq_enabled_with_callback(), it seems that irq_set_enabled() should be moved to _gpio_set_irq_enabled().

I think the design intention is to keep gpio_set_irq_enabled() and irq_set_enabled() separate because they are two different things using different parts of the hardware API and different components within the RP2040.

irq_set_enabled() is a function that acts on the Cortex M0+ registers to tell the core to "look out for" a peripherals interrupt.

gpio_set_irq_enabled() sets the peripheral's register that tells the peripheral to raise an interrupt.

I think the current design as is works as intended:
gpio_set_irq_callback()
irq_set_enabled()
gpio_set_irq_enabled()

Or, for convenience, just gpio_set_irq_enabled_with_callback().

I believe the "set call back function in some vector table, enable the interrupt in the core, then enable it in the peripheral" pattern is common in embedded systems, and is what is being done here.

[peterharperuk]

The inconsistency does seem unfortunate. But presumably it would be too dangerous to fix now?

[maqifrnswa]

In my opinion, it isn't inconsistent and doesn't need to be fixed (or am I misunderstanding?) I think it does what it is supposed to in the expected way already.

[peterharperuk]

To me, the function names gpio_set_irq_enabled and gpio_set_irq_enabled_with_callback imply they might both call irq_set_enabled . It seems inconsistent that gpio_set_irq_enabled_with_callback calls irq_set_enabled but gpio_set_irq_enabled does not.

[kilograham]

Yeah gpio_set_irq_enabled_with_callback is the ugly step-child that made it into the SDK 1.0 release from older code, and wasn't really the right API..>

The newer APIs were an attempt to disentangle the mess without breaking backwards compatibility...

The new gpio_set_irq_ refer to a specific GPIO (though perhaps we can give this one an alias of gpio_set_irq_events_).

Open to suggestions

[kilograham]

although as per @maqifrnswa it does kinda work out

[maqifrnswa]

To me, the function names gpio_set_irq_enabled and gpio_set_irq_enabled_with_callback imply they might both call irq_set_enabled . It seems inconsistent that gpio_set_irq_enabled_with_callback calls irq_set_enabled but gpio_set_irq_enabled does not.

I see, thanks. There's a blending of APIs. gpio_set_irq_enabled_with_callback uses both gpio_ and irq_. Elsewhere in the SDK, that function would be considered "higher level" and be in pico_, not gpio_.

Now that I think about it, I actually thought the same thing as @peterharperuk when I first learned the API - "why is the GPIO API special that it's the only hardware API that gets its own IRQ convenience function?" But then I got used to it, and chalked it up to being because GPIO is very very often only used for catching interrupts, and that it could be helpful for beginners. However, it does mask what beginners might need to know and could lead to a misunderstanding of what interrupts are doing.

I'm no fan of API proliferation, but maybe move it to some pico_isr API that has similar "set and enable" functions for each of the hardware APIs?