From 7f251b457a7ab103a42854bdce12df080e1d18d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Quentin?= Date: Wed, 24 Jul 2024 17:09:46 +0200 Subject: [PATCH] Fix i2c freeze (#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 --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/i2c.rs | 185 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 168 insertions(+), 18 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index b545a792d8f..a912c67dfdc 100644 --- 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 index 03fccb93992..b0fd59dc85e 100644 --- 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,6 +633,7 @@ 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, @@ -630,6 +641,7 @@ mod asynch { 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 { 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,12 +1087,26 @@ 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(); } @@ -989,12 +1114,26 @@ mod asynch { #[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(());