Skip to content

[2.0.x] Change direct eeprom access to HAL::PersistentStore#11526

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
hasenbanck:eeprom-cleanup
Aug 14, 2018
Merged

[2.0.x] Change direct eeprom access to HAL::PersistentStore#11526
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
hasenbanck:eeprom-cleanup

Conversation

@hasenbanck
Copy link
Contributor

Description

Every HAL seem to provide the HAL::PersistentStore interface. The configuration store does already use this interface, so it seems logical that all other code parts should use it to let the HAL abstract the persistence layer.

This commit changes the last two files that use the eeprom library directly.

Benefits

Better usage of the HAL abstraction. HAL then could provide persistence options on their discretion (like MCU internal Backup SRAM).

Notice

My C/C++ skills are limited. If some things can be expressed shorter, please feel free to suggest more elegant solutions. I have a feeling that we could also extend the current HAL::PersistentStore interface to include constant position / non crc methods.

@p3p
Copy link
Member

p3p commented Aug 12, 2018

Looks like the PersistentStore api could use a const std::size indexable read and write with no crc, would clean this up a bit I think, I really need to keep better track of my to do list (that may or may not exist) I did know this needed done at some point.

@hasenbanck
Copy link
Contributor Author

Hi @p3p, thank your for your comment. Do you want to implement the changes yourself to the PersistentStore api, so that I can change this little PR based on your changes, or should I implement these api changes myself in this PR?

@thinkyhead thinkyhead added PR: Improvement T: HAL & APIs Topic related to the HAL and internal APIs. labels Aug 13, 2018
@p3p
Copy link
Member

p3p commented Aug 13, 2018

I'l put in a PR with the API changes I've been meaning to add a storage capacity and change the namespace design to better fit with Marlins static class usage.

@hasenbanck
Copy link
Contributor Author

Great, I will then reflect those changes in this PR. Would be nice if you could reference this PR in yours, so that I get updated.

@thinkyhead
Copy link
Member

thinkyhead commented Aug 13, 2018

@p3p — Remember that you can check out hasenbanck/eeprom-cleanup and submit a PR to that branch, and when it's merged it will become part of this PR.

@p3p
Copy link
Member

p3p commented Aug 13, 2018

@thinkyhead The scope of my changes are going to dwarf this PR so I thought it better to create my own for the update and then update this PR for compatibility.

@thinkyhead
Copy link
Member

thinkyhead commented Aug 13, 2018

Thanks for the persistentStore static singleton update. It's great, and this was simple to update for it.

@p3p
Copy link
Member

p3p commented Aug 13, 2018

I thought you would like it, no more colons ;). I also added the read/write methods that don't need the crc passed, they are all just implemented as pass through atm. changing to capacity rather than E2END required a few offset changes that I hope are correct .. or peoples meshes may get corrupted ..

@thinkyhead thinkyhead merged commit cc0a604 into MarlinFirmware:bugfix-2.0.x Aug 14, 2018
@hasenbanck hasenbanck deleted the eeprom-cleanup branch August 14, 2018 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Improvement T: HAL & APIs Topic related to the HAL and internal APIs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants