embedded-storage icon indicating copy to clipboard operation
embedded-storage copied to clipboard

To use nb or not to use nb

Open Sympatron opened this issue 4 years ago • 2 comments

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!().

Sympatron avatar Mar 03 '21 12:03 Sympatron

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.

Dirbaio avatar Mar 03 '21 13:03 Dirbaio

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?

MathiasKoch avatar Mar 03 '21 13:03 MathiasKoch

With how usable async is has become since this issue was raised, shall we resolve it as "we'll only have blocking and async"?

chrysn avatar Oct 22 '23 20:10 chrysn

IMO yes

Dirbaio avatar Oct 22 '23 20:10 Dirbaio