Repository: knurling-rs/defmt
Pull request #505 (2021-06-11 12:56:37)

[1/n] - Logger trait v2.
Part 1 of N of #492 

- Panics on reentrant acquire.
- Remove `Write` trait.


Patch:
diff --git a/firmware/defmt-itm/src/lib.rs b/firmware/defmt-itm/src/lib.rs
index 8003fed..1f96fef 100644
--- a/firmware/defmt-itm/src/lib.rs
+++ b/firmware/defmt-itm/src/lib.rs
@@ -14,10 +14,7 @@
 #![doc(html_logo_url = "https://knurling.ferrous-systems.com/knurling_logo_light_text.svg")]
 #![no_std]
 
-use core::{
-    ptr::NonNull,
-    sync::atomic::{AtomicBool, Ordering},
-};
+use core::sync::atomic::{AtomicBool, Ordering};
 
 use cortex_m::{interrupt, itm, peripheral::ITM, register};
 
@@ -41,46 +38,39 @@ pub fn enable(itm: ITM) {
 #[defmt::global_logger]
 struct Logger;
 
-impl defmt::Write for Logger {
-    fn write(&mut self, bytes: &[u8]) {
-        // NOTE(unsafe) this function will be invoked *after* `enable` has run so this crate now has
-        // ownership over the ITM thus it's OK to instantiate the ITM register block here
-        unsafe { itm::write_all(&mut (*ITM::ptr()).stim[0], bytes) }
-    }
-}
-
 static TAKEN: AtomicBool = AtomicBool::new(false);
 static INTERRUPTS_ACTIVE: AtomicBool = AtomicBool::new(false);
 
 unsafe impl defmt::Logger for Logger {
-    fn acquire() -> Option<NonNull<dyn defmt::Write>> {
+    fn acquire() {
         if !ENABLED.load(Ordering::Relaxed) {
-            return None;
+            panic!("defmt ITM logger is not enabled")
         }
 
         let primask = register::primask::read();
         interrupt::disable();
-        if !TAKEN.load(Ordering::Relaxed) {
-            // no need for CAS because interrupts are disabled
-            TAKEN.store(true, Ordering::Relaxed);
-
-            INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
 
-            Some(NonNull::from(&Logger as &dyn defmt::Write))
-        } else {
-            if primask.is_active() {
-                // re-enable interrupts
-                unsafe { interrupt::enable() }
-            }
-            None
+        if TAKEN.load(Ordering::Relaxed) {
+            panic!("defmt logger taken reentrantly")
         }
+
+        // no need for CAS because interrupts are disabled
+        TAKEN.store(true, Ordering::Relaxed);
+
+        INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
     }
 
-    unsafe fn release(_: NonNull<dyn defmt::Write>) {
+    unsafe fn release() {
         TAKEN.store(false, Ordering::Relaxed);
         if INTERRUPTS_ACTIVE.load(Ordering::Relaxed) {
             // re-enable interrupts
             interrupt::enable()
         }
     }
+
+    unsafe fn write(bytes: &[u8]) {
+        // NOTE(unsafe) this function will be invoked *after* `enable` has run so this crate now has
+        // ownership over the ITM thus it's OK to instantiate the ITM register block here
+        itm::write_all(&mut (*ITM::ptr()).stim[0], bytes)
+    }
 }
diff --git a/firmware/defmt-rtt/src/lib.rs b/firmware/defmt-rtt/src/lib.rs
index 81c821e..d4a58e5 100644
--- a/firmware/defmt-rtt/src/lib.rs
+++ b/firmware/defmt-rtt/src/lib.rs
@@ -12,7 +12,7 @@
 #![no_std]
 
 use core::{
-    ptr::{self, NonNull},
+    ptr,
     sync::atomic::{AtomicBool, AtomicUsize, Ordering},
 };
 
@@ -25,42 +25,35 @@ const SIZE: usize = 1024;
 #[defmt::global_logger]
 struct Logger;
 
-impl defmt::Write for Logger {
-    fn write(&mut self, bytes: &[u8]) {
-        unsafe { handle().write_all(bytes) }
-    }
-}
-
 static TAKEN: AtomicBool = AtomicBool::new(false);
 static INTERRUPTS_ACTIVE: AtomicBool = AtomicBool::new(false);
 
 unsafe impl defmt::Logger for Logger {
-    fn acquire() -> Option<NonNull<dyn defmt::Write>> {
+    fn acquire() {
         let primask = register::primask::read();
         interrupt::disable();
-        if !TAKEN.load(Ordering::Relaxed) {
-            // no need for CAS because interrupts are disabled
-            TAKEN.store(true, Ordering::Relaxed);
 
-            INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
-
-            Some(NonNull::from(&Logger as &dyn defmt::Write))
-        } else {
-            if primask.is_active() {
-                // re-enable interrupts
-                unsafe { interrupt::enable() }
-            }
-            None
+        if TAKEN.load(Ordering::Relaxed) {
+            panic!("defmt logger taken reentrantly")
         }
+
+        // no need for CAS because interrupts are disabled
+        TAKEN.store(true, Ordering::Relaxed);
+
+        INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
     }
 
-    unsafe fn release(_: NonNull<dyn defmt::Write>) {
+    unsafe fn release() {
         TAKEN.store(false, Ordering::Relaxed);
         if INTERRUPTS_ACTIVE.load(Ordering::Relaxed) {
             // re-enable interrupts
             interrupt::enable()
         }
     }
+
+    unsafe fn write(bytes: &[u8]) {
+        handle().write_all(bytes)
+    }
 }
 
 #[repr(C)]
diff --git a/firmware/defmt-semihosting/src/lib.rs b/firmware/defmt-semihosting/src/lib.rs
index a8818c1..3816db2 100644
--- a/firmware/defmt-semihosting/src/lib.rs
+++ b/firmware/defmt-semihosting/src/lib.rs
@@ -7,10 +7,7 @@
 
 #![no_std]
 
-use core::{
-    ptr::NonNull,
-    sync::atomic::{AtomicBool, Ordering},
-};
+use core::sync::atomic::{AtomicBool, Ordering};
 
 use cortex_m::{interrupt, register};
 use cortex_m_semihosting::hio;
@@ -18,46 +15,37 @@ use cortex_m_semihosting::hio;
 #[defmt::global_logger]
 struct Logger;
 
-impl defmt::Write for Logger {
-    fn write(&mut self, bytes: &[u8]) {
-        // using QEMU; it shouldn't mind us opening several handles (I hope)
-        if let Ok(mut hstdout) = hio::hstdout() {
-            hstdout.write_all(bytes).ok();
-        }
-    }
-}
-
 static TAKEN: AtomicBool = AtomicBool::new(false);
 static INTERRUPTS_ACTIVE: AtomicBool = AtomicBool::new(false);
 
 unsafe impl defmt::Logger for Logger {
-    fn acquire() -> Option<NonNull<dyn defmt::Write>> {
+    fn acquire() {
         let primask = register::primask::read();
         interrupt::disable();
 
-        if !TAKEN.load(Ordering::Relaxed) {
-            // NOTE(no-CAS) interrupts are disabled
-            TAKEN.store(true, Ordering::Relaxed);
 
-            INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
-
-            Some(NonNull::from(&Logger as &dyn defmt::Write))
-        } else {
-            if primask.is_active() {
-                // re-enable interrupts
-                unsafe { interrupt::enable() }
-            }
-            None
+        if TAKEN.load(Ordering::Relaxed) {
+            panic!("defmt logger taken reentrantly")
         }
+
+        // no need for CAS because interrupts are disabled
+        TAKEN.store(true, Ordering::Relaxed);
+
+        INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
     }
 
-    unsafe fn release(_: NonNull<dyn defmt::Write>) {
-        // NOTE(no-CAS) interrupts still disabled
+    unsafe fn release() {
         TAKEN.store(false, Ordering::Relaxed);
-
         if INTERRUPTS_ACTIVE.load(Ordering::Relaxed) {
             // re-enable interrupts
             interrupt::enable()
         }
     }
+
+    unsafe fn write(bytes: &[u8]) {
+        // using QEMU; it shouldn't mind us opening several handles (I hope)
+        if let Ok(mut hstdout) = hio::hstdout() {
+            hstdout.write_all(bytes).ok();
+        }
+    }
 }
diff --git a/macros/src/lib.rs b/macros/src/lib.rs
index 30fe894..d400d21 100644
--- a/macros/src/lib.rs
+++ b/macros/src/lib.rs
@@ -77,13 +77,18 @@ pub fn global_logger(args: TokenStream, input: TokenStream) -> TokenStream {
         #vis struct #ident;
 
         #[no_mangle]
-        unsafe fn _defmt_acquire() -> Option<defmt::InternalFormatter> {
-            <#ident as defmt::Logger>::acquire().map(|nn| defmt::InternalFormatter::from_raw(nn))
+        unsafe fn _defmt_acquire()  {
+            <#ident as defmt::Logger>::acquire()
         }
 
         #[no_mangle]
-        unsafe fn _defmt_release(f: defmt::InternalFormatter)  {
-            <#ident as defmt::Logger>::release(f.into_raw())
+        unsafe fn _defmt_release()  {
+            <#ident as defmt::Logger>::release()
+        }
+
+        #[no_mangle]
+        unsafe fn _defmt_write(bytes: &[u8])  {
+            <#ident as defmt::Logger>::write(bytes)
         }
     )
     .into()
@@ -514,12 +519,12 @@ fn log(level: Level, log: FormatArgs) -> TokenStream2 {
         #[cfg(#logging_enabled)] {
             match (#(&(#args)),*) {
                 (#(#pats),*) => {
-                    if let Some(mut _fmt_) = defmt::export::acquire() {
-                        _fmt_.header(&defmt::export::istr(#sym));
-                        #(#exprs;)*
-                        _fmt_.finalize();
-                        defmt::export::release(_fmt_)
-                    }
+                    defmt::export::acquire();
+                    let mut _fmt_ = defmt::InternalFormatter::new();
+                    _fmt_.header(&defmt::export::istr(#sym));
+                    #(#exprs;)*
+                    _fmt_.finalize();
+                    defmt::export::release()
                 }
             }
         }
diff --git a/src/export.rs b/src/export.rs
index 354a820..e1f16d2 100644
--- a/src/export.rs
+++ b/src/export.rs
@@ -1,4 +1,4 @@
-use crate::{InternalFormatter, Str};
+use crate::Str;
 
 #[cfg(feature = "unstable-test")]
 thread_local! {
@@ -23,29 +23,36 @@ pub fn fetch_add_string_index() -> usize {
 }
 
 #[cfg(feature = "unstable-test")]
-pub fn acquire() -> Option<InternalFormatter> {
-    None
-}
+pub fn acquire() {}
 
 #[cfg(not(feature = "unstable-test"))]
 #[inline(never)]
-pub fn acquire() -> Option<InternalFormatter> {
+pub fn acquire() {
     extern "Rust" {
-        fn _defmt_acquire() -> Option<InternalFormatter>;
+        fn _defmt_acquire();
     }
     unsafe { _defmt_acquire() }
 }
 
 #[cfg(feature = "unstable-test")]
-pub fn release(_: InternalFormatter) {}
+pub fn release() {}
+
+#[cfg(not(feature = "unstable-test"))]
+#[inline(never)]
+pub fn release() {
+    extern "Rust" {
+        fn _defmt_release();
+    }
+    unsafe { _defmt_release() }
+}
 
 #[cfg(not(feature = "unstable-test"))]
 #[inline(never)]
-pub fn release(fmt: InternalFormatter) {
+pub fn write(bytes: &[u8]) {
     extern "Rust" {
-        fn _defmt_release(fmt: InternalFormatter);
+        fn _defmt_write(bytes: &[u8]);
     }
-    unsafe { _defmt_release(fmt) }
+    unsafe { _defmt_write(bytes) }
 }
 
 /// For testing purposes
diff --git a/src/formatter.rs b/src/formatter.rs
index 9bf69e6..e545bcb 100644
--- a/src/formatter.rs
+++ b/src/formatter.rs
@@ -1,9 +1,6 @@
-use core::{
-    fmt::{self, Write as _},
-    ptr::NonNull,
-};
+use core::fmt::{self, Write as _};
 
-use crate::{export, leb, Format, Write};
+use crate::{export, leb, Format};
 
 /// The maximum number of booleans that can be compressed together
 const MAX_NUM_BOOL_FLAGS: u8 = 8;
@@ -17,8 +14,6 @@ pub struct Formatter<'a> {
 
 #[doc(hidden)]
 pub struct InternalFormatter {
-    #[cfg(not(feature = "unstable-test"))]
-    writer: NonNull<dyn Write>,
     #[cfg(feature = "unstable-test")]
     bytes: Vec<u8>,
     /// The current group of consecutive bools
@@ -36,32 +31,24 @@ pub struct InternalFormatter {
 impl InternalFormatter {
     #[cfg(not(feature = "unstable-test"))]
     pub fn write(&mut self, bytes: &[u8]) {
-        unsafe { self.writer.as_mut().write(bytes) }
+        export::write(bytes)
     }
 
     /// Implementation detail
     ///
     /// # Safety
     ///
-    /// `writer` is `Copy` but the returned type is a singleton. Calling this function should not
-    /// break the singleton invariant (one should not create more than one instance of
-    /// `InternalFormatter`)
+    /// Must only be called when the current execution context has acquired the logger with [Logger::acquire]
     #[cfg(not(feature = "unstable-test"))]
-    pub unsafe fn from_raw(writer: NonNull<dyn Write>) -> Self {
+    #[allow(clippy::new_without_default)]
+    pub fn new() -> Self {
         Self {
-            writer,
             bool_flags: 0,
             bools_left: MAX_NUM_BOOL_FLAGS,
             omit_tag: false,
         }
     }
 
-    /// Implementation detail
-    #[cfg(not(feature = "unstable-test"))]
-    pub fn into_raw(self) -> NonNull<dyn Write> {
-        self.writer
-    }
-
     // TODO turn these public methods in `export` free functions
     /// Implementation detail
     pub fn fmt(&mut self, f: &impl Format, omit_tag: bool) {
@@ -319,22 +306,4 @@ impl super::InternalFormatter {
     pub fn write(&mut self, bytes: &[u8]) {
         self.bytes.extend_from_slice(bytes)
     }
-
-    /// Implementation detail
-    ///
-    /// # Safety
-    ///
-    /// This is always safe to call and will panic. It only exists to match the non-test API.
-    pub unsafe fn from_raw(_: NonNull<dyn Write>) -> Self {
-        core::unreachable!()
-    }
-
-    /// Implementation detail
-    ///
-    /// # Safety
-    ///
-    /// This is always safe to call and will panic. It only exists to match the non-test API.
-    pub unsafe fn into_raw(self) -> NonNull<dyn Write> {
-        core::unreachable!()
-    }
 }
diff --git a/src/lib.rs b/src/lib.rs
index e068451..7c0b443 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -31,7 +31,7 @@ mod traits;
 pub use crate::{
     adapter::{Debug2Format, Display2Format},
     formatter::{Formatter, InternalFormatter, Str},
-    traits::{Format, Logger, Write},
+    traits::{Format, Logger},
 };
 
 #[cfg(all(test, not(feature = "unstable-test")))]
@@ -241,18 +241,21 @@ pub use defmt_macros::write;
 /// # Example
 ///
 /// ```
-/// use defmt::{Logger, Write, global_logger};
-/// use core::ptr::NonNull;
+/// use defmt::{Logger, global_logger};
 ///
 /// #[global_logger]
 /// struct MyLogger;
 ///
 /// unsafe impl Logger for MyLogger {
-///     fn acquire() -> Option<NonNull<dyn Write>> {
+///     fn acquire() {
 /// # todo!()
 ///         // ...
 ///     }
-///     unsafe fn release(writer: NonNull<dyn Write>) {
+///     unsafe fn release() {
+/// # todo!()
+///         // ...
+///     }
+///     unsafe fn write(bytes: &[u8]) {
 /// # todo!()
 ///         // ...
 ///     }
diff --git a/src/traits.rs b/src/traits.rs
index 4b2d562..2f8f330 100644
--- a/src/traits.rs
+++ b/src/traits.rs
@@ -1,5 +1,3 @@
-use core::ptr::NonNull;
-
 use crate::Formatter;
 
 /// Trait for types that can be formatted via defmt.
@@ -49,34 +47,34 @@ pub trait Format {
 
 /// Global logger acquire-release mechanism
 ///
+/// The global logger can be acquired once for each "execution context". The definition
+/// of execution context is up to the implementation. For example, it can be:
+///
+/// - the entire process.
+/// - one thread in std environments.
+/// - one interrupt priority level in embedded devices.
+///
 /// # Safety contract
 ///
-/// - `acquire` returns a handle that temporarily *owns* the global logger
-/// - `acquire` must return `Some` only once, until the handle is `release`-d
-/// - `acquire` is allowed to return a handle per thread or interrupt level
-/// - `acquire` is a safe function therefore it must be thread-safe and interrupt-safe
-/// - The value returned by `acquire` is not `Send` so it cannot be moved between threads or
-/// interrupt handlers
+/// - `acquire` logically acquires the global logger in the current execution context.
+///   The acquiring is tracked internally, no Rust object is returned representing ownership.
+/// - `acquire` is a safe function, therefore it must be thread-safe and interrupt-safe
 ///
-/// And, not safety related, `acquire` should never be invoked from user code. The easiest way to
+/// And, not safety related, the methods should never be invoked from user code. The easiest way to
 /// ensure this is to implement `Logger` on a *private* `struct` and mark that `struct` as the
 /// `#[global_logger]`.
 pub unsafe trait Logger {
-    /// Returns a handle to the global logger
+    /// Acquire the global logger in the current execution context.
     ///
-    /// For the requirements of the method see the documentation of the `Logger` trait
-    fn acquire() -> Option<NonNull<dyn Write>>;
+    /// Panics if already acquired in the current execution context. Otherwise it must never fail.
+    fn acquire();
 
-    /// Releases the global logger
+    /// Releases the global logger in the current execution context.
     ///
     /// # Safety
-    /// `writer` argument must be a value previously returned by `Self::acquire` and not, say,
-    /// `NonNull::dangling()`
-    unsafe fn release(writer: NonNull<dyn Write>);
-}
+    /// Must be called exactly once for each acquire(), in the same execution context.
+    unsafe fn release();
 
-/// Trait for defmt logging targets.
-pub trait Write {
     /// Writes `bytes` to the destination.
     ///
     /// This will be called by the defmt logging macros to transmit encoded data. The write
@@ -84,5 +82,9 @@ pub trait Write {
     ///
     /// Note that a call to `write` does *not* correspond to a defmt logging macro invocation. A
     /// single `defmt::info!` call can result in an arbitrary number of `write` calls.
-    fn write(&mut self, bytes: &[u8]);
+    ///
+    /// # Safety
+    /// Must only be called when the global logger is acquired in the current execution context.
+    /// (i.e. between acquire() and release())
+    unsafe fn write(bytes: &[u8]);
 }


Test patch:
diff --git a/tests/basic_usage.rs b/tests/basic_usage.rs
index b5e5005..b8cb1f9 100644
--- a/tests/basic_usage.rs
+++ b/tests/basic_usage.rs
@@ -6,10 +6,9 @@ fn main() {
 struct Logger;
 
 unsafe impl defmt::Logger for Logger {
-    fn acquire() -> Option<core::ptr::NonNull<dyn defmt::Write>> {
-        None
-    }
-    unsafe fn release(_writer: core::ptr::NonNull<dyn defmt::Write>) {}
+    fn acquire() {}
+    unsafe fn release() {}
+    unsafe fn write(_bytes: &[u8]) {}
 }
 
 defmt::timestamp!("{=u32}", 0);