Fix Rx/Tx order in SPI slave driver (#2272)

* Add basic test and fix SPI slave dma_transfer arg order

* Reset peripheral

* Add safe way to read signal value

esp-hal/src/gpio/interconnect.rs

@@ -63,6 +63,11 @@ impl InputSignal {
         }
     }
 
+    /// Returns the current signal level.
+    pub fn get_level(&self) -> Level {
+        self.is_input_high(private::Internal).into()
+    }
+
     /// Inverts the peripheral's input signal.
     ///
     /// Calling this function multiple times toggles the setting.

esp-hal/src/spi/slave.rs

@@ -73,7 +73,7 @@ use core::marker::PhantomData;
 use super::{Error, FullDuplexMode, SpiMode};
 use crate::{
     dma::{DescriptorChain, DmaPeripheral, Rx, Tx},
-    gpio::{InputSignal, OutputSignal, PeripheralInput, PeripheralOutput, Pull},
+    gpio::{InputSignal, OutputSignal, PeripheralInput, PeripheralOutput},
     peripheral::{Peripheral, PeripheralRef},
     peripherals::spi2::RegisterBlock,
     private,
@@ -110,16 +110,16 @@ where
     ) -> Spi<'d, T, FullDuplexMode> {
         crate::into_ref!(spi, sclk, mosi, miso, cs);
 
-        sclk.init_input(Pull::None, private::Internal);
+        sclk.enable_input(true, private::Internal);
         sclk.connect_input_to_peripheral(spi.sclk_signal(), private::Internal);
 
-        mosi.init_input(Pull::None, private::Internal);
+        mosi.enable_input(true, private::Internal);
         mosi.connect_input_to_peripheral(spi.mosi_signal(), private::Internal);
 
         miso.set_to_push_pull_output(private::Internal);
         miso.connect_peripheral_to_output(spi.miso_signal(), private::Internal);
 
-        cs.init_input(Pull::None, private::Internal);
+        cs.enable_input(true, private::Internal);
         cs.connect_input_to_peripheral(spi.cs_signal(), private::Internal);
 
         Self::new_internal(spi, mode)
@@ -129,6 +129,7 @@ where
         spi: PeripheralRef<'d, T>,
         mode: SpiMode,
     ) -> Spi<'d, T, FullDuplexMode> {
+        spi.reset_peripheral();
         spi.enable_peripheral();
 
         let mut spi = Spi {
@@ -366,8 +367,8 @@ pub mod dma {
         /// line.
         pub fn dma_transfer<'t, RXBUF, TXBUF>(
             &'t mut self,
-            words: &'t TXBUF,
             read_buffer: &'t mut RXBUF,
+            words: &'t TXBUF,
         ) -> Result<DmaTransferRxTx<'t, Self>, Error>
         where
             RXBUF: WriteBuffer,
@@ -633,7 +634,17 @@ pub trait Instance: private::Sealed {
 
     fn cs_signal(&self) -> InputSignal;
 
-    fn enable_peripheral(&self);
+    fn peripheral(&self) -> crate::system::Peripheral;
+
+    #[inline(always)]
+    fn reset_peripheral(&self) {
+        PeripheralClockControl::reset(self.peripheral());
+    }
+
+    #[inline(always)]
+    fn enable_peripheral(&self) {
+        PeripheralClockControl::enable(self.peripheral());
+    }
 
     fn spi_num(&self) -> u8;
 
@@ -737,13 +748,22 @@ pub trait Instance: private::Sealed {
     }
 }
 
-#[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2))]
 impl Instance for crate::peripherals::SPI2 {
     #[inline(always)]
     fn register_block(&self) -> &RegisterBlock {
         self
     }
 
+    #[inline(always)]
+    fn peripheral(&self) -> crate::system::Peripheral {
+        crate::system::Peripheral::Spi2
+    }
+
+    #[inline(always)]
+    fn spi_num(&self) -> u8 {
+        2
+    }
+
     #[inline(always)]
     fn sclk_signal(&self) -> InputSignal {
         InputSignal::FSPICLK
@@ -763,63 +783,25 @@ impl Instance for crate::peripherals::SPI2 {
     fn cs_signal(&self) -> InputSignal {
         InputSignal::FSPICS0
     }
-
-    #[inline(always)]
-    fn enable_peripheral(&self) {
-        PeripheralClockControl::enable(crate::system::Peripheral::Spi2);
 }
 
-    #[inline(always)]
+#[cfg(spi3)]
-    fn spi_num(&self) -> u8 {
-        2
-    }
-}
-
-#[cfg(any(esp32s2, esp32s3))]
-impl Instance for crate::peripherals::SPI2 {
-    #[inline(always)]
-    fn register_block(&self) -> &RegisterBlock {
-        self
-    }
-
-    #[inline(always)]
-    fn sclk_signal(&self) -> InputSignal {
-        InputSignal::FSPICLK
-    }
-
-    #[inline(always)]
-    fn mosi_signal(&self) -> InputSignal {
-        InputSignal::FSPID
-    }
-
-    #[inline(always)]
-    fn miso_signal(&self) -> OutputSignal {
-        OutputSignal::FSPIQ
-    }
-
-    #[inline(always)]
-    fn cs_signal(&self) -> InputSignal {
-        InputSignal::FSPICS0
-    }
-
-    #[inline(always)]
-    fn enable_peripheral(&self) {
-        PeripheralClockControl::enable(crate::system::Peripheral::Spi2)
-    }
-
-    #[inline(always)]
-    fn spi_num(&self) -> u8 {
-        2
-    }
-}
-
-#[cfg(any(esp32s2, esp32s3))]
 impl Instance for crate::peripherals::SPI3 {
     #[inline(always)]
     fn register_block(&self) -> &RegisterBlock {
         self
     }
 
+    #[inline(always)]
+    fn peripheral(&self) -> crate::system::Peripheral {
+        crate::system::Peripheral::Spi3
+    }
+
+    #[inline(always)]
+    fn spi_num(&self) -> u8 {
+        3
+    }
+
     #[inline(always)]
     fn sclk_signal(&self) -> InputSignal {
         InputSignal::SPI3_CLK
@@ -839,14 +821,4 @@ impl Instance for crate::peripherals::SPI3 {
     fn cs_signal(&self) -> InputSignal {
         InputSignal::SPI3_CS0
     }
-
-    #[inline(always)]
-    fn enable_peripheral(&self) {
-        PeripheralClockControl::enable(crate::system::Peripheral::Spi3)
-    }
-
-    #[inline(always)]
-    fn spi_num(&self) -> u8 {
-        3
-    }
 }

hil-test/Cargo.toml

@@ -99,6 +99,10 @@ harness = false
 name    = "spi_half_duplex_write_psram"
 harness = false
 
+[[test]]
+name    = "spi_slave"
+harness = false
+
 [[test]]
 name    = "systimer"
 harness = false

hil-test/tests/spi_slave.rs

@@ -0,0 +1,176 @@
+//! SPI slave mode test suite.
+
+//% CHIPS: esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
+
+#![no_std]
+#![no_main]
+
+use esp_hal::{
+    dma::{Dma, DmaPriority},
+    dma_buffers,
+    gpio::{interconnect::InputSignal, Io, Level, Output, PeripheralInput},
+    peripherals::SPI2,
+    spi::{slave::Spi, FullDuplexMode, SpiMode},
+};
+use hil_test as _;
+
+cfg_if::cfg_if! {
+    if #[cfg(any(esp32, esp32s2))] {
+        type DmaChannelCreator = esp_hal::dma::Spi2DmaChannelCreator;
+    } else {
+        type DmaChannelCreator = esp_hal::dma::ChannelCreator<0>;
+    }
+}
+
+struct Context {
+    spi: Spi<'static, SPI2, FullDuplexMode>,
+    dma_channel: DmaChannelCreator,
+    bitbang_spi: BitbangSpi,
+}
+
+struct BitbangSpi {
+    sclk: Output<'static>,
+    mosi: Output<'static>,
+    miso: InputSignal,
+    cs: Output<'static>,
+}
+
+impl BitbangSpi {
+    fn new(
+        sclk: Output<'static>,
+        mosi: Output<'static>,
+        miso: InputSignal,
+        cs: Output<'static>,
+    ) -> Self {
+        Self {
+            sclk,
+            mosi,
+            miso,
+            cs,
+        }
+    }
+
+    fn assert_cs(&mut self) {
+        self.cs.set_level(Level::Low);
+    }
+
+    fn deassert_cs(&mut self) {
+        self.cs.set_level(Level::High);
+    }
+
+    // Mode 0, so sampled on the rising edge and set on the falling edge.
+    fn shift_bit(&mut self, bit: bool) -> bool {
+        self.mosi.set_level(Level::from(bit));
+        self.sclk.set_level(Level::Low);
+
+        let miso = self.miso.get_level().into();
+        self.sclk.set_level(Level::High);
+
+        miso
+    }
+
+    // Shift a byte out and in, MSB first.
+    fn shift_byte(&mut self, byte: u8) -> u8 {
+        let mut out = 0;
+        for i in 0..8 {
+            let shift = 7 - i;
+            out |= (self.shift_bit((byte >> shift) & 1 != 0) as u8) << shift;
+        }
+        out
+    }
+
+    fn transfer_buf(&mut self, rx: &mut [u8], tx: &[u8]) {
+        self.assert_cs();
+        for (tx, rx) in tx.iter().zip(rx.iter_mut()) {
+            *rx = self.shift_byte(*tx);
+        }
+        self.deassert_cs();
+    }
+}
+
+#[cfg(test)]
+#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())]
+mod tests {
+    use super::*;
+
+    #[init]
+    fn init() -> Context {
+        let peripherals = esp_hal::init(esp_hal::Config::default());
+
+        let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
+        let (mosi_pin, miso_pin) = hil_test::i2c_pins!(io);
+        let (sclk_pin, sclk_gpio) = hil_test::common_test_pins!(io);
+        let cs_pin = hil_test::unconnected_pin!(io);
+
+        let dma = Dma::new(peripherals.DMA);
+
+        cfg_if::cfg_if! {
+            if #[cfg(any(esp32, esp32s2))] {
+                let dma_channel = dma.spi2channel;
+            } else {
+                let dma_channel = dma.channel0;
+            }
+        }
+
+        let cs = cs_pin.peripheral_input();
+        let mosi = mosi_pin.peripheral_input();
+        let mut miso = miso_pin.peripheral_input();
+
+        let mosi_gpio = Output::new(mosi_pin, Level::Low);
+        let cs_gpio = Output::new(cs_pin, Level::High);
+        let sclk_gpio = Output::new(sclk_gpio, Level::Low);
+
+        let spi = Spi::new(
+            peripherals.SPI2,
+            sclk_pin,
+            mosi,
+            miso_pin,
+            cs,
+            SpiMode::Mode0,
+        );
+
+        miso.enable_input(true, unsafe { esp_hal::Internal::conjure() });
+
+        Context {
+            spi,
+            dma_channel,
+            bitbang_spi: BitbangSpi::new(sclk_gpio, mosi_gpio, miso, cs_gpio),
+        }
+    }
+
+    #[test]
+    #[timeout(10)]
+    fn test_basic(mut ctx: Context) {
+        const DMA_SIZE: usize = 32;
+        let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(DMA_SIZE);
+        let mut spi = ctx.spi.with_dma(
+            ctx.dma_channel.configure(false, DmaPriority::Priority0),
+            tx_descriptors,
+            rx_descriptors,
+        );
+        let slave_send = tx_buffer;
+        let slave_receive = rx_buffer;
+
+        // The transfer stops if the buffers are full, not when the master
+        // deasserts CS. Therefore, these need to be the same size as the DMA buffers.
+        let master_send = &mut [0u8; DMA_SIZE];
+        let master_receive = &mut [0xFFu8; DMA_SIZE];
+
+        for (i, v) in master_send.iter_mut().enumerate() {
+            *v = (i % 255) as u8;
+        }
+        for (i, v) in slave_send.iter_mut().enumerate() {
+            *v = (254 - (i % 255)) as u8;
+        }
+        slave_receive.fill(0xFF);
+
+        let transfer = spi.dma_transfer(slave_receive, &slave_send).unwrap();
+
+        ctx.bitbang_spi.transfer_buf(master_receive, master_send);
+
+        transfer.wait().unwrap();
+
+        assert_eq!(slave_receive, master_send);
+        assert_eq!(master_receive, slave_send);
+    }
+}