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

[huntc]

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

[huntc]

FYI I've implemented nrf-rs/nrf-hal#337 on the assumption that the read offset is a multiple of READ_SIZE. It'd be great to get confirmation on this. Thanks.

[MathiasKoch]

@Dirbaio i think you may be qualified to answer this? 🤷

I have only every used chips with READ_SIZE = 1

[Dirbaio]

Yeah, the current docs are missing specifying that offset and bytes.len() are multiples of READ_SIZE.

However it's true that the majority of chips (all?) have READ_SIZE=1. Maybe we can remove READ_SIZE?

WRITE_SIZE matters especially if the NOR flash is not multiwrite, you don't want impls to do "tricks" behind your back to process unaligned writes because they may write to addresses you don't expect... However for READ_SIZE, if a chip really has a read alignment requirement, impls can read "slightly more" and then give you the subset of bytes, which is fine because reads aren't suppoed to have side effects...

[huntc]

Yeah, the current docs are missing specifying that offset and bytes.len() are multiples of READ_SIZE.

However it's true that the majority of chips (all?) have READ_SIZE=1. Maybe we can remove READ_SIZE?

WRITE_SIZE matters especially if the NOR flash is not multiwrite, you don't want impls to do "tricks" behind your back to process unaligned writes because they may write to addresses you don't expect... However for READ_SIZE, if a chip really has a read alignment requirement, impls can read "slightly more" and then give you the subset of bytes, which is fine because reads aren't suppoed to have side effects...

I’m happy to drop read size. However, I’m wondering if there’s still a case for dropping write size for the write buf len… for convenience.

I’m using Postcard to serialise what I wish to write. While I pass it in a buffer to serialise, it returns me a slice of that of buffer representing what was serialised. That slice len is byte aligned of course, not Nvmc write aligned in terms of len.

So could we relax the write buf len being write aligned and keep the offset as being write aligned?

[Dirbaio]

However, I’m wondering if there’s still a case for dropping write size for the write buf len… for convenience.

The idea for the NorFlash is to be a low-level trait where impls just expose the hardware capabilities without extra magic. You need that if you're writing low level stuff such as filesystems, databases, key-value-stores.

If you just want "please write these bytes" you want the Storage trait. #14 has an adapter that converts NorFlash to Storage by doing the necessary magic padding and read-modify-writes.

[huntc]

Thanks for the explanations. Do you want a PR for the elimination of the READ_SIZE requirement?

[Dirbaio]

I'm not sure what's best! 😅

On one hand, chips with READ_SIZE>1 are rare, and in that case the impl could easily do "magic" to simulate READ_SIZE=1.

On the other hand, NorFlash aims to be no-magic lowlevel, and the magic can be done in the Storage adapter instead, like for writing, which feels a bit more consistent.

[chrysn]

There is a point to be made about optimizations for aligned reads: If the underlying interface is not byte-oriented, the unaligned-read magics would be invoked on each read even when it is aligned.

Then again, the current interface (with its &[u8]) needs to cater can't be compiled into aligned reads/writes anyway.

How about a low-level read interface that works with something like read_words(offset: Self::WordIndex, &mut [Self::ReadWord; n]), and a provided read(offset: usize, &mut [u8]) that allows unaligned access (but is not used by applications that keenly avoid unaligned access already)?

(There might even be a provided middle ground read_aligned<N: usize>(offset: usize, &mut [[u8; N]]) that goes right for read_words if N >= READ_SIZE and does workarounds otherwise -- but that may be excessive).