From 0cd19a58c3dcaa689ba57da80c02af75866f7e09 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 18 Mar 2021 02:01:29 +0100 Subject: [PATCH 1/2] Remove free() from PeripheralMutex and all nrf drivers. --- embassy-extras/src/peripheral.rs | 77 ++++++++----------------------- embassy-nrf/src/buffered_uarte.rs | 29 ++---------- embassy-nrf/src/qspi.rs | 5 -- embassy-nrf/src/spim.rs | 5 -- 4 files changed, 23 insertions(+), 93 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index e9357969d..aa23bc978 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -1,30 +1,20 @@ use core::cell::UnsafeCell; use core::marker::{PhantomData, PhantomPinned}; -use core::mem::MaybeUninit; use core::pin::Pin; use core::sync::atomic::{compiler_fence, Ordering}; use embassy::interrupt::{Interrupt, InterruptExt}; -use crate::fmt::assert; - pub trait PeripheralState { type Interrupt: Interrupt; fn on_interrupt(&mut self); } -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -enum Life { - Ready, - Created, - Freed, -} - pub struct PeripheralMutex { - life: Life, + state: UnsafeCell, - state: MaybeUninit>, // Init if life != Freed - irq: MaybeUninit, // Init if life != Freed + irq_setup_done: bool, + irq: S::Interrupt, _not_send: PhantomData<*mut ()>, _pinned: PhantomPinned, @@ -33,9 +23,10 @@ pub struct PeripheralMutex { impl PeripheralMutex { pub fn new(state: S, irq: S::Interrupt) -> Self { Self { - life: Life::Created, - state: MaybeUninit::new(UnsafeCell::new(state)), - irq: MaybeUninit::new(irq), + irq, + irq_setup_done: false, + + state: UnsafeCell::new(state), _not_send: PhantomData, _pinned: PhantomPinned, } @@ -43,77 +34,49 @@ impl PeripheralMutex { /// safety: self must be pinned. unsafe fn setup(&mut self) { - assert!(self.life == Life::Created); - - let irq = &mut *self.irq.as_mut_ptr(); - irq.disable(); + self.irq.disable(); compiler_fence(Ordering::SeqCst); - irq.set_handler(|p| { + self.irq.set_handler(|p| { // Safety: it's OK to get a &mut to the state, since // - We're in the IRQ, no one else can't preempt us // - We can't have preempted a with() call because the irq is disabled during it. let state = &mut *(p as *mut S); state.on_interrupt(); }); - irq.set_handler_context(self.state.as_mut_ptr() as *mut ()); + self.irq + .set_handler_context((&mut self.state) as *mut _ as *mut ()); compiler_fence(Ordering::SeqCst); - irq.enable(); + self.irq.enable(); - self.life = Life::Ready; + self.irq_setup_done = true; } pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S, &mut S::Interrupt) -> R) -> R { let this = unsafe { self.get_unchecked_mut() }; - if this.life != Life::Ready { + if !this.irq_setup_done { unsafe { this.setup() } } - let irq = unsafe { &mut *this.irq.as_mut_ptr() }; - - irq.disable(); + this.irq.disable(); compiler_fence(Ordering::SeqCst); // Safety: it's OK to get a &mut to the state, since the irq is disabled. - let state = unsafe { &mut *(*this.state.as_ptr()).get() }; + let state = unsafe { &mut *this.state.get() }; - let r = f(state, irq); + let r = f(state, &mut this.irq); compiler_fence(Ordering::SeqCst); - irq.enable(); + this.irq.enable(); r } - - pub fn try_free(self: Pin<&mut Self>) -> Option<(S, S::Interrupt)> { - let this = unsafe { self.get_unchecked_mut() }; - - if this.life != Life::Freed { - return None; - } - - unsafe { &mut *this.irq.as_mut_ptr() }.disable(); - compiler_fence(Ordering::SeqCst); - - this.life = Life::Freed; - - let state = unsafe { this.state.as_ptr().read().into_inner() }; - let irq = unsafe { this.irq.as_ptr().read() }; - Some((state, irq)) - } - - pub fn free(self: Pin<&mut Self>) -> (S, S::Interrupt) { - unwrap!(self.try_free()) - } } impl Drop for PeripheralMutex { fn drop(&mut self) { - if self.life != Life::Freed { - let irq = unsafe { &mut *self.irq.as_mut_ptr() }; - irq.disable(); - irq.remove_handler(); - } + self.irq.disable(); + self.irq.remove_handler(); } } diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index b1366cd9e..9b4ec6061 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -201,29 +201,6 @@ impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi fn inner(self: Pin<&mut Self>) -> Pin<&mut PeripheralMutex>> { unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) } } - - pub fn free(self: Pin<&mut Self>) -> (U, T, P1, P2, U::Interrupt) { - let (mut state, irq) = self.inner().free(); - state.stop(); - ( - state.uarte, - state.timer, - state.ppi_channel_1, - state.ppi_channel_2, - irq, - ) - } -} - -impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> Drop - for BufferedUarte<'a, U, T, P1, P2> -{ - fn drop(&mut self) { - let inner = unsafe { Pin::new_unchecked(&mut self.inner) }; - if let Some((mut state, _irq)) = inner.try_free() { - state.stop(); - } - } } impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> AsyncBufRead @@ -293,10 +270,10 @@ impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi } } -impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> - State<'a, U, T, P1, P2> +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> Drop + for State<'a, U, T, P1, P2> { - fn stop(&mut self) { + fn drop(&mut self) { self.timer.tasks_stop.write(|w| unsafe { w.bits(1) }); if let RxState::Receiving = self.rx_state { self.uarte.tasks_stoprx.write(|w| unsafe { w.bits(1) }); diff --git a/embassy-nrf/src/qspi.rs b/embassy-nrf/src/qspi.rs index f6e8175fa..026660cba 100644 --- a/embassy-nrf/src/qspi.rs +++ b/embassy-nrf/src/qspi.rs @@ -238,11 +238,6 @@ impl Qspi { unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) } } - pub fn free(self: Pin<&mut Self>) -> (QSPI, interrupt::QSPI) { - let (state, irq) = self.inner().free(); - (state.inner, irq) - } - fn wait_ready<'a>(mut self: Pin<&'a mut Self>) -> impl Future + 'a { poll_fn(move |cx| { self.as_mut().inner().with(|s, _irq| { diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index 61d415e02..4a3adeb3d 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -120,11 +120,6 @@ impl Spim { fn inner(self: Pin<&mut Self>) -> Pin<&mut PeripheralMutex>> { unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) } } - - pub fn free(self: Pin<&mut Self>) -> (T, T::Interrupt) { - let (state, irq) = self.inner().free(); - (state.spim, irq) - } } impl FullDuplex for Spim { From b57489eb5d27075e077b955b295cc97bf2fc312f Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 18 Mar 2021 02:29:03 +0100 Subject: [PATCH 2/2] peripheralmutex: separate interrupt registration to own method. --- embassy-extras/src/peripheral.rs | 25 +++++++++++++------------ embassy-nrf/src/buffered_uarte.rs | 12 +++++++++--- embassy-nrf/src/spim.rs | 1 + 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/embassy-extras/src/peripheral.rs b/embassy-extras/src/peripheral.rs index aa23bc978..e2435d63f 100644 --- a/embassy-extras/src/peripheral.rs +++ b/embassy-extras/src/peripheral.rs @@ -32,32 +32,33 @@ impl PeripheralMutex { } } - /// safety: self must be pinned. - unsafe fn setup(&mut self) { - self.irq.disable(); + pub fn register_interrupt(self: Pin<&mut Self>) { + let this = unsafe { self.get_unchecked_mut() }; + if this.irq_setup_done { + return; + } + + this.irq.disable(); compiler_fence(Ordering::SeqCst); - self.irq.set_handler(|p| { + this.irq.set_handler(|p| { // Safety: it's OK to get a &mut to the state, since // - We're in the IRQ, no one else can't preempt us // - We can't have preempted a with() call because the irq is disabled during it. - let state = &mut *(p as *mut S); + let state = unsafe { &mut *(p as *mut S) }; state.on_interrupt(); }); - self.irq - .set_handler_context((&mut self.state) as *mut _ as *mut ()); + this.irq + .set_handler_context((&mut this.state) as *mut _ as *mut ()); compiler_fence(Ordering::SeqCst); - self.irq.enable(); + this.irq.enable(); - self.irq_setup_done = true; + this.irq_setup_done = true; } pub fn with(self: Pin<&mut Self>, f: impl FnOnce(&mut S, &mut S::Interrupt) -> R) -> R { let this = unsafe { self.get_unchecked_mut() }; - if !this.irq_setup_done { - unsafe { this.setup() } - } this.irq.disable(); compiler_fence(Ordering::SeqCst); diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 9b4ec6061..6cc5f1322 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -207,7 +207,9 @@ impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi for BufferedUarte<'a, U, T, P1, P2> { fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.inner().with(|state, _irq| { + let mut inner = self.inner(); + inner.as_mut().register_interrupt(); + inner.with(|state, _irq| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, // before any DMA action has started @@ -230,7 +232,9 @@ impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi } fn consume(self: Pin<&mut Self>, amt: usize) { - self.inner().with(|state, irq| { + let mut inner = self.inner(); + inner.as_mut().register_interrupt(); + inner.with(|state, irq| { trace!("consume {:?}", amt); state.rx.pop(amt); irq.pend(); @@ -242,7 +246,9 @@ impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi for BufferedUarte<'a, U, T, P1, P2> { fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { - self.inner().with(|state, irq| { + let mut inner = self.inner(); + inner.as_mut().register_interrupt(); + inner.with(|state, irq| { trace!("poll_write: {:?}", buf.len()); let tx_buf = state.tx.push_buf(); diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index 4a3adeb3d..ef89afbc6 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -148,6 +148,7 @@ impl FullDuplex for Spim { slice_in_ram_or(rx, Error::DMABufferNotInDataMemory)?; slice_in_ram_or(tx, Error::DMABufferNotInDataMemory)?; + self.as_mut().inner().register_interrupt(); self.as_mut().inner().with(|s, _irq| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here,