nrf-rs / nrf-hal

A Rust HAL for the nRF family of devices
Apache License 2.0
503 stars 139 forks source link

Improve Nvmc implementation of embedded-storage traits #374

Closed ia0 closed 2 years ago

ia0 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

This looks good, thank you!

bors r+

bors[bot] commented 2 years ago

Build succeeded:

Robbe7730 commented 2 years ago

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