diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index dcc4525d67..25354f8872 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -99,13 +99,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Renamed variants of `CpuClock`, made the enum non-exhaustive (#2899) - SPI: Fix naming violations for `Mode` enum variants (#2902) - SPI: Fix naming violations for `Address` and `Command` enum variants (#2906) - - `ClockSource` enums are now `#[non_exhaustive]` (#2912) - - `gpio::{Input, Flex}::wakeup_enable` now returns an error instead of panicking. (#2916) - - Removed the `I` prefix from `DriveStrength` enum variants. (#2922) - Removed the `Attenuation` prefix from `Attenuation` enum variants. (#2922) +- Renamed / changed some I2C error variants (#2844, #2862) ### Fixed diff --git a/esp-hal/MIGRATING-0.22.md b/esp-hal/MIGRATING-0.22.md index 82f44edf65..59ca402ccf 100644 --- a/esp-hal/MIGRATING-0.22.md +++ b/esp-hal/MIGRATING-0.22.md @@ -326,6 +326,13 @@ To avoid abbreviations and contractions (as per the esp-hal guidelines), some er + Error::ZeroLengthInvalid ``` +The `AckCheckFailed` variant changed to `AcknowledgeCheckFailed(AcknowledgeCheckFailedReason)` + +```diff +- Err(Error::AckCheckFailed) ++ Err(Error::AcknowledgeCheckFailed(reason)) +``` + ## The crate prelude has been removed The reexports that were previously part of the prelude are available through other paths: diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index d090dba897..e87402b5c2 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -93,7 +93,7 @@ pub enum Error { /// The transmission exceeded the FIFO size. FifoExceeded, /// The acknowledgment check failed. - AckCheckFailed, + AcknowledgeCheckFailed(AcknowledgeCheckFailedReason), /// A timeout occurred during transmission. Timeout, /// The arbitration for the bus was lost. @@ -106,13 +106,58 @@ pub enum Error { ZeroLengthInvalid, } +/// I2C no acknowledge error reason. +/// +/// Consider this as a hint and make sure to always handle all cases. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[non_exhaustive] +pub enum AcknowledgeCheckFailedReason { + /// The device did not acknowledge its address. The device may be missing. + Address, + + /// The device did not acknowledge the data. It may not be ready to process + /// requests at the moment. + Data, + + /// Either the device did not acknowledge its address or the data, but it is + /// unknown which. + Unknown, +} + +impl core::fmt::Display for AcknowledgeCheckFailedReason { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + AcknowledgeCheckFailedReason::Address => write!(f, "Address"), + AcknowledgeCheckFailedReason::Data => write!(f, "Data"), + AcknowledgeCheckFailedReason::Unknown => write!(f, "Unknown"), + } + } +} + +impl From<&AcknowledgeCheckFailedReason> for embedded_hal::i2c::NoAcknowledgeSource { + fn from(value: &AcknowledgeCheckFailedReason) -> Self { + match value { + AcknowledgeCheckFailedReason::Address => { + embedded_hal::i2c::NoAcknowledgeSource::Address + } + AcknowledgeCheckFailedReason::Data => embedded_hal::i2c::NoAcknowledgeSource::Data, + AcknowledgeCheckFailedReason::Unknown => { + embedded_hal::i2c::NoAcknowledgeSource::Unknown + } + } + } +} + impl core::error::Error for Error {} impl core::fmt::Display for Error { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { Error::FifoExceeded => write!(f, "The transmission exceeded the FIFO size"), - Error::AckCheckFailed => write!(f, "The acknowledgment check failed"), + Error::AcknowledgeCheckFailed(reason) => { + write!(f, "The acknowledgment check failed. Reason: {}", reason) + } Error::Timeout => write!(f, "A timeout occurred during transmission"), Error::ArbitrationLost => write!(f, "The arbitration for the bus was lost"), Error::ExecutionIncomplete => { @@ -211,12 +256,12 @@ impl Operation<'_> { impl embedded_hal::i2c::Error for Error { fn kind(&self) -> embedded_hal::i2c::ErrorKind { - use embedded_hal::i2c::{ErrorKind, NoAcknowledgeSource}; + use embedded_hal::i2c::ErrorKind; match self { Self::FifoExceeded => ErrorKind::Overrun, Self::ArbitrationLost => ErrorKind::ArbitrationLoss, - Self::AckCheckFailed => ErrorKind::NoAcknowledge(NoAcknowledgeSource::Unknown), + Self::AcknowledgeCheckFailed(reason) => ErrorKind::NoAcknowledge(reason.into()), _ => ErrorKind::Other, } } @@ -633,7 +678,9 @@ impl<'a> I2cFuture<'a> { } if r.nack().bit_is_set() { - return Err(Error::AckCheckFailed); + return Err(Error::AcknowledgeCheckFailed(estimate_ack_failed_reason( + self.info.register_block(), + ))); } #[cfg(not(esp32))] @@ -646,7 +693,9 @@ impl<'a> I2cFuture<'a> { .resp_rec() .bit_is_clear() { - return Err(Error::AckCheckFailed); + return Err(Error::AcknowledgeCheckFailed( + AcknowledgeCheckFailedReason::Data, + )); } Ok(()) @@ -1655,7 +1704,7 @@ impl Driver<'_> { let retval = if interrupts.time_out().bit_is_set() { Err(Error::Timeout) } else if interrupts.nack().bit_is_set() { - Err(Error::AckCheckFailed) + Err(Error::AcknowledgeCheckFailed(estimate_ack_failed_reason(self.register_block()))) } else if interrupts.arbitration_lost().bit_is_set() { Err(Error::ArbitrationLost) } else { @@ -1666,11 +1715,11 @@ impl Driver<'_> { let retval = if interrupts.time_out().bit_is_set() { Err(Error::Timeout) } else if interrupts.nack().bit_is_set() { - Err(Error::AckCheckFailed) + Err(Error::AcknowledgeCheckFailed(estimate_ack_failed_reason(self.register_block()))) } else if interrupts.arbitration_lost().bit_is_set() { Err(Error::ArbitrationLost) } else if interrupts.trans_complete().bit_is_set() && self.register_block().sr().read().resp_rec().bit_is_clear() { - Err(Error::AckCheckFailed) + Err(Error::AcknowledgeCheckFailed(AcknowledgeCheckFailedReason::Data)) } else { Ok(()) }; @@ -2254,6 +2303,23 @@ fn write_fifo(register_block: &RegisterBlock, data: u8) { } } +// Estimate the reason for an acknowledge check failure on a best effort basis. +// When in doubt it's better to return `Unknown` than to return a wrong reason. +fn estimate_ack_failed_reason(_register_block: &RegisterBlock) -> AcknowledgeCheckFailedReason { + cfg_if::cfg_if! { + if #[cfg(any(esp32, esp32s2, esp32c2, esp32c3))] { + AcknowledgeCheckFailedReason::Unknown + } else { + // this is based on observations rather than documented behavior + if _register_block.fifo_st().read().txfifo_raddr().bits() <= 1 { + AcknowledgeCheckFailedReason::Address + } else { + AcknowledgeCheckFailedReason::Data + } + } + } +} + macro_rules! instance { ($inst:ident, $peri:ident, $scl:ident, $sda:ident, $interrupt:ident) => { impl Instance for crate::peripherals::$inst { diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs index 6aa5af36fb..032ef04162 100644 --- a/hil-test/tests/i2c.rs +++ b/hil-test/tests/i2c.rs @@ -7,7 +7,7 @@ #![no_main] use esp_hal::{ - i2c::master::{Config, Error, I2c, Operation}, + i2c::master::{AcknowledgeCheckFailedReason, Config, Error, I2c, Operation}, Async, Blocking, }; @@ -51,10 +51,26 @@ mod tests { #[test] fn empty_write_returns_ack_error_for_unknown_address(mut ctx: Context) { - assert_eq!( - ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]), - Err(Error::AckCheckFailed) - ); + // on some chips we can determine the ack-check-failed reason but not on all + // chips + cfg_if::cfg_if! { + if #[cfg(any(esp32,esp32s2,esp32c2,esp32c3))] { + assert_eq!( + ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]), + Err(Error::AcknowledgeCheckFailed( + AcknowledgeCheckFailedReason::Unknown + )) + ); + } else { + assert_eq!( + ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]), + Err(Error::AcknowledgeCheckFailed( + AcknowledgeCheckFailedReason::Address + )) + ); + } + } + assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[]), Ok(())); }