Feature/pid refactoring

[y0shi]

No description provided.

[github-actions]

AI Code Review

Hello team!

Great work on this pull request! It looks like you're making some significant progress on refactoring your PID control and refining your subsystem configurations. I appreciate the clear changes and the consistent approach across the Intake and Shooter subsystems.

Positive Highlights

1. Enhanced Current Limiting: It's excellent to see the move to explicitly configure both StatorCurrentLimit and SupplyCurrentLimit for the Intake and Shooter motors. This is a crucial safety and longevity measure for your motors, aligning perfectly with CTRE Phoenix 6 best practices.
2. Smart Tuning Workflow: The introduction of lastRPS and lastHoodRot in the ShooterSubsystem to remember previous tuning values is a fantastic quality-of-life improvement. This will make iterating on PID tuning much more efficient and less frustrating for the drive team.
3. Consistent PID Refactoring: The removal of MotionMagicConfigs and the addition of ClosedLoopRampsConfigs across both the Intake and Shooter subsystems shows a clear, consistent strategy for control mode changes. This makes the codebase more predictable and easier to understand.

Suggestions

FRC/WPILib Best Practices

• Intake Extension Control Mode:

  • File: src/main/java/frc/robot/subsystems/intake/IntakeSubsystem.java
  • Location: Lines 36, 88, 109
  • Suggestion: You've transitioned the intake extension motor from PositionTorqueCurrentFOC to PositionVoltage. PositionVoltage can be simpler to tune, but PositionTorqueCurrentFOC (or MotionMagicTorqueCurrentFOC) generally offers more robust control, especially when dealing with external forces or varying loads, as it directly controls motor current/torque rather than just voltage.
  • Why it matters: PositionVoltage might struggle more to hold a position against gravity or external bumps compared to a current-based control. If you're experiencing "springiness" or difficulty holding position, consider re-evaluating if PositionVoltage is the best fit, or ensure your PID gains (especially kP and kD) are well-tuned for this mode.
  • Action: Keep an eye on the intake's performance under load. If it's not holding position well, you might want to consider going back to a current-based position control (like PositionTorqueCurrentFOC) or exploring a Motion Magic variant if you need smooth, dynamic motion.

• Intake Extension Neutral Mode:

  • File: src/main/java/frc/robot/subsystems/intake/IntakeConstants.java
  • Location: Line 120 (createExtensionMotorOutputConfigs())
  • Suggestion: The createExtensionMotorOutputConfigs() method sets the NeutralMode to Coast. Given that the intake extension is a positional mechanism, Brake mode is generally preferred to help hold the mechanism in place when no power is applied, preventing it from drooping or backdriving.
  • Why it matters: Coast mode means the motor will spin freely when idle, which might allow the intake to drift down under gravity. Brake mode actively shorts the motor windings, providing resistance to movement and helping to maintain the set position.
  • Action: Consider changing newConfigs.NeutralMode = NeutralModeValue.Coast; to newConfigs.NeutralMode = NeutralModeValue.Brake; in createExtensionMotorOutputConfigs().

• Intake Compliant Mode Logic:

  • File: src/main/java/frc/robot/subsystems/intake/IntakeSubsystem.java
  • Location: Lines 41, 95, 109, and the commented-out block in setIntakeExtensionCommand
  • Suggestion: The isCompliantMode boolean is initialized to false and doesn't appear to be set to true by any commands in the current code, meaning extensionControl.withSlot(1) (which uses EXTENSION_SPRINGY_KP and EXTENSION_SPRINGY_KD) will never be active. There's also a commented-out block in setIntakeExtensionCommand that previously managed this.
  • Why it matters: If "compliant mode" is an intended feature (perhaps for when the intake is fully extended and needs to absorb impact), it's not currently active. If it's no longer needed, the related code (Slot1Configs, isCompliantMode variable, withSlot logic) could be removed for clarity.
  • Action: Either re-enable the logic to set isCompliantMode (e.g., using runOnce at the start of setIntakeExtensionCommand and finallyDo to reset it), or remove the unused compliant mode components if it's no longer planned.

Java Standards & Code Quality

• Redundant getFlywheelTargetRPMS():
  • File: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java
  • Location: Lines 147-150
  • Suggestion: You have both getFlywheelTargetRPM() (returns RPM) and getFlywheelTargetRPMS() (returns Rotations Per Second). While having both might be useful, velocityTarget.in(RotationsPerSecond) is directly accessible if needed, and the name getFlywheelTargetRPMS() is a bit ambiguous (is it "RPMs" or "Rotations Per Second"?).
  • Why it matters: Clarity in naming helps prevent confusion. If "RPS" is the intended meaning, using getFlywheelTargetRPS() would be clearer.
  • Action: Consider renaming getFlywheelTargetRPMS() to getFlywheelTargetRPS() for better clarity, or evaluate if both methods are truly necessary given velocityTarget is directly accessible if you need the raw RotationsPerSecond value.

Questions

• Regarding the IntakeSubsystem's homeIntakeCommand(): The until condition checks extensionMotor.getStatorCurrent().getValueAsDouble() > IntakeConstants.SAFE_STATOR_LIMIT;. Is SAFE_STATOR_LIMIT (0.8) an empirically determined safe stall current for homing, or is it a placeholder? Have you tested this value to ensure it reliably detects the "home" position without stalling the motor excessively?

Concluding Note

Overall, this is a solid pull request! The refactoring around PID control modes and current limiting is a great step towards a more robust and safer robot. The tuning improvements in the Shooter subsystem are also a testament to thoughtful development. Keep up the excellent work!

---

This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

[github-actions]

✓ Build successful and code formatting check passed!