Rework NorFlash & Storage traits #12

MathiasKoch · 2021-03-03T13:27:43Z

Rework NorFlash & Storage traits
Add RmwNorFlashStorage wrapper between them

The RmwNorFlashStorage implementation might be possible to improve? Please consider that as my crude first attempt. It is tested and works though. I am not 100% into the write_size/erase_size usage, so please correct me on those.

I have added an implementation of RmwNorFlashStorage that should be able to utilize MultiwriteNorFlash as well, but it is still untested. Also it requires rust-lang/rust#31844 to live alongside the implementation without multiwrite.

I currently have an implementation for STM32L4xx_HAL if you are interested.

This is my attempt to encapsulate the discussion going on, so please keep it coming!

@eldruin @Sympatron

…between them

MathiasKoch · 2021-03-05T09:37:27Z

I think the latest change her addresses most of the concerns people have highlighted, with some limitations?

capacity() has been left a runtime function, as i think lots of chip families have common read, write and erase sizes, but just change the capacity, making it easier to read capacity at runtime and return that. This might mean that it should actually take &mut self though?

#13 Should also be addressed by this

Stuff left to figure out:

https://github.com/rust-embedded-community/embedded-storage/pull/12/files#diff-d3d015d586aa5b2b1e4ccd99bc9c9e3de7cc4429a30b998d15c3091b10581dd1R125 should make use of S::ERASE_SIZE
The MultiwriteNorFlash wrapper for Storage is still missing. Suggestions on naming?
Type of addresses? u32/usize/Address()
All addresses are normalized from 0..self.capacity() ? Document this without a doubt!
Error handling (generic error? impl Into<..>? Basically same discussion as embedded-hal)

Limitations:

Multiple regions are only supported through multiple implementations: impl NorFlash for Region1 + impl NorFlash for Region2 or similar.
Non-uniform regions are unsupported, but can be implement using biggest region size, and combining smaller regions.

@Sympatron @Dirbaio

Dirbaio · 2021-03-05T13:53:44Z

should make use of S::ERASE_SIZE

To get rid of that error, add where [u8; S::ERASE_SIZE]: Sized

Type of addresses? u32/usize/Address()

For NorFlash u32/usize would be OK, I don't think there are 4GB+ NOR flashes.
For Storage, people will want to do ipmls for SD cards, which can easily be bigger than 4GB.
For consistency it's nice if all traits have the same address type.
It's not nice to force an u64 address on everyone, it's useless overhead on 32bit arches if you don't need it.

Maybe given this the best way to go is make it generic? I'm not a fan of adding even more generic params but maybe it's the best option :S

All addresses are normalized from 0..self.capacity() ? Document this without a doubt!

I'd say yes. Makes using traits harder otherwise, and I don't think it adds anything of value :|

Error handling (generic error? impl Into<..>? Basically same discussion as embedded-hal)

No idea 🤷 I somewhat prefer a non_exhaustive Enum instead of a type Error, for the reasons here. TLDR is drivers using the traits can't really handle the errors otherwise, as they know nothing about the type. The downside is it doesn't allow for custom errors. I personally don't find that a problem but other people have different opinions.

MathiasKoch · 2021-03-08T06:57:07Z

@Dirbaio Hmm, i can't seem to make the where [u8; S::ERASE_SIZE]: Sized work?

error[E0599]: no associated item named `ERASE_SIZE` found for type parameter `S` in the current scope
   --> src/nor_flash.rs:113:9
    |
113 | [u8; S::ERASE_SIZE]: Sized,
    |         ^^^^^^^^^^ associated item not found in `S`
    |
    = help: items from traits can only be used if the type parameter is bounded by the trait
help: the following trait defines an item `ERASE_SIZE`, perhaps you need to restrict type parameter `S` with it:
    |
111 | impl<S: NorFlash> Storage for RmwNorFlashStorage<S>
    |      ^^^^^^^^^^^

error: aborting due to previous error

On the error part how about something like

pub enum Error<E> {
    CommonErrors,
    ...,
    Custom(E)
}
pub trait NorFlash: ReadNorFlash {
    type UserError: Debug;

    fn try_write(&mut self, address: u32, bytes: &[u8]) -> Result<(), Error<Self::UserError>> {
         ...
    }
}

That way it should be easy to just make UserError = Infallible/() if everything is covered by the Error enum. We can still amke the Error enum non_exhaustive to allow adding error handling as we go.

Dirbaio · 2021-03-08T14:48:17Z

@Dirbaio Hmm, i can't seem to make the where [u8; S::ERASE_SIZE]: Sized work?

Do you have the code somewhere? I can try taking a look

…ase sizes. Split traits into Read and ReadWrite versions

MathiasKoch · 2021-03-08T15:12:12Z

https://github.com/rust-embedded-community/embedded-storage/tree/storage-rework

Dirbaio · 2021-03-10T19:59:17Z

Wow, I think it's not possible with the 1.51 min_const_generics. It does work with these features:

#![feature(const_generics)]
#![feature(const_evaluatable_checked)]

An alternative is something like pub struct RmwNorFlashStorage<S, const ERASE_SIZE: usize> (S), then the array works with min_const_generics BUT there's no way to enforce that S::ERASE_SIZE = Self::ERASE_SIZE :(

MathiasKoch · 2021-03-16T13:19:02Z

Hmm, how would i propagate this PR the remaining distance? It seems like all it is missing is

Error handling discussion (Could be postponed to another PR)
Handling that const merge_buffer size in the RmwNorFlashStorage
Enabling the RmwMultiwriteNorFlashStorage after finding a better name for it?

Dirbaio · 2021-03-16T14:44:02Z

No strong opinion on errors, we could leave it as it is now which is the same as embedded-hal, and revisit it later once it's revisited in embedded-hal?

Not sure what to do about the merge buffer. Maybe have the user pass a &mut [u8] and panic at runtime if it's too small?

Is the RmwMultiwriteNorFlashStorage really needed? All MultiwriteNorFlash is also NorFlash, so you can already use the current adapter for all flashes. It might allow for a more efficient implementation (if writing to a range that's all 0xFF no need to erase first), but we can always add that later.

What's the usecase for Region and OverlapIterator now that we've dropped support for multiregion flash?

MathiasKoch · 2021-03-16T14:53:27Z

No strong opinion on errors, we could leave it as it is now which is the same as embedded-hal, and revisit it later once it's revisited in embedded-hal?

Sounds good, that was exactly my point with postponing it

Maybe have the user pass a &mut [u8] and panic at runtime if it's too small

I think that sounds like a decent tradeoff, i'll see if i can make it work in an elegant way tomorrow.

Is the RmwMultiwriteNorFlashStorage really needed

Not necessarily. Personally i wont be needing it, but it sounded like @Sympatron could make use of it, as it does improve the worst-case performance quite a bit.

What's the usecase for Region and OverlapIterator now that we've dropped support for multiregion flash?

Good point! I think they are leftovers from the past.. I'll remove them if they are not needed.

Sympatron · 2021-03-16T15:05:40Z

I agree with @Dirbaio about the errors.

Maybe have the user pass a &mut [u8] and panic at runtime if it's too small

This might be a good idea, but I don't quite see how to get there... Wouldn't you either have to modify the trait or let the storage own the buffer (i.e. provide one with new())?

I think RmwMultiwriteNorFlashStorage is fine for now. It's not ideal, but I can't think of a better one.

Dirbaio · 2021-03-16T15:19:28Z

let the storage own the buffer (i.e. provide one with new())?

Yes, I'd do that. It also opens the possibility of optimizations like coalescing multiple writes?

When writing load the page from flash into the buffer and apply the modifications but don't write it to flash yet. On next writes, if they're to the same page apply them to the buffer. If they're to other pages, write out the previous page first.

The downside is it'd need a flush() method to ensure the last write is actually written out...

MathiasKoch · 2021-03-18T07:40:19Z

Hmm, at first-hand we seem to run into a double mutable borrow at self.try_read(page.start, self.merge_buffer)?;

Dirbaio · 2021-03-18T14:57:46Z

Shouldn't it do try_read on the inner NorFlash, not on self?

MathiasKoch · 2021-03-18T15:06:41Z

That would make sense yes :p Missing the obvious here!

MathiasKoch · 2021-03-18T15:13:12Z

@Dirbaio @Sympatron
Guess that should do it.
We might be able to get rid of the overlapIterator, but both Rmw implementations makes use of it, and is greatly simplified by it.
Maybe we could postpone that cleanup to a future PR if anyone wants to do it?

It could probably do with a bit more doc-comments scattered around, so i will se if i can get around to that.

One thing. Do you guys prefer address, or offset as argument names? I tend to lean towards offset given that we have agreed they are in the range (0..capacity()), and thus no langer is an actual address?

…e atleast EraseSize long. Introduce RmwMultiwriteNorFlashStorage

…sh rather than NorFlash

…rom peripheral base address

MathiasKoch · 2021-04-08T13:00:56Z

@Dirbaio @eldruin @Sympatron
I think this is ready for a final round of review, followed by a 0.1.0 release?

eldruin

Other than my comments, looks good to me! However, I have no experience with NOR.

Cargo.toml

src/iter.rs

src/lib.rs

Co-authored-by: Diego Barrios Romero <[email protected]>

eldruin

Looks good to me! (NOR not reviewed)

Dirbaio

Reviewed NOR flash stuff. Traits look great modulo a few minor doc nits.

Maybe it's best to add the RMW Storage impls in a later PR, so we don't block the traits release?

I also wonder if the RMW impls might be more readable using index loops and copy_from_slice instead of iterators

src/nor_flash.rs

Dirbaio · 2021-04-19T13:33:55Z

src/nor_flash.rs

+			// Check if we can write the data block directly, under the limitations imposed by NorFlash:
+			// - We can only change 1's to 0's
+			if is_subset {
+				self.storage.try_write(addr, data)?;


This may fail if addr or data len is not a multiple of WRITE_SIZE. For example with a WRITE_SIZE of 4 (like on nrf52s) it'd fail if you try to write a single byte.

Fortunately this is possible to do by "padding" to WRITE_SIZE with 0xFFs. For example, if you want to write 0x42 to offset 1, you can do so by writing 0xFF 0x42 0xFF 0xFF to offset 0.

I think this part solves it, if am not mistaken? But it might be possible to do it smarter?

https://github.com/rust-embedded-community/embedded-storage/pull/12/files#diff-d3d015d586aa5b2b1e4ccd99bc9c9e3de7cc4429a30b998d15c3091b10581dd1R225-R238

src/nor_flash.rs

MathiasKoch · 2021-04-20T12:18:29Z

@Dirbaio Thanks for the great comments!

I will address them as soon as i get a moment of time ;)

…mbedded-storage into storage-rework

Dirbaio · 2021-05-07T13:40:36Z

src/nor_flash.rs

+				let offset = addr as usize % S::WRITE_SIZE;
+				self.merge_buffer[..S::WRITE_SIZE]
+					.iter_mut()
+					.for_each(|c| *c = 0u8);


shouldn't this be 0xFF? 0x00 will set all bits to 0, 0xFF leaves them untouched.

Also you could use .fill(0xff) instead of using iterators :)

Same for the copies, you could use .copy_from_slice() which compiles to a memcpy. I don't think the compiler is able to optimize the iterator version to a memcpy.

Dirbaio · 2021-05-07T13:42:25Z

src/nor_flash.rs

+				self.merge_buffer[..S::WRITE_SIZE]
+					.iter_mut()
+					.skip(offset)
+					.zip(data)


Not sure this is correct, data may be bigger than WRITE_SIZE since it's chunked by ERASE_SIZE...

MathiasKoch · 2021-05-18T08:10:39Z

I have extracted the Rmw implementations to their own PR #14.

@Dirbaio @Sympatron @eldruin

Can we merge and release the traits here?

eldruin

Aside for the nitpicks, looks good to me!

eldruin · 2021-05-18T08:16:28Z

src/lib.rs

 	///
-	/// (start_addr, end_addr)
-	fn range(&self) -> (Address, Address);
+	///	This should throw an error in case `bytes.len()` will be larger than


Could you fix the tab usage clippy warning here?

eldruin · 2021-05-18T08:25:14Z

Cargo.toml

@@ -15,6 +15,4 @@ keywords = ["storage"]
 categories = ["embedded", "hardware-support", "no-std"]

 [dependencies]
-nb = "1"
-no-std-net = "0.4"
 heapless = "^0.5"


Can we bump heapless? The latest version (0.7) requires 1.51.0, though, due to const-generics.

MathiasKoch · 2021-05-18T08:33:31Z

@eldruin Good point.

I removed the heapless dependency entirely for this one, as it was only used in the Rmw implementations, and updated it to 0.7 in #14

Update:
Apparently not used by Rmw implementations either, i removed the heapless dependency all together.

Sympatron · 2021-05-18T09:48:14Z

@MathiasKoch I think it is good to go.

MathiasKoch force-pushed the storage-rework branch 2 times, most recently from 1078baa to 7667a56 Compare March 3, 2021 14:00

Rework NorFlash & Storage traits, and add RmwNorFlashStorage wrapper …

3eb1428

…between them

MathiasKoch force-pushed the storage-rework branch from 7667a56 to 3eb1428 Compare March 3, 2021 14:07

Sympatron mentioned this pull request Mar 3, 2021

Scope of embedded-storage #10

Closed

MathiasKoch mentioned this pull request Mar 5, 2021

Example of NorFlash::Region #9

Closed

MathiasKoch force-pushed the storage-rework branch from f9dd1de to adf0f9d Compare March 5, 2021 09:25

Change implementation to use associated consts for read, write and er…

5943283

…ase sizes. Split traits into Read and ReadWrite versions

MathiasKoch force-pushed the storage-rework branch from adf0f9d to 5943283 Compare March 8, 2021 15:12

This was linked to issues Mar 16, 2021

Read-only storage support #13

Closed

Specialized traits per storage type #3

Closed

Example of NorFlash::Region #9

Closed

MathiasKoch force-pushed the storage-rework branch from deec732 to 95a1196 Compare March 18, 2021 15:08

MathiasKoch closed this Mar 18, 2021

MathiasKoch deleted the storage-rework branch March 18, 2021 15:13

Make RmwNorFlashStorage::new take a mutable merge_buffer, that must b…

363bce3

…e atleast EraseSize long. Introduce RmwMultiwriteNorFlashStorage

MathiasKoch force-pushed the storage-rework branch from 95a1196 to 363bce3 Compare March 18, 2021 15:58

Add new function to MultiwriteNorFlash, and bound on MultiwriteNorFla…

931eb63

…sh rather than NorFlash

MathiasKoch force-pushed the storage-rework branch from 1a3571b to 931eb63 Compare March 24, 2021 14:24

Change from address to offset argument naming, as these are offsets f…

ac8492b

…rom peripheral base address

MathiasKoch force-pushed the storage-rework branch from a2fc00c to ac8492b Compare April 8, 2021 12:59

Update nor_flash.rs

4f4ab10

eldruin reviewed Apr 19, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/iter.rs Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

MathiasKoch and others added 2 commits April 19, 2021 13:20

Update src/lib.rs

d69248a

Co-authored-by: Diego Barrios Romero <[email protected]>

Update Cargo.toml

72560ac

eldruin approved these changes Apr 19, 2021

View reviewed changes

Dirbaio reviewed Apr 19, 2021

View reviewed changes

MathiasKoch added 2 commits May 7, 2021 09:17

Address review comments around nor_flash

2cbb985

Merge branch 'storage-rework' of github.com:rust-embedded-community/e…

e02bda5

…mbedded-storage into storage-rework

MathiasKoch force-pushed the storage-rework branch from cb66428 to e02bda5 Compare May 7, 2021 07:33

Dirbaio reviewed May 7, 2021

View reviewed changes

MathiasKoch added 2 commits May 18, 2021 10:04

Change away from iterators

55f8505

Extract RmwNorFlash impl to separate PR

0ca3bed

MathiasKoch mentioned this pull request May 18, 2021

Enhancement: RmwNorFlash #14

Merged

MathiasKoch mentioned this pull request May 18, 2021

Adds in my alternate storage traits #8

Closed

eldruin reviewed May 18, 2021

View reviewed changes

eldruin mentioned this pull request May 18, 2021

Allow different address types #15

Open

Remove unused heapless dependency, and fix indentation

6a67ff2

MathiasKoch merged commit 7d70e13 into master May 18, 2021

MathiasKoch deleted the storage-rework branch May 18, 2021 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework NorFlash & Storage traits #12

Rework NorFlash & Storage traits #12

MathiasKoch commented Mar 3, 2021 •

edited

Loading

MathiasKoch commented Mar 5, 2021 •

edited

Loading

Dirbaio commented Mar 5, 2021

MathiasKoch commented Mar 8, 2021 •

edited

Loading

Dirbaio commented Mar 8, 2021

MathiasKoch commented Mar 8, 2021

Dirbaio commented Mar 10, 2021

MathiasKoch commented Mar 16, 2021

Dirbaio commented Mar 16, 2021

MathiasKoch commented Mar 16, 2021

Sympatron commented Mar 16, 2021

Dirbaio commented Mar 16, 2021

MathiasKoch commented Mar 18, 2021

Dirbaio commented Mar 18, 2021

MathiasKoch commented Mar 18, 2021

MathiasKoch commented Mar 18, 2021

MathiasKoch commented Apr 8, 2021

eldruin left a comment

eldruin left a comment

Dirbaio left a comment

Dirbaio Apr 19, 2021

MathiasKoch May 7, 2021

MathiasKoch commented Apr 20, 2021

Dirbaio May 7, 2021

Dirbaio May 7, 2021

Dirbaio May 7, 2021

MathiasKoch commented May 18, 2021

eldruin left a comment

eldruin May 18, 2021

eldruin May 18, 2021

MathiasKoch commented May 18, 2021 •

edited

Loading

Sympatron commented May 18, 2021

Rework NorFlash & Storage traits #12

Rework NorFlash & Storage traits #12

Conversation

MathiasKoch commented Mar 3, 2021 • edited Loading

MathiasKoch commented Mar 5, 2021 • edited Loading

Stuff left to figure out:

Limitations:

Dirbaio commented Mar 5, 2021

MathiasKoch commented Mar 8, 2021 • edited Loading

Dirbaio commented Mar 8, 2021

MathiasKoch commented Mar 8, 2021

Dirbaio commented Mar 10, 2021

MathiasKoch commented Mar 16, 2021

Dirbaio commented Mar 16, 2021

MathiasKoch commented Mar 16, 2021

Sympatron commented Mar 16, 2021

Dirbaio commented Mar 16, 2021

MathiasKoch commented Mar 18, 2021

Dirbaio commented Mar 18, 2021

MathiasKoch commented Mar 18, 2021

MathiasKoch commented Mar 18, 2021

MathiasKoch commented Apr 8, 2021

eldruin left a comment

Choose a reason for hiding this comment

eldruin left a comment

Choose a reason for hiding this comment

Dirbaio left a comment

Choose a reason for hiding this comment

Dirbaio Apr 19, 2021

Choose a reason for hiding this comment

MathiasKoch May 7, 2021

Choose a reason for hiding this comment

MathiasKoch commented Apr 20, 2021

Dirbaio May 7, 2021

Choose a reason for hiding this comment

Dirbaio May 7, 2021

Choose a reason for hiding this comment

Dirbaio May 7, 2021

Choose a reason for hiding this comment

MathiasKoch commented May 18, 2021

eldruin left a comment

Choose a reason for hiding this comment

eldruin May 18, 2021

Choose a reason for hiding this comment

eldruin May 18, 2021

Choose a reason for hiding this comment

MathiasKoch commented May 18, 2021 • edited Loading

Sympatron commented May 18, 2021

MathiasKoch commented Mar 3, 2021 •

edited

Loading

MathiasKoch commented Mar 5, 2021 •

edited

Loading

MathiasKoch commented Mar 8, 2021 •

edited

Loading

MathiasKoch commented May 18, 2021 •

edited

Loading