Open Drain Output #217
Open Drain Output #217
Conversation
|
Hi, thanks a lot for the PR. I think the previous "tristate" mode was a bit misnamed, because it wasn't a full tristate implementation. I think what we actually want is two different modes: 1.
|
Rahix
added
hal-api
| /// Set the pin to tristate mode (Input without PullUp) | ||
| #[inline] | ||
| pub fn set_tristate(&mut self) { | ||
| unsafe { self.pin.make_input(false) } | ||
| } |
There was a problem hiding this comment.
To rephrase my previous comment: I think for the "open drain" mode, this method should be called set_high() for consistency. Reason is:
- "open drain" and "push pull" are both used for binary outputs and thus just represent different "encodings" of the information. To make user-code not reliant on the used "encoding", I think the methods should have the same name.
- in the
embedded-haltrait impl, the method is already calledset_high() - other HALs also keep the same method names irregardless of whether the pin is "push pull" or "open drain"
|
|
||
| /// Convert this pin into an output pin with open drain, setting the state to tristate | ||
| /// See [Digital Output Open Drain](#digital-output-open-drain) | ||
| pub fn into_opendrain_tristate(mut self) -> Pin<mode::OpenDrain, PIN> { |
There was a problem hiding this comment.
Similarly, this should then be into_opendrain_high().
| /// Check whether the pin is set high. | ||
| /// | ||
| /// *Note*: The electrical state of the pin might differ due to external circuitry. | ||
| #[inline] | ||
| pub fn is_set_high(&self) -> bool { | ||
| unsafe { self.pin.in_get() } | ||
| } |
There was a problem hiding this comment.
The is_set_high() method is supposed to return a deterministic answer to whether the pin was previously "set high" (= tristated for open-drain). Reading the pin input register here makes the result dependent on whether an outside source is driving the pin high or low.
The correct implementation would be one checking the bit in the DDR register for this pin. If it is set (= pin is output), we are currently driving the pin low. If it is cleared (= pin is input), the pin is tristated and is_set_high() should return true.
Unfortunately, PinOps does not yet expose any method for reading the DDR bit, so you'll need to add something like a dir_get() method there and also implement it in the implementation macro.
| pub fn is_set_low(&self) -> bool { | ||
| !unsafe { self.pin.in_get() } | ||
| } |
There was a problem hiding this comment.
Following the above, is_set_low() should probably just be implemented in terms of is_set_high():
| pub fn is_set_low(&self) -> bool { | |
| !unsafe { self.pin.in_get() } | |
| } | |
| pub fn is_set_low(&self) -> bool { | |
| !self.is_set_high() | |
| } |
The compiler will optimize it to be equivalent to a handrolled implementation.
| /// Check whether the pin is set high. | ||
| /// | ||
| /// *Note*: The electrical state of the pin might differ due to external circuitry. | ||
| #[inline] | ||
| pub fn is_set_high(&self) -> bool { | ||
| unsafe { self.pin.in_get() } | ||
| } | ||
|
|
||
| /// Check whether the pin is set low. | ||
| /// | ||
| /// *Note*: The electrical state of the pin might differ due to external circuitry. | ||
| #[inline] | ||
| pub fn is_set_low(&self) -> bool { | ||
| !unsafe { self.pin.in_get() } | ||
| } |
There was a problem hiding this comment.
I think what you actually wanted to do here is implement is_high() and is_low(). Those return the actual electrical state on the pin and would be implemented the way you did it here (reading the PIN bit for each pin)
| fn is_high(&self) -> Result<bool, Self::Error> { | ||
| Ok(self.is_set_high()) | ||
| } | ||
|
|
||
| fn is_low(&self) -> Result<bool, Self::Error> { | ||
| Ok(self.is_set_low()) | ||
| } |
There was a problem hiding this comment.
And these should then call self.is_high() / self.is_low() respectively.
|
@knoby do you think you could make the changes mentioned here? I'm had to fork this repo myself and implement |
|
@nikklassen, if the author of this PR doesn't respond, feel free to send a new PR superseding this one. Especially if you already continued work in this direction in your fork! |
After the redesign of the crate you can't use the open drain output as discussed in #16 .
Perhaps this also solves the point " Re-implement tri-state mode in the new port API" in #130.