feat: nested interrupts

[sethp]

A proof-of-concept for interrupt handler nesting. Running this and then mashing the "boot" button will produce ~arbitrarily deep (up to the point where the stack collides with the heap, probably) GPIO interrupts, that are themselves interrupt-able by the timer interrupts (and vice versa).

[sethp]

Still two things I want to do before integrating this pattern into the vgaterm bin proper:

• [ ] Write down the whole interrupt flow from hardware through #[interrupt] handler entry (to try and understand the layers of indirection/how the HAL is wired better)
• [x] Figure out whether we need to re-disable the interrupts before returning from our #[interrupt] handler; ~I think not, but~ yes, we really do (at least, before we start restoring the previous m* registers from the stack at the end of _start_trap)
  • [x] Also, as part of this, maybe think about copying over MPIE instead of just unconditionally setting MIE? Not sure how we'd end up in an interrupt handler with interrupts disabled globally, but turning them on when they're supposed to be off might legitimately lead to unexpected/unsafe behavior

[sethp]

The current state of this change sprawls across both this and the rustbox/esp-hal repo, and, it seems like the sum total of everything I tried is sufficient to restore our real-time bound for the pixel pushing. However, due to an unfortunate tooling incident (cargo & I had different ideas about which binary represented the "current state"), I have no idea what parts are necessary, and some are a lot easier to clean up than others.

The five things I've done (in order of effort/effectiveness) are:

1) priorities reworked so GPIO is the most important (priority = 15) 2) raising the interrupt threshold and re-enabling interrupts in uart0 isr (I just did uart0 priority +1 so it doesn’t matter when the interrupt bit is cleared) 3) no critical_section in uart0 (good ol’ static mut rides again) 4) overrides of the HAL irq machinery to jump “straight” (more or less) to the UART0 / GPIO handlers (see riscv.rs: https://github.com/rustbox/esp-hal/pull/1/commits/9b2129f8636fbb2fd037d8266c7ab7dfdfbdbb84#diff-bfd432d6d9e7adbaa68d22bca09323dec3f95e1101ca0816d86fe75c0bae4f27 ) 5) aggressive inlining/RAM pinning so that every part of the GPIO handler through to the lowest level of the DMA impl is in SRAM (upstream, largely in spi.rs : https://github.com/rustbox/esp-hal/pull/1/commits/9b2129f8636fbb2fd037d8266c7ab7dfdfbdbb84#diff-13231643cd8aa34e91c614e3f5280962f1e3f2c53a8fa3012786d95865eaf3e6 )

One is a gimmie, but it's probably not sufficient on its own (and I think I even tested that one properly to confirm). Two might be sufficient with or without three (but I don't have a meaningful test result either way).

If it's not, that's when we get into the rougher territory of four and five: both make the frame request edge -> SPI start latency lower and more predictable, but with significantly sharper tradeoffs.

So I think the next steps are:

• [x] back out 4 & 5 and re-test
• [x] ~try with & without 3~ we're good with it

If 4 & 5 turn out to be unnecessary, I suppose we can just keep them in our back pocket for later (if that ever arrives). Otherwise, we're gonna have to put some Work into them.

Either way, this PR feels like as good a place as any to hash out the implementation details of 1-3, as I'm nearly certain all of them are here to stay.

[dougli1sqrd]

For point 2 I'm not really seeing where you raise the interrupt threshold to +1 of uart?

[sethp]

Ah, that’s simply because it’s not there. I’ll take a look when I’m back at my computer, must’ve forgotten something

[sethp]

ta-da: https://github.com/rustbox/esp32c3-vgaterm/blob/308663a8ba6670c715bd1c8c6408c77ccce38271/src/uart.rs#L168-L171

Also, fun fact, this is a terrible idea and unsound: if the current priority is 15, we can't set it to 16, and trying will either trigger an exception when we write a 1 to a reserved bit (ideally) or silently drop the 4th bit and set the threshold back to 0. It works here because we "know" the UART0 is never going to be priority 15, but we'd want to do something different if we generalized it.

It irks me that there's no unsafe marker there, and/or no other way to flag that we're actually dealing with a u4, not a u8; not sure there's anything to be done about it, but try and corral a group of our neurons into sending warning signals when we see (a + 1) & 0b1111 :shrug:

[dougli1sqrd]

What if we had a const that declared what priority the uart should be, and maybe even like assert or something that the priority is what we expect? And then we can increase by 1? That at least would help document that the priority should be such and such

On Fri, Mar 3, 2023, 5:34 PM sethp @.***> wrote:

ta-da: https://github.com/rustbox/esp32c3-vgaterm/blob/308663a8ba6670c715bd1c8c6408c77ccce38271/src/uart.rs#L168-L171

Also, fun fact, this is a terrible idea and unsound: if the current priority is 15, we can't set it to 16, and trying will either trigger an exception when we write a 1 to a reserved bit (ideally) or silently drop the 4th bit and set the threshold back to 0. It works here because we "know" the UART0 is never going to be priority 15, but we'd want to do something different if we generalized it.

It irks me that there's no unsafe marker there, and/or no other way to flag that we're actually dealing with a u4, not a u8; not sure there's anything to be done about it, but try and corral a group of our neurons into sending warning signals when we see (a + 1) & 0b1111 🤷

— Reply to this email directly, view it on GitHub https://github.com/rustbox/esp32c3-vgaterm/pull/15#issuecomment-1454326371, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK3GW7ELTD7B7O5GYCGSP3W2KL2LANCNFSM6AAAAAAVAYSRIE . You are receiving this because you commented.Message ID: @.***>

[sethp]

Yeah, that's definitely a thing we could do[^1]: we could check that the uart priorities are less than 15, and/or that our constants line up in the right way (i.e. the target threshold is strictly greater than the priority).

[^1]: Maybe with https://crates.io/crates/static_assertions, either using it directly, or just crib the underlying idea which is to wrap the assertion in a const fn and then declare either a zero- or negatively sized array based on whether the property is true or not, respectively.

[sethp]

• [x] consider how to name the priority levels assigned to different components (consts, re-export, etc.): extracted to https://github.com/rustbox/esp32c3-vgaterm/issues/22
• [x] ~maybe consider what a safe-ish interface to expose around threshold_mask would be?~ broken out into https://github.com/rustbox/esp32c3-vgaterm/issues/20

So, testing notes: 1-3 are definitely required, and some amount of 5 (though, it seems, thankfully, not 4).

Next steps for me are to keep digging into the esp-hal changes to see what impact each of them has; that can be independent of this PR, thankfully. Probably I will tease apart the update branch I have from the actual perf changes, and then open a PR against rustbox/esp-hal to discuss those. Notably worthy of discussion over there: how we can maintain the recursive tree of RAM'd/inlining?

[sethp]

consider how to name the priority levels assigned to different components (consts, re-export, etc.)

It seems to me like the goals ought to be:

• collecting all the named priorities in a single place, so we can easily answer e.g. "is UART0 or UART1 going to take precedence?"
• making it very easy to re-assign priorities, e.g. "let's raise UART0's priority above UART1's" without violating safety guarantees
  • what might go wrong here? do we have an example change in mind?
• nice to have: the ability to reshape (ideally, transparently to the other modules) the priority system to introduce e.g. fn masking_threshold for a priority (i.e. the next enum element)

what do you think?

[sethp]

Ready for review! I decided to give up on what I just extracted to #22 for now; mechanically, I'm happy to throw some pub use or consts around, I'm just not sure what goal I'd be supporting (your sense of aesthetics is a valid answer, I just need more of your guidance to do it).

Independently, I'm still working on learning about / documenting the interrupt flow, but I'll open a new PR for that one once we get this merged.