Fixed the disable method in MotorSubystem (now called PositionalMotor… by spderman3333 · Pull Request #73 · Prospect-Robotics/lib2813

spderman3333 · 2025-10-11T00:34:53Z

…Subsystem for clarity).

Other changes:

Motor (interface) now adds a disable method.
TalonFxWrapper now has an implemented disable method (it defaults to coast, however).
SparkMaxWrapper has been marked as deprecated and for removal, alongside implementing the disable method (no clue if it works tho).
MotorSubsystem has been renamed to PositionalMotorSubsystem for clarity reasons.
PositionalMotorSubsystem now properly implements the disable method of the Motor interface.

…Subsyem for clarity).

…orSubsytem)

cuttestkittensrule

See my comments for my specific thoughts on individual thoughs
I do have a question though: the title of the PR says you fixed the disable method of MotorSubsystem, but the implementation does the exact thing (minus calling disable instead of set, which should be equivalent iff disable is implemented correctly, which it isn't)

cuttestkittensrule · 2025-10-11T07:56:15Z


 # Exclude local preference files generated by VSCode editors.
 .vscode/
+.idea/


I don't think we actually want to remove the entire .idea directory.
If we want it to be auto imported, we can uncomment these lines.
Even if we do want to do this, it would probably be better to have a separate discussion on this.

@spderman3333 either revert, or address in a separate PR. Generally, you want to avoid bundling unrelated changes in the same PR (this here being some random config change that has nothing to do with the PR Title, which is all about fixing some logical error in the motor drive impl).

cuttestkittensrule · 2025-10-11T07:59:53Z

+   * <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>


Enumerating the neutral modes is probably a bad idea because they are already present in the Phoenix API docs, and may change in the future

For now, its easier for anyone reading, that does not want to open a whole new tab.

cuttestkittensrule · 2025-10-11T08:06:38Z

+   */
+  @Override
+  public void disable() {
+    motor.disable();


You are probably looking for motor.stopMotor(), or even better, motor.set(new NeutralOut()) to be more explicit with the intention. The documentation from motor.disable() implies that it stops the motor controller instead of the output of the motor.

@spderman3333 given all the confusion as to the behavior of disable() on the various vendor motor classes, perhaps the new method you are adding to Motor should be named stopMotor(), not disable().

cuttestkittensrule · 2025-10-11T08:17:43Z

+  /**
+   * Clamps the given output value and provides it to the motor.
+   *
+   * <p>This was protected and non-final to allow subclasses to clamp the output. Subclasses should
+   * override {@link #clampOutput(double)}.
+   *
+   * @param output The output calculated by the PID algorithm.
+   * @param setpoint Ignored.
+   * @deprecated Subclasses should override {@link #clampOutput(double)}.
+   */
+  @Deprecated
+  protected void useOutput(double output, double setpoint) {
+    motor.set(controlMode, clampOutput(output));
+  }


We are already making a breaking change by renaming this class, so if we want to remove this deprecated code, now would be an amazing time to do it

The scope of the PR does not cover code refactoring, please make an issue so I can address this later.

+1 to keeping PRs small in scope (generally, we should prefer many and small PRs in a sequence, over few but big ones). This one is already "too" big by including random .gitignore changes. I would have kept the renaming in a separate PR too.

spderman3333 · 2025-10-11T22:13:17Z

Note: This PR is here to fix a critical bug which could lead to personal injury or damage to the robot, the ideas for fixes are great, but this PR should be merged ASAP.

spderman3333 · 2025-10-11T22:44:53Z

Closed: will split into seperate formatting and functionallity PRs

vdikov · 2025-10-11T22:31:22Z


 # Exclude local preference files generated by VSCode editors.
 .vscode/
+.idea/


@spderman3333 either revert, or address in a separate PR. Generally, you want to avoid bundling unrelated changes in the same PR (this here being some random config change that has nothing to do with the PR Title, which is all about fixing some logical error in the motor drive impl).

vdikov · 2025-10-11T22:32:05Z

Same here, keep config changes out of this PR.

vdikov · 2025-10-11T22:37:29Z

+  /**
+   * Clamps the given output value and provides it to the motor.
+   *
+   * <p>This was protected and non-final to allow subclasses to clamp the output. Subclasses should
+   * override {@link #clampOutput(double)}.
+   *
+   * @param output The output calculated by the PID algorithm.
+   * @param setpoint Ignored.
+   * @deprecated Subclasses should override {@link #clampOutput(double)}.
+   */
+  @Deprecated
+  protected void useOutput(double output, double setpoint) {
+    motor.set(controlMode, clampOutput(output));
+  }


+1 to keeping PRs small in scope (generally, we should prefer many and small PRs in a sequence, over few but big ones). This one is already "too" big by including random .gitignore changes. I would have kept the renaming in a separate PR too.

vdikov · 2025-10-11T22:39:11Z


 /**
- * Defines PID control over a motor, with values specified by an encoder
+ * This class is a mask of PositionalMotorSubsystem, kept for backwards compatibility.


"mask" typically means other things in programming (bit masks, pixel masks, etc.) You might call this "alias" here instead.

cuttestkittensrule · 2025-10-12T15:31:32Z

This PR is here to fix a critical bug which could lead to personal injury or damage to the robot

@spderman3333 what specifically is the bug? Could you create a GitHub issue describing the bug that we can mark as closed after it is fixed? If we know exactly what the issue is, then we can probably add a regression test to ensure it stays fixed.

kcooney · 2025-10-13T00:45:55Z

Note: This PR is here to fix a critical bug which could lead to personal injury or damage to the robot, the ideas for fixes are great, but this PR should be merged ASAP.

@spderman3333 If you feel that the bug fix is that problematic, I strongly recommend fixing the bug in a separate PR first. Small changes are especially important for critical bugs since people sometimes feel pressure to rush and minimal changes are often safer.

Fixed the disable method in MotorSubystem (now called PositionalMotor…

c646ea4

…Subsyem for clarity).

spderman3333 requested review from cuttestkittensrule, kcooney and vdikov October 11, 2025 00:34

spderman3333 self-assigned this Oct 11, 2025

spderman3333 added the enhancement New feature or request label Oct 11, 2025

Added .idea to .gitignore

c6fdfa2

kcooney reviewed Oct 11, 2025

View reviewed changes

Comment thread lib/src/main/java/com/team2813/lib2813/subsystems/PositionalMotorSubsystem.java

Added MotorSubsytem for backwards compat. (its just a mask of Pos.Mot…

5d1390e

…orSubsytem)

spderman3333 requested a review from kcooney October 11, 2025 04:21

Spotless: Applied

f53c6f0

cuttestkittensrule reviewed Oct 11, 2025

View reviewed changes

kcooney reviewed Oct 11, 2025

View reviewed changes

Comment thread lib/src/main/java/com/team2813/lib2813/subsystems/PositionalMotorSubsystem.java

kcooney reviewed Oct 11, 2025

View reviewed changes

Comment thread lib/src/main/java/com/team2813/lib2813/control/Motor.java

spderman3333 added 2 commits October 11, 2025 15:20

Addressed some recommendations

14cfbb5

My code was not spotless.

42d57ff

spderman3333 closed this Oct 11, 2025

vdikov reviewed Oct 11, 2025

View reviewed changes

kcooney deleted the stefan/motor_disable_fix branch February 9, 2026 07:59

Uh oh!

Conversation

spderman3333 commented Oct 11, 2025

Other changes:

Uh oh!

Uh oh!

cuttestkittensrule left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcooney Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spderman3333 commented Oct 11, 2025

Uh oh!

spderman3333 commented Oct 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuttestkittensrule commented Oct 12, 2025

Uh oh!

kcooney commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kcooney Oct 11, 2025 •

edited

Loading