From 1c4d450a2d21161c7e9220716839c4a4bfd0ff5b Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Wed, 6 Jan 2021 13:48:26 +0100 Subject: [PATCH 01/21] Add embedded_hal::serial implementation for uarte This adds a sub-module named "serial" that splits TX and RX parts of UART into separate types that implements the embedded_hal::serial interfaces. This allows using the nRF UART for drivers that rely on the embedded_hal::serial traits. The buffer management is primitive in order to support the semantics of the more restrictive embedded_hal traits. --- nrf-hal-common/src/uarte.rs | 235 +++++++++++++++++++++++++++++++++++- 1 file changed, 232 insertions(+), 3 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 5aa26477..cac723bd 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -413,6 +413,13 @@ where }, ) } + + // Split into implementations of embedded_hal::serial traits + pub fn split(self) -> (UarteTx, UarteRx) { + let tx = UarteTx::new(); + let rx = UarteRx::new(); + (tx, rx) + } } impl fmt::Write for Uarte @@ -449,18 +456,240 @@ pub enum Error { BufferNotInRAM, } -pub trait Instance: Deref + sealed::Sealed {} +pub trait Instance: Deref + sealed::Sealed { + fn ptr() -> *const uarte0::RegisterBlock; +} mod sealed { pub trait Sealed {} } impl sealed::Sealed for UARTE0 {} -impl Instance for UARTE0 {} +impl Instance for UARTE0 { + fn ptr() -> *const uarte0::RegisterBlock { + UARTE0::ptr() + } +} #[cfg(any(feature = "52833", feature = "52840", feature = "9160"))] mod _uarte1 { use super::*; impl sealed::Sealed for UARTE1 {} - impl Instance for UARTE1 {} + impl Instance for UARTE1 { + fn ptr() -> *const uarte0::RegisterBlock { + UARTE1::ptr() + } + } +} + +/// Interface for the TX part of a UART instance that can be used independently of the RX part. +pub struct UarteTx { + _marker: core::marker::PhantomData, + tx_buf: [u8; 1], +} + +/// Interface for the RX part of a UART instance that can be used independently of the TX part. +pub struct UarteRx { + _marker: core::marker::PhantomData, + rx_buf: [u8; 1], +} + +impl UarteTx +where + T: Instance, +{ + fn new() -> UarteTx { + let tx = UarteTx { + _marker: core::marker::PhantomData, + tx_buf: [0; 1], + }; + tx + } +} + +impl UarteRx +where + T: Instance, +{ + fn new() -> UarteRx { + let rx = UarteRx { + _marker: core::marker::PhantomData, + rx_buf: [0; 1], + }; + rx + } +} + +pub mod serial { + + ///! Implementation of the embedded_hal::serial::* traits for UartTx and UartRx. + use super::*; + use embedded_hal::serial; + use nb; + + impl serial::Write for UarteTx + where + T: Instance, + { + type Error = Error; + + /// Write a single byte non-blocking. Returns nb::Error::WouldBlock if not yet done. + fn write(&mut self, b: u8) -> nb::Result<(), Self::Error> { + let uarte = unsafe { &*T::ptr() }; + + // If txstarted is set, we are in the process of transmitting. + let in_progress = uarte.events_txstarted.read().bits() == 1; + + if in_progress { + self.flush() + } else { + // Start a new transmission, copy value into transmit buffer. + + let tx_buffer = &mut self.tx_buf; + tx_buffer[0] = b; + + // 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. + compiler_fence(SeqCst); + + // Reset the events. + uarte.events_endtx.reset(); + uarte.events_txstopped.reset(); + + // Set up the DMA write. + // We're giving the register a pointer to the tx buffer. + // + // The PTR field is a full 32 bits wide and accepts the full range + // of values. + uarte + .txd + .ptr + .write(|w| unsafe { w.ptr().bits(tx_buffer.as_ptr() as u32) }); + + // 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. + // + // The MAXCNT field is 8 bits wide and accepts the full range of + // values. + uarte + .txd + .maxcnt + .write(|w| unsafe { w.maxcnt().bits(tx_buffer.len() as _) }); + + // Start UARTE Transmit transaction. + // `1` is a valid value to write to task registers. + uarte.tasks_starttx.write(|w| unsafe { w.bits(1) }); + Err(nb::Error::WouldBlock) + } + } + + /// Flush the TX buffer non-blocking. Returns nb::Error::WouldBlock if not yet flushed. + fn flush(&mut self) -> nb::Result<(), Self::Error> { + let uarte = unsafe { &*T::ptr() }; + + let in_progress = uarte.events_txstarted.read().bits() == 1; + let endtx = uarte.events_endtx.read().bits() != 0; + let txstopped = uarte.events_txstopped.read().bits() != 0; + if in_progress { + if endtx || txstopped { + // We are done, cleanup the state. + uarte.events_txstarted.reset(); + // Conservative compiler fence to prevent optimizations that do not + // take in to account actions by DMA. The fence has been placed here, + // after all possible DMA actions have completed. + compiler_fence(SeqCst); + + if txstopped { + return Err(nb::Error::Other(Error::Transmit)); + } + + // Lower power consumption by disabling the transmitter once we're + // finished. + // `1` is a valid value to write to task registers. + uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) }); + Ok(()) + } else { + // Still not done, don't block. + Err(nb::Error::WouldBlock) + } + } else { + Ok(()) + } + } + } + + impl core::fmt::Write for UarteTx + where + T: Instance, + { + fn write_str(&mut self, s: &str) -> fmt::Result { + s.as_bytes() + .iter() + .try_for_each(|c| nb::block!(self.write(*c))) + .map_err(|_| core::fmt::Error) + } + } + + impl serial::Read for UarteRx + where + T: Instance, + { + type Error = Error; + fn read(&mut self) -> nb::Result { + let uarte = unsafe { &*T::ptr() }; + + compiler_fence(SeqCst); + + let in_progress = uarte.events_rxstarted.read().bits() == 1; + if in_progress && uarte.events_endrx.read().bits() == 0 { + return Err(nb::Error::WouldBlock); + } + + if in_progress { + let b = self.rx_buf[0]; + uarte.events_rxstarted.write(|w| w); + uarte.events_endrx.write(|w| w); + + compiler_fence(SeqCst); + if uarte.rxd.amount.read().bits() != 1 as u32 { + return Err(nb::Error::Other(Error::Receive)); + } + Ok(b) + } else { + let rx_buf = &mut self.rx_buf; + + // We're giving the register a pointer to the rx buffer. + // + // The PTR field is a full 32 bits wide and accepts the full range + // of values. + uarte + .rxd + .ptr + .write(|w| unsafe { w.ptr().bits(rx_buf.as_ptr() as u32) }); + + // We're giving it the length of the buffer, so no danger of + // accessing invalid memory. + // + // The MAXCNT field is at least 8 bits wide and accepts the full + // range of values. + uarte + .rxd + .maxcnt + .write(|w| unsafe { w.maxcnt().bits(rx_buf.len() as _) }); + + // Start UARTE Receive transaction. + // `1` is a valid value to write to task registers. + uarte.tasks_startrx.write(|w| unsafe { w.bits(1) }); + // Conservative compiler fence to prevent optimizations that do not + // take in to account actions by DMA. The fence has been placed here, + // after all possible DMA actions have completed. + + compiler_fence(SeqCst); + + Err(nb::Error::WouldBlock) + } + } + } } From 0e01992db2865fa73c2dcf2b2c3da820aa458e1b Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 7 Jan 2021 08:20:39 +0100 Subject: [PATCH 02/21] Pass TX and RX buffers to split() to ensure move of UartTx and UartRx does not affect memory location for DMA. --- nrf-hal-common/src/uarte.rs | 91 +++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index cac723bd..7494e849 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -414,11 +414,16 @@ where ) } - // Split into implementations of embedded_hal::serial traits - pub fn split(self) -> (UarteTx, UarteRx) { - let tx = UarteTx::new(); - let rx = UarteRx::new(); - (tx, rx) + // Split into implementations of embedded_hal::serial traits. The buffers passed here must outlive any DMA transfers + // that are initiated by the UartTx and UartRx. + pub fn split<'a>( + self, + tx_buf: &'a mut [u8], + rx_buf: &'a mut [u8], + ) -> Result<(UarteTx<'a, T>, UarteRx<'a, T>), Error> { + let tx = UarteTx::new(tx_buf)?; + let rx = UarteRx::new(rx_buf)?; + Ok((tx, rx)) } } @@ -448,6 +453,8 @@ pub struct Pins { #[derive(Debug)] pub enum Error { + TxBufferTooSmall, + RxBufferTooSmall, TxBufferTooLong, RxBufferTooLong, Transmit, @@ -483,40 +490,46 @@ mod _uarte1 { } /// Interface for the TX part of a UART instance that can be used independently of the RX part. -pub struct UarteTx { +pub struct UarteTx<'a, T> { _marker: core::marker::PhantomData, - tx_buf: [u8; 1], + tx_buf: &'a mut [u8], } /// Interface for the RX part of a UART instance that can be used independently of the TX part. -pub struct UarteRx { +pub struct UarteRx<'a, T> { _marker: core::marker::PhantomData, - rx_buf: [u8; 1], + rx_buf: &'a mut [u8], } -impl UarteTx +impl<'a, T> UarteTx<'a, T> where T: Instance, { - fn new() -> UarteTx { - let tx = UarteTx { - _marker: core::marker::PhantomData, - tx_buf: [0; 1], - }; - tx + fn new(tx_buf: &'a mut [u8]) -> Result, Error> { + if tx_buf.len() > 0 { + Ok(UarteTx { + _marker: core::marker::PhantomData, + tx_buf, + }) + } else { + Err(Error::TxBufferTooSmall) + } } } -impl UarteRx +impl<'a, T> UarteRx<'a, T> where T: Instance, { - fn new() -> UarteRx { - let rx = UarteRx { - _marker: core::marker::PhantomData, - rx_buf: [0; 1], - }; - rx + fn new(rx_buf: &'a mut [u8]) -> Result, Error> { + if rx_buf.len() > 0 { + Ok(UarteRx { + _marker: core::marker::PhantomData, + rx_buf, + }) + } else { + Err(Error::RxBufferTooSmall) + } } } @@ -527,7 +540,7 @@ pub mod serial { use embedded_hal::serial; use nb; - impl serial::Write for UarteTx + impl<'a, T> serial::Write for UarteTx<'a, T> where T: Instance, { @@ -545,8 +558,7 @@ pub mod serial { } else { // Start a new transmission, copy value into transmit buffer. - let tx_buffer = &mut self.tx_buf; - tx_buffer[0] = b; + self.tx_buf[0] = b; // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, @@ -565,18 +577,13 @@ pub mod serial { uarte .txd .ptr - .write(|w| unsafe { w.ptr().bits(tx_buffer.as_ptr() as u32) }); + .write(|w| unsafe { w.ptr().bits(self.tx_buf.as_ptr() as u32) }); - // 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. + // We're giving it a length of 1 to transmit 1 byte at a time. // // The MAXCNT field is 8 bits wide and accepts the full range of // values. - uarte - .txd - .maxcnt - .write(|w| unsafe { w.maxcnt().bits(tx_buffer.len() as _) }); + uarte.txd.maxcnt.write(|w| unsafe { w.maxcnt().bits(1) }); // Start UARTE Transmit transaction. // `1` is a valid value to write to task registers. @@ -620,7 +627,7 @@ pub mod serial { } } - impl core::fmt::Write for UarteTx + impl<'a, T> core::fmt::Write for UarteTx<'a, T> where T: Instance, { @@ -632,7 +639,7 @@ pub mod serial { } } - impl serial::Read for UarteRx + impl<'a, T> serial::Read for UarteRx<'a, T> where T: Instance, { @@ -658,8 +665,6 @@ pub mod serial { } Ok(b) } else { - let rx_buf = &mut self.rx_buf; - // We're giving the register a pointer to the rx buffer. // // The PTR field is a full 32 bits wide and accepts the full range @@ -667,17 +672,13 @@ pub mod serial { uarte .rxd .ptr - .write(|w| unsafe { w.ptr().bits(rx_buf.as_ptr() as u32) }); + .write(|w| unsafe { w.ptr().bits(self.rx_buf.as_ptr() as u32) }); - // We're giving it the length of the buffer, so no danger of - // accessing invalid memory. + // We're giving it a length of 1 to read only 1 byte. // // The MAXCNT field is at least 8 bits wide and accepts the full // range of values. - uarte - .rxd - .maxcnt - .write(|w| unsafe { w.maxcnt().bits(rx_buf.len() as _) }); + uarte.rxd.maxcnt.write(|w| unsafe { w.maxcnt().bits(1) }); // Start UARTE Receive transaction. // `1` is a valid value to write to task registers. From c21a09d5cfcc5860311938491265cead4255ec92 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 7 Jan 2021 08:29:40 +0100 Subject: [PATCH 03/21] Check that buffers are in RAM --- nrf-hal-common/src/uarte.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 7494e849..0948784f 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -506,6 +506,7 @@ where T: Instance, { fn new(tx_buf: &'a mut [u8]) -> Result, Error> { + slice_in_ram_or(tx_buf, Error::BufferNotInRAM)?; if tx_buf.len() > 0 { Ok(UarteTx { _marker: core::marker::PhantomData, @@ -522,6 +523,7 @@ where T: Instance, { fn new(rx_buf: &'a mut [u8]) -> Result, Error> { + slice_in_ram_or(rx_buf, Error::BufferNotInRAM)?; if rx_buf.len() > 0 { Ok(UarteRx { _marker: core::marker::PhantomData, From 4f6c7755c480c8c32e61d9a7ccf79f5bac45ad3c Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 7 Jan 2021 08:33:02 +0100 Subject: [PATCH 04/21] Implement Drop trait to ensure DMA is stopped --- nrf-hal-common/src/uarte.rs | 67 +++++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 0948784f..246c1805 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -490,13 +490,19 @@ mod _uarte1 { } /// Interface for the TX part of a UART instance that can be used independently of the RX part. -pub struct UarteTx<'a, T> { +pub struct UarteTx<'a, T> +where + T: Instance, +{ _marker: core::marker::PhantomData, tx_buf: &'a mut [u8], } /// Interface for the RX part of a UART instance that can be used independently of the TX part. -pub struct UarteRx<'a, T> { +pub struct UarteRx<'a, T> +where + T: Instance, +{ _marker: core::marker::PhantomData, rx_buf: &'a mut [u8], } @@ -535,6 +541,63 @@ where } } +impl<'a, T> Drop for UarteTx<'a, T> +where + T: Instance, +{ + fn drop(&mut self) { + let uarte = unsafe { &*T::ptr() }; + + let in_progress = uarte.events_txstarted.read().bits() == 1; + // Stop any ongoing transmission + if in_progress { + uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) }); + + // Wait for transmitter is stopped. + while uarte.events_txstopped.read().bits() == 0 {} + + // Reset events + uarte.events_endtx.reset(); + uarte.events_txstopped.reset(); + + // Ensure the above is done + compiler_fence(SeqCst); + } + } +} + +impl<'a, T> Drop for UarteRx<'a, T> +where + T: Instance, +{ + fn drop(&mut self) { + let uarte = unsafe { &*T::ptr() }; + + let in_progress = uarte.events_rxstarted.read().bits() == 1; + // Stop any ongoing reception + if in_progress { + uarte.tasks_stoprx.write(|w| unsafe { w.bits(1) }); + + // Wait for receive to be done to ensure memory is untouched. + while uarte.events_rxto.read().bits() == 0 {} + + uarte.events_rxto.reset(); + + // Flush DMA + uarte.tasks_flushrx.write(|w| unsafe { w.bits(1) }); + + // Wait for the flush to complete. + while uarte.events_endrx.read().bits() == 0 {} + + // Reset events + uarte.events_endrx.reset(); + + // Ensure the above is done + compiler_fence(SeqCst); + } + } +} + pub mod serial { ///! Implementation of the embedded_hal::serial::* traits for UartTx and UartRx. From 2ab223ae41394e9fb75996b15622ceaeb4fe6df0 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Mon, 11 Jan 2021 10:11:28 +0100 Subject: [PATCH 05/21] Require DMA buffers to have static lifetime * Allow making multi-byte DMA transfers by starting DMA only on flush(). Read is still limited by the embedded_hal trait, so will still read only 1 byte at a time. * Auto derive blocking trait from non-blocking version. --- nrf-hal-common/src/uarte.rs | 136 +++++++++++++++++++++--------------- 1 file changed, 78 insertions(+), 58 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 246c1805..78d268f9 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -414,12 +414,12 @@ where ) } - // Split into implementations of embedded_hal::serial traits. The buffers passed here must outlive any DMA transfers + // Split into implementations of embedded_hal::serial traits. The size of the slices passed to this method will determine the size of the DMA transfers performed. // that are initiated by the UartTx and UartRx. pub fn split<'a>( self, - tx_buf: &'a mut [u8], - rx_buf: &'a mut [u8], + tx_buf: &'static mut [u8], + rx_buf: &'static mut [u8], ) -> Result<(UarteTx<'a, T>, UarteRx<'a, T>), Error> { let tx = UarteTx::new(tx_buf)?; let rx = UarteRx::new(rx_buf)?; @@ -496,6 +496,7 @@ where { _marker: core::marker::PhantomData, tx_buf: &'a mut [u8], + written: u16, } /// Interface for the RX part of a UART instance that can be used independently of the TX part. @@ -513,14 +514,19 @@ where { fn new(tx_buf: &'a mut [u8]) -> Result, Error> { slice_in_ram_or(tx_buf, Error::BufferNotInRAM)?; - if tx_buf.len() > 0 { - Ok(UarteTx { - _marker: core::marker::PhantomData, - tx_buf, - }) - } else { - Err(Error::TxBufferTooSmall) + if tx_buf.len() == 0 { + return Err(Error::TxBufferTooSmall); } + + if tx_buf.len() > EASY_DMA_SIZE { + return Err(Error::TxBufferTooLong); + } + + Ok(UarteTx { + _marker: core::marker::PhantomData, + tx_buf, + written: 0, + }) } } @@ -530,14 +536,18 @@ where { fn new(rx_buf: &'a mut [u8]) -> Result, Error> { slice_in_ram_or(rx_buf, Error::BufferNotInRAM)?; - if rx_buf.len() > 0 { - Ok(UarteRx { - _marker: core::marker::PhantomData, - rx_buf, - }) - } else { - Err(Error::RxBufferTooSmall) + if rx_buf.len() == 0 { + return Err(Error::RxBufferTooSmall); + } + + if rx_buf.len() > EASY_DMA_SIZE { + return Err(Error::RxBufferTooLong); } + + Ok(UarteRx { + _marker: core::marker::PhantomData, + rx_buf, + }) } } @@ -600,8 +610,9 @@ where pub mod serial { - ///! Implementation of the embedded_hal::serial::* traits for UartTx and UartRx. + ///! Implementation of the embedded_hal::serial::* and embedded_hal::blocking::serial::* traits for UartTx and UartRx. use super::*; + use embedded_hal::blocking::serial as bserial; use embedded_hal::serial; use nb; @@ -611,48 +622,21 @@ pub mod serial { { type Error = Error; - /// Write a single byte non-blocking. Returns nb::Error::WouldBlock if not yet done. + /// Write a single byte to the internal buffer. Returns nb::Error::WouldBlock if buffer is full. fn write(&mut self, b: u8) -> nb::Result<(), Self::Error> { let uarte = unsafe { &*T::ptr() }; - // If txstarted is set, we are in the process of transmitting. - let in_progress = uarte.events_txstarted.read().bits() == 1; + // Prevent writing to buffer while DMA transfer is in progress. + if uarte.events_txstarted.read().bits() == 1 { + return Err(nb::Error::WouldBlock); + } - if in_progress { - self.flush() + let written = self.written as usize; + if written < self.tx_buf.len() { + self.tx_buf[written] = b; + self.written += 1; + Ok(()) } else { - // Start a new transmission, copy value into transmit buffer. - - self.tx_buf[0] = b; - - // 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. - compiler_fence(SeqCst); - - // Reset the events. - uarte.events_endtx.reset(); - uarte.events_txstopped.reset(); - - // Set up the DMA write. - // We're giving the register a pointer to the tx buffer. - // - // The PTR field is a full 32 bits wide and accepts the full range - // of values. - uarte - .txd - .ptr - .write(|w| unsafe { w.ptr().bits(self.tx_buf.as_ptr() as u32) }); - - // We're giving it a length of 1 to transmit 1 byte at a time. - // - // The MAXCNT field is 8 bits wide and accepts the full range of - // values. - uarte.txd.maxcnt.write(|w| unsafe { w.maxcnt().bits(1) }); - - // Start UARTE Transmit transaction. - // `1` is a valid value to write to task registers. - uarte.tasks_starttx.write(|w| unsafe { w.bits(1) }); Err(nb::Error::WouldBlock) } } @@ -661,10 +645,12 @@ pub mod serial { fn flush(&mut self) -> nb::Result<(), Self::Error> { let uarte = unsafe { &*T::ptr() }; + // If txstarted is set, we are in the process of transmitting. let in_progress = uarte.events_txstarted.read().bits() == 1; - let endtx = uarte.events_endtx.read().bits() != 0; - let txstopped = uarte.events_txstopped.read().bits() != 0; + if in_progress { + let endtx = uarte.events_endtx.read().bits() != 0; + let txstopped = uarte.events_txstopped.read().bits() != 0; if endtx || txstopped { // We are done, cleanup the state. uarte.events_txstarted.reset(); @@ -687,11 +673,45 @@ pub mod serial { Err(nb::Error::WouldBlock) } } else { - Ok(()) + // 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. + compiler_fence(SeqCst); + + // Reset the events. + uarte.events_endtx.reset(); + uarte.events_txstopped.reset(); + + // Set up the DMA write. + // We're giving the register a pointer to the tx buffer. + // + // The PTR field is a full 32 bits wide and accepts the full range + // of values. + uarte + .txd + .ptr + .write(|w| unsafe { w.ptr().bits(self.tx_buf.as_ptr() as u32) }); + + // We're giving it a length of the number of bytes written to the buffer. + // + // The MAXCNT field is 8 bits wide and accepts the full range of + // values. + uarte + .txd + .maxcnt + .write(|w| unsafe { w.maxcnt().bits(self.written) }); + + // Start UARTE Transmit transaction. + // `1` is a valid value to write to task registers. + uarte.tasks_starttx.write(|w| unsafe { w.bits(1) }); + Err(nb::Error::WouldBlock) } } } + // Auto-implement the blocking variant + impl<'a, T> bserial::write::Default for UarteTx<'a, T> where T: Instance {} + impl<'a, T> core::fmt::Write for UarteTx<'a, T> where T: Instance, From 28d40d724e9be303e10ec49783f28e1ab2ed7b6c Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Mon, 11 Jan 2021 10:21:25 +0100 Subject: [PATCH 06/21] Ensure write pointer is reset --- nrf-hal-common/src/uarte.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 78d268f9..87edfd3b 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -654,6 +654,7 @@ pub mod serial { if endtx || txstopped { // We are done, cleanup the state. uarte.events_txstarted.reset(); + self.written = 0; // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, // after all possible DMA actions have completed. From bbd43ac902e7959e180e98ffbed77fed3c29025d Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Mon, 11 Jan 2021 10:39:54 +0100 Subject: [PATCH 07/21] Avoid initiating transfer if no bytes have been written --- nrf-hal-common/src/uarte.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 87edfd3b..05851e8d 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -674,6 +674,11 @@ pub mod serial { Err(nb::Error::WouldBlock) } } else { + // No need to trigger transmit if we don't have anything written + if self.written == 0 { + return Ok(()); + } + // 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. From 53946624722ef1a4f84c4b4433aa042395388412 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Mon, 11 Jan 2021 10:46:25 +0100 Subject: [PATCH 08/21] Fix conversion --- nrf-hal-common/src/uarte.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 05851e8d..b48d0917 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -496,7 +496,7 @@ where { _marker: core::marker::PhantomData, tx_buf: &'a mut [u8], - written: u16, + written: usize, } /// Interface for the RX part of a UART instance that can be used independently of the TX part. @@ -631,9 +631,8 @@ pub mod serial { return Err(nb::Error::WouldBlock); } - let written = self.written as usize; - if written < self.tx_buf.len() { - self.tx_buf[written] = b; + if self.written < self.tx_buf.len() { + self.tx_buf[self.written] = b; self.written += 1; Ok(()) } else { @@ -705,7 +704,7 @@ pub mod serial { uarte .txd .maxcnt - .write(|w| unsafe { w.maxcnt().bits(self.written) }); + .write(|w| unsafe { w.maxcnt().bits(self.written as _) }); // Start UARTE Transmit transaction. // `1` is a valid value to write to task registers. From 6466c4f2311b9347ad5b5752917e75a620ab7154 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 11:50:53 +0100 Subject: [PATCH 09/21] Address review comments * Remove explicit lifetime for tx/rx structs * Refactor write/read logic into functions that are reused in the existing combined Uarte implementation and the UartTx/UarteRx implementation. --- nrf-hal-common/src/uarte.rs | 368 +++++++++++++++--------------------- 1 file changed, 153 insertions(+), 215 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index b48d0917..ee618107 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -174,39 +174,8 @@ where // We can only DMA out of RAM. slice_in_ram_or(tx_buffer, Error::BufferNotInRAM)?; - // 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. - compiler_fence(SeqCst); - - - // Set up the DMA write. - self.0.txd.ptr.write(|w| - // We're giving the register a pointer to the stack. Since we're - // waiting for the UARTE transaction to end before this stack pointer - // becomes invalid, there's nothing wrong here. - // - // The PTR field is a full 32 bits wide and accepts the full range - // of values. - unsafe { w.ptr().bits(tx_buffer.as_ptr() as u32) }); - self.0.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. - // - // The MAXCNT field is 8 bits wide and accepts the full range of - // values. - unsafe { w.maxcnt().bits(tx_buffer.len() as _) }); - - // Reset the event - self.0.events_endtx.reset(); - - // Start UARTE Transmit transaction. - self.0.tasks_starttx.write(|w| - // `1` is a valid value to write to task registers. - unsafe { w.bits(1) }); + start_write(&*self.0, tx_buffer); - // Wait for transmission to end. while self.0.events_endtx.read().bits() == 0 { // TODO: Do something here which uses less power. Like `wfi`. @@ -220,17 +189,7 @@ where // Reset the event self.0.events_txstopped.reset(); - // Lower power consumption by disabling the transmitter once we're - // finished. - self.0.tasks_stoptx.write(|w| - // `1` is a valid value to write to task registers. - unsafe { w.bits(1) }); - - // Wait for transmitter to stop. - while self.0.events_txstopped.read().bits() == 0 { - // Spin - } - + stop_write(&*self.0); Ok(()) } @@ -241,12 +200,12 @@ where /// /// The buffer must have a length of at most 255 bytes. pub fn read(&mut self, rx_buffer: &mut [u8]) -> Result<(), Error> { - self.start_read(rx_buffer)?; + start_read(&*self.0, rx_buffer)?; // Wait for transmission to end. while self.0.events_endrx.read().bits() == 0 {} - self.finalize_read(); + finalize_read(&*self.0); if self.0.rxd.amount.read().bits() != rx_buffer.len() as u32 { return Err(Error::Receive); @@ -279,7 +238,7 @@ where I: timer::Instance, { // Start the read. - self.start_read(rx_buffer)?; + start_read(&self.0, rx_buffer)?; // Start the timeout timer. timer.start(cycles); @@ -298,11 +257,11 @@ where if !event_complete { // Cancel the reception if it did not complete until now. - self.cancel_read(); + cancel_read(&self.0); } // Cleanup, even in the error case. - self.finalize_read(); + finalize_read(&self.0); let bytes_read = self.0.rxd.amount.read().bits() as usize; @@ -317,78 +276,6 @@ where Ok(()) } - /// Start a UARTE read transaction by setting the control - /// values and triggering a read task. - fn start_read(&mut self, rx_buffer: &mut [u8]) -> Result<(), Error> { - if rx_buffer.len() > EASY_DMA_SIZE { - return Err(Error::RxBufferTooLong); - } - - // NOTE: RAM slice check is not necessary, as a mutable slice can only be - // built from data located in RAM. - - // 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. - compiler_fence(SeqCst); - - // Set up the DMA read - self.0.rxd.ptr.write(|w| - // We're giving the register a pointer to the stack. Since we're - // waiting for the UARTE transaction to end before this stack pointer - // becomes invalid, there's nothing wrong here. - // - // The PTR field is a full 32 bits wide and accepts the full range - // of values. - unsafe { w.ptr().bits(rx_buffer.as_ptr() as u32) }); - self.0.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. - // - // The MAXCNT field is at least 8 bits wide and accepts the full - // range of values. - unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); - - // Start UARTE Receive transaction. - self.0.tasks_startrx.write(|w| - // `1` is a valid value to write to task registers. - unsafe { w.bits(1) }); - - Ok(()) - } - - /// Finalize a UARTE read transaction by clearing the event. - fn finalize_read(&mut self) { - // Reset the event, otherwise it will always read `1` from now on. - self.0.events_endrx.write(|w| w); - - // Conservative compiler fence to prevent optimizations that do not - // take in to account actions by DMA. The fence has been placed here, - // after all possible DMA actions have completed. - compiler_fence(SeqCst); - } - - /// Stop an unfinished UART read transaction and flush FIFO to DMA buffer. - fn cancel_read(&mut self) { - // Stop reception. - self.0.tasks_stoprx.write(|w| unsafe { w.bits(1) }); - - // Wait for the reception to have stopped. - while self.0.events_rxto.read().bits() == 0 {} - - // Reset the event flag. - self.0.events_rxto.write(|w| w); - - // Ask UART to flush FIFO to DMA buffer. - self.0.tasks_flushrx.write(|w| unsafe { w.bits(1) }); - - // Wait for the flush to complete. - while self.0.events_endrx.read().bits() == 0 {} - - // The event flag itself is later reset by `finalize_read`. - } - /// Return the raw interface to the underlying UARTE peripheral. pub fn free(self) -> (T, Pins) { let rxd = self.0.psel.rxd.read(); @@ -414,19 +301,136 @@ where ) } - // Split into implementations of embedded_hal::serial traits. The size of the slices passed to this method will determine the size of the DMA transfers performed. - // that are initiated by the UartTx and UartRx. - pub fn split<'a>( + /// Split into implementations of embedded_hal::serial traits. The size of the slices passed to this + /// method will determine the size of the DMA transfers performed. + pub fn split( self, tx_buf: &'static mut [u8], rx_buf: &'static mut [u8], - ) -> Result<(UarteTx<'a, T>, UarteRx<'a, T>), Error> { + ) -> Result<(UarteTx, UarteRx), Error> { let tx = UarteTx::new(tx_buf)?; let rx = UarteRx::new(rx_buf)?; Ok((tx, rx)) } } +/// Write via UARTE. +/// +/// This method uses transmits all bytes in `tx_buffer`. +fn start_write(uarte: &uarte0::RegisterBlock, tx_buffer: &[u8]) { + // 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. + compiler_fence(SeqCst); + + // Reset the events. + uarte.events_endtx.reset(); + uarte.events_txstopped.reset(); + + // Set up the DMA write. + uarte.txd.ptr.write(|w| + // We're giving the register a pointer to the stack. Since we're + // waiting for the UARTE transaction to end before this stack pointer + // becomes invalid, there's nothing wrong here. + // + // The PTR field is a full 32 bits wide and accepts the full range + // of values. + unsafe { w.ptr().bits(tx_buffer.as_ptr() as u32) }); + 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. + // + // The MAXCNT field is 8 bits wide and accepts the full range of + // values. + unsafe { w.maxcnt().bits(tx_buffer.len() as _) }); + + // Start UARTE Transmit transaction. + uarte.tasks_starttx.write(|w| + // `1` is a valid value to write to task registers. + unsafe { w.bits(1) }); +} + +fn stop_write(uarte: &uarte0::RegisterBlock) { + // `1` is a valid value to write to task registers. + uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) }); + + // Wait for transmitter is stopped. + while uarte.events_txstopped.read().bits() == 0 {} +} + +/// Start a UARTE read transaction by setting the control +/// values and triggering a read task. +fn start_read(uarte: &uarte0::RegisterBlock, rx_buffer: &mut [u8]) -> Result<(), Error> { + if rx_buffer.len() > EASY_DMA_SIZE { + return Err(Error::RxBufferTooLong); + } + + // NOTE: RAM slice check is not necessary, as a mutable slice can only be + // built from data located in RAM. + + // 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. + compiler_fence(SeqCst); + + // Set up the DMA read + uarte.rxd.ptr.write(|w| + // We're giving the register a pointer to the stack. Since we're + // waiting for the UARTE transaction to end before this stack pointer + // becomes invalid, there's nothing wrong here. + // + // The PTR field is a full 32 bits wide and accepts the full range + // of values. + unsafe { w.ptr().bits(rx_buffer.as_ptr() as u32) }); + 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. + // + // The MAXCNT field is at least 8 bits wide and accepts the full + // range of values. + unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); + + // Start UARTE Receive transaction. + uarte.tasks_startrx.write(|w| + // `1` is a valid value to write to task registers. + unsafe { w.bits(1) }); + + Ok(()) +} + +/// Stop an unfinished UART read transaction and flush FIFO to DMA buffer. +fn cancel_read(uarte: &uarte0::RegisterBlock) { + // Stop reception. + uarte.tasks_stoprx.write(|w| unsafe { w.bits(1) }); + + // Wait for the reception to have stopped. + while uarte.events_rxto.read().bits() == 0 {} + + // Reset the event flag. + uarte.events_rxto.write(|w| w); + + // Ask UART to flush FIFO to DMA buffer. + uarte.tasks_flushrx.write(|w| unsafe { w.bits(1) }); + + // Wait for the flush to complete. + while uarte.events_endrx.read().bits() == 0 {} + + // The event flag itself is later reset by `finalize_read`. +} + +/// Finalize a UARTE read transaction by clearing the event. +fn finalize_read(uarte: &uarte0::RegisterBlock) { + // Reset the event, otherwise it will always read `1` from now on. + uarte.events_endrx.write(|w| w); + + // Conservative compiler fence to prevent optimizations that do not + // take in to account actions by DMA. The fence has been placed here, + // after all possible DMA actions have completed. + compiler_fence(SeqCst); +} + impl fmt::Write for Uarte where T: Instance, @@ -490,30 +494,29 @@ mod _uarte1 { } /// Interface for the TX part of a UART instance that can be used independently of the RX part. -pub struct UarteTx<'a, T> +pub struct UarteTx where T: Instance, { _marker: core::marker::PhantomData, - tx_buf: &'a mut [u8], + tx_buf: &'static mut [u8], written: usize, } /// Interface for the RX part of a UART instance that can be used independently of the TX part. -pub struct UarteRx<'a, T> +pub struct UarteRx where T: Instance, { _marker: core::marker::PhantomData, - rx_buf: &'a mut [u8], + rx_buf: &'static mut [u8], } -impl<'a, T> UarteTx<'a, T> +impl UarteTx where T: Instance, { - fn new(tx_buf: &'a mut [u8]) -> Result, Error> { - slice_in_ram_or(tx_buf, Error::BufferNotInRAM)?; + fn new(tx_buf: &'static mut [u8]) -> Result, Error> { if tx_buf.len() == 0 { return Err(Error::TxBufferTooSmall); } @@ -530,12 +533,11 @@ where } } -impl<'a, T> UarteRx<'a, T> +impl UarteRx where T: Instance, { - fn new(rx_buf: &'a mut [u8]) -> Result, Error> { - slice_in_ram_or(rx_buf, Error::BufferNotInRAM)?; + fn new(rx_buf: &'static mut [u8]) -> Result, Error> { if rx_buf.len() == 0 { return Err(Error::RxBufferTooSmall); } @@ -551,7 +553,7 @@ where } } -impl<'a, T> Drop for UarteTx<'a, T> +impl Drop for UarteTx where T: Instance, { @@ -561,10 +563,7 @@ where let in_progress = uarte.events_txstarted.read().bits() == 1; // Stop any ongoing transmission if in_progress { - uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) }); - - // Wait for transmitter is stopped. - while uarte.events_txstopped.read().bits() == 0 {} + stop_write(uarte); // Reset events uarte.events_endtx.reset(); @@ -576,7 +575,7 @@ where } } -impl<'a, T> Drop for UarteRx<'a, T> +impl Drop for UarteRx where T: Instance, { @@ -586,18 +585,7 @@ where let in_progress = uarte.events_rxstarted.read().bits() == 1; // Stop any ongoing reception if in_progress { - uarte.tasks_stoprx.write(|w| unsafe { w.bits(1) }); - - // Wait for receive to be done to ensure memory is untouched. - while uarte.events_rxto.read().bits() == 0 {} - - uarte.events_rxto.reset(); - - // Flush DMA - uarte.tasks_flushrx.write(|w| unsafe { w.bits(1) }); - - // Wait for the flush to complete. - while uarte.events_endrx.read().bits() == 0 {} + cancel_read(uarte); // Reset events uarte.events_endrx.reset(); @@ -616,7 +604,7 @@ pub mod serial { use embedded_hal::serial; use nb; - impl<'a, T> serial::Write for UarteTx<'a, T> + impl serial::Write for UarteTx where T: Instance, { @@ -654,6 +642,7 @@ pub mod serial { // We are done, cleanup the state. uarte.events_txstarted.reset(); self.written = 0; + // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, // after all possible DMA actions have completed. @@ -665,8 +654,9 @@ pub mod serial { // Lower power consumption by disabling the transmitter once we're // finished. - // `1` is a valid value to write to task registers. - uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) }); + stop_write(uarte); + + compiler_fence(SeqCst); Ok(()) } else { // Still not done, don't block. @@ -678,46 +668,17 @@ pub mod serial { return Ok(()); } - // 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. - compiler_fence(SeqCst); - - // Reset the events. - uarte.events_endtx.reset(); - uarte.events_txstopped.reset(); - - // Set up the DMA write. - // We're giving the register a pointer to the tx buffer. - // - // The PTR field is a full 32 bits wide and accepts the full range - // of values. - uarte - .txd - .ptr - .write(|w| unsafe { w.ptr().bits(self.tx_buf.as_ptr() as u32) }); - - // We're giving it a length of the number of bytes written to the buffer. - // - // The MAXCNT field is 8 bits wide and accepts the full range of - // values. - uarte - .txd - .maxcnt - .write(|w| unsafe { w.maxcnt().bits(self.written as _) }); - - // Start UARTE Transmit transaction. - // `1` is a valid value to write to task registers. - uarte.tasks_starttx.write(|w| unsafe { w.bits(1) }); + start_write(uarte, &self.tx_buf[0..self.written]); + Err(nb::Error::WouldBlock) } } } // Auto-implement the blocking variant - impl<'a, T> bserial::write::Default for UarteTx<'a, T> where T: Instance {} + impl bserial::write::Default for UarteTx where T: Instance {} - impl<'a, T> core::fmt::Write for UarteTx<'a, T> + impl core::fmt::Write for UarteTx where T: Instance, { @@ -729,7 +690,7 @@ pub mod serial { } } - impl<'a, T> serial::Read for UarteRx<'a, T> + impl serial::Read for UarteRx where T: Instance, { @@ -747,38 +708,15 @@ pub mod serial { if in_progress { let b = self.rx_buf[0]; uarte.events_rxstarted.write(|w| w); - uarte.events_endrx.write(|w| w); - compiler_fence(SeqCst); + finalize_read(uarte); + if uarte.rxd.amount.read().bits() != 1 as u32 { return Err(nb::Error::Other(Error::Receive)); } Ok(b) } else { - // We're giving the register a pointer to the rx buffer. - // - // The PTR field is a full 32 bits wide and accepts the full range - // of values. - uarte - .rxd - .ptr - .write(|w| unsafe { w.ptr().bits(self.rx_buf.as_ptr() as u32) }); - - // We're giving it a length of 1 to read only 1 byte. - // - // The MAXCNT field is at least 8 bits wide and accepts the full - // range of values. - uarte.rxd.maxcnt.write(|w| unsafe { w.maxcnt().bits(1) }); - - // Start UARTE Receive transaction. - // `1` is a valid value to write to task registers. - uarte.tasks_startrx.write(|w| unsafe { w.bits(1) }); - // Conservative compiler fence to prevent optimizations that do not - // take in to account actions by DMA. The fence has been placed here, - // after all possible DMA actions have completed. - - compiler_fence(SeqCst); - + start_read(&uarte, self.rx_buf)?; Err(nb::Error::WouldBlock) } } From d64470e6bbd17161c106a683e3a0d47e924a1910 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 14:36:38 +0100 Subject: [PATCH 10/21] Update nrf-hal-common/src/uarte.rs Co-authored-by: Jonas Schievink --- nrf-hal-common/src/uarte.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index ee618107..f49eb532 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -596,9 +596,8 @@ where } } +/// Implementation of the embedded_hal::serial::* and embedded_hal::blocking::serial::* traits for UartTx and UartRx. pub mod serial { - - ///! Implementation of the embedded_hal::serial::* and embedded_hal::blocking::serial::* traits for UartTx and UartRx. use super::*; use embedded_hal::blocking::serial as bserial; use embedded_hal::serial; From 5907789deaf325265cc1133cccddd345a27dd73f Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 14:39:22 +0100 Subject: [PATCH 11/21] Move into parent module --- nrf-hal-common/src/uarte.rs | 192 ++++++++++++++++++------------------ 1 file changed, 94 insertions(+), 98 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index f49eb532..be1b3a7f 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -8,7 +8,11 @@ use core::fmt; use core::ops::Deref; use core::sync::atomic::{compiler_fence, Ordering::SeqCst}; +use embedded_hal::blocking::serial as bserial; use embedded_hal::digital::v2::OutputPin; +use embedded_hal::serial; + +use nb; #[cfg(any(feature = "52833", feature = "52840"))] use crate::pac::UARTE1; @@ -596,128 +600,120 @@ where } } -/// Implementation of the embedded_hal::serial::* and embedded_hal::blocking::serial::* traits for UartTx and UartRx. -pub mod serial { - use super::*; - use embedded_hal::blocking::serial as bserial; - use embedded_hal::serial; - use nb; - - impl serial::Write for UarteTx - where - T: Instance, - { - type Error = Error; - - /// Write a single byte to the internal buffer. Returns nb::Error::WouldBlock if buffer is full. - fn write(&mut self, b: u8) -> nb::Result<(), Self::Error> { - let uarte = unsafe { &*T::ptr() }; +impl serial::Write for UarteTx +where + T: Instance, +{ + type Error = Error; - // Prevent writing to buffer while DMA transfer is in progress. - if uarte.events_txstarted.read().bits() == 1 { - return Err(nb::Error::WouldBlock); - } + /// Write a single byte to the internal buffer. Returns nb::Error::WouldBlock if buffer is full. + fn write(&mut self, b: u8) -> nb::Result<(), Self::Error> { + let uarte = unsafe { &*T::ptr() }; - if self.written < self.tx_buf.len() { - self.tx_buf[self.written] = b; - self.written += 1; - Ok(()) - } else { - Err(nb::Error::WouldBlock) - } + // Prevent writing to buffer while DMA transfer is in progress. + if uarte.events_txstarted.read().bits() == 1 { + return Err(nb::Error::WouldBlock); } - /// Flush the TX buffer non-blocking. Returns nb::Error::WouldBlock if not yet flushed. - fn flush(&mut self) -> nb::Result<(), Self::Error> { - let uarte = unsafe { &*T::ptr() }; - - // If txstarted is set, we are in the process of transmitting. - let in_progress = uarte.events_txstarted.read().bits() == 1; - - if in_progress { - let endtx = uarte.events_endtx.read().bits() != 0; - let txstopped = uarte.events_txstopped.read().bits() != 0; - if endtx || txstopped { - // We are done, cleanup the state. - uarte.events_txstarted.reset(); - self.written = 0; - - // Conservative compiler fence to prevent optimizations that do not - // take in to account actions by DMA. The fence has been placed here, - // after all possible DMA actions have completed. - compiler_fence(SeqCst); + if self.written < self.tx_buf.len() { + self.tx_buf[self.written] = b; + self.written += 1; + Ok(()) + } else { + Err(nb::Error::WouldBlock) + } + } - if txstopped { - return Err(nb::Error::Other(Error::Transmit)); - } + /// Flush the TX buffer non-blocking. Returns nb::Error::WouldBlock if not yet flushed. + fn flush(&mut self) -> nb::Result<(), Self::Error> { + let uarte = unsafe { &*T::ptr() }; - // Lower power consumption by disabling the transmitter once we're - // finished. - stop_write(uarte); + // If txstarted is set, we are in the process of transmitting. + let in_progress = uarte.events_txstarted.read().bits() == 1; - compiler_fence(SeqCst); - Ok(()) - } else { - // Still not done, don't block. - Err(nb::Error::WouldBlock) - } - } else { - // No need to trigger transmit if we don't have anything written - if self.written == 0 { - return Ok(()); + if in_progress { + let endtx = uarte.events_endtx.read().bits() != 0; + let txstopped = uarte.events_txstopped.read().bits() != 0; + if endtx || txstopped { + // We are done, cleanup the state. + uarte.events_txstarted.reset(); + self.written = 0; + + // Conservative compiler fence to prevent optimizations that do not + // take in to account actions by DMA. The fence has been placed here, + // after all possible DMA actions have completed. + compiler_fence(SeqCst); + + if txstopped { + return Err(nb::Error::Other(Error::Transmit)); } - start_write(uarte, &self.tx_buf[0..self.written]); + // Lower power consumption by disabling the transmitter once we're + // finished. + stop_write(uarte); + compiler_fence(SeqCst); + Ok(()) + } else { + // Still not done, don't block. Err(nb::Error::WouldBlock) } + } else { + // No need to trigger transmit if we don't have anything written + if self.written == 0 { + return Ok(()); + } + + start_write(uarte, &self.tx_buf[0..self.written]); + + Err(nb::Error::WouldBlock) } } +} - // Auto-implement the blocking variant - impl bserial::write::Default for UarteTx where T: Instance {} +// Auto-implement the blocking variant +impl bserial::write::Default for UarteTx where T: Instance {} - impl core::fmt::Write for UarteTx - where - T: Instance, - { - fn write_str(&mut self, s: &str) -> fmt::Result { - s.as_bytes() - .iter() - .try_for_each(|c| nb::block!(self.write(*c))) - .map_err(|_| core::fmt::Error) - } +impl core::fmt::Write for UarteTx +where + T: Instance, +{ + fn write_str(&mut self, s: &str) -> fmt::Result { + s.as_bytes() + .iter() + .try_for_each(|c| nb::block!(self.write(*c))) + .map_err(|_| core::fmt::Error) } +} - impl serial::Read for UarteRx - where - T: Instance, - { - type Error = Error; - fn read(&mut self) -> nb::Result { - let uarte = unsafe { &*T::ptr() }; +impl serial::Read for UarteRx +where + T: Instance, +{ + type Error = Error; + fn read(&mut self) -> nb::Result { + let uarte = unsafe { &*T::ptr() }; - compiler_fence(SeqCst); + compiler_fence(SeqCst); - let in_progress = uarte.events_rxstarted.read().bits() == 1; - if in_progress && uarte.events_endrx.read().bits() == 0 { - return Err(nb::Error::WouldBlock); - } + let in_progress = uarte.events_rxstarted.read().bits() == 1; + if in_progress && uarte.events_endrx.read().bits() == 0 { + return Err(nb::Error::WouldBlock); + } - if in_progress { - let b = self.rx_buf[0]; - uarte.events_rxstarted.write(|w| w); + if in_progress { + let b = self.rx_buf[0]; + uarte.events_rxstarted.write(|w| w); - finalize_read(uarte); + finalize_read(uarte); - if uarte.rxd.amount.read().bits() != 1 as u32 { - return Err(nb::Error::Other(Error::Receive)); - } - Ok(b) - } else { - start_read(&uarte, self.rx_buf)?; - Err(nb::Error::WouldBlock) + if uarte.rxd.amount.read().bits() != 1 as u32 { + return Err(nb::Error::Other(Error::Receive)); } + Ok(b) + } else { + start_read(&uarte, self.rx_buf)?; + Err(nb::Error::WouldBlock) } } } From 706a4b9f3adac270eae0000e887009b954141cf4 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 14:59:02 +0100 Subject: [PATCH 12/21] Remove unneeded import --- nrf-hal-common/src/uarte.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index be1b3a7f..98f8f8d2 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -12,8 +12,6 @@ use embedded_hal::blocking::serial as bserial; use embedded_hal::digital::v2::OutputPin; use embedded_hal::serial; -use nb; - #[cfg(any(feature = "52833", feature = "52840"))] use crate::pac::UARTE1; From 39395df06b7a312bc5dacd1d05948bfdc43f040f Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 16:29:54 +0100 Subject: [PATCH 13/21] Update nrf-hal-common/src/uarte.rs Co-authored-by: Jonas Schievink --- nrf-hal-common/src/uarte.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 98f8f8d2..a0f8c9f9 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -701,7 +701,7 @@ where if in_progress { let b = self.rx_buf[0]; - uarte.events_rxstarted.write(|w| w); + uarte.events_rxstarted.reset(); finalize_read(uarte); From 016fcf7168071db1029c9907232c93de7caa4e81 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 16:25:49 +0100 Subject: [PATCH 14/21] Call flush if write buffer is full --- nrf-hal-common/src/uarte.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index a0f8c9f9..82ff869e 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -618,7 +618,7 @@ where self.written += 1; Ok(()) } else { - Err(nb::Error::WouldBlock) + self.flush() } } From a03f1888a722d3e3cad9227443965636f7c50892 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 12 Jan 2021 17:51:32 +0100 Subject: [PATCH 15/21] Restrict serial API to read only 1 byte at a time to avoid DMA issues. --- nrf-hal-common/src/uarte.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 82ff869e..b67e6be0 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -202,7 +202,7 @@ where /// /// The buffer must have a length of at most 255 bytes. pub fn read(&mut self, rx_buffer: &mut [u8]) -> Result<(), Error> { - start_read(&*self.0, rx_buffer)?; + start_read(&*self.0, rx_buffer, rx_buffer.len())?; // Wait for transmission to end. while self.0.events_endrx.read().bits() == 0 {} @@ -240,7 +240,7 @@ where I: timer::Instance, { // Start the read. - start_read(&self.0, rx_buffer)?; + start_read(&self.0, rx_buffer, rx_buffer.len())?; // Start the timeout timer. timer.start(cycles); @@ -363,7 +363,11 @@ fn stop_write(uarte: &uarte0::RegisterBlock) { /// Start a UARTE read transaction by setting the control /// values and triggering a read task. -fn start_read(uarte: &uarte0::RegisterBlock, rx_buffer: &mut [u8]) -> Result<(), Error> { +fn start_read( + uarte: &uarte0::RegisterBlock, + rx_buffer: &mut [u8], + nbytes: usize, +) -> Result<(), Error> { if rx_buffer.len() > EASY_DMA_SIZE { return Err(Error::RxBufferTooLong); } @@ -392,7 +396,7 @@ fn start_read(uarte: &uarte0::RegisterBlock, rx_buffer: &mut [u8]) -> Result<(), // // The MAXCNT field is at least 8 bits wide and accepts the full // range of values. - unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); + unsafe { w.maxcnt().bits(nbytes.min(rx_buffer.len()) as _) }); // Start UARTE Receive transaction. uarte.tasks_startrx.write(|w| @@ -700,7 +704,6 @@ where } if in_progress { - let b = self.rx_buf[0]; uarte.events_rxstarted.reset(); finalize_read(uarte); @@ -708,9 +711,9 @@ where if uarte.rxd.amount.read().bits() != 1 as u32 { return Err(nb::Error::Other(Error::Receive)); } - Ok(b) + Ok(self.rx_buf[0]) } else { - start_read(&uarte, self.rx_buf)?; + start_read(&uarte, self.rx_buf, 1)?; Err(nb::Error::WouldBlock) } } From ee59776db31438c6643c00fdf4089ffcdf418d3e Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Wed, 13 Jan 2021 14:35:50 +0100 Subject: [PATCH 16/21] Simplify by passing slice only --- nrf-hal-common/src/uarte.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index b67e6be0..302306cb 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -202,7 +202,7 @@ where /// /// The buffer must have a length of at most 255 bytes. pub fn read(&mut self, rx_buffer: &mut [u8]) -> Result<(), Error> { - start_read(&*self.0, rx_buffer, rx_buffer.len())?; + start_read(&*self.0, rx_buffer)?; // Wait for transmission to end. while self.0.events_endrx.read().bits() == 0 {} @@ -240,7 +240,7 @@ where I: timer::Instance, { // Start the read. - start_read(&self.0, rx_buffer, rx_buffer.len())?; + start_read(&self.0, rx_buffer)?; // Start the timeout timer. timer.start(cycles); @@ -363,11 +363,7 @@ fn stop_write(uarte: &uarte0::RegisterBlock) { /// Start a UARTE read transaction by setting the control /// values and triggering a read task. -fn start_read( - uarte: &uarte0::RegisterBlock, - rx_buffer: &mut [u8], - nbytes: usize, -) -> Result<(), Error> { +fn start_read(uarte: &uarte0::RegisterBlock, rx_buffer: &mut [u8]) -> Result<(), Error> { if rx_buffer.len() > EASY_DMA_SIZE { return Err(Error::RxBufferTooLong); } @@ -396,7 +392,7 @@ fn start_read( // // The MAXCNT field is at least 8 bits wide and accepts the full // range of values. - unsafe { w.maxcnt().bits(nbytes.min(rx_buffer.len()) as _) }); + unsafe { w.maxcnt().bits(rx_buffer.len() as _) }); // Start UARTE Receive transaction. uarte.tasks_startrx.write(|w| @@ -713,7 +709,7 @@ where } Ok(self.rx_buf[0]) } else { - start_read(&uarte, self.rx_buf, 1)?; + start_read(&uarte, &mut self.rx_buf[..1])?; Err(nb::Error::WouldBlock) } } From 2681915a4172c7d49f78addfcdd24e04b9935709 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Wed, 13 Jan 2021 14:38:28 +0100 Subject: [PATCH 17/21] Add comment --- nrf-hal-common/src/uarte.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 302306cb..01f9822d 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -709,6 +709,8 @@ where } Ok(self.rx_buf[0]) } else { + // We can only read 1 byte at a time, otherwise ENDTX might not be raised, + // causing the read to stall forever. start_read(&uarte, &mut self.rx_buf[..1])?; Err(nb::Error::WouldBlock) } From 9ecdbb8afc335b27ce8790a5ec02e763bdebe8a9 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Wed, 13 Jan 2021 17:43:54 +0100 Subject: [PATCH 18/21] Require a slice of size 1 for rx_buf --- nrf-hal-common/src/uarte.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 01f9822d..21cd26ad 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -308,7 +308,7 @@ where pub fn split( self, tx_buf: &'static mut [u8], - rx_buf: &'static mut [u8], + rx_buf: &'static mut [u8; 1], ) -> Result<(UarteTx, UarteRx), Error> { let tx = UarteTx::new(tx_buf)?; let rx = UarteRx::new(rx_buf)?; @@ -711,7 +711,7 @@ where } else { // We can only read 1 byte at a time, otherwise ENDTX might not be raised, // causing the read to stall forever. - start_read(&uarte, &mut self.rx_buf[..1])?; + start_read(&uarte, self.rx_buf)?; Err(nb::Error::WouldBlock) } } From 883680c7adf1802e0a650685a0f9e0f62e89ed44 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Tue, 16 Feb 2021 08:06:18 +0100 Subject: [PATCH 19/21] Consistent buffer length check behavior --- nrf-hal-common/src/uarte.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 21cd26ad..be22096f 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -169,6 +169,10 @@ where /// The buffer must have a length of at most 255 bytes on the nRF52832 /// and at most 65535 bytes on the nRF52840. pub fn write(&mut self, tx_buffer: &[u8]) -> Result<(), Error> { + if tx_buffer.len() == 0 { + return Err(Error::TxBufferTooSmall); + } + if tx_buffer.len() > EASY_DMA_SIZE { return Err(Error::TxBufferTooLong); } @@ -364,6 +368,10 @@ fn stop_write(uarte: &uarte0::RegisterBlock) { /// Start a UARTE read transaction by setting the control /// values and triggering a read task. fn start_read(uarte: &uarte0::RegisterBlock, rx_buffer: &mut [u8]) -> Result<(), Error> { + if rx_buffer.len() == 0 { + return Err(Error::RxBufferTooSmall); + } + if rx_buffer.len() > EASY_DMA_SIZE { return Err(Error::RxBufferTooLong); } From 8fc2df4a36f8cde322f19d0e70579d63094ffdba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20B=C3=B6ving?= Date: Thu, 29 Jul 2021 20:07:57 +0200 Subject: [PATCH 20/21] Add some docs and restart tx/rxstarted on UARTE drop() --- nrf-hal-common/src/uarte.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index be22096f..95139e17 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -307,8 +307,11 @@ where ) } - /// Split into implementations of embedded_hal::serial traits. The size of the slices passed to this - /// method will determine the size of the DMA transfers performed. + /// Split into implementations of embedded_hal::serial traits. + /// The size of the `tx_buf` slice passed to this method will determine + /// the size of the DMA transfers performed. + /// The `rx_buf` slice is an array of size 1 since the embedded_hal + /// traits only allow reading one byte at a time. pub fn split( self, tx_buf: &'static mut [u8], @@ -578,6 +581,7 @@ where // Reset events uarte.events_endtx.reset(); uarte.events_txstopped.reset(); + uarte.events_txstarted.reset(); // Ensure the above is done compiler_fence(SeqCst); @@ -599,6 +603,7 @@ where // Reset events uarte.events_endrx.reset(); + uarte.events_rxstarted.reset(); // Ensure the above is done compiler_fence(SeqCst); From 0ebf91b322ec5b7a91f5357bed77c818045358bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20B=C3=B6ving?= Date: Tue, 10 Aug 2021 23:26:07 +0200 Subject: [PATCH 21/21] Fix uarte write --- nrf-hal-common/src/uarte.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/nrf-hal-common/src/uarte.rs b/nrf-hal-common/src/uarte.rs index 95139e17..bf3d5d34 100644 --- a/nrf-hal-common/src/uarte.rs +++ b/nrf-hal-common/src/uarte.rs @@ -622,17 +622,17 @@ where let uarte = unsafe { &*T::ptr() }; // Prevent writing to buffer while DMA transfer is in progress. - if uarte.events_txstarted.read().bits() == 1 { + if uarte.events_txstarted.read().bits() == 1 && uarte.events_endtx.read().bits() == 0 { return Err(nb::Error::WouldBlock); } - if self.written < self.tx_buf.len() { - self.tx_buf[self.written] = b; - self.written += 1; - Ok(()) - } else { - self.flush() + if self.written >= self.tx_buf.len() { + self.flush()?; } + + self.tx_buf[self.written] = b; + self.written += 1; + Ok(()) } /// Flush the TX buffer non-blocking. Returns nb::Error::WouldBlock if not yet flushed.