eeprom_update_* semantics differ between AVR and ARM

[pelrun]

Coming from avr-libc, the eepromupdate* functions check the old value and only overwrite it if the new value is different. https://www.nongnu.org/avr-libc/user-manual/group__avr__eeprom.html

Where we've reimplemented that API for the different ARM variants (stm32, atsam, teensy) that behaviour has been lost, and the functions are copy-and-pasted from eepromwrite*. (Which is a code smell in and of itself.)

This is a bigger deal on the ARM platforms as they emulate eeprom with flash, and the existing implementation doesn't do proper wear levelling.

We also have duplicate checks elsewhere that should just be pushed down into eepromupdate*.

[yiancar]

The reason people don't bother performing an update function is whats called read-disturbance. Essentially reading a single block continuously will also cause wear (which is what happens with or without wear leveling in eeprom emulation)

[pelrun]

What? No! Read disturbance is only a problem in NAND flash. Microcontrollers use NOR, because you're executing code out of there, millions of reads every second! The number of flash reads that the eeprom update process will do is infinitesimal compared to that.

[pelrun]

QMK is currently calling the update functions, but wrapping them in redundant value checks. Either do the check manually and call write, or don't and call update, but don't check then call update!

[yiancar]

Ups yes you are correct! We could modify the update function for eeprom emulation to read compare and then write if needed then

[pelrun]

That's exactly the behaviour the update functions should have.

And we can be smarter than a simple equality check - since we can set bits to zero for "free", when (~old & new)==0 the byte can be flashed directly over the top of the existing one, without having to move it, and reducing the frequency of page erases even further.

[awkannan]

Yiancar and I have discussed this, and I think you were involved in the discussion too pelrun. We want to replace the thin layer over flash implementation with one using virtual addressing and multiple pages a la the stm32 datasheets.

That will take time though, and the thin layer implementation was very broken before, so the current code is pretty much a stopgap.

That does mean that it will break people's eeproms when we change it to the new version, but that's ok.

Even with the emulation, this update behavior is still necessary. I'm in favor of this update change, I'll add it in my next PR.

[yiancar]

@pelrun maybe do you have the time to submit a pr?

[awkannan]

Fixed now

[pelrun]

Still needs eeprom_update_byte and eeprom_update_word to be fixed. And also the functions in the atsam and teensy versions of that file. And it should really leverage the existing functions instead of duplicating the content, e.g.

void eeprom_update_dword(uint32_t *Address, uint32_t Value) {
    if(Value != eeprom_read_dword(Address)) {
      eeprom_write_dword(Address, Value);
    }
}

[pelrun]

I'll get to all of those, I just needed to keep this issue around to make sure I don't forget.

[drashna]

@pelrun Ideally, this function should be changed: https://github.com/qmk/qmk_firmware/blob/master/tmk_core/common/chibios/eeprom_stm32.c#L65-L108

The function there is called by all of the update functions, and given how it works, would make it more efficient, I think.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs. For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

[tzarc]

Closing due to inactivity.