[bugfix] Regression - persistent EEPROM CRC error on M501

bjarchi commented 6 years ago

I'm running bugfix-1.1.6, and periodically git stash my configs and git pull to incorporate new changes from upstream. Did so today, bringing me up to current 81e52138, and it seems a bug has been introduced that prevents settings from being loaded from EEPROM on boot or via M501. I've tried multiple build/upload cycles. It could be a hardware error, but I doubt it since there were recent changes to configuration_store.cpp. Would appreciate someone trying to reproduce on a current build tree to rule out hardware failure (configuration_store.cpp was changed in the past day).

Bug Report

Description: EEPROM checksum (CRC) fails on M501
Expected behaviour: M500 stores settings and computes checksum; M501 loads settings and compares computed checksum with stored checksum to guard against data errors
Actual behaviour: The checksum that M500 produces and stores is somehow always different from that produced by M501. Both checksums do change in response to changes in data stored - changing and committing settings both the stored and computed values to change - they're just not the same.
Steps to reproduce:
- Fresh build/flash
- M502 to restore from hardcoded defaults
- M500 to store these values to EEPROM
- M501 to read EEPROM values back - at which point the CRC error is detected and the system resets.
- Also works:
- Change settings that will be committed to EEPROM (e.g. M301, M851, ...)
- M500 to commit - note that the checksum is different from the hardcoded case
- M501 to load - checksum fail - note that both checksums are different from the hardcoded case.

config-20171202.zip

Communications log example showing CRC failure (hardcoded default case)

SENT: M502
READ: echo:Hardcoded Default Settings Loaded
Hardcoded Default Settings Loaded
READ: ok
(M105...)
SENT: M500
READ: echo:Settings Stored (622 bytes; crc 47861)
Settings Stored (622 bytes; crc 47861)
READ: ok
(M105 ...)
SENT: M501
READ: Error:EEPROM CRC mismatch - (stored) 47861 != 55978 (calculated)!
READ: echo:Hardcoded Default Settings Loaded
Hardcoded Default Settings Loaded
...

bjarchi commented 6 years ago

Update: This continues to occur whether SKEW_CORRECTION is defined or not - and interestingly I see the same two CRC values reported for a post-flash M502; M500; M501 sequence with SKEW_CORRECTION enabled as I did before with it disabled:

SENT: M502
READ: echo:Hardcoded Default Settings Loaded
Hardcoded Default Settings Loaded
READ: ok
SENT: M500
READ: echo:Settings Stored (622 bytes; crc 47861)
Settings Stored (622 bytes; crc 47861)
READ: ok
SENT: M501
READ: Error:EEPROM CRC mismatch - (stored) 47861 != 55978 (calculated)!
READ: echo:Hardcoded Default Settings Loaded
Hardcoded Default Settings Loaded

bjarchi commented 6 years ago

I found it!!

configuration_store.cpp, lines 929/933 - number of dummy values read need to be updated from 7/10 to 8/11 to reflect recent changes in DELTA configuration (lines 488/492). Changed this and M501 works again.

I can set up a PR with this patch pretty quickly, but I don't have a fork of the repo yet... so if someone with access wants to quickly make this change it might be a little faster.

Roxy-3D commented 6 years ago

Can you cut and paste your changes here? I can slip them into both branches....

bjarchi commented 6 years ago

Actually it was faster than I remembered to set up a local fork... I'm about to generate a PR. Will that work for transclusion into 2.0.x?

Roxy-3D commented 6 years ago

There needs to be a Pull Request for both bugfix branches... Two separate (but very very similar) Pull Requests.

Thank You!!!

bjarchi commented 6 years ago

Ok, should be trivial to set up a correspoding PR for bugfix-2.0.x (right branch?) since I have the local clone. I'll go ahead and do that unless you prefer to do so.

Roxy-3D commented 6 years ago

Go Ahead!!! I'll merge when they show up.

bjarchi commented 6 years ago

Awesome! I probably shouldn't be this excited about a two-line change, but it's my first free software collaboration ^_^

Might be worth someone testing on bugfix-2.0.x, but the code was clearly and identically wrong and the same patch fixed the regression on bugfix-1.1.x.

Unrelated question, since you seem to work on a lot of the leveling code - do you know where the menu items from this:

// Add a menu item to move between bed corners for manual bed adjustment
#define LEVEL_BED_CORNERS

should show up? I added a 12864 to my system and want to be able to use it for manual rough leveling, but I can't seem to find the menu item anywhere.

Roxy-3D commented 6 years ago

In ultralcd.cpp

 #if ENABLED(LEVEL_BED_CORNERS)

    /**
     * Level corners, starting in the front-left corner.
     */
    static int8_t bed_corner;
    void _lcd_goto_next_corner() {
      line_to_z(4.0);
      switch (bed_corner) {
        case 0:
          current_position[X_AXIS] = X_MIN_BED + 10;

      END_MENU();
    }
...
    void _lcd_level_bed_corners() {
      defer_return_to_status = true;
      lcd_goto_screen(_lcd_corner_submenu);
      bed_corner = 0;
      _lcd_goto_next_corner();
    }
  #endif // LEVEL_BED_CORNERS

I've never used it... I use UBL's G29 P1 U option and let it get a collection of points. (roughly 10 or 15 spread out across the bed.)

bjarchi commented 6 years ago

I've never used it... I use UBL's G29 P1 U option and let it get a collection of points. (roughly 10 or 15 spread out across the bed.)

Hmm maybe I'll have to try that (or just keep using gcode or the TFT, until I likely remove that). Do you use that to physically level the bed and then re-load your active mesh to re-activate UBL? (since P1 invalidates the current mesh?)

It looks like it should be in the 'prepare' menu, just below the EEPROM commands, if I'm reading the source right... can't quite figure out how the 'foo submenu items' vs 'foo menu' structure is laid out just yet... but I don't see any prerequisites other than being homed.

Actually maybe it only gets drawn if a non-UBL leveling system is in-use. I'll look into it further. Do you know if it was intended for LEVEL_BED_CORNERS to be invalidated by UBL?

Roxy-3D commented 6 years ago

It looks like it should be in the 'prepare' menu, just below the EEPROM commands, if I'm reading the source right... can't quite figure out how the 'foo submenu items' vs 'foo menu' structure is laid out just yet... but I don't see any prerequisites other than being homed.

The UBL "G29 P1 U" isn't in the LCD Menu. It is kind of a special case. You have to have UBL enabled. Then a G29 P1 U will ultimately do the same thing as a normal G29 P1. But the difference is, it tries to keep the points as spread out as possible. The normal G29 P1 tries to expand the radius of the probed points slowly so the user has time to stop the command if the nozzle starts getting too close to the glass. G29 P1 U doesn't worry about that. It tries to spread the points out as far from each other as possible. And the user can stop the command after 8 or 12 points and use them to do a physical bed leveling.

It is possible you are talking about the 4-corner leveling. That should be there under any bed leveling system if it is enabled.

Actually maybe it only gets drawn if a non-UBL leveling system is in-use. I'll look into it further. Do you know if it was intended for LEVEL_BED_CORNERS to be invalidated by UBL?

Just to be nit-picky... It isn't "drawn". The 4-corner stuff is probing of the bed. I suspect you know that. But if not... This information will be helpful to you.

UBL is agnostic to anything else in the firmware. Some commands are duplicated like the mesh editing with M420 and the Bed Leveling activation and deactivation. But that was done as a cutesy to help people transitioning from one bed leveling system to another. Everything UBL needs is in the G26 and G29 command set.

It really doesn't do anything that can't be done other ways. I added for myself. I wanted more than the 4 corners but I also wanted some other points as I made a decision about how much to adjust a corner. It works well for me. (Big surprise... I wrote it for me!) But it may not be the way you prefer to level the bed.

bjarchi commented 6 years ago

Thanks for the clarification!

It is possible you are talking about the 4-corner leveling. That should be there under any bed leveling system if it is enabled.

Yes this is what I was talking about - I wanted to have it in the LCD menu to assist with manual bed leveling before using UBL. It does not appear that it's being placed in the LCD menu tree with UBL enabled. A quick look through the code yesterday suggested that it was only actually added to the menu tree with LCD_LEVELING or LEVEL_MESH active, but that was only a quick look at the code.

This should probably be logged as a separate issue, assuming it is one.

Just to be nit-picky... It isn't "drawn". The 4-corner stuff is probing of the bed. I suspect you know that. But if not... This information will be helpful to you.

Yeah I wasn't very clear in that statement. When I said 'drawn' I meant drawn on the LCD - it's clear from the code that its setup functions and etc. are compiled as long as LEVEL_BED_CORNERS is defined, and they only check for the machine being homed, but as far as I could tell the 4-corner level menu doesn't actually get drawn in the menu tree when UBL is active.

I'll try out G29 P1 U in the meantime, but it sounds like this is its own bug? Might even be something I can fix with more time to get familiar with the LCD code, since it seems low priority.

I'm happy to close this issue now, but I want to give you a chance to comment in case you want to.

github-actions[bot] commented 3 years ago

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.

MarlinFirmware / Marlin

[bugfix] Regression - persistent EEPROM CRC error on M501 #8629

Bug Report