Skip to content

Add storage ReadWrite trait, and helper structs, traits and iterators #248

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

Conversation

MathiasKoch
Copy link

@MathiasKoch MathiasKoch commented Sep 18, 2020

I would like to propose an alternative solution to #28 .

There is currently one solution in #241, this is an alternative and simpler approach, highly inspired by the blog series by Cuervo in https://www.ecorax.net/as-above-so-below-1/ and https://www.ecorax.net/as-above-so-below-2/

The traits are implemented here (still working on the documentation): https://github.com/MathiasKoch/is25xp
and here: https://github.com/MathiasKoch/stm32l4xx-hal/blob/factbird-mini-1.0/src/flash.rs

I think this makes it MUCH easier for driver authors to make use of such a trait, compared to the solution given in #241 with a total of 5 traits. Also the helper iterators makes it much easier to handle optimizing for NOR flashes where writing 1s is not an option, without just defaulting to writing everything, as explained in above blog series.

EDIT:
After having used this for a while, there are a couple of notes and pain points to be addressed/discussed.

  1. I think the try_erase function is close to useless as it is currently, as it does only allow erasing the full memory range, as opposed to sections/regions/range. Any suggestions for a better approach? One possible could be to take a range (fn try_erase(&mut self, range: (Address, Address)) or fn try_erase(&mut self, from: Address, to: Address))?
  2. It might make sense to split the trait into a trait Read and a trait ReadWrite: Read, to support read-only memory? I have never encountered write-only, so i guess ReadWrite would make perfectly sense?

Any feedback is welcommed!

Closes #28

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ryankurte (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@MathiasKoch
Copy link
Author

I am closing this in favor of https://github.com/rust-embedded-community/embedded-storage that we started to get things right and mature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thoughts on Flash API?
3 participants