Double I2C read in one transaction skips a clock pulse
maarten-pennings opened this issue · 15 comments
I was writing a clock stretch injector for #5340 and then I stumbled on another error in the driver file core_esp8266_si2c.c "Software I2C library for esp8266" function twi_readFrom.
The problem occurs when issuing two reads in one I2C transaction (i.e. with a repeated START in between). In this scenario, one clock pulse is not generated by the master: the clock pulse for the 9th bit (the ack bit) just before the repeated start. I tested this with the latest release 2_5_0_BETA2 (but it also
fails on 2.4.2).
Unfortunately, I don't know how to write a single ESP8266 program that detects this. Instead, I took an arbitrary I2C slave, and hooked a logic analyzer to the I2C bus
This is the test code running on the ESP8266 (see full sketch attached at the end):
// From slave, read 2 bytes, then repeated START
int res1=Wire.requestFrom(SLAVE_ADDR,(size_t)2,false); // No STOP yet!
// From slave, read 2 bytes, then generate STOP condition
int res2=Wire.requestFrom(SLAVE_ADDR,(size_t)2,true);
and this logic analyzer capture shows the problem:
I tried to fix the code, the following seems to work (but I have not thoroughly tested it):
This leads to the following capture of the logic analyser:
Here is the test sketch:
ESP8266-i2c2xread.zip
@maarten-pennings Thank you for the analysis and for offering a fix!
I believe the current maintainers aren't involved with I2C, so help is needed to test this.
I'm not yet familiar enough with the code, so I'm not sure if the fix works in all cases.
This was my attempt to locate the place in the code where something is wrong.
By the way, I added a comment to #5340, but that was already closed. Is my comment lost, should I reopen the issue, create a new issue? Sorry - new to this process.
I have written an i2c clock stretch injector
https://github.com/maarten-pennings/I2C-tool
and a suggestion how to solve both #5340 and #5528:
https://github.com/maarten-pennings/I2C-tool/blob/master/I2Ctest8266/README.md#how-to-fix
@maarten-pennings ok, let's try to resolve here.
With just reading your description in the above link, and without actually testing (I need an afternoon of peace and quiet to set up the whole thing, which is near impossible these days), what you did is precisely what I envisioned: a master-slave set up, where the slave sketch can do a configurable clock stretch, thereby testing the master sketch.
I would therefore very much like to get the sketches into the core either as examples or as device tests. We could start as examples, then later derive formal device tests from them to run as part of the device test suite.
I would also like to get the docs you wrote into the core.
However, I think this should come after #5464 to get the Wire lib straight first.
Setting milestone for 2.5.0. There's time, but not too much.
@devyte your comment "Is there any chance you can implement something that doesn't depend on specialized hardware? Example: two ESPs where one is master and the other is slave," did make sense to me. However I could not commit to developing that back then. And on hindsight, it was way more work than expected :-)
Please have a look (when you find your afternoon of peace). My I2C-tool suffers from long interrupt latency and from something I don't understand; the first couple of interrupts are missed, but once it is running it is fine (see the for loop in i2c_init).
Let me know if you have question. And let me know if I can be of any assistance.
the first couple of interrupts are missed, but once it is running it is fine
The primary issue here has a proposed solution, but testing it depends on having a testbench, i.e.: a robust master-slave example, and that's not yet done. Pushing back.
Hi, I'm confused. I wrote a test framework (https://github.com/maarten-pennings/I2C-tool), you seemed positive about this ("what you did is precisely what I envisioned: a master-slave set up, where the slave sketch can do a configurable clock stretch"), but now you "push back".
What do you want me to do?
@maarten-pennings I was waiting for a PR with the changes proposed in "how to fix". The two sketches should be added as examples in the Wire lib.
I was also expecting to have #5464 to test your clock stretching on, but I couldn't get those to work.
Can you make a PR with your work here?
I submitted 5718. Is this what you wanted?
Ok, as I said above, let's resolve in this issue.
Can you please make a Pull Request with your proposed changes to twi* and Wire.cpp, as well as the two sketches as examples in Wire/examples/ ?
@maarten-pennings Are you going to make a PR for the changes you posted at https://github.com/maarten-pennings/I2C-tool/blob/master/I2Ctest8266/README.md#how-to-fix and the examples as requested by @devyte ?
Since all the hard work of @maarten-pennings has not resulted in a pull request, I made one. See #6579
Maybe the changes in there can be discussed and/or reviewed by someone with a proper logic analyzer and knowledge of I2C.
Thanks @TD-er for picking this up. I already forgot about it...