esp8266: i2c doesn't support clock stretching

[deshipu]

I'm trying to get the ESP8266 to talk over I²C to an Arduino board working as a slave using the standard Wire.h library there. The code I'm using to test this on the micropython side is:

from machine import Pin, I2C
bus = I2C(scl=Pin(5), sda=Pin(4))
bus.writeto(0x9, 'aaaaaaaa')

This works, but in about 1 time out of 5 I get an OSError: I2C bus error. 1 time in 50 this error happens in such a moment, that the slave becomes stuck and unresponsive. I looked at the signals with an osciloscope, and while the data line looks the same in both cases, the clock line differs.

This is the final packet of a working command:

And this is the final packet of a command that fails and throws OSError:

I'm not entirely sure what is happening here, but it might have something to do with the clock stretching on the Arduino side.

[deshipu]

I tried with those two lines modified to get a slower speed:

#define mphal_i2c_wait_a() os_delay_us(20)
#define mphal_i2c_wait_b() os_delay_us(10)

And that makes it super-reliable, so I suppose this is the problem with clock stretching.

[pfalcon]

1 time in 50 this error happens in such a moment, that the slave becomes stuck and unresponsive.

"The slave" you mean here is Arduino board, right? Isn't this being stuck a bug in Arduino Wire library?

We definitely don't support clock stretching and other advanced I2C features. Do you have a "real" hardware I2C device to test instead? Something like FlashROM would be ideal - try to read it in loop with checksuming.

[deshipu]

"Real" I²C devices do clock stretching too sometimes. It's not like it's an optional part of the standard.

I didn't have such problems with an I²C magnetometer, but that sends very short messages, and probably is fast enough to not have to stretch the clock.

[deshipu]

I suppose that not supporting clock stretching explains this behavior, and makes it completely expected. Sorry for wasting your time.

[pfalcon]

I mean the current implementation doesn't support clock stretching, and the topic of the discussion was to differentiate between "I2C doesn't work reliably" vs "I2C clock stretching not supported". I assume you're not interested in confirming that it's 100% clock stretching issue (by testing another kind of device), and that's fully ok, as the report above is already of big help. Should be left open, waiting for our testing and resolution.

[deshipu]

I should get an SSD1306 OLED display soon, that should be a good candidate for testing. In the mean time, I can only test sending (relatively) long data with microcontroller-based slaves, such as the Ardunio, PyBoard, Teensy, etc. I suppose I could also try testing with RPi, but its I²C hardware is known to be buggy.

[pfalcon]

Ack, thanks. I don't see big problem to support clock stretching, the usual problem is how to test all this stuff. Then 2nd problem is to make sure that new features added don't regress previously working setups/devices. So, we could use help with diversified testing (but we aren't dumping it on somebody else - it's our task in the end!). Anyway, our with @dpgeorge default work split is that I'm looking after networking stuff and he's after hardware, so I'll leave further discussion to him for now (I can join later if needed).

[gojimmypi]

fwiw - on my Rigol I2C decoding, there were question marks between each packet, apparently indicating a missing "ACK". And when setting "stop=False" the data was so bad, the decoder was unable to properly parse it.

[gojimmypi]

oh, and btw - hooking the sameI2C hardware (in my case, the RTC1307) to the RPi, the scope showed perfectly formatted data, no question marks between packets.

[deshipu]

That's expected with stop=False. It's only useful for doing "multiple start" things that some devices require. It's not for sending normal data.

[dpgeorge]

Now supported.

[mstarostik]

Well, but if I read the code correctly, with an I2C_STRETCH_LIMIT of 255 (~255 µs?), this support might be too limited.
The SHT21 temperature/humidity sensor when run in master hold mode will wait for the measurement to complete while pulling SCL low. We're talking in the order of up to 85 _milli_s here. The sensor supports a polling mode as an alternative to that, but master hold needs somewhat less code, especially when the sensor is the only slave anyway.

[dpgeorge]

Well, but if I read the code correctly, with an I2C_STRETCH_LIMIT of 255 (~255 µs?), this support might be too limited.

Yes it's currently fixed at 255us timeout. The I2C specs say that there is no limit to the stretching and probably we should increase the timet to a much larger value, order 100ms.

[deshipu]

I see two practical issues with increasing the timeout by default:

The first is that clock stretching will not only block the whole i2c bus, but also -- the way it is implemented in the software i2c driver currently -- the whole microcontroller. This might be tolerable if the only thing the microcontroller does is talking to the sensor. For a microcontroller that is supposed to also handle WiFi, like the ESP8266, react to user input, keep refreshing a display -- or do anything an actual practical device would need to do -- this is a catastrophe.

The second is the watchdog timer, which will simply restart the microcontroller, making it very hard to even figure out what is going on.

Currently, if for some reason the clock line is shorted to ground (for instance, by the WeMoS D1 Mini Motor Shield, which does that on any communication error, for some reason), it will take the microcontroller around 4µs to get to the point where it expects an ACK or NAK from the slave, and interrupt the operation. With the timeout set to 100ms by default, that time will become almost 1s. For one second the microcontroller will spin in place, waiting for the timeout on every clock cycle, until it gets to the point where it figures out that something is wrong.

I have two ideas about how to alleviate those problems, and still allow for a longer timeout.

The first is to make the timeout configurable. That's what NodeMCU does. The user can then set that timeout to the value that can be reasonably expected in the particular setup without causing trouble.

The second is to make the driver error out as soon as the timeout is reached. The subsequent data is not going to make any sense anyways.

[dpgeorge]

The second is to make the driver error out as soon as the timeout is reached. The subsequent data is not going to make any sense anyways.

I think this is a correct thing to do, because there is an obvious error on the bus. Exceptions can then be caught by the user if they need to handle them.

I also think the timeout should be configurable.

[dhylands]

smbus specifies a maximum clock stretch of 25-35 msec, after which it assumes that the bus is hung and needs to be reset. So any i2c devices which are compatible with smbus (which is most newer devices) will be working within those parameters.

[dpgeorge]

smbus specifies a maximum clock stretch of 25-35 msec

SMBus is apparently a strict sub-set of the I2C protocol, so not all I2C devices conform to its spec. In particular @mstarostik implies that the SHT21 doesn't conform.

[deshipu]

This is fixed by #2420. The #2550 adds an enhancement. Should we close this?

[pfalcon]

Everything good on docs side?

[deshipu]

Well, #2420 doesn't change anything at the user-visible level, and the lack of clock stretching wasn't documented. #2550 adds a non-standard parameter to the i2c.init() method, so it requires further discussion before it's accepted, and then I will add docs to it (I don't want to make changes in multiple places, easy to forget something that way).

[pfalcon]

Ok, this ticket was originally yours, so you should be able to close it. If not, let me know.

[deshipu]

Sure, just wanted to make sure we are finished here.