Relax the MultiwriteNorFlash to only need clearing of words
#63
Conversation
korken89
force-pushed
the
relax-miltiwritenorflash-bound
branch
from
798be4f to
259cfc6
Compare
| }; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct ItemHeader { | ||
| pub struct ItemHeader<S> { |
There was a problem hiding this comment.
I'm not in love with the S here, but it's a good way to achieve the goal. So it can stay
There was a problem hiding this comment.
Yeah I agree, basically all methods had the <S> before so it was a "not better not worse" change. :)
| const DATA_CRC_LENGTH: usize = if <S as NorFlashExt>::WORD_SIZE < 4 { | ||
| 4 // The minimum is 4 to hold the CRC32. | ||
| } else { | ||
| <S as NorFlashExt>::WORD_SIZE | ||
| }; |
There was a problem hiding this comment.
Yeah I can see this work!
However, this punishes people with true multiwrite flashes with >4 wordsize.
I wonder if we can find a way to do the selection based on the implemented trait.
Any ideas?
There was a problem hiding this comment.
Directly I'm having issues comping up with a way.
As the supertrait chain is: NorFlash -> WordclearNorFlash -> MultiwriteNorFlash
I'm getting ambiguity errors when trying to add another DataCrcSize trait that is implemented on different T.
More or less:
trait DataCrcSize {
const SIZE: usize;
}
impl<T> DataCrcSize for T where T: MultiwriteNorFlash {
const SIZE: usize = 4;
}
impl<T> DataCrcSize for T where T: WordclearNorFlash {
const SIZE: usize = if <S as NorFlashExt>::WORD_SIZE < 4 {
4 // The minimum is 4 to hold the CRC32.
} else {
<S as NorFlashExt>::WORD_SIZE
};
}Do you know of any other approach?
There was a problem hiding this comment.
Yeah, that doesn't work yeah.
Like I said in your embedded-storage PR, if MultiwriteNorFlash had a const instead of having an extra trait, this could be made to work.
Except, probably not! Because the ItemHeader is generic over NorFlash, so it still wouldn't have access to that info...
Hmmmm annoying.
Maybe using extension traits to encode this info is the wrong way to do it?
Maybe NorFlash should provide that info directly?
But const generics probably aren't implemented far enough to then have a function with bounds for Multiwrite.
pub trait NorFlash: ReadNorFlash {
const WRITE_SIZE: usize;
const ERASE_SIZE: usize;
const CLEAR_WORDS: bool;
const MULTI_WRITE_WORDS: bool;
fn erase(&mut self, from: u32, to: u32) -> Result<(), Self::Error>;
fn write(&mut self, offset: u32, bytes: &[u8]) -> Result<(), Self::Error>;
}???
This sucks as well...
There was a problem hiding this comment.
Your point in my PR to embedded-storage makes sense though. If the trait had a bool the check can be made.
I'll update to that and give it a try!
There was a problem hiding this comment.
Ah right, that's not very nice either. As then the entire header needs to be bound to MultiwriteNorFlash, which puts unnecessary bounds on reading.
Is this OK in your opinion?
There was a problem hiding this comment.
No, I really want to support non-multiwrite as well
|
On another note, the failing test is also on |
Oh! Thanks for noticing. I'll look at it |
You sure? Just reran it: https://github.com/tweedegolf/sequential-storage/actions/runs/10097189388/job/28315754842 |
|
Ah I see what's happening in the error. There is some fun being had with the mock flash so it can be used in doc tests, but not have it available in the normal API... Let's ignore for now until we have something we can actually merge |
|
Ah there were 2 failures 😅 there is one actual test that is failing and one that is related to doc-comments. |
|
This would enable use of |
Hey! I'd love for RIOT-rs to use this. So this PR is kinda stuck. I'd like to have the ability, but with this implementation it comes at great cost for people on big STM32 chips. An item header is 8 bytes, padded to the next flash word boundary. On big STM32 chips the word size is 32 bytes, so that's already a giant waste of space. With this PR the item header will always cover two words to make it work. That'd mean a 64 byte header! And only 12.5% of it would be used. I can't really accept that. But also I can't fix that with the existing set of traits. So what really needs to happen is:
This only really works if my trait is actually a good idea. For now it remains unproven. This is a non-trivial amount of work, but work I do want to do at some point. Are you blocked on this? |
Not blocked on this, already s-s is awesome enough that we'll use even if esp doesn't work yet! |
Thanks! Can't promise anything but we've got a hackday coming up in two weeks. Might spend my time on this. |
This relaxes the requirement on the
ItemHeaderso flashes that do not allow forMultiwriteNorFlashcan be used.A new bound in introduced which encodes that only clearing of words are required.
This change is a breaking change for flashes that have word sizes > 4 bytes.
For words sizes of 4 or smaller the old behavior is kept.
The new trait resides in the crate for now, but should be up-streamed to
embedded-storage.Up-streaming PR: rust-embedded-community/embedded-storage#57