From 36c369bb8fc8c76945203018e97db70387da6df4 Mon Sep 17 00:00:00 2001
From: Gijs Noorlander <gijs.noorlander@gmail.com>
Date: Sun, 27 Oct 2019 04:59:19 +0100
Subject: [PATCH] Double I2C read in one transaction skips a clock pulse
 (#5528) (#6654)

* Double I2C read in one transaction skips a clock pulse (#5528)

See https://github.com/esp8266/Arduino/issues/5528  and the more elaborate [description](https://github.com/maarten-pennings/I2C-tool/blob/master/I2Ctest8266/README.md#how-to-fix)
where @maarten-pennings did all the hard work, but as far as I could see, no PR was made.

I also noticed some code duplication, which I moved to separate functions.

According to [this documentation on I2C clock stretching](https://www.i2c-bus.org/clock-stretching/) it is not allowed to have some slave keep the clock low during transmission of a byte, only after an ack.
So that's why it is not done in the while loop.
But I wondered if there should be an extra delay between the sequence of pulses and the extra added clock "valley". See my comment in the code.

Fixes #5528

* [I2C] Restore clock stretch functionality during transfer

As requested here: https://github.com/esp8266/Arduino/pull/6654/files/8357a8c644244096cce1df30acd660c35d24a0e4#r339310509

* [I2C] Move duplicated clock stretch call to twi_scl_valley function
---
 cores/esp8266/core_esp8266_si2c.cpp | 32 +++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/cores/esp8266/core_esp8266_si2c.cpp b/cores/esp8266/core_esp8266_si2c.cpp
index 7200f6315..281b19680 100644
--- a/cores/esp8266/core_esp8266_si2c.cpp
+++ b/cores/esp8266/core_esp8266_si2c.cpp
@@ -128,6 +128,8 @@ private:
         }
     }
 
+    // Generate a clock "valley" (at the end of a segment, just before a repeated start)
+    void twi_scl_valley(void);
 
 public:
     void setClock(unsigned int freq);
@@ -386,13 +388,16 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned
     {
         write_stop();
     }
+    else
+    {
+        twi_scl_valley();
+        // TD-er: Also busywait(twi_dcount) here?
+        // busywait(twi_dcount);
+    }
     i = 0;
     while (!SDA_READ() && (i++) < 10)
     {
-        SCL_LOW();
-        busywait(twi_dcount);
-        SCL_HIGH();
-        WAIT_CLOCK_STRETCH();
+        twi_scl_valley();
         busywait(twi_dcount);
     }
     return 0;
@@ -422,13 +427,16 @@ unsigned char Twi::readFrom(unsigned char address, unsigned char* buf, unsigned
     {
         write_stop();
     }
+    else
+    {
+        twi_scl_valley();
+        // TD-er: Also busywait(twi_dcount) here?
+        // busywait(twi_dcount);
+    }
     i = 0;
     while (!SDA_READ() && (i++) < 10)
     {
-        SCL_LOW();
-        busywait(twi_dcount);
-        SCL_HIGH();
-        WAIT_CLOCK_STRETCH();
+        twi_scl_valley();
         busywait(twi_dcount);
     }
     return 0;
@@ -649,6 +657,14 @@ void ICACHE_RAM_ATTR Twi::onTwipEvent(uint8_t status)
     }
 }
 
+void Twi::twi_scl_valley(void)
+{
+    SCL_LOW();
+    busywait(twi_dcount);
+    SCL_HIGH();
+    WAIT_CLOCK_STRETCH();
+}
+
 void ICACHE_RAM_ATTR Twi::onTimer(void *unused)
 {
     (void)unused;