Skip to content
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

add future support #38

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/bors.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ delete_merged_branches = true
required_approvals = 1
status = [
"ci-linux (stable, x86_64-unknown-linux-gnu)",
"ci-linux (1.35.0, x86_64-unknown-linux-gnu)",
"ci-linux (1.36.0, x86_64-unknown-linux-gnu)",
]
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:

include:
# Test MSRV
- rust: 1.35.0
- rust: 1.36.0
TARGET: x86_64-unknown-linux-gnu

# Test nightly but don't fail
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ homepage = "https://github.com/rust-embedded/nb"
documentation = "https://docs.rs/nb"
readme = "README.md"
version = "1.0.0" # remember to update html_root_url
edition = "2018"
edition = "2018"
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ This project is developed and maintained by the [HAL team][team].

## Minimum Supported Rust Version (MSRV)

This crate is guaranteed to compile on stable Rust 1.35 and up. It *might*
This crate is guaranteed to compile on stable Rust 1.36 and up. It *might*
andrewgazelka marked this conversation as resolved.
Show resolved Hide resolved
compile with older versions but that may change in any new patch release.

## License
Expand Down
46 changes: 46 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@
#![doc(html_root_url = "https://docs.rs/nb/1.0.0")]

use core::fmt;
use core::future::Future;
use core::pin::Pin;
use core::task::{Context, Poll};

/// A non-blocking result
pub type Result<T, E> = ::core::result::Result<T, Error<E>>;
Expand Down Expand Up @@ -265,3 +268,46 @@ macro_rules! block {
}
};
}

pub struct NbFuture<Ok, Err, Gen: FnMut() -> Result<Ok, Err>> {
gen: Gen,
}

impl<Ok, Err, Gen: FnMut() -> Result<Ok, Err>> From<Gen> for NbFuture<Ok, Err, Gen> {
fn from(gen: Gen) -> Self {
Self { gen }
}
}

impl<Ok, Err, Gen: FnMut() -> Result<Ok, Err>> Future for NbFuture<Ok, Err, Gen> {
type Output = core::result::Result<Ok, Err>;

fn poll(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Self::Output> {
Copy link

@ryankurte ryankurte Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey thanks for the PR! given that the waker isn't getting propagated here i wouldn't expect the scheduler to poll this again, and always hitting wake runs into performance problems on non-embedded platforms... does this behave as expected / how're you using this at the moment?

(cc. @Dirbaio the futures-whisper)

Copy link
Member

@Dirbaio Dirbaio Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it'll hang with most executors out there due to not waking the waker. Docs on Future::poll() say supporting wakers is mandatory.

It'll only work with very dumb executors that do loop { fut.poll(cx) } ignoring the waker, spinning the CPU at 100%.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one way around it, which is immediately waking ourselves on poll: cx.waker().wake_by_ref(). It'll still spin the CPU at 100% but at least it won't hang. In theory this might cause other tasks to get starved of CPU, but in practice executors usually handle a "self-wake" like this by moving the task to the back of the queue, so other tasks still get to run. I don't think it's guaranteed though.

IMHO the way forward is HALs implementing the async traits, and deprecate nb. This allows HALs to wake the wakers from irqs, avoiding spinning the cpu at 100%. It also allows using DMA (you can't use DMA soundly with nb).

There's another issue: the way nb is currently used, the task is polled for each byte. This makes it too easy to lose data if the task isn't polled fast enough, as the hardware buffers are usually tiny. If HALs impl the futures instead, they have powerful tools to avoid that (irqs, dma).

let gen = unsafe { &mut self.get_unchecked_mut().gen };
let res = gen();

match res {
Ok(res) => Poll::Ready(Ok(res)),
Err(Error::WouldBlock) => Poll::Pending,
Err(Error::Other(err)) => Poll::Ready(Err(err)),
}
}
}

/// The equivalent of [block], expect instead of blocking this creates a
/// [Future] which can `.await`ed.
///
/// # Input
///
/// An expression `$e` that evaluates to `nb::Result<T, E>`
///
/// # Output
///
/// - `Ok(t)` if `$e` evaluates to `Ok(t)`
/// - `Err(e)` if `$e` evaluates to `Err(nb::Error::Other(e))`
#[macro_export]
macro_rules! fut {
($call:expr) => {{
nb::NbFuture::from(|| $call)
}};
}