esp-rs / esp-idf-hal

embedded-hal implementation for Rust on ESP32 and ESP-IDF
https://docs.esp-rs.org/esp-idf-hal/
Apache License 2.0
473 stars 172 forks source link

I2C timeout calculated wrong for MCU !esp32 #412

Closed empirephoenix closed 5 months ago

empirephoenix commented 7 months ago

The current timeout() for the i2c bus is calculated using

impl From<Duration> for APBTickType {
    fn from(duration: Duration) -> Self {
        APBTickType(
            ((duration.as_nanos() + APB_TICK_PERIOD_NS as u128 - 1) / APB_TICK_PERIOD_NS as u128)
                as ::core::ffi::c_int,
        )
    }
}

Basically the calculation for some newer chips is different and the valid values for the low level api are now 1-22 and the actual timeout is calculated as: 2^12 * SCLK_PERIOD In contrast to the idf api documenation the actual one for the chip correctly explains this:

Chips using the newer formula seems to be at least:

Using a 5 bit register with the new formular ESP32-C2 21.4.7 https://www.espressif.com/sites/default/files/documentation/esp8684_technical_reference_manual_en.pdf ESP32-C3 28.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-c3_technical_reference_manual_en.pdf ESP32-C6 28.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-c6_technical_reference_manual_en.pdf ESP32-S3 27.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf ESP32-H2 27.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-h2_technical_reference_manual_en.pdf

Using a 10bit register with calculation as currently ESP32 11.5 https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf ESP32-S2 25.3.3 https://www.espressif.com/sites/default/files/documentation/esp32-s2_technical_reference_manual_en.pdf

Unclear: ESP32-P4 technical document linked on espressif website returns 404 https://www.espressif.com/sites/default/files/documentation/esp32-p4_technical_reference_manual_en.pdf

If someone else could check that I'm understanding this documents corrrectly, I would like to do a pull request for this. Possible changes I see a) Calculate the nearest valid timeout number for the api and print? if the wished for timeout is not exactly representable b) Add a enum containing the valid 22 timeouts and implement From for APBTickType ? what approach would be prefered?

Also how would one do the case difference based on the current MCU, is there somewhere a similar part of code, I can take a look at? (coming from a c world, I only know #define magic for this, but this is obviously not the rust way)

I also did take a look at the arduino wrapper how they do this... https://github.com/espressif/arduino-esp32/blob/cf448906b3836fbe9368934713b697469254c62f/cores/esp32/esp32-hal-i2c.c#L133 They just set it to the maximum possible timeout, so apart from waiting now >100ms instead of ~13 most probably will not have noticed

ivmarkov commented 7 months ago

The current timeout() for the i2c bus is calculated using

impl From<Duration> for APBTickType {
    fn from(duration: Duration) -> Self {
        APBTickType(
            ((duration.as_nanos() + APB_TICK_PERIOD_NS as u128 - 1) / APB_TICK_PERIOD_NS as u128)
                as ::core::ffi::c_int,
        )
    }
}

Basically the calculation for some newer chips is different and the valid values for the low level api are now 1-22 and the actual timeout is calculated as: 2^12 * SCLK_PERIOD In contrast to the idf api documenation the actual one for the chip correctly explains this:

Chips using the newer formula seems to be at least:

Using a 5 bit register with the new formular ESP32-C2 21.4.7 https://www.espressif.com/sites/default/files/documentation/esp8684_technical_reference_manual_en.pdf ESP32-C3 28.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-c3_technical_reference_manual_en.pdf ESP32-C6 28.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-c6_technical_reference_manual_en.pdf ESP32-S3 27.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf ESP32-H2 27.4.8 https://www.espressif.com/sites/default/files/documentation/esp32-h2_technical_reference_manual_en.pdf

Using a 10bit register with calculation as currently ESP32 11.5 https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf ESP32-S2 25.3.3 https://www.espressif.com/sites/default/files/documentation/esp32-s2_technical_reference_manual_en.pdf

Unclear: ESP32-P4 technical document linked on espressif website returns 404 https://www.espressif.com/sites/default/files/documentation/esp32-p4_technical_reference_manual_en.pdf

If someone else could check that I'm understanding this documents corrrectly, I would like to do a pull request for this.

PR welcome!

Possible changes I see a) Calculate the nearest valid timeout number for the api and print?

This one except for the "print" part.

Also how would one do the case difference based on the current MCU, is there somewhere a similar part of code, I can take a look at? (coming from a c world, I only know #define magic for this, but this is obviously not the rust way)

#cfg(esp32c3)] with the all, not and any combinators. Besides the esp32c3 config bool, you have similar constants defined for each MCU.

Vollbrecht commented 6 months ago

@ivmarkov @empirephoenix can this be closed now?

Vollbrecht commented 5 months ago

we also had #438 merged. i hope that makes it work now. If you still think its wrong after that feel free to reopen this issue