Merge #640

640: Skip EasyDMA slice location check for empty slices and copy data if necessary r=Dirbaio a=TilBlechschmidt

As discussed, this PR makes the following changes:
- Ignore pointer location of zero-length slices (fixes #631)
- Change default functions so they copy the tx buffer if it does not reside in RAM
- Introduce new variants for `write`, `transfer`, and their blocking versions which fails instead of copying
- Add documentation about the motivation behind all these variants
<img width="984" alt="image" src="https://user-images.githubusercontent.com/5037967/155415788-c2cd1055-9289-4004-959d-be3b1934a439.png">


Remaining TODOs:

- [x] Change copying behaviour for other peripherals
    - [x] TWI
    - [x] UART
- [x] Add module-level documentation regarding EasyDMA and `_from_ram` method variants

`@Dirbaio` it probably makes sense for you to review it now before I "copy" over the changes to the other two peripherals.

Co-authored-by: Til Blechschmidt <til@blechschmidt.de>

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",

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};
@@ -28,6 +29,9 @@ pub enum Error {
     DMABufferNotInDataMemory,
 }
 
+/// Interface for the SPIM peripheral using EasyDMA to offload the transmission and reception workload.
+///
+/// For more details about EasyDMA, consult the module documentation.
 pub struct Spim<'d, T: Instance> {
     phantom: PhantomData<&'d mut T>,
 }
@@ -223,7 +227,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 +238,20 @@ 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) => {
+                trace!("Copying SPIM tx buffer into RAM for DMA");
+                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),
+        }
+    }
+
+    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 +270,87 @@ 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) => {
+                trace!("Copying SPIM tx buffer into RAM for DMA");
+                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),
+        }
+    }
+
+    /// 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> {
+    /// Same as [`blocking_transfer`](Spim::blocking_transfer) but will fail instead of copying data into RAM. Consult the module level documentation to learn more.
-        self.blocking_inner(data, data)
+    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. 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)
+    }
+
+    /// 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> {
+    /// Same as [`transfer`](Spim::transfer) but will fail instead of copying data into RAM. Consult the module level documentation to learn more.
-        self.async_inner(data, data).await
+    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. 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
+    }
 }
 
 impl<'d, T: Instance> Drop for Spim<'d, T> {

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>,
 }
@@ -287,7 +289,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 +349,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 +389,38 @@ 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 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),
+        }
+    }
+
+    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 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),
+        }
+    }
+
     /// Write to an I2C slave.
     ///
     /// The buffer must have a length of at most 255 bytes on the nRF52832
@@ -395,6 +434,16 @@ 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();
+        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 +477,20 @@ impl<'d, T: Instance> Twim<'d, T> {
         Ok(())
     }
 
-    /// Copy data into RAM and write to an I2C slave.
+    /// 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(
-    /// 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(
         &mut self,
         address: u8,
         wr_buffer: &[u8],
         rd_buffer: &mut [u8],
     ) -> Result<(), Error> {
-        if wr_buffer.len() > FORCE_COPY_BUFFER_SIZE {
+        self.setup_write_read_from_ram(address, wr_buffer, rd_buffer, false)?;
-            return Err(Error::TxBufferTooLong);
+        self.blocking_wait();
-        }
+        compiler_fence(SeqCst);
-
+        self.check_errorsrc()?;
-        // Copy to RAM
+        self.check_tx(wr_buffer.len())?;
-        let wr_ram_buffer = &mut [0; FORCE_COPY_BUFFER_SIZE][..wr_buffer.len()];
+        self.check_rx(rd_buffer.len())?;
-        wr_ram_buffer.copy_from_slice(wr_buffer);
+        Ok(())
-
-        self.blocking_write_read(address, wr_ram_buffer, rd_buffer)
     }
 
     pub async fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> {
@@ -487,6 +511,16 @@ 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;
+        compiler_fence(SeqCst);
+        self.check_errorsrc()?;
+        self.check_tx(buffer.len())?;
+        Ok(())
+    }
+
     pub async fn write_read(
         &mut self,
         address: u8,
@@ -501,6 +535,22 @@ impl<'d, T: Instance> Twim<'d, T> {
         self.check_rx(rd_buffer.len())?;
         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,
+        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 +651,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)
-            }
         }
     }
 }

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;
@@ -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,11 @@ 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
+    }
+
     pub fn blocking_read(&mut self, buffer: &mut [u8]) -> Result<(), Error> {
         self.rx.blocking_read(buffer)
     }
@@ -231,10 +238,28 @@ impl<'d, T: Instance> Uarte<'d, T> {
     pub fn blocking_write(&mut self, buffer: &[u8]) -> Result<(), Error> {
         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)
+    }
 }
 
 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 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),
+        }
+    }
+
+    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 +314,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 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),
+        }
+    }
+
+    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);

embassy-nrf/src/util.rs

@@ -19,10 +19,11 @@ pub(crate) fn slice_in_ram<T>(slice: *const [T]) -> bool {
     ptr >= SRAM_LOWER && (ptr + len * core::mem::size_of::<T>()) < 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<T, E>(slice: *const [T], err: E) -> Result<(), E> {
-    if slice_in_ram(slice) {
+    let (_, len) = slice_ptr_parts(slice);
+    if len == 0 || slice_in_ram(slice) {
         Ok(())
     } else {
         Err(err)