18 basic climb#25
Open
thartnett27-sudo wants to merge 48 commits intomainfrom
Open
Conversation
Contributor
Summary by CodeRabbitRelease NotesNew Features
WalkthroughAdds a Climb subsystem: new Changes
Sequence Diagram(s)sequenceDiagram
participant Robot
participant Climb
participant ClimbIO
participant Motor_CANcoder
Robot->>Climb: periodic()
Climb->>ClimbIO: updateInputs(inputs)
ClimbIO->>Motor_CANcoder: read sensors & status
Motor_CANcoder-->>ClimbIO: pos, vel, temps, currents
ClimbIO-->>Climb: populated ClimbIOInputs
Climb->>Climb: handleCurrentState()
alt state = MOVING
Climb->>ClimbIO: setPosition(targetRads)
else state = MANUAL
Climb->>ClimbIO: runVolts(filteredVolts)
else other
Climb->>ClimbIO: holdPosition(targetRads)
end
Climb->>Robot: update RobotState.climbState
sequenceDiagram
participant Client
participant Climb
participant RobotState
Client->>Climb: setGoalState(LEVEL_ONE)
Climb->>Climb: validate goal (reject MOVING)
Climb->>RobotState: setClimbState(goalState)
Note over Climb: update lastStateChange timestamp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/team5924/frc2026/subsystems/exampleSystem/ExampleSystem.java (1)
32-85: Avoid unconditional disconnect notificationsInput polling/logging and connectivity tracking are commented out, so the disconnect notification fires every periodic tick. That spams alerts and no longer reflects actual motor connectivity. Restore input updates and gate the notification on a transition (or remove the notification entirely).
🔧 Suggested fix (restore gated notification flow)
- // private final ExampleSystemIOInputsAutoLogged inputs = new ExampleSystemIOInputsAutoLogged(); + private final ExampleSystemIOInputsAutoLogged inputs = + new ExampleSystemIOInputsAutoLogged(); `@Override` public void periodic() { - // io.updateInputs(inputs); - // Logger.processInputs("ExampleSystem", inputs); + io.updateInputs(inputs); + Logger.processInputs("ExampleSystem", inputs); Logger.recordOutput("ExampleSystem/GoalState", goalState.toString()); Logger.recordOutput( "ExampleSystem/CurrentState", RobotState.getInstance().getExampleSystemState()); Logger.recordOutput("ExampleSystem/TargetRads", goalState.rads); - // exampleMotorDisconnected.set(!inputs.exampleMotorConnected); + exampleMotorDisconnected.set(!inputs.exampleMotorConnected); // prevents error spam - // if (!inputs.exampleMotorConnected && wasExampleMotorConnected) { - Elastic.sendNotification(exampleMotorDisconnectedNotification); - } + if (!inputs.exampleMotorConnected && wasExampleMotorConnected) { + Elastic.sendNotification(exampleMotorDisconnectedNotification); + } + wasExampleMotorConnected = inputs.exampleMotorConnected; + } - - // wasExampleMotorConnected = inputs.exampleMotorConnected; - // }
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 95-98: The switch case in Climb.java that handles the enum STOW
currently reports "MOVING is an invalid goal state" and either should be removed
or corrected: decide if STOW is a valid goal—if yes, delete the entire case
block (or change logic to allow STOW as a normal goal) and remove the erroneous
DriverStation.reportError call; if STOW must be blocked, update the
DriverStation.reportError call to reference "STOW is an invalid goal state" (and
leave the break) so the log matches the handled enum; locate the case labeled
STOW in the Climb class to apply the change.
- Around line 78-80: The empty if is a no-op; replace it with logic that fires
the climbMotorDisconnectedNotification when inputs.climbMotorConnected
transitions from true to false (i.e., when !inputs.climbMotorConnected &&
wasClimbMotorConnected) by creating/sending a notification via the Elastic
helper and setting the climbMotorDisconnectedNotification field accordingly so
it only notifies once; also add the import import
org.team5924.frc2026.util.Elastic; and ensure the transition flag
(wasClimbMotorConnected) is updated after handling.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOInputs.java`:
- Around line 21-27: The two fields climbVelocityRadsPerSec and
climbPositionRads in ClimbIOInputs are declared as Object which breaks type
safety and diverges from ClimbIO.ClimbIOInputs; change their types to double to
match ClimbIO.ClimbIOInputs, update any getters/setters/usages and logging that
assume Object to use primitive doubles (and handle any NaN/defaults if needed),
and run compilation to fix resulting casts/usages so the inputs class remains
strongly typed and consistent.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 93-101: ClimbIOTalonFX currently lacks an override for setPosition
so calling io.setPosition(...) is a no-op; implement setPosition in
ClimbIOTalonFX to drive the Talon to the requested angle by calling
climbTalon.setControl with the position control command (e.g. use your existing
positionControl object or factory and call positionControl.withPosition(...)).
Convert the input radians to the motor/encoder native units before passing to
withPosition (use your gear ratio/encoder resolution or any existing helper like
radiansToNative or kRadiansPerTick constant), and ensure the method switches to
position control mode rather than voltage control so the motor actually moves to
the goal.
🧹 Nitpick comments (4)
src/main/java/org/team5924/frc2026/RobotState.java (1)
52-53: Clarify the climb section headerThe new
climbStatefield is labeled as “Example Subsystem,” which is misleading in the shared state model. Rename the header for clarity.✏️ Suggested change
- /* ### Example Subsystem ### */ + /* ### Climb Subsystem ### */src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
45-46: UnusedpositionOutfield.
positionOutis declared but never used in this class. IfsetPositionfunctionality is needed (it's called inClimb.javaline 101), this control request should be used. Otherwise, remove it.src/main/java/org/team5924/frc2026/Constants.java (1)
144-161: Defined configs not applied to TalonFX.
CANDI_CONFIG,OPEN_LOOP_RAMPS_CONFIGS, andCLOSED_LOOP_RAMPS_CONFIGSare defined but not used inClimbIOTalonFX. If these are needed, they should be applied in the constructor. If they're for future use, consider adding a TODO comment or removing them to reduce clutter.src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (1)
42-48: Naming inconsistency: field namedradsbut OPERATOR_CONTROL represents voltage.The
OPERATOR_CONTROLstate comment says "voltage at which the climb subsystem motor moves" but the enum field is namedrads. This creates confusion since this value is used differently than the position-based states.Consider either:
- Renaming the field to something more generic like
value- Separating voltage-based and position-based state handling more explicitly
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Line 76: Logger.recordOutput is being given the LoggedTunableNumber object
(goalState.rads) instead of its numeric value; update the call to pass the
numeric value by invoking the LoggedTunableNumber accessor (e.g., the get() or
getValue() method on goalState.rads) so Logger.recordOutput receives a
primitive/double rather than the object reference.
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (1)
92-107: RobotState always set to STOW regardless of actual goal.Line 103 sets
RobotStatetoClimbState.STOWfor all states handled by the default case (STOW, LEVEL_ONE, LEVEL_TWO, LEVEL_THREE, CLIMB_DOWN, DEPLOY, DROP). This means the robot state won't accurately reflect the actual climb goal.🔧 Proposed fix
default: - RobotState.getInstance().setClimbState(ClimbState.STOW); + RobotState.getInstance().setClimbState(goalState); io.setPosition(goalState.rads.getAsDouble()); break;
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (1)
35-52: Consider renaming the field or separating voltage from position states.The
radsfield is semantically inconsistent: most states use it for position (radians), butOPERATOR_CONTROLuses it for voltage (4.5V). This could cause confusion during maintenance.Consider either:
- Renaming
radsto a more generic name likevalueortargetValue- Adding a comment clarifying the dual purpose
- Separating voltage and position into distinct fields
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 103-105: The setPosition method is passing radians directly to
positionOut.withPosition; instead convert mechanism radians to motor rotations
before commanding: first convert radians to rotations (rotations = rads /
(2*pi)), then scale by Constants.Climb.REDUCTION to get motor rotations, and
pass that value to climbTalon.setControl(positionOut.withPosition(...)). Update
the setPosition method (referencing setPosition, positionOut.withPosition, and
Constants.Climb.REDUCTION) to apply the radians→rotations conversion and
multiply by REDUCTION so it is the exact inverse of the conversion done in
updateInputs.
…ware implementation)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 105-108: The default branch incorrectly sets
RobotState.getInstance().setClimbState(ClimbState.STOW) while commanding
io.setPosition(goalState.rads.getAsDouble()), causing RobotState to disagree
with the commanded goal; update the default handling so RobotState is set to the
actual goal enum value (e.g., goalState or a corresponding ClimbState like
LEVEL_ONE/LEVEL_TWO/etc.) instead of STOW, locating the logic in Climb.java
where RobotState.getInstance().setClimbState(...) and
io.setPosition(goalState.rads.getAsDouble()) are called and ensuring the chosen
ClimbState matches the goalState being commanded.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (1)
77-79: Inconsistent enum logging.Line 77 explicitly calls
.toString()ongoalState, but line 78 passes the enum directly. For consistency and to ensure proper string representation in logs, consider calling.toString()on the result ofgetClimbState()as well.♻️ Suggested fix
Logger.recordOutput("Climb/GoalState", goalState.toString()); - Logger.recordOutput("Climb/CurrentState", RobotState.getInstance().getClimbState()); + Logger.recordOutput("Climb/CurrentState", RobotState.getInstance().getClimbState().toString()); Logger.recordOutput("Climb/TargetRads", goalState.rads.getAsDouble());
arcadeArchitect
requested changes
Feb 24, 2026
| public static final String BUS = "rio"; | ||
| public static final double MOTOR_TO_CANCODER = (7.0 / 1.0); // TODO: Update Reductions | ||
| public static final double CANCODER_TO_MECHANISM = (7.0 / 1.0); | ||
| public static final double MOTOR_TO_MECHANISM = MOTOR_TO_CANCODER * CANCODER_TO_MECHANISM; |
Contributor
There was a problem hiding this comment.
we'll have to add more constants here for converting btw. distance/rads of the boat hook roll
these will include the starting radius of the roll, boat hook diameter (thickness), maybe more...
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/team5924/frc2026/Constants.java (2)
489-489:⚠️ Potential issue | 🟡 MinorUpdate discontinuity point per reviewer feedback.
Per the earlier review comment from arcadeArchitect, change
AbsoluteSensorDiscontinuityPointto1.0to ensure only positive position values (0→1 range instead of ±0.5 range).- .withAbsoluteSensorDiscontinuityPoint(0.5) + .withAbsoluteSensorDiscontinuityPoint(1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` at line 489, Update the AbsoluteSensorDiscontinuityPoint value from 0.5 to 1.0 by changing the call to withAbsoluteSensorDiscontinuityPoint on the encoder configuration (the line that currently reads .withAbsoluteSensorDiscontinuityPoint(0.5)); this ensures the encoder reports only positive positions in the 0→1 range instead of a ±0.5 range.
442-446:⚠️ Potential issue | 🟠 MajorPosition conversion formula is off by factor of 2.
The comments indicate
MIN_POSITION_MULTIandMAX_POSITION_MULTIare in rotations, but the conversion usesMath.PIinstead of2 * Math.PI. One full rotation equals2πradians, so the current formula yields half the expected range.
Value Current Expected 0.8 rotations 0.8π rad ≈ 144° 1.6π rad ≈ 288° 🔧 Proposed fix
- public static final double MIN_POSITION_RADS = -Math.PI * MIN_POSITION_MULTI; - public static final double MAX_POSITION_RADS = Math.PI * MAX_POSITION_MULTI; + public static final double MIN_POSITION_RADS = -2 * Math.PI * MIN_POSITION_MULTI; + public static final double MAX_POSITION_RADS = 2 * Math.PI * MAX_POSITION_MULTI;Or use
Units.rotationsToRadians()for clarity:- public static final double MIN_POSITION_RADS = -Math.PI * MIN_POSITION_MULTI; - public static final double MAX_POSITION_RADS = Math.PI * MAX_POSITION_MULTI; + public static final double MIN_POSITION_RADS = -Units.rotationsToRadians(MIN_POSITION_MULTI); + public static final double MAX_POSITION_RADS = Units.rotationsToRadians(MAX_POSITION_MULTI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 442 - 446, MIN_POSITION_RADS and MAX_POSITION_RADS use Math.PI but rotations -> radians requires 2*Math.PI; update the conversion so MIN_POSITION_RADS = -2*Math.PI * MIN_POSITION_MULTI and MAX_POSITION_RADS = 2*Math.PI * MAX_POSITION_MULTI (or replace the expressions with Units.rotationsToRadians(MIN_POSITION_MULTI) / Units.rotationsToRadians(MAX_POSITION_MULTI) for clarity) while keeping the MIN_POSITION_MULTI, MAX_POSITION_MULTI constants unchanged.
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/Constants.java (1)
465-466: Fix indentation ofOPEN_LOOP_RAMPS_CONFIGS.The constant declaration has inconsistent indentation compared to other constants in the
Climbclass.- public static final OpenLoopRampsConfigs OPEN_LOOP_RAMPS_CONFIGS = + public static final OpenLoopRampsConfigs OPEN_LOOP_RAMPS_CONFIGS = new OpenLoopRampsConfigs()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 465 - 466, The declaration of the constant OPEN_LOOP_RAMPS_CONFIGS in the Climb class is misindented; locate the OPEN_LOOP_RAMPS_CONFIGS field and adjust its leading spaces to match the indentation level used by the other constants in the Climb class (so it lines up with sibling declarations), ensuring consistent formatting for the class fields and preserving the existing modifiers and assignment.src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (2)
159-164: Use latency-compensated signal values for position control accuracy.The current implementation uses
getValueAsDouble()directly, but for closed-loop position control accuracy, you should useBaseStatusSignal.getLatencyCompensatedValue()to account for CAN bus latency. This was flagged in a previous review.- inputs.climbPositionRads = Units.rotationsToRadians(climbPosition.getValueAsDouble()); - inputs.climbVelocityRadsPerSec = Units.rotationsToRadians(climbVelocity.getValueAsDouble()); + inputs.climbPositionRads = Units.rotationsToRadians( + BaseStatusSignal.getLatencyCompensatedValue(climbPosition, climbVelocity)); + inputs.climbVelocityRadsPerSec = Units.rotationsToRadians(climbVelocity.getValueAsDouble());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 159 - 164, The code reads raw sensor values via getValueAsDouble() for climbPosition, climbVelocity, climbAppliedVoltage, climbSupplyCurrent, climbTorqueCurrent, and climbTempCelsius which fails to account for CAN latency; replace those calls with BaseStatusSignal.getLatencyCompensatedValue() (or the signal's getLatencyCompensatedValue method) when populating inputs.climbPositionRads, inputs.climbVelocityRadsPerSec, and the other inputs so the closed-loop position/velocity calculations use latency-compensated measurements (keep the Units.rotationsToRadians conversion for position and velocity after using the compensated value).
171-173: Clarify units inclimbPositionCancoderfield name.
climbPositionCancoderis computed as mechanism radians (afterrotationsToRadiansand dividing by the gear ratio), but the field name lacks a unit suffix unlikeclimbPositionRads. Consider renaming toclimbPositionCancoderRadsfor consistency and clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 171 - 173, The field inputs.climbPositionCancoder holds mechanism angle in radians (computed via Units.rotationsToRadians(...) / Constants.Climb.CANCODER_TO_MECHANISM) but its name lacks a unit suffix; rename it to inputs.climbPositionCancoderRads (and update every reference: declarations, assignments, getters/setters, uses in ClimbIOTalonFX and any telemetry/serialization code) to match climbPositionRads and clarify units; ensure compilation by updating all symbol occurrences and any related documentation/comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 24-25: Remove the duplicated import statements for
FeedbackConfigs, MagnetSensorConfigs, and FeedbackSensorSourceValue so each type
is imported only once; locate the duplicate import lines in Constants.java (the
import declarations that mention FeedbackConfigs, MagnetSensorConfigs, and
FeedbackSensorSourceValue) and delete the redundant occurrences, leaving a
single import for each symbol.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Line 136: ClimbIOTalonFX currently calls climbTalon.setPosition(0) immediately
without waiting for the CANcoder absolute reading or resetting the CANcoder;
update the initialization to mirror TurretIOTalonFX by waiting for the CANcoder
absolute position readiness (use BaseStatusSignal.waitForAll with a 0.5s timeout
and the cancoder absolute position signal used in this class), then call
climbCANCoder.setPosition(0.0) followed by climbTalon.setPosition(0.0) so both
the CANCoder and Talon are synchronized before enabling closed-loop
FusedCANcoder feedback.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Line 489: Update the AbsoluteSensorDiscontinuityPoint value from 0.5 to 1.0 by
changing the call to withAbsoluteSensorDiscontinuityPoint on the encoder
configuration (the line that currently reads
.withAbsoluteSensorDiscontinuityPoint(0.5)); this ensures the encoder reports
only positive positions in the 0→1 range instead of a ±0.5 range.
- Around line 442-446: MIN_POSITION_RADS and MAX_POSITION_RADS use Math.PI but
rotations -> radians requires 2*Math.PI; update the conversion so
MIN_POSITION_RADS = -2*Math.PI * MIN_POSITION_MULTI and MAX_POSITION_RADS =
2*Math.PI * MAX_POSITION_MULTI (or replace the expressions with
Units.rotationsToRadians(MIN_POSITION_MULTI) /
Units.rotationsToRadians(MAX_POSITION_MULTI) for clarity) while keeping the
MIN_POSITION_MULTI, MAX_POSITION_MULTI constants unchanged.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 465-466: The declaration of the constant OPEN_LOOP_RAMPS_CONFIGS
in the Climb class is misindented; locate the OPEN_LOOP_RAMPS_CONFIGS field and
adjust its leading spaces to match the indentation level used by the other
constants in the Climb class (so it lines up with sibling declarations),
ensuring consistent formatting for the class fields and preserving the existing
modifiers and assignment.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 159-164: The code reads raw sensor values via getValueAsDouble()
for climbPosition, climbVelocity, climbAppliedVoltage, climbSupplyCurrent,
climbTorqueCurrent, and climbTempCelsius which fails to account for CAN latency;
replace those calls with BaseStatusSignal.getLatencyCompensatedValue() (or the
signal's getLatencyCompensatedValue method) when populating
inputs.climbPositionRads, inputs.climbVelocityRadsPerSec, and the other inputs
so the closed-loop position/velocity calculations use latency-compensated
measurements (keep the Units.rotationsToRadians conversion for position and
velocity after using the compensated value).
- Around line 171-173: The field inputs.climbPositionCancoder holds mechanism
angle in radians (computed via Units.rotationsToRadians(...) /
Constants.Climb.CANCODER_TO_MECHANISM) but its name lacks a unit suffix; rename
it to inputs.climbPositionCancoderRads (and update every reference:
declarations, assignments, getters/setters, uses in ClimbIOTalonFX and any
telemetry/serialization code) to match climbPositionRads and clarify units;
ensure compilation by updating all symbol occurrences and any related
documentation/comments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/RobotState.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/team5924/frc2026/RobotState.java
- src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/team5924/frc2026/Constants.java (1)
432-436:⚠️ Potential issue | 🟡 MinorIncorrect indentation.
OPEN_LOOP_RAMPS_CONFIGSdeclaration has extra leading spaces beforepublic static final, making it inconsistent with other constants in this class.🔧 Proposed fix
- public static final OpenLoopRampsConfigs OPEN_LOOP_RAMPS_CONFIGS = + public static final OpenLoopRampsConfigs OPEN_LOOP_RAMPS_CONFIGS = new OpenLoopRampsConfigs()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 432 - 436, The declaration for OPEN_LOOP_RAMPS_CONFIGS is mis-indented compared to other constants; adjust the leading whitespace so the line starting with "public static final OpenLoopRampsConfigs OPEN_LOOP_RAMPS_CONFIGS" aligns with the other top-level constant declarations in Constants.java and keep the chained .with... calls indented as the surrounding style uses (aligning the new OpenLoopRampsConfigs() call and its fluent methods consistently with neighboring constants).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 500-504: The declaration of OPEN_LOOP_RAMPS_CONFIGS in Constants
(symbol OPEN_LOOP_RAMPS_CONFIGS) is misindented; remove the extra leading spaces
so the line starts aligned with other members (match the indentation used at
line 432 for similar declarations) and ensure the chained .with... calls remain
correctly indented underneath the declaration.
---
Outside diff comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 432-436: The declaration for OPEN_LOOP_RAMPS_CONFIGS is
mis-indented compared to other constants; adjust the leading whitespace so the
line starting with "public static final OpenLoopRampsConfigs
OPEN_LOOP_RAMPS_CONFIGS" aligns with the other top-level constant declarations
in Constants.java and keep the chained .with... calls indented as the
surrounding style uses (aligning the new OpenLoopRampsConfigs() call and its
fluent methods consistently with neighboring constants).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/java/org/team5924/frc2026/generated/TunerConstants.javais excluded by!**/generated/**
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/RobotState.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHoodIOTalonFX.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/RobotState.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/Constants.java (1)
473-477:⚠️ Potential issue | 🟠 MajorRadian conversion for position limits is off by a factor of 2.
MIN_POSITION_MULTI/MAX_POSITION_MULTIare labeled as rotations, but lines 476-477 useMath.PI * ...instead of full-rotation conversion.🔧 Proposed fix
- public static final double MIN_POSITION_RADS = -Math.PI * MIN_POSITION_MULTI; - public static final double MAX_POSITION_RADS = Math.PI * MAX_POSITION_MULTI; + public static final double MIN_POSITION_RADS = Units.rotationsToRadians(-MIN_POSITION_MULTI); + public static final double MAX_POSITION_RADS = Units.rotationsToRadians(MAX_POSITION_MULTI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 473 - 477, MIN_POSITION_MULTI and MAX_POSITION_MULTI are expressed as rotations but MIN_POSITION_RADS and MAX_POSITION_RADS are converting with Math.PI (half-rotation); change the conversion to use 2*Math.PI so radians = rotations * 2*Math.PI (update MIN_POSITION_RADS and MAX_POSITION_RADS to Math.PI * 2 * MIN_POSITION_MULTI / MAX_POSITION_MULTI or 2*Math.PI * ...), and optionally clarify the comment on MIN_POSITION_MULTI/MAX_POSITION_MULTI to note "rotations".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 112-116: The timeout check in isAtSetpoint() is inverted: replace
the current "RobotState.getTime() - lastStateChange <
Constants.Climb.STATE_TIMEOUT" with a check that the timeout has elapsed (e.g.,
"RobotState.getTime() - lastStateChange > Constants.Climb.STATE_TIMEOUT" or
">=") so the method does not return true immediately after a goal change; keep
the existing epsilon comparison using inputs.setpointRads and
inputs.climbPositionRads with Constants.Climb.EPSILON_RADS unchanged.
- Around line 129-131: In handleManualState(), the code incorrectly compares
goalState to TurretState.MANUAL which prevents manual control from ever running;
change the comparison to ClimbState.MANUAL so the branch executes (update the
condition in handleManualState to compare goalState against ClimbState.MANUAL),
then verify the subsequent manual voltage assignment block that uses
goalState/goalVoltage runs as intended.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 85-86: The clamp bounds for radians are being loaded from the
multiplier constants: replace uses of Constants.Climb.MIN_POSITION_MULTI and
MAX_POSITION_MULTI with the radians constants Constants.Climb.MIN_POSITION_RADS
and Constants.Climb.MAX_POSITION_RADS where minPositionRads and maxPositionRads
are initialized and any subsequent clamp logic (including the clamp in
setPosition()). Ensure the values used to clamp the incoming radians setpoint
are in radians (not multipliers) so conversions and setpoint calculations in
setPosition() remain correct.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 473-477: MIN_POSITION_MULTI and MAX_POSITION_MULTI are expressed
as rotations but MIN_POSITION_RADS and MAX_POSITION_RADS are converting with
Math.PI (half-rotation); change the conversion to use 2*Math.PI so radians =
rotations * 2*Math.PI (update MIN_POSITION_RADS and MAX_POSITION_RADS to Math.PI
* 2 * MIN_POSITION_MULTI / MAX_POSITION_MULTI or 2*Math.PI * ...), and
optionally clarify the comment on MIN_POSITION_MULTI/MAX_POSITION_MULTI to note
"rotations".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIO.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/team5924/frc2026/Constants.java (1)
580-581:⚠️ Potential issue | 🟠 Major
MIN/MAX_POSITION_RADSuse half-rotation conversion while comments say rotations.For full rotations, use
Units.rotationsToRadians(...)(or2 * Math.PI * ...), otherwise the bounds are off by 2×.🔧 Suggested fix
- public static final double MIN_POSITION_RADS = -Math.PI * MIN_POSITION_MULTI; - public static final double MAX_POSITION_RADS = Math.PI * MAX_POSITION_MULTI; + public static final double MIN_POSITION_RADS = -Units.rotationsToRadians(MIN_POSITION_MULTI); + public static final double MAX_POSITION_RADS = Units.rotationsToRadians(MAX_POSITION_MULTI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 580 - 581, The constants MIN_POSITION_RADS and MAX_POSITION_RADS currently multiply Math.PI by MIN_POSITION_MULTI/MAX_POSITION_MULTI (half-rotation) but comments and intent expect full rotations; update their computation to use full-rotation conversion (e.g., Units.rotationsToRadians(MIN_POSITION_MULTI) and Units.rotationsToRadians(MAX_POSITION_MULTI) or 2 * Math.PI * MIN_POSITION_MULTI and 2 * Math.PI * MAX_POSITION_MULTI) so the radian bounds match the configured rotation multipliers.src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (2)
129-131:⚠️ Potential issue | 🔴 CriticalManual state guard compares against the wrong enum type.
goalStateisClimbState; comparing toTurretState.MANUALmakes this branch unreachable, so manual voltage control never executes.🔧 Suggested fix
private void handleManualState() { - if (!goalState.equals(TurretState.MANUAL)) return; + if (goalState != ClimbState.MANUAL) return;#!/bin/bash # Verify no cross-enum manual comparison remains in Climb.java rg -nP --type java 'TurretState\.MANUAL|goalState\.equals\(TurretState\.MANUAL\)' src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java -C 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java` around lines 129 - 131, In handleManualState() in class Climb, the guard incorrectly compares goalState (a ClimbState) to TurretState.MANUAL, making the branch unreachable; change the comparison to use ClimbState.MANUAL (or the correct ClimbState enum constant) so the manual-voltage control executes when goalState == ClimbState.MANUAL, and update any related imports or fully-qualified references if necessary (search for goalState.equals(TurretState.MANUAL) or TurretState.MANUAL in Climb.java to locate all occurrences).
112-116:⚠️ Potential issue | 🟠 MajorTimeout check in
isAtSetpoint()is inverted.This currently returns true immediately after a goal change, so transition logic can complete before the mechanism reaches setpoint.
🔧 Suggested fix
public boolean isAtSetpoint() { - return RobotState.getTime() - lastStateChange < Constants.Climb.STATE_TIMEOUT + return RobotState.getTime() - lastStateChange > Constants.Climb.STATE_TIMEOUT || EqualsUtil.epsilonEquals( inputs.setpointRads, inputs.climbPositionRads, Constants.Climb.EPSILON_RADS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java` around lines 112 - 116, The isAtSetpoint() timeout check is inverted: it currently returns true immediately after a setpoint change. In the isAtSetpoint method, flip the comparison so that RobotState.getTime() - lastStateChange > Constants.Climb.STATE_TIMEOUT (rather than <) so the method only reports “at setpoint” after the timeout has elapsed or when EqualsUtil.epsilonEquals(inputs.setpointRads, inputs.climbPositionRads, Constants.Climb.EPSILON_RADS) is true; update the comparison operator where RobotState.getTime(), lastStateChange, and Constants.Climb.STATE_TIMEOUT are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 140-155: The setGoalState method currently assigns this.goalState
and updates lastStateChange before rejecting an invalid MOVING request; change
the logic so you first validate the incoming goalState and if it equals
ClimbState.MOVING call DriverStation.reportError(...) and return without
mutating this.goalState, RobotState (via
RobotState.getInstance().setClimbState(...)) or lastStateChange; for valid
states (MANUAL and others) perform the assignments and RobotState update as
before—use the existing method name setGoalState and fields goalState and
lastStateChange to locate the code to modify.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 580-581: The constants MIN_POSITION_RADS and MAX_POSITION_RADS
currently multiply Math.PI by MIN_POSITION_MULTI/MAX_POSITION_MULTI
(half-rotation) but comments and intent expect full rotations; update their
computation to use full-rotation conversion (e.g.,
Units.rotationsToRadians(MIN_POSITION_MULTI) and
Units.rotationsToRadians(MAX_POSITION_MULTI) or 2 * Math.PI * MIN_POSITION_MULTI
and 2 * Math.PI * MAX_POSITION_MULTI) so the radian bounds match the configured
rotation multipliers.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 129-131: In handleManualState() in class Climb, the guard
incorrectly compares goalState (a ClimbState) to TurretState.MANUAL, making the
branch unreachable; change the comparison to use ClimbState.MANUAL (or the
correct ClimbState enum constant) so the manual-voltage control executes when
goalState == ClimbState.MANUAL, and update any related imports or
fully-qualified references if necessary (search for
goalState.equals(TurretState.MANUAL) or TurretState.MANUAL in Climb.java to
locate all occurrences).
- Around line 112-116: The isAtSetpoint() timeout check is inverted: it
currently returns true immediately after a setpoint change. In the isAtSetpoint
method, flip the comparison so that RobotState.getTime() - lastStateChange >
Constants.Climb.STATE_TIMEOUT (rather than <) so the method only reports “at
setpoint” after the timeout has elapsed or when
EqualsUtil.epsilonEquals(inputs.setpointRads, inputs.climbPositionRads,
Constants.Climb.EPSILON_RADS) is true; update the comparison operator where
RobotState.getTime(), lastStateChange, and Constants.Climb.STATE_TIMEOUT are
used.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIO.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
100-101:⚠️ Potential issue | 🔴 CriticalClamp bounds are in the wrong units (radians vs multipliers).
Line 100 and Line 101 load clamp bounds from
*_MULTI, but Line 319 clamps radians. This can force incorrect setpoints.🔧 Proposed fix
- minPositionRads = Constants.Climb.MIN_POSITION_MULTI; - maxPositionRads = Constants.Climb.MAX_POSITION_MULTI; + minPositionRads = Constants.Climb.MIN_POSITION_RADS; + maxPositionRads = Constants.Climb.MAX_POSITION_RADS;Also applies to: 318-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 100 - 101, The clamp bounds are being loaded from multiplier constants but used to clamp radians (minPositionRads/maxPositionRads are set from Constants.Climb.MIN_POSITION_MULTI and MAX_POSITION_MULTI); change the initialization in ClimbIOTalonFX so minPositionRads and maxPositionRads are assigned the radian constants (e.g., Constants.Climb.MIN_POSITION_RADS / MAX_POSITION_RADS or the proper radian equivalents), and ensure any clamping logic that uses minPositionRads/maxPositionRads (the clamp around the setpoint near the code that currently clamps at "line 319") operates in the same units (radians) or converts multipliers to radians consistently.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
274-276: Use a stable instance key for Motion Magic tunable updates.Line 275 uses
ifChanged(0, ...); prefer the instance key (same pattern as Line 245) to avoid cross-caller collisions.🔧 Proposed fix
- LoggedTunableNumber.ifChanged( - 0, + LoggedTunableNumber.ifChanged( + hashCode(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 274 - 276, The LoggedTunableNumber.ifChanged call inside ClimbIOTalonFX currently passes a literal 0 as the instance key; replace that literal with the stable instance key used elsewhere in this class (the same identifier used on the earlier LoggedTunableNumber.ifChanged call at line ~245) so the Motion Magic tunable updates use a consistent per-instance key (i.e., change LoggedTunableNumber.ifChanged(0, () -> { ... }) to LoggedTunableNumber.ifChanged(instance, () -> { ... }) or whatever the class-level per-instance identifier is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 123-130: The statusArray in ClimbIOTalonFX duplicates
Constants.Climb.FEEDBACK_CONFIGS for statusArray[3] and statusArray[4]; remove
the duplicate and replace statusArray[4] with the intended config (or remove the
slot) by updating the climbTalonConfig.apply calls so each index uses a unique
config constant (e.g., ensure you call climbTalonConfig.apply with the correct
constant instead of FEEDBACK_CONFIGS for statusArray[4], and verify the array
size and order still align with motionMagicConfigs and other expected configs).
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 100-101: The clamp bounds are being loaded from multiplier
constants but used to clamp radians (minPositionRads/maxPositionRads are set
from Constants.Climb.MIN_POSITION_MULTI and MAX_POSITION_MULTI); change the
initialization in ClimbIOTalonFX so minPositionRads and maxPositionRads are
assigned the radian constants (e.g., Constants.Climb.MIN_POSITION_RADS /
MAX_POSITION_RADS or the proper radian equivalents), and ensure any clamping
logic that uses minPositionRads/maxPositionRads (the clamp around the setpoint
near the code that currently clamps at "line 319") operates in the same units
(radians) or converts multipliers to radians consistently.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 274-276: The LoggedTunableNumber.ifChanged call inside
ClimbIOTalonFX currently passes a literal 0 as the instance key; replace that
literal with the stable instance key used elsewhere in this class (the same
identifier used on the earlier LoggedTunableNumber.ifChanged call at line ~245)
so the Motion Magic tunable updates use a consistent per-instance key (i.e.,
change LoggedTunableNumber.ifChanged(0, () -> { ... }) to
LoggedTunableNumber.ifChanged(instance, () -> { ... }) or whatever the
class-level per-instance identifier is).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIO.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
214-216:⚠️ Potential issue | 🔴 CriticalClosed-loop unit conversion appears to double-apply climb gearing.
With
Constants.Climb.FEEDBACK_CONFIGSapplied (Line 129), Talon closed-loop units are already ratio-scaled; applyingmotorToMechanismagain in helper conversions can skew commands/targets by the ratio factor.🔧 Proposed fix (if Talon closed-loop units are mechanism rotations)
private double radsToMotorPosition(double rads) { - return Units.radiansToRotations(rads * motorToMechanism); + return Units.radiansToRotations(rads); } private double motorPositionToRads(double motorPosition) { - return Units.rotationsToRadians(motorPosition / motorToMechanism); + return Units.rotationsToRadians(motorPosition); }In CTRE Phoenix 6, when TalonFX FeedbackConfigs sets RotorToSensorRatio and SensorToMechanismRatio, are Position/MotionMagic setpoints and getPosition()/getClosedLoopReference() reported in mechanism rotations?Also applies to: 308-316, 322-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 214 - 216, The conversion is double-applying the climb gearing because Constants.Climb.FEEDBACK_CONFIGS already sets RotorToSensorRatio/SensorToMechanismRatio on climbTalon, so values from climbTalon.getClosedLoopReference() and getClosedLoopReferenceSlope() are already in mechanism units; remove the extra motor-to-mechanism scaling by not calling motorPositionToRads/motorVelocityToRads (or motorToMechanism) on those closed-loop reads and instead assign them directly (or update the helpers to accept mechanism-scaled inputs). Update occurrences around motorPositionToRads(climbTalon.getClosedLoopReferenceSlope().getValueAsDouble()), inputs.motionMagicPositionTarget = motorPositionToRads(climbTalon.getClosedLoopReference().getValueAsDouble()), and the similar blocks at 308-316 and 322-327 so closed-loop reference values are treated as already-scaled by FEEDBACK_CONFIGS.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
274-276: Use an instance-specificifChangedid instead of0.Using a global literal id can cross-couple change tracking between subsystems.
🔧 Proposed fix
- LoggedTunableNumber.ifChanged( - 0, + LoggedTunableNumber.ifChanged( + hashCode(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 274 - 276, The call to LoggedTunableNumber.ifChanged currently passes the literal id 0 which can cross-couple change tracking; replace the literal with an instance-specific id (e.g., a private final int instanceId initialized once per ClimbIOTalonFX instance using a unique counter or System.identityHashCode(this) or a per-instance TunableNumber id) and use that instanceId in the LoggedTunableNumber.ifChanged invocation so each ClimbIOTalonFX has a unique change-tracking key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 619-624: The forward/reverse soft limits are inverted so both
thresholds evaluate to the same positive value; update SOFTWARE_LIMIT_CONFIGS to
set withForwardSoftLimitThreshold to MAX_POSITION_MULTI * MOTOR_TO_MECHANISM and
withReverseSoftLimitThreshold to MIN_POSITION_MULTI * MOTOR_TO_MECHANISM (or
otherwise ensure the reverse threshold is the negative/min value), keeping the
use of SoftwareLimitSwitchConfigs, withForwardSoftLimitThreshold,
withReverseSoftLimitThreshold, MIN_POSITION_MULTI, MAX_POSITION_MULTI, and
MOTOR_TO_MECHANISM consistent so the reverse bound is a distinct (negative)
limit.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Line 135: The current manual branch in Climb.java applies distanceToRadians to
a voltage tunable
(io.runVolts(distanceToRadians(ClimbState.MANUAL.getDistance().getAsDouble()) *
input)), producing wrong units; change the manual path to use the voltage
DoubleSupplier (or a lambda voltage tunable) instead of the distance-to-radians
conversion and pass that voltage into io.runVolts multiplied by the operator
input (e.g. use ClimbState.MANUAL.getVoltage().getAsDouble() * input or the
appropriate voltage supplier), removing the distanceToRadians call so motor
commands are in volts.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Line 69: The LoggedTunableNumber motionJerk in ClimbIOTalonFX is using the
turret namespace; change its key from "Turret/MotionJerk" to the climb namespace
(e.g., "Climb/MotionJerk") so climb-specific tuning is isolated; locate the
field motionJerk in class ClimbIOTalonFX and update the string literal to the
climb path.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 214-216: The conversion is double-applying the climb gearing
because Constants.Climb.FEEDBACK_CONFIGS already sets
RotorToSensorRatio/SensorToMechanismRatio on climbTalon, so values from
climbTalon.getClosedLoopReference() and getClosedLoopReferenceSlope() are
already in mechanism units; remove the extra motor-to-mechanism scaling by not
calling motorPositionToRads/motorVelocityToRads (or motorToMechanism) on those
closed-loop reads and instead assign them directly (or update the helpers to
accept mechanism-scaled inputs). Update occurrences around
motorPositionToRads(climbTalon.getClosedLoopReferenceSlope().getValueAsDouble()),
inputs.motionMagicPositionTarget =
motorPositionToRads(climbTalon.getClosedLoopReference().getValueAsDouble()), and
the similar blocks at 308-316 and 322-327 so closed-loop reference values are
treated as already-scaled by FEEDBACK_CONFIGS.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 274-276: The call to LoggedTunableNumber.ifChanged currently
passes the literal id 0 which can cross-couple change tracking; replace the
literal with an instance-specific id (e.g., a private final int instanceId
initialized once per ClimbIOTalonFX instance using a unique counter or
System.identityHashCode(this) or a per-instance TunableNumber id) and use that
instanceId in the LoggedTunableNumber.ifChanged invocation so each
ClimbIOTalonFX has a unique change-tracking key.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/team5924/frc2026/Constants.java (1)
596-605:⚠️ Potential issue | 🔴 CriticalClosed-loop gains are not visible in the applied climb motor config.
Climb.javacommands position control (io.setPosition(...)), but this config has noSlot0Configs. IfClimbIOTalonFXalso doesn’t apply Slot0 elsewhere, position loop gains remain zero and setpoints won’t actuate correctly. This was previously flagged and still appears unresolved in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 596 - 605, The TalonFXConfiguration named CONFIG is missing closed-loop Slot0 gains used by Climb (which calls io.setPosition(...)); add a Slot0Configs to CONFIG with the appropriate PIDF/closed-loop gains (e.g., P/I/D/F, integral zone, and max integral accumulator) so Slot0 is populated, or ensure ClimbIOTalonFX applies equivalent Slot0 settings when it initializes the Talon; update the TalonFXConfiguration by chaining .withSlot0(new Slot0Configs().with... ) (or move those settings into ClimbIOTalonFX) so the position controller has non-zero gains.src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (1)
56-59:⚠️ Potential issue | 🟡 Minor
TargetRadstelemetry is incorrect in MANUAL mode (unit mix).Line 97 converts
goalState.distanceto radians for all states, butMANUALstores voltage in that supplier. That makesClimb/TargetRadsmisleading whenever manual control is active.Also applies to: 95-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java` around lines 56 - 59, The telemetry calculation always converts goalState.distance to radians even for MANUAL, which stores a voltage in the distance supplier; update the code that computes and publishes Climb/TargetRads to special-case the MANUAL enum value (the MANUAL constant and its LoggedTunableNumber "Climb/OperatorVoltage") so that when current state == MANUAL you publish the raw voltage (or a separate telemetry key) instead of converting goalState.distance to radians; locate the conversion logic that references goalState.distance and TargetRads and add a branch: if mode == MANUAL use goalState.distance directly (or publish to "Climb/OperatorVoltage") else perform the radians conversion as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 596-605: Constants.Climb.CONFIG currently sets supply and stator
limits but doesn't enable them; update the CurrentLimitsConfigs construction
inside Constants.Climb.CONFIG to call withSupplyCurrentLimitEnable(true) and
withStatorCurrentLimitEnable(true) on the same CurrentLimitsConfigs instance so
the TalonFX current limits are actually active (keep the existing
TalonFXConfiguration, CurrentLimitsConfigs, and MotorOutputConfigs usage).
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 116-124: The code sets RobotState to the goal immediately in the
default branch so MOVING/isAtSetpoint() is bypassed; update the goal-setting
flow so transitions to a new position go through the MOVING state and only flip
to the goal state when isAtSetpoint() returns true: modify setGoalState(...) (or
the code that currently assigns RobotState directly) to set
RobotState.getInstance().setClimbState(ClimbState.MOVING) and store the desired
goal in a separate variable (goalState) instead of replacing the RobotState
immediately, and ensure handleCurrentState()'s default branch only commands
io.setPosition(distanceToRadians(goalState.distance.getAsDouble())) without
changing RobotState; when isAtSetpoint() becomes true, call
RobotState.getInstance().setClimbState(goalState) as originally intended.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 596-605: The TalonFXConfiguration named CONFIG is missing
closed-loop Slot0 gains used by Climb (which calls io.setPosition(...)); add a
Slot0Configs to CONFIG with the appropriate PIDF/closed-loop gains (e.g.,
P/I/D/F, integral zone, and max integral accumulator) so Slot0 is populated, or
ensure ClimbIOTalonFX applies equivalent Slot0 settings when it initializes the
Talon; update the TalonFXConfiguration by chaining .withSlot0(new
Slot0Configs().with... ) (or move those settings into ClimbIOTalonFX) so the
position controller has non-zero gains.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 56-59: The telemetry calculation always converts
goalState.distance to radians even for MANUAL, which stores a voltage in the
distance supplier; update the code that computes and publishes Climb/TargetRads
to special-case the MANUAL enum value (the MANUAL constant and its
LoggedTunableNumber "Climb/OperatorVoltage") so that when current state ==
MANUAL you publish the raw voltage (or a separate telemetry key) instead of
converting goalState.distance to radians; locate the conversion logic that
references goalState.distance and TargetRads and add a branch: if mode == MANUAL
use goalState.distance directly (or publish to "Climb/OperatorVoltage") else
perform the radians conversion as before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
308-311:⚠️ Potential issue | 🔴 CriticalReconcile position command conversions with configured feedback ratios.
Line 323 and Line 327 manually apply
motorToMechanism, while Line 129 already appliesConstants.Climb.FEEDBACK_CONFIGS(ratio-based scaling). This can reintroduce ratio double-scaling and drive the wrong setpoints.Suggested fix
private double radsToMotorPosition(double rads) { - return Units.radiansToRotations(rads * motorToMechanism); + return Units.radiansToRotations(rads); } private double motorPositionToRads(double motorPosition) { - return Units.rotationsToRadians(motorPosition / motorToMechanism); + return Units.rotationsToRadians(motorPosition); }In CTRE Phoenix 6, when FeedbackConfigs uses FusedCANcoder with RotorToSensorRatio and SensorToMechanismRatio, what units do TalonFX.getPosition(), TalonFX.getClosedLoopReference(), and MotionMagicVoltage.withPosition() use?Also applies to: 322-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 308 - 311, setPosition is double-scaling the feedback ratio: Constants.Climb.FEEDBACK_CONFIGS already applies RotorToSensorRatio/SensorToMechanismRatio, but setPosition calls radsToMotorPosition and motionMagicVoltage.withPosition which then re-applies motor->mechanism scaling; remove the duplicate scaling by ensuring the value passed to climbTalon.setControl(motionMagicVoltage.withPosition(...)) is in the Talon/feedback sensor native units expected by MotionMagicVoltage.withPosition — update radsToMotorPosition or setPosition to convert mechanism radians to the Talon-native position once (accounting for FEEDBACK_CONFIGS) and stop applying motorToMechanism twice, referencing setPosition, radsToMotorPosition, motionMagicVoltage.withPosition, climbTalon.setControl, and Constants.Climb.FEEDBACK_CONFIGS to locate the changes.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java (1)
213-227: Use the refreshed slope signal consistently for acceleration math.Line 220 uses
closedLoopReferenceSlopetimestamp, but Line 214 reads a fresh slope value from a separate getter path. Use the same refreshed signal object for both value and timestamp to avoid sampling skew.Suggested fix
- inputs.motionMagicVelocityTarget = - motorPositionToRads(climbTalon.getClosedLoopReferenceSlope().getValueAsDouble()); + inputs.motionMagicVelocityTarget = + motorPositionToRads(closedLoopReferenceSlope.getValueAsDouble());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java` around lines 213 - 227, The acceleration math reads the fresh slope value from climbTalon.getClosedLoopReferenceSlope() but uses a different stale signal (closedLoopReferenceSlope) for the timestamp, causing sampling skew; fix by capturing the refreshed slope signal once (e.g., assign climbTalon.getClosedLoopReferenceSlope() to a local variable), then use that local signal's getValueAsDouble() for inputs.motionMagicVelocityTarget and getTimestamp().getTime() for currentTime, and then update prevClosedLoopReferenceSlope and prevReferenceSlopeTimestamp from those same captured values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 139-150: The setGoalState switch incorrectly funnels OFF into the
MOVING/default path so the subsystem keeps calling io.setPosition instead of
stopping; add an explicit case for ClimbState.OFF in setGoalState (in class
Climb) that sets RobotState.getInstance().setClimbState(ClimbState.OFF), invokes
io.stop() (or the existing stop behavior), and returns, so OFF is handled
immediately rather than falling through to the MOVING/default branch that issues
io.setPosition calls.
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Line 144: In ClimbIOTalonFX replace the incorrect telemetry namespace used in
Logger.recordOutput by changing the literal "Turret/InitConfReport" to the
climb-specific namespace (e.g. "Climb/InitConfReport") so the call
Logger.recordOutput("Climb/InitConfReport", statusArray) correctly reflects the
subsystem; update the Logger.recordOutput invocation in the ClimbIOTalonFX class
where statusArray is logged.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 308-311: setPosition is double-scaling the feedback ratio:
Constants.Climb.FEEDBACK_CONFIGS already applies
RotorToSensorRatio/SensorToMechanismRatio, but setPosition calls
radsToMotorPosition and motionMagicVoltage.withPosition which then re-applies
motor->mechanism scaling; remove the duplicate scaling by ensuring the value
passed to climbTalon.setControl(motionMagicVoltage.withPosition(...)) is in the
Talon/feedback sensor native units expected by MotionMagicVoltage.withPosition —
update radsToMotorPosition or setPosition to convert mechanism radians to the
Talon-native position once (accounting for FEEDBACK_CONFIGS) and stop applying
motorToMechanism twice, referencing setPosition, radsToMotorPosition,
motionMagicVoltage.withPosition, climbTalon.setControl, and
Constants.Climb.FEEDBACK_CONFIGS to locate the changes.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java`:
- Around line 213-227: The acceleration math reads the fresh slope value from
climbTalon.getClosedLoopReferenceSlope() but uses a different stale signal
(closedLoopReferenceSlope) for the timestamp, causing sampling skew; fix by
capturing the refreshed slope signal once (e.g., assign
climbTalon.getClosedLoopReferenceSlope() to a local variable), then use that
local signal's getValueAsDouble() for inputs.motionMagicVelocityTarget and
getTimestamp().getTime() for currentTime, and then update
prevClosedLoopReferenceSlope and prevReferenceSlopeTimestamp from those same
captured values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java (2)
93-95: Minor:TargetRadslogging is misleading for MANUAL state.When
goalStateisMANUAL,distanceToRadians(goalState.distance.getAsDouble())converts the voltage value (4.5V) as if it were a distance, logging a meaningless number. Consider guarding this log or documenting that it's only valid for position-based states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java` around lines 93 - 95, The log call for "Climb/TargetRads" converts goalState.distance (via distanceToRadians) even when goalState == MANUAL, producing meaningless output; update the code around Logger.recordOutput("Climb/TargetRads", ...) to only log target radians when the climb is in a position-based state (i.e., not MANUAL) and when goalState.distance is a valid position value (use the goalState enum check and/or Optional presence on goalState.distance) so MANUAL mode does not convert a voltage into radians.
43-61: Consider renaming the enum field for clarity.The
distancefield is semantically correct for position states but misleading forMANUALwhich stores a voltage value. Consider renaming to something more generic likevalueor adding a separatevoltagefield forMANUAL, or add a comment clarifying the dual purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java` around lines 43 - 61, The ClimbState enum's field named distance is misleading because MANUAL holds a voltage; rename the field and getter from distance to value (e.g., change `@Getter` private final DoubleSupplier distance to `@Getter` private final DoubleSupplier value) and update the constructor parameter and all references to use value, and adjust the comment above MANUAL to note it represents operator voltage; alternatively, if you prefer to keep semantics distinct, add a second DoubleSupplier voltage field used only by MANUAL and keep distance for positional states—ensure constructor(s), `@Getter`(s), and all usages of ClimbState.distance are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/climb/Climb.java`:
- Around line 93-95: The log call for "Climb/TargetRads" converts
goalState.distance (via distanceToRadians) even when goalState == MANUAL,
producing meaningless output; update the code around
Logger.recordOutput("Climb/TargetRads", ...) to only log target radians when the
climb is in a position-based state (i.e., not MANUAL) and when
goalState.distance is a valid position value (use the goalState enum check
and/or Optional presence on goalState.distance) so MANUAL mode does not convert
a voltage into radians.
- Around line 43-61: The ClimbState enum's field named distance is misleading
because MANUAL holds a voltage; rename the field and getter from distance to
value (e.g., change `@Getter` private final DoubleSupplier distance to `@Getter`
private final DoubleSupplier value) and update the constructor parameter and all
references to use value, and adjust the comment above MANUAL to note it
represents operator voltage; alternatively, if you prefer to keep semantics
distinct, add a second DoubleSupplier voltage field used only by MANUAL and keep
distance for positional states—ensure constructor(s), `@Getter`(s), and all usages
of ClimbState.distance are updated accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/subsystems/climb/Climb.javasrc/main/java/org/team5924/frc2026/subsystems/climb/ClimbIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHoodIOTalonFX.java
💤 Files with no reviewable changes (1)
- src/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHoodIOTalonFX.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.