Have interrupt handlers pass in CriticalSection token to share resources more efficiently

[YuhanLiin]

When entering an ISR in MSP430, interrupts will always be disabled. As such, accessing global resources such as a bare-metal mutex can be done safely without calling interrupt::free, which is required right now. I suggest that the [interrupt] macro be modified to have the signature fn ISR(cs: CriticalSection) so mutexes can be accessed without additional overhead. Using the cs token after calling interrupt:enable is unsound, but enable is unsafe already. Applications that achieve concurrency without using cs, such as RTFM, can simply ignore the token.

[cr1901]

Yes, I want to do this as well/agree with it in principle.

Sometimes however, you may want to reenable interrupts within the ISR. There is currently no way to do this with safe code. The rationale for making interrupt::enable() unsafe is that if you call it within a critical section, you violate the invariants of interrupt::free().

I think there should be a separate token type call it InInterruptSection, and both InInterruptSection and CriticalSection implement a trait called InterruptsDisabled. interrupt::free can be parameterized over the InterruptsDisabled trait and since IIRC interrupt::free is always inlined, code duplication of monomorphization doesn't matter.

Then we could create a new function: interrupt::enable_nested(cs: InInterruptSection) that's safe to call within an interrupt. Thoughts?

[YuhanLiin]

If we parametrize interrupt::free over InterruptDisabled trait wouldn't that require existing code to add type annotations to closures in the form of interrupt::free(|cs: &ConcreteInterruptSectionType| ...) to avoid ambiguity? We can also pass a cs token into main, since MSP430 starts with interrupts disabled. This way we can use an API like enable_nested(cs) everywhere (perhaps even have it replace interrupt::enable). This also eliminates the need for multiple critical section types.

[cr1901]

If we parametrize interrupt::free over InterruptDisabled trait wouldn't that require existing code to add type annotations to closures in the form of interrupt::free(|cs: &ConcreteInterruptSectionType| ...) to avoid ambiguity?

Oh crap. Indeed, that's a problem.

We can also pass a cs token into main, since MSP430 starts with interrupts disabled. This way we can use an API like enable_nested(cs) everywhere (perhaps even have it replace interrupt::enable). This also eliminates the need for multiple critical section types.

I wouldn't outright replace enable, just in case code that's trying to create an alternate safe abstraction needs it. However, consuming cs probably means that enable interrupts is safe inside a critical section, because without an alternate safe abstraction one can't do (safe!) I/O within a critical section after the interrupts are enabled.

Would like some more eyes on this though (any chance you could idle in Matrix again?)...

[YuhanLiin]

With the currently proposed API you actually can't enable interrupts safely inside a critical section because the closure passes in a &CriticalSection, which can't be consumed. I'm actually fine with that because interrupt::free restores previous interrupt settings on exit, so it could disable interrupts implicitly after enabling them inside and cause confusion.

I'm idle on Matrix right now so we can talk more.

[YuhanLiin]

Closed by #10