Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
makinako opened this issue Jan 13, 2019 · 4 comments

Comments

@makinako
Copy link
Contributor

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
Copy link
Member

nseidle commented Jan 24, 2019

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
Copy link
Contributor Author

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
Copy link
Member

nseidle commented Jan 25, 2019

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants