I2C error recovery (#2141)

* Recover from I2C errors (the hard way) * Adjust test to cover the problematic case * Replace macro-usage * CHANGELOG.md entry * Appease Clippy * TIL: `Result::inspect_err` exists * Turn public `recover` into private `internal_recover`
esp-rs · Sep 11, 2024 · 24c545d · 24c545d
1 parent 7a733a7
commit 24c545d
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 36 deletions.
diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
@@ -47,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed an issue with LCD_CAM i8080 where it would send double the clocks in 16bit mode (#2085)
 - Fix i2c embedded-hal transaction (#2028)
 - Fix SPI DMA alternating `write` and `read` for ESP32 and ESP32-S2 (#2131)
+- Fix I2C ending up in a state when only re-creating the peripheral makes it useable again (#2141)
 
 ### Removed
 

diff --git a/esp-hal/src/i2c.rs b/esp-hal/src/i2c.rs
@@ -187,6 +187,8 @@ impl From<Ack> for u32 {
 pub struct I2C<'d, T, DM: crate::Mode> {
     peripheral: PeripheralRef<'d, T>,
     phantom: PhantomData<DM>,
+    frequency: HertzU32,
+    timeout: Option<u32>,
 }
 
 impl<T> I2C<'_, T, crate::Blocking>
@@ -195,12 +197,16 @@ where
 {
     /// Reads enough bytes from slave with `address` to fill `buffer`
     pub fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Error> {
-        self.peripheral.master_read(address, buffer)
+        self.peripheral
+            .master_read(address, buffer)
+            .inspect_err(|_| self.internal_recover())
     }
 
     /// Writes bytes to slave with address `address`
     pub fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
-        self.peripheral.master_write(addr, bytes)
+        self.peripheral
+            .master_write(addr, bytes)
+            .inspect_err(|_| self.internal_recover())
     }
 
     /// Writes bytes to slave with address `address` and then reads enough bytes
@@ -211,7 +217,9 @@ where
         bytes: &[u8],
         buffer: &mut [u8],
     ) -> Result<(), Error> {
-        self.peripheral.master_write_read(address, bytes, buffer)
+        self.peripheral
+            .master_write_read(address, bytes, buffer)
+            .inspect_err(|_| self.internal_recover())
     }
 }
 
@@ -222,7 +230,9 @@ where
     type Error = Error;
 
     fn read(&mut self, address: u8, buffer: &mut [u8]) -> Result<(), Self::Error> {
-        self.peripheral.master_read(address, buffer)
+        self.peripheral
+            .master_read(address, buffer)
+            .inspect_err(|_| self.internal_recover())
     }
 }
 
@@ -233,7 +243,9 @@ where
     type Error = Error;
 
     fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> {
-        self.peripheral.master_write(addr, bytes)
+        self.peripheral
+            .master_write(addr, bytes)
+            .inspect_err(|_| self.internal_recover())
     }
 }
 
@@ -249,7 +261,9 @@ where
         bytes: &[u8],
         buffer: &mut [u8],
     ) -> Result<(), Self::Error> {
-        self.peripheral.master_write_read(address, bytes, buffer)
+        self.peripheral
+            .master_write_read(address, bytes, buffer)
+            .inspect_err(|_| self.internal_recover())
     }
 }
 
@@ -287,28 +301,32 @@ where
                     // execute a write operation:
                     // - issue START/RSTART if op is different from previous
                     // - issue STOP if op is the last one
-                    self.peripheral.write_operation(
-                        address,
-                        bytes,
-                        last_op != Op::Write,
-                        next_op == Op::None,
-                        cmd_iterator,
-                    )?;
+                    self.peripheral
+                        .write_operation(
+                            address,
+                            bytes,
+                            last_op != Op::Write,
+                            next_op == Op::None,
+                            cmd_iterator,
+                        )
+                        .inspect_err(|_| self.internal_recover())?;
                     last_op = Op::Write;
                 }
                 Operation::Read(buffer) => {
                     // execute a read operation:
                     // - issue START/RSTART if op is different from previous
                     // - issue STOP if op is the last one
                     // - will_continue is true if there is another read operation next
-                    self.peripheral.read_operation(
-                        address,
-                        buffer,
-                        last_op != Op::Read,
-                        next_op == Op::None,
-                        next_op == Op::Read,
-                        cmd_iterator,
-                    )?;
+                    self.peripheral
+                        .read_operation(
+                            address,
+                            buffer,
+                            last_op != Op::Read,
+                            next_op == Op::None,
+                            next_op == Op::Read,
+                            cmd_iterator,
+                        )
+                        .inspect_err(|_| self.internal_recover())?;
                     last_op = Op::Read;
                 }
             }
@@ -347,9 +365,11 @@ where
             _ => unreachable!(), // will never happen
         });
 
-        let mut i2c = I2C {
+        let i2c = I2C {
             peripheral: i2c,
             phantom: PhantomData,
+            frequency,
+            timeout,
         };
 
         // avoid SCL/SDA going low during configuration
@@ -392,6 +412,24 @@ where
             crate::interrupt::enable(T::interrupt(), handler.priority()).unwrap();
         }
     }
+
+    fn internal_recover(&self) {
+        PeripheralClockControl::reset(match self.peripheral.i2c_number() {
+            0 => crate::system::Peripheral::I2cExt0,
+            #[cfg(i2c1)]
+            1 => crate::system::Peripheral::I2cExt1,
+            _ => unreachable!(), // will never happen
+        });
+
+        PeripheralClockControl::enable(match self.peripheral.i2c_number() {
+            0 => crate::system::Peripheral::I2cExt0,
+            #[cfg(i2c1)]
+            1 => crate::system::Peripheral::I2cExt1,
+            _ => unreachable!(), // will never happen
+        });
+
+        self.peripheral.setup(self.frequency, self.timeout);
+    }
 }
 
 impl<'d, T> I2C<'d, T, crate::Blocking>
@@ -1089,7 +1127,7 @@ pub trait Instance: crate::private::Sealed {
 
     /// Configures the I2C peripheral with the specified frequency, clocks, and
     /// optional timeout.
-    fn setup(&mut self, frequency: HertzU32, timeout: Option<u32>) {
+    fn setup(&self, frequency: HertzU32, timeout: Option<u32>) {
         self.register_block().ctr().modify(|_, w| unsafe {
             // Clear register
             w.bits(0)
@@ -1170,7 +1208,7 @@ pub trait Instance: crate::private::Sealed {
 
     /// Sets the filter with a supplied threshold in clock cycles for which a
     /// pulse must be present to pass the filter
-    fn set_filter(&mut self, sda_threshold: Option<u8>, scl_threshold: Option<u8>) {
+    fn set_filter(&self, sda_threshold: Option<u8>, scl_threshold: Option<u8>) {
         cfg_if::cfg_if! {
             if #[cfg(any(esp32, esp32s2))] {
                 let sda_register = &self.register_block().sda_filter_cfg();
@@ -1202,7 +1240,7 @@ pub trait Instance: crate::private::Sealed {
     /// Sets the frequency of the I2C interface by calculating and applying the
     /// associated timings - corresponds to i2c_ll_cal_bus_clk and
     /// i2c_ll_set_bus_timing in ESP-IDF
-    fn set_frequency(&mut self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
+    fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
         let source_clk = source_clk.raw();
         let bus_freq = bus_freq.raw();
 
@@ -1280,7 +1318,7 @@ pub trait Instance: crate::private::Sealed {
     /// Sets the frequency of the I2C interface by calculating and applying the
     /// associated timings - corresponds to i2c_ll_cal_bus_clk and
     /// i2c_ll_set_bus_timing in ESP-IDF
-    fn set_frequency(&mut self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
+    fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
         let source_clk = source_clk.raw();
         let bus_freq = bus_freq.raw();
 
@@ -1338,7 +1376,7 @@ pub trait Instance: crate::private::Sealed {
     /// Sets the frequency of the I2C interface by calculating and applying the
     /// associated timings - corresponds to i2c_ll_cal_bus_clk and
     /// i2c_ll_set_bus_timing in ESP-IDF
-    fn set_frequency(&mut self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
+    fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option<u32>) {
         let source_clk = source_clk.raw();
         let bus_freq = bus_freq.raw();
 
@@ -1411,7 +1449,7 @@ pub trait Instance: crate::private::Sealed {
     #[allow(clippy::too_many_arguments, unused)]
     /// Configures the clock and timing parameters for the I2C peripheral.
     fn configure_clock(
-        &mut self,
+        &self,
         sclk_div: u32,
         scl_low_period: u32,
         scl_high_period: u32,
@@ -2078,7 +2116,7 @@ pub trait Instance: crate::private::Sealed {
 
     /// Send data bytes from the `bytes` array to a target slave with the
     /// address `addr`
-    fn master_write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
+    fn master_write(&self, addr: u8, bytes: &[u8]) -> Result<(), Error> {
         // Clear all I2C interrupts
         self.clear_all_interrupts();
         self.write_operation(
@@ -2094,7 +2132,7 @@ pub trait Instance: crate::private::Sealed {
     /// Read bytes from a target slave with the address `addr`
     /// The number of read bytes is deterimed by the size of the `buffer`
     /// argument
-    fn master_read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Error> {
+    fn master_read(&self, addr: u8, buffer: &mut [u8]) -> Result<(), Error> {
         // Clear all I2C interrupts
         self.clear_all_interrupts();
         self.read_operation(
@@ -2110,12 +2148,7 @@ pub trait Instance: crate::private::Sealed {
 
     /// Write bytes from the `bytes` array first and then read n bytes into
     /// the `buffer` array with n being the size of the array.
-    fn master_write_read(
-        &mut self,
-        addr: u8,
-        bytes: &[u8],
-        buffer: &mut [u8],
-    ) -> Result<(), Error> {
+    fn master_write_read(&self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Error> {
         // it would be possible to combine the write and read
         // in one transaction but filling the tx fifo with
         // the current code is somewhat slow even in release mode

diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs
@@ -37,6 +37,11 @@ mod tests {
     fn test_read_cali(mut ctx: Context) {
         let mut read_data = [0u8; 22];
 
+        // have a failing read which might could leave the peripheral in an undesirable
+        // state
+        ctx.i2c.write_read(0x55, &[0xaa], &mut read_data).ok();
+
+        // do the real read which should succeed
         ctx.i2c.write_read(0x77, &[0xaa], &mut read_data).ok();
 
         assert_ne!(read_data, [0u8; 22])