Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Climber subsystem to use state‐driven speed control while introducing a singleton for the Climber class and finalizing some constant refactoring.
- Exposes ClimberStates.getSpeed() as public for external access.
- Adds a speedSetpoint in ClimberIOReal and ClimberIO, along with a state field.
- Introduces a singleton pattern in Climber.java and replaces a TODO comment in the constants with a new static class.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ClimberStates.java | Changed getSpeed() from protected to public to support state‐driven usage. |
| ClimberIOReal.java | Added speedSetpoint field and updated inputs with minimal style adjustments. |
| ClimberIO.java | Added speedSetpoint and state fields in ClimberIOInputs. |
| ClimberConstants.java | Eliminated a TODO by introducing a new static class Real. |
| Climber.java | Introduced a singleton implementation and updated state handling in runState. |
Comments suppressed due to low confidence (1)
src/main/java/frc/robot/Subsystems/Climber/ClimberStates.java:25
- Changing getSpeed's access modifier from protected to public increases its visibility. Please verify that exposing this method aligns with your intended API design without compromising encapsulation.
public double getSpeed() {
src/main/java/frc/robot/Subsystems/Climber/ClimberConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/robot/Subsystems/Climber/ClimberConstants.java
Outdated
Show resolved
Hide resolved
|
You can fix these or lmk and I'll do it |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Climber subsystem to drive motor output purely from its state machine, adds setpoint monitoring, and implements a singleton pattern for Climber.
- Expose
getSpeed()inClimberStatesfor external callers - Track and log
speedSetpointinClimberIOandClimberIOReal - Introduce a singleton (
getInstance()) inClimberand simplifyrunState()to use state speeds - Clean up stray TODO comment in
ClimberConstants
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/frc/robot/Subsystems/Climber/ClimberStates.java | Changed getSpeed() from protected to public |
| src/main/java/frc/robot/Subsystems/Climber/ClimberIOReal.java | Added speedSetpoint field, set it in setSpeed(), and propagate in updateInputs() |
| src/main/java/frc/robot/Subsystems/Climber/ClimberIO.java | Added speedSetpoint to ClimberIOInputs |
| src/main/java/frc/robot/Subsystems/Climber/ClimberConstants.java | Removed redundant // TODO comment |
| src/main/java/frc/robot/Subsystems/Climber/Climber.java | Added singleton instance and getInstance(), replaced manual trigger logic with state-driven speed |
Comments suppressed due to low confidence (2)
src/main/java/frc/robot/Subsystems/Climber/Climber.java:11
- [nitpick] The singleton
instancefield is declared public; consider making it private static and only exposing it viagetInstance()to encapsulate the pattern.
public static Climber instance;
src/main/java/frc/robot/Subsystems/Climber/ClimberIOReal.java:27
- Add or update unit tests to verify that
speedSetpointis correctly stored inClimberIORealand propagated throughupdateInputs.
this.speedSetpoint = speed;
Tortuga-AM
left a comment
There was a problem hiding this comment.
your choice merge or fix.
No description provided.