set_state and toggle for digital::OutputPin

[burrbull]

set_state and toggle with default implementation

[therealprof]

I quite like the set_state() and toggle() methods, especially since the latter is specialisable in case there's hardware toggle support.

Not so happy about the snuck in default implementation for is_high(). IMHO we should rather (in a separate topic because this might be controversial!) add a get_state() function (also for the InputPin) and make is_high() and is_low() be default implementations of the get_state() function. In case of the OutputPin we may even have to make that fallible since it is not on all platforms possible to query the state of output pins (though my memory is faint on which platforms exactly didn't support that).

[therealprof]

This does not look right.

[japaric]

I personally feel that the get_state methods don't add expressivity to the API. On the contrary,
using them makes the code less readable, IMO:

if pa0.get_state() { // what do you mean by state? input state? output state? floating state?
    // ..
}

if pa0.is_high() { // OTOH, this is totally unambiguous
    // ..
}

Now if get_state returned a State enum then it would make more sense but I feel that a two
variant enum is a bit overkill in this case.

Both get_states get a 👎 from me in their current state.

I'm of the (unpopular) opinion that set_state(bool) decreases readability but I understand that
people want to be able to easily write stuff like pa0.set_state(pa1.is_high()). I personally would
write:

if pa1.is_high() {
    pa0.set_high()
} else {
    pa0.set_low()
}

Yes, it's longer but it's pretty clear what it does.

I give set_state a zero; I'm not opposed to merging it.

toggle gets a 👍 from me

---

And a remainder: adding methods to a trait is a breaking change unless the methods have a default implementation.

[burrbull]

Will set_value be better?
Or set_level ?
Or set_bool ?

[therealprof]

@japaric Usually the state represents the current interpretation, i.e. the level while the "state" in your reading is called the mode, sometimes also separated into administrative mode and operational mode.

[wez]

I came here to propose adding a toggle_level method and found this PR.

FWIW, over time I have come to the opinion that boolean input parameters are to be avoided because the intent is not clear. These days I try to prefer using an enum. In this case I would humbly advocate for get_level and set_level methods that return/accept an enum { Low, High } value.

Can we split out the toggle functionality as a separate PR since that one seems not to be controversial and could be accepted ahead of the rest of this discussion?

[therealprof]

@wez I agree with both of your points. However I wanted to add that using a custom enum makes using the methods a bit more tedious to use unless there's an adapter interface mapping the generally accepted true == high relation.