-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
mem::unitialized() is deprecated, use mem::MaybeUninit instead which exists since rustc 1.36.0.
This is horribly wrong - I'll update the PR once it's fixed. |
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. |
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? |
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 |
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 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. |
@mgottschlag @tchamelot Sorry for missing this when it first came in. It's been an interesting past few months! |
)?)?; | ||
|
||
loop { | ||
match events.next().await { |
There was a problem hiding this comment.
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);
}
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 <[email protected]>
Until root cause is established, disable this target. Signed-off-by: Paul Osborne <[email protected]>
bors r+ |
Build succeeded: |
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:
async
/await
in the example.LineEventHandle
. I implementedAsRef<LineEventHandle>
instead of adding a separateget_value()
function. Do we want a function to destroy anAsyncLineEventHandle
and get the originalLineEventHandle
back?async_tokio
module and behind anasync_tokio
feature flag, under the expectation that one day there might be wrapper types for other async I/O frameworks (async_std?) as well.