Double I2C read in one transaction skips a clock pulse (#5528) (based on I2C reduce iRAM)

[TD-er]

See #5528 and the more elaborate description
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 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 "valey". See my comment in the code.

Have not tested it myself, this is just the long awaited PR.

N.B. This PR was suggested by @Jason2866 here: #6579 (comment)
It is based on this PR by @earlephilhower : #6326
This PR is almost the same as #6579 but now following the changes made in #6326

[Jason2866]

A quick test was successful. Tasmota does run with your I2C driver

[TD-er]

Does it add much to the iram usage?

[Jason2866]

Still nice iram usage. You can try if you want, use in platformio.ini as platform:
platform = https://github.com/Jason2866/platform-espressif8266/tree/newfeature/stage

[TD-er]

Apparently it does fix something as reported here: #6497 (comment)

[d-a-v]

Confirmation from #6497 (comment)

@TD-er patch is huge and we cannot read it.

I propose to switch it to allman-style like in

Arduino/tests/restyle.sh

Line 12 in 122e870

all="libraries/ESP8266mDNS"

Just modify it like this

all="
libraries/ESP8266mDNS
cores/esp8266/core_esp8266_si2c.cpp
"

run the script and also add it to your commit

[TD-er]

@d-a-v I guess it has become unreadable because I had based it on an existing PR which later was updated.
I did take care not to modify any other line I did not touch.

[TD-er]

I did rebase it and removed the time stretch functions I added and @earlephilhower also implemented in his PR.
Apparently still need to run the formatting script you mentioned.
At least the last 2 commits should give you an indication of what I changed.

Edit:
They should look similar to my initial commit for this PR: 1c63358

[d-a-v]

I fixed the conflict

[Jason2866]

@TD-er Updated platform = https://github.com/Jason2866/platform-espressif8266/tree/newfeature/stage
to latest changes

[TD-er]

Should I rebase this PR on the master branch and force push it?

[d-a-v]

No,
Here's what I just tried and works.

• checkout your local master copy and make it up to date
• checkout back your PR branch
• update by hand test/restyle.sh, add libraries/Wire after cores/esp8266/core_esp8266_si2c.cpp
• run the restyle script
• commit the changes
• now the magic command: cd <core-rootdir>; git merge-octopus master
• your PR is now clean like below, just push it as is to your branch

diff --git a/cores/esp8266/core_esp8266_si2c.cpp b/cores/esp8266/core_esp8266_si2c.cpp
index 7200f631..2ad2016d 100644
--- a/cores/esp8266/core_esp8266_si2c.cpp
+++ b/cores/esp8266/core_esp8266_si2c.cpp
@@ -128,7 +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);
     void setClockStretchLimit(uint32_t limit);
@@ -386,13 +387,17 @@ unsigned char Twi::writeTo(unsigned char address, unsigned char * buf, unsigned
     {
         write_stop();
     }
+    else
+    {
+        twi_scl_valley();
+        twi_wait_clockStretchLimit();
+        // TD-er: Also twi_delay here?
+        // delay(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;
[...]

(I just found out this octopus thing, after searching why my git diff -u looked so clean and git merge so garbaged)

[TD-er]

Well I tried it, but it doesn't look like it is making it more readable.
I don't know what this octopus merge should do, but it was done with it even faster than a normal git status executes.
So I don't think it did any magic here.

[earlephilhower]

At stages like this, @TD-er, I usually give up on git and restart fresh from a new branch. :(

Your changes end about here. There's now a method "WAIT_CLOCK_STRETCH" which replaces your twi_wait_clockStretchLimit, and the rest of the changes look relatively simple to adjust to the class: readFrom/writeTo methods:

1c63358

[TD-er]

OK, so should I make a new PR for it, or do you cut'n-paste it?

[earlephilhower]

It's your code, so suggest you do the new PR/branch. I'm not trying to take the git blame for it. :)

I'd start a new PR, make the 3 changes in the new branch, link it as "supersedes #6592", close this PR, and go on from there.

If that's a problem, I can make a PR from your changes, too. Just let me know.

[TD-er]

Superseded by #6654