Implements I2C Wire Refactoring on top of IDF #5664
Conversation
|
Thank you so much for this fix. I just discovered the problem with the repeated start condition not being honoured (#4729), when testing code on my ESP32-S2 design where I'm using a Vishay VEML3328 I2C RGB sensor, which requires the repeated start condition to read data from the device. After applying your fix (4 affected files), it works as expected if the sensor is the only device on the I2C bus (I can see the repeated start condition on my logic analyzer, and data is read correctly), however, when I add another device (SSD1306 oled display) on the same bus, I get a crash with the message:
The stack trace report is: Update: Also when I ONLY have the SSD1306 display on the I2C bus I get the above result (using the Adafruit_SSD1306 library v2.4.6). Update 2: I discovered that the above crash happens when Wire.begin() is executed twice. Once explicitly at the beginning of setup(), and secondly within Adafruit_SSD1306::begin(), with the default argument for periphBegin = true. However, when I remove the explicit Wire.begin() OR supply Adafruit_SSD1306::begin() with periphBegin = false (so Wire.begin() is only called once) the above crash doesn't happen, but instead I get the following message: So in either case it still doesn't work. |
Hi @marcboon, thank you for testing and reporting the results back here. |
|
@marcboon , I fixed the issue with begin() you reported. |
|
@marcboon , The issue with Please try again and let me know. |
Yes, thank you very much! Thanks again for the quick update. I hope there won't be any more issues with this patch :) |
| @@ -120,136 +133,28 @@ bool TwoWire::begin(int sdaPin, int sclPin, uint32_t frequency) | |||
| void TwoWire::setTimeOut(uint16_t timeOutMillis) | |||
| { | |||
| _timeOutMillis = timeOutMillis; | |||
| i2cSetTimeOut(i2c, timeOutMillis); | |||
There was a problem hiding this comment.
According to the IDF documentation, the timeout is not in units of a msec, but its unit is: APB 80Mhz clock cycle.
Also it is a bit strange to have it here offered as millis, while the somewhat related stretch limit in the ESP8266 implementation is in usec.
I guess it will break some implementations out there that use the timeout, but at least the parameter suggestion is not what it does.
Having it in msec is fine I guess, but then you need to convert it in units of 1/80th MHz. (thus effectively multiply it by 80000)
There was a problem hiding this comment.
Thanks for the comment.
TwoWire::setTimeOut is an API from Arduino Stream Class.
https://www.arduino.cc/reference/en/language/functions/communication/stream/streamsettimeout/
It takes its argument in milliseconds. Thus, the implementation follows Arduino standard.
If users follow Arduino Standards, nothing will break.
The link you sent is regarding something else, not the whole transaction timeout, but actually the time in cycles that i2c bus can be keept idle in order to the ESP i2c hardware machine state considers that there was no response for ACK/NACK.
The implentation of Arduino HAL i2cSetTimeOut() calculates FreeRTOS ticks and keep this value to use it in the IDF I2C APIs regarading a full i2c transaction, but not the bus idle time in cycles.
You won't find a call to IDF i2c_set_timeout() in the code of this PR.
You can read this PR code in order to see how it's used.
There was a problem hiding this comment.
In this case, I understand there is nothing to change reagarding the matter you presented.
Please let me know if you see diferently or there is something I am missing.
Thank you.
|
Hi All! I am closing this in favor of #5683 Please confirm that all is working as expected :) |
Summary
This PR is a complete refactoring of Wire Library and I2C HAL in order to use IDF instead of current Register manipulation approach.
It implements Arduino Wire and TWI functionality.
Fix #5478
Fix #5635
Fix #5636
Fix #4729
Fix #5470
Impact
It solves many reported issues related to I2C.