esp-hal icon indicating copy to clipboard operation
esp-hal copied to clipboard

Fix bound checking in the NorFlash::erase implementation of FlashRegion

Open wzhd opened this issue 4 months ago • 4 comments

Submission Checklist 📝

  • [ ] I have updated existing examples or added new ones.
  • [x] I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • [x] My changes were added to the CHANGELOG.md in the proper section.
  • [ ] I have added necessary changes to user code to the Migration Guide.
  • [x] My changes are in accordance to the esp-rs developer guidelines

Extra:

Pull Request Details 📖

Description

address_to could be just past the range, being exactly equal to the end that is exclusive.

Testing

I configured littlefs to use partition size / erase size blocks, and got an OutOfBounds error when it tried to erase the last block.

wzhd avatar Sep 05 '25 04:09 wzhd

Thanks!

Ideally, we would add tests for this but that would require implementing NorFlash for MockFlash

the ! takes precedence so you need braces

bjoernQ avatar Sep 05 '25 06:09 bjoernQ

According to the documentation:

Erase the given storage range, clearing all data within [from..to].

That's a closed-closed interval. Is our current check really incorrect?

bugadani avatar Sep 05 '25 07:09 bugadani

The check_erase helper function only rejects to > flash.capacity(), I think it's consistent with the way ranges are usually passed. Perhaps the documentation isn't using mathematical notation.

wzhd avatar Sep 05 '25 10:09 wzhd

Maybe we should then ask for clarification on the documentation, instead of guessing something and risking doing the wrong thing. This may be a case where the rest of our implementation is wrong, but the documentation is right. If we don't know, we shouldn't change behaviour.

I guess the helper is a good hint, IMO we still should be sure.

bugadani avatar Sep 05 '25 10:09 bugadani