Fixing Printrbot's "Legacy" firmware

[RetireeJay]

Printrbot has provided firmware for people with legacy printers who are installing Rev F boards. It's found at https://github.com/Printrbot/Marlin/releases/ and called "RevF - Ver5 - Legacy."

By and large, it's fine. But it assumes that the user has a bed probe. I don't have one, and don't plan to install one, so I wanted to remove that feature. Yeah, I know I could just simply never call G29 - but let's just say I'm a masochist and wanted to learn more about the code.

So, there's a line in Configuration.h

Code: Select all

#define ENABLE_AUTO_BED_LEVELING // Delete the comment to enable (remove // at the start of the line)

which clearly already had the "//" deleted. So I put it back in.

Code: Select all

//#define ENABLE_AUTO_BED_LEVELING // Delete the comment to enable (remove // at the start of the line)

Now the code would not compile. But I studied the error messages and realized they were all coming from ConfigurationStore.cpp, which I quickly realized was the code that handles the EEPROM commands. There were variables for "bed_level_offset" that were causing the errors; those variables were supposed to be defined elsewhere and used in ConfigurationStore. But upon further study, and a bit of discussion on Github's "issues" forums, I realized that the proper way to handle these orphan variables is to turn them off if not used, but substitute dummy variables in their place so the EEPROM data structure remains intact:

Code: Select all

#ifdef ENABLE_AUTO_BED_LEVELING
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[0]);
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[1]);
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[2]);
 #else
   for (int q = 3; q--;) EEPROM_WRITE_VAR(i, dummy); 
 #endif

Once I did this (in two places) I could compile with the bed leveling turned on or off.

[RetireeJay]

Well, for some reason the above solution - though it compiles - doesn't quite behave right (M500, then M501 doesn't bring back what was saved with the M500, with an offset starting apparently in the vicinity of the Auto-leveling option).
So here's better code for the writing and reading sections, making them virtually identical and very explicit:

Code: Select all

#ifdef ENABLE_AUTO_BED_LEVELING
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[0]);
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[1]);
   EEPROM_WRITE_VAR(i,bed_level_probe_offset[2]);
 #else
   EEPROM_WRITE_VAR(i,dummy);
   EEPROM_WRITE_VAR(i,dummy);
   EEPROM_WRITE_VAR(i,dummy);
 #endif
 
  #ifdef PIDTEMP
    EEPROM_WRITE_VAR(i,Kp);
    EEPROM_WRITE_VAR(i,Ki);
    EEPROM_WRITE_VAR(i,Kd);
  #else
      float dummy = 3000.0f;
    EEPROM_WRITE_VAR(i,dummy);
      dummy = 0.0f;
    EEPROM_WRITE_VAR(i,dummy);
    EEPROM_WRITE_VAR(i,dummy);
  #endif

Code: Select all

      #ifdef ENABLE_AUTO_BED_LEVELING  //Dummy variables should be in here
         EEPROM_READ_VAR(i,bed_level_probe_offset[0]);
         EEPROM_READ_VAR(i,bed_level_probe_offset[1]);
         EEPROM_READ_VAR(i,bed_level_probe_offset[2]);
      #else
         EEPROM_READ_VAR(i,dummy);
         EEPROM_READ_VAR(i,dummy);
         EEPROM_READ_VAR(i,dummy);
      #endif
      
        #ifdef PIDTEMP
          // do not need to scale PID values as the values in EEPROM are already scaled      
         EEPROM_READ_VAR(i,Kp);
         EEPROM_READ_VAR(i,Ki);
         EEPROM_READ_VAR(i,Kd);       
        #else
         EEPROM_READ_VAR(i,dummy);
         EEPROM_READ_VAR(i,dummy);
         EEPROM_READ_VAR(i,dummy);
      #endif


[Mooselake]

It looks like "dummy" is dependent on PIDTEMP being undefined (it's declared in ConfigurationStore.cpp line 81), so if that gets defined you might have a problem. You need to use a float (well, or something that has the same sizeof()). I didn't dig real deep...

An alternative would be to remove the bed leveling variables from EEPROM, and change the version (forget where that is) to force initializing the EEPROM the first time you execute the new version.

I'm not sure why your for loop didn't work. I initially thought it was a problem with using post decrement, but that wasn't it. It should have executed the loop 3 times, what you wanted. I imagine I'm missing something obvious, despite blowing the dust off K&R and doing some review.

Kirk

[RetireeJay]

Actually, "dummy" was declared at the beginning of the subroutines. The assignment of the value 3000 during writing was just something I retained from the previous version of the code.

Code: Select all

#ifdef EEPROM_SETTINGS
void Config_StoreSettings()
{
  char ver[4]= "000";
  float dummy = 0.0f;
  int i=EEPROM_OFFSET;
  EEPROM_WRITE_VAR(i,ver); // invalidate data first
  EEPROM_WRITE_VAR(i,axis_steps_per_unit); 
  EEPROM_WRITE_VAR(i,max_feedrate); 
  EEPROM_WRITE_VAR(i,max_acceleration_units_per_sq_second);
...
...

Code: Select all

#ifdef EEPROM_SETTINGS
void Config_RetrieveSettings()
{
    int i=EEPROM_OFFSET;
   float dummy = 0.0f;
    char stored_ver[4];
    char ver[4]=EEPROM_VERSION;
    EEPROM_READ_VAR(i,stored_ver); //read stored version
    //  SERIAL_ECHOLN("Version: [" << ver << "] Stored version: [" << stored_ver << "]");
    if (strncmp(ver,stored_ver,3) == 0)
    {
        // version number match
        EEPROM_READ_VAR(i,axis_steps_per_unit); 
        EEPROM_READ_VAR(i,max_feedrate); 
        EEPROM_READ_VAR(i,max_acceleration_units_per_sq_second);
...
...

By the way, changing the Version of the EEPROM (or maybe something else I did...) invalidates the Repetier ability to do a WYSIWYG edit of the most-used EEPROM values.