Implement SERCOM/Wire I2C timeout detection

[jrowberg]

This small set of modifications adds basic I2C timeout detection to Wire and SERCOM on the SAMD platform.

The blocking nature of the standard Wire library on most Arduino platforms has been a huge headache for many people, myself included. The challenge is that the Wire library itself isn't to blame, but rather the underlying layer of platform-specific code (SERCOM in the case of the SAMD platform, twi for AVR, etc.). You can't rely on the Stream class timeout functionality since the hardware layer is separate. To solve this, both SERCOM and Wire require modifications.

This implementation works as follows:

• Timeouts are detected in the SERCOM layer with a flexible initTimeout() and testTimeout() method pair. initTimeout() is called immediately before a previously blocking loop starts, and testTimeout() is called repeatedly as part of the continuing condition for the loop.
• If a timeout occurs, the I2C bus is reset and re-initialized at the previously configured clock rate.
• The timeout init/test functionality is accomplished via millis(), which has certain drawbacks (e.g. interrupts required), but given the CPU execution state when Wire/SERCOM blocks during an I2C transaction, these drawbacks are irrelevant. Switching to a simpler implementation like a volatile int accumulator is not hard, but would be less precise.
• The modified Wire library exposes the "passthrough" methods setTimeout(...) and didTimeout() for applying a new timeout value and checking whether a timeout occurred at the SERCOM layer, since SERCOM familiarity is not expected for most users.
• The default timeout value is set to 1000 ms (1 second). A value of 0 disables timeout detection (current behavior, before this fix). Whether this is a safe value or not is up to others; in my experience, it is far longer than anyone should ever have to wait assuming a properly behaving I2C peripheral. However, I readily admit that some devices may implement ridiculous clock stretching or something else that make 1000 ms unsafe. I chose this value since it's also the default for the Stream class.
• The beginTransmission() method has a new return value of 4 to indicate that a timeout occurred. Although this value was previously noted in the comments as "Other error", I found no instances of this in the code.
• Regardless of the previous point, the recommended method for checking for a timeout is to use Wire.didTimeout() right after any relevant calls.

As you can see, this implementation doesn't change Wire (or SERCOM) into a true non-blocking library in the event-driven sense, but it does provide a much-needed easy way to break out of "frozen I2C jail" that the current official core/library doesn't have.

[jrowberg]

Following up after: @facchinm or others, what additional steps are necessary for this to be considered for merging? Or is there anything in it which inherently prevents it from consideration?

I'm happy to do any additions/removals/modifications/testing/porting required (e.g. the same thing in the AVR core) if it might help get this one in. The blocking hang issue continues to plague people using the current official Wire implementation with less-than-perfect I2C slaves.

[Rocketct]

hi @jrowberg i have tested the PR it works as expected, make two test:

• make a shorts between SCL and VCC: in this case the timeout is respected and follow with the routine implemented on the sketch, if I remove the short circuit the I2C start works fine again;

• make a short between SDA and VCC : in this case the board stop to execute the sketch and even if i remove the short circuit the board still do nothing

[jrowberg]

@Rocketct Thanks for the review. To confirm, are you saying that one test works (SCL-VCC short), but another test does not work (SDA-VCC)?

[Rocketct]

@jrowberg yes exactly

[jrowberg]

@Rocketct That's interesting. There are no blocking loops without timeout tests in the SERCOM I2C code anymore, except for SYNCBUSY bit waits which should complete almost instantly. I wonder if shorting SDA to VCC causes some other system to fail, like connecting VCC/GND rails. Since I2C is typically open-drain with signals either pulled high or driven low, actually driving either line high would seem to be pretty far out of spec. (Though, of course, if this can be detected and worked around, that's even better.)

Were you using a custom sketch to test this, or a standard example sketch from some library? I'd like to reproduce if I can.

[Rocketct]

yes i run on a mkr1010 the example i2cdetect of the same library, available on library manager.

when yo try scl-vcc you'll see that instead of report values it start to print 'XX' in the matrix that example shows on the serial monitor

when test the SDA-VCC the SErial monitor stop to print and only an hard reset allow you to make it run again, pay attention there is another issue over this maybe could help #476

[CLAassistant]

All committers have signed the CLA.

[Xtreemtec]

What is the holdup on this? It really would be useful to have this as we also have projects were a i2c device ( SFP with eeprom inside ) can be unplugged and so the device is gone from the bus. Calling the i2c eeprom will lock up the sketch as it will wait till infinity for the eeprom to respond. Needs Timeout!!

[ghost]

@Xtreemtec, AVR has only recently (as in last year or so) had a wire timeout pushed through, that was many years in the making and I think a little niche in the end. What @jrowberg has done here is a good approach for the samd. I use a slightly modified version in all my projects now and it has been very reliable (wire_timeout). And as you probably are already aware, it's also useful to modify any dependent libraries i.e. display/sensor etc, to handle and manage the timeouts when they occur to take full advantage of this feature.

Feedback, preferably from actual testing, is always welcome to help things along. And if you find something that works well for you let us know.

[NicholasNelsonwood]

I am just commenting to say that this feature would be greatly appreciated for my application. I got very excited when I saw the documentation saying that Wire now had timeout functionality. It was only after a bunch of digging that I was able to figure out it had only been implement for AVR (arduino/ArduinoCore-avr#107) and not SAMD.

It would be greatly appreciated for the capability to exist on all architectures. So thanks to @jrowberg for doing this work.

I will likely test out this branch on Monday to see if the solution works for me.

[evs2023]

@jrowberg I have looked for a solution on the wire.h and SERCOM.h libraries for a long time due to hang ups related to hardware and not always possible to make it 100% robust. So happy to find finally this. I have tested it on a following hardware: Nano 33 IoT connected via level shifter to IO expander mcp23017, 2 pieces of ADS1115 analog inputs, EEPROM MCP24C01. All pull ups have been installed and checked for the right values depending on the side of the level shifter and taking in to account the Nano 33 IoT installed on board pull ups. The signals have been checked with an oscilloscope and they look nice.

With the original libraries the sketch was hanging randomly, sometimes after 1 minute, other times after 5 minutes. If the communication to the mcp23017 is switched off in the software, but not in the hardware, it improved stability. If the software for the communication with the EEPROM is removed, there is no crash. With only the mcp23017 it is crashing regularly. (I am aware of the hardware bug of the mcp23017 for pins PAB7 and PAA7, which can only be used as outputs because they will interfere with the I2C communication if used as inputs (see documentation of microchip in the datasheet from 2022).

So I have installed the SERCOM and WIRE libraries that you have modified Jeff and checked if the timeouts are working, and they do. I put the timeout at 1000ms - and see the behaviour when there is a delay because of a bus problem on the hardware described above. The set up is more stable now, but it still hangs up.

I looked through the code and I can not understand how it can hang now, except for that one instance where you did not include the timeout in the while loop.

Would it be possible to also implement it for this while loop?

Thanks for your efforts - I am looking forward to finally get rid of this problem with the I2C communication. Any software should overcome a hardware issue, whatever the cause is. - looking forward to your relpy