diff options
author | Dario Nieuwenhuis <dirbaio@dirbaio.net> | 2021-07-29 14:08:32 +0200 |
---|---|---|
committer | Dario Nieuwenhuis <dirbaio@dirbaio.net> | 2021-08-02 19:55:04 +0200 |
commit | af87031d62ca9ee5e7dd44cba297f3d171ec0708 (patch) | |
tree | 82f43b826d016c0a2b9a86ec6f27a1f0d5fd63d5 /embassy-hal-common | |
parent | de207764aee0dd9c23bd02f92b55a55babd47b1a (diff) | |
download | embassy-af87031d62ca9ee5e7dd44cba297f3d171ec0708.zip |
hal-common: remove Pin in PeripheralMutex
Diffstat (limited to 'embassy-hal-common')
-rw-r--r-- | embassy-hal-common/src/peripheral.rs | 124 | ||||
-rw-r--r-- | embassy-hal-common/src/usb/mod.rs | 49 | ||||
-rw-r--r-- | embassy-hal-common/src/usb/usb_serial.rs | 10 |
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); |