Skip to content

To use nb or not to use nb #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Sympatron opened this issue Mar 3, 2021 · 4 comments
Closed

To use nb or not to use nb #11

Sympatron opened this issue Mar 3, 2021 · 4 comments

Comments

@Sympatron
Copy link
Contributor

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

@Dirbaio
Copy link
Contributor

Dirbaio commented Mar 3, 2021

Very strong 👎 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.

@MathiasKoch
Copy link
Collaborator

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?

@chrysn
Copy link
Member

chrysn commented Oct 22, 2023

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

@Dirbaio
Copy link
Contributor

Dirbaio commented Oct 22, 2023

IMO yes

@Sympatron Sympatron closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants