Climber setup

[Orcasphynx]

Pull request to merge the basic climber subsystem code to the development branch

[github-actions]

AI Code Review

Hey team! This is a fantastic start on the Climber subsystem. I can see a lot of care has gone into following our guidelines, especially with the use of WPILib units and command factory methods. Great job getting this foundational code in place!

Positive Highlights

• Type-Safe Units: I love seeing the consistent use of WPILib's Units system (Angle, Current, Rotations.of()) throughout the constants and subsystem. This is a crucial best practice for preventing errors!
• Command Factory Methods: The climbToPosition, resetClimb, and climbToPreference methods in ClimberSubsystem are excellent examples of command factory methods, encapsulating behavior and keeping the subsystem clean.
• Current-Based Homing: The homeClimber() command effectively uses current to detect the mechanism's home position, which is a robust way to implement automatic homing.
• Configuration Management: Separating constants into ClimberConstants and using factory methods for Slot0Configs and SoftwareLimitSwitchConfigs makes the code very organized and easy to tune.

Suggestions

Here are a few suggestions to make this code even more robust and aligned with our team's best practices:

Code Quality & Java Standards

• Access Modifiers for Constants/Preferences:
  • In ClimberConstants.java and ClimberPreferences.java, consider changing the protected static final fields and protected static methods to package-private (no modifier) or public static final. Since these are final classes with private constructors, they cannot be subclassed, making protected access functionally equivalent to package-private but potentially misleading.
  • Educational Context: Using package-private access (no explicit modifier) for constants and preferences that are only intended for use within the climber package helps encapsulate implementation details. If these constants or methods need to be accessed from outside the climber package, public would be appropriate.
• Consistent PositionTorqueCurrentFOC Initialization:
  • In ClimberSubsystem.java, the climbControl is initialized as new PositionTorqueCurrentFOC(0). While 0 is a double, it might be slightly more consistent with our unit-safe approach to initialize it with climbTarget.in(Rotations) or Rotations.of(0).in(Rotations) to explicitly convey that it's a rotational position.
  • Educational Context: While Java handles the double literal fine, being explicit with unit conversions, even for zero, reinforces the use of WPILib Units.
• Javadoc Documentation:
  • Consider adding Javadoc comments to the ClimberSubsystem class and its public methods (climbToPosition, resetClimb, climbToPreference, homeClimber). Also, add Javadoc for the ClimberConstants and ClimberPreferences classes and their public/package-private members.
  • Educational Context: Clear Javadoc helps other team members (and your future self!) understand the purpose and usage of your code without having to dive into the implementation details. It's especially important for public APIs.

FRC/WPILib Best Practices

• Add Current Limiting:
  • In ClimberConstants.java, I noticed that createClimbMotorSlot0Configs() is defined, but there isn't a method to configure current limits for the TalonFX (e.g., SupplyCurrentLimit, StatorCurrentLimit). Our guidelines recommend protecting motors from overcurrent.
  • Suggestion: Add a static factory method in ClimberConstants to create and apply CurrentLimitsConfigs to the motor.
  • Educational Context: Current limiting is vital for motor longevity and preventing brownouts. It's a proactive safety measure to protect your robot's electronics.
• Logging in periodic():
  • In ClimberSubsystem.java, within the periodic() method, it would be extremely helpful to record telemetry using Logger.recordOutput (assuming we're using AdvantageKit). This allows us to visualize the motor's position, velocity, current, and the climbTarget during matches and replay logs.
  • Suggestion: Add lines like Logger.recordOutput("Climber/Position", climbMotor.getPosition().getValueAsDouble()); and Logger.recordOutput("Climber/Target", climbTarget.in(Rotations)); etc.
  • Educational Context: Good logging is indispensable for debugging, tuning, and understanding robot behavior. It's like having a black box recorder for your robot.
• Automatic Homing with State Management:
  • The homeClimber() command is a great start. For mechanisms that always need to be homed on robot boot or after a reset, our guidelines suggest implementing an explicit HomingState enum within the subsystem's periodic() method. This ensures the subsystem consistently manages its homing status.
  • Suggestion: Consider adding a HomingState enum (e.g., NEEDS_HOMING, HOMING, HOMED) and update it in periodic() based on sensor inputs, rather than relying solely on a command to perform a one-off homing routine. The command could then trigger the HOMING state.
  • Educational Context: A state machine in periodic() provides continuous state management, which is often more robust for critical initialization sequences than a transient command, as it ensures the subsystem is always in a known, safe state.
• Cached Sensor Reads (for robustness):
  • In atClimbSetpoint() and homeClimber(), calls like climbMotor.getPosition().getValueAsDouble() and climbMotor.getStatorCurrent().getValueAsDouble() directly access the TalonFX. While AdvantageKit's LoggedRobot often handles caching, for maximum robustness and to explicitly follow our "Cached Sensor Reads" guideline, consider creating an ClimberIO interface and ClimberIOInputs record. This abstracts the hardware interaction and ensures you're always working with cached values updated once per periodic cycle.
  • Educational Context: An IO interface pattern clearly separates the subsystem's logic from hardware access, making the code more testable, maintainable, and explicitly managing data freshness.

Questions

• Homing Strategy: Is the climber designed to always be homed at the start of a match or after a robot reset, or is homing an action the driver can initiate when needed? This will help determine if the HomingState enum pattern in periodic() is more suitable.
• AdvantageKit Integration: Are we currently using LoggedRobot from AdvantageKit as the base class for Robot.java? Knowing this helps understand how motor inputs are being updated and cached.

This is a really solid foundation for the Climber subsystem. Keep up the great work, and don't hesitate to ask if any of these suggestions are unclear!

---

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!