Stefan/fix disable 79#81
Conversation
…Subsyem for clarity).
| */ | ||
| Current getAppliedCurrent(); | ||
|
|
||
| /** Stops the motor. */ |
There was a problem hiding this comment.
Do you know: does this only cut power off, or does it also actively apply the break to the motor?
I see that for the TalonFXWrapper you can control this, so you might have to clarify here:
Depending on the motor setting, this can cruise or break... or something along those lines.
There was a problem hiding this comment.
This method depends on the individual implementation of the motor controller, it is out of our hands I believe.
There was a problem hiding this comment.
If you are correct that the behavior may differ by motor then let's please update the Javadoc to mention that.
CC @cuttestkittensrule in case he has knowledge about how the APIs of the motors we use might differ.
There was a problem hiding this comment.
The current changes lgtm.
This wouldn't fully fix #79 though, as MotorSubsystem will always work badly with positional control, even in the other methods. If you want to fully fix #79, you should raise an IllegalArgumentException if the end user passes in a control mode that has positional control (currently just MotionMagic)
| * <ul> | ||
| * <li>Coast: The motor stops applying an input, but continues to move with its inertia. | ||
| * <li>Brake: The motor stops applying an input, and actively opposes its inertia. | ||
| * </ul> |
There was a problem hiding this comment.
Same as prior PR; CTRE may add, remove, or modify the meaning of the neutral mode values, so enumerating them isn't a good idea.
cuttestkittensrule
left a comment
cuttestkittensrule
left a comment
There was a problem hiding this comment.
After in person discussion, @spderman3333 and I agreed that we can fix the issues with the subsystem in another PR.
I still am not a fan of the documentation, but it is good enough.
|
|
||
| /** Stops the motor. */ | ||
| void disable(); | ||
| } |
There was a problem hiding this comment.
@spderman3333 as mentioned in the other PR, disable() is a confusing name since the reader could think this disables the controller (especially if a class implements Motor and Controller) and/or that you need to re-enable it somehow.
Given the description of this method, could you please send a PR renaming this to stopMotor()?
There was a problem hiding this comment.
The scope of the PR does not cover refactoring, as the method is introduced as "disable()" in MotorSubsystem, i have decided to name the method accordingly.
There was a problem hiding this comment.
This PR was submitted. My request is to send another, renaming Motor.disable() to stopMotor()
We could deprecate MotorSubsystem.disable() if you'd like
This is indeed the same use-after-free bug I reported to the WPILib maintainers |
This was broken when #81 was merged to main. We do not use a merge queue, so this wasn't caught before the merge.
This was broken when #81 was merged to main. We do not use a merge queue, so GitHub did rerun the build before merging.
This includes the following: - Prospect-Robotics/lib2813#46 - Prospect-Robotics/lib2813#67 - Prospect-Robotics/lib2813#71 - Prospect-Robotics/lib2813#75 - Prospect-Robotics/lib2813#77 - Prospect-Robotics/lib2813#78 - Prospect-Robotics/lib2813#80 - Prospect-Robotics/lib2813#81 - Prospect-Robotics/lib2813#85
4 participants
Only fixes the disabling bug of MotorSubsystem, without any extra code refactoring.