Design discussion regarding usability issues

[ia0]

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::ReadNorFlash and nor_flash::NorFlash since 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:

• I'll call user the component using the trait (i.e. calling the trait functions on an object implementing the trait). I'll consider those users to be abstract (i.e. they are generic over the object and will usually take something like <T: NorFlash> as argument somewhere).
• I'll call implementation the component implementing the trait for a given object (i.e. this is most probably some HAL or something).

Why read(&mut self) instead of read(&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 be WRITE_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_COUNT times to a WRITE_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:

• Nor(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 an ERASE_SIZE-aligned region.
• Add other technologies here.

Read:

• Direct: The flash is memory-mapped and can be read directly from there without any additional mechanism.
• Indirect: The flash is not mapped to memory or needs specific mechanism to read safely.

NorWrite (Nor only):

• Row(MIN_WRITE_SIZE, WRITE_SIZE, WRITE_COUNT): The user can only write WRITE_COUNT times to the same WRITE_SIZE-aligned region. MIN_WRITE_SIZE divides WRITE_SIZE and WRITE_SIZE divides ERASE_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 than MIN_WRITE_SIZE.
• Add other ways to write to a Nor flash here.

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)

[ia0]

There is another minor point I forgot.

Why mix u32 and usize?

Offsets are expressed as u32 while sizes (e.g. WRITE_SIZE or capacity()) are expressed as usize. This forces a fair amount of casts. It seems more natural and coherent to always use usize. By definition, we have offset: u32 ≤ capacity(): usize, so any valid offset should fit usize, rendering the usage of u32 awkward.

The alternative of using u32 everywhere seems a bit arbitrary. Why would u32 be more fit than usize which is related to the architecture and thus the flash?

I guess the exceptions (and thus interesting cases to consider) would be flashes that are not memory-mapped. I don't know any of those, but I also don't know many flashes. I would be interested to get some pointers and see if they can actually address more flash than memory (and should we still consider such huge flashes as embedded?).

[Dirbaio]

Why read(&mut self) instead of read(&self)?

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 read(&self) would work only for reading. This "splitter" adapter could be part of the implementor, or it could be a wrapper that internally shares a single flash with RefCell.

Do we need READ_SIZE? .. the implementation should take care of that, not the user

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 NorFlash<READ_SIZE=4> into a NorFlash<READ_SIZE=1> doing the required "magic", so that it's opt-in. I think in the nRF case this "magic" would require reading into an intermediate buffer from an aligned address, then copying to the final user's buffer, which is not very nice.

Do we need write() to be WRITE_SIZE-aligned?

nrf's QSPI DMA needs addr to be multiple of 4. Also, same as above, we could add wrappers that turn NorFlash<WRITE_SIZE=4> into a NorFlash<WRITE_SIZE=1>, I'd rather not force the overhead onto everyone.

Flash taxonomy

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 512, 181 thing is super cursed... why did they do that!?!?

Why mix u32 and usize?

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.

[ia0]

External flashes need exclusive access to the underlying SPI/QSPI peripheral to do the operation.

Maybe external flashes need a different trait, see below.

What's the usecase for "sharing" readonly access to the same flash?

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 NorFlash::read() takes &mut self without wrapping F into RefCell or Mutex, which the implementor cannot choose. So this must be left to the user (of the key-value library) to choose the appropriate wrapper. The implementor of NorFlash on the other hand has knowledge about the chip (if it's not an external flash, see above) and knows which of RefCell or Mutex makes sense, assuming read can't be implemented with &self in the first place which is probably rarely the case for embedded (or is it called internal?) flashes.

The motivation was nrf's QSPI peripheral which can only DMA read at addresses multiple of 4.

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).

IMO I prefer for NorFlash impls to be "low level, no automatic magic".

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 NorFlash are essentially necessary by any library trying to write a generic data-structure on top of NorFlash or not. Depending on the answer, I would suggest the following options (which actually applies to all other points too):

• Either provide the wrappers in embedded-storage
• Or create a separate crate (e.g. embedded-storage-helpers)

100% agree we need to specify this more

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 cargo doc to render the taxonomy and use existing tools to edit the taxonomy (rust-analyzer for renaming/refactoring, rustc to check for typos, etc). It could look like:

// 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;
}

Do we want to support >4gb devices? 16gb, 32gb SD cards are ubiquitous

This sounds again like external flash devices. Another reason to distinguish between embedded and external flashes.

usize addresses would be awkward because you could support 64bit addrs only in 64bit targets

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.

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.

Why not. This sounds like the safest option. Although it only looks required for external flash.

[chemicstry]

To complicate things even more, STM32F4 series have different sector sizes for different regions (reference page 77). So the ERASE_SIZE is dependent on the address. Would it make sense to have const ERASE_SIZE: &'static [Page]?

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:
Thinking about this more, the &'static [Page] approach would not work for large memories with small page sizes as the array would get too big. Maybe a dynamic iterator approach would work here?

[Dirbaio]

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

[chrysn]

way too cursed

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.

[chrysn]

There are more properties that may be worth describing:

• Erased read value. Some flashes report erased memory as 0x00 rather than 0xFF; this seems to affect singly writable pages (like NorWrite::Row(128, 128, 1)). My impression is that they do some checksumming, the erased page's checksum obviously doesn't work out, and the controller decides to report 0x00.
• Some users of flash need to know whether the backend has NOR semantics to do "blind writes", ie. writing ones that don't turn back any zero. (For example, SPIFFS wants to know whether "blind writes" are OK). It may be that all implementations of this support blind writes, but may also be that there are devices that have sufficiently similar semantics to NorFlash that they'd want to implement this (but would then need to declare that blind writes don't work that way). It may also make sense to acknowledge the existence of such devices, but prescribe that if they don't support blind writes, they have to emulate it by doing extra reads.

(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).

[chrysn]

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 Read::Direct, NorWrite::Row(1, 1, Infinity).

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 "trait ReadX ... with the semantics typically associated with X technology, i.e. property and property" to avoid the confusing situation where something is actually NAND memory but used though a NOR trait.

(This is, I think, orthogonal to the question of whether reads through a shared reference are useful in a device on a shared bus).

[ia0]

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 NorFlash trait. The main reason being that flash devices are too different to fit under the same specification without significantly limiting what the actual device can do.

For more details, here is the approach I'm currently pursuing:

A:    Binary or higher-level library (user)
   ------------------------------------------- Public API of B (me)
B:        Generic storage library (me)
   ------------------------------------------- Internal flash abstraction (me)
C: Generic or custom flash implementation (me)
   ------------------------------------------- NorFlash (embedded-storage)
D:   Flash device implementations (xxx-hal)

Instead of using NorFlash directly, I add another layer of abstraction with the properties that are relevant for my library. I also provide implementations for this abstraction:

• When there's a NorFlash implementation, I also ask the user for the missing information (e.g. the maximum number of times a page can be erased before it starts misbehaving, whether read is &self or &mut self, etc.). This "conversion" is not well-behaved because it adds properties out of nowhere. But the idea is that those properties hold for the actual NorFlash implementation being used.
• For cases where there is no NorFlash implementation or it's broken for some reason, I provide a direct implementation of my trait.

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 NorFlash may seem small but it's actually there. By providing a unique API to access a flash device it factors a lot of code out of the library and leaves only the "configuration" part to the library.

[chrysn]

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:

1. Write granularity -- which blocks need to be set in one go.
2. Whether one may overwrite
3. How often one may overwrite (and how overwrites are counted)

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 NorWrite::Row(a, b, c) can be usable easily.

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.

[ia0]

I agree with the sentiment. The way I see it the trait can range from full parametric (FP) to common denominator (CD):

• FP: Try to capture all properties of all supported flash devices. This is what you would like and that's what I'm doing in my library flash abstraction but only for the properties I need. In particular, I need the maximum number of times a region can be erased, which is currently missing from your list. And I believe, there are other properties we don't think of yet. We need more examples of generic storage libraries and see what properties they rely on. In some way that's the reverse taxonomy. The goal being to bridge the gap between the flash taxonomy and the library taxonomy.
• CD: Only expose the common properties shared by all supported flash devices (think intersection of the above). This is what I understand the NorFlash trait is trying to do. My understanding comes from the fact that this crate direction is to ensure people write portable libraries, i.e. they work for all supported flash devices. However, I'm not sure this is feasible in practice. For example in my library I reject some flash devices (erase region size must be a power of two, within a given range, and a multiple of the write region size, which must also be a power of two and within a given range, etc).

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):

• Erase region size is a power of 2 and at most X
• Write region size is a power of 2 and at most Y
• An erase region can be erased at least once and at most Z
• A write region can be written at least once between erase (I might add improvements if multi-write but that's not a priority)
• Erase operations flip all bits to 1
• Write operations can only flip a selected set of bits from 1 to 0
• There are at least 8 write regions in an erase region (could be improved with multi-write)
• SLC-like properties regarding power-loss (those are probabilistic but the probability is assumed to be high enough):
  • Interrupted erase operation only flipped some bits from 0 to 1 (never from 1 to 0)
  • Interrupted write operation only flipped some of the bits written to 0 from 1 to 0 (never from 0 to 1 or some bits that were not written to 0)
  • Reading the same region without mutable operations in between returns the same result (i.e. floating bits due to interrupted operations give a deterministic value)