Add digital::IoPin to resolve issue 29#269
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (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. |
|
PR on linux-embedded-hal: rust-embedded/linux-embedded-hal#60 |
| /// | ||
| /// If the pin is already in the requested state, this method should | ||
| /// succeed. | ||
| fn try_switch_to_output_pin(self, state: PinState) -> Result<TOutput, Self::Error>; |
There was a problem hiding this comment.
Would not setting an initial state also make sense?
There was a problem hiding this comment.
If the user didn't specify the state, then the result would be a pin in output mode that's either high or low, but the user doesn't know which it is. I think it's better to make the user specify which state they want rather than to leave it ambiguous.
That being, said I'm no expert in the embedded space. Do you see a valid use case?
There was a problem hiding this comment.
I was just wondering. From a software perspective it makes sense to always change to a defined state. However, from a hardware perspective, it may not always make sense to set a pin, since it has electrical implications.
Does anybody else here have an use case for changing the pin type to output but leaving it unset?
There was a problem hiding this comment.
IIUC, an example use-case might be an output pin in open drain mode (I could be wrong; like I said, I'm not an embedded expert). However, the current digital traits don't seem to provide any support around these states either. Maybe it makes sense to look at that separately, eg. as an addition to PinState? Definitely interested in what other folks think here, as @eldruin said.
|
I've added an example to the IoPin docs. I am not a lawyer, but I expect that if there isn't a formal process for tracking copyright at the moment, then the PR/commit history will be fine. |
|
@MorganR This is looking great. I'm wondering if we can make the type stuff a little bit easier on the eyes and the mind but other than that I like it. |
|
Thanks @therealprof! I agree the type system becomes a bit verbose when using the abstract trait. However, it's actually fairly clean when implementing the trait, and in user code that gives a concrete implementation to something using the abstraction. To simplify things a little more, I've removed the |
|
If you could squash the commits and maybe add a "Closes #29" to your initial post, that would be great. |
Done |
therealprof
left a comment
therealprof
left a comment
There was a problem hiding this comment.
Thanks a lot for your efforts!
bors r+
269: Add digital::IoPin to resolve issue 29 r=therealprof a=MorganR Closes #29. This implementation is inspired by [chrismoos's comment](#29 (comment)) in #29. I've demonstrated using this trait for a [no-std DHT11 driver](https://github.com/MorganR/rust-simple-sensors/blob/main/src/dht11.rs), and verified this on a Raspberry Pi 3B+ using an implementation in linux-embedded-hal (PR to follow). I'll also be sending a PR to implement this in at least one additional HAL implementation library (recommendations welcome). One question I have is how copyright/authorship is tracked in this repo? I believe the copyright owner for my code would technically be Google LLC. When patching repos, I'm meant to add that to the appropriate copyright/authors list. Co-authored-by: Morgan Roff <mroff@google.com>
|
Build failed: |
|
Sorry, fixed that remaining fmt error. Should be good now. |
|
@MorganR I guess rustfmt doesn't like your formatting. 😅 It's a pity the new GH protection mechanisms seem to silently prevent actions from running... |
therealprof
left a comment
therealprof
left a comment
There was a problem hiding this comment.
Let's try this again.
bors r+
|
Which combinations of output and input modes this should be implemented? |
60: Implementation of embedded_hal::digital::IoPin r=eldruin a=MorganR This is just a draft, and is pending the PR merge of an [IoPin implementation](rust-embedded/embedded-hal#269) into embedded_hal. I've verified it with a [no-std DHT11 driver based on the IoPin PR](https://github.com/MorganR/rust-simple-sensors/blob/main/src/dht11.rs). Co-authored-by: Morgan Roff <mroff@google.com>
5 participants
Closes #29.
This implementation is inspired by chrismoos's comment in #29.
I've demonstrated using this trait for a no-std DHT11 driver, and verified this on a Raspberry Pi 3B+ using an implementation in linux-embedded-hal (PR to follow).
I'll also be sending a PR to implement this in at least one additional HAL implementation library (recommendations welcome).
One question I have is how copyright/authorship is tracked in this repo? I believe the copyright owner for my code would technically be Google LLC. When patching repos, I'm meant to add that to the appropriate copyright/authors list.