Check for ST_TOUT / MAIN_ST_TOUT (#3333)

* Check for ST_TOUT / MAIN_ST_TOUT

* Fix & CHANGELOG

* cfg gate

* Field renaming in PACs

* Introduce FsmTimeout type

* `new_const`

* Review comments

* Async I2C tests

* Remove FSM timeout for S2

* Add test

* Don't run `async_test_timeout_when_scl_kept_low` on S2 - we know it fails

* Fix

---------

Co-authored-by: Scott Mabin <scott@mabez.dev>

esp-hal/CHANGELOG.md

@@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Moved numbered GPIO pin types from `esp_hal::gpio::GpioPin<N>` to `esp_hal::peripherals::GPION<'_>` (#3349)
 - Moved DMA channel types from `esp_hal::dma::DmaChannelN`/`esp_hal::dma::XYDmaChannel` to `esp_hal::peripherals::DMA_XY` (#3372)
 - `ParlIoFullDuplex`, `ParlIoTxOnly` and `ParlIoRxOnly` have been merged into `ParlIo` (#3366)
+- I2C checks ST_TOUT / MAIN_ST_TOUT where available (#3333)
 - All `Camera` pins are now configured using `with_*()` methods (#3237)
 - The `ESP_HAL_CONFIG_PLACE_SPI_DRIVER_IN_RAM` configuration option has been renamed to `ESP_HAL_CONFIG_PLACE_SPI_MASTER_DRIVER_IN_RAM`. (#3402)
 - Made the `ParlIo` traits for `TxPins`, `RxPins`, `ConfigurePins` public (#3398)

esp-hal/src/i2c/master/mod.rs

@@ -165,6 +165,59 @@ impl BusTimeout {
     }
 }
 
+/// When the FSM remains unchanged for more than the 2^ the given amount of bus
+/// clock cycles a timeout will be triggered.
+///
+/// The default value is 0x10
+#[instability::unstable]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+#[cfg(not(any(esp32, esp32s2)))]
+pub struct FsmTimeout {
+    value: u8,
+}
+
+#[cfg(not(any(esp32, esp32s2)))]
+impl FsmTimeout {
+    const FSM_TIMEOUT_MAX: u8 = 23;
+
+    const FSM_TIMEOUT_DEFAULT: u8 = 0x10;
+
+    /// Creates a new timeout.
+    ///
+    /// The meaning of the value and the allowed range of values is different
+    /// for different chips.
+    pub const fn new_const<const VALUE: u8>() -> Self {
+        const {
+            core::assert!(VALUE <= Self::FSM_TIMEOUT_MAX, "Invalid timeout value");
+        }
+        Self { value: VALUE }
+    }
+
+    /// Creates a new timeout.
+    ///
+    /// The meaning of the value and the allowed range of values is different
+    /// for different chips.
+    pub fn new(value: u8) -> Result<Self, ConfigError> {
+        if value > Self::FSM_TIMEOUT_MAX {
+            return Err(ConfigError::TimeoutInvalid);
+        }
+
+        Ok(Self { value })
+    }
+
+    fn value(&self) -> u8 {
+        self.value
+    }
+}
+
+#[cfg(not(any(esp32, esp32s2)))]
+impl Default for FsmTimeout {
+    fn default() -> Self {
+        Self::new_const::<{ Self::FSM_TIMEOUT_DEFAULT }>()
+    }
+}
+
 /// I2C-specific transmission errors
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
@@ -425,6 +478,16 @@ pub struct Config {
 
     /// I2C SCL timeout period.
     timeout: BusTimeout,
+
+    /// Sets the threshold value for the unchanged period of the SCL_FSM.
+    #[cfg(not(any(esp32, esp32s2)))]
+    #[builder_lite(unstable)]
+    scl_st_timeout: FsmTimeout,
+
+    /// Sets the threshold for the unchanged duration of the SCL_MAIN_FSM.
+    #[cfg(not(any(esp32, esp32s2)))]
+    #[builder_lite(unstable)]
+    scl_main_st_timeout: FsmTimeout,
 }
 
 impl Default for Config {
@@ -432,6 +495,10 @@ impl Default for Config {
         Config {
             frequency: Rate::from_khz(100),
             timeout: BusTimeout::BusCycles(10),
+            #[cfg(not(any(esp32, esp32s2)))]
+            scl_st_timeout: Default::default(),
+            #[cfg(not(any(esp32, esp32s2)))]
+            scl_main_st_timeout: Default::default(),
         }
     }
 }
@@ -634,6 +701,10 @@ impl<'a> I2cFuture<'a> {
             w.arbitration_lost().set_bit();
             w.time_out().set_bit();
             w.nack().set_bit();
+            #[cfg(not(any(esp32, esp32s2)))]
+            w.scl_main_st_to().set_bit();
+            #[cfg(not(any(esp32, esp32s2)))]
+            w.scl_st_to().set_bit();
 
             w
         });
@@ -669,6 +740,16 @@ impl<'a> I2cFuture<'a> {
             )));
         }
 
+        #[cfg(not(any(esp32, esp32s2)))]
+        if r.scl_st_to().bit_is_set() {
+            return Err(Error::Timeout);
+        }
+
+        #[cfg(not(any(esp32, esp32s2)))]
+        if r.scl_main_st_to().bit_is_set() {
+            return Err(Error::Timeout);
+        }
+
         #[cfg(not(esp32))]
         if r.trans_complete().bit_is_set() && self.info.regs().sr().read().resp_rec().bit_is_clear()
         {
@@ -1147,6 +1228,10 @@ fn async_handler(info: &Info, state: &State) {
         w.trans_complete().clear_bit();
         w.arbitration_lost().clear_bit();
         w.time_out().clear_bit();
+        #[cfg(not(esp32))]
+        w.scl_main_st_to().clear_bit();
+        #[cfg(not(esp32))]
+        w.scl_st_to().clear_bit();
 
         #[cfg(not(any(esp32, esp32s2)))]
         w.txfifo_wm().clear_bit();
@@ -1428,6 +1513,16 @@ impl Driver<'_> {
         // Configure frequency
         self.set_frequency(config, config.timeout)?;
 
+        // Configure additional timeouts
+        #[cfg(not(any(esp32, esp32s2)))]
+        self.regs()
+            .scl_st_time_out()
+            .write(|w| unsafe { w.scl_st_to().bits(config.scl_st_timeout.value()) });
+        #[cfg(not(any(esp32, esp32s2)))]
+        self.regs()
+            .scl_main_st_time_out()
+            .write(|w| unsafe { w.scl_main_st_to().bits(config.scl_main_st_timeout.value()) });
+
         self.update_config();
 
         // Reset entire peripheral (also resets fifo)

hil-test/tests/i2c.rs

@@ -1,7 +1,7 @@
 //! I2C test
 
 //% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
-//% FEATURES: unstable
+//% FEATURES: unstable embassy
 
 #![no_std]
 #![no_main]
@@ -29,7 +29,7 @@ const DUT_ADDRESS: u8 = 0x77;
 const NON_EXISTENT_ADDRESS: u8 = 0x6b;
 
 #[cfg(test)]
-#[embedded_test::tests(default_timeout = 3)]
+#[embedded_test::tests(default_timeout = 3, executor = hil_test::Executor::new())]
 mod tests {
     use super::*;
 
@@ -122,4 +122,88 @@ mod tests {
 
         assert_ne!(read_data, [0u8; 22])
     }
+
+    #[test]
+    async fn async_empty_write_returns_ack_error_for_unknown_address(ctx: Context) {
+        let mut i2c = ctx.i2c.into_async();
+
+        // on some chips we can determine the ack-check-failed reason but not on all
+        // chips
+        cfg_if::cfg_if! {
+            if #[cfg(any(esp32,esp32s2,esp32c2,esp32c3))] {
+                assert_eq!(
+                    i2c.write_async(NON_EXISTENT_ADDRESS, &[]).await,
+                    Err(Error::AcknowledgeCheckFailed(
+                        AcknowledgeCheckFailedReason::Unknown
+                    ))
+                );
+            } else {
+                assert_eq!(
+                    i2c.write_async(NON_EXISTENT_ADDRESS, &[]).await,
+                    Err(Error::AcknowledgeCheckFailed(
+                        AcknowledgeCheckFailedReason::Address
+                    ))
+                );
+            }
+        }
+
+        assert_eq!(i2c.write_async(DUT_ADDRESS, &[]).await, Ok(()));
+    }
+
+    #[test]
+    async fn async_test_read_cali(ctx: Context) {
+        let mut i2c = ctx.i2c.into_async();
+        let mut read_data = [0u8; 22];
+
+        // have a failing read which might could leave the peripheral in an undesirable
+        // state
+        i2c.write_read_async(NON_EXISTENT_ADDRESS, &[0xaa], &mut read_data)
+            .await
+            .ok();
+
+        // do the real read which should succeed
+        i2c.write_read_async(DUT_ADDRESS, &[0xaa], &mut read_data)
+            .await
+            .ok();
+
+        assert_ne!(read_data, [0u8; 22])
+    }
+
+    #[test]
+    async fn async_test_read_cali_with_transactions(ctx: Context) {
+        let mut i2c = ctx.i2c.into_async();
+        let mut read_data = [0u8; 22];
+
+        // do the real read which should succeed
+        i2c.transaction_async(
+            DUT_ADDRESS,
+            &mut [Operation::Write(&[0xaa]), Operation::Read(&mut read_data)],
+        )
+        .await
+        .ok();
+
+        assert_ne!(read_data, [0u8; 22])
+    }
+
+    // This is still an issue on ESP32-S2
+    #[cfg(not(esp32s2))]
+    #[test]
+    async fn async_test_timeout_when_scl_kept_low(_ctx: Context) {
+        let mut i2c = I2c::new(
+            unsafe { esp_hal::peripherals::I2C0::steal() },
+            Config::default(),
+        )
+        .unwrap()
+        .with_sda(unsafe { esp_hal::peripherals::GPIO4::steal() })
+        .with_scl(unsafe { esp_hal::peripherals::GPIO5::steal() })
+        .into_async();
+
+        esp_hal::gpio::InputSignal::I2CEXT0_SCL.connect_to(&esp_hal::gpio::Level::Low);
+
+        let mut read_data = [0u8; 22];
+        // will run into an error but it should return at least
+        i2c.write_read_async(DUT_ADDRESS, &[0xaa], &mut read_data)
+            .await
+            .ok();
+    }
 }