ESP32/ESP32-S2: I2C Workaround #3199
Conversation
|
We need a test case in the HIL tester that fails without this PR. |
I would love that, but we would need a suitable I2C slave - seems the BMP180 doesn't like to get large writes/reads |
hil-test/tests/i2c.rs
Outdated
| @@ -71,7 +71,7 @@ mod tests { | |||
| } | |||
| } | |||
|
|
|||
| assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[]), Ok(())); | |||
| assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[0xaa]), Ok(())); | |||
There was a problem hiding this comment.
This is wrong, we need to support 0-byte writes
There was a problem hiding this comment.
ah - yeah now I see it was not always a no-op apparently
As said we can't with the BMP180 - one thing we could do it artificially reduce the FIFO size for the test to force the error (maybe) - not sure if that is something we really want - if we want it, we could make the FIFO size user-configurable |
In case y'all decide to add another i2c slave, I vote for a gpio expander, as it can be validated with other peripherals (Like RMT, SPI slave, pcnt, etc) They also tend to support infinite reads and writes |
|
Probably fixes #3060 |
esp-hal/src/i2c/master/mod.rs
Outdated
| @@ -2415,8 +2587,13 @@ impl Driver<'_> { | |||
| if buffer.is_empty() { | |||
| return self.write_operation_blocking(address, &[], start, stop); | |||
| } | |||
| let chunk_count = buffer.len().div_ceil(I2C_CHUNK_SIZE); | |||
| for (idx, chunk) in buffer.chunks(I2C_CHUNK_SIZE).enumerate() { | |||
| let chunk_size = if buffer.len() % I2C_CHUNK_SIZE < 2 { | |||
There was a problem hiding this comment.
What's the logic here? Are you trying to avoid having a single byte chunk at the end? Because if so, it's still possible. For esp32, chunk_size is either 31 or 29. If the buffer length is slightly longer than 31 * 29 = 899, the last chunk will be the same size for each chunk_size. Indeed I am hitting timeouts when the buffer length is 900 and 901.
For me on esp32 I am hitting timeouts with certain buffer sizes when the last chunk is of size is 2. So any buffer length in the set {31, 64, 95, 126 ...}, I cannot finish the read (the last chunk times out).
There was a problem hiding this comment.
True - this needs to get fixed
There was a problem hiding this comment.
But the good news it's that the general way to handle it is fine
bjoernQ
force-pushed
the
esp32-i2c-fix-for-real
branch
from
a7a24d3 to
e06ad6c
Compare
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Fixes #3178
Fixes #3167
Basically, this does three things
write,readandwrite_readI don't see how to guarantee things will always work for a custom
transactionon ESP32/ESP32-S2 - so I added some docsTesting
I used an VL53L5 sensor (which requires large (32k) writes and reads (a few hundred bytes). Additionally, I tested with a bit-banged I2C slave ( https://github.com/bjoernQ/i2c-dummy-device )