I2C clock stretching not implemented at all occurrences

[maarten-pennings]

When using as I2C slave the CCS811 gas sensor from ams, e.g. using this board, there are communication errors. These are due to the CCS811 doing clock stretching.

The driver file core_esp8266_si2c.c "Software I2C library for esp8266" implements clock stretching. However, at two places clock stretching code is missing. Functions twi_writeTo() and twi_readFrom() set the clock line high using SCL_HIGH(), without checking whether SCL really goes high. All other places in the driver that use SCL_HIGH() do have the clock stretch check.

In both functions, one line should be added (here and here):

  while(SDA_READ() == 0 && (i++) < 10){
    SCL_LOW();
    twi_delay(twi_dcount);
    SCL_HIGH();

    unsigned int t=0; while(SCL_READ()==0 && (t++)<twi_clockStretchLimit); // Clock stretching

    twi_delay(twi_dcount);
  }

See this screenshot for a diff.

[devyte]

@maarten-pennings I don't have a way to test this. Of note, there are currently no I2C examples or tests. 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, and one sends a hello world to the other. Something like that could then be used as a starting point to test functionality like the issue you describe here.

[maarten-pennings]

Thanks for the reaction.

I understand what you are asking, but if course this is real work, so I don't know how fast I can deliver this. I need to think about it.

I was hoping that a code review would suffice: most CLK_HIGH() calls are followed by a wait till CLK is really high, except two. That does not look right.

[devyte]

@suculent @mrwgx3 @bjoham You've contributed to our I2C. Could one of you please help out with assessing the changes proposed here, and with my request for a Wire master/slave example with 2 ESPs?

[suculent]

I have working implementation of master-slave communication, it’s just a bit complex as it involves simple protocol with CRC check and request-retransfer logic. It would all need to be stripped down first. Odesláno z iPhonu 19. 11. 2018 v 18:56, Develo <[email protected]>:

…

@suculent @mrwgx3 @bjoham You've contributed to our I2C. Could one of you please help out with assessing the changes proposed here, and with my request for a Wire master/slave example with 2 ESPs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[devyte]

@suculent that would make a great example.
Out of curiosity, why the crc and retry? Did you experience communication errors?

[suculent]

Well, maybe it was just a bad wiring but it made me do it. Afterwards I’ve realized that the protocol is almost identical to XMODEM-CRC32. And because I’m transmitting sensitive data over I2C, there’s also an encryption layer. I’ll strip that off and see how it works, once I’ll have suitable test setup. M. Odesláno z iPhonu 20. 11. 2018 v 1:12, Develo <[email protected]>:

…

@suculent that would make a great example. Out of curiosity, why the crc and retry? Did you experience communication errors? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[suculent]

Here is working version of the I2C master/slave example with two ESPs. Please review create issues in that repository, so I can clean it, style it and get ready for real PR (in case it will work to someone else than me :o).

https://github.com/suculent/esp8266-I2C-slave

Unfortunately I don't have CCS811 or anything else I'd now does clock-stretching at all to test the suggestion above. I can just confirm that those changes above did not break my generic communication and it's indeed pure copy from 8 another places in the same source file.

[devyte]

@suculent it looks very good! There's a lot of code duplicity between the master and slave sketches, but it's an example, so I'm ok with that.
My other comments are specific to code style and conventions. I think we can discuss that directly in the PR.
The next step after this would be to figure out how to mock clock stretching in the slave to test the code change proposed by @maarten-pennings here.

[suculent]

I was wondering whether the code duplicity could be solved by shared header but including ../ seems expectably forbidden. Odesláno z iPhonu 21. 11. 2018 v 17:40, Develo <[email protected]>:

…

@suculent it looks very good! There's a lot of code duplicity between the master and slave sketches, but it's an example, so I'm ok with that. My other comments are specific to code style and conventions. I think we can discuss that directly in the PR. The next step after this would be to figure out how to mock clock stretching in the slave to test the code change proposed by @maarten-pennings here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[suculent]

Clock-stretch fix as proposed by @maarten-pennings: #5362
My humble master-slave I2C test converted to example: #5360

[suculent]

The clock-stretching can be simulated/performed from slave side by holding the clock LOW. What the fix does is that it reads the clock and waits until it is low in its two cases as highlighted in copy-paste below. It serves the purpose where returning value (e.g. measurement) takes more time than I2C clock allows.

Source

In an I2C communication, the master device determines the clock speed. The I2C bus provides an explicit clock signal which relieves master and slave from synchronizing exactly to a predefined baud rate.
However, there are situations where an I2C slave is not able to co-operate with the clock speed given by the master and needs to slow down a little. This is done by a mechanism referred to as clock stretching.
An I2C slave is allowed to hold down the clock if it needs to reduce the bus speed. The master, on the other hand, is required to read back the clock signal after releasing it to the high state and wait until the line has actually gone high.

[maarten-pennings]

This is the official specification of i2c:

https://www.nxp.com/docs/en/user-guide/UM10204.pdf

"Generation of clock signals on the I2C-bus is always the responsibility of master devices; each master generates its own clock signals when transferring data on the bus. Bus clock signals from a master can only be altered when they are stretched by a slow slave device holding down the clock" ... and several other quotes

[suculent]

Maarten, your input has been already merged to the master branch and will be present in next release. We're following up with the I2C peer-to-peer communication example, which still needs some cleanup after being thoroughly reviewed by @devyte, which must have been hell of a job.

Your input about clock stretching points me the way that there's no sofware-only way to test this. It's peripheral-dependent and can be tested only by observation, hardly by simple automation.

[devyte]

Closing since #5362 is already merged.
The master-slave example PR covers #5288 , and will be handled in that context.

[maarten-pennings]

Is it allowed to add comments when an issue is closed?
I'm getting a bit more familiar with the code, and now start to have doubts.

Yes, a SCL high by master could be forced down by the slave (clock stretch).
And yes that check is missing in the final while loop in both twi_writeTo and twi_readFrom.
So on first sight my fix makes sense.

However, I now think this while loop in itself is already an exception handler. It checks if SDA is low at the end of an "I2C segment" (I use the word "segment" for a part of an I2C transaction between START and either STOP or a repeated START). If SDA is low, the SCL is pulsed up to 10 times. This looks like the bus clear in the I2C spec https://www.nxp.com/docs/en/user-guide/UM10204.pdf (although that one specs 9 pulses, not 10).

The fact that my "fix" does help is dubious. I'm pretty sure that the I2C bus is not blocked (that is a very rare situation), so the fact that the master runs the bus clear loop is a bad sign. That the bus clear loop needs clock stretch wait code is even more dubious.

What is also dubious is that just before this exception handler loop is the generation of a STOP condition.

  if(sendStop) twi_write_stop();

However, this generation is conditional. So, in the case the STOP is not generated, the I2C transaction is not yet over, so the slave is allowed to pull SDA low. This would not constitute a bus block, so the loop should not run.

[devyte]

Yes, comments are allowed after closing!
I can't answer your comment, because I'm not really familiar with the protocol specifics. However, someone else might. If you find a specific problem, feel free to open a new issue for discussion.
In the meantime, please continue investigating!
Also, there are other I2C libs out there, you could compare behavior.

[maarten-pennings]

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