Merge #451

451: Small critical-section-related fixes. r=adamgreig a=Dirbaio

See individual commit messages.

Co-authored-by: Dario Nieuwenhuis <[email protected]>

Author: bors[bot]

src/critical_section.rs

@@ -1,27 +1,22 @@
-#[cfg(all(cortex_m, feature = "critical-section-single-core"))]
-mod single_core_critical_section {
-    use critical_section::{set_impl, Impl, RawRestoreState};
+use critical_section::{set_impl, Impl, RawRestoreState};
 
-    use crate::interrupt;
-    use crate::register::primask;
+use crate::interrupt;
+use crate::register::primask;
 
-    struct SingleCoreCriticalSection;
-    set_impl!(SingleCoreCriticalSection);
+struct SingleCoreCriticalSection;
+set_impl!(SingleCoreCriticalSection);
 
-    unsafe impl Impl for SingleCoreCriticalSection {
-        unsafe fn acquire() -> RawRestoreState {
-            let was_active = primask::read().is_active();
-            interrupt::disable();
-            was_active
-        }
+unsafe impl Impl for SingleCoreCriticalSection {
+    unsafe fn acquire() -> RawRestoreState {
+        let was_active = primask::read().is_active();
+        interrupt::disable();
+        was_active
+    }
 
-        unsafe fn release(was_active: RawRestoreState) {
-            // Only re-enable interrupts if they were enabled before the critical section.
-            if was_active {
-                interrupt::enable()
-            }
+    unsafe fn release(was_active: RawRestoreState) {
+        // Only re-enable interrupts if they were enabled before the critical section.
+        if was_active {
+            interrupt::enable()
         }
     }
 }
-
-pub use critical_section::with;

src/lib.rs

@@ -9,6 +9,16 @@
 //!
 //! # Optional features
 //!
+//! ## `critical-section-single-core`
+//!
+//! This feature enables a [`critical-section`](https://github.com/rust-embedded/critical-section)
+//! implementation suitable for single-core targets, based on disabling interrupts globally.
+//!
+//! It is **unsound** to enable it on multi-core targets or for code running in unprivileged mode,
+//! and may cause functional problems in systems where some interrupts must be not be disabled
+//! or critical sections are managed as part of an RTOS. In these cases, you should use
+//! a target-specific implementation instead, typically provided by a HAL or RTOS crate.
+//!
 //! ## `cm7-r0p1`
 //!
 //! This feature enables workarounds for errata found on Cortex-M7 chips with revision r0p1. Some
@@ -49,10 +59,6 @@ mod macros;
 pub mod asm;
 #[cfg(armv8m)]
 pub mod cmse;
-// This is only public so the `singleton` macro does not require depending on
-// the `critical-section` crate separately.
-#[doc(hidden)]
-pub mod critical_section;
 pub mod delay;
 pub mod interrupt;
 #[cfg(all(not(armv6m), not(armv8m_base)))]
@@ -61,3 +67,13 @@ pub mod peripheral;
 pub mod register;
 
 pub use crate::peripheral::Peripherals;
+
+#[cfg(all(cortex_m, feature = "critical-section-single-core"))]
+mod critical_section;
+
+/// Used to reexport items for use in macros. Do not use directly.
+/// Not covered by semver guarantees.
+#[doc(hidden)]
+pub mod _export {
+    pub use critical_section;
+}

src/macros.rs

@@ -31,7 +31,10 @@ macro_rules! iprintln {
 /// at most once in the whole lifetime of the program.
 ///
 /// # Notes
-/// This macro is unsound on multi core systems.
+///
+/// This macro requires a `critical-section` implementation to be set. For most single core systems,
+/// you can enable the `critical-section-single-core` feature for this crate. For other systems, you
+/// have to provide one from elsewhere, typically your chip's HAL crate.
 ///
 /// For debuggability, you can set an explicit name for a singleton.  This name only shows up the
 /// the debugger and is not referencable from other code.  See example below.
@@ -62,7 +65,7 @@ macro_rules! iprintln {
 #[macro_export]
 macro_rules! singleton {
     ($name:ident: $ty:ty = $expr:expr) => {
-        $crate::critical_section::with(|_| {
+        $crate::_export::critical_section::with(|_| {
             // this is a tuple of a MaybeUninit and a bool because using an Option here is
             // problematic:  Due to niche-optimization, an Option could end up producing a non-zero
             // initializer value which would move the entire static from `.bss` into `.data`...