feat: use vectoring to jump straight to interruptN

[sethp]

Previously, the vector table was set up in a pseudo-direct mode where all interrupts were redirected to a single _start_trap handler regardless of number.

This change introduces 31 new _start_trapN targets that encode which interruptN function to jump to, at a cost of 180 bytes per function (~5kb total).

Not shown (because the linker scripts live in the HAL) are the big block of PROVIDES:

PROVIDE(_start_trap1 = default_start_trap1)
PROVIDE(_start_trap2 = default_start_trap2)
...
PROVIDE(_start_trap31 = default_start_trap31)

As a side benefit, this does enable overriding on a per-irq basis and could even allow someone to switch back to the current psuedo-direct mode if they so chose (though, I think they wouldn't see anything get GC'd yet).

Potential improvements include:

• Shrinking the saved register set to just being the callee-save parts of the C ABI, since only default_start_trap really has a use for the full frame
• Finer-grained control around which interrupts are vectored, and which are semi-direct: maybe we can get away with just three modes at the start (all direct, only 1-16 vectored, all vectored), but I'm curious if we could find a way to precisely express in a downstream crate "I want precisely 1, 2, 4..." to be vectored. Maybe a proc macro or other generator script?

[bjoernQ]

Maybe a macro to generate all those default_start_trapX would be nice

Shrinking the saved register set to just being the callee-save parts of the C ABI, since only default_start_trap really has a use for the full frame

We need the full trap frame in e.g. esp-wifi for the task scheduler (in a timer triggered interrupt). But maybe having this a feature would be an option

[onsdagens]

Nice! I've been working on essentially the same thing but wasn't 100% happy with how it looks, cool to have another set of eyes on it, and some discussion

What is your take on how the hardware -> CPU interrupt mapping should work? Currently the HAL will just shove all HW interrupts of prio x onto CPU interrupt x, which i guess works, but feels inefficient (you have to check which HW interrupt is actually triggered). I think a nice feature would be letting the user map interrupts as they see fit and define these interruptx CPU interrupt handlers themselves. In my case i'm working on a runtime specific to RTIC so i let the framework map the HW interrupts so no sharing of the CPU interrupts happens if possible (less than 31 enabled interrupts), and generate interrupt handlers automatically.(this maybe isn't too relevant for this change specifically but general interrupt flow).

[sethp]

Thanks for taking a look @bjoernQ and @onsdagens!

Maybe a macro to generate all those default_start_trapX would be nice

I tried a few things, but I wasn't super thrilled with any of them. Do you have an idea of what you'd want it to look like? Lacking the ability to evaluate e.g. n - 1 in a declarative macro it seemed like I'd have to list out all the interrupt numbers either way. It also looks like I'd have to pull in or crib from https://github.com/dtolnay/paste to glue the identifiers together, because std's concat_idents! isn't really up to the job.

We need the full trap frame in e.g. esp-wifi for the task scheduler (in a timer triggered interrupt). But maybe having this a feature would be an option

Oh, interesting: is it just the timer triggered interrupt that needs the whole frame? This change would allow overriding a specific entry point to pass whatever, but there's probably a lot we could do to make it easier.

What is your take on how the hardware -> CPU interrupt mapping should work?

I think we're of similar mind that priority -> irq num is a good broad default for a bunch of different cases, but offers the opportunity for a few improvements. Truth be told, I haven't gelled an idea yet of what I want to do, so hopefully you don't mind a bit of a brain dump.

<brain dump>

In addition to the points you raised about using the full suite of available hardware I've got some concerns about the shape of defined handlers. In our project, pretty much immediately @dougli1sqrd built us a way to register a closure so we could stop messing with statics quite so much, which was nice. Sadly, I had to pretty severely limit it as part of this work to only take function pointers and not full closures. The dynamically generated functions were getting emitted as part of .text, which was landing in flash and I couldn't see a way to have them end up in RAM instead.

There's also 4-5 pieces that need to line up just so for the handler to get called at all (interrupt::map, interrupt::set_priority, interrupt::enable_cpu_interrupt, having interuptN defined, and then whatever the source needs to generate the interrupt in the first place), and the lack of feedback when something goes wrong makes me wonder if there isn't a better way to express some of those constraints.

That's to say nothing of interupt::set_kind, which is actually a safety concern. Somewhere in my drafts I have a half-assed write up of an issue we ran into, but roughly: we were losing interrupts for the UART fifo because we'd read() them all down, and then in between receiving None and when we cleared the (edge) interrupt more bytes would be received so the edge never fired again. The upshot for this discussion is that whether something's an edge or level trigger means the handler (sometimes?) has to change to avoid racy behavior, but anyone can call interrupt::set_kind from anywhere at any time 🌶️

It does seem common to have the magic mapping of e.g. fn GPIO() -> GPIO interrupts in the embedded space, though. I don't have enough experience to understand where that comes from and what's particularly attractive about it; is it an arduino thing?

</brain dump>

So yeah. My plan is to try and push the vectoring a little further "down" closer to the user-defined handlers, which I expect will be informative to me about what the challenges are, since I don't think I really understand them yet. "Fence was torn down; other side contained tiger. 0 stars."

i let the framework map the HW interrupts so no sharing of the CPU interrupts happens if possible (less than 31 enabled interrupts), and generate interrupt handlers automatically

Oooh, that sounds neat: can you share a link? In messing around with the debug assist (which was really an exercise in interrupt handling too), I wrote a little baby assembler in an effort to get runtime-remappable vector slots so I could avoid having to clobber the whole interrupt table to change one target. But, alas, writes to flash memory don't exactly work the way writes to RAM do 🙃, so there was at least a partial hard dependency on moving the vector to RAM. Or, the changes in this PR would let me do what I needed much more readily by just overriding e.g. _start_trap31.

Thanks for getting the discussion rolling: I'm looking forward to collaborating more on this!

[onsdagens]

Finer-grained control around which interrupts are vectored, and which are semi-direct: maybe we can get away with just three modes at the start (all direct, only 1-16 vectored, all vectored), but I'm curious if we could find a way to precisely express in a downstream crate "I want precisely 1, 2, 4..." to be vectored. Maybe a proc macro or other generator script?

maybe weakly linked default_start_trapx() pointing to the "direct" handler could be something? letting the user hook the vector table directly by overriding them? not sure if it's possible, just brain dumping. would be really interested in something like this though since right now i've just kicked the can down the road and feature gated each of the CPU interrupts so i can have the linker not complain about missing symbols and work on RTIC directly instead in the meanwhile.

It does seem common to have the magic mapping of e.g. fn GPIO() -> GPIO interrupts in the embedded space, though. I don't have enough experience to understand where that comes from and what's particularly attractive about it; is it an arduino thing?

It sort of makes sense on a Cortex-M at least since HW interrupts are 1 to 1 with what actually happens on hardware level. In the case of ESP's though, you've got this arbitrary user defined mapping between HW interrupts and CPU interrupts, with the CPU interrupts being what is actually driving the interrupt flow on hardware level, meaning a GPIO interrupt doesn't always move you to the same vector table entry depending on implementation. So i think it would make sense to stray from convention and make everything more transparent in this case, at least as a feature.

Oooh, that sounds neat: can you share a link?

These generate handlers for software/hardware interrupts
https://github.com/onsdagens/rtic/blob/improved-rt/rtic-macros/src/codegen/async_dispatchers.rs
https://github.com/onsdagens/rtic/blob/improved-rt/rtic-macros/src/codegen/hardware_tasks.rs
by hooking bindings from here:
https://github.com/onsdagens/rtic/blob/improved-rt/rtic-macros/src/codegen/bindings/esp32c3.rs
the enabling and mapping of interrupts happens here:
https://github.com/onsdagens/rtic/blob/improved-rt/rtic-macros/src/codegen/bindings/esp32c3.rs#L74
it's all a bit ugly and hacky still, working PoC state. Also probably not applicable in your case depending on what you're trying to do since all of the vector table hooking happens at compile-time.

[onsdagens]

after some experimenting i've ended up with this https://github.com/onsdagens/esp32c3-rtic-rt/blob/main/src/lib.rs .
It doesn't cover corner cases yet (exception handling etc.), and it could look much neater, but the core idea with using weak linkage and letting the user override default behavior (the "direct" handler for example, or in this case just panic looping if a reasonable handler isn't provided) as they wish is there. let me know what you think

[sethp]

Oh, I like that: both paying far fewer bytes per function and keeping the whole layout thing out of linker scripts and in the esp-riscv-rt crate seem like wins to me.

I do think it trades off the hook-ability/code size a little bit though: e.g. esp-backtrace wants to override the DefaultExceptionHandler so that it can print a debug message which is pretty nice. I've been thinking about doing the same for DefaultHandler so that it can say "you forgot to override interrupt 27." I don't think that'd be possible with the Rust-only scheme, though. Right now how I think that works is that there's a PROVIDE(DefaultHandler = DefaultInterruptHandler) as well as a bunch of PROVIDE(interruptN = DefaultHandler), and a similar two-step lookup for the exception handling. That does two things:

1. If someone like esp-backtrace defines an ExceptionHandler or DefaultHandler directly, downstream consumers can still override interruptN without issue
2. All of the absent interruptNs don't actually exist, the ELF just has a bunch of pointers that get replaced with the single address of the DefaultHandler

I think the rust-level equivalent would be more like:

#[linkage = "weak"]
#[no_mangle] // or #[export_name = "..."]
extern "C" fn default_handler() { 
    loop {}
}

#[linkage = "weak"]
#[no_mangle]
extern "C" fn handler1() {
    default_handler()
}

which would recover the ability to override the default once. It'd still cost us 4*31 bytes or so over the linker script version when no handlers are defined. That looks like a cost baked in to the relative in-expressiveness of the rust linker model (per the discussion around this pre-RFC, which someone might be able to pick back up with our non-GNU example). I wonder how weak linkage interacts with PROVIDEs; could we have a situation where the linker script bits become optional as a way to save a few bytes? That seems like it'd be cool.

[onsdagens]

I wonder how weak linkage interacts with PROVIDEs; could we have a situation where the linker script bits become optional as a way to save a few bytes? That seems like it'd be cool.

I don't think i follow 😅

[sethp]

If we told the linker PROVIDE(handler1 = DefaultInterruptHandler) with a #[linkage = "weak"] definition for handler1, what would it do? If it prefers to use DefaultInterruptHandler in that case, then we'd save the 4 bytes for the jump instruction, and we'd be in a situation where not having (that part of) the linker script would still allow the program to build, it would just cost a few bytes at runtime.

At least to my aesthetic sense, well-scoped opt-in complexity like that which solves a concrete problem is very nice: it's a good way to learn about linker scripts and PROVIDEs, because anyone reaching for it knows exactly what they're looking for out of it.

It sort of makes sense on a Cortex-M at least since HW interrupts are 1 to 1 with what actually happens on hardware level. In the case of ESP's though, you've got this arbitrary user defined mapping between HW interrupts and CPU interrupts, with the CPU interrupts being what is actually driving the interrupt flow on hardware level, meaning a GPIO interrupt doesn't always move you to the same vector table entry depending on implementation. So i think it would make sense to stray from convention and make everything more transparent in this case, at least as a feature.

I've been musing on these truths that you provided, and I think you're exactly right: in our case, at least, the by-name GPIO binding isn't really helping us much, and I think if we made the cpu interrupt mapping explicit we could probably end up with a pretty good outcome. Especially if/when my extern "riscv-interrupt-m" PR gets merged, since then an application might reasonably be able to define its own interrupt handlers with no other support.

[onsdagens]

If we told the linker PROVIDE(handler1 = DefaultInterruptHandler) with a #[linkage = "weak"] definition for handler1, what would it do? If it prefers to use DefaultInterruptHandler in that case, then we'd save the 4 bytes for the jump instruction, and we'd be in a situation where not having (that part of) the linker script would still allow the program to build, it would just cost a few bytes at runtime.

At least to my aesthetic sense, well-scoped opt-in complexity like that which solves a concrete problem is very nice: it's a good way to learn about linker scripts and PROVIDEs, because anyone reaching for it knows exactly what they're looking for out of it.

Now i gotcha, i agree.

I've been musing on these truths that you provided, and I think you're exactly right: in our case, at least, the by-name GPIO binding isn't really helping us much, and I think if we made the cpu interrupt mapping explicit we could probably end up with a pretty good outcome. Especially if/when my extern "riscv-interrupt-m" PR gets merged, since then an application might reasonably be able to define its own interrupt handlers with no other support.

Not sure which PR you're referring to. I think this PR warrants something be done about the interrupt enable on HAL level as well, to make the mapping explicit as you said. Right now it's sort of arbitrary which makes sense if you're not hooking the vector table anyway.

How are you looking on time? I'd love to see this upstreamed somewhere in the near future if possible, since my RTIC port is pretty much merge ready, only thing missing is all-vanilla crates (only this one is patched at this stage). Otherwise, i'm of course willing to work on this as well.

Feel free to DM me on matrix if you want to discuss anything in real-time, i'm @\onsdag in the esp-rs room

[sethp]

The PR that'd open up some possibilities here is rust-lang/rust#111891 : then we might be able to delete everything related to interrupts from this crate except the vector itself (+/- some linker shenanigans). But, that'll probably take weeks if the rust-lang docs are to be believed.

So I'm thinking for the purposes of this PR the goal ought to be enabling the downstream work you & I are trying to do: to me, the shortest path to that sounds like getting the _vector_table set up with 32 individually override-able slots by way of some combination of assembly, weak linkage, and/or linker script shenanigans.

Then, once your generated handlers can successfully be slotted in to the table and I can skip over the whole handle_interrupts machinery for my real-time-bounded handler, we can start experimenting in the HAL with different options for expressing more natural mappings and making it easier to write non-default interrupt handlers. How does that sound to you?

[MabezDev]

That's to say nothing of interupt::set_kind, which is actually a safety concern. Somewhere in my drafts I have a half-assed write up of an issue we ran into, but roughly: we were losing interrupts for the UART fifo because we'd read() them all down, and then in between receiving None and when we cleared the (edge) interrupt more bytes would be received so the edge never fired again. The upshot for this discussion is that whether something's an edge or level trigger means the handler (sometimes?) has to change to avoid racy behavior, but anyone can call interrupt::set_kind from anywhere at any time hot_pepper

Is there a reason why you were using edge interrupts with UART? AFAIK, edge mode is only required for peripherals that don't support level based interrupts (i.e they hold there signal until cleared).

It does seem common to have the magic mapping of e.g. fn GPIO() -> GPIO interrupts in the embedded space, though. I don't have enough experience to understand where that comes from and what's particularly attractive about it; is it an arduino thing?

I suppose this most likely stems from the fact that ARM chips have an interrupt handler per peripheral (up to 256 or something like that), all of which are dispatched by the CPU directly. The ARM part is relevant as most of the rust-embedded early days were shaped around ARM (RISCV support was early stages + not much silicon around). In general though, when you're not trying to shave microsecond interrupt latencies down, it is very convenient to have peripheral based interrupt handlers. Before I added it in esp-rs/esp-hal#118, the interrupt examples we not so nice to read; they used the CPU handlers directly and then did register reads to figure out which peripheral bits needed clearing. It also makes writing async drivers much cleaner and simpler.

[MabezDev]

The PR that'd open up some possibilities here is rust-lang/rust#111891 : then we might be able to delete everything related to interrupts from this crate except the vector itself (+/- some linker shenanigans). But, that'll probably take weeks if the rust-lang docs are to be believed.

Firstly, this is awesome! I would love to see this land. I wasn't aware of the functionality in LLVM. This could be a huge perf win without costing much if at all on ergonomics/maintainability.

So I'm thinking for the purposes of this PR the goal ought to be enabling the downstream work you & I are trying to do: to me, the shortest path to that sounds like getting the _vector_table set up with 32 individually override-able slots by way of some combination of assembly, weak linkage, and/or linker script shenanigans.

This sounds good to me, I see you've already made the changes so I will test them next week :).

[onsdagens]

Small suggestion, but i really like the way this ended up looking for what it's worth.

[onsdagens]

Suggested change

	.weak _vector_table
	.weak _vector_table
	.type _vector_table, @function
	.option push
	.balign 0x100
	.option norelax
	.option norvc
	_vector_table:
	j _start_trap
	j _start_trap1
	j _start_trap2
	j _start_trap3
	j _start_trap4
	j _start_trap5
	j _start_trap6
	j _start_trap7
	j _start_trap8
	j _start_trap9
	j _start_trap10
	j _start_trap11
	j _start_trap12
	j _start_trap13
	j _start_trap14
	j _start_trap15
	j _start_trap16
	j _start_trap17
	j _start_trap18
	j _start_trap19
	j _start_trap20
	j _start_trap21
	j _start_trap22
	j _start_trap23
	j _start_trap24
	j _start_trap25
	j _start_trap26
	j _start_trap27
	j _start_trap28
	j _start_trap29
	j _start_trap30
	j _start_trap31

My suggestion is replacing the default vector table with one hooking the weakly linked handlers. This should come at no cost (since by default they just point to the generic handler anyway) and gives a much more powerful default implementation.

[onsdagens]

I wasn't able to tame the review functionality here, so the suggestion is just a gist.

[sethp]

Oh yeah, excellent point! I'll fix that right up.

[onsdagens]

Is nightly really still needed here?

[sethp]

Oh, huh: looks like it's not for clippy (this part). I lean towards keeping it, to line it up with the rustfmt and check builds that both use nightly.

[onsdagens]

I hadn't looked, in that case, i agree.

[onsdagens]

I'd also like to add a feature flag providing simple trap entry hooks so you don't have to provide a context store/restore for each of the overridden handlers, but that's probably irrelevant to what you've done here, so i'll do it in another PR i think.

[sethp]

Is there a reason why you were using edge interrupts with UART? AFAIK, edge mode is only required for peripherals that don't support level based interrupts (i.e they hold there signal until cleared).

Not an on-purpose reason, we got it from the example: https://github.com/esp-rs/esp-hal/blob/171e66e87ad994cc9f8004a42d7ea6461214c20e/esp32c3-hal/examples/serial_interrupts.rs#L73 . That example actually has the same safety issue: if more than 30 bytes come in over the UART between the .read() and the .reset_rx_fifo_full_interrupt() (which in this case only seems possible because of the writeln!s, for us it was a combination of a low threshold + a 400k baud input) then the edge interrupt gets cleared while the rx_fifo_full signal is high, so it never rises again.

Firstly, this is awesome! I would love to see this land. I wasn't aware of the functionality in LLVM. This could be a huge perf win without costing much if at all on ergonomics/maintainability.

Thanks! I was thrilled with how that turned out; I had stumbled across the interrupt clang attribute at some point, and it seems like pretty much a slam dunk for a lot of cases. I think it'll even allow some ergonomics wins for any handler that doesn't need the full trap frame (there's no provision for recovering that from LLVM that I can see).

And I appreciate the additional context around peripheral-based interrupt handlers: that PR and the async drivers sound like something I can learn a lot from checking out.

[sethp]

I'd also like to add a feature flag providing simple trap entry hooks so you don't have to provide a context store/restore for each of the overridden handlers, but that's probably irrelevant to what you've done here, so i'll do it in another PR i think.

Oh, sorry yeah I meant to look into that: I think it does make sense to do it separately, since you've got more context on it at this point. I'm definitely happy to review, though!

A feature flag sounds good, though if you wanted to avoid that I think you could try putting it in a .trap.* sub-section. After esp-rs/esp-hal#541 landed it should get GC'd if there are no references to it.

[MabezDev]

Took a bit longer to get around to testing this, sorry about that! LGTM, thanks!