Add support for reading asynchronously with mio::Evented and tokio_core::Stream #26
Conversation
|
CI is failing only for Rust 1.5.0, seemingly because mio 0.6.1 does something in its Cargo.toml that isn't compatible with older versions. |
|
@emosenkis Go ahead and make the changes to bump the minimum supported version up to what is required. |
|
r? @nastevens |
|
@posborne Done (I had to bump the minimum Rust version again for futures-rs compatibility and also replace my use of the new |
| @@ -15,6 +15,14 @@ interrupts) GPIOs from userspace. | |||
| See https://www.kernel.org/doc/Documentation/gpio/sysfs.txt | |||
| """ | |||
|
|
|||
| [features] | |||
| default = ["tokio"] | |||
There was a problem hiding this comment.
I'm on the fence on including tokio in the default features as it is not a small dependency and may send users down a bit of a tangent. At least for now, I'm guessing most users will still be using this library for just basic control or dealing with polling in a blocking fashion. Obviously, they are not forced to use the feature, but many people will not disable it.
|
Thanks for the change @emosenkis. Would you mind adding an example that showcases the asynchronous usage? I think a good example could be as simple as demonstrating how you could use the async features to poll multiple pins concurrently without the use of threads. You could base this off of what is present for interrupt.rs but make the program take a variable number of arguments (each being a pin to add to the group of pins polled). Finally, some mention of the asynchronous features would be nice to add to the top-level README documentation. |
|
@posborne Done. |
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "sysfs_gpio" | |||
| version = "0.4.4" | |||
| version = "0.4.5" | |||
There was a problem hiding this comment.
This seems like a big enough backwards compatible change that it could probably be a new minor version. What do you think @posborne?
There was a problem hiding this comment.
My two cents: I don't see any reason to bump the minor version for a change that's 100% backwards compatible (therefore requiring everyone to update their Cargo.toml)
There was a problem hiding this comment.
Let's bump the version if we are going to have tokio be part of the default features. Dropping a bunch of extra features/dependencies on a user of the library when they did a cargo update doesn't seem polite. Plus, we dropped support for older versions of Rust - that is enough in my book to go to 0.5.0.
There was a problem hiding this comment.
The new features are no longer enabled by default. Unless they are manually enabled, there is no additional dependency and no change in the minimum Rust version (the new dependencies are what caused the change in Rust version compatibility).
There was a problem hiding this comment.
Let me know if you still want me to change the version to 0.5.0.
There was a problem hiding this comment.
Yes, let's bump it as we are dropping official support for the older versions of rust.
| @@ -447,7 +486,8 @@ impl PinPoller { | |||
| /// making this call. This call makes use of epoll under the | |||
| /// covers. If it is desirable to poll on multiple GPIOs or | |||
| /// other event source, you will need to implement that logic | |||
| /// yourself. | |||
| /// yourself or poll asynchronously using the integration with | |||
There was a problem hiding this comment.
I think it'd be fine to remove the "do it yourself" option in favor of using the mio/tokio option.
| #[cfg(feature = "mio-evented")] | ||
| impl AsyncPinPoller { | ||
| fn new(pin_num: u64) -> Result<Self> { | ||
| let devfile: File = try!(File::open(&format!("/sys/class/gpio/gpio{}/value", pin_num))); |
There was a problem hiding this comment.
Is the explicit File type needed here? It seems like type inference would figure it out.
|
I bumped the version as requested but also changed the new API slightly since I realized that reading the pin's value is unnecessary if interrupt is only enabled for one edge. |
|
@emosenkis Any changes coming in or do you think this is ready for a merge? |
|
@posborne The only issue I'm aware of is that when using PinStream and PinValueStream, it immediately triggers. I think I should probably try to consume that initial event before it gets to the user. I also want to squash my commits before you merge. |
|
No problem, let me know when you are happy with things and I'll do another final review before merging and cutting 0.5.0. Thanks again for the contribution! |
|
@posborne Ready for merge. I fixed the issue with the bogus first event and did a little bit of cleanup and optimization. I tested the example on real hardware with a rotary encoder/button and it works as expected. |
|
Excellent, thanks again! |
|
Would you be willing to cut 0.5.0 and upload it to crates.io? I have a rotary encoder library that depends on it which I'd like to release. |
|
Yes, I'll release 0.5.0 this afternoon. |
No description provided.