Example of NorFlash::Region

[MathiasKoch]

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

[Sympatron]

Some flash ICs are not completely uniform. They have areas that can be erase on 4k boundaries and other areas were you can only erase 16k blocks. So I tried to model the general case there assuming that no flash has more than 4 areas with different erase sizes.
Here is an excerpt from the STM32F205 manual which has a non-uniform internal flash with 3 different regions in this case:

Then I added the UniformNorFlash marker trait to let flash driver writers indicate that their flash is uniform and does not contain differently sized areas. That way users of these flashes can use the methods range, page_size and erase_sizes directly on the flash to make using them more convenient.

If that is to complicated we might decide not to support non-uniform flashes for the moment.

I just reread your question and I think this did not answer your question... 🙃

Hopefully this answers your real question:
I imagined the NOR flash driver implementer to implement NorFlashRegion for the type of flash it supports. Then regions() would easily be usable from the users perspective to get information about the flash. This information would be needed by any kind of flash translation layer or file system based on embedded-storage. If the flash driver is generic over the type of flash chips it supports, it may require the user to provide this information, because it may be difficult (or impossible) to retrieve from the chip at runtime.

Maybe I misinterpreted what Region was meant to be and built on it wrongly/misleadingly.

[eldruin]

Would it be possible to model the interface so that there is no hard 4 erase size regions limit? I'm just thinking if we want to increase it to 5 in the future, that would be a breaking change. Furthermore, 4 seems a bit of a magic number.

[MathiasKoch]

I had a long chat in the embedded-rust matrix channel, mostly with @Dirbaio, trying to work out some elegant traits that would focus on gaining adoption and ease of use, rather than covering ALL edge cases.

Our suggestion is still a bit WIP, but basically it revolves around two key things:

Only support uniform flashes for now

This makes everything much easier, and can still handle non-uniform edge cases if users REALLY want to in two ways.

• Implement the trait once per erase size, and have some helpers to provide the one for a given address/area. This does not easily allow to write accross region boundries though. Above STM32F205 example would be sector 0-=3 of erase size = 16Kbytes, sector 4 with erase size = 64 kbytes etc.
• Implement the traits such that the biggest sector size is the one to use. Above STM32F205 example would for example be all sectors of 128Kbyte, if one wanted to utilize the full range.

These two options can be combined as implementors see fit, but reality is that this will be a very small amount of implementations, as most are uniform.
If we come up woth an elegant way of adding actual non-uniform regions later, it can be added then

Marker trait for MultiwriteNorFlash

What we found is that most people wanting to utilize the more advanced features of a nor flash needs to know if the same page can be written multiple times without an erase in between. For this we came up with a marker trait.

All peripherals implementing NorFlash will:

• Erase to all 1's
• Always only change 1's to 0's through write(..)

Peripherals implementing MultiwriteNorFlash can:

• Write the same page multiple times (still only changing 1's to 0's)

This is required, because some devices (eg. STM32L47x) has an internal ECC that does not allow writing without erasing.

Wrapper types to implement transparent Storage trait

We can provide wrapper types on the form struct NorFlashStorage<S: NorFlash>(S); that implements a transparent JustWorks Storage trait, which only consist of read(), write() and capacity() functions.
In these wrapper implementations the performance at best is equal, and at worst is magnitudes worse that using the type specific trait correctly

Stuff TBD

• Error handling (generic error? impl Into<..>? Basically same discussion as embedded-hal)
• fn minimum_write_size(&self) -> usize; How should this return the minimum size in an intuitive way? ideally it should return u32, u64 etc?

The suggestion so far (still WIP) is:

use crate::Storage;

/// NOR flash trait.
pub trait NorFlash {
	/// An enumeration of storage errors
	type Error;

	/// Read a slice of data from the storage peripheral, starting the read
	/// operation at the given address, and reading until end address
	/// (`self.range().1`) or buffer length, whichever comes first.
	fn try_read(&mut self, address: u32, bytes: &mut [u8]) -> Result<(), Self::Error>;

	/// Erase the given storage range, clearing all data within `[from..to]`.
	/// The given range will contain all 1s afterwards.
	///
	/// This should return an error if the range is not aligned to a proper
	/// erase resolution
	/// Erases page at addr, sets it all to 0xFF
	/// If power is lost during erase, contents of the page are undefined
	fn try_erase(&mut self, from: u32, to: u32) -> Result<(), Self::Error>;

	/// Writes data to addr, bitwise ANDing if there's already data written at that location,
	/// If power is lost during write, the contents of the written words are undefined. The rest of the page is guaranteed to be unchanged.
	/// It is not allowed to write to the same word twice.
	fn try_write(&mut self, address: u32, bytes: &[u8]) -> Result<(), Self::Error>;

	/// The erase granularity of the storage peripheral
	fn erase_size(&self) -> usize;

	/// The minumum write size of the storage peripheral
	fn minimum_write_size(&self) -> usize;

	/// The capacity of the peripheral in bytes.
	fn capacity(&self) -> u32;
}

/// Marker trait for NorFlash relaxing the restrictions on `write`.
///
/// Writes to the same word twice are now allowed. The result is the logical AND of the
/// previous data and the written data. That is, it is only possible to change 1 bits to 0 bits.
///
/// If power is lost during write:
/// - Bits that were 1 on flash and are written to 1 are guaranteed to stay as 1
/// - Bits that were 1 on flash and are written to 0 are undefined
/// - Bits that were 0 on flash are guaranteed to stay as 0
/// - Rest of the bits in the page are guaranteed to be unchanged
trait MultiwriteNorFlash: NorFlash {}

///
pub struct NorFlashStorage<S: NorFlash>(S);

impl<S: NorFlash> NorFlashStorage<S> {
	/// Instantiate a new generic `Storage` from a `NorFlash` peripheral
	pub fn new(nor_flash: S) -> Self {
		Self(nor_flash)
	}
}

impl<S: NorFlash> Storage for NorFlashStorage<S> {
	type Error = S::Error;

	fn try_read(&mut self, address: u32, bytes: &mut [u8]) -> Result<(), Self::Error> {
		self.0.try_read(address, bytes)
	}

	fn try_write(&mut self, address: u32, bytes: &[u8]) -> Result<(), Self::Error> {
		// TODO: RMW
		self.0.try_write(address, bytes)
	}

	fn capacity(&self) -> u32 {
		self.0.capacity()
	}
}

Other stuff

It might be that Eeproms are so simple that they might not need an Eeprom trait? @eldruin Would it makes sense for an eeprom to directly implement Storage without loss of performance? Only read(), write() and capacity() functions.

[eldruin]

@MathiasKoch From what I know from the 24 EEPROM series, that would work just fine. My driver also includes a couple other methods but they are just convenience.

[Sympatron]

@MathiasKoch I think the solution you came up with is good. You are probably right that non-uniform regions are a bit niche and don't necessarily need to be handled by embedded-storage. At least at the moment.

I would add the following:

• NorFlashStorage should have a name that makes it clear that this is a simple RMW storage without anything fancy, because as I said it is extremely inefficient for NOR flash. NOR flash take up to a few hundred ms to erase a sector. If you were to just write byte by byte into NorFlashStorage and it would erase the sector every time it could take minutes to write a few hundred bytes and the flash would be worn out pretty fast. As I said it's not that I don't think this can't be useful to have, but NorFlashStorage as a name is too generic in my opinion.
• I think Storage is a good name for a general storage. Maybe adding a read-only version ReadonlyStorage or ReadStorage might be useful, too. The easiest would be to move try_read() and capacity() into ReadStorage and make Storage: ReadStorage.
• I would like a more generic type for addresses and sizes more than u32. So I would either stick with Address or add an associated type to NorFlash and Storage. This type could default to u32. It's not super important though.
  Why does capacity() return u32 and erase_size() and minimum_write_size() return usize?

[Dirbaio]

Looks very nice!

• Regarding this on read: I think it makes more sense to just return an error on out-of-bounds reads.

  starting the read operation at the given address, and reading until end address
  (self.range().1) or buffer length, whichever comes first

• I'd rename minimum_write_size to write_size, for consistency (the erase size is also a "minimum" erase size anyway)

• on write, specify that addr and bytes.len() must be multiple of write_size

• on erase, specify that from and to must be multiple of erase_size, and from <= to

[MathiasKoch]

NorFlashStorage should have a name that makes it clear that this is a simple RMW storage without anything fancy, because as I said it is extremely inefficient for NOR flash. NOR flash take up to a few hundred ms to erase a sector. If you were to just write byte by byte into NorFlashStorage and it would erase the sector every time it could take minutes to write a few hundred bytes and the flash would be worn out pretty fast. As I said it's not that I don't think this can't be useful to have, but NorFlashStorage as a name is too generic in my opinion.

It can be extremely bad performing, but i think in most cases where a user would just hand it a slice of data to persist, you wouldn't be able to make the performance any better without RMW?
Take an example. Someone want to make a basic key-value store without super-high throughput, think config database? Whenever a save(..) function is called, it should write the data right away (you can't wait for a full page, as there is no indication on when the next save will be called). Any implementation of this, no matter how many bytes saved would require the same RMW steps as the wrapper would make? This doesn't mean it will do RMW for every byte it writes, but rather once for call to write() per page used by the data.

Maybe adding a read-only version ReadonlyStorage or ReadStorage

Personally i am not a fan of splitting this? I don't see what we gain from it, other than more complexity on the users? If you really want a ReadOnlyStorage, implement Storage and return an unsupported/illegal error on write/erase? Thoughts?

@Dirbaio
Absolutely, great additions/corrections. Regarding the write_size, what would make sense to return on that one? number of bytes? in either case i think it might become hard to use? Could you maybe fill our erase_size & write_size for the example of STM32F205 above, sectors 0 to 3?

[Sympatron]

As I said: A simple RMW implementation has it's use cases, like the one you described. And there are certainly cases were it's hard or impossible to do better. I was just saying that it should be conveyed that it's not suitable for arbitrary usage, especially not random writes.
Maybe a name like SimpleNorFlashStorage or RmwNorFlashStorage or something like that, because NorFlashStorage sounds like the be-all and end-all storage type that fits all purposes, which I think is misleading. I know it's a nitpick, but I think naming is important.

[MathiasKoch]

I think it's a fair point 👍 I have no strong naming opinions, so for me it's alright. As long as the code works 😄

Another thing:
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?

[Dirbaio]

I got more edge cases! 🎉

The nrf52 QSPI peripehral can only read in multiples of 4 bytes, even if the underlying QSPI chip can read at any byte offset. 🤦‍♂️

READ.SRC Word-aligned flash memory source address.
READ.CNT Read transfer length in number of bytes. The length must be a multiple of 4 bytes.

This could be addressed by adding a read_size() method to the trait, similar to write_size().

Even more annoyingly, the RAM addresses must also be 4-byte aligned, for both reading and writing. &[u8]s are not necessarily 4-byte aligned.

WRITE.SRC Word-aligned RAM source address.
READ.DST Word-aligned RAM destination address.

I'm very not sure what to do about this one. Options:

• Add a Word associated type: u8 normally, u32 if buffer needs to be 4-byte aligned. This will make using the trait a pain in the ass though.
• Add a buffer_alignment() method: return 1 normally, 4 if the buffer needs to be 4-byte-aligned. read and write panic if the &[u8] is not aligned.
• Do nothing. The nrf52 QSPI impl would specify the alignment requirements in the docs. read and write panic if the &[u8] is not aligned.

[MathiasKoch]

Stop doing that! :p

Regarding the alignment, what we are currently doing is: https://github.com/stm32-rs/stm32l4xx-hal/blob/683c78138871ee39910bfcc944b2b67ba28ff313/src/flash.rs#L240-L318

And then implement these traits ontop of the write fn rather than the write_native, again with the knowledge that it might come with a performance penalty.

[Dirbaio]

Regarding the write_size, what would make sense to return on that one? number of bytes? in either case i think it might become hard to use? Could you maybe fill our erase_size & write_size for the example of STM32F205 above, sectors 0 to 3?

STM32F205 supports byte write, so for sectors 0..=3 it would be

capacity = 64kb
read_size = 1
write_size = 1
erase_size = 16kb

In the L4s with ECC, it would be

read_size = 1
write_size = 8

[Dirbaio]

And then implement these traits ontop of the write fn rather than the write_native, again with the knowledge that it might come with a performance penalty.

Hm... IMO the NorFlash trait should capture the reality of the hardware and expose it with no performance penalty.

There could be a wrapper that turns a NorFlash with write_size=8 into a NorFlash with write_size=1. Users could optionally wrap the HAL impl with it if they want to use it with something that requires write_size=1...