reduce speed of I2C master bus reset routine and release SDA

[jeremyherbert]

The I2C master reset has two major issues I have discovered:

1. The clock rate of the SCL line is at the maximum speed possible (many MHz), which is likely not a valid I2C data rate. In my case, the bus had too much capacitance and also the slave device could not use a clock rate that fast.
2. The SDA line is driven low during the reset sequence. This is not correct because a stuck/desynced slave will recognise this as an ACK and may continue to hold the bus. Instead, the SDA line should be released before the clocks.

This pull request sets the clock rate to 100kHz (the standard "slow" I2C clock rate) and releases the SDA line during the reset. At this stage, it is not possible to use the clock rate that the user has provided when configuring the I2C port, because this variable is lost when the I2C controller is reset on i2c.c line 582.

[negativekelvin]

Did you find it was necessary to slow it to 100Khz or just enough to overcome capacitive effect? I think signal should also be Start, 9 clocks, Start, Stop

[stickbreaker]

@jeremyherbert I agree with the speed reduction you propose(I stole it and added it to my pr #2496), but I think the reset code needs to handle interrupted reads.

[jeremyherbert]

@negativekelvin In this case, I needed to slow it down so the chip would be within spec (although at the max frequency, the bus capacitance did prevent the clocks from being generated). It worked at a fair bit above 100kHz, but I think this is out of spec. The other option is to store the user specified clock rate somewhere other than the I2C registers (like a static global) and then use that to compute the correct delay. To be honest, I think that the latter is a better solution, but I didn't want to submit a pull request with new global variables given that I do not know what the rules are around them in this codebase.

@stickbreaker I'm not sure I fully understand the issue with interrupted reads. I just followed the I2C spec which says that if SDA is stuck low, send 9 clocks. Obviously if SDA is stuck low because the ESP32 is holding it low as a GPIO (as is the case at the moment), the clocks won't do anything to release it ;) When performing a read from the slave, according to the I2C spec the NACK will cause the slave to release the line (if it still has the ability to) because it isn't possible to generate a stop without it being released:

(section 3.1.6)
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to abort the
transfer, or a repeated START condition to start a new transfer. 

If the NACK doesn't allow the master to generate the STOP, then the slave device is probably seriously broken and needs to be hard-reset anyway, so no amount of clocks will help.

[jeremyherbert]

@negativekelvin one more thing I realised; it's not possible to generate an initial start condition if the SDA line is stuck low ?

Also I didn't add the stop sequence (and by definition, there also needs to be a start before it, because SDA is high once released). I will add it and squash it into the old commit. I don't really mind which pull request gets added though, as long as the problem is fixed :)

[negativekelvin]

it's not possible to generate an initial start condition if the SDA line is stuck low ?

You are right but you can still generate the rising clock edge and it shouldn't hurt to try to generate the first start and the second start should work.

[stickbreaker]

@jeremyherbert in my experience with 24LCxx EEPROMS, The NAK will not stop the EEPROM from outputting the next byte when it sees the clock signal. I wrote the current I2C driver for the Arduino-ESP32 repo, during my testing of I2C bus recovery I discovered these I2C devices would hold the bus forever as long as a STOP or START was not issued. A NAK did not release the bus. When I reset the ESP32 while it was transacting a READ operation, a 9 bit manual stimulation of the SCL pin with SDA high would not cause the EEPROM to release SDA. The only way to get the 24LCxx EEPROMS to drop out of READ mode was to generate a valid STOP or START. This behavior also applied to the 9 axis MPU9250.

Before I figured out the need to synchronize with NAK or a 1 bit, I had to power cycle the EEPROM to reliably exit READ mode after a reboot. This only occurred because the data in my EEPROM had a big block of ZEROS, and the MPU9250 also had a block of ZEROS in its fifo/registers. I was testing with READS of 64k-1, so I usually reset the ESP32 while it was in the middle of one of these block reads.

Unless the data being read out happened to end at a 1bit, the existing reset code could not generate a STOP.

If the I2C hardware peripheral was initialized with SDA low, the peripheral would lockup in a BUS_BUSY state and never manipulate SDA and SCL.

@jeremyherbert
If the NACK doesn't allow the master to generate the STOP, then the slave device is probably seriously broken and needs to be hard-reset anyway, so no amount of clocks will help.

Yes, but the reset code needs to synchronize with the NAK to generate the STOP.

@jeremyherbert
The other option is to store the user specified clock rate somewhere other than the I2C registers (like a static global) and then use that to compute the correct delay.

You can regenerate the clock rate by directly reading scl_high_period.period and scl_low_period.period
I do it like this in arduino-esp32:

  uint32_t old_count = (i2c->dev->scl_low_period.period+i2c->dev->scl_high_period.period);
    if(old_count>0) {
        result = APB_CLK_FREQ / old_count;

dev-> is the direct offset into the peripheral register block.

This is the arduino-esp32 code to clear a stuck i2c bus checkLineState()

With my implementation of i2c, I have found it is not necessary to actually generate the STOP. It is enough just to get SDA and SCL HIGH then start the peripheral. The peripheral's first action will be to generate a START.

Chuck.

[jeremyherbert]

@stickbreaker what you are saying makes sense to me now, thank you. Although that behaviour does appear to violate the I2C standard (I think?) I'm sure there are devices out there which do not meet it.

As far as the scl_high_period, I did try to print those values in-place (it printed zero), but I believe that they are destroyed when the I2C statemachine is reset in i2c.c line 582 as I mentioned earlier. But I could be mistaken in this regard? Maybe there are multiple call sites for this function that I haven't found?

[stickbreaker]

@jeremyherbert I missed the line 582 significance.

I think your use of a fixed 100khz freq is satisfactory. I guess if someone designs a board with such weak pullups and high capacitance that it cannot support the minimum i2c freq, they can change a constant and re-compile.

Chuck.

[projectgus]

Thanks for contributing this fix, @jeremyherbert . This commit has been cherry-picked with some other I2C fixes and should be merged to master shortly.