The Io struct has multiple constructors which makes it confusing to use correctly

[jessebraham]

As a workaround, in #1861 the Io::new_no_bind_interrupt constructor was added to allow initializing the Io struct without binding the GPIO interrupt.

This is a bit of a footgun, and is too easy to mess up. We should try to find a better way to handle this scenario.

[bjoernQ]

IIRC correctly this driver is a bit different than all the others because we want people to easily mix async and their own ISR - if we still think this is something we want to support and do by default I'm not sure how to solve this

• Add more documentation? Probably won't help?
• Remove that constructor and explain (in docs) how to unsafely override the interrupt-handler and tell them they should forget about using async then? (Would make the default use-case less confusing but your special use-case more of a pain)

Maybe there are other options I just can't think of right now

[bugadani]

Can we allow per-pin ISR binding (i.e. similar to how we bind interrupts currently, put function pointers into a big table)? Would that help here?

[Dominaezzz]

Input could be changed to accept Async/Blocking in its type signature, then users can do Input::new() or Input::new_async(). This solves the "can this pin async or not" problem.

Then an additional parameter to Input::new_async would be a type that represents "I pinky promise I'm calling the handler for this pin". (Something along the lines of https://docs.embassy.dev/embassy-stm32/git/stm32f100rc/macro.bind_interrupts.html). This forces the user to be aware of what they're doing.

Then maybe this "pinky promise" type can be easily acquired by either calling Io::new_async or using an unsafe ctor?
The documentation for the unsafe ctor can say something like, you must call handle_gpio_interrupt.

For the easy/common path, I'm picturing code that looks like this.

let io = Io::new_async(peripherals.GPIO, peripherals.IO_MUX);

let button = Input::new_async(io.pins.gpio7, Pull::Up, io.handler); // name pending bikeshed

Then for the uncommon path.

let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

let handler = unsafe { GpioHandlerRegisteration::new() }; // name pending bikeshed
let button = Input::new_async(io.pins.gpio7, Pull::Up, handler);

[bugadani]

Why would you allow overriding the handler of the async API? Why would you require the async api to allow registering an irq handler? And thirdly, why would you need unsafe code for this? We can add an unsafe way to override the global handler, sure, but this is some unnecessary complication on the per-pin API.

[Dominaezzz]

Why would you allow overriding the handler of the async API?

What do you mean? In my comment I described an "either or" situation.

• Either you give ownership of the handler registration to the hal and get async,
• Or you set up your own handler, and you can opt-in to the async API of a pin if you promise to call the hal's handler.

Why would you require the async api to allow registering an irq handler?

I was trying to allow mixing of both async and custom irq.

And thirdly, why would you need unsafe code for this?

To raise some eyebrows. I agree it's not strictly necessary. Having your async program hang is perfectly sound Rust.

We can add an unsafe way to override the global handler, sure, but this is some unnecessary complication on the per-pin API.

I suppose that works. It ensures that users are aware of the fact that the async API won't work as expected if the handler is overridden.
The only minor peeve I have with this is registering a handler shouldn't be an unsafe operation. It's safe everywhere else within the HAL.
Also to you third point, this way uses unsafe too 😛 .

[bugadani]

So what I have in mind:

• Allow unsafely replacing the global irq handler by calling io.set_interrupt_handler. This needs to be done before moving out the first pin, so it's not possible to do once pins are in use.
• Modify the default handler into an array of handler functions. This way we can freely attach/detach irq handlers to specific pins.
• Do not allow replacing the irq handler of an async pin (at least not safely).

This is simple as it has sane defaults, at least as far as my midnight brain is concerned. This is also flexible as we can mix and match async pins with blocking pins + irq handlers. There may be a slight perf hit caused by the fn table, but if the default handlers are in RAM, it shouldn't be too bad?

What am I missing?

[bjoernQ]

These are all very good ideas 👍 However I think the original problem with RTIC is this one:

To handle the GPIO interrupt they generate a function unsafe extern "C" fn GPIO(...) to replace https://github.com/esp-rs/esp-pacs/blob/c717453df31e5f37c48edd287906bbf09aa82112/esp32c3/src/lib.rs#L114 at compile time

In the constructor the GPIO driver changes that handler address - so their implementation won't get called

So maybe all it needs would be to use https://docs.esp-rs.org/esp-hal/esp-hal/0.20.0/esp32c3/esp_hal/interrupt/fn.bind_interrupt.html to replace the handler after creating the driver in RTIC (no need for the additional constructor then)?

Or they could use https://docs.esp-rs.org/esp-hal/esp-hal/0.20.0/esp32c3/esp_hal/interrupt/fn.enable_direct.html since they want direct vectoring 🤔

But I have to admit I don't know enough about RTIC - I think last time I tried to use it was when it still was named RTFM

[bugadani]

This also means that RTIC, when it sits on the GPIO interrupt, is incompatible with a significant portion of our API (any async GPIO, any GPIO interrupt handling). How can we teach the users about this?

[bugadani]

We can add a bind_io_interrupts: IoInterrupt to esp_hal::Config where IoInterrupt is an enum and move the IO_MUX initialisation into esp_hal::init. That way we can remove Io::new completely and just expose the pins to the user.

enum IoInterrupt {
    DoNotBind,
    Default(Priority),
    User(InterruptHandler)
}

[bjoernQ]

I think I would like to have it initialized in esp_hal::init given 99.9% of all real-world use-cases will need at least some pins.

Users will still need to know when to do what but the enum is definitely more intuitive than a weird named second constructor.

And while writing this, even if we don't want to move initialization to esp_hal::init the constructor could take that enum.
Only "downside" would be Io won't implement InterruptConfigurable anymore but not sure that would be a real issue

[Dominaezzz]

What exactly is "IO_MUX initialisation" ? It seems to be just setting the interrupt handler.
What's the value in hiding this from the user by putting it in esp_hal::init and making it different from all the other peripheral APIs? And what's wrong with Io::new?

[bugadani]

Are you really asking why I want to hide something that the users need to type out currently, but don't really need?

[Dominaezzz]

Yes that's what I'm asking sans the "but don't really need" bit. Just because most users tend to do something one particular way, doesn't mean it's "boilerplate" that should be pruned with alacrity.

Since there is a consistent/uniform way to set an interrupt handler on all peripheral drivers, GPIO should follow suit.
Or are you intending to have all setting of interrupt handlers happen in esp_hal::init?

[bugadani]

GPIOs can't really follow suit. To do so would require the ability to set a handler for each pin, which we can't really do (well, unless hackery, but that again removes the need for Io), or not in the same form as other peripherals. We have a single GPIO interrupt source, but users rarely use the peripheral as a single unit.

A user-provided GPIO interrupt handler currently is hugely problematic as we don't have any guarantees against messing up. Advertising that as the normal way to use interrupts, just like in the case of other peripherals is currently a mistake.

[Dominaezzz]

GPIO is currently following suit albeit not exactly since it's missing the new_async constructor.

My suggestion (which always prefer flexibility over ergonomics) would be to have an all or nothing API. Either you have the async API or you can set your handler. No mixing, just like all the other peripherals do. This way there's no room for mistakes or messing up.

Btw, this isn't the only peripheral where a handler is shared between multiple objects, LCD_CAM and GDMA (on the chips with fewer channels) also have this issue. Do you want those handlers moved to esp_hal::init as well?

Ideally, interrupt handler setting would be separate from the driver. Similar to how esp-idf does it and how embassy-stm32 (I cannot overstate how much I love their approach to interrupts) also does it.

Just to be clear, removing Io::new mostly makes sense but the interrupt setup being different doesn't. @bjoernQ's "interrupt as resource" idea would make sense here, where it's separate from the driver and you can combine both to give you something async.

[bjoernQ]

... have an all or nothing API. Either you have the async API or you can set your handler.

Yes - it all boils down to this. Doing that would be an easy fix I guess

But GPIO is also a bit special - e.g. a user might totally want to use async for almost everything but also wants to bit-bang a not too slow protocol where the penalty of asynchronously handling state changes is too much (ok - sounds a little bit made up but that was my understanding why we do things this way for GPIO (and only for GPIO))

[Dominaezzz]

(ok - sounds a little bit made up but that was my understanding why we do things this way for GPIO (and only for GPIO)

Definitely not made up, I had the same problem with SPI (but the move API solved it for me).
async is slow, which makes it inappropriate for timing sensitive use cases, especially if there are lots of futures to poll.

e.g. a user might totally want to use async for almost everything but also wants to bit-bang a not too slow protocol

I'm curious if this protocol could be implemented using one of the other peripherals.

I still like my "I promise I'm calling the handler" type idea here tbh, though I'll admit it's somewhat verbose.