Remove register_interrupt_flags and PeripheralInterruptFlags from the interpreter and friends

[rrbutani]

what

Background

One of our more debated early design decisions was the question of how to support interrupts from peripherals in the interpreter.

We went through a few different designs:

• interpreter provided callback functions that peripherals would invoke
• callback functions that device implementations would, at runtime patch into the interrupt vector table as appropriate (this had many issues)
• callback functions that peripheral impls would store in global mutable state and then call from static interrupt service routines
• until, ultimately, we settled on using AtomicBools to represent interrupt flags

The initial motivation for using a callback based model for interrupts was performance but we quickly ran into issues getting such an approach to work in a no_std friendly way (for example, having your callback push into an event queue that the interpreter drains works fine on hosted platforms but is tricky on embedded for the usual reasons: synchronization, allocation, etc.) and decided that we'd worry about performance if it provided to be an issue.

From there we pivoted to a polling based approach but did so in a somewhat roundabout manner:

• we added functions to each interrupt-supporting peripheral trait for interrupt_occurred/reset_interrupt (good, not tying peripherals to a particular representation)
• we added a register_interrupt_flags function to each peripheral trait (well intentioned but weird)
  • the idea was that the interpreter would own these flags and pass them down unto the peripherals, like it did with the callbacks
  • but... the interpreter wouldn't actually read the flags itself but would instead call methods like interrupt_occurred
• we realized that we'd need the flags to be 'static in cases where peripheral implementations pass them to static interrupt service routines so.. we made the Interpreter generic over some lifetime ('int) representing the flags and allowed it to be passed into the Interpreter so the entire interpreter wouldn't need to be 'static and thus (probably) const-constructible
  • we also came up with a few abstractions for still supporting "owned" flags that did not work
  • this was the really bad bit because as part of this we ended up "infecting" lots of the codebase with the 'int lifetime parameter

A Better Solution: Removing register_interrupt_flags

With the benefit of hindsight, some of the discrepancies in the design we have today are apparent. Whether or not we need to invest in a more performant approach to handling interrupts than polling is still an open question, but I think we have more than enough evidence that it's possible to simplify the design we currently have.

Additional peripheral trait implementations like the shims and the embedded-hal based GPIO implementation have shown that mandating the use of AtomicBools to represent and synchronize interrupt state only serves to burden implementations that do not need synchronization with an ISR while not aiding implementations that do.

All of this leads me to believe that the interpreter should not have an opinion about how interrupt state is stored or synchronized and that this should be entirely under the purview of individual peripheral trait implementations. This is effectively the paradigm we already have with the interrupt_occurred/reset_interrupt_flag methods on interrupt supporting traits.

It's also worth noting that the one use case that all of the PeripheralInterruptFlags machinery was designed for (device implementations of the peripheral traits whose only method of knowing an interrupt has fired in an ISR invocation) is also better served by a separation of the PeripheralInterruptFlags from the interpreter: under the current system device implementors would have to take care that the interpreter was passed the same set of flags that hardcoded ISRs were written to set.

The many upsides include:

• removing invalid states that the peripheral implementations have to handle (i.e. other methods called before register_interrupt_flags, unwrapping an Option<_> of flags, etc.)
• simplifying the peripheral traits a little bit, fewer functions to implement
• and most significantly: removing the 'int lifetime from many many places throughout the codebase, making many APIs easier to use

steps

• remove register_interrupt_flags from the interrupt support peripheral traits
• remove PeripheralInterruptFlags from the interpreter and it's builder
• remove the 'int lifetime from the codebase
• move PeripheralInterruptFlags to device-support

core/traits/src/peripherals/gpio.rs

Line 266 in 5e0f0eb

fn register_interrupt_flags(&mut self, flags: &'a GpioPinArr<AtomicBool>);

core/traits/src/peripherals/timers.rs

Line 257 in 5e0f0eb

fn register_interrupt_flags(&mut self, flags: &'a TimerArr<AtomicBool>);

core/traits/src/peripherals/input.rs

Line 23 in 5e0f0eb

fn register_interrupt_flag(&mut self, flag: &'a AtomicBool);

core/traits/src/peripherals/output.rs

Line 18 in 5e0f0eb

fn register_interrupt_flag(&mut self, flag: &'a AtomicBool);

open questions