Add AcknowledgeCheckFailedReason (#2862)

* Add `AcknowledgeCheckFailedReason`

* Fix

* Remove unused AcknowledgeCheckFailedReason variants

* Re-introduce Data and Address, again

* Clippy + Tests

---------

Co-authored-by: Jesse Braham <jessebraham@users.noreply.github.com>

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
 

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:

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 {

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(()));
     }