Refactor SPI MISO setup (#2557)

* Refactor SPI MISO setup * update HIL * docs * missed a spot * fix changelog --------- Co-authored-by: Dominic Fischer <[email protected]> Co-authored-by: Scott Mabin <[email protected]>
esp-rs · Jan 7, 2025 · b3401bf · b3401bf
1 parent 2a4e58a
commit b3401bf
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 6 deletions.
diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
@@ -87,6 +87,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Updated `esp-pacs` with support for Wi-Fi on the ESP32 and made the peripheral non virtual
 - `SpiBitOrder`, `SpiDataMode`, `SpiMode` were renamed to `BitOder`, `DataMode` and `Mode` (#2828)
 - `crate::Mode` was renamed to `crate::DriverMode` (#2828)
+- `Spi::with_miso` has been overloaded into `Spi::with_miso` and `Spi::with_sio1` (#2557)
 - Renamed some I2C error variants (#2844)
 - I2C: Replaced potential panics with errors. (#2831)
 - UART: Make `AtCmdConfig` and `ConfigError` non-exhaustive (#2851)

diff --git a/esp-hal/MIGRATING-0.22.md b/esp-hal/MIGRATING-0.22.md
@@ -386,3 +386,11 @@ e.g.)
 The previous blocking implementation of `read_bytes` has been removed, and the non-blocking `drain_fifo` has instead been renamed to `read_bytes` in its place.
 
 Any code which was previously using `read_bytes` to fill a buffer in a blocking manner will now need to implement the necessary logic to block until the buffer is filled in their application instead.
+
+## Spi `with_miso` has been split
+
+Previously, `with_miso` set up the provided pin as an input and output, which was necessary for half duplex.
+Full duplex does not require this, and it also creates an artificial restriction.
+
+If you were using half duplex SPI with `with_miso`,
+you should now use `with_sio1` instead to get the previous behavior.
diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs
@@ -72,7 +72,12 @@ use crate::{
     asynch::AtomicWaker,
     clock::Clocks,
     dma::{DmaChannelFor, DmaEligible, DmaRxBuffer, DmaTxBuffer, Rx, Tx},
-    gpio::{interconnect::PeripheralOutput, InputSignal, NoPin, OutputSignal},
+    gpio::{
+        interconnect::{PeripheralInput, PeripheralOutput},
+        InputSignal,
+        NoPin,
+        OutputSignal,
+    },
     interrupt::{InterruptConfigurable, InterruptHandler},
     peripheral::{Peripheral, PeripheralRef},
     peripherals::spi2::RegisterBlock,
@@ -676,9 +681,24 @@ where
 
     /// Assign the MISO (Master In Slave Out) pin for the SPI instance.
     ///
+    /// Enables input functionality for the pin, and connects it to the MISO
+    /// signal.
+    pub fn with_miso<MISO: PeripheralInput>(self, miso: impl Peripheral<P = MISO> + 'd) -> Self {
+        crate::into_mapped_ref!(miso);
+        miso.enable_input(true, private::Internal);
+
+        self.driver().info.miso.connect_to(&mut miso);
+
+        self
+    }
+
+    /// Assign the SIO1/MISO pin for the SPI instance.
+    ///
     /// Enables both input and output functionality for the pin, and connects it
     /// to the MISO signal and SIO1 input signal.
-    pub fn with_miso<MISO: PeripheralOutput>(self, miso: impl Peripheral<P = MISO> + 'd) -> Self {
+    ///
+    /// Note: You do not need to call [Self::with_miso] when this is used.
+    pub fn with_sio1<SIO1: PeripheralOutput>(self, miso: impl Peripheral<P = SIO1> + 'd) -> Self {
         crate::into_mapped_ref!(miso);
         miso.enable_input(true, private::Internal);
         miso.enable_output(true, private::Internal);

diff --git a/hil-test/tests/qspi.rs b/hil-test/tests/qspi.rs
@@ -280,7 +280,7 @@ mod tests {
         let [pin, pin_mirror, _] = ctx.gpios;
         let pin_mirror = Output::new(pin_mirror, Level::High);
 
-        let spi = ctx.spi.with_miso(pin).with_dma(ctx.dma_channel);
+        let spi = ctx.spi.with_sio1(pin).with_dma(ctx.dma_channel);
 
         super::execute_write_read(spi, pin_mirror, 0b0010_0010);
     }
@@ -355,7 +355,7 @@ mod tests {
         let spi = ctx
             .spi
             .with_mosi(mosi)
-            .with_miso(gpio)
+            .with_sio1(gpio)
             .with_dma(ctx.dma_channel);
 
         super::execute_write(unit0, unit1, spi, 0b0000_0010, true);

diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs
@@ -68,14 +68,16 @@ mod tests {
             }
         }
 
+        let (miso, mosi) = mosi.split();
         #[cfg(pcnt)]
-        let (mosi_loopback_pcnt, mosi) = mosi.split();
+        let mosi_loopback_pcnt = miso.clone();
+
         // Need to set miso first so that mosi can overwrite the
         // output connection (because we are using the same pin to loop back)
         let spi = Spi::new(peripherals.SPI2, Config::default().with_frequency(10.MHz()))
             .unwrap()
             .with_sck(sclk)
-            .with_miso(unsafe { mosi.clone_unchecked() })
+            .with_miso(miso)
             .with_mosi(mosi);
 
         let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);