I2C: clean up, validate address and reset state machine/fifos before in the same place (#3466)

* Simplify I2C future

* Wait for completion with a real timeout

* Reset before a new transmission

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

@@ -51,7 +51,7 @@ use crate::{
     peripherals::Interrupt,
     private,
     system::{PeripheralClockControl, PeripheralGuard},
-    time::Rate,
+    time::{Duration, Instant, Rate},
 };
 
 const I2C_LL_INTR_MASK: u32 = if cfg!(esp32s2) { 0x1ffff } else { 0x3ffff };
@@ -59,9 +59,7 @@ const I2C_FIFO_SIZE: usize = if cfg!(esp32c2) { 16 } else { 32 };
 
 // Chunk writes/reads by this size
 const I2C_CHUNK_SIZE: usize = I2C_FIFO_SIZE - 1;
-
+const TIMEOUT: Duration = Duration::from_millis(10);
-// on ESP32 there is a chance to get trapped in `wait_for_completion` forever
-const MAX_ITERATIONS: u32 = 1_000_000;
 
 /// Representation of I2C address.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -677,21 +675,23 @@ pub enum Event {
 #[cfg(not(esp32))]
 #[must_use = "futures do nothing unless you `.await` or poll them"]
 struct I2cFuture<'a> {
-    event: Event,
+    events: EnumSet<Event>,
     info: &'a Info,
     state: &'a State,
 }
 
 #[cfg(not(esp32))]
 impl<'a> I2cFuture<'a> {
-    pub fn new(event: Event, info: &'a Info, state: &'a State) -> Self {
+    pub fn new(events: EnumSet<Event>, info: &'a Info, state: &'a State) -> Self {
         info.regs().int_ena().modify(|_, w| {
-            let w = match event {
+            for event in events {
-                Event::EndDetect => w.end_detect().set_bit(),
+                match event {
-                Event::TxComplete => w.trans_complete().set_bit(),
+                    Event::EndDetect => w.end_detect().set_bit(),
-                #[cfg(not(any(esp32, esp32s2)))]
+                    Event::TxComplete => w.trans_complete().set_bit(),
-                Event::TxFifoWatermark => w.txfifo_wm().set_bit(),
+                    #[cfg(not(any(esp32, esp32s2)))]
-            };
+                    Event::TxFifoWatermark => w.txfifo_wm().set_bit(),
+                };
+            }
 
             w.arbitration_lost().set_bit();
             w.time_out().set_bit();
@@ -705,18 +705,15 @@ impl<'a> I2cFuture<'a> {
             w
         });
 
-        Self { event, state, info }
+        Self {
+            events,
+            state,
+            info,
+        }
     }
 
-    fn event_bit_is_clear(&self) -> bool {
+    fn is_done(&self) -> bool {
-        let r = self.info.regs().int_ena().read();
+        !self.info.interrupts().is_disjoint(self.events)
-
-        match self.event {
-            Event::EndDetect => r.end_detect().bit_is_clear(),
-            Event::TxComplete => r.trans_complete().bit_is_clear(),
-            #[cfg(not(any(esp32, esp32s2)))]
-            Event::TxFifoWatermark => r.txfifo_wm().bit_is_clear(),
-        }
     }
 
     fn check_error(&self) -> Result<(), Error> {
@@ -768,10 +765,8 @@ impl core::future::Future for I2cFuture<'_> {
         let error = self.check_error();
 
         if error.is_err() {
-            return Poll::Ready(error);
+            Poll::Ready(error)
-        }
+        } else if self.is_done() {
-
-        if self.event_bit_is_clear() {
             Poll::Ready(Ok(()))
         } else {
             Poll::Pending
@@ -1106,6 +1101,8 @@ impl embedded_hal_async::i2c::I2c for I2c<'_, Async> {
 }
 
 fn async_handler(info: &Info, state: &State) {
+    // Disable all interrupts. The I2C Future will check events based on the
+    // interrupt status bits.
     info.regs().int_ena().write(|w| unsafe { w.bits(0) });
 
     state.waker.wake();
@@ -1403,15 +1400,15 @@ impl Driver<'_> {
     /// Resets the I2C controller (FIFO + FSM + command list)
     fn reset(&self) {
         // Reset the FSM
-        // (the option to reset the FSM is not available
+        // (the option to reset the FSM is not available for the ESP32)
-        // for the ESP32)
         #[cfg(not(esp32))]
         self.regs().ctr().modify(|_, w| w.fsm_rst().set_bit());
+        self.reset_before_transmission();
+    }
 
+    fn reset_before_transmission(&self) {
         // Clear all I2C interrupts
-        self.regs()
+        self.clear_all_interrupts();
-            .int_clr()
-            .write(|w| unsafe { w.bits(I2C_LL_INTR_MASK) });
 
         // Reset fifo
         self.reset_fifo();
@@ -1918,114 +1915,106 @@ impl Driver<'_> {
     async fn wait_for_completion(&self, end_only: bool) -> Result<(), Error> {
         self.check_errors()?;
 
-        if end_only {
+        let event = if end_only {
-            I2cFuture::new(Event::EndDetect, self.info, self.state).await?;
+            Event::EndDetect.into()
         } else {
-            let res = embassy_futures::select::select(
+            Event::TxComplete | Event::EndDetect
-                I2cFuture::new(Event::TxComplete, self.info, self.state),
+        };
-                I2cFuture::new(Event::EndDetect, self.info, self.state),
+        I2cFuture::new(event, self.info, self.state).await?;
-            )
+        self.check_all_commands_done().await
-            .await;
-
-            match res {
-                embassy_futures::select::Either::First(res) => res?,
-                embassy_futures::select::Either::Second(res) => res?,
-            }
-        }
-        self.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
+        let start = Instant::now();
-        // given the short time we spend here
 
-        let mut tout = MAX_ITERATIONS / 10; // adjust the timeout because we are yielding in the loop
+        while !self.is_completed(end_only)? {
-        loop {
+            if Instant::now() - start > TIMEOUT {
-            let interrupts = self.regs().int_raw().read();
-
-            self.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.check_all_commands_done()?;
+
-        Ok(())
+        self.check_all_commands_done().await
     }
 
     /// Waits for the completion of an I2C transaction.
     fn wait_for_completion_blocking(&self, end_only: bool) -> Result<(), Error> {
-        let mut tout = MAX_ITERATIONS;
+        let start = Instant::now();
-        loop {
-            let interrupts = self.regs().int_raw().read();
 
-            self.check_errors()?;
+        while !self.is_completed(end_only)? {
-
+            if Instant::now() - start > TIMEOUT {
-            // 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);
             }
         }
-        self.check_all_commands_done()?;
+
+        self.check_all_commands_done_blocking()
+    }
+
+    fn is_completed(&self, end_only: bool) -> Result<bool, Error> {
+        let interrupts = self.regs().int_raw().read();
+
+        self.check_errors()?;
+
+        // Handle completion cases
+        // A full transmission was completed (either a STOP condition or END was
+        // processed)
+        Ok((!end_only && interrupts.trans_complete().bit_is_set())
+            || interrupts.end_detect().bit_is_set())
+    }
+
+    fn all_commands_done(&self) -> bool {
+        for cmd_reg in self.regs().comd_iter() {
+            let cmd = cmd_reg.read();
+
+            // if there is a valid command which is not END, check if it's marked as done
+            if cmd.bits() != 0x0 && !cmd.opcode().is_end() && !cmd.command_done().bit_is_set() {
+                // Let's retry
+                return false;
+            }
+
+            // once we hit END or STOP we can break the loop
+            if cmd.opcode().is_end() || cmd.opcode().is_stop() {
+                break;
+            }
+        }
+        true
+    }
+
+    /// Checks whether all I2C commands have completed execution.
+    fn check_all_commands_done_blocking(&self) -> Result<(), Error> {
+        // NOTE: on esp32 executing the end command generates the end_detect interrupt
+        //       but does not seem to clear the done bit! So we don't check the done
+        //       status of an end command
+        // loop until commands are actually done
+
+        let start = Instant::now();
+
+        while !self.all_commands_done() {
+            if Instant::now() - start > TIMEOUT {
+                return Err(Error::ExecutionIncomplete);
+            }
+        }
+
         Ok(())
     }
 
     /// Checks whether all I2C commands have completed execution.
-    fn check_all_commands_done(&self) -> Result<(), Error> {
+    async fn check_all_commands_done(&self) -> Result<(), Error> {
         // NOTE: on esp32 executing the end command generates the end_detect interrupt
         //       but does not seem to clear the done bit! So we don't check the done
         //       status of an end command
-        let mut cnt = MAX_ITERATIONS;
         // loop until commands are actually done
-        loop {
-            let mut not_done = false;
-            for cmd_reg in self.regs().comd_iter() {
-                let cmd = cmd_reg.read();
 
-                // if there is a valid command which is not END, check if it's marked as done
+        let start = Instant::now();
-                if cmd.bits() != 0x0 && !cmd.opcode().is_end() && !cmd.command_done().bit_is_set() {
-                    not_done = true;
-                }
 
-                // once we hit END or STOP we can break the loop
+        while !self.all_commands_done() {
-                if cmd.opcode().is_end() || cmd.opcode().is_stop() {
+            if Instant::now() - start > TIMEOUT {
-                    break;
-                }
-            }
-
-            if !not_done {
-                break;
-            }
-
-            cnt -= 1;
-            if cnt == 0 {
                 return Err(Error::ExecutionIncomplete);
             }
+            embassy_futures::yield_now().await;
         }
+
         Ok(())
     }
 
@@ -2152,9 +2141,6 @@ impl Driver<'_> {
         bytes: &[u8],
         start: bool,
     ) -> Result<(), Error> {
-        address.validate()?;
-        self.reset_fifo();
-        self.reset_command_list();
         let cmd_iterator = &mut self.regs().comd_iter();
 
         if start {
@@ -2186,10 +2172,6 @@ impl Driver<'_> {
         start: bool,
         will_continue: bool,
     ) -> Result<(), Error> {
-        address.validate()?;
-        self.reset_fifo();
-        self.reset_command_list();
-
         let cmd_iterator = &mut self.regs().comd_iter();
 
         if start {
@@ -2219,7 +2201,7 @@ impl Driver<'_> {
         stop: bool,
     ) -> Result<(), Error> {
         address.validate()?;
-        self.clear_all_interrupts();
+        self.reset_before_transmission();
 
         // Short circuit for zero length writes without start or end as that would be an
         // invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
@@ -2256,7 +2238,7 @@ impl Driver<'_> {
         will_continue: bool,
     ) -> Result<(), Error> {
         address.validate()?;
-        self.clear_all_interrupts();
+        self.reset_before_transmission();
 
         // Short circuit for zero length reads as that would be an invalid operation
         // read lengths in the TRM (at least for ESP32-S3) are 1-255
@@ -2310,7 +2292,7 @@ impl Driver<'_> {
         stop: bool,
     ) -> Result<(), Error> {
         address.validate()?;
-        self.clear_all_interrupts();
+        self.reset_before_transmission();
 
         // Short circuit for zero length writes without start or end as that would be an
         // invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
@@ -2347,7 +2329,7 @@ impl Driver<'_> {
         will_continue: bool,
     ) -> Result<(), Error> {
         address.validate()?;
-        self.clear_all_interrupts();
+        self.reset_before_transmission();
 
         // Short circuit for zero length reads as that would be an invalid operation
         // read lengths in the TRM (at least for ESP32-S3) are 1-255