Merge pull request #835 from embassy-rs/fixes

Misc API and doc fixes.

embassy/src/blocking_mutex/mod.rs

@@ -160,11 +160,20 @@ mod thread_mode_mutex {
     }
 
     impl<T: ?Sized> ThreadModeMutex<T> {
+        /// Lock the `ThreadModeMutex`, granting access to the data.
+        ///
+        /// # Panics
+        ///
+        /// This will panic if not currently running in thread mode.
         pub fn lock<R>(&self, f: impl FnOnce(&T) -> R) -> R {
             f(self.borrow())
         }
 
         /// Borrows the data
+        ///
+        /// # Panics
+        ///
+        /// This will panic if not currently running in thread mode.
         pub fn borrow(&self) -> &T {
             assert!(
                 raw::in_thread_mode(),

embassy/src/blocking_mutex/raw.rs

@@ -3,12 +3,32 @@
 //! This module provides a trait for mutexes that can be used in different contexts.
 use core::marker::PhantomData;
 
-/// Any object implementing this trait guarantees exclusive access to the data contained
+/// Raw mutex trait.
-/// within the mutex for the duration of the lock.
+///
-/// Adapted from <https://github.com/rust-embedded/mutex-trait>.
+/// This mutex is "raw", which means it does not actually contain the protected data, it
-pub trait RawMutex {
+/// just implements the mutex mechanism. For most uses you should use [`super::Mutex`] instead,
+/// which is generic over a RawMutex and contains the protected data.
+///
+/// Note that, unlike other mutexes, implementations only guarantee no
+/// concurrent access from other threads: concurrent access from the current
+/// thread is allwed. For example, it's possible to lock the same mutex multiple times reentrantly.
+///
+/// Therefore, locking a `RawMutex` is only enough to guarantee safe shared (`&`) access
+/// to the data, it is not enough to guarantee exclusive (`&mut`) access.
+///
+/// # Safety
+///
+/// RawMutex implementations must ensure that, while locked, no other thread can lock
+/// the RawMutex concurrently.
+///
+/// Unsafe code is allowed to rely on this fact, so incorrect implementations will cause undefined behavior.
+pub unsafe trait RawMutex {
+    /// Create a new `RawMutex` instance.
+    ///
+    /// This is a const instead of a method to allow creating instances in const context.
     const INIT: Self;
 
+    /// Lock this `RawMutex`.
     fn lock<R>(&self, f: impl FnOnce() -> R) -> R;
 }
 
@@ -24,12 +44,13 @@ unsafe impl Send for CriticalSectionRawMutex {}
 unsafe impl Sync for CriticalSectionRawMutex {}
 
 impl CriticalSectionRawMutex {
+    /// Create a new `CriticalSectionRawMutex`.
     pub const fn new() -> Self {
         Self { _phantom: PhantomData }
     }
 }
 
-impl RawMutex for CriticalSectionRawMutex {
+unsafe impl RawMutex for CriticalSectionRawMutex {
     const INIT: Self = Self::new();
 
     fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
@@ -51,12 +72,13 @@ pub struct NoopRawMutex {
 unsafe impl Send for NoopRawMutex {}
 
 impl NoopRawMutex {
+    /// Create a new `NoopRawMutex`.
     pub const fn new() -> Self {
         Self { _phantom: PhantomData }
     }
 }
 
-impl RawMutex for NoopRawMutex {
+unsafe impl RawMutex for NoopRawMutex {
     const INIT: Self = Self::new();
     fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
         f()
@@ -84,12 +106,13 @@ mod thread_mode {
     unsafe impl Sync for ThreadModeRawMutex {}
 
     impl ThreadModeRawMutex {
+        /// Create a new `ThreadModeRawMutex`.
         pub const fn new() -> Self {
             Self { _phantom: PhantomData }
         }
     }
 
-    impl RawMutex for ThreadModeRawMutex {
+    unsafe impl RawMutex for ThreadModeRawMutex {
         const INIT: Self = Self::new();
         fn lock<R>(&self, f: impl FnOnce() -> R) -> R {
             assert!(in_thread_mode(), "ThreadModeMutex can only be locked from thread mode.");

embassy/src/channel/mpmc.rs

@@ -180,6 +180,7 @@ where
     }
 }
 
+/// Future returned by [`Channel::recv`] and  [`Receiver::recv`].
 pub struct RecvFuture<'ch, M, T, const N: usize>
 where
     M: RawMutex,
@@ -201,6 +202,7 @@ where
     }
 }
 
+/// Future returned by [`DynamicReceiver::recv`].
 pub struct DynamicRecvFuture<'ch, T> {
     channel: &'ch dyn DynamicChannel<T>,
 }
@@ -216,6 +218,7 @@ impl<'ch, T> Future for DynamicRecvFuture<'ch, T> {
     }
 }
 
+/// Future returned by [`Channel::send`] and  [`Sender::send`].
 pub struct SendFuture<'ch, M, T, const N: usize>
 where
     M: RawMutex,
@@ -246,6 +249,7 @@ where
 
 impl<'ch, M, T, const N: usize> Unpin for SendFuture<'ch, M, T, N> where M: RawMutex {}
 
+/// Future returned by [`DynamicSender::send`].
 pub struct DynamicSendFuture<'ch, T> {
     channel: &'ch dyn DynamicChannel<T>,
     message: Option<T>,

embassy/src/channel/pubsub/mod.rs

@@ -104,7 +104,7 @@ impl<M: RawMutex, T: Clone, const CAP: usize, const SUBS: usize, const PUBS: usi
     /// Create a new subscriber. It will only receive messages that are published after its creation.
     ///
     /// If there are no subscriber slots left, an error will be returned.
-    pub fn dyn_subscriber<'a>(&'a self) -> Result<DynSubscriber<'a, T>, Error> {
+    pub fn dyn_subscriber(&self) -> Result<DynSubscriber<'_, T>, Error> {
         self.inner.lock(|inner| {
             let mut s = inner.borrow_mut();
 
@@ -136,7 +136,7 @@ impl<M: RawMutex, T: Clone, const CAP: usize, const SUBS: usize, const PUBS: usi
     /// Create a new publisher
     ///
     /// If there are no publisher slots left, an error will be returned.
-    pub fn dyn_publisher<'a>(&'a self) -> Result<DynPublisher<'a, T>, Error> {
+    pub fn dyn_publisher(&self) -> Result<DynPublisher<'_, T>, Error> {
         self.inner.lock(|inner| {
             let mut s = inner.borrow_mut();
 
@@ -369,7 +369,7 @@ impl<T: Clone, const CAP: usize, const SUBS: usize, const PUBS: usize> PubSubSta
 }
 
 /// Error type for the [PubSubChannel]
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, PartialEq, Eq, Clone)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub enum Error {
     /// All subscriber slots are used. To add another subscriber, first another subscriber must be dropped or
@@ -404,7 +404,7 @@ pub trait PubSubBehavior<T> {
 }
 
 /// The result of the subscriber wait procedure
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub enum WaitResult<T> {
     /// The subscriber did not receive all messages and lagged by the given amount of messages.

embassy/src/channel/signal.rs

@@ -4,11 +4,19 @@ use core::future::Future;
 use core::mem;
 use core::task::{Context, Poll, Waker};
 
-/// Synchronization primitive. Allows creating awaitable signals that may be passed between tasks.
+/// Single-slot signaling primitive.
-/// For a simple use-case where the receiver is only ever interested in the latest value of
-/// something, Signals work well. For more advanced use cases, you might want to use [`Channel`](crate::channel::mpmc::Channel) instead..
 ///
-/// Signals are generally declared as being a static const and then borrowed as required.
+/// This is similar to a [`Channel`](crate::channel::mpmc::Channel) with a buffer size of 1, except
+/// "sending" to it (calling [`Signal::signal`]) when full will overwrite the previous value instead
+/// of waiting for the receiver to pop the previous value.
+///
+/// It is useful for sending data between tasks when the receiver only cares about
+/// the latest data, and therefore it's fine to "lose" messages. This is often the case for "state"
+/// updates.
+///
+/// For more advanced use cases, you might want to use [`Channel`](crate::channel::mpmc::Channel) instead.
+///
+/// Signals are generally declared as `static`s and then borrowed as required.
 ///
 /// ```
 /// use embassy::channel::signal::Signal;
@@ -34,6 +42,7 @@ unsafe impl<T: Send> Send for Signal<T> {}
 unsafe impl<T: Send> Sync for Signal<T> {}
 
 impl<T> Signal<T> {
+    /// Create a new `Signal`.
     pub const fn new() -> Self {
         Self {
             state: UnsafeCell::new(State::None),
@@ -42,7 +51,7 @@ impl<T> Signal<T> {
 }
 
 impl<T: Send> Signal<T> {
-    /// Mark this Signal as completed.
+    /// Mark this Signal as signaled.
     pub fn signal(&self, val: T) {
         critical_section::with(|_| unsafe {
             let state = &mut *self.state.get();
@@ -52,6 +61,7 @@ impl<T: Send> Signal<T> {
         })
     }
 
+    /// Remove the queued value in this `Signal`, if any.
     pub fn reset(&self) {
         critical_section::with(|_| unsafe {
             let state = &mut *self.state.get();
@@ -59,7 +69,7 @@ impl<T: Send> Signal<T> {
         })
     }
 
-    pub fn poll_wait(&self, cx: &mut Context<'_>) -> Poll<T> {
+    fn poll_wait(&self, cx: &mut Context<'_>) -> Poll<T> {
         critical_section::with(|_| unsafe {
             let state = &mut *self.state.get();
             match state {

embassy/src/executor/raw/mod.rs

@@ -445,6 +445,10 @@ impl Executor {
 /// Wake a task by raw pointer.
 ///
 /// You can obtain task pointers from `Waker`s using [`task_from_waker`].
+///
+/// # Safety
+///
+/// `task` must be a valid task pointer obtained from [`task_from_waker`].
 pub unsafe fn wake_task(task: NonNull<TaskHeader>) {
     task.as_ref().enqueue();
 }

embassy/src/executor/raw/waker.rs

@@ -33,12 +33,18 @@ pub(crate) unsafe fn from_task(p: NonNull<TaskHeader>) -> Waker {
 /// # Panics
 ///
 /// Panics if the waker is not created by the Embassy executor.
-pub unsafe fn task_from_waker(waker: &Waker) -> NonNull<TaskHeader> {
+pub fn task_from_waker(waker: &Waker) -> NonNull<TaskHeader> {
-    let hack: &WakerHack = mem::transmute(waker);
+    // safety: OK because WakerHack has the same layout as Waker.
+    // This is not really guaranteed because the structs are `repr(Rust)`, it is
+    // indeed the case in the current implementation.
+    // TODO use waker_getters when stable. https://github.com/rust-lang/rust/issues/96992
+    let hack: &WakerHack = unsafe { mem::transmute(waker) };
     if hack.vtable != &VTABLE {
         panic!("Found waker not created by the embassy executor. Consider enabling the `executor-agnostic` feature on the `embassy` crate.")
     }
-    NonNull::new_unchecked(hack.data as *mut TaskHeader)
+
+    // safety: we never create a waker with a null data pointer.
+    unsafe { NonNull::new_unchecked(hack.data as *mut TaskHeader) }
 }
 
 struct WakerHack {

embassy/src/executor/spawner.rs

@@ -93,7 +93,7 @@ impl Spawner {
     pub async fn for_current_executor() -> Self {
         poll_fn(|cx| unsafe {
             let task = raw::task_from_waker(cx.waker());
-            let executor = (&*task.as_ptr()).executor.get();
+            let executor = (*task.as_ptr()).executor.get();
             Poll::Ready(Self::new(&*executor))
         })
         .await
@@ -169,7 +169,7 @@ impl SendSpawner {
     pub async fn for_current_executor() -> Self {
         poll_fn(|cx| unsafe {
             let task = raw::task_from_waker(cx.waker());
-            let executor = (&*task.as_ptr()).executor.get();
+            let executor = (*task.as_ptr()).executor.get();
             Poll::Ready(Self::new(&*executor))
         })
         .await

embassy/src/lib.rs

@@ -3,6 +3,7 @@
 #![cfg_attr(all(feature = "nightly", target_arch = "xtensa"), feature(asm_experimental_arch))]
 #![allow(clippy::new_without_default)]
 #![doc = include_str!("../../README.md")]
+#![warn(missing_docs)]
 
 // This mod MUST go first, so that the others see its macros.
 pub(crate) mod fmt;

embassy/src/util/forever.rs

@@ -31,6 +31,7 @@ unsafe impl<T> Send for Forever<T> {}
 unsafe impl<T> Sync for Forever<T> {}
 
 impl<T> Forever<T> {
+    /// Create a new `Forever`.
     #[inline(always)]
     pub const fn new() -> Self {
         Self {
@@ -39,11 +40,15 @@ impl<T> Forever<T> {
         }
     }
 
-    /// Gives this `Forever` a value.
+    /// Store a value in this `Forever`, returning a mutable reference to it.
     ///
-    /// Panics if this `Forever` already has a value.
+    /// Using this method, the compiler usually constructs `val` in the stack and then moves
+    /// it into the `Forever`. If `T` is big, this is likely to cause stack overflows.
+    /// Considering using [`Signal::put_with`] instead, which will construct it in-place inside the `Forever`.
     ///
-    /// Returns a mutable reference to the stored value.
+    /// # Panics
+    ///
+    /// Panics if this `Forever` already has a value stored in it.
     #[inline(always)]
     #[allow(clippy::mut_from_ref)]
     pub fn put(&'static self, val: T) -> &'static mut T {
@@ -52,7 +57,7 @@ impl<T> Forever<T> {
             .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed)
             .is_err()
         {
-            panic!("Forever.put() called multiple times");
+            panic!("Forever::put() called multiple times");
         }
 
         unsafe {
@@ -63,6 +68,14 @@ impl<T> Forever<T> {
         }
     }
 
+    /// Store the closure return value in this `Forever`, returning a mutable reference to it.
+    ///
+    /// The advantage over [`Forever::put`] is that this method allows the closure to construct
+    /// the `T` value in-place directly inside the `Forever`, saving stack space.
+    ///
+    /// # Panics
+    ///
+    /// Panics if this `Forever` already has a value stored in it.
     #[inline(always)]
     #[allow(clippy::mut_from_ref)]
     pub fn put_with(&'static self, val: impl FnOnce() -> T) -> &'static mut T {
@@ -82,9 +95,17 @@ impl<T> Forever<T> {
         }
     }
 
+    /// Unsafely get a mutable reference to the contents of this Forever.
+    ///
+    /// # Safety
+    ///
+    /// This is undefined behavior if:
+    ///
+    /// - The `Forever` has not been initialized yet (with `put' or `put_with`), or
+    /// - A reference to the contents (mutable or not) already exists.
     #[inline(always)]
     #[allow(clippy::mut_from_ref)]
-    pub unsafe fn steal(&'static self) -> &'static mut T {
+    pub unsafe fn steal(&self) -> &mut T {
         let p = self.t.get();
         let p = (&mut *p).as_mut_ptr();
         &mut *p

embassy/src/util/select.rs

@@ -2,9 +2,12 @@ use core::future::Future;
 use core::pin::Pin;
 use core::task::{Context, Poll};
 
+/// Result for [`select`].
 #[derive(Debug, Clone)]
 pub enum Either<A, B> {
+    /// First future finished first.
     First(A),
+    /// Second future finished first.
     Second(B),
 }
 
@@ -55,10 +58,14 @@ where
 
 // ====================================================================
 
+/// Result for [`select3`].
 #[derive(Debug, Clone)]
 pub enum Either3<A, B, C> {
+    /// First future finished first.
     First(A),
+    /// Second future finished first.
     Second(B),
+    /// Third future finished first.
     Third(C),
 }
 
@@ -109,11 +116,16 @@ where
 
 // ====================================================================
 
+/// Result for [`select4`].
 #[derive(Debug, Clone)]
 pub enum Either4<A, B, C, D> {
+    /// First future finished first.
     First(A),
+    /// Second future finished first.
     Second(B),
+    /// Third future finished first.
     Third(C),
+    /// Fourth future finished first.
     Fourth(D),
 }
 

embassy/src/waitqueue/waker.rs

@@ -16,13 +16,14 @@ pub struct WakerRegistration {
 }
 
 impl WakerRegistration {
+    /// Create a new `WakerRegistration`.
     pub const fn new() -> Self {
         Self { waker: None }
     }
 
     /// Register a waker. Overwrites the previous waker, if any.
     pub fn register(&mut self, w: &Waker) {
-        let w = unsafe { task_from_waker(w) };
+        let w = task_from_waker(w);
         match self.waker {
             // Optimization: If both the old and new Wakers wake the same task, do nothing.
             Some(w2) if w == w2 => {}
@@ -72,6 +73,7 @@ pub struct AtomicWaker {
 }
 
 impl AtomicWaker {
+    /// Create a new `AtomicWaker`.
     pub const fn new() -> Self {
         Self {
             waker: AtomicPtr::new(ptr::null_mut()),
@@ -80,7 +82,7 @@ impl AtomicWaker {
 
     /// Register a waker. Overwrites the previous waker, if any.
     pub fn register(&self, w: &Waker) {
-        let w = unsafe { task_from_waker(w) };
+        let w = task_from_waker(w);
         self.waker.store(w.as_ptr(), Ordering::Relaxed);
         compiler_fence(Ordering::SeqCst);
     }

embassy/src/waitqueue/waker_agnostic.rs

@@ -12,6 +12,7 @@ pub struct WakerRegistration {
 }
 
 impl WakerRegistration {
+    /// Create a new `WakerRegistration`.
     pub const fn new() -> Self {
         Self { waker: None }
     }
@@ -60,6 +61,7 @@ pub struct AtomicWaker {
 }
 
 impl AtomicWaker {
+    /// Create a new `AtomicWaker`.
     pub const fn new() -> Self {
         Self {
             waker: Mutex::const_new(CriticalSectionRawMutex::new(), Cell::new(None)),