Design discussion regarding usability issues #23
Comments
|
There is another minor point I forgot. Why mix
|
External flashes need exclusive access to the underlying SPI/QSPI peripheral to do the operation. What's the usecase for "sharing" readonly access to the same flash? Is it to give different "partitions" to different "components"? For that IMO allowing "splitting" a flash into disjoint address ranges would work better, it could allow read/write access to the different partitions, while
The motivation was nrf's QSPI peripheral which can only DMA read at addresses multiple of 4. IMO I prefer for NorFlash impls to be "low level, no automatic magic". We could have a wrapper that turns a
nrf's QSPI DMA needs addr to be multiple of 4. Also, same as above, we could add wrappers that turn
100% agree we need to specify this more, and perhaps make it more fine-grained... Currently NorFlash is either write-once or write-infinite-times (multiwrite) which doesn't accurately model reality... The nrf52832
Do we want to support >4gb devices? 16gb, 32gb SD cards are ubiquitous and dirt cheap and it's quite likely you want to use them in an embedded 32bit device... usize addresses would be awkward because you could support 64bit addrs only in 64bit targets. On the other hand, mixing is weird though. Perhaps addresses and capacity should be u64, and ERASE_SIZE, READ_SIZE, WRITE_SIZE can stay as usize? These will never be as big as the whole capacity and it's common to use them as array sizes which would reduce the need for casts. |
Maybe external flashes need a different trait, see below.
A generic key-value library, e.g. something with the following hypothetical signature: pub struct Store<F: NorFlash> { flash: F }
impl<F: NorFlash> Store<F> {
// Notice the &self instead of &mut self.
fn get(&self, key: Self::Key) -> Self::Result<Option<Self::Value>>;
// Here it's &mut self as expected.
fn insert(&mut self, key: Self::Key, value: Self::Value) -> Self::Result<()>;
// ...
}One cannot implement this signature if
Is the QSPI peripheral to access external or embedded flash? This could be another argument to have different traits (embedded flash could always be seen as external flash though, but not the other way around).
This is fair and I understand why this is important. I still need to continue progress on my library to see if the wrappers I add on top of
Actually an idea I had in the meantime would be a crate documentation instead of a markdown. Flash devices would be structs and properties would be trait. This way we could use // Traits
/// Write only flips selected bits in region from 1 to 0. Erase flips all bits in region to 1. Write region divides erase region.
pub trait Nor {
const WRITE_SIZE: usize;
const ERASE_SIZE: usize;
}
/// Count restriction on Nor writes. A write region can only be written a given amount of time before erase.
pub trait WriteCount: Nor {
const MAX_COUNT: usize;
}
/// Count restriction on Nor writes. Only a given amount of write regions can be written in a row region. Write regions divide row regions which divide erase regions.
pub trait WriteRow: Nor {
const ROW_SIZE: usize;
const MAX_COUNT: usize;
}
// Generic impls
impl<F: WriteCount> WriteRow for F {
const ROW_SIZE: usize = <F as Nor>::WRITE_SIZE;
const MAX_COUNT: usize = <F as WriteCount>::MAX_COUNT;
}
// Flash devices
pub struct Nrf52840;
impl Nor for Nrf52840 {
const WRITE_SIZE: usize = 4;
const ERASE_SIZE: usize = 4096;
}
impl WriteCount for Nrf52840 {
const MAX_COUNT: usize = 2;
}
This sounds again like external flash devices. Another reason to distinguish between embedded and external flashes.
This probably only applies to external flash. For embedded flash, it's probably expected to only support 64-bits addresses on 64-bits targets. I would be surprised to see a chip with more flash that it can address (but I agree that it's theoretically possible, the closest I know is 1M flash versus the 4G addressable in which RAM and registers need to fit too). But then, I'm also assuming that embedded flash devices are memory-mapped. I don't know counter-examples yet. They should go to the taxonomy.
Why not. This sounds like the safest option. Although it only looks required for external flash. |
|
To complicate things even more, STM32F4 series have different sector sizes for different regions (reference page 77). So the Moreover, the sector layout is also dependent on runtime bank configuration: single bank or dual bank. However, to reduce complexity in traits, I think this could be solved by typestates in HAL: two different flash types with different trait implementations. EDIT: |
|
This was discussed before, see #9 (comment) tldr the conclusion was to not support non-uniform sector sizes, it's way too cursed. The HAL can expose an impl covering the whole range with 128kb sector size (merging the smaller sectors), and an impl covering just 0x0800 0000 - 0x0800 FFFF with 16kb sector size, and 0x0800 0000 - 0x0801 FFFF with 64kb sector size |
429: Implement embedded-storage traits r=burrbull a=chemicstry This PR implements [embedded-storage](https://github.com/rust-embedded-community/embedded-storage) traits for flash. One major headache with F4 series is dual-bank flash and non-uniform sector sizes, which required quite a bit of code to abstract away. I went through all of the F4 reference manuals and believe that `flash_sectors(...)` function should be correct for all variants. AFAIK, there is no single source about flash layout for all the chips. Moreover, ST [lists](https://www.st.com/en/microcontrollers-microprocessors/stm32f4-series.html#products) F429 and F469 with 512 KB of flash as dual-bank, but there is no information about it in the reference manual. I believe this could be an error in the website and only 1 MB chips have dual-bank capabilities. The PAC crate is also missing `DB1M` field for some of the dual-bank capable chips, so for now I hardcoded the bit position in `OPTCR` register. The writable `NorFlash` trait was implemented for the largest sector size of 128 KB, because `embedded-storage` does not intend to supprot non-uniform sectors (see [comment](rust-embedded-community/embedded-storage#23 (comment))). Smaller sectors are erased together in the larger 128 KB group. There was a suggestion to add different types for the smaller sector ranges, but I'm not sure if that is useful? Co-authored-by: chemicstry <[email protected]>
Agreed. I've seen the flash abstraction in RIOT that does try for numbered (erase) pages with possibly different sizes, and it's a mess to work with, and some users just rely on a uniform size to be provided anyway. Having per-type size characteristics is a good approach, and whoever really uses inhomogenous memory in a single application can use their own useful abstractions. |
|
There are more properties that may be worth describing:
(Sorry, I don't find the example after having read too many things in the last days, I hope to add them when I find them or if they become more relevant). |
|
It may also be useful to separate the names from the technologies. We're talking of NOR devices in this thread, and the current distinction in embedded-storage is between the full abstraction (that may even perform RMW operations silently) and the NOR flashes. However, there is some ground inbetween: SPI NANDs that behave like the current RmwMultiwriteNorFlashStorage, or like a It may make sense to use a dedicated type for the Infinity variant, but I suggest that either we use more generic terminology than the technologies (given vendors do things like embedding one but emulating the other), or at least declare them as " (This is, I think, orthogonal to the question of whether reads through a shared reference are useful in a device on a shared bus). |
|
I agree that naming with properties is more useful than naming with technologies (that was my initial intention). As I'm making progress on my library (still not presentable but will definitely ping this thread when done), I start to believe that we should maybe not try to over-specify the For more details, here is the approach I'm currently pursuing: Instead of using
The library needs to provide features (or another constant mechanism) to select the properties of the actual flash device. This adds work on the user but can be made minimal by just having to select from a list. The advantage of using |
|
I don't think we should leave any information for out-of-band -- this is where Rust's type system shines, and it can do all we've described so far. When overhauling the abstractions I'm working on I've hoped to get away with less, but to my understanding the bare minimum is:
I had hoped to get away without 3, but even simple use cases like the riotboot bootloader need, like, one overwrite for (in)validation of partitions. For RIOT I'm thinking something simplified like "is one overwrite per page allowed?" (because there is no type stating); in Rust the full In particular, what we can provide even in the library (as #23 (comment) suggested) all the tools to get from, say, a Row(4, 4, 4) to a Row(1, 1, 1) -- I wouldn't want to implement the required buffering for all drivers individually. |
|
I agree with the sentiment. The way I see it the trait can range from full parametric (FP) to common denominator (CD):
I guess a useful trait (there's no perfect solution, just useful ones) would probably lie in between those 2 extremes. To find that point, I think both taxonomies are needed (what can flash devices provide and what do library developers need). And both taxonomies can be adapted: for flash devices we can decide to support less, and for libraries we can improve the algorithms to support more devices. To start the library taxonomy, here are the properties I rely on (for now, it's not finished, and I will adapt as the flash taxonomy grows to be sure to cover as much reasonable devices as possible):
|
|
The nice thing is that we can build upward from CD -- but if CD is to be
really common, we already have to know what a reasonable expectation is,
because CD API will represent the least usable, strictest but at the
same time most exotic device.
For example, you mention non-power-of-2 sizes of erase and write units
-- it wouldn't even have occurred to me that such may exist. If such
exist, CD would describe a device that can be written to in some
device-defined units that may not even be power-of-2. (This may be a bad
example because I know of such flash, it would be highly weird, and
you're not saying they do, just that it's a limitation you place; I hope
we can constrain CD to power-of-2 already).
We can then add traits toward FP as we need them. (Not necessarily as
flash chips provide them: they may provdie all kinds of weird
properties, but what should guide us toward FP is what applications can
meaningfully use -- or we wind up with all kinds of
practically-vendor-specific parameters).
In particular, I need the maximum number of times a region can be erased [...]
That's a property that is good to know, but does this make much
practical sense? AIU these cycle counts are minimum counts (and
practically easily a lot larger than advertised), and at least in
developent situations you can't rely on the minimum either (for it may
have been reprogrammed before).
I think a usable tiering might be (where only those with a `*` matter
initially):
- Readable(*) (As discussed earlier, probably doesn't need its granularity
advertised as that ca be emulated).
- Memory-mapped
- Writable(*) (with some granularity)
- Overwritable(*) (with some number of overwrites per multiple of the
write size)
- Overwrites may be blind writes of zeros(+) (often provided by NAND
flash)
- Overwrites can arbitrarily set bits (provided by block devices)
- Semi-atomicity(+) (no new 1s)
- Full atomicity of writes (any checksummed memory would provide
that)
- Erasable(*) (with some granularity, this-and-that many times)
- Erased memory may be read(+) (returning pattern X, typically 0xff)
- Semi-atomic guarantees(+) (interrupted erases don't set any 0)
- Atomic (old-or-erased) guarantees
- Partially-erased or partially-written-to memory returns
deterministically(+) (more of a marker trait -- formally it depends
on readability, but it only says something useful when combined with
either erasure or writing)
(I'm assuming here that our granularities can indeed always be
power-of-2 -- if not, a non-starred item would envelope the power-of-2
ones)
In addition to those I consider essential, those marked with `+` would
correspond to your check list. To satisfy your "and at most X/Y", these
could be done with cost generic constraints. (As we can't say
`X::EraseSize: < 4096` yet, you'd probably demand `X::EraseSize: 4096`,
and use wrapper types that do upconversions -- or just a static / const
assert, which would also help with the 8 write regions).
While the more exotic traits are brewing, they may not even need to
reside in the original storage crate (although eventually I think they
should, but anyone can define their own extensions and impl them for
some storages that provide them).
|
The way I use it is to provide the library user with lifetime tracking, such that they can notify their own user when the device is close to becoming unpredictable to plan for a replacement (but the device could continue to work with higher risks, it's up to the library user to decide). This is indeed useless for development and only aimed at production devices.
This essentially matches my vision too yes. My concern with such a trait is the complexity/expressivity trade-off. If the trait is too complicated, users might not use it. But apart from that, I would be satisfied with such a trait (assuming it indeed covers many flash devices). Actually I forgot some other properties I rely on:
That's correct.
Currently I do my checks at runtime because Rust is not yet very convenient to use for static assertions, in particular when they involve generic constants and in particular when they are associated constants. But I'm fine with this limitation for now.
Yes, that's where I'm currently going. I define the trait I would like to have in my library and convert from |
|
> [property hierarchy]
My concern with such a trait is the complexity/expressivity trade-off.
Each of these lines would probably be a single trait (depending on its
parents). Users would only see methods they demand in their type
requirements, and for selection we'd need a good overview doc page like
the one in std::collections. (Possibly also traits may be split in
modules -- essentials like read, write and erase could sit in a module
distinct from those around atomicity, which would only be pointed to).
- It is possible to resume an interrupted write operation by writing
the same content to the same region (i.e. repeating the write
operation). This doesn't consume an extra write cycle.
That'd be neat, is that a property storages actually have? (I suppose,
then, that the to-be-written content doesn't need to be exactly the
original one, but only needs to have at least the previously-written
zeros).
…---
So, how do we go on from here? Sketching a demonstration PR with the
more granular traits that subdivide what NorFlash and MultiwriteNorFlash
now express?
What are the stability expectations of embedded-storage, esp. w/rt
generalized names? (Trait aliases are not stable yet, so we can't use
these to carry things over). I don't see a large enough number of
dependents on crates.io that would warrant doing something like
embedded-hal-compatibility does.
|
The problem of doing this is explained in this comment: #22 (comment). Ideally, there would be a single trait to enforce users to write generic libraries. I don't think it's very realistic though. Because even with a single trait like
I don't know. I didn't see any flash device making any useful claims about power interruptions. In particular it's all about probabilities, it may also depend on the usage temperature, etc. But for SLC chips I think it's quite reasonable to assume that.
That on the other hand I'm pretty sure is wrong. I think the stm32l432 specifies this. This is due to the ECC they use. If you write a different content (that is valid with respect to flipping bits from 1 to 0), then the ECC doesn't necessarily respect that property. A simple example is if you try to write |
|
I'm not sure the argument of #22 applies for having split traits. It certainly does apply for memory-mapped readability (so that should not be an extra trait, but could be a provided method of the read trait as you suggest there IIUC). For multiwrite (and also the atomicity guarantees), these would be marker traits with no methods of their own, just maybe with some associated constants. Unlike memory mapped reading, I don't see how those would be selected based on laziness and/or as a premature optimization. And really, if it were to happen, selection for these properties could just as well or even easier happen in a const-generic-typestated setup where one would impl their code
I understand now why that's wrong and how it would fail -- but then, how is the property still useful? The program would need to reconstruct precisely what it did when it attempted that write. Sure, that works for cases when the crash happened during some kind of defragmentation or journal replay (and makes sense there), but for most of the write cases I that data would be lost (after all, most data is written to flash because the program has it in RAM now and wants to be able to get it back later). So yes, I do see use cases, but they are so niche they'd probably need explaining in the docs. |
|
Note to self for further investigation: There are applications that make guarantees on sequential writing nffs's property 2, which indicates there could be embedded storage that requires this. The only piece of hardware I'm currently aware of that behaves like this is SMR (new large hard disks), I doubt that is what was on their minds. |
Very interesting, thanks for the concept. I wasn't aware of this. It's good to know, I can easily adapt my library design for this. I already write in a sequential manner except for one block (my terminology for a write region), but I'll move it at the end of the page then (my terminology for an erase region). |
I've now come across a chip that has this property (STM32WB), but it has a neat extra: It does allow one value (0x0000000000000000 -- yes, it has 64bit flash words) to be written no matter what was previously written. That makes it way more useful. Is that true for the flash you are using as well? In that case, the property of the flash could be "overwrites need to be exactly (Reading that memory comes with its own challenges due to an NMI firing when an incorrectable read error is encountered, like I'd expect it to happen when power is cut mid-write -- but that's a different topic.) |
I think this goes for all STM32's? I am actively using this property in my bootloader at least (stm32l475). |
|
I think we should distinguish the following 2 properties and not merge them:
Those properties are different because the first doesn't tell anything about power-loss while the second addresses exactly that point. Note that constructors usually don't try to specify what happens with power-loss or often just say "anything can happen". This is not really useful to build products on top of such hardware. So those properties may not come from constructors but just be assumptions/hypotheses instead. At least, I'm documenting such assumptions in my code. |
|
If after having "no rewrites unless it is with 0" (possibly "or identical") there is still a use case around for "no rewrites but identical doesn't count", sure we can have both. (I had hoped the former would suffice, but maybe not). |
STM32L0 have smaller granularity and no ECCs, so it varies at least by the first digit of the chip number. |
Yes there is still a need, see the examples I gave in the first post (all nRF52 essentially permit multiple writes and flip 1s to 0s, the next write doesn't even need to be the final value, you can write a 1 on a 0 and the value will stay 0). |
|
I would like to drop here my remarks around having
|
Hi all,
I'd like to list a few issues for those trying to write libraries on top of the abstractions provided by this crate. I'll only focus on
nor_flash::ReadNorFlashandnor_flash::NorFlashsince those are the only traits I need and thus have looked at. I'd be happy to propose an alternative to those traits with a demonstration as to how to use them (i.e. write a generic library on top of it) as well as how to implement them (i.e. how a chip can provide the trait). But before I start this effort, I would like to know if there are any strong reasons as to why those choices have been made.Some term definition to be sure we understand ourselves:
<T: NorFlash>as argument somewhere).Why
read(&mut self)instead ofread(&self)?I couldn't find anything about it in the documentation and code. Because of this design, the user can't share the object for read-only operations which is quite counter-intuitive. One would need interior mutability to work around it. But then why shouldn't the implementation do this instead of the user?
Note that this is related to direct reads as having
read(&mut self)prevents reading until the slice of the previous read is dropped.Do we need
READ_SIZE?There's already some discussion in #19. I would argue the same way as for
read(&mut self)and say that the implementation should take care of that, not the user. Some helper functions could be provided by this crate to help implementations translate from a coarse read to a fine read. I can write this helper as part of the demonstration.Do we need
write()to beWRITE_SIZE-aligned?(Note that we still need to expose
WRITE_SIZE.)Similar to the point above, the implementation could take care of this. And it's a user error to write more than
WRITE_COUNTtimes to aWRITE_SIZE-aligned region (see taxonomy below), because the implementation can't possibly track/enforce this for all types of flash without shadowing the whole flash.Flash taxonomy
This is not a usability issue per se, but I believe this to be critical to design those traits. I'm thinking about a markdown in this crate that could look like:
Technology:
ERASE_SIZE): The flash can be written by flipping bits from 1 to 0. To flip back from 0 to 1, the flash must be erased. The user can choose which bits are flipped from 1 to 0 during write, while erasing flips all bits back to 1 in anERASE_SIZE-aligned region.Read:
NorWrite (Nor only):
MIN_WRITE_SIZE,WRITE_SIZE,WRITE_COUNT): The user can only writeWRITE_COUNTtimes to the sameWRITE_SIZE-aligned region.MIN_WRITE_SIZEdividesWRITE_SIZEandWRITE_SIZEdividesERASE_SIZE. The user only needs to set to 0 the bits they want to set to 0. Other bits could be set to 1 and they will preserve their existing value. The flash can't write less thanMIN_WRITE_SIZE.Add other specifications here.
Examples (all units are in bytes):
nRF52840: Technology::Nor(4096), Read::Direct, NorWrite::Row(4, 4, 2)nRF52832: Technology::Nor(4096), Read::Direct, NorWrite::Row(4, 512, 181)STM32L432KC: Technology::Nor(2048), Read::Direct, NorWrite::Row(8, 8, 1)The text was updated successfully, but these errors were encountered: