Questions regarding begin() for ESP and speed tests
wolfbert opened this issue · 12 comments
Hi Rob, first of all, great work with this library. I'm impressed. I've tried it out with AT24C32 and it seems to work fine. Just two questions.
- What is the significance of the test in begin for ESP? There are 42 pins on the ESP32, which would be an upper limit, but what would be the significance of 0xFF?
- I can't quite understand the output of the performance test at the end of the example. First, the code still works with a fixed
delay()of 2 ms (datasheet claims it should be 10 ms). Below are the figures for I2C_WRITEDELAY of 5000 on ESP8266:
20:04:27.692 -> TEST: timing writeByte() TIME: 523
20:04:27.692 -> TEST: timing writeBlock(50) TIME: 9606
20:04:27.726 -> TEST: timing readByte() TIME: 2090
20:04:27.726 -> TEST: timing readBlock(50) TIME: 7141
20:04:27.726 -> TOTALS: 19360
20:04:27.726 ->
20:04:27.726 -> TEST: timing writeByte() TIME: 526
20:04:27.726 -> TEST: timing writeBlock(50) TIME: 8205
20:04:27.762 -> TEST: timing readByte() TIME: 672
20:04:27.762 -> TEST: timing readBlock(50) TIME: 6956
20:04:27.762 -> TOTALS: 16359
Why would the tests be faster with a 5 ms delay once before the test, in particular readByte()? Even an I2C_WRITEDELAY of 10000 doesn't change much. Could that be caused by endTransmission in waitEEReady running into a timeout, so that it actually takes longer than a fixed delay()?
Thanks for the issue.
I will try to answer the questions coming days (already late here)
Answer 1.
The test for 255 is to check if the I2C default pins are overridden. In the ESP begin they are default 255.
(Need to check details)
If on an ESP begin() is called without parameters or with begin(255, 255) default pins are used.
This construct is inherited from another lib, but never explained in the readme.md, so documentation needs rework.
In some of my other libraries the SCA and SCL pins are both default 255, which is not the case here as it would conflict with the parameterless begin().
Answer 2.
Note the library keeps track of the last time written in _lastWrite
If more than I2C_WRITEDELAY (5) milliseconds have passed, _waitEEReady() will return immediately,
Therefore it will not go into the loop (line 492-498) testing availability and doing a yield() call.
The latter call can be expensive in time, depending on other tasks on the ESP.
Furthermore, if the while condition is lets say 4.9 millis, it will do another loop, going "a lot" over the 5 millis.
So the functions that would normally block on _waitEEReady() will not do so due to the delay(5) in the performance test code, resulting in a slightly faster time.
never measured how much the actual waiting time is, or the duration of one loop - depends on I2C clockspeed at least
(to be continued tomorrow)
Hi Rob, first of all, great work with this library. I'm impressed.
Thanks, appreciated
Wow, that was quick, thanks.
As for answer 2, I figured something like that, and I can't think of a better solution. It comes down to a delay and/or ACK polling. For what it's worth, here's a small optimization you might try (4.6% speedup for the readBlock test):
- yield either inside the loop in readBlock() (line 189), or in _readBlock() one level down, but not both (same goes for updateBlock())
- if in _readBlock(), then not after read() (line 479) inside the loop, as read() only copies from buffer; instead, yield outside the loop
As for answer 1, hadn't thought of that. That said, I had a look at Wire (for ESP8266), and from what I can see, sda and scl are initialized to -1 in the constructor, but are set to default values SDA and SCL in begin(). If invalid pin numbers (including -1) are passed to Wire.begin(scl, sda), they are not checked and sent straight to twi. The check in I2C_eeprom::begin won't hurt, though.
Thanks again, sometimes my curiosity gets the better of me - my (professional) programming days are long gone ...
One more idea... A delay is only required after writing the EEPROM. So, if we introduce a variable bool _lastOpWasWrite we can skip the calls to _waitEEReady() following a read. May not matter much, though.
Anyway, the comparatively high time for readByte() in the test above is due to the fact that it follows immediately on a write operation. Subsequent readBytes are as fast as expected.
If I recall correctly.some EEPROMs just are "offline" after a write for 5 millis. So a read without the wait would fail. Therefore it also checks the lastWrrite in read. Please note that reads will not set the EEPROMs offline. So subsequent reads will be fast.
I need to check your optimization proposal later.
.
For what it's worth, here's a small optimization you might try (4.6% speedup for the readBlock test):
- yield either inside the loop in readBlock() (line 189), or in _readBlock() one level down, but not both (same goes for updateBlock())
- if in _readBlock(), then not after read() (line 479) inside the loop, as read() only copies from buffer; instead, yield outside the loop
yes I agree it is overkill to call it in the loop line 479 as that is indeed a copy action.
Moving it outside the loop would be better. (I'll add it for the next release)
uint8_t cnt = 0;
yield();
while (cnt < readBytes)
{
buffer[cnt++] = _wire->read();
}
return readBytes;line 189 and line 228 can indeed be without.
Will prepare a new branch asap
@wolfbert
Created a develop branch - https://github.com/RobTillaart/I2C_EEPROM/tree/develop
Could you verify it works?
- removed 2x yield()
- moved 1x yield()
- added a few lines around begin() to make usage a little bit more explicit.
Also extracted the history in a separate file and added RP2040 to the build-CI
Could you verify it works?
Done, had a look at the code and tried it out, works for me.
OK then I will merge it after my current task
Thanks for testing!
@wolfbert
Merged the develop branch and created 1.6.2
Thanks for the issue and for testing.
If there are other issues / questions just open a new issue !