EEPROM store and check

I am designing a product and it includes an atmega2560 that I am programming with arduino.

The product has a ton of settings the user can change and they need to be kept when it is unpluged and pluged again, hence I am making use of EEPROM to store those values.

This product is going to be used on a business setting so it is important that it performs flawlessly so I am trying to make sure the EEPROM works as flawlessly as possible.

I’ve created this function that tries to store a value in the EEPROM, then reads it back and compares to make sure the byte in EEPROM is the right one.
If the comparison fails it tries to store it and checks again. It does this up to 20 times (arbitrary number) and if it fails 20 times, it considers the EEPROM byte is messed up and changes the address to a different one from an array of spare addresses.

This function is going to be used a lot of times on the sketch (everytime a setting is stored).

Do you see something wrong whith it or is there a better way to accomplish the same?

void store_value(int &address, int value)
{
  bool check = 0;
  byte check_counter = 0;
  while (check == 0){
    if(check_counter > 20)
    {
        // byte unresponsive, change address
        for(byte i = 0; i < 20; i++){
          if(alternate_add_used[i] == 0){
            address = alternate_add[i];
            return;
          }
        }
        check_counter = 0;
    }
    EEPROM.update(address, value);
    byte read_check = 0;
    read_check = EEPROM.read(address);
    if(value == read_check){
      check = 1;
    }
    check_counter++;
  }
}

Thank you very much for your time!

Why are you passing int type (16-bit) when you are storing/reading/comparing 1-byte data?

Please, post your complete sketch so that the declared variables of your SUR (sub routine) could be traced.

You are right, the value variable should be a byte, not an int.

The full sketch is just enourmous ( I have it split in 7 different files) and I don't think it is necessary to understand this function, I am simply storing bytes (most of the times a 1 or a 0 to know if a setting is active or inactive).

Edit, I just added the lines in all the files and it has 1912 lines. I am half way trhough I would say, so I expect the whole programm to end up being around 4000 lines of code.

What does...

alternate_add_used[i] == 0

..indicate?

What is the point of this?

         if(alternate_add_used[i] == 0){
            address = alternate_add[i];
            return;

Presumably, as that code executes in a loop after you have tried 20 times, that is supposed to mean you have found an alternate address? If so...

a) Why are you "return"ing without updating the variable at the new address?
b) How are you updating alternate_add_used so the program knows that the detected faulty address has been "used"?
c) How are you remembering which address you are using once you have moved to a new address?
d) What is the point of the arrays? I would have thought, if say you require 10 bytes of EEPROM total, you could just advance address by 10 and use the next available block? This doesn't require an array, just to store the address of which block you are currently using and add 10 for the next one.

Personally I would start with all my settings that I need to save in a struct. Then write code to save that struct to EEPROM, then check it is correct after writing. If I got an error I'd advance the address. Having code which writes and checks a single value, and does a partial job of address management doesn't seem a great choice to me.

pcbbc:
What does...

alternate_add_used[i] == 0

..indicate?

It indicates the alternate address is not yet used.

pcbbc:
What is the point of this?

         if(alternate_add_used[i] == 0){

address = alternate_add[i];
            return;



Presumably, as that code executes in a loop after you have tried 20 times, that is supposed to mean you have found an alternate address? If so...

a) Why are you "return"ing without updating the variable at the new address?
b) How are you updating alternate_add_used so the program knows that the detected faulty address has been "used"?
c) How are you remembering which address you are using once you have moved to a new address?
d) What is the point of the arrays? I would have thought, if say you require 10 bytes of EEPROM total, you could just advance address by 10 and use the next available block? This doesn't require an array, just to store the address of which block you are currently using and add 10 for the next one.

Personally I would start with all my settings that I need to save in a struct. Then write code to save that struct to EEPROM, then check it is correct after writing. If I got an error I'd advance the address. Having code which writes and checks a single value, and does a partial job of address management doesn't seem a great choice to me.

a) You are right, I need to update the value at the new address.
b) I am not doing it, I forgot that part, thanks for pointing that out!
c) That's a good question, I am not storing anyway the new address so once unplugged it will loose the new address and revert to the old one. I should do something to fix this.
d) I need hundreds of bytes of EEPROM, I thought it would be easier to create an array with a few spare ones. I need somthing like 500 bytes at least, so with 4k in the 2560 I could only advance the whole thing 8 times before runing out of EEPROM, should it be enough?

joancasas:
d) I need hundreds of bytes of EEPROM, I thought it would be easier to create an array with a few spare ones. I need something like 500 bytes at least, so with 4k in the 2560 I could only advance the whole thing 4 times before running out of EEPPROM.

Then your whole approach is wrong. You need a mapping of "failed addresses" to "replacement addresses" in a table.

Before writing to an address you need to search through this table and see if the address has been replaced. Use the updated address if necessary for the write.

After detecting a failure writing you need to again search through the list and either mark the current replacement bad (if it has already been replaced) and then allocate a new replacement.

The disadvantage is for any significant number of "replacement addresses" you also require quite a lot of EEPROM to store this table.

Just how often are you thinking of writing to EEPROM? You are guaranteed at least 50,000 erase write cycles. If a user is changing a setting 50,000 times even at 10 changes a day that's 13 years at least. Seems to me you are coding for a situation that my never occur within a reasonable lifetime of a product.

pcbbc:
Just how often are you thinking of writing to EEPROM? You are guaranteed at least 50,000 erase write cycles. If a user is changing a setting 50,000 times even at 10 changes a day that's 13 years at least. Seems to me you are coding for a situation that my never occur within a reasonable lifetime of a product.

Maybe I am being too afraid of the EEPROM failing.
I expect most bytes will be written less than 100 times in the life of the product, maybe even less than 50 in most cases.
Should I scratch the whole idea and trust the EEPROM to work just fine? Am I being too cautious?