From 4bc9ef2f5b8baa773244c591359dfad490675fb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Quentin?= Date: Tue, 22 Apr 2025 13:11:11 +0200 Subject: [PATCH] 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 --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/i2c/master/mod.rs | 95 +++++++++++++++++++++++++++++++++++ hil-test/tests/i2c.rs | 88 +++++++++++++++++++++++++++++++- 3 files changed, 182 insertions(+), 2 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index a1097b249..5aea8726f 100644 --- a/esp-hal/CHANGELOG.md +++ b/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` 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) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index acf36e6eb..cfa9eb415 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/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() -> 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 { + 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) diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs index 349b04c12..22abf6d2e 100644 --- a/hil-test/tests/i2c.rs +++ b/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(); + } }