Skip to content

Enhancement: RmwNorFlash #14

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

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Enhancement: RmwNorFlash #14

merged 3 commits into from
Sep 15, 2021

Conversation

MathiasKoch
Copy link
Collaborator

This PR implements RMW helpers for Nor flashes.

It was extracted from the previous PR #12, in order to land the traits and release them faster.

@MathiasKoch MathiasKoch force-pushed the enhancement/rmw-nor-flash branch from cf0be9f to 0d0c7a5 Compare May 18, 2021 08:37
@MathiasKoch MathiasKoch force-pushed the enhancement/rmw-nor-flash branch from 0d0c7a5 to 484ae9e Compare May 18, 2021 08:38
@Sympatron
Copy link
Contributor

I like the approach. Especially that the merge_buffer does allow you to offload the merge process off the stack.

I am not a big fan of the overlaps() function, because it is not very readable / unclear what's going on at first. On the other hand it makes the implementation quite concise. So I have no strong opinion either way.

Currently every write out of bounds is just ignored. Returning an error might be better, but I think you would have to wrap the Storage's Error type to do that.

@MathiasKoch
Copy link
Collaborator Author

I am not a big fan of the overlaps() function, because it is not very readable / unclear what's going on at first. On the other hand it makes the implementation quite concise. So I have no strong opinion either way.

I have the exact same reservation, thus the remark here:

// TODO: This might be possible to do in a smarter way?

So anything cleaner or easier to read is more than welcomed!

@MathiasKoch
Copy link
Collaborator Author

MathiasKoch commented Sep 13, 2021

Could we merge and release this PR as is? Or do any of you have any objections, nit-picks or similar?
@Dirbaio @eldruin @Sympatron

@Sympatron
Copy link
Contributor

I'd say we can. My main concern was that writes out of bounds are just silently ignored, but this could probably be fixed in a separate PR.

@MathiasKoch MathiasKoch merged commit 3b3671c into master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants