From d0eb59d47d32092edc415f4bd101ea5f9e7c0925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 8 May 2025 09:35:56 +0200 Subject: [PATCH] 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 | 220 ++++++++++++++++------------------ 1 file changed, 101 insertions(+), 119 deletions(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 938937c44..e5e5d955b 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/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; - -// on ESP32 there is a chance to get trapped in `wait_for_completion` forever -const MAX_ITERATIONS: u32 = 1_000_000; +const TIMEOUT: Duration = Duration::from_millis(10); /// 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, 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, info: &'a Info, state: &'a State) -> Self { info.regs().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(), - }; + for event in events { + 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(), + }; + } 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 { - let r = self.info.regs().int_ena().read(); - - 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 is_done(&self) -> bool { + !self.info.interrupts().is_disjoint(self.events) } 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); - } - - if self.event_bit_is_clear() { + Poll::Ready(error) + } else if self.is_done() { 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 - // for the ESP32) + // (the option to reset the FSM is not available 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() - .int_clr() - .write(|w| unsafe { w.bits(I2C_LL_INTR_MASK) }); + self.clear_all_interrupts(); // 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 { - I2cFuture::new(Event::EndDetect, self.info, self.state).await?; + let event = if end_only { + Event::EndDetect.into() } else { - let res = embassy_futures::select::select( - I2cFuture::new(Event::TxComplete, self.info, self.state), - I2cFuture::new(Event::EndDetect, self.info, self.state), - ) - .await; - - match res { - embassy_futures::select::Either::First(res) => res?, - embassy_futures::select::Either::Second(res) => res?, - } - } - self.check_all_commands_done()?; - - Ok(()) + Event::TxComplete | Event::EndDetect + }; + I2cFuture::new(event, self.info, self.state).await?; + self.check_all_commands_done().await } #[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 start = Instant::now(); - let mut tout = MAX_ITERATIONS / 10; // adjust the timeout because we are yielding in the loop - loop { - 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 { + while !self.is_completed(end_only)? { + if Instant::now() - start > TIMEOUT { 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; - loop { - let interrupts = self.regs().int_raw().read(); + let start = Instant::now(); - 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 { + while !self.is_completed(end_only)? { + if Instant::now() - start > TIMEOUT { return Err(Error::Timeout); } } - self.check_all_commands_done()?; + + self.check_all_commands_done_blocking() + } + + fn is_completed(&self, end_only: bool) -> Result { + 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 - if cmd.bits() != 0x0 && !cmd.opcode().is_end() && !cmd.command_done().bit_is_set() { - not_done = true; - } + let start = Instant::now(); - // once we hit END or STOP we can break the loop - if cmd.opcode().is_end() || cmd.opcode().is_stop() { - break; - } - } - - if !not_done { - break; - } - - cnt -= 1; - if cnt == 0 { + while !self.all_commands_done() { + if Instant::now() - start > TIMEOUT { 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