Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make EEPROM storage consistent #1428

Merged
merged 5 commits into from
Jan 29, 2015

Conversation

thinkyhead
Copy link
Member

Update ConfigurationStore to always write dummy values for disabled options, including FWRETRACT, DELTA, and SCARA. Update the EEPROM version to “V15.” Also fixed a buffer overrun with axis_scaling in Config_ResetDefault.

Update ConfigurationStore to always write dummy values for disabled
options, including FWRETRACT, DELTA, and SCARA. Update the EEPROM
version to “V15.” Also fixes a buffer overrun with axis_scaling in
Config_ResetDefault.
@clefranc
Copy link
Contributor

Problem with PIDTEMP aligment again: if 16 values are written, 16 must be read.

297 + // 4 x 4 = 16 slots for PID parameters
298 + for (int q=16; q--;) EEPROM_READ_VAR(i, dummy);

@clefranc
Copy link
Contributor

Also, should not it be a section for volumetric extrusion?

59 + * VOLUMETRIC EXTRUSION
60 + * volumetric_enabled
61 + * filament_size (x4)

value++;
}while(--size);
void _EEPROM_writeData(int &pos, uint8_t* value, uint8_t size) {
do {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thinkyhead
Hi Scott,
great work! This should help a lot.
What do you think, wouldn't it be a good idea to implement a write verify here?
We could read the written *value and check if it worked. If not, give it some 2 or 3 retries.
If the retries don't work, give a error message to serial.
Something like

void _EEPROM_writeData(int &pos, uint8_t* value, uint8_t size) {
  uint8_t retries,
              value_read;
  int pos_read;

  do {

    retries = 0;
    do {
      eeprom_write_byte((unsigned char*)pos, *value);
      value_read = eeprom_read_byte((unsigned char*)pos);
    } while ( (*value != value_read) && (retries++<3) )

    if (*value != value_read) {
      SERIAL_ECHO_START;
      SERIAL_ECHOPAIR("ERROR writing to EEPORM: at pos ", pos);
      SERIAL_ECHOPAIR(" wrote " , *value);
      SERIAL_ECHOPAIR(" read " , value_read);
      SERIAL_ECHOLN("");
    }

    pos++;
    value++;
  } while (--size);
}

This is just a q+d hack, but could help.

This would perhaps fix #416 in nearly most cases. @daid told 20 days ago :

We had to replace a few Arduino's at Ultimaker due to the EEPROM not working properly (settings got lost all the time)

In all other cases there would be a feedback to the user.

Cheers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that's it's very rare for EEPROM not to work, according to what I've seen. We've shipped thousands of printers, and only a few showed this problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daid I see. And #416 seems to be a hard to repoduce problem, too.

But wouldn't it be safer to check nevertheless? What if EEPROM gets old and fails after some years? Even Harddisks and SSDs do verify and the are good for many TB written.

I think this would use only some bytes of RAM and PROGMEM, and performance is IMHO not critical during writing to EEPROM. And I am not sure if every supplier checks his boards for shipping.

Cheers

@clefranc
Copy link
Contributor

@CONSULitAS
I like the idea of a write check, and if you go this route, can I suggest to make a read check too?

If the same data is to be written to the same position, then skip this write.

Another suggestion is to add a bad EEPROM cell map, where you can add positions reported by the failed write check, and skip these positions. This is surely overkill for the amount of data and write cycle we use here, but it could make someone happy to regain access to his EEPROM.

@thinkyhead
Copy link
Member Author

@clefranc I will fix the PIDTEMP alignment asap.

@CONSULitAS I like the idea of verifying write to catch hardware errors. If it fails even once, that should be flagged.

Should be reading 4 x 4 PID values instead of 4 x 3
@thinkyhead
Copy link
Member Author

@clefranc OK, I fixed the PIDTEMP restore proper alignment.

The volumetric options seem to be fine as they are. There's no #define to enable/disable the volumetric_enabled variable, so I have not labeled that section or enclosed it in an #if block. If the volumetric code becomes conditional, then I'll add a label and enclose those parts in a block.

- Also adds a language string for the error.
- Also adds SERIAL_EOL as an alias for SERIAL_ECHOLN(“”)
@clefranc
Copy link
Contributor

@thinkyhead
So? What's wrong?

@thinkyhead
Copy link
Member Author

@clefranc Last commit failed the build due to a bad line. Fixed.

thinkyhead added a commit that referenced this pull request Jan 29, 2015
@thinkyhead thinkyhead merged commit 7df9ca0 into MarlinFirmware:Development Jan 29, 2015
@thinkyhead thinkyhead deleted the issue_1388_eeprom branch January 31, 2015 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants