Fix i2c freeze (esp-rs#1847)

* Check errors while waiting for TXFIFO_WM * Limit iterations in `wait_for_completion` * Detect errors in async-I2C * CHANGELOG.md * async `wait_for_completion` ESP32 special treatment * Simplify ESP32 workaround * Clippy
Dominaezzz · Jul 24, 2024 · 7f251b4 · 7f251b4
1 parent 4ef91c5
commit 7f251b4
Show file tree

Hide file tree

Showing 2 changed files with 168 additions and 18 deletions.
diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Peripheral driver constructors don't take `InterruptHandler`s anymore. Use `set_interrupt_handler` to explicitly set the interrupt handler now. (#1819)
 
 ### Fixed
+- Improve error detection in the I2C driver (#1847)
 
 - Fix I2S async-tx (#1833)
 - Fix PARL_IO async-rx (#1851)

diff --git a/esp-hal/src/i2c.rs b/esp-hal/src/i2c.rs
@@ -91,6 +91,9 @@ cfg_if::cfg_if! {
     }
 }
 
+// on ESP32 there is a chance to get trapped in `wait_for_completion` forever
+const MAX_ITERATIONS: u32 = 1_000_000;
+
 /// I2C-specific transmission errors
 #[derive(Debug, Clone, Copy, PartialEq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
@@ -455,6 +458,13 @@ where
     ) -> Self {
         crate::into_ref!(i2c, sda, scl);
 
+        PeripheralClockControl::reset(match i2c.i2c_number() {
+            0 => crate::system::Peripheral::I2cExt0,
+            #[cfg(i2c1)]
+            1 => crate::system::Peripheral::I2cExt1,
+            _ => unreachable!(), // will never happen
+        });
+
         PeripheralClockControl::enable(match i2c.i2c_number() {
             0 => crate::system::Peripheral::I2cExt0,
             #[cfg(i2c1)]
@@ -599,13 +609,13 @@ where
 
 #[cfg(feature = "async")]
 mod asynch {
+    #[cfg(not(esp32))]
     use core::{
         pin::Pin,
         task::{Context, Poll},
     };
 
     use cfg_if::cfg_if;
-    use embassy_futures::select::select;
     use embassy_sync::waitqueue::AtomicWaker;
     use embedded_hal::i2c::Operation;
     use procmacros::handler;
@@ -623,13 +633,15 @@ mod asynch {
     const INIT: AtomicWaker = AtomicWaker::new();
     static WAKERS: [AtomicWaker; NUM_I2C] = [INIT; NUM_I2C];
 
+    #[cfg_attr(esp32, allow(dead_code))]
     pub(crate) enum Event {
         EndDetect,
         TxComplete,
         #[cfg(not(any(esp32, esp32s2)))]
         TxFifoWatermark,
     }
 
+    #[cfg(not(esp32))]
     pub(crate) struct I2cFuture<'a, T>
     where
         T: Instance,
@@ -638,20 +650,38 @@ mod asynch {
         instance: &'a T,
     }
 
+    #[cfg(not(esp32))]
     impl<'a, T> I2cFuture<'a, T>
     where
         T: Instance,
     {
         pub fn new(event: Event, instance: &'a T) -> Self {
-            instance
-                .register_block()
-                .int_ena()
-                .modify(|_, w| match event {
+            instance.register_block().int_ena().modify(|_, w| {
+                let w = match event {
                     Event::EndDetect => w.end_detect().set_bit(),
                     Event::TxComplete => w.trans_complete().set_bit(),
                     #[cfg(not(any(esp32, esp32s2)))]
                     Event::TxFifoWatermark => w.txfifo_wm().set_bit(),
-                });
+                };
+
+                #[cfg(not(esp32))]
+                w.arbitration_lost()
+                    .set_bit()
+                    .time_out()
+                    .set_bit()
+                    .nack()
+                    .set_bit();
+
+                #[cfg(esp32)]
+                w.arbitration_lost()
+                    .set_bit()
+                    .time_out()
+                    .set_bit()
+                    .ack_err()
+                    .set_bit();
+
+                w
+            });
 
             Self { event, instance }
         }
@@ -666,19 +696,63 @@ mod asynch {
                 Event::TxFifoWatermark => r.txfifo_wm().bit_is_clear(),
             }
         }
+
+        fn check_error(&self) -> Result<(), Error> {
+            let r = self.instance.register_block().int_raw().read();
+
+            if r.arbitration_lost().bit_is_set() {
+                return Err(Error::ArbitrationLost);
+            }
+
+            if r.time_out().bit_is_set() {
+                return Err(Error::TimeOut);
+            }
+
+            #[cfg(not(esp32))]
+            if r.nack().bit_is_set() {
+                return Err(Error::AckCheckFailed);
+            }
+
+            #[cfg(esp32)]
+            if r.ack_err().bit_is_set() {
+                return Err(Error::AckCheckFailed);
+            }
+
+            #[cfg(not(esp32))]
+            if r.trans_complete().bit_is_set()
+                && self
+                    .instance
+                    .register_block()
+                    .sr()
+                    .read()
+                    .resp_rec()
+                    .bit_is_clear()
+            {
+                return Err(Error::AckCheckFailed);
+            }
+
+            Ok(())
+        }
     }
 
+    #[cfg(not(esp32))]
     impl<'a, T> core::future::Future for I2cFuture<'a, T>
     where
         T: Instance,
     {
-        type Output = ();
+        type Output = Result<(), Error>;
 
         fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll<Self::Output> {
             WAKERS[self.instance.i2c_number()].register(ctx.waker());
 
+            let error = self.check_error();
+
+            if error.is_err() {
+                return Poll::Ready(error);
+            }
+
             if self.event_bit_is_clear() {
-                Poll::Ready(())
+                Poll::Ready(Ok(()))
             } else {
                 Poll::Pending
             }
@@ -737,14 +811,14 @@ mod asynch {
             loop {
                 self.peripheral.check_errors()?;
 
-                I2cFuture::new(Event::TxFifoWatermark, self.inner()).await;
+                I2cFuture::new(Event::TxFifoWatermark, self.inner()).await?;
 
                 self.peripheral
                     .register_block()
                     .int_clr()
                     .write(|w| w.txfifo_wm().clear_bit_by_one());
 
-                I2cFuture::new(Event::TxFifoWatermark, self.inner()).await;
+                I2cFuture::new(Event::TxFifoWatermark, self.inner()).await?;
 
                 if index >= bytes.len() {
                     break Ok(());
@@ -755,23 +829,60 @@ mod asynch {
             }
         }
 
+        #[cfg(not(esp32))]
         async fn wait_for_completion(&self, end_only: bool) -> Result<(), Error> {
             self.peripheral.check_errors()?;
 
             if end_only {
-                I2cFuture::new(Event::EndDetect, self.inner()).await;
+                I2cFuture::new(Event::EndDetect, self.inner()).await?;
             } else {
-                select(
+                let res = embassy_futures::select::select(
                     I2cFuture::new(Event::TxComplete, self.inner()),
                     I2cFuture::new(Event::EndDetect, self.inner()),
                 )
                 .await;
+
+                match res {
+                    embassy_futures::select::Either::First(res) => res?,
+                    embassy_futures::select::Either::Second(res) => res?,
+                }
             }
             self.peripheral.check_all_commands_done()?;
 
             Ok(())
         }
 
+        #[cfg(esp32)]
+        async fn wait_for_completion(&self, end_only: bool) -> Result<(), Error> {
+            // for ESP32 we need a timeout here but wasting a timer seems unnecessary
+            // given the short time we spend here
+
+            let mut tout = MAX_ITERATIONS / 10; // adjust the timeout because we are yielding in the loop
+            loop {
+                let interrupts = self.inner().register_block().int_raw().read();
+
+                self.inner().check_errors()?;
+
+                // Handle completion cases
+                // A full transmission was completed (either a STOP condition or END was
+                // processed)
+                if (!end_only && interrupts.trans_complete().bit_is_set())
+                    || interrupts.end_detect().bit_is_set()
+                {
+                    break;
+                }
+
+                tout -= 1;
+                if tout == 0 {
+                    return Err(Error::TimeOut);
+                }
+
+                embassy_futures::yield_now().await;
+            }
+            self.peripheral.check_all_commands_done()?;
+            Ok(())
+        }
+
         async fn write_operation<'a, I>(
             &self,
             address: u8,
@@ -976,25 +1087,53 @@ mod asynch {
     #[handler]
     pub(super) fn i2c0_handler() {
         let regs = unsafe { &*crate::peripherals::I2C0::PTR };
-        regs.int_ena()
-            .modify(|_, w| w.end_detect().clear_bit().trans_complete().clear_bit());
+        regs.int_ena().modify(|_, w| {
+            w.end_detect()
+                .clear_bit()
+                .trans_complete()
+                .clear_bit()
+                .arbitration_lost()
+                .clear_bit()
+                .time_out()
+                .clear_bit()
+        });
 
         #[cfg(not(any(esp32, esp32s2)))]
         regs.int_ena().modify(|_, w| w.txfifo_wm().clear_bit());
 
+        #[cfg(not(esp32))]
+        regs.int_ena().modify(|_, w| w.nack().clear_bit());
+
+        #[cfg(esp32)]
+        regs.int_ena().modify(|_, w| w.ack_err().clear_bit());
+
         WAKERS[0].wake();
     }
 
     #[cfg(i2c1)]
     #[handler]
     pub(super) fn i2c1_handler() {
         let regs = unsafe { &*crate::peripherals::I2C1::PTR };
-        regs.int_ena()
-            .modify(|_, w| w.end_detect().clear_bit().trans_complete().clear_bit());
+        regs.int_ena().modify(|_, w| {
+            w.end_detect()
+                .clear_bit()
+                .trans_complete()
+                .clear_bit()
+                .arbitration_lost()
+                .clear_bit()
+                .time_out()
+                .clear_bit()
+        });
 
         #[cfg(not(any(esp32, esp32s2)))]
         regs.int_ena().modify(|_, w| w.txfifo_wm().clear_bit());
 
+        #[cfg(not(esp32))]
+        regs.int_ena().modify(|_, w| w.nack().clear_bit());
+
+        #[cfg(esp32)]
+        regs.int_ena().modify(|_, w| w.ack_err().clear_bit());
+
         WAKERS[1].wake();
     }
 }
@@ -1571,6 +1710,7 @@ pub trait Instance: crate::private::Sealed {
     }
 
     fn wait_for_completion(&self, end_only: bool) -> Result<(), Error> {
+        let mut tout = MAX_ITERATIONS;
         loop {
             let interrupts = self.register_block().int_raw().read();
 
@@ -1584,6 +1724,11 @@ pub trait Instance: crate::private::Sealed {
             {
                 break;
             }
+
+            tout -= 1;
+            if tout == 0 {
+                return Err(Error::TimeOut);
+            }
         }
         self.check_all_commands_done()?;
         Ok(())
@@ -1699,7 +1844,9 @@ pub trait Instance: crate::private::Sealed {
                 .read()
                 .txfifo_wm()
                 .bit_is_set()
-            {}
+            {
+                self.check_errors()?;
+            }
 
             self.register_block()
                 .int_clr()
@@ -1711,7 +1858,9 @@ pub trait Instance: crate::private::Sealed {
                 .read()
                 .txfifo_wm()
                 .bit_is_set()
-            {}
+            {
+                self.check_errors()?;
+            }
 
             if index >= bytes.len() {
                 break Ok(());