Skip to content

Add try_set_state method for OutputPin #239

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

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

eldruin
Copy link
Member

@eldruin eldruin commented Jul 22, 2020

This is an implementation of #200 to gather some opinions and so we can either accept it or close the issue.
This was earlier discussed at #44.

I added a conversion from bool following the usual convention as well as an ops::Not implementation as suggested in #200, which seemed appropriate.

I also added a default implementation for the try_set_state method. This bears the question whether a default implementation for try_set_high() / try_set_low() by using try_set_state() would be useful, so that potential implementors can choose to implement less methods.

It should be noted that adding a default implementation for all 3 methods has the somewhat amusing property of generating an endless loop if none is overwritten.

Closes #200

@eldruin eldruin requested review from ilya-epifanov and a team as code owners July 22, 2020 07:44
@eldruin eldruin force-pushed the add-output-pin-set-state branch from 17a0d6c to f90d9d1 Compare July 22, 2020 07:54
@eldruin
Copy link
Member Author

eldruin commented Jul 22, 2020

Seems like rust-highfive has not run here
r? @therealprof

@ryankurte
Copy link
Contributor

idea seems good to me!

It should be noted that adding a default implementation for all 3 methods has the somewhat amusing property of generating an endless loop if none is overwritten.

definitely think we only want to default impl one of these (probably try_set_state) because this situation is, v cursed to come across.

@eldruin
Copy link
Member Author

eldruin commented Sep 14, 2020

@therealprof What do you think about this?

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM

@eldruin
Copy link
Member Author

eldruin commented Sep 14, 2020

Seems we all agree.

bors r=therealprof

@eldruin eldruin added this to the v1.0.0 milestone Sep 14, 2020
@bors bors bot merged commit 6f17287 into rust-embedded:master Sep 14, 2020
@eldruin eldruin deleted the add-output-pin-set-state branch September 14, 2020 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OutputPin trait usability -- add set_value
3 participants