Skip to content

Commit

Permalink
Fix i2c freeze (#1847)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@bjoernQ
bjoernQ authored Jul 24, 2024
1 parent 4ef91c5 commit 7f251b4
Show file tree
Hide file tree
Showing 2 changed files with 168 additions and 18 deletions.
1 change: 1 addition & 0 deletions esp-hal/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
185 changes: 167 additions & 18 deletions esp-hal/src/i2c.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand All @@ -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 }
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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(());
Expand All @@ -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,
Expand Down Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -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();

Expand All @@ -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(())
Expand Down Expand Up @@ -1699,7 +1844,9 @@ pub trait Instance: crate::private::Sealed {
.read()
.txfifo_wm()
.bit_is_set()
{}
{
self.check_errors()?;
}

self.register_block()
.int_clr()
Expand All @@ -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(());
Expand Down

0 comments on commit 7f251b4

Please sign in to comment.