Reset EEPROM when the layout changes

[thinkyhead]

Issues like #1378 and others indicate that new EEPROM layouts can cause Marlin to break on new installs, because Marlin tries to read the old layout as the new one. There needs to be either:

• Code that converts the previous layout version(s) to the new layout

or

• Code that resets the EEPROM and (tries to) present a message to the user

Is it OK to clear the EEPROM for users who may have stored values that don't match their configuration, if possibly they won't notice that this has occurred? Or, is it better to allow users to encounter undefined behavior, but instruct them to clear the EEPROM when flashing new firmware should issues occur?

[grob6000]

Great topic, I've been thinking about this too.

Currently, the firmware is supposed to read a stored version string in eeprom, if it doesn't match what the firmware was expecting, then it loads default values instead. This message is echoed to the serial, so you can see it in the host terminal.

This has been undermined by using #ifdef in the eeprom code - so depending on the configuration file, for the same eeprom version string, there's potentially multiple different eeprom structures, and hence the problems.

My preferred approach would be to continue using the version check, but make sure that the eeprom structure is constant no matter what happens in the config file. This means we'd need to store default values for things like delta & scara parameters, extra extruders, etc on write, even if it's a cartesian config. Oh, and then ban #ifdef from ConfigurationStore.cpp/h. :)

Killing old settings is annoying, but normally when updating firmware users are aware that their old settings may not roll over, and they should record them. If this was easy, e.g. if M503 was complete and in a consistent format, then it wouldn't be a big problem.

I could take a look at this if you like?

[daid]

I would suggest adding a "magic value" at the end of the EEPROM write, and checking that magic value when reading. If the magic value mismatches, erase the EEPROM start tag, and load the default. This should prevent most of the current problems.

(Note, the length of the EEPROM data did not change about a year ago, as it wrote dummy data in place of all the unused places, so recent patches must have broken that)

[odewdney]

Or you could store a collection of tag:value or tag:len:value, where each parameter has a fixed tag, and skip the tags you dont recognise.

[ledvinap]

One possibility is to store configuration G-codes into EEPROM...

[Booli]

I've been working with 3D printers for over a year now and only recently noticed that flashing new firmware on the Arduino does not clear the EEPROM. Probably a big reason for weird steps / movements / etc I encountered a while ago.

Doing M502 / M500 after a flash should create the correct EEPROM, but for me it seems that, when you are able to flash firmware, you should also be aware that your EEPROM settings can be changed.

I would prefer just cleaning out the EEPROM and storing default settings into it when flashing new firmware to the board.

[nophead]

I might be wrong but I think the fuse settings dictate whether flashing
clears the EEPROM.

On 20 January 2015 at 13:41, Pim Rutgers [email protected] wrote:

I've been working with 3D printers for over a year now and only recently
noticed that flashing new firmware on the Arduino does not clear the
EEPROM. Probably a big reason for weird steps / movements / etc I
encountered a while ago.

Doing M502 / M500 after a flash should create the correct EEPROM, but for
me it seems that, when you are able to flash firmware, you should also be
aware that your EEPROM settings can be changed.

I would prefer just cleaning out the EEPROM and storing default settings
into it when flashing new firmware to the board.

Reply to this email directly or view it on GitHub
#1388 (comment)
.

[Booli]

Seems that you are correct. EESAVE bit (#3), defaults to 1, so not preserved.

I do not know if the Chip Erase is being called when reflashing firmware tho

[daid]

Firmware loading is done with the arduino bootloader. Which has all eeprom functions commented out. Looked into this when making the cura firmware loader.

[Booli]

Yep, did some research and found that the boot loader lacks: chip erase(because it would erase itself) and some more commands.

So, that's not an option.

[CONSULitAS]

@thinkyhead Just my 2ct:

This solutions seem a) to complicated or b) to userunfriendly or c) not reliable enough to me.

I am afraid, that improper settings in EEPROM could damage Hardware of not experienced users.

• Why don't we save #define STRING_VERSION_CONFIG_H __DATE__ " " __TIME__ // build date and time or a similar binary timestamp (4 bytes should do) with the compile time to EEPROM?
• Then we can compare it to the compile time for our running firmware and clear EEPROM if it is different.
• Write message to serial console like New firmware detected - setting EEPROM to factory defaults would make the process transparent for the user.

Should be rather simple to code, user friendly and reliable. I think its hard to get broken, too.

Only disadvantage is, that EEPROM is reset after every compile. So it could be convenient for users if we provide .hex files with our example_configs for standard printers.

BTW: This took me three days to find out why my fresh rebased K8200 configuration did not work (insane speed for x and y - did not sound good for the hardware). And today I disassembled my Hardware thinking the E-stepper driver could be burnt. But it was again missing M502 / M500. Very annoying. Even if you have some Arduino coding experience like me.

BTW 2: First time I flashed a new firmware for my K8200 i used the firmware upgrade provided by Vellemann. The instructions by them do not tell to reset the EEPROM after flashing. It works just by chance. :-)

[grob6000]

If we get rid of the #ifdefs in ConfigurationStore.cpp (e.g. delta, scara, lcd parameters) such that the structure is consistent, then the existing version string method will work fine, without these issues (e.g. if you flash new firmware and the eeprom structure has not been tinkered with, the old values will be loaded happily, which is very convenient for most users).

The only hole is if we change the scaling/meaning of stored values elsewhere in the code, then we'd likely want to up the eeprom version too.

[alexborro]

I think saving the EEPROM version and comparing the current firmware
version is the way to go.

If the saved EEPROM layout is 8 and the current firmware users version 9,
it reset the EEPROM.

What is the drawback of this approach?

Cheers.

Alex.
Em 20/01/2015 20:49, "CONSULitAS" [email protected] escreveu:

@thinkyhead https://github.com/thinkyhead Just my 2ct:

This solutions seem a) to complicated or b) to userunfriendly or c) not
reliable enough to me.

I am afraid, that improper settings in EEPROM could damage Hardware of not
experienced users.

• Why don't we save #define STRING_VERSION_CONFIG_H DATE " "
  TIME // build date and time or a similar binary timestamp (4 bytes
  should do) with the compile time to EEPROM?
• Then we can compare it to the compile time for our running firmware
  and clear EEPROM if it is different.
• Write message to serial console like New firmware detected - setting
  EEPROM to factory defaults would make the process transparent for the
  user.

Should be rather simple to code, user friendly and reliable. I think its
hard to get broken, too.

Only disadvantage is, that EEPROM is reset after every compile. So it
could be convenient for users if we provide .hex files with our
example_configs for standard printers.

BTW: This took me three days to find out why my fresh rebased K8200
configuration did not work (insane speed for x and y - did not sound good
for the hardware). And today I disassembled my Hardware thinking the
E-stepper driver could be burnt. But it was again missing M502 / M500. Very
annoying. Even if you have some Arduino coding experience like me.

BTW 2: First time I flashed a new firmware for my K8200 i used the
firmware upgrade provided by Vellemann. The instructions by them do not
tell to reset the EEPROM after flashing. It works just by chance. :-)

—
Reply to this email directly or view it on GitHub
#1388 (comment)
.

[grob6000]

There is no drawback, as long as we don't have things like this (ConfigurationStore.cpp):

  #ifdef DELTA
  EEPROM_WRITE_VAR(i,endstop_adj);
  EEPROM_WRITE_VAR(i,delta_radius);
  EEPROM_WRITE_VAR(i,delta_diagonal_rod);
  EEPROM_WRITE_VAR(i,delta_segments_per_second);
  #endif//DELTA

Because then for the same version number, changing the configuration (e.g. changing the printer to a delta), the eeprom structure is completely different.

I would not just add padding, but would make sure that the default values for the stored settings (e.g. for delta_radius) were defined even if DELTA wasn't, and write these.

[daid]

If you look at older versions of the ConfigurationStore, you'll notice that the length and proper defaults where preserved, no matter the defines:
https://github.com/Ultimaker/Ultimaker2Marlin/blob/master/Marlin/ConfigurationStore.cpp#L44

So the above #ifdef DELTA is a bug. And there are a few more of those in there that where never properly implemented.

Now, I do see that implementing it with the "dummy values" might not be the best solution, as it's very maintenance intensive. Having an end-tag as I suggested would prevent most problems. Key-value combinations would be the best, technical wise, but I think it would also be quite complex in code.

[CONSULitAS]

@alexborro Hi Alex,

in an ideal world you would be right! In the case of Marlin we can see two drawbacks:

1. We have problems with poor coding, like @grob6000 mentioned.
2. The second drawback is, that - after i checked it - that there is a potential bug: Switching from a firmware version with "EEPROM_VERSION "V07"" to one with "EEPROM_VERSION "V14"" it worked only after manual reset with M502/M500. Hmmmm.

What i think in my humble opinion is, that the solution with the compile-time-stamp is nearly "unbreakable".

Perhaps we could do the following:

Invent a option #define EEPROM_SAFE_RESET (or something like this) which is ON by default.

This would cause the timestamp to be used and init a reset of EEPROM after each compile.

The Pros with us who are familiar to daily flashing their Marlin with a funky version switch this to OFF and get only a warning on LCD and Serial while booting "WARNING: Check EEPROM settings".

This would get it for the BDU to work and for the pro convenient.

What do you think? I had a short look at ConfigurationStore.cpp and it seems to be quite easy to implement.

• Use DateTime Library for calculations - could be skipped if wie think a bit harder. On Wikipedia is a sample code which does it without a library http://de.wikipedia.org/wiki/Unixzeit.
• Use a function like in http://stackoverflow.com/questions/1765014/convert-string-from-date-into-a-time-t to get a long time_t value compile_time.
• in Config_StoreSettings() we add a EEPROM_WRITE_VAR(i,compile_time); directly after the first "ver".
• in Config_RetrieveSettings() we add a EEPROM_READ_VAR(i,stored_compile_time) after reading the stored_ver
• the if (strncmp(ver,stored_ver,3) == 0) would be preceded with a #ifdef EEPROM_SAFE_RESET and a corresponding check to the 4 byte long integer with compile time
• Thats it?

Should I try to implement it?

Cheers Jochen

I think saving the EEPROM version and comparing the current firmware
version is the way to go.

If the saved EEPROM layout is 8 and the current firmware users version 9,
it reset the EEPROM.

What is the drawback of this approach?

Cheers.

Alex.

[CONSULitAS]

@alexborro @daid
Hi, please have a look at #1398.

It seems to me, that there is perhaps in fact a bug in detecting firmware upgrades. His version from march 2014 should have an older Vxx, so why didn't it work automagically? Hmmm...

Just my 2¢.

[alexborro]

I have other idea for this issue..
What if we have the EEPROM split in pages?

We may have a main page with, let's say, 256 bytes for the basic
parameters. And then more pages with 32 bytes for additional features under
#ifdefs.

So one specific page will belong to that feature until it is removed from
Marlin. New features will use new pages.

I think we have plenty EEPROM for that. Even the atmega 328 has 1K EEPROM,
so 1 main page and more 24 additional pages, much more than enough for
current features.

With this approach we don't need to control the EEPROM map version nether
care about null parameters.

Cheers.

Alex
Em 23/01/2015 07:49, "CONSULitAS" [email protected] escreveu:

@alexborro https://github.com/alexborro @daid https://github.com/daid
Hi, please have a look at #1398
#1398.

It seems to me, that there is perhaps in fact a bug in detecting firmware
upgrades. His version from march 2014 should have an older Vxx, so why
didn't it work automagically? Hmmm...

Just my 2¢.

—
Reply to this email directly or view it on GitHub
#1388 (comment)
.

[thinkyhead]

I like the idea of keeping the EEPROM layout the same regardless of added features (no #ifdef in ConfigurationStore) so that the version number definitely reflects the layout. This will minimize potential fires and strange behavior.

The idea of having a magic value (at the end) is not much different from storing the size of the EEPROM storage in the header (after the version), but it has the added advantage of being backward-compatible with older EEPROM values. So I like that idea as a secondary failsafe.

There was mention of making the EEPROM into a C struct. If that makes the code prettier and helps formalize the EEPROM structure then I'm all for that too.

I'm not sure how reliable any timestamp-based solution will be. To some extent I think we want those whose EEPROMs are not configured to reset on flash to keep their existing settings, so I don't think we want to just always reset it on a new compile. We just need to wrangle EEPROM so it won't change unpredictably from now on.

OTOH if there is quite a lot of storage, then maybe it would make sense to use the suggestion of storing the settings as G-Code instructions, and just playing them back when restoring from EEPROM. That would make the EEPROM system extensible without having to be concerned about storage order at all.

[wgm4321]

Why don't we just prefix the config EEPROM with CRC-32 calculation of the values and discard the version. Then upon reading, the CRC is recalced and if different, things are reset to default. This not only verifies the structure but also the actual values stored. If an additional field or fields are added, the CRC will fail.
I'm not sure backward compatibility with old config values chucks when the structure is extended is wise. It's possible that the extension defaults might only make sense of the values before the extension are defaults too.
http://www.opensource.apple.com/source/xnu/xnu-1456.1.26/bsd/libkern/crc32.c

[thinkyhead]

Currently the EEPROM storage can differ based on conditionals, but there's no longer a conditional check to assign the version number based on features. The code that did this was removed recently. It picked a version number based on the data structure that it would read / write. This avoided the problem with the EEPROM version not corresponding to the data structure (as it fails to do now).

As a temporary measure, for now we may want to put back the code that uses the old version number (v11) when there's no new info, and/or add code that skips reading the DELTA values when the version number is older (less than 14).

Going forward, we should pick a new version number (15, 20...) that expects some extra data which describes (or is otherwise unique, based on) the data structure. That said, the version number actually is good enough, but it must always mean a certain data structure.

[grob6000]

I'm with Scott - there's plenty of room in eeprom to store all the values,
used or not. So we may as well just have one comprehensive structure right?
What's the harm in storing (e.g.) default delta parameters even if they're
not used? It makes the eeprom module independent of the config, which would
be nice.

On Saturday, January 24, 2015, Scott Lahteine [email protected]
wrote:

Currently the EEPROM storage can differ based on conditionals, but there's
no longer a conditional check to assign the version number based on
features. The code that did this was removed recently. It picked a version
number based on the data structure that it would read / write. This avoided
the problem with the EEPROM version not corresponding to the data structure
(as it fails to do now).

As a temporary measure, for now we may want to put back the code that uses
the old version number (v11) when there's no new info, and/or add code that
skips reading the DELTA values when the version number is older (less than
14).

Going forward, we should pick a new version number (15, 20...) that
expects some extra data which describes (or is otherwise unique, based on)
the data structure. That said, the version number actually is good enough,
but it must always mean a certain data structure.

—
Reply to this email directly or view it on GitHub
#1388 (comment)
.

[thinkyhead]

If there's going to be only a single structure, with "default values" stored, I think these "defaults" should be stand-ins (such as 0xDEADBEEF) that tell Marlin to "use the default value" as set in the firmware. We should not put any default values into the EEPROM that might be read and interpreted, as this is likely to lead to more unintended / unexpected behavior.

[thinkyhead]

#1428 makes sure the EEPROM is consistent and does a little bit to try to avoid using bad dummy values.

[boelle]

will close this one as there have been no activity in 6 months

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.