Improve FlashStorage documentation #4447
Conversation
esp-storage/src/nor_flash.rs
Outdated
| /// Requirements: | ||
| /// - `offset` must be aligned to a word (4 bytes). | ||
| /// - `bytes.len()` must be a multiple of the word size. | ||
| /// - The bytes being written to must have been previously erased by [`FlashStorage::erase`]. |
There was a problem hiding this comment.
Maybe this should get rephrased to explain how NOR-flash works (i.e. flipping from 1 to 0 is fine, otherwise erase is needed) - the ability to flip bits w/o erasing is the most important reason to prefer https://docs.rs/embedded-storage/0.3.1/embedded_storage/nor_flash/trait.NorFlash.html over https://docs.rs/embedded-storage/0.3.1/embedded_storage/trait.Storage.html
There was a problem hiding this comment.
Good idea, I couldn't think of any case where writing without erasing would be used but it's better to be more specific where possible.
There was a problem hiding this comment.
I couldn't think of any case where writing without erasing would be used
It's common enough in filesystems to keep updating a word over and over again without erasing (like, you can define separate bits in the same byte as allocated, valid, deleted for EEPROM-emulation, for example, and NOR flash allows you to set them from 1 to 0 if the chip does not have ECC).
Dark42ed
force-pushed
the
flash_documentation
branch
from
c215ade to
ff54dfa
Compare
| const WRITE_SIZE: usize = Self::WORD_SIZE as _; | ||
| const ERASE_SIZE: usize = Self::SECTOR_SIZE as _; | ||
|
|
||
| /// Writes `bytes` to the flash storage at `offset`. |
There was a problem hiding this comment.
Documentation is inherited from traits. I think if the trait's docs are insufficient, this should be sent to embedded-storage instead.
There was a problem hiding this comment.
Incidently, clearing up embedded-storage documentation could help close #4094
There was a problem hiding this comment.
Even so, since we are overriding the trait method's documentation I believe a short synopsis is justified for convenience (and because docsrs shows the first line as a short description).
Perhaps a better idea would to put this documentation on the struct and improve the embedded-storage docs?
4 participants
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
As far as I can tell, documentation changes don't require a changelog update? If needed I will add it.
I spent a couple hours attempting to figure out why writing to the flash produced invalid data in some circumstances. I eventually stumbled upon some esp-idf documentation stating that NOR flash can only write bits 1->0, so writes require an erase beforehand. I thought it would be helpful to add it here. While I was at it I also added the alignment requirements for writing bytes.
Additionally I added a hint for people to create a partition for the data to be stored. Ideally this would link to some espflash documentation about partition tables, but I couldn't find any good pages.