Remove need for generic array and typenum constants #101
Conversation
This will also allow for more flexible `Storage` implementations that use `Vec<u8>` as a cache buffer and are not fixed to a single block size
|
Since the default values were mostly targetted for our lpc55 use case and made it annoying to debug the nrf version that has a smaller block size, I wanted to make it easier to switch between the two. This can be useful for a lot of host tooling that could not be made previously. |
src/driver.rs
Outdated
| } | ||
|
|
||
| /// Safety: implemented only by `[u8; N]` and `Vec<u8>` if the alloc feature is enabled | ||
| pub unsafe trait Buffer: private::Sealed { |
There was a problem hiding this comment.
AFAIS the Filesystem implementation only uses with_capacity and the as_ptr/as_mut_ptr methods. Why do we need resizing as part of the trait?
src/fs.rs
Outdated
| impl<S: driver::Storage> FileAllocation<S> { | ||
| pub fn new() -> Self { | ||
| let cache_size: u32 = <S as driver::Storage>::CACHE_SIZE::to_u32(); | ||
| pub fn new(cache_size: usize) -> Self { |
There was a problem hiding this comment.
I would prefer taking a &Storage to ensure that the correct value is used.
There was a problem hiding this comment.
That could lead to UB with safe code.
The config of the littlefs struct is only defined in the initial allocation and includes the size of the buffer. This is defined by a safe function, it's output could potentially change during execution, while the filesystem is opened, so we need to cache the value and each time we do something with an allocation we must check that the length of the buffers is correct (I can remove the need for assert by instead requesting the correct length for the buffer at that point.
There was a problem hiding this comment.
Sorry, that was a typo – I meant &Filesystem<_, Storage> (just like File::allocate).
There was a problem hiding this comment.
I removed the need for the cache_size here and made the length of the buffer be set on usage of the allocation, where we're already in a "faillible context".
src/driver.rs
Outdated
| fn set_capacity(&mut self, capacity: usize); | ||
|
|
||
| /// Can panic if `capacity` > `Self::MAX_CAPACITY` | ||
| fn with_capacity(capacity: usize) -> Self; |
There was a problem hiding this comment.
Shouldn’t this be fallible so that invalid configuration leads to an error instead of a panic?
There was a problem hiding this comment.
I changed this to make the function private as part of Sealed, and Buffer is "publicly empty", that way there is no risk of public use of this.
I'm also documenting the fact that this can panic from the definitions in the Storage trait.
There was a problem hiding this comment.
Made it fallible.
|
I think this is the right approach to get rid of the configuration constants. What do you think about using a |
|
That does seem better overall but on the other hand it does not allow for default values, which we make use of. One could argue it's better anyway because users should be aware of default values. It also means that we can't add new fields with default values without it becoming a breaking change |
src/driver.rs
Outdated
| pub trait Sealed { | ||
| /// The maximum capacity of the buffer type. | ||
| /// Can be [`usize::Max`]() | ||
| const MAX_CAPACITY: usize; |
There was a problem hiding this comment.
AFAIS this is not used currently. Cache::new could use it to validate the sizes, but I still think it would be the easiest solution to remove this constant and make with_len fallible.
src/driver.rs
Outdated
| /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, | ||
| /// its last call defines the minimum number of valid bytes |
There was a problem hiding this comment.
This should refer to with_len instead of set_capacity.
| /// its last call defines the minimum number of valid bytes | ||
| fn as_ptr(&self) -> *const u8; | ||
| /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, | ||
| /// its last call defines the minimum number of valid bytes |
src/driver.rs
Outdated
| /// Current capacity, set by the last call to [`set_capacity`](Buffer::set_capacity) | ||
| /// or at initialization through [`with_capacity`](Buffer::with_capacity) |
There was a problem hiding this comment.
Needs to be updated for with_len.
sosthene-nitrokey
force-pushed
the
const-types
branch
from
4bd7371 to
09bc4ad
Compare
|
Some changes:
|
This also allows for more flexible
Storageimplementations that useVec<u8>as a cache buffer and are not fixed to a single block size, this is used in thelistexample to allow listing filesystem with varying block sizes without requiring recompilation.