rust-embedded-community/embedded-storage

An Embedded Storage Abstraction Layer

embedded-storage's Issues

Design discussion regarding usability issues

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)

RmwNorFlashStorage requires aligned reads

The Storage trait does not expose any alignment information, and one would thus assume that implementations of it would allow for unaligned reads, but RmwNorFlashStorage currently does not. As an example, the following code snippets fails to run:

use embedded_storage::nor_flash::*;
use embedded_storage::ReadStorage;

/// A fake storage driver, that requires reads to be aligned to 4 bytes, and which will fill all of them with 0xFF
struct StrictApi;

impl ErrorType for StrictApi {
    type Error = NorFlashErrorKind;
}

impl ReadNorFlash for StrictApi {
    const READ_SIZE: usize = 4;

    fn read(&mut self, offset: u32, bytes: &mut [u8]) -> Result<(), Self::Error> {
        let offset = offset as usize;

        if offset % Self::READ_SIZE != 0 || bytes.len() % Self::READ_SIZE != 0 {
            Err(NorFlashErrorKind::NotAligned)
        } else {
            for byte in bytes {
                *byte = 0xFF;
            }
            Ok(())
        }
    }

    fn capacity(&self) -> usize {
        8
    }
}

// Only required for RmwNorFlashStorage::new
impl NorFlash for StrictApi {
    const WRITE_SIZE: usize = 4;
    const ERASE_SIZE: usize = 4;

    fn erase(&mut self, from: u32, to: u32) -> Result<(), Self::Error> { unreachable!() }
    fn write(&mut self, offset: u32, bytes: &[u8]) -> Result<(), Self::Error> { unreachable!() }
}

fn test_read_unaligned() {
    let mut buffer = [0x00; 4];
    let mut storage = RmwNorFlashStorage::new(StrictApi, &mut buffer);

    let mut my_buffer = [0x00; 1];
    storage.read(3, &mut my_buffer).unwrap();
    assert_eq!(my_buffer[0], 0xFF);
}

Scope of embedded-storage

I wanted to start a discussion about the scope of embedded-storage.

It could be one of the following:

• Define a common interface between device drivers and generic storage implementations (e.g. filesystems) like embedded-hal.
  That way things like LittleFS on any flash chip with an embedded-storage compatible driver would be possible. That's basically what NorFlash currently does.
• Define an traits for generic, simple, easy to use, byte addressed storage that could be implemented based on any technology. That is currently the Storage trait.
• Provide most commonly used storage implementations based on the traits above.

In my experience with Rust most crates contain either traits to act as a common interface or implementations of other crates' traits. That would mean that the proposed NorFlashStorage of #9 would be better off in a separate crate. I have no strong feelings about this, this is just my observation and I wanted to start a discussion about this so we are all on the same page what the goals of embedded-storage are. It may even be a good idea to start with one crate to easily experiment and separate them later.

At the moment I am not fully convinced of the Storage trait's usefulness. I think traits are only useful if you plan to use it in a library that should be independent of the implementation. I don't really see that for Storage, because it seems to be geared towards the application which can just as easily use the concrete implementation rather than the trait.

Of course it may be that one of you had a specific use case in mind that I am missing and that is why I wanted to have a conversation about it.

To use nb or not to use nb

Quote from @MathiasKoch in #9:

How do you guys feel about nb::? Should these functions be nb? i am not sure it makes sense, unless implementors spend a lot of effort on stuff like DMA?

Let's discuss this here for clarity.

Personally I am unsure about this. On the one hand writes can be extremely slow, so having some kind of asynchronicity would be nice and it is just as easy for implementors to write synchronous code with nb.
On the other hand it is a little bit more effort for users, because they have to wrap everything in block!(...) if they want it to be synchronous and I imagine it is extremely hard to write an agnostic asynchronous driver. So I am not sure if this possibility will be used in practice.

Maybe we could add a nb variant later alongside the current version. Or we could change it to nb now and provide blanket implementations that just call block!().

Type of addresses: mix of u32 and usize.

The WRITE_SIZE, ERASE_SIZE etc are usize:

embedded-storage/embedded-storage-async/src/nor_flash.rs

Line 7 in ef20738

const READ_SIZE: usize;

But the addresses are u32:

embedded-storage/embedded-storage-async/src/nor_flash.rs

Line 20 in ef20738

fn read<'a>(&'a mut self, offset: u32, bytes: &'a mut [u8]) -> Self::ReadFuture<'a>;

This makes doing math on addresses quite annoying: it needs a lot of casts. I sort of lean towards unifying this on usize, since slice lengths are usize and we can't change that.

Erase value on nor flash

It would be really useful if the erase value of a flash was exposed on the NorFlash trait(s). Maybe with a default of 0xFF, something like:

trait NorFlash {
    ...
    const ERASE_VALUE: u8 = 0xFF;
}

Secure storage

Would a "secure storage" abstraction fit in this repo? My initial use case is something rather high level to store and retrieve secrets by a generic key but happy to hear more ideas of what this kind of API should look like.

Read-only storage support

Regarding the read-only storage trait split discussed in #9, I have two use cases:

1. Read-only memory: For chips that really cannot be reprogrammed after leaving the factory, it would be nicer if any attempt to do a write() would simply not compile, rather than adding some kind of UnsupportedOperation error variant (which now you need to handle) or panicking at runtime.
2. Lockable memory: For chips that provide lockable memory, initially the driver could provide an struct implementing only ReadOnlyStorage.
   2a. Then on an unlock() driver method, it can transform that into one implementing the full ReadWriteStorage.
   2b. When done, another lock() method can transform that into a ReadOnlyStorage.

In several drivers I have written a somewhat similar implementation of (2), offering only some of the operations depending on the mode. See for example this driver.

Advantadges

• Support ROM natively.
• Impossible to run the wrong operation for the mode the device is in as the code will not compile.
• Skip that OopsWrongMode error variant or panics and any runtime handling thereof.

Disadvantadges

• Locking/unlocking dance is more complicated than when just returning an error, especially around error recovery.
• Devices/setups where the memory does a reset independently of the software need to be modeled correctly by the driver/app or cannot use the ReadOnly version, depending on how the system works.

PS: Please feel free to choose different names.

Add this crate to awesome-embedded-rust

In order to attract people with interest in the matter.

Example of NorFlash::Region

@Sympatron

I am a bit unsure on what your initial idea behind type Region: NorFlashRegion was, and how you intend it to be used?

Could you come with an implementation example?

It seems like this fn regions(&self) -> Vec<Self::Region, U4>; becomes rather unusable with the restriction on the type above?
What would these 4 regions potentially be? The only way i can see it usefull would be an enum? Just wanted to confirm with you, that it was indeed you original idea.

Consider making capacity async

The function capacity on the async ReadNotFlash trait is not async: https://github.com/rust-embedded-community/embedded-storage/blob/master/embedded-storage-async/src/nor_flash.rs#L18

I'm trying to make a shared flash based on these traits and to do that I have an async mutex that needs to be locked. But I can't do that in a sync function.

It would also be solved by having the capacity be a constant. But there may be a good reason why it's not a constant already

Relevant Matrix discussion

First up

For reference around the current implementation of this crate: rust-embedded/embedded-hal#248

Matrix discussion based on this:

I just wanted to anchor this discussion here, as i think it is very relevant for this repo:

@MathiasKoch

Just circling back to my storage traits from before, do you have any suggestions to make the erase function more usable? I think my main problem is that i have no knowledge of the erase resolution of the implementor..

@Dirbaio

dunno, maybe have it take an address range
and return error if it can't erase that range
ie if flash has 4kb pages, start and end should be page-aligned

@MathiasKoch

Yeah, that was my suggestion as well. I guess it could work, but it kinda runs into the same "alignment" issues

@Dirbaio

yeah :(
and maybe add a funcition where the user can request what's the erase size
but to complicate it more, there are flahes with multiple erase sizes..

@MathiasKoch

Yeah

@Dirbaio

mx25r6435f can erase 64kb, 32kb or 4kb
and doing a 64kb erase is faster than 16 4kb erases

@MathiasKoch

maybe a const EraseSizes: [u8] as an associated const?

@Dirbaio

and I've seen somewhere a flash that had differently-sized blocks
can't remember where

@MathiasKoch

Yeah, i have one of those :p

@Dirbaio

but like
it has N 4kb blocks and M 16kb blocks
and the 4kb blocks must be erased with a 4kb erase and the 16kb blocks must be erased with a 16kb erase
so you can't even say the flash has an "erase size" :D

@MathiasKoch

Ahh.. true!
Cause the traits i have currently suggested should support differently-sized blocks fully, but at the expense of only having an "erase all"..
That might be okay though, as write can be implemented to check and erase rather easy, due to the added iterator helpers
ie

fn try_write(&mut self, address: Address, bytes: &[u8]) -> nb::Result<(), Self::Error> {
    for (block, page, addr) in self.memory_map.pages().overlaps(bytes, address) {
        let merge_buffer = &mut [0u8; MAX_PAGE_SIZE][0..page.size];
        let offset_into_page = addr.0.saturating_sub(page.location.0) as usize;

        self.try_read(page.location, merge_buffer)?;

        if block.is_subset_of(&merge_buffer[offset_into_page..page.size]) {
            self.write_bytes(block, addr)?;
        } else {
            self.erase_page(&page)?;
            merge_buffer
                .iter_mut()
                .skip(offset_into_page)
                .zip(block)
                .for_each(|(byte, input)| *byte = *input);
            self.write_bytes(merge_buffer, page.location)?;
        }
    }

    Ok(())
}

@Dirbaio

not sure if implementing "write" with "read modify write" is a good idea
if you're writing a single word in a page, you'd expect that if the device powers down mid-write, only that word will become garbage

@MathiasKoch

Well, it's a necessity some places.. NOR flash eg

@Dirbaio

but if the driver does RMW behind your back, the entire page can become garbage
if you as a user know the page is erased it's OK to write word-by-word
and you can even write the same word multiple times, the result is the bitwise AND
the nrf52 internal flash explicitly allows this, for example
not sure if the trait should abstract that
or maybe there's room for a trait that does and a trait that doesn't hahaha
my firmware uses a key-value flash store that requires the flash hal to allow word-writes
it's really hard (impossible?) to atomically write records otherwsie

@MathiasKoch

I don't think the trait itself should, but the implementation should.. So when implemented for a NOR flash i think it makes sense to do so..
That said i agree with you, it probably shouldn't for an internal flash.
If it's not that way, i think we loose the whole point of embedded-hal traits (the drop-in replaceability)

@Dirbaio

if the impl does RMW, it becomes useless for many usecases such as that one

@MathiasKoch

But if a driver author can't call try_write with a byte slice, and expect it to be written to "some" storage, without having knowledge of which kind of storage that is (NOR flash, NAND flash, eeprom, internal flash etc), i think the traits become utterly useless.

Eg a simple try_write to a NAND flash, should probably contain nothing but alignment checks and a write (atomically?), but the same write to a NOR chip, would have to do RMW, as you cannot write high bits

@Dirbaio

maybe then have a "NorFlash" trait specifiying "erase sets everything to 0xFF, write does AND"
and a "Flash" trait specifying "write overwrites whatever was before. Careful that it may do RMW if the device is a NOR flash"

@MathiasKoch

And how would i target those traits as a driver author?

@Dirbaio

and even a generic impl that gives you a Flash from a NorFlash
well if you're writing a driver for a NOR flash, implement NorFlash
and let the user use the generic adapter to Flash if he wants RMW..?

@MathiasKoch

Nono, the highlevel user author.. just wanting some non-volatile storage
Sorry, might not be "driver"..
middleware author?

@whitequark

is it a good idea to have a trait for all kinds of flash?
even further, is it a good idea to have traits specifically about flash?

@MathiasKoch

I don't think it is, i would much rather just have a single "storage" trait of some kind

@Dirbaio

I'm halfway done writing a "key-value database" for NOR flash
that specifically takes advantage of NOR flash properties for better efficiency
(for example, to delete a record I overwrite its magic header with another magic header that just changes 1s to 0s)
that would definitely benefit from a "NorFlash" trait
so you can run it on top of any NorFlash

@whitequark

oh yeah that makes sense

@MathiasKoch

Hmm..

@Dirbaio

an alternative would be a "BlockDevice" trait
where you can only overwrite full blocks
and if the write gets interrupted the block can be garbage (ie writes are not atomic)
that's like the "block device" concept of Linux
and yet another alternative would be a generic "Storage" trait
that does RMW
but doing RMW makes it quite useless for writing databases / filesystems, because you can't know how much data can be trashed by an interrupted write

@MathiasKoch

I think my point would be that i would like to see some "default"/super-trait that is basically a "give me some non-volatile storage" trait. That said, i think it could be made up of "sub-traits" with default implementations. That way a crate using the special features of eg NOR, can have a bound on only Nor, but crates that just want any storage can bound on the "super" trait?

@Dirbaio

yeah that makes sens
highlevel "storage" trait
lowlevel "NorFlash" trait
and if you're writing a driver, you just need to implement NorFlash

@MathiasKoch

Yeah, and a default impl Storage for T where T: NorFlash that "adds" RMW

@Dirbaio

the hal could have a NorFlashStorage struct that takes a NorFlash and impls Storage
impl Storage for NorFlash would work too, but then drivers can't have their own Storage impl if the hardware has some faster way or something

@MathiasKoch

Hmm... Specialization where are you? :p

@Dirbaio

hahaha

@whitequark

it's unclear if "give me some non-volatile storage" works in general
though maybe i'm pessimistic
do you really want a completely transparent FTL?
sure, thumbdrives exist, but they also fail in really bad ways because they're fully transparent

@MathiasKoch

I don't see why it shouldn't? Of course with the tradeoff that there will be lots of cases where the result is sub-optimal
FTL?

@whitequark

flash translation layer

@MathiasKoch

ahh

@Dirbaio

but even these FTLs are block-level, not byte-level
they let you overwrite a block, not a byte
512b / 4kb

@whitequark

FTLs get pretty weird
there are flashes with subpage writes for example
even if you limit yourself to only "simple" ONFI NAND configurations, it's still not exactly block level
Emil Karlson
there is no reason you could not have byte addressable FTL, most nand FTLs expose smallest logical write size smaller than smallest physical write size

@whitequark

because the RMW unit is larger than the block

@Dirbaio

very unfun

@whitequark

of course it's technically possible to have an unifying trait that abstracts over all kinds of flash
in API terms
the issue is that you need different algorithms to work with those, so having this API might not be very helpful
can you put plain FAT on NOR flash? yes but since the block erase times are really high you might not like the result
personally what i would do is define kinds of high-level storage (filesystem, key-value, etc) and implement that directly on top of various kinds of flash
because if you know what the end use looks like, even approximately, you can actually use flash in a way that provides nice results
klob left the room.

@whitequark

like i said, it's obviously technically possible, the issue is that you might not want to actually use the result
you say that most NAND FTLs expose smaller logical write size than the page size, that's true
but it's done for compatibility and actually using that feature leads to write amplification

@jschievink

I could see value in a trait for NAND Flash access without an FTL

Allow different address types

As of version 0.1.0 (coming from #12) addresses are fixed to u32, however, this is inconvenient for devices with very small small (wasteful) or very big (> 4GB) capacity (cannot reach all address space).
We should revisit this.

Support defmt?

Should we add support for defmt via a feature flag?

Support for non-NOR (or subtly different) storage styles

Currently, embedded-storage contains the NOR traits and the byte-addressed Storage (which is risky to use as it has unbounded RMW impact on system failure).

I'd like to track the need and requirements for other storage kinds in this issue:

• Block storage := storage with a single page size that can be overwritten arbitrarily.

  This can be expressed as NorFlash (by setting WRITE_SIZE = ERASE_SIZE), but that is not ideal because a) it necessitates an extra erase call, and b) it may amplify writes to the backend due to the requriement that erased areas are all ones.

  If we prefer not to have a dedicated trait, these downsides could also mitigated by having a write_page function that acts on the erase size, is provided to run an erase and a page write, but may be implemented more efficiently by just issuing an "overwrite page" command. Such implementations may still need to costily implement the erase command with a overwrite-with-all-ones, but users that "think" block-wise (or users with highly generic code that detect that WRITE_SIZE = ERASE_SIZE) can avoid calling that ever.

  SD cards fall in this category.

• Byte storage := block storage with a size of 1.

  This could freeload on whatever we do with block storage.

  EEPROM falls in this category.

• Flash memories with limited multi-write.

  Some chips such as the EFR32XG1 allow words to be written only a limited number of times (eg. twice per 16-bit word as in EFR32XG1, nWRITE=2 times per 32-bit word as in nRF5340 and nRF52840, or nWRITE,BLOCK=181 times per erase page as in the nRF52832)

  This could be expressed in a parametrized NorFlashLimitedMultiwrite trait; the downside being that using that trait on a MultiwriteNorFlash would likely mean going through a newtype: while all arbitrarily multiwrite flashes do satisfy any NorFlashLimitedMultiwrite constraints, chances are nobody will impl<const NWRITE: usize> NorFlashLimitedMultiwrite<NWRITE> for TheirType {}.

• Flash memories with error correction.

  While those look like they fit the NOR API, there are two stages of not fitting in:

  • Some (like STM32H5) have relatively transparent error handling.

    These can be expressed with the NOR API, but can't do MultiwriteNorFlash (which is convenient because they couldn't uphold its guarantees in the power loss case).

    They might profit from extra APIs that indicate ECC failure, but can reasonably implement the regular interface by either panicking (it's an error the user has not anticipated) or even hard faulting (which AIU is the default behavior unless ECC errors are handled).

  • Some (like LPC55xx as explained in their knowledge base) have an erase state that is not ffffffff. It may be possible for those to emulate the NOR flash API, but only at relatively high cost of doing extra checks for every read.

    As the data sheet UM11126 is unavailable and none of the descriptions are fully clear, I'd hold off proposing traits for that until anyone around has actually worked with this flash to confirm.

• Memories that are known to go bad (even when not being written to during power loss), and need bad blocks handling.

  Never worked with those so can't tell.

Publish this crate

Even though many breaking releases are done later on, it would be good to have something published.
Once a release is done, we can properly track changes in the changelog.

Stronger focus on storage

First off, I am glad this finally got it's own repository so progress can be made in this area.

I would vote to remove most of the traits currently in this crate or at least move them to a sub-module, because they are not directly related to storage.

• BitSubset (almost) completely unrelated IMO.
• I don't think Add<Address> should be implemented for Address. Semantically this makes no sense. Add<isize> and Sub<usize>/Sub<isize> would be useful on the other hand.
• Region is related to storage, but I am not entirely convinced it has to be a trait, except you want to support disjoint regions?
• I have to admit I don't fully understand the purpose of OverlapIterator, but I don't really see the value for users of this crate. Please correct me if I am wrong.

Specialized traits per storage type

Currently we have one trait ReadWrite that aims to be a general transparent storage with the exception, that erase is only supported for aligned addresses.

I think it may be useful to have a trait that is supposed to be completely transparent and solves all challenges of the actual storage medium under the hood. This trait should not require alignment for any operation IMO.

Additionally there should be traits for specific kinds of storage that share common properties. For example NOR flashes always AND while writing and only support block aligned erase. So having a NorFlash trait (and others like NandFlash, Eeprom, etc.) which can be used to write generic file systems optimized for specific storage types, would be extremely useful to have. This basically the same conclusion @MathiasKoch and @Dirbaio came to in #1.

If I find time today, I'll have a go at NorFlash, but I wanted to see what you guys thought.

Should read offset be expressed in the same terms as write offset?

Shouldn't the read offset be expressed in the same terms as the write offset and be a multiple of READ_SIZE?

See https://github.com/rust-embedded-community/embedded-storage/blob/master/src/nor_flash.rs#L14

Examples needed and a PR in-flight

Thanks for this initiative.

I'd be keen on seeing some examples or at least references to these traits having been implemented. Meanwhile, I have a PR in progress for nrf-hal which uses them. I thought you'd like to know. :-)

nrf-rs/nrf-hal#337