From 5b10ac9cacd4044c53eb89dea6a4d01d8e7f04e8 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Wed, 6 Jan 2021 23:36:46 +0100 Subject: [PATCH] Add PPI+TIMER to buffered_uarte to prevent IRQ storm --- embassy-nrf-examples/src/bin/buffered_uart.rs | 6 + embassy-nrf/src/buffered_uarte.rs | 181 ++++++++++-------- 2 files changed, 106 insertions(+), 81 deletions(-) diff --git a/embassy-nrf-examples/src/bin/buffered_uart.rs b/embassy-nrf-examples/src/bin/buffered_uart.rs index 68a76f71..57c6b4cf 100644 --- a/embassy-nrf-examples/src/bin/buffered_uart.rs +++ b/embassy-nrf-examples/src/bin/buffered_uart.rs @@ -8,6 +8,7 @@ use example_common::*; use cortex_m_rt::entry; use defmt::panic; +use nrf52840_hal as hal; use nrf52840_hal::gpio; use embassy::executor::{task, Executor}; @@ -35,9 +36,14 @@ async fn run() { rts: None, }; + let ppi = hal::ppi::Parts::new(p.PPI); + let irq = interrupt::take!(UARTE0_UART0); let mut u = buffered_uarte::BufferedUarte::new( p.UARTE0, + p.TIMER0, + ppi.ppi0, + ppi.ppi1, irq, unsafe { &mut RX_BUFFER }, unsafe { &mut TX_BUFFER }, diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index ceb52916..fac3703a 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -16,22 +16,20 @@ use embedded_hal::digital::v2::OutputPin; use crate::fmt::{panic, todo, *}; use crate::hal::gpio::Port as GpioPort; +use crate::hal::ppi::ConfigurablePpi; use crate::interrupt::{self, OwnedInterrupt}; use crate::pac; -use crate::pac::uarte0; use crate::util::peripheral::{PeripheralMutex, PeripheralState}; use crate::util::ring_buffer::RingBuffer; // Re-export SVD variants to allow user to directly set values pub use crate::hal::uarte::Pins; -pub use uarte0::{baudrate::BAUDRATE_A as Baudrate, config::PARITY_A as Parity}; +pub use pac::uarte0::{baudrate::BAUDRATE_A as Baudrate, config::PARITY_A as Parity}; #[derive(Copy, Clone, Debug, PartialEq)] enum RxState { Idle, Receiving, - ReceivingReady, - Stopping, } #[derive(Copy, Clone, Debug, PartialEq)] @@ -40,8 +38,11 @@ enum TxState { Transmitting(usize), } -struct State<'a, U: Instance> { - inner: U, +struct State<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> { + uarte: U, + timer: T, + ppi_channel_1: P1, + ppi_channel_2: P2, rx: RingBuffer<'a>, rx_state: RxState, @@ -60,12 +61,16 @@ struct State<'a, U: Instance> { /// are disabled before using `Uarte`. See product specification: /// - nrf52832: Section 15.2 /// - nrf52840: Section 6.1.2 -pub struct BufferedUarte<'a, U: Instance> { - inner: PeripheralMutex>, +pub struct BufferedUarte< + 'a, + U: Instance, + T: TimerInstance, + P1: ConfigurablePpi, + P2: ConfigurablePpi, +> { + inner: PeripheralMutex>, } -impl<'a, U: Instance> Unpin for BufferedUarte<'a, U> {} - #[cfg(any(feature = "52833", feature = "52840"))] fn port_bit(port: GpioPort) -> bool { match port { @@ -74,9 +79,14 @@ fn port_bit(port: GpioPort) -> bool { } } -impl<'a, U: Instance> BufferedUarte<'a, U> { +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> + BufferedUarte<'a, U, T, P1, P2> +{ pub fn new( uarte: U, + timer: T, + mut ppi_channel_1: P1, + mut ppi_channel_2: P2, irq: U::Interrupt, rx_buffer: &'a mut [u8], tx_buffer: &'a mut [u8], @@ -141,10 +151,41 @@ impl<'a, U: Instance> BufferedUarte<'a, U> { irq.disable(); irq.pend(); + // BAUDRATE register values are `baudrate * 2^32 / 16000000` + // source: https://devzone.nordicsemi.com/f/nordic-q-a/391/uart-baudrate-register-values + // + // We want to stop RX if line is idle for 2 bytes worth of time + // That is 20 bits (each byte is 1 start bit + 8 data bits + 1 stop bit) + // This gives us the amount of 16M ticks for 20 bits. + let timeout = 0x8000_0000 / (baudrate as u32 / 40); + + timer.tasks_stop.write(|w| w.tasks_stop().set_bit()); + timer.bitmode.write(|w| w.bitmode()._32bit()); + timer.prescaler.write(|w| unsafe { w.prescaler().bits(0) }); + timer.cc[0].write(|w| unsafe { w.bits(timeout) }); + timer.mode.write(|w| w.mode().timer()); + timer.shorts.write(|w| { + w.compare0_clear().set_bit(); + w.compare0_stop().set_bit(); + w + }); + + ppi_channel_1.set_event_endpoint(&uarte.events_rxdrdy); + ppi_channel_1.set_task_endpoint(&timer.tasks_clear); + ppi_channel_1.set_fork_task_endpoint(&timer.tasks_start); + ppi_channel_1.enable(); + + ppi_channel_2.set_event_endpoint(&timer.events_compare[0]); + ppi_channel_2.set_task_endpoint(&uarte.tasks_stoprx); + ppi_channel_2.enable(); + BufferedUarte { inner: PeripheralMutex::new( State { - inner: uarte, + uarte, + timer, + ppi_channel_1, + ppi_channel_2, rx: RingBuffer::new(rx_buffer), rx_state: RxState::Idle, @@ -159,19 +200,23 @@ impl<'a, U: Instance> BufferedUarte<'a, U> { } } - fn inner(self: Pin<&mut Self>) -> Pin<&mut PeripheralMutex>> { + fn inner(self: Pin<&mut Self>) -> Pin<&mut PeripheralMutex>> { unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) } } } -impl<'a, U: Instance> Drop for BufferedUarte<'a, U> { +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> Drop + for BufferedUarte<'a, U, T, P1, P2> +{ fn drop(&mut self) { // stop DMA before dropping, because DMA is using the buffer in `self`. todo!() } } -impl<'a, U: Instance> AsyncBufRead for BufferedUarte<'a, U> { +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> AsyncBufRead + for BufferedUarte<'a, U, T, P1, P2> +{ fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { self.inner().with(|state, _irq| { // Conservative compiler fence to prevent optimizations that do not @@ -190,13 +235,6 @@ impl<'a, U: Instance> AsyncBufRead for BufferedUarte<'a, U> { } trace!(" empty"); - - if state.rx_state == RxState::ReceivingReady { - trace!(" stopping"); - state.rx_state = RxState::Stopping; - state.inner.tasks_stoprx.write(|w| unsafe { w.bits(1) }); - } - state.rx_waker.register(cx.waker()); Poll::>::Pending }) @@ -211,7 +249,9 @@ impl<'a, U: Instance> AsyncBufRead for BufferedUarte<'a, U> { } } -impl<'a, U: Instance> AsyncWrite for BufferedUarte<'a, U> { +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> AsyncWrite + 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| { trace!("poll_write: {:?}", buf.len()); @@ -241,38 +281,28 @@ impl<'a, U: Instance> AsyncWrite for BufferedUarte<'a, U> { } } -impl<'a, U: Instance> PeripheralState for State<'a, U> { +impl<'a, U: Instance, T: TimerInstance, P1: ConfigurablePpi, P2: ConfigurablePpi> PeripheralState + for State<'a, U, T, P1, P2> +{ type Interrupt = U::Interrupt; - fn on_interrupt(&mut self) { trace!("irq: start"); - let mut more_work = true; - while more_work { - more_work = false; + loop { match self.rx_state { RxState::Idle => { trace!(" irq_rx: in state idle"); - if self.inner.events_rxdrdy.read().bits() != 0 { - trace!(" irq_rx: rxdrdy?????"); - self.inner.events_rxdrdy.reset(); - } - - if self.inner.events_endrx.read().bits() != 0 { - panic!("unexpected endrx"); - } - let buf = self.rx.push_buf(); if buf.len() != 0 { trace!(" irq_rx: starting {:?}", buf.len()); self.rx_state = RxState::Receiving; // Set up the DMA read - self.inner.rxd.ptr.write(|w| + self.uarte.rxd.ptr.write(|w| // The PTR field is a full 32 bits wide and accepts the full range // of values. unsafe { w.ptr().bits(buf.as_ptr() as u32) }); - self.inner.rxd.maxcnt.write(|w| + self.uarte.rxd.maxcnt.write(|w| // We're giving it the length of the buffer, so no danger of // accessing invalid memory. We have verified that the length of the // buffer fits in an `u8`, so the cast to `u8` is also fine. @@ -282,60 +312,34 @@ impl<'a, U: Instance> PeripheralState for State<'a, U> { unsafe { w.maxcnt().bits(buf.len() as _) }); trace!(" irq_rx: buf {:?} {:?}", buf.as_ptr() as u32, buf.len()); - // Enable RXRDY interrupt. - self.inner.events_rxdrdy.reset(); - self.inner.intenset.write(|w| w.rxdrdy().set()); - // Start UARTE Receive transaction - self.inner.tasks_startrx.write(|w| + self.uarte.tasks_startrx.write(|w| // `1` is a valid value to write to task registers. unsafe { w.bits(1) }); } + break; } RxState::Receiving => { trace!(" irq_rx: in state receiving"); - if self.inner.events_rxdrdy.read().bits() != 0 { - trace!(" irq_rx: rxdrdy"); + if self.uarte.events_endrx.read().bits() != 0 { + self.timer.tasks_stop.write(|w| w.tasks_stop().set_bit()); - // Disable the RXRDY event interrupt - // RXRDY is triggered for every byte, but we only care about whether we have - // some bytes or not. So as soon as we have at least one, disable it, to avoid - // wasting CPU cycles in interrupts. - self.inner.intenclr.write(|w| w.rxdrdy().clear()); - - self.inner.events_rxdrdy.reset(); - - self.rx_waker.wake(); - self.rx_state = RxState::ReceivingReady; - more_work = true; // in case we also have endrx pending - } - } - RxState::ReceivingReady | RxState::Stopping => { - trace!(" irq_rx: in state ReceivingReady"); - - if self.inner.events_rxdrdy.read().bits() != 0 { - trace!(" irq_rx: rxdrdy"); - self.inner.events_rxdrdy.reset(); - } - - if self.inner.events_endrx.read().bits() != 0 { - let n: usize = self.inner.rxd.amount.read().amount().bits() as usize; + let n: usize = self.uarte.rxd.amount.read().amount().bits() as usize; trace!(" irq_rx: endrx {:?}", n); self.rx.push(n); - self.inner.events_endrx.reset(); + self.uarte.events_endrx.reset(); self.rx_waker.wake(); self.rx_state = RxState::Idle; - more_work = true; // start another rx if possible + } else { + break; } } } } - more_work = true; - while more_work { - more_work = false; + loop { match self.tx_state { TxState::Idle => { trace!(" irq_tx: in state Idle"); @@ -345,11 +349,11 @@ impl<'a, U: Instance> PeripheralState for State<'a, U> { self.tx_state = TxState::Transmitting(buf.len()); // Set up the DMA write - self.inner.txd.ptr.write(|w| + self.uarte.txd.ptr.write(|w| // The PTR field is a full 32 bits wide and accepts the full range // of values. unsafe { w.ptr().bits(buf.as_ptr() as u32) }); - self.inner.txd.maxcnt.write(|w| + self.uarte.txd.maxcnt.write(|w| // We're giving it the length of the buffer, so no danger of // accessing invalid memory. We have verified that the length of the // buffer fits in an `u8`, so the cast to `u8` is also fine. @@ -359,21 +363,23 @@ impl<'a, U: Instance> PeripheralState for State<'a, U> { unsafe { w.maxcnt().bits(buf.len() as _) }); // Start UARTE Transmit transaction - self.inner.tasks_starttx.write(|w| + self.uarte.tasks_starttx.write(|w| // `1` is a valid value to write to task registers. unsafe { w.bits(1) }); } + break; } TxState::Transmitting(n) => { trace!(" irq_tx: in state Transmitting"); - if self.inner.events_endtx.read().bits() != 0 { - self.inner.events_endtx.reset(); + if self.uarte.events_endtx.read().bits() != 0 { + self.uarte.events_endtx.reset(); trace!(" irq_tx: endtx {:?}", n); self.tx.pop(n); self.tx_waker.wake(); self.tx_state = TxState::Idle; - more_work = true; // start another tx if possible + } else { + break; } } } @@ -388,9 +394,14 @@ mod sealed { impl Instance for crate::pac::UARTE0 {} #[cfg(any(feature = "52833", feature = "52840", feature = "9160"))] impl Instance for crate::pac::UARTE1 {} + + pub trait TimerInstance {} + impl TimerInstance for crate::pac::TIMER0 {} + impl TimerInstance for crate::pac::TIMER1 {} + impl TimerInstance for crate::pac::TIMER2 {} } -pub trait Instance: Deref + sealed::Instance { +pub trait Instance: Deref + sealed::Instance { type Interrupt: OwnedInterrupt; } @@ -402,3 +413,11 @@ impl Instance for pac::UARTE0 { impl Instance for pac::UARTE1 { type Interrupt = interrupt::UARTE1Interrupt; } + +pub trait TimerInstance: + Deref + sealed::TimerInstance +{ +} +impl TimerInstance for crate::pac::TIMER0 {} +impl TimerInstance for crate::pac::TIMER1 {} +impl TimerInstance for crate::pac::TIMER2 {}