embedded-storage
embedded-storage copied to clipboard
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!().
Very strong :-1: against using nb from me.
It is impossible to implement traits using nb with DMA.
Consider an API like this:
fn read(&mut self, address: u32, bytes: &mut [u8]) -> nb::Result<(), Self::Error> { .. }
On first call, this would start the DMA read into bytes, and then return WouldBlock. The problem is after return, the bytes buffer is no longer borrowed. The user could read from it which would race with DMA writes. Or even deallocate it, which would cause DMA to overwrite arbitrary memory.
To allow DMA implementations, the API would have to look like this:
// on NorFlash
fn read<'a>(&'a mut self, address: u32, bytes: &'a mut [u8]) -> Transfer<'a> { .. }
// on Transfer
fn is_done(&mut self) -> bool;
fn wait_done(&mut self);
The key is that Transfer borrows self and bytes, so the user can't use them while a transfer is ongoing.
Or, alternatively, a Futures-based API
fn read<'a>(&'a mut self, address: u32, bytes: &'a mut [u8]) -> impl Future<Output=Result<(), Error>> + 'a { .. }
The problem is a trait with either of these APIs (Transfer or Future) requires GATs (due to the generic lifetime), so they're nightly-only for now. Embassy has such a trait here.
So as i read what @Dirbaio is saying, it would make sense to make a regular blocking -> Result<..> api, until GAT lifetimes are stabilized, after which i think we will generally see much more push towards futures based APIs?
With how usable async is has become since this issue was raised, shall we resolve it as "we'll only have blocking and async"?
IMO yes