Twi::status() is incorrect, locks SDA low if read

[Tech-TX]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [ESP-12]
• Core Version: [latest git 10/19/2019]
• Development Env: [Arduino IDE]
• Operating System: [Windows]

Settings in IDE

• Module: [Wemos D1 mini r2]
• Flash Mode: [qio]
• Flash Size: [4MB]
• lwip Variant: [v2 Lower Memory]
• Reset Method: [ck]
• Flash Frequency: [40Mhz]
• CPU Frequency: [80Mhz]
• Upload Using: [SERIAL]
• Upload Speed: [921600] (serial upload only)

Problem Description

Doing Wire.status() from inside a sketch returns zero, but leaves SDA low, killing the bus.

Short example sketch, with D4 configured as OUTPUT_OPEN_DRAIN and optionally shorted to SDA to simulate a 'stuck slave' during testing. core_esp8266_si2c.cpp was modified to support the 'stuck slave' test, but the error occurs with or without any changes to core_esp8266_si2c.cpp

/* brief test to simulate a stuck slave using D4 OUTPUT_OPEN_DRAIN
   shorted to SDA, do one write to show the bus is working, then force
   D4 low and do Wire.status().
*/

#include <Wire.h>

void setup() {
  Serial.begin(115200);
  pinMode(D4, OUTPUT_OPEN_DRAIN);
}

void loop() {
  GPOS = (1 << 2); // D4 connected to SDA to simulate a 'stuck slave', starts off not stuck
  delayMicroseconds(5);
  Wire.begin();
  Wire.beginTransmission(0x76); // arbitrary address for test to show the bus works
  Wire.endTransmission();
  delayMicroseconds(5);
  GPOC = (1 << 2);  // simulate stuck slave, cleared inside Twi::status() bus recovery sequence
  delayMicroseconds(5);
  int err = Wire.status();
  Serial.print("Wire.status return = ");
  Serial.println(err);
  err = digitalRead(SDA);
  Serial.print("state of SDA = ");
  Serial.println(err);
  delay(100);
}

Debug Messages (none)

Serial output shows:

Wire.status return = 0
state of SDA = 0

Clean return from Wire.status() as it kills SDA just before exit.

The proposed pull request adds a STOP after the START at the end of Twi::status(), releasing the bus. Added a bit of extra error-state checking by first checking for clock stretch before the initial SCL test, as the slaves and bus are in an unknown state when we first start Twi::status().
Additionally, there's an error in one of the clock stretch functions: you're comparing t (unsigned int) to twi_clockStretchLimit (uint32_t), surprising that it doesn't throw a warning.

Here are 2 logic analyzer captures of the fixed library, the first one a simple run ending with Wire.status(), the second with D4 shorted to SDA, causing a simulated 'stuck slave'.

The serial output with the fixed code shows:

Wire.status return = 0
state of SDA = 1

Here's the pull request:
https://github.com/Tech-TX/Arduino/pull/1

The small change I made to core_esp8266_si2c.cpp to test the 'stuck slave' is not included in the pull request. Inside the clock recovery loop in Twi::status() I set the D4 pin HIGH after several clocks so that it looked like a successfully recovered stuck slave.

    int clockCount = 20;
    while (SDA_READ() == 0 && clockCount-- > 0) { // if SDA low, read the bits slaves have to sent to a max
        twi_read_bit();
        if (SCL_READ() == 0) {
          return I2C_SCL_HELD_LOW_AFTER_READ; // I2C bus error. SCL held low beyond slave clock stretch time
        }
		if (clockCount == 16) {
			  GPOS = (1 << 2); // release our 'stuck slave' test
		}
    }

[TD-er]

Does this relate to my PR? #6654
If so, could this one fix the issues we try to tackle in #6654 (and thus making that one obsolete) ?

[Tech-TX]

You and I are looking at different issues. Simply looking at the code, the twi_status (now Twi::status) has never worked, as the last instruction before it exited was a write_start() which leaves SDA low, blocking the bus. There are still issues in clock handling as I shorted SCL to ground last night and saw it wiggle SDA with the 0x76 address in the example above. It should have detected that as a collision and aborted the write.

I'm going to remove my request as I'm having troubles getting compiles to work again, and can't recompile what I posted yesterday.