Problem with EEPROM saving

Firmware
1

Hi,

We are using EEPROM to save values over power off.

However, after a firmware update, all data is lost.

We are using a struct to save stuff:

    struct {
        char mode; // g=growing, d=dryimg
        int dayCounter; // counting days the grow or dry was active. Starting from 1
        bool isDay; // true if the light is on
        int minutesInPhotoperiod; // how long has the system been in current photoperiod?  E.g. 31 minutes in NIGHT
        int dayDuration; // how long is the light on?
        bool fanAutoMode; // 1 for auto, 0 for manual
        float fanSpeed; // 0-100%
        char tempUnit; // F or C
        bool wifiStatus; // 1=on, 0=off
    } eeprom;

        eeprom.mode = 'g';
        eeprom.dayCounter = -1;
        eeprom.isDay = true;
        eeprom.minutesInPhotoperiod = 0;
        eeprom.dayDuration = 18 * 60;
        eeprom.fanAutoMode = 1;
        eeprom.fanSpeed = 30;
        eeprom.tempUnit = 'F';
        eeprom.wifiStatus = 1;
        

        EEPROM.put(0, eeprom);

Now we added more features and need to save more stuff in EEPROM.

At first, I added more values to the struct:

    struct {
        char mode; // g=growing, d=dryimg
        int dayCounter; // counting days the grow or dry was active. Starting from 1
        bool isDay; // true if the light is on
        int minutesInPhotoperiod; // how long has the system been in current photoperiod?  E.g. 31 minutes in NIGHT
        int dayDuration; // how long is the light on?
        bool fanAutoMode; // 1 for auto, 0 for manual
        float fanSpeed; // 0-100%
        char tempUnit; // F or C
        bool wifiStatus; // 1=on, 0=off
        uint8_t version;
        float targetTemperature;
        float targetHumidity;
        uint8_t fanSpeedMin;
        uint8_t fanSpeedMax;
        uint8_t ledBrightnessMax;
    } eeprom;

But now, on first start after Software-Update, the system could not get() the struct anymore ... likely because it was changed.

That's what the documentation says here:

(..) Use the same type of object you used in the put call.

To solve this problem, I put new values into a second struct:

    struct {
        char mode; // g=growing, d=dryimg
        int dayCounter; // counting days the grow or dry was active. Starting from 1
        bool isDay; // true if the light is on
        int minutesInPhotoperiod; // how long has the system been in current photoperiod?  E.g. 31 minutes in NIGHT
        int dayDuration; // how long is the light on?
        bool fanAutoMode; // 1 for auto, 0 for manual
        float fanSpeed; // 0-100%
        char tempUnit; // F or C
        bool wifiStatus; // 1=on, 0=off
    } eeprom;
    
     struct {
        uint8_t version;
        float targetTemperature;
        float targetHumidity;
        uint8_t fanSpeedMin;
        uint8_t fanSpeedMax;
        uint8_t ledBrightnessMax;
    } eeprom2;  


    EEPROM.put(0, eeprom);
    EEPROM.put(sizeof(eeprom), eeprom2);

and I retrieve them like this:

    EEPROM.get(0, eeprom);
    EEPROM.get(sizeof(eeprom), eeprom2);

But this still doesn't work.

After a firmware update, the system forgets all the values.

Can someone see a problem with my approach?

The full code can be seen here.

Thanks and kind regards

2

I think the problem is that TentState reads the EEPROM out of the constructor. That variable is part of Tent which is a global variable.

The problem is that it’s not safe to read the EEPROM out of a globally constructed object constructor as the system may not be able to read it that early in system setup. Because the order that globally constructed objects are initialized is indeterminate, it’s best to avoid anything other than simple variable initialization there.

Using a two-phase initialization where you have a setup or begin method that is called from setup() is usually the best and safest solution.

4 Likes
3

@volker
The other problem with a firmware update is if you add values in the struct and you wish to retain ‘default’ that might have been updated since the struct was defined. This is has been a big issue for me managing an installed base where I subsequently re-factored the code.

Thanks to the guys who liked Rick’s reply I have implemented the following. If you search around there are explanations of the magic number use and the checksum.

The struct template looks like this:

//eeprom storage using struct for settings using magic number and checksum
struct deviceSettings
{
    int     magicNumber = 0x11DEADEF;   //must be changed if struct changes or default values
    int     bright = LITEON;            //screen brightness
    int     stayOn = STAYONTIME;        //screen stay on period in minutes
    .....
    uint8_t padding[1] = {0};           //fill to 4 byte boundaries - calculated as 1 byte example
    int     checkSum  = 0;              //checksum value is always the last item
};

deviceSettings param;                   //create eeprom settings object

Then in setup()
restoreParameters(); //need to read back params from eeprom before use!

when one of the params is updated then need to call put function.

    param.stayOn = 0;
    putParameters();

//Helper to calculate the checkSum of parameters from structure param in EEPROM and return true if checks to that stored, false if not
bool checkSumParameters()
{
    uint8_t* _data   = (uint8_t*) &param;
    int      _chkSum = 0;
    int paramCheckSum;
    EEPROM.get(PARAM_ADDR + sizeof(param) - sizeof(paramCheckSum), paramCheckSum);  //read what is stored in the EEPROM copy of checksum
    for (unsigned int i = 0; i < sizeof(param); i++)
    {
        _data[i] = EEPROM.read(PARAM_ADDR + i);
        if (i < sizeof(param) - sizeof(_chkSum)) _chkSum += _data[i];
    }
    return (_chkSum == paramCheckSum);
}

//Helper to calculate the checkSum of parameters for the param struct
int calcCheckSumParameters()
{
    uint8_t* _data   = (uint8_t*) &param;
    int      _chkSum = 0;
    for (unsigned int i = 0; i < sizeof(param); i++)
    {
        if (i < (sizeof(param) - sizeof(_chkSum))) _chkSum += _data[i];
    }
    return _chkSum;
}

//function to write parameters to EEPROM with checkSum
void putParameters()
{
    param.checkSum = calcCheckSumParameters();
    EEPROM.put(PARAM_ADDR, param);
}

//function to get parameters from EEPROM if checkSum correct
void getParameters()
{
    if (checkSumParameters())                       //checkSum matches
    {
        EEPROM.get(PARAM_ADDR, param);
    }
    else                                            //checksum wrong
    {
        Log.error("getParameters checksum failed");
    }
} 

//function to restore parameters from EEPROM in setup()
void restoreParameters()
{
    int eepromMagicNumber;
    EEPROM.get(PARAM_ADDR, eepromMagicNumber);
    if (eepromMagicNumber == param.magicNumber)     //magic number is the same (no struct change)
    {
        EEPROM.get(PARAM_ADDR, param);              //read whole struct
    }
    else
    {
        restoreConfigurationFileItems();         //use special magic numbers to determine if update from pre-magic number and checksum struct or whether 
    }
}
4

Thanks everybody for the help.

I was able to solve my problem.

Turned out I unwittingly changed my struct.

Initially it looked like this:

I then moved the value 'mode' to the beginning.

This did not work anymore (The struct must remain the same)

I then also moved it into a begin() function to take it out of the constructor.

:+1:

5

I’m not convinced it’s a good idea to have two implementations of the checksum calculator function. If you ever decided to alter the logic you will have to do that twice (or break the logic).
Wouldn’t it be better to only have one and pass in a pointer and length and reuse that for the EEPROM and RAM based set of values?

6

Thanks for that spot - it could do with a bit of refactoring of the “Don’t Repeat Yourself” variety. I assume you are referring to checkSumParameters() and calcCheckSumParameters()?

They are actually doing different things - one is calculating the checksum from the RAM version of the struct and the other from the eeprom stored version. Question back - how would you refactor it?

7

But the calculating bit is the same, so that should get its own function as it doesn't matter where the data comes from the result should in both cases be the same, right?

int calcCheckSum(void* data, size_t len) {
  int retVal = 0;
  for (int i = 0; i < len; i++)
    retVal += *((uint8_t*)(data + i));
  return retVal;
}
2 Likes
8

I understood your point - it is just time to go back, refactor and re test! Working code first!

9

The compiler warned about 2 things in the calcCheckSum(). The first was that i was declared as int rather than unsigned int because it is compared to type size_t which is unsigned int. The second I am not so clear about what the fix is. Warning - pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] related to (data + i)? It compiles and works - I don’t like to see warnings.

10

This should compile without warnings

int calcCheckSum(void* data, size_t len) {
  int retVal = 0;
  for (size_t i = 0; i < len; i++)
    retVal += *((uint8_t*)data + i);
  return retVal;
}
1 Like
11

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.