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

Updated logic around sysfs/cdev pin selection #34

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

ryankurte
Copy link
Contributor

  • added both features by default
  • re-export Pin type using type aliases
  • default behaviour exports cdev for backwards compatibility
  • package builds with any combination of gpio features

@ryankurte ryankurte requested a review from a team as a code owner March 10, 2020 22:17
@rust-highfive
Copy link

r? @nastevens

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Review is incomplete T-embedded-linux labels Mar 10, 2020
@ryankurte ryankurte mentioned this pull request Mar 10, 2020
@ryankurte ryankurte assigned posborne and unassigned nastevens Mar 10, 2020
src/lib.rs Outdated

#[cfg(all(feature = "gpio_cdev", not(feature = "gpio_sysfs")))]
/// Re-export of `cdev_pin::CdevPin` pin type when `gpio_cdev` feature is selected
pub type Pin = cdev_pin::CdevPin;
Copy link
Member

Choose a reason for hiding this comment

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

My gut feeling is that it could make sense to re-export SysfsPin as Pin if the gpio_sysfs feature is enabled. I think I would prefer to not export any symbol with the name Pin that ever points at a different type that is cdev. I think that's probably just going to cause confusion without any real benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably fair, I put this in because a) there was a duplicated Pin impl and b) it makes the change non-breaking.

If we're going to break it, does it make sense to have the re-export at all? We could just export whatever is enabled and indicate a preference in the comments?

Copy link
Member

Choose a reason for hiding this comment

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

I guess that would be my preference overall -- With a semver bump I think the move to be explicit about the subsystem being used to talk to the GPIO makes sense. Any breakage with the semver major version is not something I think will be a big deal for users if they are sticking with sysfs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweet as, updated

@ryankurte
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Mar 11, 2020
@bors
Copy link
Contributor

bors bot commented Mar 11, 2020

try

Build succeeded

Copy link
Member

@posborne posborne left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 12, 2020

Build succeeded

@bors bors bot merged commit a653349 into rust-embedded:master Mar 12, 2020
@ryankurte ryankurte deleted the fix/multi-gpio branch March 13, 2020 08:25
bors bot added a commit that referenced this pull request Nov 17, 2020
35: Add transactional SPI r=ryankurte a=ryankurte

Implementation of transactional SPI

- replaces #33
- blocked on:
  - ~rust-embedded/embedded-hal#191
  - ~#34

~Important: remove patch and bump hal version before merging~

Co-authored-by: ryan <ryan@kurte.nz>
Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
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.

None yet

4 participants