tweedegolf / sequential-storage

A crate for storing data in flash memory with minimal need for erasing pages
Apache License 2.0
99 stars 10 forks source link

Async? #6

Closed diondokter closed 9 months ago

diondokter commented 11 months ago

Writing and reading from an external flash chip can take quite a while. So this library should ideally be async.

But how to do that? I don't want to maintain both a blocking and an async version of the same thing...

gorazdko commented 11 months ago

If we are all using this within embassy, maybe support only the async version of this lib?

Async write, erase and read traits could still be implemented by blocking functions if that's the only way how MCU supports flash operations.

nrf52x already writes to internal flash asynchronously via softdevice.

diondokter commented 11 months ago

Yeah that's what I was thinking. The only thing is that cancellation would be a nightmare. I already handwave errors away. 'Oh you got an error? Well, an error is an error, so don't be surprised that your flash might be corrupted from now on!'.

The only feasible thing to do is ask to complete the future and give no guarantees for dropped futures.

lulf commented 9 months ago

@diondokter Have you had anymore thoughts on how to approach this? I made a stab locally replacing embedded-storage with embedded-storage-async, and first hurdle was the callback/closure-based iteration of the headers. In order to simplify the conversion to async in the future, I'd like to change that to have an iterator-like API that can more easily be converted to async for instance.

As for the non-async version, there is https://github.com/embassy-rs/embassy/blob/main/embassy-embedded-hal/src/adapter/blocking_async.rs which can be used to wrap a non-async flash in an async interface. Depending on how you want to do it, one could either expose the blocking APIs and use that type internally or just point users to it.

The part about cancellation I'm not sure, in general cancellation is a nightmare and having a big sign saying 'thou shalt not cancel this future' is OK imho.

diondokter commented 9 months ago

@lulf Yeah, converting it to an iterator style could be possible. My first thought would be to make the closure return a future (so it simulates an async closure) so the API can remain the same. But if you can find a better API for it, then I'm interested. The current one works, but isn't amazing.

I didn't know embassy had adapters like that! (Though it's missing the async multiwrite norflash, but that is easily added.) Ok, yeah, in that case I think it's fine to just move everything over to async and point people to the adapter and a block_on function.

With regard to cancellation, that is what #16 is also for. James Munns argued that cancellation isn't all that different to a random power shutdown. So that's why I worked on that. Hopefully any cancellation will only lead to possible repairable corruption. So we'd just have to document that and say 'cancel on your own risc'.

diondokter commented 9 months ago

Closed by #17