Move blink, pulse, etc to DigitalOutputOperations - #749
Conversation
|
Note that these are pre-existing issues and highlighted because code is moved around unchanged. |
Doesn't it address the following problem (quote from the issue) by providing DigitalOutputOperations backed defaults:
To address the rest, my plan is
|
| * @param state The initial state of the pin. | ||
| * @param callback The method to call, if any, once the blinking is done. | ||
| */ | ||
| static DigitalOutput blink(DigitalOutput digitalOutput, int delay, int duration, TimeUnit unit, DigitalState state, Callable<Void> callback) { |
There was a problem hiding this comment.
If this class had a DigitalOutput property, these functions could be non-static and share the exact same method signature as already exists on the output interface.
Maybe not an issue with the all-arg variants, but if there's a possibility that the user references DigitalOutput pulse(int interval, TimeUnit unit) as a BiFunction function reference and delegates the actual decision about whether to use pulseHigh or pulseLow to runtime, migrating to Operations be more work than necessary.
There was a problem hiding this comment.
This class is not intended to be exposed to users. It's sole point is to be able to have default implementations for all blink/pulse methods in DigitalOutput (which we should deprecate there), addressing point 2 in your issue (DigitalOutput implementation complexity without inheriting from DigitalOutputBase).
I am not fundamentally opposed to exposing the class in the form you suggest, wrapping a DigitalOutput. I just thought it provides little non-trivial value over something like SoftwarePwm (tbd), so we should rather deprecate the original methods without a 1:1 replacement than carrying these helpers over into the future in a new, slightly better form.
There was a problem hiding this comment.
That said, I think we should just go ahead with this change as is, keeping PRs atomic / minimal:
- It clearly addresses a significant part of the issue
- It doesn't make anything worse
- As DigitalOutputOperations is currently not exposed, we retain freedom to change it as we like
- In which way we expose it (if at all) is probably better decided when we have SoftwarePwm as a clean replacement for some of the functionality
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** Internal helper implementing legacy operations for DigitalOutput. */ | ||
| final class DigitalOutputOperations { |
There was a problem hiding this comment.
Rather than being a helper, this could / should be an interface which owns the spec for the operations.
The original premise of #742 was that the operations don't belong on DigitalOutput and should be relocated
Technically, yes. If this point is considered in isolation. It doesn't address the preceding two points:
|
I don't think there we have a policy here that a PR needs to fully fix an issue. One issue why I don't want to make DigitalOutputOperations public yet is that the name is very broad. Shouldn't we make it more specific to the functionality it provides? If we add support for bit patterns we could name it DigitalOutputSequenceModulator |
2 participants
Addressing the second part of #742, simplifying DigitalOutput implementations
Operations are copied as-is, moving execution responsibility to a light executor, cutting the context dependency for non-critical tasks