Skip to content

Gpio cdev feature #29

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

Closed
wants to merge 3 commits into from
Closed

Gpio cdev feature #29

wants to merge 3 commits into from

Conversation

almusil
Copy link
Contributor

@almusil almusil commented Dec 14, 2019

As requested in #20. This PR adds support for feature that will replace sysfs gpio with cdev gpio.

Default feature set is still the same so it won't broke any existing code.

@almusil almusil requested a review from a team as a code owner December 14, 2019 09:10
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ryankurte (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.

@rust-highfive rust-highfive added S-waiting-on-review Status: Review is incomplete T-embedded-linux labels Dec 14, 2019
@almusil
Copy link
Contributor Author

almusil commented Dec 14, 2019

I know that the doc creation will not work like this. But I have to admit I am lost how to update the doc to be able state there is feature flag. Can someone please point me to the documentation that might help with this?

Thank you

@thejpster
Copy link

Do you mean on docs.rs? You can specify which flags are enabled for the docs.rs rustdoc build - see https://docs.rs/about. If you want to tell the users that a feature flag exists, then I guess you can just put it in the README?

This feature adds possibility to use newer gpio cdev
instead of deprecated gpio sysfs. At the same time maintains
backward compatibility as default feature set is still using sysfs.
@almusil
Copy link
Contributor Author

almusil commented Dec 15, 2019

Do you mean on docs.rs? You can specify which flags are enabled for the docs.rs rustdoc build - see https://docs.rs/about. If you want to tell the users that a feature flag exists, then I guess you can just put it in the README?

Right, I have added commit to update the README. Thank you.

@@ -11,15 +11,19 @@
//! [0]: https://crates.io/keywords/embedded-hal

#![deny(missing_docs)]
#![feature(doc_cfg)]
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to remove this? i don't think we want to stop building on stable

error[E0554]: `#![feature]` may not be used on the stable release channel
  --> src/lib.rs:14:1
   |
14 | #![feature(doc_cfg)]
   | ^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am afraid that #[doc(cfg(feature = "gpio_cdev"))] won't work then. So I guess no point in keeping them either. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh yeah, that's a bit frustrating, i think building on stable is more important for now. i'd also like to see ci coverage (even though it's only running cargo check).

i guess we could build both in sub-paths (under default-features gates) and choose the default export with a feature, which would put them both in the docs, not require an additional test run, and let people runtime select if required, but, seems more complicated than is probably necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I have created alternative PR #30 . Let me know which one we should proceed with. Thanks

@ryankurte
Copy link
Contributor

Closing in favour of #30, thanks!

@ryankurte ryankurte closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Review is incomplete T-embedded-linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants