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

RFC: Line event stream support for Tokio #35

Merged
merged 6 commits into from
Aug 2, 2020

Conversation

mgottschlag
Copy link
Contributor

The second commit (on top of #34 for my convenience, but completely independent) adds support for asynchronous handling of line events/interrupts based on Tokio. This patch probably fixes #18 .

This probably needs some further discussion:

  • I set the Rust edition to 2018 to be able to use async/await in the example.
  • The API is just a light wrapper around LineEventHandle. I implemented AsRef<LineEventHandle> instead of adding a separate get_value() function. Do we want a function to destroy an AsyncLineEventHandle and get the original LineEventHandle back?
  • I placed the type in an async_tokio module and behind an async_tokio feature flag, under the expectation that one day there might be wrapper types for other async I/O frameworks (async_std?) as well.

mem::unitialized() is deprecated, use mem::MaybeUninit instead which exists
since rustc 1.36.0.
@mgottschlag
Copy link
Contributor Author

This is horribly wrong - I'll update the PR once it's fixed.

@mgottschlag
Copy link
Contributor Author

Now it should be in a state where feedback would be appropriate. I can rebase it on master if you do not want to merge #34 (right now, this is the version I use for development).

I tested both the async_tokio and the gpioevents example, and they seem to work.

@fpagliughi
Copy link
Contributor

Apologies, I've been slammed at work this week, but want to give this a proper look soon, because it really would be a cool feature to add. (Not that I have write access here, but still a vested interest).

My opinion on libraries at this level has now become that we should try to resist selecting one runtime over another. I might use this lib with any runtime, and even if the runtime is not exposed in the API, having to compile both tokio and another runtime adds a bit of code to the build.

There isn't much tokio here. Couldn't we just up the MSRV to 1.39 and use standard futures and streams?

@mgottschlag
Copy link
Contributor Author

While it might be possible to build directly upon mio instead of using Tokio, I don't think that would be of much use.

When I looked at async-std for a similar problem, I found that they don't have proper support for custom Evented event sources: async-rs/async-std#293 and async-rs/async-std#626

@tchamelot
Copy link

I looked at this PR and I have done something similar lately.

I feel that the mio abstraction should be available as is and not only through an async function relying on tokio. To do that you could remove the PollWrapper which seems useless to me and directly implement Evented for LineEventHandle.

Like in rust-embedded/rust-sysfs-gpio , it would be nice to hide mio behind a feature and tokio behind another one which relies on mio.

@posborne
Copy link
Member

@mgottschlag @tchamelot Sorry for missing this when it first came in. It's been an interesting past few months!

)?)?;

loop {
match events.next().await {
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's slightly more idiomatic to write this as:

while let Some(event_result) = events.next().await {
    let event = event_result?;
    println!("{:?}", event);
}

@posborne
Copy link
Member

CI is failing due to a requirement in 1.38 it looks like. @mgottschlag could you bump the msrv to 1.38.0?

Signed-off-by: Paul Osborne <osbpau@gmail.com>
Until root cause is established, disable this target.

Signed-off-by: Paul Osborne <osbpau@gmail.com>
@posborne
Copy link
Member

posborne commented Aug 2, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 2, 2020

Build succeeded:

@bors bors bot merged commit 448d6fd into rust-embedded:master Aug 2, 2020
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

Successfully merging this pull request may close these issues.

How to do monitor for an event on more than one input?
4 participants