EEPROM Checksum

[thinkyhead]

Implement a checksum in the EEPROM to verify the stored data.

This also bumps the EEPROM version to V24.

[lrpirlet]

@thinkyhead Maybe some reorganiztion of the EEPROM is needed first... Today if I re-compile the same version of Marlin, just changing the number of Mesh Bed Leveling points, I corrupt all subsequent values... See #4086

[MagoKimbra]

// Now store checksum in stored_checksum variable
uint16_t stored_checksum = eeprom_checksum;

[Grogyan]

Do we have for M500 the ability to erase the EEPROM before doing a "factory reset"?

[thinkyhead]

@Grogyan - Do you mean, write zeros to the EEPROM first before writing the data?

[Grogyan]

@thinkyhead yes, zeros or FF typically.

This isn't the first time EEPROM corruption has happened, hence why i'm inquiring whether an erase is done first.
Eg
If EEPROM_VERSION != V24 Then inform user, erase EEPROM before writing new version

[thinkyhead]

@Grogyan It will be interesting to see how often this checksum check actually gets tripped. It should be generally rare, but yes as you point out we need to account for "worst-case" for all data we store. So we should make room for a 10x10 mesh and make that the maximum size allowed. Or use a MAX_MESH_POINTS setting that enforces it.

[thinkyhead]

If EEPROM_VERSION != V24 Then inform user, erase EEPROM before writing new version

The logic Marlin follows, which I think is right, is if the version or checksum fails to match, it will throw an error and just apply the defaults explicitly. Users can then just write to EEPROM with M500 to replace the outdated or malformed data.

In the long run it would be good to be able to specify, from Marlin version to Marlin version, that we only add to the end of the EEPROM structure and never change anything before it (from the previous EEPROM version), so the old data can always be read, even if it doesn't have all the newer data. But I expect we will continue to shuffle the EEPROM around from point-release to point-release.

I've proposed that we just write GCode to the EEPROM instead. There's enough room for all the output of M503 to fit. Then you could just load it and "play it back."

[Grogyan]

@thinkyhead that assumes that the EEPROM data recalled for certain functions is in the same location in EEPROM, which in this PR is not, hence if not erased before writing the new EEPROM version.
Keeping the old version of EEPROM with an M500 can also cause corruption issues.

The ideal method, as suggested would be to call the old settings into RAM, erase the EEPROM then load the settings from RAM back into EEPROM on the new version EEPROM. Thereby the correct EEPROM addresses will be correct.

[MagoKimbra]

Now i = 104 you must add 2 for uint16 eeprom-checksum or write eeprom_checksum 0...

[thinkyhead]

Thanks again! I'm trying to do too many things at once, obviously.

[thinkyhead]

The ideal method, as suggested would be to call the old settings into RAM, erase the EEPROM then load the settings from RAM back into EEPROM on the new version EEPROM.

@Grogyan So far we haven't kept track of the old layouts or preserved the old reading code when we update to a new layout. It would be nice if we could do that, but it's not so simple. Should we convert every old version to the latest one, or just the previous one?

If we stored the GCode text for the current configuration, rather than the binary, that would make it much easier, but that would require a complete rewrite of the EEPROM code. Note that we only have 512 bytes (guaranteed) to work with. On larger boards we have plenty of space - at least 1K, and up to 4K - but we want to keep supporting smaller boards.

In the long run, we do want to improve the EEPROM and configuration stuff generally. But I think we need to start over and come up with a good robust system. Perhaps we can use IFF formatting.

Keeping the old version of EEPROM with an M500 can also cause corruption issues.

I don't see that anywhere. M500 overwrites all the space it needs to overwrite, skipping over nothing. M501 won't even try to read an older EEPROM layout.

Here's the code that reads the mesh. Apparently it just reads –but ignores– the existing data if it doesn't match the new mesh dimensions. (The first values written for the mesh are a "mini-header.")

#if ENABLED(MESH_BED_LEVELING)
  mbl.status = dummy_uint8;
  mbl.z_offset = dummy;
  if (mesh_num_x == MESH_NUM_X_POINTS && mesh_num_y == MESH_NUM_Y_POINTS) {
    EEPROM_READ_VAR(i, mbl.z_values);
  } else {
    mbl.reset();
    for (uint8_t q = 0; q < mesh_num_x * mesh_num_y; q++) EEPROM_READ_VAR(i, dummy);
  }
#else
  for (uint8_t q = 0; q < mesh_num_x * mesh_num_y; q++) EEPROM_READ_VAR(i, dummy);
#endif // MESH_BED_LEVELING

The EEPROM-reading code is actually pretty smart about all the issues you raise. I think if you look closely you will not be able to find a snippet that illustrates a potential for corruption. But please point out any that do!

[Grogyan]

@thinkyhead it is something worth looking into, IMO after a 1.10 (stable) release.
The EEPROM reading looks OK to me, with my limited coding skills.

[thinkyhead]

The configurability of Repetier-Host and Smoothieware make me a bit jealous. They put Marlin to shame. Of course, Smoothie runs on a much faster board, so they can make everything parametric and still get great performance. We have to explicitly pre-configure a lot more to get decent performance.

I'm sure that after 1.1.0 we'll do a great re-write of the EEPROM code, and we can add an option to save/restore current settings to SD also.