Reorganize MotorSubsystem; clearly label overrides of Motor, Encoder,… - #92
Reorganize MotorSubsystem; clearly label overrides of Motor, Encoder,…#92vdikov wants to merge 6 commits into
Conversation
… and Subsystem; use 'm_' prefix for MotorSubsystem members
…lasses inheriting from MotorSubsystem directly access these fields and that PR started creating breakages
a1a4b2b to
273d258
Compare
… it turned out, these are used by subclasses and cannot be changed without breaking user code
…hanged in this branch
|
|
||
| private boolean isEnabled; | ||
| /** A configuration for a MotorSubsystem */ | ||
| public static class MotorSubsystemConfiguration { |
There was a problem hiding this comment.
Moving this nested class to another place in the file makes it very hard to see what was changed. Could we leave it where it was and if it needs to be moved move it in a separate PR?
Alternatively have a commit in this PR that moves it but makes no other changes, and clearly state that in the commit message.
| protected final Motor motor; | ||
| protected final Encoder encoder; | ||
| // Specifies if the mechanism is currently under the control of the PID controller. | ||
| private boolean isEnabled; |
There was a problem hiding this comment.
I suspect this field was listed after the others because it is not final.
If we move the fields around, it would be good to have some consistent ordering (ex: private before protected, or final vs non-final).
Having the nested class in the middle is unusual, and I'm fine with defining it after the variables, but moving that in the same PR as other changes does make it harder to review (see below)
| * <p>The MotorSybsystem supports a dual operation mode: | ||
| * | ||
| * <ul> | ||
| * <li><b>PID Mode</b> - the user set a destination position (aka "setpoint") and the motor |
There was a problem hiding this comment.
Thank you for the updates here. This is a huge improvement.
| * Engages the PID Controller. | ||
| * | ||
| * <p>The motor voltage will be periodically updated to move the motor towards the current | ||
| * setupoint. |
There was a problem hiding this comment.
| * setupoint. | |
| * setpoint. |
|
Thanks for the thoughtful review, @kcooney , Thanks to your comments I'm starting to realize I'm wrecking more havoc than doing good with all this refactoring. I learned to understand Java better along the way (e.g., ordering members "private before protected, or final vs non-final" makes more sense to me now). Staying away from I'm going to slash down this PR, and put forward a smaller one, where I'm just adding the documentation to MotorSubsystem, but otherwise keep the class organization pretty much untouched. |
| * controller. The motor system's {@link isEnabled()} returns {@code true}. | ||
| * | ||
| * <p>The <b>Direct User Input Mode</b> is activated when the user calls the {@link | ||
| * set(ControlType,double,double)} or {@link set(ControlType,double)} method, where the user |
There was a problem hiding this comment.
| * set(ControlType,double,double)} or {@link set(ControlType,double)} method, where the user | |
| * set(ControlMode,double,double)} or {@link set(ControlMode,double)} method, where the user |
| * <p>The <b>Direct User Input Mode</b> is activated when the user calls the {@link | ||
| * set(ControlType,double,double)} or {@link set(ControlType,double)} method, where the user | ||
| * provides direct input of type ControlType (specified via {@link | ||
| * MotorSubsystemConfiguration#controlMode(ControlType)}). The PID Mode is interrupted and |
There was a problem hiding this comment.
| * MotorSubsystemConfiguration#controlMode(ControlType)}). The PID Mode is interrupted and | |
| * MotorSubsystemConfiguration#controlMode(ControlMode)}). The PID Mode is interrupted and |
2 participants
… and Subsystem; use 'm_' prefix for MotorSubsystem members