cortex-m
cortex-m copied to clipboard
rust-embedded
Fix `interrupt::free`.
Don't pass CricialSection to closure passed to interrupt::free.
Depends on https://github.com/embassy-rs/critical-section/pull/13.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @adamgreig (or someone else) soon.
Please see the contribution instructions for more information.
Thanks for the PR! We discussed this a bit in last week's meeting here; the trouble is basically that interrupt::free() doesn't meet the guarantees required by CriticalSection on multi-core systems or systems executing in unprivileged mode. Ultimately the solution is probably to move towards something like the critical-section crate (possibly integrating that into bare-metal). In the meantime for cortex-m 0.8, it might be best to actually have interrupt::free() not give the closure a CriticalSection token at all, and just execute the closure in the context of disabled interrupts. That's still useful, but it wouldn't permit safe access to a Mutex any longer, so we'd want something else in place before releasing 0.8.
I guess in this case we need three things:
interrupt::freewhich does not pass abare_metal::CriticalSectionsection.- Implementation of
critical_section::CriticalSectionfor single and multi-core. - Some function which does pass a
bare_metal::CriticalSectionto a closure so we can actually lock abare_metal::Mutex.
As far as I am aware, you can’t implement it for multi-core devices using only the standard Cortex-M peripherals. For example, on the Raspberry Silicon RP2040 you need to use the Spinlock registers in the SIO peripheral.
I also don’t think you could even know you were on a multi-core system using only the standard Cortex-M peripherals.
The critical_section::CriticalSection implementation would have to be an opt-in feature anyways.
I think it makes sense to then have a HAL either provide this implementation or opt into the single-core version in this crate.
I think the HALs should provide an implementation. I’m unsure whether it’s ok for them to re-export one from here. Perhaps if it’s behind a feature flag, or in a macro, it’s OK.
I’m unsure whether it’s ok for them to re-export one from here.
Not sure what you mean by re-export here. The HAL (for a single-core chip) will then simply activate the cortex-m/singlecore feature.
Some function which does pass a
bare_metal::CriticalSectionto a closure so we can actually lock abare_metal::Mutex.
Ah, critical_section::CriticalSection is a re-export of bare_metal::CriticalSection, I though it would have to be the other way around. So critical_section::with already allows this.
I've released critical-section 1.0.0-alpha.1. Unless concerns are found, I think it's likely this'll be the final 1.0 design.
