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

Improve Nvmc implementation of embedded-storage traits #374

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

ia0
Copy link
Contributor

@ia0 ia0 commented Jan 9, 2022

This PR opens a design discussion regarding the Nvmc API and implementation.

It contains the following 3 commits:

  1. Modify the example to surface some of the following issues:
    • Bug: new doesn't check that the storage is well-aligned.
    • Bug: read confuses offsets in the storage and offsets in the output buffer.
    • Bug: Many arithmetic overflows like here
    • Bug: Off-by-one error when the read output buffer is word-aligned.
    • Bug: read reads big-endian words regardless of the architecture.
    • Bug: erase doesn't check that the input is within bounds.
    • Bug: erase divides by 4 twice (other time is in erase_page).
    • Bug: write doesn't check that the input is within bounds.
    • Bug: write confuses offsets in the storage and offsets in the input buffer.
    • Bug: write writes big-endian words regardless of the architecture.
    • Error-prone: read is very complicated and seems to have other bugs.
    • Not ideal: READ_SIZE is 4 instead of 1, which is unnecessarily constraining (at least for nRF52840).
    • Not ideal: The storage slice has type [u32] instead of [u8], which optimizes for writes instead of reads.
  2. Fix most of the issues. This is a breaking change because it changes the READ_SIZE from 4 to 1 and adds NvmcError::OutOfBounds. But those changes seem needed.
  3. Fix the remaining issues. This is a more controversial breaking change. It simplifies the implementation a bit more but is not needed (although it could improve the read performance significantly which is probably the most common operation).

@diondokter
Copy link
Contributor

This PR looks pretty good to me! Solves some real problems the API currently has and I would love to see it merged

@jonas-schievink
Copy link
Contributor

This looks good, thank you!

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 4, 2022

Build succeeded:

@bors bors bot merged commit 6b1c234 into nrf-rs:master Feb 4, 2022
@ia0 ia0 deleted the better_nvmc branch February 5, 2022 08:50
@Robbe7730
Copy link

Since some of these bugs made Nvmc pretty much impossible to use, are you planning to make a new release with these changes?

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.

None yet

4 participants