Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AcknowledgeCheckFailedReason #2862

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions esp-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- I2C: Replaced potential panics with errors. (#2831)
- UART: Make `AtCmdConfig` and `ConfigError` non-exhaustive (#2851)
- UART: Make `AtCmdConfig` use builder-lite pattern (#2851)
- Renamed / changed some I2C error variants (#2844, #2862)

### Fixed

Expand Down
7 changes: 7 additions & 0 deletions esp-hal/MIGRATING-0.22.md
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,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:
Expand Down
63 changes: 54 additions & 9 deletions esp-hal/src/i2c/master/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -106,13 +106,54 @@ pub enum Error {
ZeroLengthInvalid,
}

/// I2C no acknowledge error reason.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
#[non_exhaustive]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this non_exhaustive, isn't this enum complete? What else is there for the device to not acknowledge?

(Using both Unknown and non_exhaustive seems wrong to me at a glance)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In future we might want to provide more information with the error (e.g. at which byte count the nack was detected) - adding such a variant would be a breaking change then. I imagine user-code will do the same when matching Unknown or _ so I don't think it's a big deal

I agree for now we wouldn't even need Address and Data for now, then

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 => {
Expand Down Expand Up @@ -211,12 +252,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,
}
}
Expand Down Expand Up @@ -657,7 +698,9 @@ impl<'a> I2cFuture<'a> {
}

if r.nack().bit_is_set() {
return Err(Error::AckCheckFailed);
return Err(Error::AcknowledgeCheckFailed(
AcknowledgeCheckFailedReason::Unknown,
));
}

#[cfg(not(esp32))]
Expand All @@ -670,7 +713,9 @@ impl<'a> I2cFuture<'a> {
.resp_rec()
.bit_is_clear()
{
return Err(Error::AckCheckFailed);
return Err(Error::AcknowledgeCheckFailed(
AcknowledgeCheckFailedReason::Unknown,
));
}

Ok(())
Expand Down Expand Up @@ -1684,7 +1729,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(AcknowledgeCheckFailedReason::Unknown))
} else if interrupts.arbitration_lost().bit_is_set() {
Err(Error::ArbitrationLost)
} else {
Expand All @@ -1695,11 +1740,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(AcknowledgeCheckFailedReason::Unknown))
} 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::Unknown))
} else {
Ok(())
};
Expand Down
7 changes: 5 additions & 2 deletions hil-test/tests/i2c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![no_main]

use esp_hal::{
i2c::master::{Config, Error, I2c, Operation},
i2c::master::{AcknowledgeCheckFailedReason, Config, Error, I2c, Operation},
Async,
Blocking,
};
Expand Down Expand Up @@ -50,9 +50,12 @@ mod tests {

#[test]
fn empty_write_returns_ack_error_for_unknown_address(mut ctx: Context) {
// we don't specify the exact reason yet
assert_eq!(
ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]),
Err(Error::AckCheckFailed)
Err(Error::AcknowledgeCheckFailed(
AcknowledgeCheckFailedReason::Unknown
))
);
assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[]), Ok(()));
}
Expand Down
Loading