A couple enhancements for your review... #69
Conversation
|
Thanks for thi PR. Interested in the extended version, preferably as a derived class I think, as that gives people choice which class to use. |
|
Relates to #68 |
|
Got the review started.
OK I see the application and the implementation is quite straightforward, however the implementation is missing command line support. Furthermore the default value of the flag EN_AUTO_WRITE_PROTECT in the library must be 0 (false) in code: I2C_EEPROM.h // set the flag EN_AUTO_WRITE_PROTECT to 1 to enable the Write Control at compile time
// used if the write_protect pin is explicitly set in the begin() function.
// the flag can be set as command line option.
#ifndef EN_AUTO_WRITE_PROTECT
#define EN_AUTO_WRITE_PROTECT 0
#endifSame command line argument would be true for the PER_BYTE_COMPARE flag. Remarks:
Point of attention: To be continued. |
Yes interested, do you have a link? Read a bit about the LIP, it works like an (separate) EEPROM of 128 bytes (memAddress 0..127). It would need these 5 functions in the derived class.
|
|
Hmm - a derived class might be more elegant than my current brute force modification of adding an additional bool to each function: bool IDPage (defaulted to =false). The separate IDPage i2c address is always the chosen EEPROM base address + 8 and set during the begin(...). And in the end:
But on the flip-side, I can manage both from one instantiation. Guess it's a coin-toss. BTW, it's an irreversible lock! You'll find it here: I2C_eeprom_wIDPage Something else to note: to avoid address wrapping, I added a couple validation tests in the function _pageBlock which would apply to setBlock, writeBlock and updateBlock functions |
perByteCompareFound some time to dive into the perByteCompare modus. In the code you allocate two buffers that are potentially large, larger than I2C buffer.
Note that you do not need the writeBuf as the data you want to write is already in buffer.
Furthermore the final remaining different bytes can be caught in the for loop too. uint16_t I2C_eeprom::updateBlock(const uint16_t memoryAddress, const uint8_t * buffer, const uint16_t length)
{
uint16_t addr = memoryAddress;
uint16_t len = length;
uint16_t rv = 0;
if (_perByteCompare) {
// Serial.println("Performing BYTE SIZE updates");
uint16_t writeCnt = 0;
// Read the original data block from the EEPROM.
uint8_t origBuf[length];
_ReadBlock(addr, origBuf, length);
uint16_t diffCount = 0;
uint16_t startDiffAddr = 0;
// Iterate over each byte to find differences.
for (uint16_t i = 0; i < len; ++i) {
if (buffer[i] != origBuf[i]) {
// Start buffering when the first difference is found.
// Save the starting address of the difference.
if (diffCount == 0) {
startDiffAddr = addr + i;
}
diffCount++; // count the differing bytes.
if (i < len - 1) continue; // <<<<<<<<<<<<<< falls through when i == len -1.
}
// If there was a difference and now it stops,
// write the buffered changes.
if (diffCount > 0) {
rv += diffCount;
_pageBlock(startDiffAddr, &buffer[startDiffAddr], diffCount, true);
diffCount = 0; // Reset difference count after writing.
writeCnt++;
}
}
// Serial.print("EEPROM Write cycles: ");
// Serial.println(writeCnt);
return rv;
}
// Serial.println("Performing BUFFERSIZE updates");
while (len > 0)
{
uint8_t buf[I2C_BUFFERSIZE];
uint8_t cnt = I2C_BUFFERSIZE;
if (cnt > len) cnt = len;
_ReadBlock(addr, buf, cnt);
if (memcmp(buffer, buf, cnt) != 0)
{
rv += cnt; // update rv to actual number of bytes written due to failed compare
_pageBlock(addr, buffer, cnt, true);
}
addr += cnt;
buffer += cnt;
len -= cnt;
}
return rv;
}Did you run tests like:
Can you post some figures? |
Thanks, If time permits I will dive into the code. |
|
Hi Rob - thank you for the code recommendations, I've updated my stuff to follow. As to your questions:
The byte size updates were randomly placed through the 256 byte test data, so in essence if there were 16 changes, there would be 16 individual write calls when in Byte Compare mode (conversely 2 updates in BUFFER mode). Certainly not surprising that the 128 modified bytes took an astounding 429934µs to complete. But hey, that's 128 individually addressed write cycles with an avg of 3358µs each... Note: the following output originates with the 256 byte test data being loaded, and then each test. The same base 256 bytes was reloaded between each test, but I eliminated the Serial.print() for those reloads to minimize the noise. I think the best way to quantify the usefulness of the Byte Size Compare would be: With regards to the I2C_BUFFERSIZE : Again, I appreciate your feedback. And I hope the additional information helpful. -Terry |
|
Thanks for these Insightful figures. The tests should be ran on an UNO (MEGA) too, to see what behavior it shows. My expectation is that the AVR will show a larger spread of times. The issue with buffers beyond 8 bit is known and so far it was not reported as a problem. However imho it is important (in fact a bug) for systems with more resources. So I will make a separate issue for it. |
|
@microfoundry I still need to test the compareByte code as I am not convinced of the added value. Question: can the library decide which mode is fastest? automatically? |
|
Had a though about a variation in strategy (not tested) which might write non-changed bytes. If you have time, please share your opinion. Reading is done in a number of blocks (Think length >> I2C_BUFFERSIZE) uint16_t I2C_eeprom::updateBlock(const uint16_t memoryAddress, const uint8_t * buffer, const uint16_t length)
{
uint16_t address = memoryAddress;
uint16_t len = length;
uint16_t bufSize = I2C_BUFFERSIZE;
uint16_t writeCount = 0;
if (_perByteCompare) {
// Serial.println("Performing BYTE SIZE updates");
while (len > 0) {
if (bufSize > len) bufSize = len; // final iteration.
// Read data block from the EEPROM.
uint8_t buf[I2C_BUFFERSIZE];
uint16_t start = 65535;
uint16_t end = 0;
_ReadBlock(address, buf, bufSize);
for (uint16_t i = 0; i < bufSize; ++i) {
// which part of the buffer should be written to EEPROM
if (buffer[i] != buf[i]) {
if (start == 65535) start = i;
end = i;
}
}
if (start != 65535) writeCount += _pageBlock(address + start, buffer + start, end - start + 1);
}
return writeCount ;
}
// Serial.println("Performing BUFFERSIZE updates");
while (len > 0)
{
uint8_t buf[I2C_BUFFERSIZE];
uint8_t bufSize = I2C_BUFFERSIZE;
if (bufSize > len) bufSize = len;
_ReadBlock(address, buf, bufSize);
if (memcmp(buffer, buf, bufSize) != 0) {
writeCount += bufSize; // update rv to actual number of bytes written due to failed compare
_pageBlock(address, buffer, bufSize, true);
}
address += bufSize;
buffer += bufSize;
len -= bufSize;
}
return writeCount;
}Expect that on boards with a smaller I2C buffer (UNO, MEGA) , it will work quite well. |
|
Another function to give your opinion on. Almost the above without the write, in fact I wrote this function first as knowing the number of different bytes (or ratio diffCount vs length) might help to determine the strategy. uint16_t I2C_eeprom::diffCount(const uint16_t memoryAddress, const uint8_t * buffer, const uint16_t length)
{
uint16_t address = memoryAddress;
uint16_t len = length;
uint16_t bufSize = I2C_BUFFERSIZE;
uint16_t count = 0;
while (len > 0) {
if (bufSize > len) bufSize = len; // final iteration.
uint8_t buf[I2C_BUFFERSIZE];
_ReadBlock(address, buf, bufSize);
for (uint16_t i = 0; i < bufSize; ++i) {
if (buffer[i] != buf[i]) count++;
}
address += bufSize;
buffer += bufSize;
len -= bufSize;
}
return count;
} |
|
Some theoretical thoughts GapsIf I want to store one byte in EE, I need to send a header (byte deviceAddress, 2 byte memoryAddress) plus the value. The transport efficiency is 25%. If the gap == 1 If the gap == 2 If the gap == 3 page factorAfter a write to a page the EE is off line for up to 5 milliseconds. So if 2 separate single byte writes are on the same page the EE will block for up to 10 ms. If 1 block of 8 bytes is written on the same page it will block for up to 5 ms. If the writes are done over 2 pages, they are roughly equally performant. Optimal block size.Within one page one can calculate the break even point of how long a gap may be before blocking becomes slower. Assume 100 KHz == 1 byte takes ~ 100 us 2 separate takes 2 x 4 x 100 us + 10 ms = 10800 us Assuming 400 KHz == 1 byte takes ~ 25 us If the write delay is lower e.g 3.5 ms the numbers will be lower of course. The conclusion seems to be that combining writes per page seems to be an important efficiency factor. Question remains how to translate these insights into code. Code
So far my theoretical thoughts, Does this makes sense? |
|
@microfoundry |
Greetings Rob - thank you for the great library. I've added a couple features you might be interested in:
I utilize Write Control and wanted it enabled by default, but made it a compile time define for those who don't. Implemented in the begin function and if there's no WC pin supplied, the #define is irrelevant.
Modified the updateBlock function have the choice to perform a byte level compare and multiple updates as needed, or stick with the I2C_BUFFERSIZE compare. Also a minor update to the original block of code that would return the full length of the buffer as number of bytes changed, even if it were just a buffer chunk (assuming the supplied buffer was larger than a I2C_BUFFERSIZE)
Best regards,
Terry Phillips
BTW - I'm also working with the ST M24256-D series of EEPROM which includes an additional Lockable Identification Page. It was super easy to extend the library for the functionality. I have a separate codebase for that if you're interested...