`fetch_item` requests unlocked flash

[ryan-summers]

It seems odd that we require a NorFlash object for a read operation instead of a read-only flash. I looked at the code, and it looks like the only case here is that we intentionally erase the flash range if we failed to deserialize a requested item (which doesn't actually seem like what we want to do?).

I propose that we relax the bounds here and make it just request a ReadNorFlash object, which allows us to keep flash locked when reading, which limits the potential for unintended modifications.

[diondokter]

Ah yeah, that seems reasonable!

About the erase, yeah idk. At that point something pretty bad went wrong and in principle the data is not recoverable. The only thing you could do to continue is to erase the flash.

But changing it to retuning an error is good too. The users then have to do the erase themselves.

[diondokter]

Btw, were you looking to make the changes yourself?

I could do it too, but probably next Wednesday then.

[ryan-summers]

I can look at implementation next week as well - not super pressing here, just looking at integrating this into Stabilizer so we can maintain some settings, and this crossed my mind while implementing :)

[diondokter]

Oh cool! 😁

[diondokter]

Ah, I've found a problem why this can't really happen. Even when fetching data we need to know the page size, which is the erase size.

However, this const is only found on NorFlash and not on ReadNorFlash.

Any ideas?

[ryan-summers]

It seems odd to me that we need to know the flash page size when reading data. Is it not possible to deserialize items without knowledge of the underlying flash page structuring? It seems like that should be possible.

Right now, it seems like the only reason we need this is because the serialization mechanism structures data such that the first and last bytes of a flash page are marks (i.e 0), but I'm not certain that these are actually required. Can't you tell whether or not a flash page is in use by just looking for any non-erased bytes instead?

[diondokter]

Is it not possible to deserialize items without knowledge of the underlying flash page structuring? It seems like that should be possible.

In theory one could create a library where that's possible yeah, but this crate is designed page-first. There are three problems if you don't know the page size (and thus can't find the page markers).

1. Normal data can contain sections with 0xFF. The only contract is that not ALL of them may be 0xFF. So in theory you could have some data with 100 0xFF's while the amount of free space is less than that. This means you can't really determine the difference between them.
2. You don't know where the page markers are, so they would be considered to be normal data. This will break everything.
3. We don't know the write word size. So we don't know where to round up to in some cases.

Even if we could maybe fix 1, 2 and 3 are real blockers.

The only option I really see is this:

fn fetch_item<F: NorFlash>() -> () {
    fetch_item_read_only::<F, F::WRITE_SIZE, F::ERASE_SIZE>()
}

fn fetch_item_read_only<F: ReadNorFlash, const WORD_SIZE: usize, const ERASE_SIZE: usize>() -> () {
    // The real impl
}

But that'd have the cost of having to add the generics everywhere

[diondokter]

It's only a small portion of this issue, but I created a new release 0.4.2 where the crate doesn't auto-erase anymore and instead returns an error.

[ryan-summers]

Maybe we could provide the page size as a const generic argument instead? I generally want to keep the flash locked as often as possible, so I don't like the idea of unlocking flash in the controller just so I can provide it to this function and expose some const-expr.

[diondokter]

Closing this issue for now since the issue is with embedded-storage. If it gets updated so that the erase and write sizes are known in the norflash trait, then the issue can be reopened and I'll update the crate.