From 63518a0cafa82a8edf10f4b3332d8529c27dab25 Mon Sep 17 00:00:00 2001 From: Gijs Noorlander Date: Sat, 19 Oct 2019 17:13:55 +0200 Subject: [PATCH 1/3] 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 --- cores/esp8266/core_esp8266_si2c.cpp | 33 ++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/cores/esp8266/core_esp8266_si2c.cpp b/cores/esp8266/core_esp8266_si2c.cpp index 7200f63154..ec47dc7538 100644 --- a/cores/esp8266/core_esp8266_si2c.cpp +++ b/cores/esp8266/core_esp8266_si2c.cpp @@ -128,6 +128,8 @@ class Twi } } + // 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,17 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned { write_stop(); } + else + { + twi_scl_valley(); + WAIT_CLOCK_STRETCH(); + // 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 +428,17 @@ unsigned char Twi::readFrom(unsigned char address, unsigned char* buf, unsigned { write_stop(); } + else + { + twi_scl_valley(); + WAIT_CLOCK_STRETCH(); + // 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 +659,13 @@ void ICACHE_RAM_ATTR Twi::onTwipEvent(uint8_t status) } } +void Twi::twi_scl_valley(void) +{ + SCL_LOW(); + busywait(twi_dcount); + SCL_HIGH(); +} + void ICACHE_RAM_ATTR Twi::onTimer(void *unused) { (void)unused; From aac129518b9c08b924e8ea7c01229646f8812837 Mon Sep 17 00:00:00 2001 From: Gijs Noorlander Date: Sat, 26 Oct 2019 23:16:14 +0200 Subject: [PATCH 2/3] [I2C] Restore clock stretch functionality during transfer As requested here: https://github.com/esp8266/Arduino/pull/6654/files/8357a8c644244096cce1df30acd660c35d24a0e4#r339310509 --- cores/esp8266/core_esp8266_si2c.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cores/esp8266/core_esp8266_si2c.cpp b/cores/esp8266/core_esp8266_si2c.cpp index ec47dc7538..d4e4be6594 100644 --- a/cores/esp8266/core_esp8266_si2c.cpp +++ b/cores/esp8266/core_esp8266_si2c.cpp @@ -399,6 +399,7 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned while (!SDA_READ() && (i++) < 10) { twi_scl_valley(); + WAIT_CLOCK_STRETCH(); busywait(twi_dcount); } return 0; @@ -439,6 +440,7 @@ unsigned char Twi::readFrom(unsigned char address, unsigned char* buf, unsigned while (!SDA_READ() && (i++) < 10) { twi_scl_valley(); + WAIT_CLOCK_STRETCH(); busywait(twi_dcount); } return 0; From 702361ce72692b6f401a66306d22de7bcebb48f1 Mon Sep 17 00:00:00 2001 From: Gijs Noorlander Date: Sun, 27 Oct 2019 00:39:28 +0200 Subject: [PATCH 3/3] [I2C] Move duplicated clock stretch call to twi_scl_valley function --- cores/esp8266/core_esp8266_si2c.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cores/esp8266/core_esp8266_si2c.cpp b/cores/esp8266/core_esp8266_si2c.cpp index d4e4be6594..281b19680f 100644 --- a/cores/esp8266/core_esp8266_si2c.cpp +++ b/cores/esp8266/core_esp8266_si2c.cpp @@ -391,7 +391,6 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned else { twi_scl_valley(); - WAIT_CLOCK_STRETCH(); // TD-er: Also busywait(twi_dcount) here? // busywait(twi_dcount); } @@ -399,7 +398,6 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned while (!SDA_READ() && (i++) < 10) { twi_scl_valley(); - WAIT_CLOCK_STRETCH(); busywait(twi_dcount); } return 0; @@ -432,7 +430,6 @@ unsigned char Twi::readFrom(unsigned char address, unsigned char* buf, unsigned else { twi_scl_valley(); - WAIT_CLOCK_STRETCH(); // TD-er: Also busywait(twi_dcount) here? // busywait(twi_dcount); } @@ -440,7 +437,6 @@ unsigned char Twi::readFrom(unsigned char address, unsigned char* buf, unsigned while (!SDA_READ() && (i++) < 10) { twi_scl_valley(); - WAIT_CLOCK_STRETCH(); busywait(twi_dcount); } return 0; @@ -666,6 +662,7 @@ void Twi::twi_scl_valley(void) SCL_LOW(); busywait(twi_dcount); SCL_HIGH(); + WAIT_CLOCK_STRETCH(); } void ICACHE_RAM_ATTR Twi::onTimer(void *unused)