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 <notifications@github.com>:

…

@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 <notifications@github.com>:

…

@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 <notifications@github.com>:

…

@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.