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

I2C timeout calculated wrong for MCU !esp32 #412

Open
empirephoenix opened this issue Apr 28, 2024 · 2 comments
Open

I2C timeout calculated wrong for MCU !esp32 #412

empirephoenix opened this issue Apr 28, 2024 · 2 comments

Comments

@empirephoenix
Copy link
Contributor

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
Copy link
Collaborator

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.

ivmarkov pushed a commit that referenced this issue May 22, 2024
* add initial i2c timeout rework for newer esps

* rewrite with direct approach

* fix formatting

* clippy :1

* use integer calculation as suggested

* cleanup code and variable names

---------

Co-authored-by: Empire Phoenix <[email protected]>
@Vollbrecht
Copy link
Collaborator

@ivmarkov @empirephoenix can this be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

3 participants