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

Clean up EEPROM #17803

Merged

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Apr 29, 2020

  • Remove unused functions.
  • Eschew Arduino style interfaces for custom EEPROM.
  • Split up Flash / "wired" in the STM32F4/F7 HAL.
  • Consolidate DUE flash-based EEPROM.

References: #17651, #17772

@p3p
Copy link
Member

p3p commented Apr 30, 2020

If you are removing eeprom_init from the shared eeprom_read_byte function you need to make sure that it is being called in the appropriate PersistentStore::access_start(), it was needed because there wasn't a "start transfer" function so it was just called every byte.

@thinkyhead
Copy link
Member Author

If you are removing eeprom_init from the shared eeprom_read_byte function…

Thank you for bringing that to my attention here and on the other issue. I had the vague impression that persistent-store implementations already had this covered, but now I see: I still need to migrate some dangling inits over to PersistentStore::access_start.

I've been trying to tread lightly and not modify existing behavior until I had time and space to develop the full picture. I think I'm just about caught up now.

@thinkyhead thinkyhead force-pushed the bf2_wrangle_eeprom_PR branch 2 times, most recently from 8a3c5fe to 7bc2167 Compare April 30, 2020 22:42
@thinkyhead thinkyhead marked this pull request as draft April 30, 2020 22:43
@thinkyhead
Copy link
Member Author

It looks like, across the board, the Flash-based EEPROM emulations want to call their init method on every read / write. Is this strictly necessary, or were they just implemented following an obsolete pattern?

@thinkyhead thinkyhead force-pushed the bf2_wrangle_eeprom_PR branch 2 times, most recently from d8e9887 to ab5a002 Compare April 30, 2020 23:09
@p3p
Copy link
Member

p3p commented Apr 30, 2020

Looks like they are just following the old pattern then adding a flag in the init function so it only actually happens once.

Could only have a quick look, not even sure how the STM32_F4_F7 is handling the flash write buffering without using access_start() and access_finish() to handle the transaction .. you cant just write bytes to the flash ..

@thinkyhead thinkyhead marked this pull request as ready for review May 1, 2020 00:34
@thinkyhead
Copy link
Member Author

Could only have a quick look, not even sure how the STM32_F4_F7 is handling the flash

Actually I tried to build default with STM32F4 and I couldn't get past it not grokking our serial class names. I think that HAL is fundamentally fakakta.

@thinkyhead thinkyhead force-pushed the bf2_wrangle_eeprom_PR branch 9 times, most recently from 064b384 to 5e3ad0e Compare May 1, 2020 03:50
@thinkyhead
Copy link
Member Author

Ok, this is better in-general. The eeprom_init function now exists for Marlin's version of the Arduino EEPROM interfaces. And, if Arduino framework EEPROM code is used instead of Marlin's, the "wired" PersistentStore:access_start will call it anyway, adding that minor bridge. This is an overall improvement, so will be merged now. But please follow up if one of the patched items here is off the mark.

@thinkyhead thinkyhead merged commit 2107bc5 into MarlinFirmware:bugfix-2.0.x May 1, 2020
@thinkyhead thinkyhead deleted the bf2_wrangle_eeprom_PR branch May 1, 2020 05:05
jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
njibhu pushed a commit to njibhu/Marlin that referenced this pull request Aug 24, 2020
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
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.

2 participants