Merge pull request #943 from driveraid/smp

defmt-semihosting: use critical_section for synchronization.

Author: jonathanpallant

CHANGELOG.md

@@ -705,6 +705,8 @@ Initial release
 
 ### [defmt-semihosting-next]
 
+* [#943] use critical_section for synchronization.
+
 ### [defmt-semihosting-v0.1.0] (2024-11-27)
 
 Initial release

firmware/defmt-semihosting/Cargo.toml

@@ -14,3 +14,7 @@ version = "0.1.0"
 defmt = { version = "0.3", path = "../../defmt" }
 cortex-m = "0.7"
 cortex-m-semihosting = "0.5"
+critical-section = "1.2"
+
+[features]
+critical-section-single-core = ["cortex-m/critical-section-single-core"]

firmware/defmt-semihosting/src/lib.rs

@@ -4,6 +4,19 @@
 //!
 //! WARNING using `cortex_m_semihosting`'s `hprintln!` macro or `HStdout` API will corrupt `defmt`
 //! log frames so don't use those APIs.
+//!
+//! # Critical section implementation
+//!
+//! This crate uses [`critical-section`](https://github.com/rust-embedded/critical-section) to ensure only one thread
+//! is writing to the buffer at a time. You must import a crate that provides a `critical-section` implementation
+//! suitable for the current target. See the `critical-section` README for details.
+//!
+//! For example, for single-core privileged-mode Cortex-M targets, you can add the following to your Cargo.toml.
+//!
+//! ```toml
+//! [dependencies]
+//! cortex-m = { version = "0.7.6", features = ["critical-section-single-core"]}
+//! ```
 
 #![no_std]
 // nightly warns about static_mut_refs, but 1.76 (our MSRV) does not know about
@@ -12,20 +25,19 @@
 
 use core::sync::atomic::{AtomicBool, Ordering};
 
-use cortex_m::{interrupt, register};
 use cortex_m_semihosting::hio;
 
 #[defmt::global_logger]
 struct Logger;
 
 static TAKEN: AtomicBool = AtomicBool::new(false);
-static INTERRUPTS_ACTIVE: AtomicBool = AtomicBool::new(false);
+static mut CS_RESTORE: critical_section::RestoreState = critical_section::RestoreState::invalid();
 static mut ENCODER: defmt::Encoder = defmt::Encoder::new();
 
 unsafe impl defmt::Logger for Logger {
     fn acquire() {
-        let primask = register::primask::read();
-        interrupt::disable();
+        // safety: Must be paired with corresponding call to release(), see below
+        let restore = unsafe { critical_section::acquire() };
 
         if TAKEN.load(Ordering::Relaxed) {
             panic!("defmt logger taken reentrantly")
@@ -34,7 +46,8 @@ unsafe impl defmt::Logger for Logger {
         // no need for CAS because interrupts are disabled
         TAKEN.store(true, Ordering::Relaxed);
 
-        INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
+        // safety: accessing the `static mut` is OK because we have acquired a critical section.
+        unsafe { CS_RESTORE = restore };
 
         // safety: accessing the `static mut` is OK because we have disabled interrupts.
         unsafe { ENCODER.start_frame(do_write) }
@@ -52,10 +65,12 @@ unsafe impl defmt::Logger for Logger {
         ENCODER.end_frame(do_write);
 
         TAKEN.store(false, Ordering::Relaxed);
-        if INTERRUPTS_ACTIVE.load(Ordering::Relaxed) {
-            // re-enable interrupts
-            interrupt::enable()
-        }
+
+        // safety: accessing the `static mut` is OK because we have acquired a critical section.
+        let restore = unsafe { CS_RESTORE };
+
+        // safety: Must be paired with corresponding call to acquire(), see above
+        unsafe { critical_section::release(restore) };
     }
 
     unsafe fn write(bytes: &[u8]) {

firmware/qemu/Cargo.toml

@@ -14,7 +14,7 @@ harness = false
 defmt = { path = "../../defmt" }
 defmt-semihosting = { path = "../defmt-semihosting" }
 defmt-test = { path = "../defmt-test" }
-cortex-m = "0.7"
+cortex-m = { version = "0.7", features = ["critical-section-single-core"] }
 cortex-m-rt = "0.7"
 cortex-m-semihosting = "0.5"
 alloc-cortex-m = { version = "0.4", optional = true }