Runtime interrupt handler binding #1121

bjoernQ · 2024-01-26T16:15:02Z

This is not intended to get merged as is - it's exploring a possible solution to #1063

Idea is to have minimal changes and keep compatibility (to not break the direct-vectored feature and to reduce friction and effort)

Basically, to runtime bind an interrupt handler (without unsafe) the user needs to own or have a mutable reference of the corresponding peripheral. (Instead of making interrupts resources). This also means a user could re-bind / unbind the handler.

For RISC-V we would need to change the PACs to place the interrupt vector to .rwtext (tested with ESP32-C3 locally only - but is probably a good thing anyways).

Nothing changed regarding linking.

I only implemented it for C3 and S3 and only for GPIO here.

For the mentioned edge-case (and I think we have more like that) I have an idea which I will explore next

bjoernQ · 2024-01-30T08:12:20Z

BTW this isn't expected to build in CI because it references a local PAC for C3 - so to try it you would need to checkout the branch and make sure you have a local C3-PAC with the interrupt handler table placed in .rwtext ( https://github.com/esp-rs/esp-pacs/blob/760718123d244a93c6cd07a146773a48801c1a7c/xtask/src/main.rs#L213C47-L213C52 )

bjoernQ · 2024-01-31T08:40:52Z

Not sure when I will get to check my ideas regarding DMA so I will just write it down here:

We probably should not handle those things in lower-level code since it's very peripheral specific - but we could handle that in the driver.

e.g. imagine we could attach an interrupt handler to a single DMA channel ... like channl0.set_interrupt_handler(...); - the driver would install the real interrupt handler which will check the channel causing the interrupt and dispatch to the channel's interrupt handler

Thinking about it and given my experiments here I also wonder if that topic could/should be separated.
The current approach already solves our original problems and not doing anything about those edge cases now at least doesn't make anything worse 🤔

MabezDev · 2024-01-31T12:23:26Z

Not sure when I will get to check my ideas regarding DMA so I will just write it down here:

We probably should not handle those things in lower-level code since it's very peripheral specific - but we could handle that in the driver.

e.g. imagine we could attach an interrupt handler to a single DMA channel ... like channl0.set_interrupt_handler(...); - the driver would install the real interrupt handler which will check the channel causing the interrupt and dispatch to the channel's interrupt handler

Thinking about it and given my experiments here I also wonder if that topic could/should be separated. The current approach already solves our original problems and not doing anything about those edge cases now at least doesn't make anything worse 🤔

You're right, it might be worth separating the DMA concerns. It's unlikely that a user will use async but then want to do SPI + DMA + Interrupts (though it is possible, and we should support it eventually), they would most likely use async SPI instead. This could cut down on the work we need to do initially. I will make a note of that in the tracking issue.

MabezDev · 2024-01-31T12:37:45Z

esp32c3-hal/examples/embassy_wait.rs

@@ -31,7 +31,7 @@ async fn main(_spawner: Spawner) {
        esp32c3_hal::timer::TimerGroup::new(peripherals.TIMG0, &clocks),
    );

-    let io = IO::new(peripherals.GPIO, peripherals.IO_MUX);
+    let io = IO::new_async(peripherals.GPIO, peripherals.IO_MUX);


How does creating IO in a blocking way, prevent async calls on the pins produced? I guess the pins also need a type state :(.

Maybe we just make the async handler accessible, and document then if you want to mix async pins and interrupt pins, you must make sure to call the async handler too?

oh yes ... type state everywhere

I like the idea of letting the user mix-and-match async and blocking - on the other hand forcing users to not misuse the API at compile time feels a bit cleaner. I wonder if the freedom of mixing async and blocking is something that people would like to have.

In that case it could be nice if we could at least somehow check they are calling our async handler at runtime - no idea how to do that unfortunately

I wonder if the freedom of mixing async and blocking is something that people would like to have.

Yes, people have requested it a few times.

I've been thinking some more about this a bit, I think we really want to avoid type states in the pins. I think we can figure something out with IO mux to always call the async handler after the user handler or something like that, but this would need some thorough documentation as it would probably be quite easy to break things.

MabezDev · 2024-01-31T13:02:29Z

esp-hal-common/src/peripheral.rs

+                $(
+                    $(
+                        paste::paste!{
+                            pub fn [<bind_ $interrupt:lower _interrupt >](_peripheral: &mut peripherals::$name, handler: unsafe extern "C" fn() -> ()) {


I struggle to parse macros, so I might be missing something here, should this just be a method on the peripheral itself?

E.g

impl GPIO { fn bind_interrupt(&mut self, handler: extern "C" fn()) {} }

That's brilliant! Much better

We should also cfg this method away if neither of the rt features are enabled too, that would be a fun one to debug if we didn't :D.

MabezDev · 2024-01-31T14:31:26Z

esp-hal-common/src/gpio.rs

+    }
+
+    pub fn set_interrupt_handler(&mut self, handler: unsafe extern "C" fn() -> ()) {
+        let mut gpio = unsafe { crate::peripherals::GPIO::steal() };


I think this probably needs to be in a critical section? maybe all the bind functions do, but especially this one as we may get a race condition updating the handler in operation 🤔

MabezDev · 2024-01-31T14:34:31Z

By the way, the review comments I'm leaving are just that, don't feel the need to update this branch (unless you want to!); it's been very helpful so far for figuring out how this is going to look, so thanks for doing this!

MabezDev · 2024-02-09T10:30:16Z

esp-hal-procmacros/src/lib.rs

@@ -437,6 +437,66 @@ pub fn interrupt(args: TokenStream, input: TokenStream) -> TokenStream {
    .into()
 }

+#[cfg(feature = "interrupt")]
+#[proc_macro_attribute]
+pub fn handler(args: TokenStream, input: TokenStream) -> TokenStream {


Do we need this macro? Won't the type system i.e the set_interrupt_handler function how handle ensuring that the type is of extern "C" with no arguments?

Yes, this is just sugar to hide the extern "C"

MabezDev · 2024-02-09T10:35:16Z

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.

bjoernQ · 2024-02-09T10:58:14Z

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 that will still work the same with these changes currently (IIRC I tested esp-wifi with the changes).

Independently of this I am currently looking into different ways to implement esp-wifi's scheduler in a way that normal interrupts are "cheap" again (in the spirit of esp-rs/xtensa-lx#27 but even more stripped down). I had three ideas until now - I already checked two of them which are not really working, currently checking the third (most effort) idea and currently a fourth idea is growing in my head. I think having "lighter" interrupt handling for normal interrupts would really help especially for async on Xtensa

bjoernQ · 2024-03-05T14:14:10Z

Closing this. The real work happens in #1231

Runtime interrupt handler binding

8df4e5f

MabezDev reviewed Jan 31, 2024

View reviewed changes

MabezDev reviewed Feb 9, 2024

View reviewed changes

bjoernQ mentioned this pull request Mar 1, 2024

Runtime interrupt binding #1231

Merged

bjoernQ closed this Mar 5, 2024

bjoernQ deleted the explore-runtime-interrupt-binding branch November 26, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime interrupt handler binding #1121

Runtime interrupt handler binding #1121

bjoernQ commented Jan 26, 2024

bjoernQ commented Jan 30, 2024

bjoernQ commented Jan 31, 2024

MabezDev commented Jan 31, 2024

MabezDev Jan 31, 2024

MabezDev Jan 31, 2024

bjoernQ Jan 31, 2024

MabezDev Feb 20, 2024

MabezDev Jan 31, 2024

bjoernQ Jan 31, 2024

MabezDev Jan 31, 2024

MabezDev Jan 31, 2024

MabezDev commented Jan 31, 2024

MabezDev Feb 9, 2024

bjoernQ Feb 9, 2024

MabezDev commented Feb 9, 2024

bjoernQ commented Feb 9, 2024

bjoernQ commented Mar 5, 2024

Runtime interrupt handler binding #1121

Runtime interrupt handler binding #1121

Conversation

bjoernQ commented Jan 26, 2024

bjoernQ commented Jan 30, 2024

bjoernQ commented Jan 31, 2024

MabezDev commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MabezDev commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MabezDev commented Feb 9, 2024

bjoernQ commented Feb 9, 2024

bjoernQ commented Mar 5, 2024