From ed9fad8c7e71d7a868683957bbfdf64c0eca7ffe Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 23 Feb 2022 22:51:01 +0100 Subject: [PATCH 01/10] Skip EasyDMA slice location check if slice is empty --- embassy-nrf/src/lib.rs | 2 +- embassy-nrf/src/util.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/embassy-nrf/src/lib.rs b/embassy-nrf/src/lib.rs index b448f6ab..a9df231e 100644 --- a/embassy-nrf/src/lib.rs +++ b/embassy-nrf/src/lib.rs @@ -1,7 +1,7 @@ #![no_std] #![cfg_attr( feature = "nightly", - feature(generic_associated_types, type_alias_impl_trait) + feature(generic_associated_types, type_alias_impl_trait, slice_ptr_len) )] #[cfg(not(any( diff --git a/embassy-nrf/src/util.rs b/embassy-nrf/src/util.rs index b24bc452..84848e87 100644 --- a/embassy-nrf/src/util.rs +++ b/embassy-nrf/src/util.rs @@ -19,10 +19,10 @@ pub(crate) fn slice_in_ram(slice: *const [T]) -> bool { ptr >= SRAM_LOWER && (ptr + len * core::mem::size_of::()) < SRAM_UPPER } -/// Return an error if slice is not in RAM. +/// Return an error if slice is not in RAM. Skips check if slice is zero-length. #[cfg(not(feature = "nrf51"))] pub(crate) fn slice_in_ram_or(slice: *const [T], err: E) -> Result<(), E> { - if slice_in_ram(slice) { + if slice.len() > 0 && slice_in_ram(slice) { Ok(()) } else { Err(err) From e96dd3654a5b3919ad853a33a89927596f5ca1c2 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 23 Feb 2022 22:51:59 +0100 Subject: [PATCH 02/10] Change SPIM methods to copy slice if required and add non-copying variants --- embassy-nrf/src/spim.rs | 105 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 6 deletions(-) diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index 6dbf67e3..fedd4e48 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -28,6 +28,40 @@ pub enum Error { DMABufferNotInDataMemory, } +/// Interface for the SPIM peripheral using EasyDMA to offload the transmission and reception workload. +/// +/// ## Data locality requirements +/// +/// On nRF chips, EasyDMA requires the buffers to reside in RAM. However, Rust +/// slices will not always do so. Take the following example: +/// +/// ```no_run +/// // As we pass a slice to the function whose contents will not ever change, +/// // the compiler writes it into the flash and thus the pointer to it will +/// // reference static memory. Since EasyDMA requires slices to reside in RAM, +/// // this function call will fail. +/// let result = spim.write_from_ram(&[1, 2, 3]); +/// assert_eq!(result, Error::DMABufferNotInDataMemory); +/// +/// // The data is still static and located in flash. However, since we are assigning +/// // it to a variable, the compiler will load it into memory. Passing a reference to the +/// // variable will yield a pointer that references dynamic memory, thus making EasyDMA happy. +/// // This function call succeeds. +/// let data = [1, 2, 3]; +/// let result = spim.write_from_ram(&data); +/// assert!(result.is_ok()); +/// ``` +/// +/// Each function in this struct has a `_from_ram` variant and one without this suffix. +/// - Functions with the suffix (e.g. [`write_from_ram`](Spim::write_from_ram), [`transfer_from_ram`](Spim::transfer_from_ram)) will return an error if the passed slice does not reside in RAM. +/// - Functions without the suffix (e.g. [`write`](Spim::write), [`transfer`](Spim::transfer)) will check whether the data is in RAM and copy it into memory prior to transmission. +/// +/// Since copying incurs a overhead, you are given the option to choose from `_from_ram` variants which will +/// fail and notify you, or the more convenient versions without the suffix which are potentially a little bit +/// more inefficient. +/// +/// Note that the [`read`](Spim::read) and [`transfer_in_place`](Spim::transfer_in_place) methods do not have the corresponding `_from_ram` variants as +/// mutable slices always reside in RAM. pub struct Spim<'d, T: Instance> { phantom: PhantomData<&'d mut T>, } @@ -223,7 +257,7 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(()) } - fn blocking_inner(&mut self, rx: *mut [u8], tx: *const [u8]) -> Result<(), Error> { + fn blocking_inner_from_ram(&mut self, rx: *mut [u8], tx: *const [u8]) -> Result<(), Error> { self.prepare(rx, tx)?; // Wait for 'end' event. @@ -234,7 +268,18 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(()) } - async fn async_inner(&mut self, rx: *mut [u8], tx: *const [u8]) -> Result<(), Error> { + fn blocking_inner(&mut self, rx: &mut [u8], tx: &[u8]) -> Result<(), Error> { + match self.blocking_inner_from_ram(rx, tx) { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + let tx_copied = tx.clone(); + self.blocking_inner_from_ram(rx, tx_copied) + } + Err(error) => Err(error), + } + } + + async fn async_inner_from_ram(&mut self, rx: *mut [u8], tx: *const [u8]) -> Result<(), Error> { self.prepare(rx, tx)?; // Wait for 'end' event. @@ -253,37 +298,85 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(()) } + async fn async_inner(&mut self, rx: &mut [u8], tx: &[u8]) -> Result<(), Error> { + match self.async_inner_from_ram(rx, tx).await { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + let tx_copied = tx.clone(); + self.async_inner_from_ram(rx, tx_copied).await + } + Err(error) => Err(error), + } + } + + /// Reads data from the SPI bus without sending anything. Blocks until the buffer has been filled. pub fn blocking_read(&mut self, data: &mut [u8]) -> Result<(), Error> { self.blocking_inner(data, &[]) } + /// Simultaneously sends and receives data. Blocks until the transmission is completed. + /// If necessary, the write buffer will be copied into RAM (see struct description for detail). pub fn blocking_transfer(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { self.blocking_inner(read, write) } - pub fn blocking_transfer_in_place(&mut self, data: &mut [u8]) -> Result<(), Error> { - self.blocking_inner(data, data) + /// Same as [`blocking_transfer`](Spim::blocking_transfer) but will fail instead of copying data into RAM. + pub fn blocking_transfer_from_ram( + &mut self, + read: &mut [u8], + write: &[u8], + ) -> Result<(), Error> { + self.blocking_inner(read, write) } + /// Simultaneously sends and receives data. + /// Places the received data into the same buffer and blocks until the transmission is completed. + pub fn blocking_transfer_in_place(&mut self, data: &mut [u8]) -> Result<(), Error> { + self.blocking_inner_from_ram(data, data) + } + + /// Sends data, discarding any received data. Blocks until the transmission is completed. + /// If necessary, the write buffer will be copied into RAM (see struct description for detail). pub fn blocking_write(&mut self, data: &[u8]) -> Result<(), Error> { self.blocking_inner(&mut [], data) } + /// Same as [`blocking_write`](Spim::blocking_write) but will fail instead of copying data into RAM. + pub fn blocking_write_from_ram(&mut self, data: &[u8]) -> Result<(), Error> { + self.blocking_inner(&mut [], data) + } + + /// Reads data from the SPI bus without sending anything. pub async fn read(&mut self, data: &mut [u8]) -> Result<(), Error> { self.async_inner(data, &[]).await } + /// Simultaneously sends and receives data. + /// If necessary, the write buffer will be copied into RAM (see struct description for detail). pub async fn transfer(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { self.async_inner(read, write).await } - pub async fn transfer_in_place(&mut self, data: &mut [u8]) -> Result<(), Error> { - self.async_inner(data, data).await + /// Same as [`transfer`](Spim::transfer) but will fail instead of copying data into RAM. + pub async fn transfer_from_ram(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { + self.async_inner_from_ram(read, write).await } + /// Simultaneously sends and receives data. Places the received data into the same buffer. + pub async fn transfer_in_place(&mut self, data: &mut [u8]) -> Result<(), Error> { + self.async_inner_from_ram(data, data).await + } + + /// Sends data, discarding any received data. + /// If necessary, the write buffer will be copied into RAM (see struct description for detail). pub async fn write(&mut self, data: &[u8]) -> Result<(), Error> { self.async_inner(&mut [], data).await } + + /// Same as [`write`](Spim::write) but will fail instead of copying data into RAM. + pub async fn write_from_ram(&mut self, data: &[u8]) -> Result<(), Error> { + self.async_inner_from_ram(&mut [], data).await + } } impl<'d, T: Instance> Drop for Spim<'d, T> { From 66fdec7abe649cb7d7203af526035e2a64a55776 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 23 Feb 2022 23:27:12 +0100 Subject: [PATCH 03/10] Add defmt log outputs for SPIM memcpy --- embassy-nrf/src/spim.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index fedd4e48..5895558e 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -272,6 +272,7 @@ impl<'d, T: Instance> Spim<'d, T> { match self.blocking_inner_from_ram(rx, tx) { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying SPIM tx buffer into RAM for DMA"); let tx_copied = tx.clone(); self.blocking_inner_from_ram(rx, tx_copied) } @@ -302,6 +303,7 @@ impl<'d, T: Instance> Spim<'d, T> { match self.async_inner_from_ram(rx, tx).await { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying SPIM tx buffer into RAM for DMA"); let tx_copied = tx.clone(); self.async_inner_from_ram(rx, tx_copied).await } From 6dc58645d22ebebf6abe8d4d07bcc3001cac91c6 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 23 Feb 2022 23:30:50 +0100 Subject: [PATCH 04/10] Change slice length check to use stable method --- embassy-nrf/src/lib.rs | 2 +- embassy-nrf/src/util.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/embassy-nrf/src/lib.rs b/embassy-nrf/src/lib.rs index a9df231e..b448f6ab 100644 --- a/embassy-nrf/src/lib.rs +++ b/embassy-nrf/src/lib.rs @@ -1,7 +1,7 @@ #![no_std] #![cfg_attr( feature = "nightly", - feature(generic_associated_types, type_alias_impl_trait, slice_ptr_len) + feature(generic_associated_types, type_alias_impl_trait) )] #[cfg(not(any( diff --git a/embassy-nrf/src/util.rs b/embassy-nrf/src/util.rs index 84848e87..42265dc2 100644 --- a/embassy-nrf/src/util.rs +++ b/embassy-nrf/src/util.rs @@ -22,7 +22,8 @@ pub(crate) fn slice_in_ram(slice: *const [T]) -> bool { /// Return an error if slice is not in RAM. Skips check if slice is zero-length. #[cfg(not(feature = "nrf51"))] pub(crate) fn slice_in_ram_or(slice: *const [T], err: E) -> Result<(), E> { - if slice.len() > 0 && slice_in_ram(slice) { + let (_, len) = slice_ptr_parts(slice); + if len > 0 && slice_in_ram(slice) { Ok(()) } else { Err(err) From 62407da29b5d1867f50f30c796cc1ecfc7dc77c1 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 23 Feb 2022 23:38:18 +0100 Subject: [PATCH 05/10] Fix EasyDMA slice copying not actually copying data --- embassy-nrf/src/spim.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index 5895558e..3e793f39 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -8,6 +8,7 @@ use embassy::util::Unborrow; use embassy_hal_common::unborrow; use futures::future::poll_fn; +use crate::chip::FORCE_COPY_BUFFER_SIZE; use crate::gpio::sealed::Pin as _; use crate::gpio::{self, AnyPin}; use crate::gpio::{Pin as GpioPin, PselBits}; @@ -273,8 +274,9 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying SPIM tx buffer into RAM for DMA"); - let tx_copied = tx.clone(); - self.blocking_inner_from_ram(rx, tx_copied) + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..tx.len()].copy_from_slice(tx); + self.blocking_inner_from_ram(rx, &tx_buf[..tx.len()]) } Err(error) => Err(error), } @@ -304,8 +306,9 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying SPIM tx buffer into RAM for DMA"); - let tx_copied = tx.clone(); - self.async_inner_from_ram(rx, tx_copied).await + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..tx.len()].copy_from_slice(tx); + self.async_inner_from_ram(rx, &tx_buf[..tx.len()]).await } Err(error) => Err(error), } From 2c402ecf1695974ae5474f828f062d06b2392295 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 2 Mar 2022 22:23:43 +0100 Subject: [PATCH 06/10] Change UARTE methods to copy slice if required and add non-copying variants --- embassy-nrf/src/uarte.rs | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 119a1798..4aa1f02d 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -22,7 +22,7 @@ use embassy_hal_common::drop::OnDrop; use embassy_hal_common::unborrow; use futures::future::poll_fn; -use crate::chip::EASY_DMA_SIZE; +use crate::chip::{EASY_DMA_SIZE, FORCE_COPY_BUFFER_SIZE}; use crate::gpio::sealed::Pin as _; use crate::gpio::{self, AnyPin, Pin as GpioPin, PselBits}; use crate::interrupt::Interrupt; @@ -224,6 +224,10 @@ impl<'d, T: Instance> Uarte<'d, T> { self.tx.write(buffer).await } + pub async fn write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { + self.tx.write_from_ram(buffer).await + } + pub fn blocking_read(&mut self, buffer: &mut [u8]) -> Result<(), Error> { self.rx.blocking_read(buffer) } @@ -231,10 +235,27 @@ impl<'d, T: Instance> Uarte<'d, T> { pub fn blocking_write(&mut self, buffer: &[u8]) -> Result<(), Error> { self.tx.blocking_write(buffer) } + + pub fn blocking_write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { + self.tx.blocking_write_from_ram(buffer) + } } impl<'d, T: Instance> UarteTx<'d, T> { pub async fn write(&mut self, buffer: &[u8]) -> Result<(), Error> { + match self.write_from_ram(buffer).await { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying UARTE tx buffer into RAM for DMA"); + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..buffer.len()].copy_from_slice(buffer); + self.write_from_ram(&tx_buf[..buffer.len()]).await + } + Err(error) => Err(error), + } + } + + pub async fn write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { slice_in_ram_or(buffer, Error::DMABufferNotInDataMemory)?; if buffer.len() == 0 { return Err(Error::BufferZeroLength); @@ -289,6 +310,19 @@ impl<'d, T: Instance> UarteTx<'d, T> { } pub fn blocking_write(&mut self, buffer: &[u8]) -> Result<(), Error> { + match self.blocking_write_from_ram(buffer) { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying UARTE tx buffer into RAM for DMA"); + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..buffer.len()].copy_from_slice(buffer); + self.blocking_write_from_ram(&tx_buf[..buffer.len()]) + } + Err(error) => Err(error), + } + } + + pub fn blocking_write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { slice_in_ram_or(buffer, Error::DMABufferNotInDataMemory)?; if buffer.len() == 0 { return Err(Error::BufferZeroLength); From 3f2d9cfe0a643b1f1afcaef0fcb2cbbf305fd83d Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 2 Mar 2022 22:45:38 +0100 Subject: [PATCH 07/10] Change TWIM methods to copy slice if required and add non-copying variants --- embassy-nrf/src/twim.rs | 127 +++++++++++++++++++++++++++------------- 1 file changed, 86 insertions(+), 41 deletions(-) diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index ed2844f7..675029a8 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -287,7 +287,12 @@ impl<'d, T: Instance> Twim<'d, T> { }) } - fn setup_write(&mut self, address: u8, buffer: &[u8], inten: bool) -> Result<(), Error> { + fn setup_write_from_ram( + &mut self, + address: u8, + buffer: &[u8], + inten: bool, + ) -> Result<(), Error> { let r = T::regs(); compiler_fence(SeqCst); @@ -342,7 +347,7 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } - fn setup_write_read( + fn setup_write_read_from_ram( &mut self, address: u8, wr_buffer: &[u8], @@ -382,6 +387,43 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + fn setup_write_read( + &mut self, + address: u8, + wr_buffer: &[u8], + rd_buffer: &mut [u8], + inten: bool, + ) -> Result<(), Error> { + match self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, inten) { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying TWIM tx buffer into RAM for DMA"); + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..wr_buffer.len()].copy_from_slice(wr_buffer); + self.setup_write_read_from_ram( + address, + &tx_buf[..wr_buffer.len()], + rd_buffer, + inten, + ) + } + Err(error) => Err(error), + } + } + + fn setup_write(&mut self, address: u8, wr_buffer: &[u8], inten: bool) -> Result<(), Error> { + match self.setup_write_from_ram(address, wr_buffer, inten) { + Ok(_) => Ok(()), + Err(Error::DMABufferNotInDataMemory) => { + trace!("Copying TWIM tx buffer into RAM for DMA"); + let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; + tx_buf[..wr_buffer.len()].copy_from_slice(wr_buffer); + self.setup_write_from_ram(address, &tx_buf[..wr_buffer.len()], inten) + } + Err(error) => Err(error), + } + } + /// Write to an I2C slave. /// /// The buffer must have a length of at most 255 bytes on the nRF52832 @@ -395,6 +437,15 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + pub fn blocking_write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { + self.setup_write_from_ram(address, buffer, false)?; + self.blocking_wait(); + compiler_fence(SeqCst); + self.check_errorsrc()?; + self.check_tx(buffer.len())?; + Ok(()) + } + /// Read from an I2C slave. /// /// The buffer must have a length of at most 255 bytes on the nRF52832 @@ -428,45 +479,19 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } - /// Copy data into RAM and write to an I2C slave. - /// - /// The write buffer must have a length of at most 255 bytes on the nRF52832 - /// and at most 1024 bytes on the nRF52840. - pub fn blocking_copy_write(&mut self, address: u8, wr_buffer: &[u8]) -> Result<(), Error> { - if wr_buffer.len() > FORCE_COPY_BUFFER_SIZE { - return Err(Error::TxBufferTooLong); - } - - // Copy to RAM - let wr_ram_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; - wr_ram_buffer.copy_from_slice(wr_buffer); - - self.blocking_write(address, wr_ram_buffer) - } - - /// Copy data into RAM and write to an I2C slave, then read data from the slave without - /// triggering a stop condition between the two. - /// - /// The write buffer must have a length of at most 255 bytes on the nRF52832 - /// and at most 1024 bytes on the nRF52840. - /// - /// The read buffer must have a length of at most 255 bytes on the nRF52832 - /// and at most 65535 bytes on the nRF52840. - pub fn blocking_copy_write_read( + pub fn blocking_write_read_from_ram( &mut self, address: u8, wr_buffer: &[u8], rd_buffer: &mut [u8], ) -> Result<(), Error> { - if wr_buffer.len() > FORCE_COPY_BUFFER_SIZE { - return Err(Error::TxBufferTooLong); - } - - // Copy to RAM - let wr_ram_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; - wr_ram_buffer.copy_from_slice(wr_buffer); - - self.blocking_write_read(address, wr_ram_buffer, rd_buffer) + self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, false)?; + self.blocking_wait(); + compiler_fence(SeqCst); + self.check_errorsrc()?; + self.check_tx(wr_buffer.len())?; + self.check_rx(rd_buffer.len())?; + Ok(()) } pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> { @@ -487,6 +512,15 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + pub async fn write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { + self.setup_write_from_ram(address, buffer, true)?; + self.async_wait().await; + compiler_fence(SeqCst); + self.check_errorsrc()?; + self.check_tx(buffer.len())?; + Ok(()) + } + pub async fn write_read( &mut self, address: u8, @@ -501,6 +535,21 @@ impl<'d, T: Instance> Twim<'d, T> { self.check_rx(rd_buffer.len())?; Ok(()) } + + pub async fn write_read_from_ram( + &mut self, + address: u8, + wr_buffer: &[u8], + rd_buffer: &mut [u8], + ) -> Result<(), Error> { + self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, true)?; + self.async_wait().await; + compiler_fence(SeqCst); + self.check_errorsrc()?; + self.check_tx(wr_buffer.len())?; + self.check_rx(rd_buffer.len())?; + Ok(()) + } } impl<'a, T: Instance> Drop for Twim<'a, T> { @@ -601,11 +650,7 @@ mod eh02 { bytes: &'w [u8], buffer: &'w mut [u8], ) -> Result<(), Error> { - if slice_in_ram(bytes) { - self.blocking_write_read(addr, bytes, buffer) - } else { - self.blocking_copy_write_read(addr, bytes, buffer) - } + self.blocking_write_read(addr, bytes, buffer) } } } From 993428e2d45d0183cc7d778d964f046272b9d424 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Wed, 2 Mar 2022 22:48:58 +0100 Subject: [PATCH 08/10] Refactor _from_ram methods to use more readable copy operation --- embassy-nrf/src/spim.rs | 12 ++++++------ embassy-nrf/src/twim.rs | 17 ++++++----------- embassy-nrf/src/uarte.rs | 12 ++++++------ 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index 3e793f39..c9c9cb25 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -274,9 +274,9 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying SPIM tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..tx.len()].copy_from_slice(tx); - self.blocking_inner_from_ram(rx, &tx_buf[..tx.len()]) + let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..tx.len()]; + tx_ram_buf.copy_from_slice(tx); + self.blocking_inner_from_ram(rx, tx_ram_buf) } Err(error) => Err(error), } @@ -306,9 +306,9 @@ impl<'d, T: Instance> Spim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying SPIM tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..tx.len()].copy_from_slice(tx); - self.async_inner_from_ram(rx, &tx_buf[..tx.len()]).await + let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..tx.len()]; + tx_ram_buf.copy_from_slice(tx); + self.async_inner_from_ram(rx, tx_ram_buf).await } Err(error) => Err(error), } diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index 675029a8..c8ad2a0e 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -398,14 +398,9 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying TWIM tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..wr_buffer.len()].copy_from_slice(wr_buffer); - self.setup_write_read_from_ram( - address, - &tx_buf[..wr_buffer.len()], - rd_buffer, - inten, - ) + let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; + tx_ram_buf.copy_from_slice(wr_buffer); + self.setup_write_read_from_ram(address, &tx_ram_buf, rd_buffer, inten) } Err(error) => Err(error), } @@ -416,9 +411,9 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying TWIM tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..wr_buffer.len()].copy_from_slice(wr_buffer); - self.setup_write_from_ram(address, &tx_buf[..wr_buffer.len()], inten) + let tx_ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()]; + tx_ram_buf.copy_from_slice(wr_buffer); + self.setup_write_from_ram(address, &tx_ram_buf, inten) } Err(error) => Err(error), } diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 4aa1f02d..7d7b904b 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -247,9 +247,9 @@ impl<'d, T: Instance> UarteTx<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying UARTE tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..buffer.len()].copy_from_slice(buffer); - self.write_from_ram(&tx_buf[..buffer.len()]).await + let ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..buffer.len()]; + ram_buf.copy_from_slice(buffer); + self.write_from_ram(&ram_buf).await } Err(error) => Err(error), } @@ -314,9 +314,9 @@ impl<'d, T: Instance> UarteTx<'d, T> { Ok(_) => Ok(()), Err(Error::DMABufferNotInDataMemory) => { trace!("Copying UARTE tx buffer into RAM for DMA"); - let mut tx_buf = [0u8; FORCE_COPY_BUFFER_SIZE]; - tx_buf[..buffer.len()].copy_from_slice(buffer); - self.blocking_write_from_ram(&tx_buf[..buffer.len()]) + let ram_buf = &mut [0; FORCE_COPY_BUFFER_SIZE][..buffer.len()]; + ram_buf.copy_from_slice(buffer); + self.blocking_write_from_ram(&ram_buf) } Err(error) => Err(error), } From 7540b44050f42ba75ea855f904b959cc4cfdefe5 Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Tue, 8 Mar 2022 16:29:42 +0100 Subject: [PATCH 09/10] Fix inverted boolean condition --- embassy-nrf/src/util.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/embassy-nrf/src/util.rs b/embassy-nrf/src/util.rs index 42265dc2..cd0f5949 100644 --- a/embassy-nrf/src/util.rs +++ b/embassy-nrf/src/util.rs @@ -23,7 +23,7 @@ pub(crate) fn slice_in_ram(slice: *const [T]) -> bool { #[cfg(not(feature = "nrf51"))] pub(crate) fn slice_in_ram_or(slice: *const [T], err: E) -> Result<(), E> { let (_, len) = slice_ptr_parts(slice); - if len > 0 && slice_in_ram(slice) { + if len == 0 || slice_in_ram(slice) { Ok(()) } else { Err(err) From 63030bf998b0787b421f30af4f75159e2cb5c99f Mon Sep 17 00:00:00 2001 From: Til Blechschmidt Date: Tue, 8 Mar 2022 16:42:46 +0100 Subject: [PATCH 10/10] Move EasyDMA documentation to module level --- embassy-nrf/src/lib.rs | 36 +++++++++++++++++++++++++++++++++++ embassy-nrf/src/spim.rs | 41 +++++----------------------------------- embassy-nrf/src/twim.rs | 8 +++++++- embassy-nrf/src/uarte.rs | 6 +++++- 4 files changed, 53 insertions(+), 38 deletions(-) diff --git a/embassy-nrf/src/lib.rs b/embassy-nrf/src/lib.rs index b448f6ab..06e8235e 100644 --- a/embassy-nrf/src/lib.rs +++ b/embassy-nrf/src/lib.rs @@ -1,3 +1,39 @@ +//! ## EasyDMA considerations +//! +//! On nRF chips, peripherals can use the so called EasyDMA feature to offload the task of interacting +//! with peripherals. It takes care of sending/receiving data over a variety of bus protocols (TWI/I2C, UART, SPI). +//! However, EasyDMA requires the buffers used to transmit and receive data to reside in RAM. Unfortunately, Rust +//! slices will not always do so. The following example using the SPI peripheral shows a common situation where this might happen: +//! +//! ```no_run +//! // As we pass a slice to the function whose contents will not ever change, +//! // the compiler writes it into the flash and thus the pointer to it will +//! // reference static memory. Since EasyDMA requires slices to reside in RAM, +//! // this function call will fail. +//! let result = spim.write_from_ram(&[1, 2, 3]); +//! assert_eq!(result, Err(Error::DMABufferNotInDataMemory)); +//! +//! // The data is still static and located in flash. However, since we are assigning +//! // it to a variable, the compiler will load it into memory. Passing a reference to the +//! // variable will yield a pointer that references dynamic memory, thus making EasyDMA happy. +//! // This function call succeeds. +//! let data = [1, 2, 3]; +//! let result = spim.write_from_ram(&data); +//! assert!(result.is_ok()); +//! ``` +//! +//! Each peripheral struct which uses EasyDMA ([`Spim`](spim::Spim), [`Uarte`](uarte::Uarte), [`Twim`](twim::Twim)) has two variants of their mutating functions: +//! - Functions with the suffix (e.g. [`write_from_ram`](Spim::write_from_ram), [`transfer_from_ram`](Spim::transfer_from_ram)) will return an error if the passed slice does not reside in RAM. +//! - Functions without the suffix (e.g. [`write`](Spim::write), [`transfer`](Spim::transfer)) will check whether the data is in RAM and copy it into memory prior to transmission. +//! +//! Since copying incurs a overhead, you are given the option to choose from `_from_ram` variants which will +//! fail and notify you, or the more convenient versions without the suffix which are potentially a little bit +//! more inefficient. Be aware that this overhead is not only in terms of instruction count but also in terms of memory usage +//! as the methods without the suffix will be allocating a statically sized buffer (up to 512 bytes for the nRF52840). +//! +//! Note that the methods that read data like [`read`](spim::Spim::read) and [`transfer_in_place`](spim::Spim::transfer_in_place) do not have the corresponding `_from_ram` variants as +//! mutable slices always reside in RAM. + #![no_std] #![cfg_attr( feature = "nightly", diff --git a/embassy-nrf/src/spim.rs b/embassy-nrf/src/spim.rs index c9c9cb25..5d88b232 100644 --- a/embassy-nrf/src/spim.rs +++ b/embassy-nrf/src/spim.rs @@ -31,38 +31,7 @@ pub enum Error { /// Interface for the SPIM peripheral using EasyDMA to offload the transmission and reception workload. /// -/// ## Data locality requirements -/// -/// On nRF chips, EasyDMA requires the buffers to reside in RAM. However, Rust -/// slices will not always do so. Take the following example: -/// -/// ```no_run -/// // As we pass a slice to the function whose contents will not ever change, -/// // the compiler writes it into the flash and thus the pointer to it will -/// // reference static memory. Since EasyDMA requires slices to reside in RAM, -/// // this function call will fail. -/// let result = spim.write_from_ram(&[1, 2, 3]); -/// assert_eq!(result, Error::DMABufferNotInDataMemory); -/// -/// // The data is still static and located in flash. However, since we are assigning -/// // it to a variable, the compiler will load it into memory. Passing a reference to the -/// // variable will yield a pointer that references dynamic memory, thus making EasyDMA happy. -/// // This function call succeeds. -/// let data = [1, 2, 3]; -/// let result = spim.write_from_ram(&data); -/// assert!(result.is_ok()); -/// ``` -/// -/// Each function in this struct has a `_from_ram` variant and one without this suffix. -/// - Functions with the suffix (e.g. [`write_from_ram`](Spim::write_from_ram), [`transfer_from_ram`](Spim::transfer_from_ram)) will return an error if the passed slice does not reside in RAM. -/// - Functions without the suffix (e.g. [`write`](Spim::write), [`transfer`](Spim::transfer)) will check whether the data is in RAM and copy it into memory prior to transmission. -/// -/// Since copying incurs a overhead, you are given the option to choose from `_from_ram` variants which will -/// fail and notify you, or the more convenient versions without the suffix which are potentially a little bit -/// more inefficient. -/// -/// Note that the [`read`](Spim::read) and [`transfer_in_place`](Spim::transfer_in_place) methods do not have the corresponding `_from_ram` variants as -/// mutable slices always reside in RAM. +/// For more details about EasyDMA, consult the module documentation. pub struct Spim<'d, T: Instance> { phantom: PhantomData<&'d mut T>, } @@ -325,7 +294,7 @@ impl<'d, T: Instance> Spim<'d, T> { self.blocking_inner(read, write) } - /// Same as [`blocking_transfer`](Spim::blocking_transfer) but will fail instead of copying data into RAM. + /// Same as [`blocking_transfer`](Spim::blocking_transfer) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_transfer_from_ram( &mut self, read: &mut [u8], @@ -346,7 +315,7 @@ impl<'d, T: Instance> Spim<'d, T> { self.blocking_inner(&mut [], data) } - /// Same as [`blocking_write`](Spim::blocking_write) but will fail instead of copying data into RAM. + /// Same as [`blocking_write`](Spim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_from_ram(&mut self, data: &[u8]) -> Result<(), Error> { self.blocking_inner(&mut [], data) } @@ -362,7 +331,7 @@ impl<'d, T: Instance> Spim<'d, T> { self.async_inner(read, write).await } - /// Same as [`transfer`](Spim::transfer) but will fail instead of copying data into RAM. + /// Same as [`transfer`](Spim::transfer) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn transfer_from_ram(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Error> { self.async_inner_from_ram(read, write).await } @@ -378,7 +347,7 @@ impl<'d, T: Instance> Spim<'d, T> { self.async_inner(&mut [], data).await } - /// Same as [`write`](Spim::write) but will fail instead of copying data into RAM. + /// Same as [`write`](Spim::write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_from_ram(&mut self, data: &[u8]) -> Result<(), Error> { self.async_inner_from_ram(&mut [], data).await } diff --git a/embassy-nrf/src/twim.rs b/embassy-nrf/src/twim.rs index c8ad2a0e..40705477 100644 --- a/embassy-nrf/src/twim.rs +++ b/embassy-nrf/src/twim.rs @@ -64,7 +64,9 @@ pub enum Error { Overrun, } -/// Interface to a TWIM instance. +/// Interface to a TWIM instance using EasyDMA to offload the transmission and reception workload. +/// +/// For more details about EasyDMA, consult the module documentation. pub struct Twim<'d, T: Instance> { phantom: PhantomData<&'d mut T>, } @@ -432,6 +434,7 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + /// Same as [`blocking_write`](Twim::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { self.setup_write_from_ram(address, buffer, false)?; self.blocking_wait(); @@ -474,6 +477,7 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + /// Same as [`blocking_write_read`](Twim::blocking_write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_read_from_ram( &mut self, address: u8, @@ -507,6 +511,7 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + /// Same as [`write`](Twim::write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_from_ram(&mut self, address: u8, buffer: &[u8]) -> Result<(), Error> { self.setup_write_from_ram(address, buffer, true)?; self.async_wait().await; @@ -531,6 +536,7 @@ impl<'d, T: Instance> Twim<'d, T> { Ok(()) } + /// Same as [`write_read`](Twim::write_read) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_read_from_ram( &mut self, address: u8, diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 7d7b904b..111c8341 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -60,7 +60,9 @@ pub enum Error { // TODO: add other error variants. } -/// Interface to the UARTE peripheral +/// Interface to the UARTE peripheral using EasyDMA to offload the transmission and reception workload. +/// +/// For more details about EasyDMA, consult the module documentation. pub struct Uarte<'d, T: Instance> { phantom: PhantomData<&'d mut T>, tx: UarteTx<'d, T>, @@ -224,6 +226,7 @@ impl<'d, T: Instance> Uarte<'d, T> { self.tx.write(buffer).await } + /// Same as [`write`](Uarte::write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub async fn write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { self.tx.write_from_ram(buffer).await } @@ -236,6 +239,7 @@ impl<'d, T: Instance> Uarte<'d, T> { self.tx.blocking_write(buffer) } + /// Same as [`blocking_write`](Uarte::blocking_write) but will fail instead of copying data into RAM. Consult the module level documentation to learn more. pub fn blocking_write_from_ram(&mut self, buffer: &[u8]) -> Result<(), Error> { self.tx.blocking_write_from_ram(buffer) }