Merge #961

961: Parameterize Signal with RawMutex r=ivmarkov a=ivmarkov

The `RawMutex` parameter is deliberately chosen to be the second one, so as it can take as a default `CriticalSectionRawMutex`. This way backwards compatibility is preserved, and users utilizing the `critical-section` crate everywhere can just continue to use the more ergonomic single-generic-parameter version of Signal.

I'm thinking we should probably do the same for `Channel`, and move the `RawMutex` parameter as the last one in the list, with a `CriticalSectionRawMutex` being its default. But that's a backwards-incompatible change of course.

Co-authored-by: ivmarkov <[email protected]>

4 files changed, 49 insertions, 33 deletions

diff --git a/embassy-sync/src/signal.rs b/embassy-sync/src/signal.rs
index b4d99513a..c3c10a8af 100644
--- a/embassy-sync/src/signal.rs
+++ b/embassy-sync/src/signal.rs
@@ -1,9 +1,11 @@
 //! A synchronization primitive for passing the latest value to a task.
-use core::cell::UnsafeCell;
+use core::cell::Cell;
 use core::future::{poll_fn, Future};
-use core::mem;
 use core::task::{Context, Poll, Waker};
 
+use crate::blocking_mutex::raw::RawMutex;
+use crate::blocking_mutex::Mutex;
+
 /// Single-slot signaling primitive.
 ///
 /// This is similar to a [`Channel`](crate::channel::Channel) with a buffer size of 1, except
@@ -20,16 +22,20 @@ use core::task::{Context, Poll, Waker};
 ///
 /// ```
 /// use embassy_sync::signal::Signal;
+/// use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
 ///
 /// enum SomeCommand {
 ///   On,
 ///   Off,
 /// }
 ///
-/// static SOME_SIGNAL: Signal<SomeCommand> = Signal::new();
+/// static SOME_SIGNAL: Signal<CriticalSectionRawMutex, SomeCommand> = Signal::new();
 /// ```
-pub struct Signal<T> {
-    state: UnsafeCell<State<T>>,
+pub struct Signal<M, T>
+where
+    M: RawMutex,
+{
+    state: Mutex<M, Cell<State<T>>>,
 }
 
 enum State<T> {
@@ -38,24 +44,27 @@ enum State<T> {
     Signaled(T),
 }
 
-unsafe impl<T: Send> Send for Signal<T> {}
-unsafe impl<T: Send> Sync for Signal<T> {}
-
-impl<T> Signal<T> {
+impl<M, T> Signal<M, T>
+where
+    M: RawMutex,
+{
     /// Create a new `Signal`.
     pub const fn new() -> Self {
         Self {
-            state: UnsafeCell::new(State::None),
+            state: Mutex::new(Cell::new(State::None)),
         }
     }
 }
 
-impl<T: Send> Signal<T> {
+impl<M, T: Send> Signal<M, T>
+where
+    M: RawMutex,
+{
     /// Mark this Signal as signaled.
     pub fn signal(&self, val: T) {
-        critical_section::with(|_| unsafe {
-            let state = &mut *self.state.get();
-            if let State::Waiting(waker) = mem::replace(state, State::Signaled(val)) {
+        self.state.lock(|cell| {
+            let state = cell.replace(State::Signaled(val));
+            if let State::Waiting(waker) = state {
                 waker.wake();
             }
         })
@@ -63,31 +72,27 @@ 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();
-            *state = State::None
-        })
+        self.state.lock(|cell| cell.set(State::None));
     }
 
-    /// Manually poll the Signal future.
-    pub fn poll_wait(&self, cx: &mut Context<'_>) -> Poll<T> {
-        critical_section::with(|_| unsafe {
-            let state = &mut *self.state.get();
+    fn poll_wait(&self, cx: &mut Context<'_>) -> Poll<T> {
+        self.state.lock(|cell| {
+            let state = cell.replace(State::None);
             match state {
                 State::None => {
-                    *state = State::Waiting(cx.waker().clone());
+                    cell.set(State::Waiting(cx.waker().clone()));
+                    Poll::Pending
+                }
+                State::Waiting(w) if w.will_wake(cx.waker()) => {
+                    cell.set(State::Waiting(w));
                     Poll::Pending
                 }
-                State::Waiting(w) if w.will_wake(cx.waker()) => Poll::Pending,
                 State::Waiting(w) => {
-                    let w = mem::replace(w, cx.waker().clone());
+                    cell.set(State::Waiting(cx.waker().clone()));
                     w.wake();
                     Poll::Pending
                 }
-                State::Signaled(_) => match mem::replace(state, State::None) {
-                    State::Signaled(res) => Poll::Ready(res),
-                    _ => unreachable!(),
-                },
+                State::Signaled(res) => Poll::Ready(res),
             }
         })
     }
@@ -99,6 +104,14 @@ impl<T: Send> Signal<T> {
 
     /// non-blocking method to check whether this signal has been signaled.
     pub fn signaled(&self) -> bool {
-        critical_section::with(|_| matches!(unsafe { &*self.state.get() }, State::Signaled(_)))
+        self.state.lock(|cell| {
+            let state = cell.replace(State::None);
+
+            let res = matches!(state, State::Signaled(_));
+
+            cell.set(state);
+
+            res
+        })
     }
 }
diff --git a/examples/nrf/src/bin/usb_hid_keyboard.rs b/examples/nrf/src/bin/usb_hid_keyboard.rs
index 70318b78f..4eb7d37c9 100644
--- a/examples/nrf/src/bin/usb_hid_keyboard.rs
+++ b/examples/nrf/src/bin/usb_hid_keyboard.rs
@@ -12,6 +12,7 @@ use embassy_futures::select::{select, Either};
 use embassy_nrf::gpio::{Input, Pin, Pull};
 use embassy_nrf::usb::{Driver, PowerUsb};
 use embassy_nrf::{interrupt, pac};
+use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
 use embassy_sync::signal::Signal;
 use embassy_usb::control::OutResponse;
 use embassy_usb::{Builder, Config, DeviceStateHandler};
@@ -77,7 +78,7 @@ async fn main(_spawner: Spawner) {
     // Build the builder.
     let mut usb = builder.build();
 
-    let remote_wakeup = Signal::new();
+    let remote_wakeup: Signal<CriticalSectionRawMutex, _> = Signal::new();
 
     // Run the USB device.
     let usb_fut = async {
diff --git a/examples/stm32h7/src/bin/signal.rs b/examples/stm32h7/src/bin/signal.rs
index cc3e4e3ca..6d7c168d5 100644
--- a/examples/stm32h7/src/bin/signal.rs
+++ b/examples/stm32h7/src/bin/signal.rs
@@ -4,11 +4,12 @@
 
 use defmt::{info, unwrap};
 use embassy_executor::Spawner;
+use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
 use embassy_sync::signal::Signal;
 use embassy_time::{Duration, Timer};
 use {defmt_rtt as _, panic_probe as _};
 
-static SIGNAL: Signal<u32> = Signal::new();
+static SIGNAL: Signal<CriticalSectionRawMutex, u32> = Signal::new();
 
 #[embassy_executor::task]
 async fn my_sending_task() {
diff --git a/examples/stm32wl/src/bin/subghz.rs b/examples/stm32wl/src/bin/subghz.rs
index 3c60a8de4..32c8b5515 100644
--- a/examples/stm32wl/src/bin/subghz.rs
+++ b/examples/stm32wl/src/bin/subghz.rs
@@ -12,6 +12,7 @@ use embassy_stm32::gpio::{Input, Level, Output, Pull, Speed};
 use embassy_stm32::interrupt;
 use embassy_stm32::interrupt::{Interrupt, InterruptExt};
 use embassy_stm32::subghz::*;
+use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
 use embassy_sync::signal::Signal;
 use {defmt_rtt as _, panic_probe as _};
 
@@ -64,7 +65,7 @@ async fn main(_spawner: Spawner) {
     let button = Input::new(p.PA0, Pull::Up);
     let mut pin = ExtiInput::new(button, p.EXTI0);
 
-    static IRQ_SIGNAL: Signal<()> = Signal::new();
+    static IRQ_SIGNAL: Signal<CriticalSectionRawMutex, ()> = Signal::new();
     let radio_irq = interrupt::take!(SUBGHZ_RADIO);
     radio_irq.set_handler(|_| {
         IRQ_SIGNAL.signal(());