hal-common: remove Pin in PeripheralMutex

3 files changed, 91 insertions, 92 deletions

diff --git a/embassy-hal-common/src/peripheral.rs b/embassy-hal-common/src/peripheral.rs
index 92512a0f..d3df06e8 100644
--- a/embassy-hal-common/src/peripheral.rs
+++ b/embassy-hal-common/src/peripheral.rs
@@ -1,6 +1,5 @@
-use core::cell::UnsafeCell;
-use core::marker::{PhantomData, PhantomPinned};
-use core::pin::Pin;
+use core::marker::PhantomData;
+use core::mem::MaybeUninit;
 
 use cortex_m::peripheral::scb::VectActive;
 use cortex_m::peripheral::{NVIC, SCB};
@@ -10,23 +9,23 @@ use embassy::interrupt::{Interrupt, InterruptExt};
 ///
 /// It needs to be `Send` because `&mut` references are sent back and forth between the 'thread' which owns the `PeripheralMutex` and the interrupt,
 /// and `&mut T` is only `Send` where `T: Send`.
-///
-/// It also requires `'static` to be used safely with `PeripheralMutex::register_interrupt`,
-/// because although `Pin` guarantees that the memory of the state won't be invalidated,
-/// it doesn't guarantee that the lifetime will last.
 pub trait PeripheralState: Send {
     type Interrupt: Interrupt;
     fn on_interrupt(&mut self);
 }
 
-pub struct PeripheralMutex<S: PeripheralState> {
-    state: UnsafeCell<S>,
+pub struct StateStorage<S>(MaybeUninit<S>);
 
-    irq_setup_done: bool,
-    irq: S::Interrupt,
+impl<S> StateStorage<S> {
+    pub fn new() -> Self {
+        Self(MaybeUninit::uninit())
+    }
+}
 
-    _not_send: PhantomData<*mut ()>,
-    _pinned: PhantomPinned,
+pub struct PeripheralMutex<'a, S: PeripheralState> {
+    state: *mut S,
+    _phantom: PhantomData<&'a mut S>,
+    irq: S::Interrupt,
 }
 
 /// Whether `irq` can be preempted by the current interrupt.
@@ -50,58 +49,45 @@ pub(crate) fn can_be_preempted(irq: &impl Interrupt) -> bool {
     }
 }
 
-impl<S: PeripheralState + 'static> PeripheralMutex<S> {
-    /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it.
-    ///
-    /// This requires this `PeripheralMutex`'s `PeripheralState` to live for `'static`,
-    /// because `Pin` only guarantees that it's memory won't be repurposed,
-    /// not that it's lifetime will last.
+impl<'a, S: PeripheralState> PeripheralMutex<'a, S> {
+    /// Create a new `PeripheralMutex` wrapping `irq`, with the initial state `state`.
     ///
-    /// To use non-`'static` `PeripheralState`, use the unsafe `register_interrupt_unchecked`.
+    /// self requires `state` to live for `'static`, because if the `PeripheralMutex` is leaked, the
+    /// interrupt won't be disabled, which may try accessing the state at any time. To use non-`'static`
+    /// state, see [`Self::new_unchecked`].
     ///
-    /// Note: `'static` doesn't mean it _has_ to live for the entire program, like an `&'static T`;
-    /// it just means it _can_ live for the entire program - for example, `u8` lives for `'static`.
-    pub fn register_interrupt(self: Pin<&mut Self>) {
-        // SAFETY: `S: 'static`, so there's no way it's lifetime can expire.
-        unsafe { self.register_interrupt_unchecked() }
+    /// Registers `on_interrupt` as the `irq`'s handler, and enables it.
+    pub fn new(storage: &'a mut StateStorage<S>, state: S, irq: S::Interrupt) -> Self
+    where
+        'a: 'static,
+    {
+        // safety: safe because state is `'static`.
+        unsafe { Self::new_unchecked(storage, state, irq) }
     }
-}
 
-impl<S: PeripheralState> PeripheralMutex<S> {
-    /// Create a new `PeripheralMutex` wrapping `irq`, with the initial state `state`.
-    pub fn new(state: S, irq: S::Interrupt) -> Self {
+    /// Create a `PeripheralMutex` without requiring the state is `'static`.
+    ///
+    /// See also [`Self::new`].
+    ///
+    /// # Safety
+    /// The created instance must not be leaked (its `drop` must run).
+    pub unsafe fn new_unchecked(
+        storage: &'a mut StateStorage<S>,
+        state: S,
+        irq: S::Interrupt,
+    ) -> Self {
         if can_be_preempted(&irq) {
             panic!("`PeripheralMutex` cannot be created in an interrupt with higher priority than the interrupt it wraps");
         }
 
-        Self {
-            irq,
-            irq_setup_done: false,
+        let state_ptr = storage.0.as_mut_ptr();
 
-            state: UnsafeCell::new(state),
-            _not_send: PhantomData,
-            _pinned: PhantomPinned,
-        }
-    }
-
-    /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it.
-    ///
-    /// # Safety
-    /// The lifetime of any data in `PeripheralState` that is accessed by the interrupt handler
-    /// must not end without `Drop` being called on this `PeripheralMutex`.
-    ///
-    /// This can be accomplished by either not accessing any data with a lifetime in `on_interrupt`,
-    /// or making sure that nothing like `mem::forget` is used on the `PeripheralMutex`.
-
-    // TODO: this name isn't the best.
-    pub unsafe fn register_interrupt_unchecked(self: Pin<&mut Self>) {
-        let this = self.get_unchecked_mut();
-        if this.irq_setup_done {
-            return;
-        }
+        // Safety: The pointer is valid and not used by anyone else
+        // because we have the `&mut StateStorage`.
+        state_ptr.write(state);
 
-        this.irq.disable();
-        this.irq.set_handler(|p| {
+        irq.disable();
+        irq.set_handler(|p| {
             // Safety: it's OK to get a &mut to the state, since
             // - We checked that the thread owning the `PeripheralMutex` can't preempt us in `new`.
             //   Interrupts' priorities can only be changed with raw embassy `Interrupts`,
@@ -110,23 +96,24 @@ impl<S: PeripheralState> PeripheralMutex<S> {
             let state = unsafe { &mut *(p as *mut S) };
             state.on_interrupt();
         });
-        this.irq
-            .set_handler_context((&mut this.state) as *mut _ as *mut ());
-        this.irq.enable();
+        irq.set_handler_context(state_ptr as *mut ());
+        irq.enable();
 
-        this.irq_setup_done = true;
+        Self {
+            irq,
+            state: state_ptr,
+            _phantom: PhantomData,
+        }
     }
 
-    pub fn with<R>(self: Pin<&mut Self>, f: impl FnOnce(&mut S) -> R) -> R {
-        let this = unsafe { self.get_unchecked_mut() };
-
-        this.irq.disable();
+    pub fn with<R>(&mut self, f: impl FnOnce(&mut S) -> R) -> R {
+        self.irq.disable();
 
         // Safety: it's OK to get a &mut to the state, since the irq is disabled.
-        let state = unsafe { &mut *this.state.get() };
+        let state = unsafe { &mut *self.state };
         let r = f(state);
 
-        this.irq.enable();
+        self.irq.enable();
 
         r
     }
@@ -152,9 +139,14 @@ impl<S: PeripheralState> PeripheralMutex<S> {
     }
 }
 
-impl<S: PeripheralState> Drop for PeripheralMutex<S> {
+impl<'a, S: PeripheralState> Drop for PeripheralMutex<'a, S> {
     fn drop(&mut self) {
         self.irq.disable();
         self.irq.remove_handler();
+
+        // safety:
+        // - we initialized the state in `new`, so we know it's initialized.
+        // - the irq is disabled, so it won't preempt us while dropping.
+        unsafe { self.state.drop_in_place() }
     }
 }
diff --git a/embassy-hal-common/src/usb/mod.rs b/embassy-hal-common/src/usb/mod.rs
index 1fb501d7..0940e6b0 100644
--- a/embassy-hal-common/src/usb/mod.rs
+++ b/embassy-hal-common/src/usb/mod.rs
@@ -9,14 +9,31 @@ use usb_device::device::UsbDevice;
 mod cdc_acm;
 pub mod usb_serial;
 
-use crate::peripheral::{PeripheralMutex, PeripheralState};
+use crate::peripheral::{PeripheralMutex, PeripheralState, StateStorage};
 use embassy::interrupt::Interrupt;
 use usb_serial::{ReadInterface, UsbSerial, WriteInterface};
 
 /// Marker trait to mark an interrupt to be used with the [`Usb`] abstraction.
 pub unsafe trait USBInterrupt: Interrupt + Send {}
 
-pub(crate) struct State<'bus, B, T, I>
+pub struct State<'bus, B, T, I>(StateStorage<StateInner<'bus, B, T, I>>)
+where
+    B: UsbBus,
+    T: ClassSet<B>,
+    I: USBInterrupt;
+
+impl<'bus, B, T, I> State<'bus, B, T, I>
+where
+    B: UsbBus,
+    T: ClassSet<B>,
+    I: USBInterrupt,
+{
+    pub fn new() -> Self {
+        Self(StateStorage::new())
+    }
+}
+
+pub(crate) struct StateInner<'bus, B, T, I>
 where
     B: UsbBus,
     T: ClassSet<B>,
@@ -34,7 +51,7 @@ where
     I: USBInterrupt,
 {
     // Don't you dare moving out `PeripheralMutex`
-    inner: RefCell<PeripheralMutex<State<'bus, B, T, I>>>,
+    inner: RefCell<PeripheralMutex<'bus, StateInner<'bus, B, T, I>>>,
 }
 
 impl<'bus, B, T, I> Usb<'bus, B, T, I>
@@ -43,30 +60,22 @@ where
     T: ClassSet<B>,
     I: USBInterrupt,
 {
-    pub fn new<S: IntoClassSet<B, T>>(device: UsbDevice<'bus, B>, class_set: S, irq: I) -> Self {
-        let state = State {
+    pub fn new<S: IntoClassSet<B, T>>(
+        state: &'bus mut State<'bus, B, T, I>,
+        device: UsbDevice<'bus, B>,
+        class_set: S,
+        irq: I,
+    ) -> Self {
+        let initial_state = StateInner {
             device,
             classes: class_set.into_class_set(),
             _interrupt: PhantomData,
         };
-        let mutex = PeripheralMutex::new(state, irq);
+        let mutex = unsafe { PeripheralMutex::new_unchecked(&mut state.0, initial_state, irq) };
         Self {
             inner: RefCell::new(mutex),
         }
     }
-
-    /// # Safety
-    /// The `UsbDevice` passed to `Self::new` must not be dropped without calling `Drop` on this `Usb` first.
-    pub unsafe fn start(self: Pin<&mut Self>) {
-        let this = self.get_unchecked_mut();
-        let mut mutex = this.inner.borrow_mut();
-        let mutex = Pin::new_unchecked(&mut *mutex);
-
-        // Use inner to register the irq
-        // SAFETY: the safety contract of this function makes sure the `UsbDevice` won't be invalidated
-        // without the `PeripheralMutex` being dropped.
-        mutex.register_interrupt_unchecked();
-    }
 }
 
 impl<'bus, 'c, B, T, I> Usb<'bus, B, T, I>
@@ -129,7 +138,7 @@ where
     }
 }
 
-impl<'bus, B, T, I> PeripheralState for State<'bus, B, T, I>
+impl<'bus, B, T, I> PeripheralState for StateInner<'bus, B, T, I>
 where
     B: UsbBus,
     T: ClassSet<B>,
diff --git a/embassy-hal-common/src/usb/usb_serial.rs b/embassy-hal-common/src/usb/usb_serial.rs
index a229b200..8b27152b 100644
--- a/embassy-hal-common/src/usb/usb_serial.rs
+++ b/embassy-hal-common/src/usb/usb_serial.rs
@@ -10,9 +10,10 @@ use usb_device::class_prelude::*;
 use usb_device::UsbError;
 
 use super::cdc_acm::CdcAcmClass;
+use super::StateInner;
 use crate::peripheral::PeripheralMutex;
 use crate::ring_buffer::RingBuffer;
-use crate::usb::{ClassSet, SerialState, State, USBInterrupt};
+use crate::usb::{ClassSet, SerialState, USBInterrupt};
 
 pub struct ReadInterface<'a, 'bus, 'c, I, B, T, INT>
 where
@@ -22,7 +23,7 @@ where
     INT: USBInterrupt,
 {
     // Don't you dare moving out `PeripheralMutex`
-    pub(crate) inner: &'a RefCell<PeripheralMutex<State<'bus, B, T, INT>>>,
+    pub(crate) inner: &'a RefCell<PeripheralMutex<'bus, StateInner<'bus, B, T, INT>>>,
     pub(crate) _buf_lifetime: PhantomData<&'c T>,
     pub(crate) _index: PhantomData<I>,
 }
@@ -39,7 +40,7 @@ where
     INT: USBInterrupt,
 {
     // Don't you dare moving out `PeripheralMutex`
-    pub(crate) inner: &'a RefCell<PeripheralMutex<State<'bus, B, T, INT>>>,
+    pub(crate) inner: &'a RefCell<PeripheralMutex<'bus, StateInner<'bus, B, T, INT>>>,
     pub(crate) _buf_lifetime: PhantomData<&'c T>,
     pub(crate) _index: PhantomData<I>,
 }
@@ -54,7 +55,6 @@ where
     fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<io::Result<&[u8]>> {
         let this = self.get_mut();
         let mut mutex = this.inner.borrow_mut();
-        let mutex = unsafe { Pin::new_unchecked(&mut *mutex) };
         mutex.with(|state| {
             let serial = state.classes.get_serial();
             let serial = Pin::new(serial);
@@ -76,7 +76,6 @@ where
     fn consume(self: Pin<&mut Self>, amt: usize) {
         let this = self.get_mut();
         let mut mutex = this.inner.borrow_mut();
-        let mutex = unsafe { Pin::new_unchecked(&mut *mutex) };
         mutex.with(|state| {
             let serial = state.classes.get_serial();
             let serial = Pin::new(serial);
@@ -100,7 +99,6 @@ where
     ) -> Poll<io::Result<usize>> {
         let this = self.get_mut();
         let mut mutex = this.inner.borrow_mut();
-        let mutex = unsafe { Pin::new_unchecked(&mut *mutex) };
         mutex.with(|state| {
             let serial = state.classes.get_serial();
             let serial = Pin::new(serial);