Circular dma improvements (#2409)

* Check for error when pushing into a circular dma transaction too late

* Adapt tests

* Adapt example

* Don't block forever if trxing to pop from a circular DMA transfer too late

* Have a dedicated error for circular-DMA reading/writing too late

* Stop I2S RX before resetting it

* Migration guide

* Address review comment, make CI pass

* Adopt ideas from review

* Fix

* Update esp-hal/MIGRATING-0.21.md

Co-authored-by: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>

* assert

---------

Co-authored-by: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>

esp-hal/CHANGELOG.md

@@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Renamed `SpiDma` functions: `dma_transfer` to `transfer`, `dma_write` to `write`, `dma_read` to `read`. (#2373)
 - Peripheral type erasure for UART (#2381)
 - Changed listening for UART events (#2406)
+- Circular DMA transfers now correctly error, `available` returns `Result<usize,DmaError>` now (#2409)
 
 ### Fixed
 

esp-hal/MIGRATING-0.21.md

@@ -160,3 +160,19 @@ You can now listen/unlisten multiple interrupt bits at once:
 -uart0.listen_rx_fifo_full();
 +uart0.listen(UartInterrupt::AtCmd | UartConterrupt::RxFifoFull);
 ```˛
+## Circular DMA transfer's `available` returns `Result<usize, DmaError>` now
+
+In case of any error you should drop the transfer and restart it.
+
+```diff
+     loop {
+-        let avail = transfer.available();
++        let avail = match transfer.available() {
++            Ok(avail) => avail,
++            Err(_) => {
++                core::mem::drop(transfer);
++                transfer = i2s_tx.write_dma_circular(&tx_buffer).unwrap();
++                continue;
++            },
++        };
+```

esp-hal/src/dma/gdma.rs

@@ -151,6 +151,12 @@ impl<C: GdmaChannel> RegisterAccess for ChannelTxImpl<C> {
 }
 
 impl<C: GdmaChannel> TxRegisterAccess for ChannelTxImpl<C> {
+    fn set_auto_write_back(&self, enable: bool) {
+        self.ch()
+            .out_conf0()
+            .modify(|_, w| w.out_auto_wrback().bit(enable));
+    }
+
     fn last_dscr_address(&self) -> usize {
         self.ch()
             .out_eof_des_addr()

esp-hal/src/dma/mod.rs

@@ -781,6 +781,9 @@ pub enum DmaError {
     UnsupportedMemoryRegion,
     /// Invalid DMA chunk size
     InvalidChunkSize,
+    /// Indicates writing to or reading from a circular DMA transaction is done
+    /// too late and the DMA buffers already overrun / underrun.
+    Late,
 }
 
 impl From<DmaBufError> for DmaError {
@@ -1313,7 +1316,7 @@ impl TxCircularState {
         }
     }
 
-    pub(crate) fn update<T>(&mut self, channel: &T)
+    pub(crate) fn update<T>(&mut self, channel: &T) -> Result<(), DmaError>
     where
         T: Tx,
     {
@@ -1323,6 +1326,23 @@ impl TxCircularState {
         {
             channel.clear_out(DmaTxInterrupt::Eof);
 
+            // check if all descriptors are owned by CPU - this indicates we failed to push
+            // data fast enough in future we can enable `check_owner` and check
+            // the interrupt instead
+            let mut current = self.last_seen_handled_descriptor_ptr;
+            loop {
+                let descr = unsafe { current.read_volatile() };
+                if descr.owner() == Owner::Cpu {
+                    current = descr.next;
+                } else {
+                    break;
+                }
+
+                if current == self.last_seen_handled_descriptor_ptr {
+                    return Err(DmaError::Late);
+                }
+            }
+
             let descr_address = channel.last_out_dscr_address() as *mut DmaDescriptor;
 
             let mut ptr = self.last_seen_handled_descriptor_ptr;
@@ -1376,6 +1396,8 @@ impl TxCircularState {
 
             self.last_seen_handled_descriptor_ptr = descr_address;
         }
+
+        Ok(())
     }
 
     pub(crate) fn push(&mut self, data: &[u8]) -> Result<usize, DmaError> {
@@ -1404,6 +1426,8 @@ impl TxCircularState {
         &mut self,
         f: impl FnOnce(&mut [u8]) -> usize,
     ) -> Result<usize, DmaError> {
+        // this might write less than available in case of a wrap around
+        // caller needs to check and write the remaining part
         let written = unsafe {
             let dst = self.buffer_start.add(self.write_offset).cast_mut();
             let block_size = usize::min(self.available, self.buffer_len - self.write_offset);
@@ -1414,12 +1438,15 @@ impl TxCircularState {
         let mut forward = written;
         loop {
             unsafe {
-                let dw0 = self.write_descr_ptr.read_volatile();
-                let segment_len = dw0.len();
-                self.write_descr_ptr = if dw0.next.is_null() {
+                let mut descr = self.write_descr_ptr.read_volatile();
+                descr.set_owner(Owner::Dma);
+                self.write_descr_ptr.write_volatile(descr);
+
+                let segment_len = descr.len();
+                self.write_descr_ptr = if descr.next.is_null() {
                     self.first_desc_ptr
                 } else {
-                    dw0.next
+                    descr.next
                 };
 
                 if forward <= segment_len {
@@ -1454,7 +1481,7 @@ impl RxCircularState {
         }
     }
 
-    pub(crate) fn update(&mut self) {
+    pub(crate) fn update(&mut self) -> Result<(), DmaError> {
         if self.last_seen_handled_descriptor_ptr.is_null() {
             // initially start at last descriptor (so that next will be the first
             // descriptor)
@@ -1465,6 +1492,7 @@ impl RxCircularState {
             unsafe { self.last_seen_handled_descriptor_ptr.read_volatile() }.next;
         let mut current_in_descr = unsafe { current_in_descr_ptr.read_volatile() };
 
+        let last_seen_ptr = self.last_seen_handled_descriptor_ptr;
         while current_in_descr.owner() == Owner::Cpu {
             self.available += current_in_descr.len();
             self.last_seen_handled_descriptor_ptr = current_in_descr_ptr;
@@ -1472,7 +1500,13 @@ impl RxCircularState {
             current_in_descr_ptr =
                 unsafe { self.last_seen_handled_descriptor_ptr.read_volatile() }.next;
             current_in_descr = unsafe { current_in_descr_ptr.read_volatile() };
+
+            if current_in_descr_ptr == last_seen_ptr {
+                return Err(DmaError::Late);
+            }
         }
+
+        Ok(())
     }
 
     pub(crate) fn pop(&mut self, data: &mut [u8]) -> Result<usize, DmaError> {
@@ -1902,6 +1936,10 @@ where
         self.tx_impl.set_link_addr(chain.first() as u32);
         self.tx_impl.set_peripheral(peri as u8);
 
+        // enable descriptor write back in circular mode
+        self.tx_impl
+            .set_auto_write_back(!(*chain.last()).next.is_null());
+
         Ok(())
     }
 
@@ -2035,6 +2073,9 @@ pub trait RxRegisterAccess: RegisterAccess {
 
 #[doc(hidden)]
 pub trait TxRegisterAccess: RegisterAccess {
+    /// Enable/disable outlink-writeback
+    fn set_auto_write_back(&self, enable: bool);
+
     /// Outlink descriptor address when EOF occurs of Tx channel.
     fn last_dscr_address(&self) -> usize;
 }
@@ -2378,14 +2419,14 @@ where
     }
 
     /// Amount of bytes which can be pushed.
-    pub fn available(&mut self) -> usize {
-        self.state.update(self.instance.tx());
-        self.state.available
+    pub fn available(&mut self) -> Result<usize, DmaError> {
+        self.state.update(self.instance.tx())?;
+        Ok(self.state.available)
     }
 
     /// Push bytes into the DMA buffer.
     pub fn push(&mut self, data: &[u8]) -> Result<usize, DmaError> {
-        self.state.update(self.instance.tx());
+        self.state.update(self.instance.tx())?;
         self.state.push(data)
     }
 
@@ -2394,7 +2435,7 @@ where
     /// The closure *might* get called with a slice which is smaller than the
     /// total available buffer.
     pub fn push_with(&mut self, f: impl FnOnce(&mut [u8]) -> usize) -> Result<usize, DmaError> {
-        self.state.update(self.instance.tx());
+        self.state.update(self.instance.tx())?;
         self.state.push_with(f)
     }
 
@@ -2454,9 +2495,9 @@ where
     ///
     /// It's expected to call this before trying to [DmaTransferRxCircular::pop]
     /// data.
-    pub fn available(&mut self) -> usize {
-        self.state.update();
-        self.state.available
+    pub fn available(&mut self) -> Result<usize, DmaError> {
+        self.state.update()?;
+        Ok(self.state.available)
     }
 
     /// Get available data.
@@ -2468,7 +2509,7 @@ where
     /// Fails with [DmaError::BufferTooSmall] if the given buffer is too small
     /// to hold all available data
     pub fn pop(&mut self, data: &mut [u8]) -> Result<usize, DmaError> {
-        self.state.update();
+        self.state.update()?;
         self.state.pop(data)
     }
 }

esp-hal/src/dma/pdma.rs

@@ -90,6 +90,11 @@ impl<C: PdmaChannel<RegisterBlock = SpiRegisterBlock>> RegisterAccess for SpiDma
 }
 
 impl<C: PdmaChannel<RegisterBlock = SpiRegisterBlock>> TxRegisterAccess for SpiDmaTxChannelImpl<C> {
+    fn set_auto_write_back(&self, enable: bool) {
+        // there is no `auto_wrback` for SPI
+        assert!(!enable);
+    }
+
     fn last_dscr_address(&self) -> usize {
         let spi = self.0.register_block();
         spi.out_eof_des_addr().read().dma_out_eof_des_addr().bits() as usize
@@ -507,6 +512,13 @@ impl<C: PdmaChannel<RegisterBlock = I2sRegisterBlock>> RegisterAccess for I2sDma
 }
 
 impl<C: PdmaChannel<RegisterBlock = I2sRegisterBlock>> TxRegisterAccess for I2sDmaTxChannelImpl<C> {
+    fn set_auto_write_back(&self, enable: bool) {
+        let reg_block = self.0.register_block();
+        reg_block
+            .lc_conf()
+            .modify(|_, w| w.out_auto_wrback().bit(enable));
+    }
+
     fn last_dscr_address(&self) -> usize {
         let reg_block = self.0.register_block();
         reg_block

esp-hal/src/i2s.rs

@@ -64,7 +64,7 @@
 //! let mut transfer = i2s_rx.read_dma_circular(&mut rx_buffer).unwrap();
 //!
 //! loop {
-//!     let avail = transfer.available();
+//!     let avail = transfer.available().unwrap();
 //!
 //!     if avail > 0 {
 //!         let mut rcv = [0u8; 5000];
@@ -1545,6 +1545,9 @@ mod private {
 
         fn reset_rx(&self) {
             let i2s = self.register_block();
+
+            i2s.rx_conf().modify(|_, w| w.rx_start().clear_bit());
+
             i2s.rx_conf().modify(|_, w| {
                 w.rx_reset().set_bit();
                 w.rx_fifo_reset().set_bit()
@@ -1954,7 +1957,7 @@ pub mod asynch {
         /// Will wait for more than 0 bytes available.
         pub async fn available(&mut self) -> Result<usize, Error> {
             loop {
-                self.state.update(&self.i2s_tx.tx_channel);
+                self.state.update(&self.i2s_tx.tx_channel)?;
                 let res = self.state.available;
 
                 if res != 0 {
@@ -2077,7 +2080,7 @@ pub mod asynch {
         /// Will wait for more than 0 bytes available.
         pub async fn available(&mut self) -> Result<usize, Error> {
             loop {
-                self.state.update();
+                self.state.update()?;
 
                 let res = self.state.available;
 

examples/src/bin/i2s_read.rs

@@ -68,7 +68,7 @@ fn main() -> ! {
     println!("Started transfer");
 
     loop {
-        let avail = transfer.available();
+        let avail = transfer.available().unwrap();
 
         if avail > 0 {
             let mut rcv = [0u8; 5000];

examples/src/bin/i2s_sound.rs

@@ -96,7 +96,7 @@ fn main() -> ! {
     let mut transfer = i2s_tx.write_dma_circular(&tx_buffer).unwrap();
 
     loop {
-        let avail = transfer.available();
+        let avail = transfer.available().unwrap();
         if avail > 0 {
             let avail = usize::min(10000, avail);
             for bidx in 0..avail {

hil-test/tests/i2s.rs

@@ -235,7 +235,7 @@ mod tests {
         assert_eq!(0, rx_transfer.pop(&mut rcv[..100]).unwrap());
 
         // no data available yet
-        assert_eq!(0, rx_transfer.available());
+        assert_eq!(0, rx_transfer.available().unwrap());
 
         let mut tx_transfer = i2s_tx.write_dma_circular(tx_buffer).unwrap();
 
@@ -243,7 +243,7 @@ mod tests {
         let mut sample_idx = 0;
         let mut check_samples = SampleSource::new();
         loop {
-            let tx_avail = tx_transfer.available();
+            let tx_avail = tx_transfer.available().unwrap();
 
             // make sure there are more than one descriptor buffers ready to push
             if tx_avail > 5000 {
@@ -254,13 +254,13 @@ mod tests {
             }
 
             // test calling available multiple times doesn't break anything
-            rx_transfer.available();
-            rx_transfer.available();
-            rx_transfer.available();
-            rx_transfer.available();
-            rx_transfer.available();
-            rx_transfer.available();
-            let rx_avail = rx_transfer.available();
+            rx_transfer.available().unwrap();
+            rx_transfer.available().unwrap();
+            rx_transfer.available().unwrap();
+            rx_transfer.available().unwrap();
+            rx_transfer.available().unwrap();
+            rx_transfer.available().unwrap();
+            let rx_avail = rx_transfer.available().unwrap();
 
             // make sure there are more than one descriptor buffers ready to pop
             if rx_avail > 0 {
@@ -298,4 +298,69 @@ mod tests {
             }
         }
     }
+
+    #[test]
+    fn test_i2s_push_too_late(ctx: Context) {
+        let (_, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(0, 16000);
+
+        let i2s = I2s::new(
+            ctx.i2s,
+            Standard::Philips,
+            DataFormat::Data16Channel16,
+            16000.Hz(),
+            ctx.dma_channel.configure(false, DmaPriority::Priority0),
+            rx_descriptors,
+            tx_descriptors,
+        );
+
+        let (_, dout) = hil_test::common_test_pins!(ctx.io);
+
+        let mut i2s_tx = i2s
+            .i2s_tx
+            .with_bclk(NoPin)
+            .with_ws(NoPin)
+            .with_dout(dout)
+            .build();
+
+        let mut tx_transfer = i2s_tx.write_dma_circular(tx_buffer).unwrap();
+
+        let delay = esp_hal::delay::Delay::new();
+        delay.delay_millis(300);
+
+        assert!(matches!(tx_transfer.push(&[0; 128]), Err(_)));
+    }
+
+    #[test]
+    #[timeout(1)]
+    fn test_i2s_read_too_late(ctx: Context) {
+        let (rx_buffer, rx_descriptors, _, tx_descriptors) = dma_buffers!(16000, 0);
+
+        let i2s = I2s::new(
+            ctx.i2s,
+            Standard::Philips,
+            DataFormat::Data16Channel16,
+            16000.Hz(),
+            ctx.dma_channel.configure(false, DmaPriority::Priority0),
+            rx_descriptors,
+            tx_descriptors,
+        );
+
+        let (_, dout) = hil_test::common_test_pins!(ctx.io);
+        let din = dout.peripheral_input();
+
+        let mut i2s_rx = i2s
+            .i2s_rx
+            .with_bclk(NoPin)
+            .with_ws(NoPin)
+            .with_din(din)
+            .build();
+
+        let mut buffer = [0u8; 1024];
+        let mut rx_transfer = i2s_rx.read_dma_circular(rx_buffer).unwrap();
+
+        let delay = esp_hal::delay::Delay::new();
+        delay.delay_millis(300);
+
+        assert!(matches!(rx_transfer.pop(&mut buffer), Err(_)));
+    }
 }