SPI: stop transfer if Spi::transfer_in_place is cancelled (#3242)

* Move OnDrop

* Abort async transfer

* Finish sentence

Author: bugadani

esp-hal/CHANGELOG.md

@@ -28,7 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Uart::{with_tx, with_rx}` can now be called on the async driver as well (#3212)
 - ESP32: Fixed SPI3 QSPI signals (#3201)
 - ESP32-C6/H2: The `flip_link` feature should no longer crash (#3203)
-
+- SPI: `Spi::transfer_in_place_async` now stops the transfer when cancelled (#3242)
 - ESP32/ESP32-S2: Avoid running into timeouts with reads/writes larger than the FIFO (#3199)
 
 - ESP32-C6: Keep ADC enabled to improve radio signal strength (#3249)

esp-hal/src/lib.rs

@@ -367,6 +367,8 @@ impl crate::private::Sealed for Blocking {}
 impl crate::private::Sealed for Async {}
 
 pub(crate) mod private {
+    use core::mem::ManuallyDrop;
+
     pub trait Sealed {}
 
     #[non_exhaustive]
@@ -390,6 +392,23 @@ pub(crate) mod private {
             Self
         }
     }
+
+    pub(crate) struct OnDrop<F: FnOnce()>(ManuallyDrop<F>);
+    impl<F: FnOnce()> OnDrop<F> {
+        pub fn new(cb: F) -> Self {
+            Self(ManuallyDrop::new(cb))
+        }
+
+        pub fn defuse(self) {
+            core::mem::forget(self);
+        }
+    }
+
+    impl<F: FnOnce()> Drop for OnDrop<F> {
+        fn drop(&mut self) {
+            unsafe { (ManuallyDrop::take(&mut self.0))() }
+        }
+    }
 }
 
 #[cfg(feature = "unstable")]

esp-hal/src/spi/master.rs

@@ -59,7 +59,7 @@ use crate::{
     interrupt::InterruptHandler,
     pac::spi2::RegisterBlock,
     peripheral::{Peripheral, PeripheralRef},
-    private::{self, Sealed},
+    private::{self, OnDrop, Sealed},
     spi::AnySpi,
     system::{Cpu, PeripheralGuard},
     time::Rate,
@@ -849,7 +849,12 @@ impl<'d> Spi<'d, Async> {
         Ok(())
     }
 
-    /// Sends `words` to the slave. Returns the `words` received from the slave
+    /// Sends `words` to the slave. Returns the `words` received from the slave.
+    ///
+    /// This function aborts the transfer when its Future is dropped. Some
+    /// amount of data may have been transferred before the Future is
+    /// dropped. Dropping the future may block for a short while to ensure
+    /// the transfer is aborted.
     pub async fn transfer_in_place_async(&mut self, words: &mut [u8]) -> Result<(), Error> {
         // We need to flush because the blocking transfer functions may return while a
         // transfer is still in progress.
@@ -2871,6 +2876,22 @@ impl Driver {
         unsafe { &*self.info.register_block }
     }
 
+    fn abort_transfer(&self) {
+        // Note(danielb): This method came later than DmaDriver::abort_transfer. That
+        // function works for DMA so I have left it unchanged, but does not work
+        // for CPU-controlled transfers on the ESP32. Toggling slave mode works on
+        // ESP32, but not on later chips.
+        cfg_if::cfg_if! {
+            if #[cfg(esp32)] {
+                self.regs().slave().modify(|_, w| w.mode().set_bit());
+                self.regs().slave().modify(|_, w| w.mode().clear_bit());
+            } else {
+                self.configure_datalen(1, 1);
+            }
+        }
+        self.update();
+    }
+
     /// Initialize for full-duplex 1 bit mode
     fn init(&self) {
         self.regs().user().modify(|_, w| {
@@ -3390,8 +3411,7 @@ impl Driver {
     }
 
     fn busy(&self) -> bool {
-        let reg_block = self.regs();
-        reg_block.cmd().read().usr().bit_is_set()
+        self.regs().cmd().read().usr().bit_is_set()
     }
 
     // Check if the bus is busy and if it is wait for it to be idle
@@ -3416,7 +3436,16 @@ impl Driver {
     #[cfg_attr(place_spi_driver_in_ram, ram)]
     async fn transfer_in_place_async(&self, words: &mut [u8]) -> Result<(), Error> {
         for chunk in words.chunks_mut(FIFO_SIZE) {
-            self.write_async(chunk).await?;
+            // Cut the transfer short if the future is dropped. We'll block for a short
+            // while to ensure the peripheral is idle.
+            let cancel_on_drop = OnDrop::new(|| {
+                self.abort_transfer();
+                while self.busy() {}
+            });
+            let res = self.write_async(chunk).await;
+            cancel_on_drop.defuse();
+            res?;
+
             self.read_from_fifo(chunk)?;
         }
 

esp-hal/src/uart.rs

@@ -62,6 +62,7 @@ use crate::{
     pac::uart0::RegisterBlock,
     peripheral::{Peripheral, PeripheralRef},
     peripherals::Interrupt,
+    private::OnDrop,
     system::{PeripheralClockControl, PeripheralGuard},
     Async,
     Blocking,
@@ -3052,18 +3053,3 @@ impl Instance for AnyUart {
         }
     }
 }
-
-struct OnDrop<F: FnOnce()>(Option<F>);
-impl<F: FnOnce()> OnDrop<F> {
-    fn new(cb: F) -> Self {
-        Self(Some(cb))
-    }
-}
-
-impl<F: FnOnce()> Drop for OnDrop<F> {
-    fn drop(&mut self) {
-        if let Some(cb) = self.0.take() {
-            cb();
-        }
-    }
-}

hil-test/tests/spi_full_duplex.rs

@@ -12,6 +12,8 @@
 use embedded_hal::spi::SpiBus;
 use embedded_hal_async::spi::SpiBus as SpiBusAsync;
 use esp_hal::{
+    gpio::Input,
+    peripheral::Peripheral,
     spi::master::{Config, Spi},
     time::Rate,
     Blocking,
@@ -26,10 +28,7 @@ cfg_if::cfg_if! {
             gpio::{Level, NoPin},
         };
         #[cfg(pcnt)]
-        use esp_hal::{
-            gpio::interconnect::InputSignal,
-            pcnt::{channel::EdgeMode, unit::Unit, Pcnt},
-        };
+        use esp_hal::pcnt::{channel::EdgeMode, unit::Unit, Pcnt};
     }
 }
 
@@ -53,8 +52,7 @@ struct Context {
     tx_buffer: &'static mut [u8],
     #[cfg(feature = "unstable")]
     tx_descriptors: &'static mut [DmaDescriptor],
-    #[cfg(all(pcnt, feature = "unstable"))]
-    pcnt_source: InputSignal,
+    miso_input: Input<'static>,
     #[cfg(all(pcnt, feature = "unstable"))]
     pcnt_unit: Unit<'static, 0>,
 }
@@ -70,7 +68,13 @@ mod tests {
             esp_hal::Config::default().with_cpu_clock(esp_hal::clock::CpuClock::max()),
         );
 
-        let (_, mosi) = hil_test::common_test_pins!(peripherals);
+        let (_, miso) = hil_test::common_test_pins!(peripherals);
+
+        // A bit ugly but the peripheral interconnect APIs aren't yet stable.
+        let mosi = unsafe { miso.clone_unchecked() };
+        let miso_input = unsafe { miso.clone_unchecked() };
+        // Will be used later to detect edges directly or through PCNT.
+        let miso_input = Input::new(miso_input, Default::default());
 
         #[cfg(feature = "unstable")]
         cfg_if::cfg_if! {
@@ -83,16 +87,8 @@ mod tests {
 
         cfg_if::cfg_if! {
             if #[cfg(feature = "unstable")] {
-                let (miso, mosi) = mosi.split();
-
-                #[cfg(pcnt)]
-                let mosi_loopback_pcnt = miso.clone();
-
                 let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);
             } else {
-                use esp_hal::peripheral::Peripheral;
-                let miso = unsafe { mosi.clone_unchecked() };
-
                 static mut TX_BUFFER: [u8; 4096] = [0; 4096];
                 static mut RX_BUFFER: [u8; 4096] = [0; 4096];
 
@@ -121,19 +117,19 @@ mod tests {
                     spi,
                     rx_buffer,
                     tx_buffer,
+                    miso_input,
                     dma_channel,
                     rx_descriptors,
                     tx_descriptors,
                     #[cfg(pcnt)]
-                    pcnt_source: mosi_loopback_pcnt,
-                    #[cfg(pcnt)]
                     pcnt_unit: pcnt.unit0,
                 }
             } else {
                 Context {
                     spi,
                     rx_buffer,
                     tx_buffer,
+                    miso_input,
                 }
             }
         }
@@ -192,7 +188,8 @@ mod tests {
 
         let unit = ctx.pcnt_unit;
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -210,7 +207,8 @@ mod tests {
 
         let unit = ctx.pcnt_unit;
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -229,7 +227,8 @@ mod tests {
 
         let unit = ctx.pcnt_unit;
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -254,7 +253,8 @@ mod tests {
 
         let unit = ctx.pcnt_unit;
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -272,7 +272,8 @@ mod tests {
 
         let unit = ctx.pcnt_unit;
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -356,7 +357,8 @@ mod tests {
         let unit = ctx.pcnt_unit;
         let mut spi = ctx.spi.with_dma(ctx.dma_channel);
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -396,7 +398,8 @@ mod tests {
         let unit = ctx.pcnt_unit;
         let mut spi = ctx.spi.with_dma(ctx.dma_channel);
 
-        unit.channel0.set_edge_signal(ctx.pcnt_source);
+        unit.channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         unit.channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
@@ -500,7 +503,9 @@ mod tests {
         let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
         let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
 
-        ctx.pcnt_unit.channel0.set_edge_signal(ctx.pcnt_source);
+        ctx.pcnt_unit
+            .channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         ctx.pcnt_unit
             .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
@@ -601,7 +606,9 @@ mod tests {
             .with_buffers(dma_rx_buf, dma_tx_buf)
             .into_async();
 
-        ctx.pcnt_unit.channel0.set_edge_signal(ctx.pcnt_source);
+        ctx.pcnt_unit
+            .channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         ctx.pcnt_unit
             .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
@@ -635,7 +642,9 @@ mod tests {
             .with_buffers(dma_rx_buf, dma_tx_buf)
             .into_async();
 
-        ctx.pcnt_unit.channel0.set_edge_signal(ctx.pcnt_source);
+        ctx.pcnt_unit
+            .channel0
+            .set_edge_signal(ctx.miso_input.peripheral_input());
         ctx.pcnt_unit
             .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
@@ -701,9 +710,53 @@ mod tests {
         assert_eq!(&[0xff, 0xff, 0xff, 0xff], dma_rx_buf.as_slice());
     }
 
+    #[test]
+    async fn cancel_stops_basic_async_spi_transfer(mut ctx: Context) {
+        // Slow down. At 80kHz, the transfer is supposed to take a bit over 3 seconds.
+        // We don't rely on the transfer speed much, just that it's slow enough
+        // that we can detect pulses if cancelling the future leaves the transfer
+        // running.
+        ctx.spi
+            .apply_config(&Config::default().with_frequency(Rate::from_khz(800)))
+            .unwrap();
+
+        let mut spi = ctx.spi.into_async();
+
+        for i in 0..ctx.tx_buffer.len() {
+            ctx.tx_buffer[i] = (i % 256) as u8;
+        }
+
+        let transfer = spi.transfer_in_place_async(ctx.tx_buffer);
+
+        // Wait for a bit before cancelling
+        let cancel = async {
+            for _ in 0..100 {
+                embassy_futures::yield_now().await;
+            }
+        };
+
+        embassy_futures::select::select(transfer, cancel).await;
+
+        // Listen for a while to see if the SPI peripheral correctly stopped.
+        let detect_edge = ctx.miso_input.wait_for_any_edge();
+        let wait = async {
+            for _ in 0..10000 {
+                embassy_futures::yield_now().await;
+            }
+        };
+
+        let result = embassy_futures::select::select(detect_edge, wait).await;
+
+        // Assert that we timed out - we should not have detected any edges
+        assert!(
+            matches!(result, embassy_futures::select::Either::Second(_)),
+            "Detected edge after cancellation"
+        );
+    }
+
     #[test]
     #[cfg(feature = "unstable")]
-    fn cancel_stops_transaction(mut ctx: Context) {
+    fn cancel_stops_dma_transaction(mut ctx: Context) {
         // Slow down. At 80kHz, the transfer is supposed to take a bit over 3 seconds.
         // This means that without working cancellation, the test case should
         // fail.