From e7278ac2f0cddcee1e822f2bdcf51dbc96ee0fed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Quentin?= <bjoernQ@users.noreply.github.com>
Date: Tue, 7 Jan 2025 09:58:16 +0100
Subject: [PATCH] Re-introduce Data and Address, again

---
 esp-hal/src/i2c/master/mod.rs | 47 +++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs
index 76275d11c..7a4fdb484 100644
--- a/esp-hal/src/i2c/master/mod.rs
+++ b/esp-hal/src/i2c/master/mod.rs
@@ -107,10 +107,19 @@ pub enum Error {
 }
 
 /// 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,
@@ -119,6 +128,8 @@ pub enum AcknowledgeCheckFailedReason {
 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"),
         }
     }
@@ -127,6 +138,10 @@ impl core::fmt::Display for AcknowledgeCheckFailedReason {
 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
             }
@@ -687,9 +702,9 @@ impl<'a> I2cFuture<'a> {
         }
 
         if r.nack().bit_is_set() {
-            return Err(Error::AcknowledgeCheckFailed(
-                AcknowledgeCheckFailedReason::Unknown,
-            ));
+            return Err(Error::AcknowledgeCheckFailed(estimate_ack_failed_reason(
+                self.info.register_block(),
+            )));
         }
 
         #[cfg(not(esp32))]
@@ -703,7 +718,7 @@ impl<'a> I2cFuture<'a> {
                 .bit_is_clear()
         {
             return Err(Error::AcknowledgeCheckFailed(
-                AcknowledgeCheckFailedReason::Unknown,
+                AcknowledgeCheckFailedReason::Data,
             ));
         }
 
@@ -1719,7 +1734,7 @@ impl Driver<'_> {
                 let retval = if interrupts.time_out().bit_is_set() {
                     Err(Error::Timeout)
                 } else if interrupts.nack().bit_is_set() {
-                    Err(Error::AcknowledgeCheckFailed(AcknowledgeCheckFailedReason::Unknown))
+                    Err(Error::AcknowledgeCheckFailed(estimate_ack_failed_reason(self.register_block())))
                 } else if interrupts.arbitration_lost().bit_is_set() {
                     Err(Error::ArbitrationLost)
                 } else {
@@ -1730,11 +1745,11 @@ impl Driver<'_> {
                 let retval = if interrupts.time_out().bit_is_set() {
                     Err(Error::Timeout)
                 } else if interrupts.nack().bit_is_set() {
-                    Err(Error::AcknowledgeCheckFailed(AcknowledgeCheckFailedReason::Unknown))
+                    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::AcknowledgeCheckFailed(AcknowledgeCheckFailedReason::Unknown))
+                    Err(Error::AcknowledgeCheckFailed(AcknowledgeCheckFailedReason::Data))
                 } else {
                     Ok(())
                 };
@@ -2318,6 +2333,24 @@ 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 {
+                return AcknowledgeCheckFailedReason::Address;
+            } else {
+                return AcknowledgeCheckFailedReason::Data;
+            }
+
+        }
+    }
+}
+
 macro_rules! instance {
     ($inst:ident, $peri:ident, $scl:ident, $sda:ident, $interrupt:ident) => {
         impl Instance for crate::peripherals::$inst {