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

I²C Polling (detecting NACKs) #181

Closed
dbrgn opened this issue Jan 24, 2020 · 12 comments
Closed

I²C Polling (detecting NACKs) #181

dbrgn opened this issue Jan 24, 2020 · 12 comments

Comments

@dbrgn
Copy link
Contributor

dbrgn commented Jan 24, 2020

Is there a hardware agnostic way for a driver to poll an I2C bus and return the information whether or not a NACK was received?

This seems possible in the concrete HAL implementations (e.g. in the stm32l0xx-hal), but embedded-hal doesn't seem to offer any predefined errors.

Right now I'm simply using a blocking delay to wait the max duration until a sensor measurement is ready, but polling would be cleaner.

@ryan-summers
Copy link
Contributor

This is a feature that I would like to see as well. It's very often the case with daughter boards where an I2C EEPROM or UID chip is polled to detect whether or not a daughter card is installed.

Attempting to read the EEPROM/UID and detecting the failure isn't as clear as checking if the device is present on the bus and is somewhat confusing.

@therealprof
Copy link
Contributor

Unfortunately I2C doesn't offer a timeout feature so if your connection goes away (or is unstable) there's no standardised way to detect that. SMBUS (an extension of I2C) does specify a timeout so some MCUs which support SMBUS will have builtin support for timeouts, though then again those may require to actually use the SMBUS mode which has a few other implications as well and is not fully compatible with I2C devices.

The only portable solution to implement this in blocking mode would be to require the impl to count the number of attempts getting to get some information off the bus and return an error once the number of attempts go overboard. But then again you might run in troubles when some I2C on the bus is using clock stretching...

@ryan-summers
Copy link
Contributor

ryan-summers commented Jun 21, 2020

I don't think the intention here is to have the embedded HAL automatically detect the transaction failure, but rather have the embedded-hal provide a new API, such as:

pub fn is_present(addr: u16) -> bool;

The is_present() function would then perform a write (of zero length) with the specified address and return true if the write request was ACK'd and return false if NACK'd. This would allow a higher-level driver to implement a polling loop for a device:

let mut i2c_device = <instantiate i2c>;

i2c_device.write([0xAB, 0xCD]).unwrap();

while !i2c_device.is_present() {}

let mut result = [0: 2];
i2c_device.read(&result)?;

There's a large number of I2C sensors / EEPROM that will NACK I2C transactions while they are performing internal calculations / write sequences, so the NACK indication is actually used to indicate the device is busy as opposed to not present.

@therealprof
Copy link
Contributor

I'm not sure that's what the @dbrgn was requesting, at least I'm reading it differently.

i2c_device.write([0xAB, 0xCD]).unwrap();

This will not work. If the MCU writes to the bus and detects that no device is present, this will happily panic your application, thus the next line would be superfluous because you've essentially already detected that the device is not there.

In fact you can already check whether a device is there by simply doing an empty write to some address.

There's a large number of I2C sensors / EEPROM that will NACK I2C transactions while they are performing internal calculations / write sequences, so the NACK indication is actually used to indicate the device is busy as opposed to not present.

I don't think that's how I2C are supposed to operate. Can you give an concrete example of a chip which does this? Anyhow you can do an empty write to test whether the chip is ready to receive more data...

@ryan-summers
Copy link
Contributor

ryan-summers commented Jun 21, 2020

i2c_device.write([0xAB, 0xCD]).unwrap();

This will not work. If the MCU writes to the bus and detects that no device is present, this will happily panic your application, thus the next line would be superfluous because you've essentially already detected that the device is not there.

This is under the assumption that the device is present on the bus and that the [0xAB, CD] data causes it to enter some internally-timed sequence - sorry for lacking clarity there.

In fact you can already check whether a device is there by simply doing an empty write to some address.

Yes, but the intent is less clear when auditing the code. Take the following example:

let device_present = match i2c.write([]) {
    Ok(_) => true,
    Err(_) => false,
};

is less clear than

let device_present = i2c.is_present();

Sure, HAL implements could implement this by extending the embedded-hal API (or device driver crates could do this as well), but my thought here was that the embedded-hal could provide an easy default implementation for anyone implementing the I2c::Write trait.

I don't think that's how I2C are supposed to operate. Can you give an concrete example of a chip which does this? Anyhow you can do an empty write to test whether the chip is ready to receive more data...

Check out the AT24CM01. The relevant portion is shown below:

image

@therealprof
Copy link
Contributor

Check out the AT24CM01. The relevant portion is shown below:

Okay, but that is IMHO a different situation and essentially an optimisation to avoid having to handle unprocessed reads/writes in case chip is busy. I'm still not convinced that this is a case where it would be useful to add special methods and complexity to the traits.

After re-reading the discussion and the original description a few times I think you're actually very much in-line in what you intend to achieve however this is already possible today.

This seems possible in the concrete HAL implementations (e.g. in the stm32l0xx-hal), but embedded-hal doesn't seem to offer any predefined errors.

This point strikes me as something which would be worthwhile to have. Instead of impls providing their own Error types it seems worthwhile to have a fixed set of errors in e-h to allow for drivers to react upon them.

Also since we can now define #[non_exhaustive] enum values we don't run into the problem of constantly having to update impls and drivers to handle each and every obscure error case we add.

@rnestler
Copy link

I guess @dbrgn wants to detect a NACK not for scanning for the presence of devices, but to wait until the device is ready. Some devices (most of Sensirions sensors like SHTC3) will give you a NACK when you try to read the measurement result and it is not yet available. So in pseude code:

sensor.start_measurement();
sensor.wait_for_measurement();
...
fn wait_for_measurement(&mut self) -> Result<E, T> {
    // TODO some driver specific timeout handling
    loop {
        match sensor.try_read() {
              Ok(result) => {
                  ...
                  return data;
              }
              // not yet ready...
              Err(Error::I2cNack) => {},
              // any other error
              Err(e) => {
                   return e;
              }
        }
    }
}

So Error::I2cNack would need to be provided by the embedded HAL to still be able to write the driver in a HAL implementation independent way.

Does that summarize it correctly @dbrgn?

@dbrgn
Copy link
Contributor Author

dbrgn commented Jun 22, 2020

Yes, precisely. Some HALs / MCUs provide a way to detect NACKs, e.g. stm32l0xx:

https://github.com/stm32-rs/stm32l0xx-hal/blob/489685ccd8a55c7371656438b6a86080d0f40e81/src/i2c.rs#L172-L174

(See i2c::Error::Nack)

I'm not sure about timeouts in the standard (maybe @rnestler knows more), but it could be implementation-defined. An implementation with hardware NACK detection would use that, while an implementation without hardware support for detecting NACKs (does that exist?) would allow the user to set the timeout on the I2C peripheral instance.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jun 22, 2020

I imagined something like an I²C error enum defined by embedded-hal, for example:

enum Error<T> {
    Nack,
    Other(T),
}

(Enum potentially marked as #[non_exhaustive])

The methods like write would then wrap the custom error type in that enum:

pub trait Write {
    type Error;
    fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Error<Self::Error>>;
}

(Naming with two different Error types is not ideal, that could still be bikeshedded)

(Edit: Ah, I think this is what @therealprof already suggested above! I overlooked that reply.)

(Edit2: Discussion continues here: #229)

@therealprof
Copy link
Contributor

Yes, please check out #229 for a general proposal to move the Error types from the implementations into embedded-hal for broader and more generic use.

@yodaldevoid
Copy link
Contributor

I believe this has been resolved with #296, but there is a PR that is applicable specifically to this discussion in #316.

@dbrgn
Copy link
Contributor Author

dbrgn commented Jan 10, 2024

With merging of #296 and follow-up pull requests, we now have:

Thus, I think this feature is fully implemented and available to use today! 🎉

@dbrgn dbrgn closed this as completed Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants