Runtime interrupt binding

[bjoernQ]

This implements the idea outlined in #1063 and explored in #1121

Only GPIO is changed for now to contain the user-facing API to set the interrupt. This is compatible with link-time binding of the interrupts.

I dropped the idea of having (additional) type-state on the drivers. Instead, it's now possible to have a user-handler in addition to the async interrupt handler. There is one possible pitfall: the user must not modify the interrupt status (i.e. clear the interrupt of a GPIO). If they do the async handler won't be able to identify the GPIO. We could avoid that by saving and restoring the interrupt state before/after calling user code. Wasn't sure if we really want that or not

As said, it's for now just implemented for GPIO since we probably first want to align on this before doing the same thing for the other drivers.

In theory we don't even need to apply the change to all drivers at once

[jessebraham]

Thank you for working on this! I took a quick look and things seems reasonable enough, but I'll do a proper review on Monday when I have a bit more brainpower at my disposal.

[MabezDev]

Thanks so much for working on this!

I have some comments to address, hopefully nothing too major. My main focus is making sure we don't create an environment for users to shoot themselves in the foot with this new way of setting interrupts :D.

[MabezDev]

Oh I missed this:

If they do the async handler won't be able to identify the GPIO. We could avoid that by saving and restoring the interrupt state before/after calling user code. Wasn't sure if we really want that or not

I don't think we want to do this, IMO interrupts are an "advanced" concept, and care needs to be given when using them. I think documenting this is good enough for now, if users really keep messing this up, then we could add it behind a features/config option.

[bjoernQ]

I somehow missed the valid concern in your comment on the original exploration PR: #1121 (comment)

One final consideration, what do we want to do when we want to get the Context during an interrupt, this is something we use in esp-wifi so it would be good to have a solution for that.

I think getting the mutable context is very, very unsafe and something most users won't and shouldn't do. I guess it would be fine to require an unsafe transmute for something like that if needed?

[MabezDev]

I somehow missed the valid concern in your comment on the original exploration PR: #1121 (comment)

One final consideration, what do we want to do when we want to get the Context during an interrupt, this is something we use in esp-wifi so it would be good to have a solution for that.

I think getting the mutable context is very, very unsafe and something most users won't and shouldn't do. I guess it would be fine to require an unsafe transmute for something like that if needed?

I agree, I don't expect most users to do this, just as long as it's possible for us, or advanced users to build schedulers etc.

[jessebraham]

I think this is looking pretty good overall, especially for the first iteration. We can probably go ahead and merge it once we've published our patch release tomorrow, this gets rebased, and CHANGELOG.md gets updated.

[JurajSadel]

LGTM!

[MabezDev]

LGTM, thanks for addressing my review comments! This is looking really good.

Just a note to future folks as we implement other drivers I would not use this as a reference - GPIO is a super weird edge case where you actually might want to have async gpio and interrupt driven gpio at the same time. Almost every other driver will be initialized in a "mode" either blocking or async.

[jessebraham]

This just needs a rebase and a CHANGELOG.md entry and we're good to go, I think.

[MabezDev]

One last question, how do #[handler] and #[interrupt] interact with each other? Is the plan that interrupt macro goes away, or is made private? I see that #[interrupt] is still used in the GPIO driver, is this intended?

[bjoernQ]

One last question, how do #[handler] and #[interrupt] interact with each other? Is the plan that interrupt macro goes away, or is made private? I see that #[interrupt] is still used in the GPIO driver, is this intended?

I think #[interrupt] is somewhat convenient to initialize the __INTERRUPTS/__EXTERNAL_INTERRUPTS lookup table.

But I also see how it might be confusing to users. So probably once all drivers are changed, it would be good to at least hide it from users (if we want to continue to use it inside esp-hal).

Or might it be also be confusing to use it in esp-hal? If that's the case and we want to eventually get rid of the macro it would make sense if I remove it from the GPIO driver already now

It's not confusing to me since I worked on this, so probably it's better if I'm not the one to decide that

[jessebraham]

I don't really understand the distinction between #[interrupt] and #[handler] honestly, so if we're going to use both I think this needs to be well documented.

I think having only one way to do something is preferable, as it's impossible to screw up that way, however as noted I don't understand the nuances between the macros.

[MabezDev]

LGTM, thanks for working on this!

I've dropped the RFC label from #1063 and with this merged I will change its status to in-progress.