Futures 0.3

[benjumanji]

First effort at adding support for the new futures ecosystem. I haven't given it much of a run through yet, this is more just to let people know I am working on it. Comments welcome.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nastevens (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[benjumanji]

I can see that this probably won't compile without bumping the MSRV. What's the policy on that?

[nastevens]

For managing pins I much prefer pin-project-lite - the API is clearer to me and it doesn't change the signature of poll functions. It's also what the tokio project itself uses, which seems like a pretty good endorsement 😃

[nastevens]

Before I go into a detailed review I'd like to get @posborne's thoughts on overall direction. Do we release a new major version with only Futures 0.3 support? Or should that support be behind a different feature flag (like use_futures_03? Or some other option not listed here?

@benjumanji are you using this library in a project? If so, what are your opinions on what would work best for your workflow?

[benjumanji]

I am using it for a project yes, but I am happy to run my own fork (praise be to cargo for making this so easy) until you have a clear idea of how you want this merged, so I'm easy.

[benjumanji]

For managing pins I much prefer pin-project-lite - the API is clearer to me and it doesn't change the signature of poll functions. It's also what the tokio project itself uses, which seems like a pretty good endorsement smiley

I'll switch over to that.

[benjumanji]

Before I go into a detailed review I'd like to get @posborne's thoughts on overall direction. Do we release a new major version with only Futures 0.3 support? Or should that support be behind a different feature flag (like use_futures_03? Or some other option not listed here?

@benjumanji are you using this library in a project? If so, what are your opinions on what would work best for your workflow?

I've been thinking about this more, I think a major version bump that just supports new futures would suit me best. This is a relatively small library, so I think back porting bug fixes to prior versions wouldn't be so painful. Maintaining both kinds of futures impl behind gates seems work for not so much gain.

[nastevens]

I've been thinking about this more, I think a major version bump that just supports new futures would suit me best.

Yup, I think I agree with you there. I think it also helps that there are compat layers that can be used to go to/from futures 0.1/0.3.

Plus, with sysfs-gpio being removed from the kernel soon (or already removed? I forget...), this library won't be needed anymore right? Because embedded devices always use the newest kernel? /s 😉

[benjumanji]

Something that is kind of a bummer, which I haven't yet figured out is caused by me not knowing what I am doing or is inherent to new tokio, is this pr means that you can't create a stream outside of a reactor anymore.

EDIT: having read around a bit more, and seeing that tokio-serial has the same new restriction I don't feel bad about this anymore.

[benjumanji]

Plus, with sysfs-gpio being removed from the kernel soon (or already removed? I forget...), this library won't be needed anymore right? Because embedded devices always use the newest kernel?

cries in legacy kernel

[slyshykO]

@nastevens , I think, 4.19 with us for years.

[kaojo]

Hi,
I have migrated my project to futures 0.3 using the fork for this PR. the features I use are working.

Regarding the version for the new release I also vote for a new major version and cutting the support for the old futures API.

This PR would resolve #56

Thanks for the work.

[benjumanji]

Thank you for testing it out!

[slyshykO]

@nastevens @pheki
Is it any chance that this PR will be merged?

[posborne]

@nastevens @ryankurte I added a few commits to this to hopefully get things into a mergeable state. I didn't get a chance to run on hardware this evening as I didn't have anything handy to test with ATM but can throw it on something to validate.

[benjumanji]

Thanks for cleaning it up! 😬 Funnily enough for my own projects I have actually dropped async entirely and migrated to the cdev driver, but for the few months that I had it running on devices (fleet of rpis) it worked just fine.

[eldruin]

Is there anything against merging this?
I want to make a PR with several updates and then make a new release and merging this one first would be great.

[ryankurte]

Is there anything against merging this? I want to make a PR with several updates and then make a new release and merging this one first would be great.

@eldruin i'd prefer to have an updated tokio dep but if you're planning to resolve that in your updates, no opposition from me! from previous feature experience, it's probably worth adding to the test matrix a build with / without the use_tokio feature enabled so we can make sure both stay working (and idk whether we have a preference for dashes or underscores in feature names?)

[pheki]

@ryankurte about the naming convention, I went to check and according to the official API guidelines by the rust library team it's unclear, but:

• It should probably be just tokio, as it's stated in the guideline "Do not include words in the name of a Cargo feature that convey zero meaning, as in use-abc or with-abc. Name the feature abc directly."
• Otherwise I'd say it would be probably more consistent to use the same convention as the crate name (kebab-case), but that is just a personal opinion.

Sources:

• https://rust-lang.github.io/api-guidelines/naming.html#c-feature
• Case convention for feature names rust-lang/api-guidelines#101

[eldruin]

I proposed using async-tokio as a feature name like in gpio-cdev at #67

[eldruin]

Fixed in #69.
Thank you for your work!