More Motor Commands #203
More Motor Commands #203
Conversation
Still referenced pipes. Good times.
| public MotorControl(Motor motor, Controller controller, int axis, double scale) { | ||
| super("MotorControl"); | ||
| public MotorControl(String name, Motor motor, Supplier<Double> speedSupplier, double scale, double offset) { | ||
| super(name); |
There was a problem hiding this comment.
This is pretty cool, however, we may run into compatibility problems with older versions. 10/10 would recommend for 2018-code, and maybe for 2017 if we take the time to do some refactoring.
There was a problem hiding this comment.
Compatibility is something we rarely consider as we are not supporting a huge number of people on historical versions. If older software relies on something, a branch can be created to support it, or the older software can adapt.
commands/motor/MotorControl.java
Outdated
| * @param axis | ||
| * @param scale | ||
| */ | ||
| public MotorControl(Motor motor, Supplier<Double> speedSupplier, double scale) { |
There was a problem hiding this comment.
What is the point of this? Where does the scale get factored in/used?
There was a problem hiding this comment.
Yeah -- it looks like scale is just stored on the instance, and then never used. IMO scale is irrelevant when using speedSupplier; it was used when reading directly from the joystick, but with DoubleSupplier the user could just add the scale factor in the supplier itself.
Nice catch.
|
This goes for the whole PR (and it's not critical), but there's a preexisting specialization of |
|
|
||
| @Override | ||
| protected void initialize() { | ||
| speed = speedSupply.get() * scale + offset; |
There was a problem hiding this comment.
As @ajnadel mentioned, the scale is unnecessary if we can just adjust this in the supplier, no?
|
|
Also, like someone should make sure I didn't mess anything up. |
motorsuppliedconstant
|
I moved all of the scalings and offset into the supplier of the double, as that intuitively makes sense... we want the speed to be set in one location, not partially set in one location then finished being set and setting the motor to that speed in the other. That said if anyone knows of any places where this might cause compatibility issues, please double check before merging. |
Adding some more motor commands after seeing some potential use cases.
Also trying to move to better constructors and double suppliers where possible (for more options in the future), thus adding the controller to double supplier adapter.