sparkfun / OpenLCD

An open source serial LCD (HD44780) controller based on the ATmega328.
Other
32 stars 16 forks source link

Reduce the number of EEPROM writes with a 'Save Settings' command or EEPROM.update #16

Closed makinako closed 5 years ago

makinako commented 5 years ago

EEPROM.write is used all over the place to save settings on-the-fly (without an explicit 'save settings' command). Since sketches will mostly configure the SerLCD in setup(), this means that every time the sketch starts (at least) it is writing to the EEPROM a number of times for Backlight, Contrast, etc. It also dramatically slows the operation down.

Two solutions:

  1. Simply change all calls for EEPROM.write to EEPROM.update, which will check if the value has changed first
  2. Remove all the calls to EEPROM.write and have a specific 'Save Settings' command, so that all changes occur only in RAM unless persisted.

Option 2 is the more performant one, but Option 1 is probably preferred because of compatibility.

nseidle commented 5 years ago

I wasn't aware that EEPROM.update() existed! That's neat.

I disagree with your premise however. The number or EEPROM.writes is pretty limited to configuration changes. If the user issues the command to change UART speed, yes, we write to EEPROM and perhaps we could have saved a EEPROM write cycle using .update but I don't see us approaching the 100k datasheet limit or the 16M field limit of EEPROM ware.

Don't get me wrong, I'll write better code next time, but I don't see this as an issue.

There quite a few bounds checking writes like this one:

settingUARTSpeed = EEPROM.read(LOCATION_BAUD);
if (settingUARTSpeed > BAUD_1000000) //Check to see if the baud rate has ever been set
{
  settingUARTSpeed = DEFAULT_BAUD; //Reset UART to 9600 if there is no baud rate stored
  EEPROM.write(LOCATION_BAUD, settingUARTSpeed);
}

But these .writes will rarely get called (initial boot and rare edge cases). Is there a .write that you see as particularly bad?

makinako commented 5 years ago

Just to put it in context for how I use it, I am connecting to a SerLCD module via UART. At reset, because I can't read what the current settings are I configure the SerLCD module as if it was the first time, which includes contrast, backlight, etc. On top of this, our device has a sleep mode which turns off the LCD and backlight after a minute of inactivity. The backlight needs to be turned off separately here because the display on/off command doesn't affect it. Extending this beyond my own application, if someone else allowed these parameters to be adjusted via trimpots or similar, this could result on a rapid succession of EEPROM writes.

The EEPROM.update change would largely get rid of this problem, but another firmware solution at least in my use case would be to read the current settings. I had considered just recording the last settings in my own EEPROM, but we have built 20 of these devices (8-channel audio recorders with LCD UI) and some of them have already had their LCD modules swapped out, which my firmware wouldn't have been aware of.

Hope this clears up the intent at least. There may be a better way to do what I'm doing that I haven't come up with. It's certainly not a high-priority issue but considering it's most likely just a search/replace of EEPROM.write to EEPROM.update it is a fairly easy fix with hopefully some performance implications as well (when updating rapidly). I'd be more than happy to fork/change/test it and submit as a pull request if you were inclined to go down this path!

nseidle commented 5 years ago

Ah, yes that makes more sense to me now, thanks. Indeed, your update() recommendation is a much better way to go.

I would be thrilled with a PR if you get that far. I plan to spend some time on the firmware early next week so please PR as far as you get and I can pickup where you leave off.

makinako commented 5 years ago

Changes merged and tested by @nseidle! Closing this baby down.