Dev #29
Dev #29
Conversation
WalkthroughRetune and behavioral updates across many subsystems: arm/elevator recalibration and state-machine rewrite, intake/end‑effector distance-sensor migration and defaults, transporter command/pulse refactor, servo stop support, swerve predicted-pose PID change, camera/pose-estimator renames, new operator reset triggers, and multiple command-flow reorganizations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EE as EndEffector
participant GC as GeneralCommands
Note over EE,GC: EndEffector default command selection
EE->>EE: hasGamePiece()
EE->>GC: getContinuousConditionalCommand(HOLD_CORAL, REST, EE::hasGamePiece)
GC-->>EE: runs HOLD_CORAL when true, else REST
sequenceDiagram
autonumber
participant CC as CoralCollectionCommands
participant IN as Intake
participant TR as Transporter
participant AE as ArmElevator
CC->>IN: getIntakeCoralCommand() → set Intake COLLECT (parallel)
CC->>TR: set Transporter COLLECT (parallel)
Note over IN,TR: run until IN.hasCoral() || TR.hasCoral()
alt has coral detected
IN-->>CC: hasCoral() or TR-->>CC: hasCoral()
CC->>CC: schedule getCollectionConfirmationCommand()
CC->>AE: chain load and REST_AFTER_LOADING, hold EndEffector
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-18T22:46:49.269ZApplied to files:
🧬 Code graph analysis (1)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java (1)
157-170: Servo configuration disabled without explanation.All servo PWM boundary configuration has been commented out. This aligns with the servo control being disabled in Climber.java. If this is temporary, add a TODO comment. If permanent, remove the dead code and the unused servo PWM constants at lines 53-58.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
20-20: Consider making SHOULD_CALIBRATE_ELEVATOR configurable.The calibration flag is hardcoded as
false. For SysID workflows or debugging, consider exposing this through a dashboard toggle or constants file so it can be changed without recompiling.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (3)
63-67: Document tuning for reduced real-robot motion limits.Real robot velocity decreased from simulation values (2.4614 -> 2.5 rps, relatively close) but acceleration dramatically reduced (67.2344 -> 7 rps², ~90% reduction). Document whether this is:
- A safety measure due to mechanical constraints
- Based on empirical tuning to prevent oscillation
- A temporary conservative limit
This massive reduction suggests the arm may have stability issues at higher accelerations on real hardware.
111-111: Document SysID voltage reduction.
ARM_SYSID_CONFIGvoltage reduced from 3V to 1.5V. This is a 50% reduction in characterization voltage. Document whether this change is due to:
- Safety concerns during characterization
- Previous voltage causing excessive acceleration
- Improved motor characterization at lower voltages
117-117: Document SysID voltage reduction.
ELEVATOR_SYSID_CONFIGvoltage reduced from 3V to 2V. Ensure this provides sufficient data for accurate characterization while remaining safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/frc/trigon/robot/RobotContainer.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(11 hunks)src/main/java/frc/trigon/robot/subsystems/climber/Climber.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(8 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(20-119)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(27-372)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(35-118)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
🔇 Additional comments (28)
src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
30-30: Clarify hardcoded logging and improve TODO
- Extensive logging is now always enabled on real hardware (previously sim-only), so
updateMechanism()will run each periodic cycle—this may impact performance.- The comment “AHAHAHAHAHHA” is unclear; replace it with a descriptive note or remove it.
- Confirm if using
truehere is intentional for debugging. If not, revert toRobotHardwareStats.isSimulation():- private static final LoggedNetworkBoolean ENABLE_EXTENSIVE_LOGGING = new LoggedNetworkBoolean("/SmartDashboard/EnableExtensiveLogging", true);//TODO: AHAHAHAHAHHA + private static final LoggedNetworkBoolean ENABLE_EXTENSIVE_LOGGING = new LoggedNetworkBoolean("/SmartDashboard/EnableExtensiveLogging", RobotHardwareStats.isSimulation());src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (3)
69-70: Verify angle bounds are logically correct.MINIMUM_ANGLE is 0° and MAXIMUM_ANGLE is -127°. This is unusual since "maximum" is numerically less than "minimum". Confirm this matches the physical mechanism's rotation direction and that downstream code correctly handles this inverted range.
226-227: State voltage polarities reversed.COLLECT voltage changed from 5 to -8 and EJECT voltage changed from -5 to 5. Verify these changes align with the mechanical direction of the intake mechanism and that all command code expecting these states has been updated accordingly.
36-38: No DIO channel conflicts detected.No other
DigitalInput(3|4|5)instantiations found outside IntakeConstants.java.src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (2)
3-6: LGTM: Explicit imports improve clarity.Replacing the wildcard import with explicit imports makes dependencies clear and avoids namespace pollution.
24-27: LGTM: Fluent API usage is cleaner.Calling
repeatedly()directly on theSequentialCommandGroupis more idiomatic than wrapping it in aRepeatCommand. The behavior is equivalent.src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (2)
105-108: Verify motor voltage polarity changes.Multiple transporter states have had their motor voltages' signs flipped:
- COLLECT: right motor 5 → -4
- ALIGN_CORAL: right motor 5 → -5
- HOLD_CORAL: right motor 1 → -1
- EJECT: both motors flipped
Confirm these changes align with the mechanical behavior and that command logic expecting these states has been updated.
24-24: No DIO channel conflict detected. Channel 6 is not referenced by any other sensor constant or DigitalInput.src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
47-47: LGTM: Keybinding updated to avoid conflict.Moving CORAL_COLLECTION_TRIGGER from
c()toz()avoids conflict with CLIMB_TRIGGER which usesc()at line 52.src/main/java/frc/trigon/robot/RobotContainer.java (1)
78-78: LGTM: Debugging trigger added.The debugging trigger provides a useful tool for testing ArmElevator positions while ignoring constraints.
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
21-21: No DIO channel 0 conflicts detected. Confirmed no other_CHANNELconstants are set to 0 across the Java codebase.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (9)
131-131: Verify correctness of subtractFrom180Degrees for reversed state.The logic now uses
subtractFrom180Degrees(targetState.targetAngle)when reversed. Given the new angle range (-90° to 270°) and the method's implementation (180° - angle), verify this produces the intended mechanical positions for all state angles, especially negative ones like REST (-90°) which would become 270°.Example calculations:
- REST (-90°) reversed: 180° - (-90°) = 270° ✓
- PREPARE_SCORE_L1 (20°) reversed: 180° - 20° = 160° ✓
- SCORE_L4 (10°) reversed: 180° - 10° = 170° ✓
However, confirm these angles are mechanically valid and safe for your robot's geometry.
150-151: Confirm ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET is correctly applied.The offset is added to the encoder reading. Given that
ANGLE_ENCODER_GRAVITY_OFFSETis set in the CANcoder configuration (line 321 in Constants) andARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETis computed asUnits.degreesToRotations(-23.56) - ANGLE_ENCODER_GRAVITY_OFFSET, verify this produces the correct absolute angle for your robot's zero position.Calculation chain:
- CANcoder applies
ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875asMagnetOffset- Reading is then adjusted by
ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = degreesToRotations(-23.56) - (-0.059326171875)- Net effect: reading is shifted by approximately
degreesToRotations(-23.56)from gravity-downConfirm this matches your intended zero position (horizontal? vertical?).
217-224: Verify safety angle calculation with new coordinate system.The calculation now uses
-90°as baseline (line 222) and addskCCW_90deg(line 223). With the new angle range (-90° to 270°), confirm this produces correct minimum safe angles across all elevator heights, especially at the extremes.Example: At maximum elevator height (1.382m):
heightFromSafeZone = 1.382 - 0.05 = 1.332mcosOfMinimumSafeAngle = 1.332 / 0.52 = 2.56(clamped to 1)acos(1) = 0- Result:
0 - 90° = -90°At minimum height (0m + 0.05m safety):
heightFromSafeZone = 0cosOfMinimumSafeAngle = 0acos(0) = 90°- Result:
90° - 90° = 0°This matches
MAXIMUM_ARM_SAFE_ANGLE = 0°, confirming the safety logic is consistent.
226-234: Verify elevator safety calculation with adjusted arm angle.Line 227 adds
kCCW_90degto the current arm angle before taking cosine. Confirm this correctly accounts for the new coordinate system where the arm's zero position has changed.With the new offsets:
- If arm is at physical horizontal (0° in new system), angle + 90° = 90°, cos(90°) = 0 ✓
- If arm is at -90° (pointing down), angle + 90° = 0°, cos(0°) = 1 ✓
- If arm is at 90° (pointing up), angle + 90° = 180°, cos(180°) = -1 (negative elevation contribution)
Verify this matches your intended safety logic for the elevator minimum height calculation.
240-242: Renamed method is correct.The change from
subtractFrom360DegreestosubtractFrom180Degreesaligns with the comment in previous learnings that angles above 90° would bypass safety logic, and now the coordinate system has been adjusted accordingly.
244-250: Verify visualization offset of PI/2.Line 247 adds
Math.PI / 2(90°) to the arm angle for visualization. Confirm this correctly orients the arm in the 3D visualization given the new coordinate system where -90° is the minimum angle.If the arm's zero position is now horizontal and you're adding 90° for visualization, the visual representation should point upward at zero. Verify this matches your robot's actual orientation.
80-82: Inconsistent offset application in mechanism update.Line 80 adds
ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETtogetCurrentArmAngle()(which already includes the offset per line 150), while line 81 adds it toCLOSED_LOOP_REFERENCE. This double-applies the offset to the current angle for visualization.Apply this diff to remove the duplicate offset:
ArmElevatorConstants.ARM_MECHANISM.update( - Rotation2d.fromRotations(getCurrentArmAngle().getRotations()), + getCurrentArmAngle(), Rotation2d.fromRotations(armMasterMotor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) + ArmElevatorConstants.ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET) );Likely an incorrect or invalid review comment.
64-70: Verify ROTOR_VELOCITY conversion
Confirm thatTalonFXSignal.ROTOR_VELOCITYreturns raw sensor units (ticks/100 ms) and isn’t already in rotor rotations/sec. If it is native ticks/100 ms, adjust your conversion to rotations/sec (e.g.,raw * 10 / (ticksPerRev * ELEVATOR_GEAR_RATIO)) before converting to meters/sec.
205-206: Remove offset warning: compensation is correctly applied. The code subtracts ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET (degreesToRotations(-23.56) – ANGLE_ENCODER_GRAVITY_OFFSET) from the setpoint and adds it back when reading CLOSED_LOOP_REFERENCE and POSITION, matching the TalonFX’s MagnetOffset configuration. No off-by-offset error present.Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (8)
34-34: Document the reason for limit sensor channel change.
REVERSE_LIMIT_SENSOR_CHANNELchanged from 0 to 1. Ensure this matches your robot's actual wiring to avoid limit switch failures that could damage the elevator mechanism.
85-86: Verify angle range change is intentional.Angle range changed from (0°, 360°) to (-90°, 270°). This shifts the coordinate system by -90°. Confirm:
- All state angles in
ArmElevatorStatehave been updated consistently- The simulation limits match the physical hard stops
- Safety logic in
isArmAboveSafeAngle()is updated (it compares toMAXIMUM_ARM_SAFE_ANGLE = 0°)Based on learnings, angles above 90° bypass safety logic, but now the coordinate system is shifted. With the new range:
- Old 0° (horizontal forward) is now -90°
- Old 90° (straight up) is now 0°
- Old 180° (horizontal backward) is now 90°
Verify this is the intended behavior.
166-166: MAXIMUM_ARM_SAFE_ANGLE change affects safety logic.Changed from 90° to 0°. Combined with the angle range shift (-90° to 270°), this moves the safety boundary from "straight up" to "horizontal" in the new coordinate system. Verify:
isArmAboveSafeAngle()at line 116-118 in ArmElevator.java now correctly prevents interference- Safety calculations in
calculateMinimumArmSafeAngle()andcalculateMinimumSafeElevatorHeightRotations()are consistent with this changeThe safety logic should prevent the arm from moving into the elevator's travel zone. With the new angle:
- Arm angles ≥ 0° (horizontal and above) are "safe" and bypass elevator height constraints
- Arm angles < 0° (below horizontal) trigger minimum elevator height requirements
Confirm this matches your robot's mechanical constraints.
198-204: Verify PID tuning after gear ratio change.Real-robot PID gains are set with the new 42:1 gear ratio (changed from 40:1). The gains may need retuning since the gear ratio affects the relationship between motor output and arm movement.
Current gains:
- kP = 70 (increased from unknown previous value)
- kS = 0.02
- kG = 0.39 (gravity compensation)
Test the arm's response to verify:
- No oscillation at setpoints
- Adequate settling time
- Proper gravity compensation at all angles
232-233: ROTOR_VELOCITY and ROTOR_POSITION signals added.These signals provide raw motor shaft data. Confirm these are used correctly in ArmElevator.java line 73 where rotor velocity is divided by gear ratio for SysID logging.
337-356: Extensive state enum changes - verify all angles.All
ArmElevatorStateenum values have been redefined with new target angles and elevator positions. Given the coordinate system changes (angle range shift, gear ratio change, etc.), verify each state:Critical states to test:
- REST states (-90° or 90°) - ensure arm doesn't collide with frame
- SCORE_L4 (10°, 1.2m) - at maximum elevator extension
- COLLECT_ALGAE_FLOOR (-40°, 0.2m) - arm below horizontal, low elevator
- SCORE_NET (70°, 1.382m) - maximum elevator height
For each state, confirm:
- Arm angle is achievable within (-90° to 270°) range
- Elevator height is within (0 to 1.382m) range
- No collisions between arm and elevator/frame
- Safety logic allows the configuration
Consider creating a visualization tool or running simulation tests for all states before deploying to hardware.
320-322: Align sensor config with SwerveModuleConstants and validate arm behaviorThese settings mirror those in SwerveModuleConstants (SensorDirection = CounterClockwise_Positive, AbsoluteSensorDiscontinuityPoint = 0.5). Ensure:
- All arm-elevator position calculations account for the new range (–0.25 to 0.75 rotations) and wrap at ±0.5
- Positive commands move the arm in the expected physical direction without unintended wrapping
Test by commanding small positive angle changes and observing actual arm movement.
50-52: Verify gear ratio change and update dependent calculations
- Confirm 42:1 matches your physical gearbox swap.
- Update all motion profiles and hard-coded arm setpoints to account for the new 5% difference.
- Adjust PID gains (Slot0 kP/kV/kA in ArmElevatorConstants.java lines 198–204) for the new ratio.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
Outdated
Show resolved
Hide resolved
| // config.Feedback.VelocityFilterTimeConstant = 0.2; | ||
|
|
||
| config.MotionMagic.MotionMagicCruiseVelocity = ARM_DEFAULT_MAXIMUM_VELOCITY; | ||
| config.MotionMagic.MotionMagicAcceleration = ARM_DEFAULT_MAXIMUM_ACCELERATION; | ||
| config.MotionMagic.MotionMagicJerk = config.MotionMagic.MotionMagicAcceleration * 10; | ||
|
|
||
| config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | ||
| config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); | ||
|
|
||
| // config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Commented-out velocity filter and limit switches.
Lines 209, 215-219 are commented out:
- Velocity filter (line 209)
- Forward/reverse software limit switches (lines 215-219)
Document why these are disabled. If this is temporary for testing, add TODO comments. If permanent, remove the dead code.
-// config.Feedback.VelocityFilterTimeConstant = 0.2;
-
config.MotionMagic.MotionMagicCruiseVelocity = ARM_DEFAULT_MAXIMUM_VELOCITY;
config.MotionMagic.MotionMagicAcceleration = ARM_DEFAULT_MAXIMUM_ACCELERATION;
config.MotionMagic.MotionMagicJerk = config.MotionMagic.MotionMagicAcceleration * 10;
-// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true;
-// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270);
-
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true;
-// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270);Without software limits, the arm could exceed mechanical bounds if commanded beyond the angle range. Consider re-enabling with the updated angle range:
config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true;
config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;
config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true;
config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
around lines 209 to 219 the velocity filter and forward/reverse software limit
switch configurations are commented out with no explanation; either re-enable
these safety controls or mark them explicitly as temporary. Restore the velocity
filter line (or add a TODO explaining why it’s disabled and when to re-enable)
and re-enable the Forward/Reverse software limits using the updated angle range
and ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET via
edu.wpi.first.math.util.Units.degreesToRotations(...) for thresholds; if you
truly intend to keep them disabled, remove the dead code or add clear TODOs with
justification and a ticket/owner for follow-up.
| // config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; | ||
| // | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; |
There was a problem hiding this comment.
Elevator software limits commented out.
Similar to the arm, elevator software limits are disabled. This is more concerning for the elevator as it could crash into physical stops, potentially damaging the mechanism or causing safety hazards.
Strongly recommend re-enabling elevator limits:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true;
-// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS;
-//
-// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true;
-// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8;
+ config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true;
+ config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS;
+
+ config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true;
+ config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS);If the limits are disabled for debugging, add a TODO comment and re-enable before competition.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; | |
| // config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; | |
| // | |
| // config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | |
| // config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; | |
| config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; | |
| config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; | |
| config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | |
| config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS); |
| // rightServo.setTargetSpeed(power); | ||
| // leftServo.setTargetSpeed(-power); |
There was a problem hiding this comment.
Servo control disabled without explanation.
The servo power commands have been commented out, effectively disabling servo control in the climber subsystem. If this is temporary for testing or due to missing hardware, add a TODO comment explaining when/why this will be re-enabled. If permanent, consider removing the dead code.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/climber/Climber.java around lines
104 to 105 the servo control calls have been commented out, leaving dead code
and no explanation; either re-enable the calls if they were temporarily disabled
for testing or hardware reasons, or remove them if they’re permanently unused —
if keeping them commented, add a concise TODO comment on the same lines
explaining why they’re disabled, when they should be re-enabled, and who is
responsible (e.g., "TODO: disabled for bench testing until servo hardware
arrives; re-enable when hardware ID XYZ installed"), so future readers know the
intent.
src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java
Outdated
Show resolved
Hide resolved
| // config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = MINIMUM_ANGLE.getRotations(); | ||
| // | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; | ||
| // config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = MAXIMUM_ANGLE.getRotations(); |
There was a problem hiding this comment.
Soft limits disabled without explanation.
The forward and reverse soft limits have been commented out, removing a safety boundary that prevents the intake from exceeding mechanical limits. Verify this is intentional and that the mechanism has alternative protection (e.g., hard limit switches).
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java around
lines 191-195 the forward and reverse software soft limits are commented out,
removing a safety boundary; either re-enable them by setting
config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true with threshold =
MINIMUM_ANGLE.getRotations() and
config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true with threshold =
MAXIMUM_ANGLE.getRotations(), or if this was intentional, add a clear comment
above these lines explaining why they are disabled and ensure and document
alternative protections (e.g., configured hardware limit switches, motor current
limiting, and/or mechanical stops) and where those protections are implemented
so reviewers know the safety is preserved.
| ANGLE_MOTOR.registerSignal(TalonFXSignal.ROTOR_VELOCITY, 100); | ||
| ANGLE_MOTOR.registerSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE, 100); | ||
|
|
||
| OperatorConstants.DEBUGGING_TRIGGER.onTrue(new InstantCommand(() -> ANGLE_MOTOR.setPosition(0)).ignoringDisable(true)); |
There was a problem hiding this comment.
Debug trigger directly resets motor position.
This trigger allows resetting the angle motor position to 0 at runtime via DEBUGGING_TRIGGER. If triggered accidentally during operation, it could cause the control system to believe the intake is at a different position than reality, leading to unsafe movements.
Consider adding safeguards: require the robot to be disabled, add a confirmation step, or log a warning when this is triggered.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java around
line 210, the DEBUGGING_TRIGGER currently calls ANGLE_MOTOR.setPosition(0)
immediately, which is unsafe; change the handler so it only performs the reset
when the robot is disabled (e.g., check DriverStation.isDisabled() or
Robot.isDisabled() first), add a simple confirmation requirement (for example
require a separate confirmation flag/button or a long-press) and emit a
warning/log entry when the reset is executed or when an attempt is blocked;
implement this by replacing the raw InstantCommand with a conditional command
that checks the disabled state and confirmation, logs the action with context,
and otherwise no-ops and logs the blocked attempt.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
72-79: Restore the SWERVE (and other critical) default commands.With these lines commented out the drivetrain never receives a default drive command, so the robot will sit idle (and other subsystems stay in their last state). That’s a blocking regression for teleop safety.
-// SWERVE.setDefaultCommand(GeneralCommands.getFieldRelativeDriveCommand()); -// ARM_ELEVATOR.setDefaultCommand(ArmElevatorCommands.getDefaultCommand()); -// CLIMBER.setDefaultCommand(ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.REST)); -// END_EFFECTOR.setDefaultCommand(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.REST)); + SWERVE.setDefaultCommand(GeneralCommands.getFieldRelativeDriveCommand()); + ARM_ELEVATOR.setDefaultCommand(ArmElevatorCommands.getDefaultCommand()); + CLIMBER.setDefaultCommand(ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.REST)); + END_EFFECTOR.setDefaultCommand(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.REST)); @@ -// configureSysIDBindings(ARM_ELEVATOR); -// OperatorConstants.DEBUGGING_TRIGGER.whileTrue(ArmElevatorCommands.getDebuggingCommand(true)); +// configureSysIDBindings(ARM_ELEVATOR); +// OperatorConstants.DEBUGGING_TRIGGER.whileTrue(ArmElevatorCommands.getDebuggingCommand(true));
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
103-121: Prefer structured telemetry over rawSystem.out.
System.out.printlnruns on the main loop thread and will flood the console when states toggle, hurting timing and making logs noisy. Use the AdvantageKit logger you already depend on so the data is timestamped and filterable.- System.out.println("Setting intake state to " + targetState.name()); + Logger.recordOutput("Intake/TargetState", targetState.name()); @@ - System.out.println("Setting intake voltage to " + voltage); + Logger.recordOutput("Intake/TargetVoltageVolts", voltage); @@ - System.out.println("Setting intake angle to " + targetAngle.getDegrees() + " degrees"); + Logger.recordOutput("Intake/TargetAngleDegrees", targetAngle.getDegrees());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(27-372)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(36-119)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (7)
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
IntakeAssistCommand(22-190)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(36-119)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)
src/main/java/frc/trigon/robot/RobotContainer.java (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(19-50)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(17-39)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(15-65)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)
| if (SHOULD_CALIBRATE_ELEVATOR) { | ||
| log.motor("Elevator") | ||
| .linearPosition(Units.Meters.of(getElevatorPositionRotations())) | ||
| .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) | ||
| .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | ||
| return; |
There was a problem hiding this comment.
Fix SysId elevator logging units.
linearPosition/linearVelocity must be in meters, but you’re passing raw rotations (and rotor RPS). That will produce bogus SysId fits for the elevator.
- log.motor("Elevator")
- .linearPosition(Units.Meters.of(getElevatorPositionRotations()))
- .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO))
+ final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations());
+ final double elevatorVelocityMetersPerSecond = rotationsToMeters(
+ elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO
+ );
+ log.motor("Elevator")
+ .linearPosition(Units.Meters.of(elevatorPositionMeters))
+ .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (SHOULD_CALIBRATE_ELEVATOR) { | |
| log.motor("Elevator") | |
| .linearPosition(Units.Meters.of(getElevatorPositionRotations())) | |
| .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) | |
| .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | |
| return; | |
| if (SHOULD_CALIBRATE_ELEVATOR) { | |
| final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); | |
| final double elevatorVelocityMetersPerSecond = rotationsToMeters( | |
| elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) | |
| / ArmElevatorConstants.ELEVATOR_GEAR_RATIO | |
| ); | |
| log.motor("Elevator") | |
| - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) | |
| - .linearVelocity(Units.MetersPerSecond.of( | |
| - elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) | |
| - / ArmElevatorConstants.ELEVATOR_GEAR_RATIO | |
| .linearPosition(Units.Meters.of(elevatorPositionMeters)) | |
| .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) | |
| .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java around
lines 66 to 71, the SysId logging is passing raw rotations and rotor velocity
into linearPosition/linearVelocity (which expect meters and m/s). Convert
rotations to linear meters and rotor velocity to meters/second before calling
Units.Meters.of and Units.MetersPerSecond.of: compute positionMeters =
(rotations / ArmElevatorConstants.ELEVATOR_GEAR_RATIO) * (2 * Math.PI *
ArmElevatorConstants.DRUM_RADIUS) (or use a spool circumference constant) and
velocityMps = (rotorVelocityRPS / ArmElevatorConstants.ELEVATOR_GEAR_RATIO) * (2
* Math.PI * ArmElevatorConstants.DRUM_RADIUS); then pass those values to the
logging calls and keep the voltage log as-is.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
209-209: Commented-out velocity filter.The velocity filter is disabled without explanation. If this is temporary for testing, add a TODO comment. If permanent, remove the dead code.
Either restore the velocity filter:
-// config.Feedback.VelocityFilterTimeConstant = 0.2; + config.Feedback.VelocityFilterTimeConstant = 0.2;Or remove it entirely if no longer needed:
-// config.Feedback.VelocityFilterTimeConstant = 0.2; -Or document why it's disabled:
-// config.Feedback.VelocityFilterTimeConstant = 0.2; + // TODO: Re-enable velocity filter after tuning (disabled for SysID characterization) + // config.Feedback.VelocityFilterTimeConstant = 0.2;
57-58: Document the encoder offset values.
ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875(approximately -21.36° when converted) and theARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETcalculation involve magic numbers. Add comments explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSETwas measured (encoder reading at gravity-down position?)- Why
-23.56°is subtracted inARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET- The intended zero position of the arm (horizontal? vertical?)
Apply this diff to add documentation:
+ // Encoder reading when arm is pointing straight down (gravity-down position) private static final double ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875; + // Offset to make arm zero position horizontal (measured offset from gravity-down to horizontal) + private static final double ARM_HORIZONTAL_OFFSET_DEGREES = -23.56; + // Combined offset for arm position (simulation uses ideal zero, real robot applies physical offset) - static final double ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = RobotHardwareStats.isSimulation() ? 0 : edu.wpi.first.math.util.Units.degreesToRotations(-23.56) - ANGLE_ENCODER_GRAVITY_OFFSET; + static final double ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = RobotHardwareStats.isSimulation() + ? 0 + : edu.wpi.first.math.util.Units.degreesToRotations(ARM_HORIZONTAL_OFFSET_DEGREES) - ANGLE_ENCODER_GRAVITY_OFFSET;
215-219: Arm software limit switches commented out.Software limits for the arm are disabled. Without these limits, the arm could exceed mechanical bounds if commanded beyond the angle range.
Consider re-enabling with the updated angle range:
-// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); - -// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270); + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET; + + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;If these are disabled for debugging, add a TODO comment and re-enable before competition.
275-279: Elevator software limit switches commented out.Similar to the arm, elevator software limits are disabled. This is more concerning for the elevator as it could crash into physical stops, potentially damaging the mechanism or causing safety hazards.
Strongly recommend re-enabling elevator limits:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS);If the limits are disabled for debugging, add a TODO comment and re-enable before competition.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
68-74: Fix SysId elevator logging units.
linearPosition/linearVelocitymust be in meters, but you're passing raw rotations (and rotor RPS). That will produce bogus SysId fits for the elevator.Apply this diff to convert units properly:
if (SHOULD_CALIBRATE_ELEVATOR) { + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); return; }src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
191-195: Soft limits remain disabled (previously flagged).The software soft limits are still commented out, as noted in the previous review. This removes a safety boundary that prevents the intake from exceeding mechanical limits.
If this is intentional (e.g., relying on hardware limit switches or mechanical hard stops), please document the rationale and alternative protections in a comment above these lines. Otherwise, re-enable the soft limits.
210-210: Debug trigger still lacks safeguards (previously flagged).The debug trigger that resets the angle motor position to 0 is now active but still lacks safeguards, as noted in the previous review. While
ignoringDisable(true)allows this to work when the robot is disabled, accidental triggering during operation could cause the control system to believe the intake is at a different position than reality, leading to unsafe movements.Consider the recommendations from the previous review: add a confirmation step, require the robot to be disabled (check via
DriverStation.isDisabled()), or at minimum log a warning when this is triggered.
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
22-22: Hardcoded calibration flag should be configurable.
SHOULD_CALIBRATE_ELEVATOR = trueis a hardcoded constant that determines which subsystem to calibrate. This requires a code change and redeployment to switch between calibrating the elevator and the arm.Consider making this configurable via:
- A dashboard boolean input (SmartDashboard/Shuffleboard)
- A command-line parameter
- A configuration file
Example using SmartDashboard:
- private static final boolean SHOULD_CALIBRATE_ELEVATOR = true; + private final boolean shouldCalibrateElevator; + + public ArmElevator() { + setName("ArmElevator"); + shouldCalibrateElevator = SmartDashboard.getBoolean("ArmElevator/CalibrateElevator", true); + }Then update all references from
SHOULD_CALIBRATE_ELEVATORtoshouldCalibrateElevator.src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
33-38: Consider simplifying the redundant end condition.The command uses both
.until(RobotContainer.END_EFFECTOR::hasGamePiece)and.unless(RobotContainer.END_EFFECTOR::hasGamePiece). This is redundant—both prevent execution when the end effector already has a game piece. Theunlesscheck prevents the command from starting, whileuntilwould end it immediately.Since the outcome is functionally identical, you can simplify to just the
unlessguard:public static Command getLoadCoralCommand() { return new ParallelCommandGroup( ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.LOAD_CORAL), EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.LOAD_CORAL) - ).until(RobotContainer.END_EFFECTOR::hasGamePiece).unless(RobotContainer.END_EFFECTOR::hasGamePiece); + ).unless(RobotContainer.END_EFFECTOR::hasGamePiece); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/frc/trigon/robot/RobotContainer.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(11 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(7 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/RobotContainer.java
- src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (4)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(27-372)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(36-164)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(36-164)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-45)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(32-238)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(21-120)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-99)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
🔇 Additional comments (26)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (3)
64-65: Verify pulse timing adjustments through testing.The pulse timing constants have been reduced:
- Applied time: 0.1s → 0.08s (20% reduction)
- Wait time: 0.05s → 0.03s (40% reduction)
These changes suggest performance tuning, but ensure that the shorter durations still provide reliable coral manipulation without mechanical issues.
25-25: Confirm beam break sensor wiring and documentation
No in-code references to DIO channel 3 remain. Manually verify that the beam break sensor is physically wired to channel 6 and update any external documentation or diagrams accordingly.
106-109: Verify transporter motor direction changes for coral-handling states.
- COLLECT: right motor voltage changed 5→ –4 (reversed & reduced)
- HOLD_CORAL: right motor changed 1→ –1 (reversed)
- EJECT: right motor changed –5→ 5 (reversed)
Confirm these inversions match the intended mechanical behavior and have been validated with physical testing.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (5)
337-356: Extensive ArmElevatorState enum rewrite.All state target angles and positions have been changed. This is a major behavioral change affecting the entire robot.
The new state definitions appear consistent with the updated angle range [-90°, 270°] and the new safe angle threshold (0°). However, verify:
- All target positions have been tested on the physical robot
- The
prepareStatereferences are correct (e.g., SCORE_L2 → PREPARE_SCORE_L2)- The
ignoreConstraintsflags are set appropriately for states that need to bypass safety checks- The
speedScalarvalues provide acceptable performance for each stateNotable changes:
- REST is now at -90° (was previously a different angle)
- Multiple new states added: PREPARE_SCORE_L1-L4, REST variants, COLLECT_ALGAE variants
- Elevator positions range from 0.2m to 1.382m
Consider adding integration tests to verify all state transitions work as expected.
34-34: No leftover channel 0 references—only ArmElevatorConstants.java:34 defines REVERSE_LIMIT_SENSOR_CHANNEL = 1. Ensure your physical wiring matches this update.
52-53: Verify new gear ratio impacts
- Confirm the physical gear ratio is 4:1
- Recompute each
ArmElevatorStateelevatorPositionRotationsfor ratio = 4- Test the elevator to ensure it reaches the intended heights
320-322: Confirm encoder direction and wrap settings
Ensure the new SensorDirectionValue.CounterClockwise_Positive matches the physical mounting, AbsoluteSensorDiscontinuityPoint = 0.5 aligns with your intended [–90°, 270°] range, and all angle calculations handle wrapping at ±180°. Test key positions (–90°, 0°, 180°, 270°) to verify smooth transitions.
85-86: All arm angle targets and safety calculations conform to the new [-90°, 270°] range
- Every ArmElevatorState targetAngle lies within –90°…270°
- calculateMinimumArmSafeAngle() returns between –90° and 0° as expected
- Simulation is bounded by ARM_MINIMUM_ANGLE = –90° and ARM_MAXIMUM_ANGLE = 270°
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (10)
55-57: LGTM: Brake now includes follower motors.Good addition. Applying brake to both master and follower motors ensures consistent behavior and prevents any potential drift from the followers.
76-78: Arm SysId logging now uses rotor velocity.The arm logging now uses
ROTOR_VELOCITYdirectly and divides byARM_GEAR_RATIO. This provides the angular velocity of the arm mechanism, which is correct for SysId characterization.
109-113: LGTM: Conditional SysId drive based on calibration flag.The
sysIDDrivemethod now correctly routes voltage to either the elevator or arm motor based onSHOULD_CALIBRATE_ELEVATOR. This ensures only one mechanism is being characterized at a time.
136-137: Updated to use subtractFrom180Degrees for reversed states.This change aligns with the encoder's new discontinuity point at 0.5 rotations (180°) instead of 1.0 rotation (360°).
The logic is consistent with:
- Line 245-247: The renamed
subtractFrom180Degreesmethod- Line 322 in ArmElevatorConstants.java:
AbsoluteSensorDiscontinuityPoint = 0.5
155-156: getCurrentArmAngle now includes position offset.The arm angle calculation now adds
ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETto the encoder reading. This is consistent with the encoder configuration changes in ArmElevatorConstants.java (lines 57-58, 321).Verify that:
- The offset is applied in the correct direction (addition, not subtraction)
- Reported angles match the physical arm position
- The offset behaves correctly in simulation (where it's set to 0)
Test the arm at known physical positions (e.g., horizontal, vertical, gravity-down) and confirm the reported angles are correct.
210-211: setTargetArmAngle subtracts offset before commanding motor.The method calculates the target position, then subtracts
ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETbefore sending it to the motor. This is the inverse ofgetCurrentArmAngle()(line 155), which adds the offset.This ensures:
getCurrentArmAngle()returns the arm angle in the robot's coordinate frame- The motor is commanded in the encoder's raw coordinate frame
- The offset is consistently applied/removed at the boundary
227-228: calculateMinimumArmSafeAngle updated for new angle range.The method now returns -90° when NaN (instead of 0°) and uses
kCCW_90degin the calculation. This aligns with the new angle range of [-90°, 270°] defined in ArmElevatorConstants.java (lines 85-86).The NaN case occurs when
acos()is undefined (e.g., when the elevator is fully retracted), and returning -90° is appropriate as it's the minimum angle in the new range.
232-233: calculateMinimumSafeElevatorHeightRotations adds 90° offset.The elevator safety calculation now adds
Rotation2d.kCCW_90degto the current arm angle before taking the cosine. This accounts for the new coordinate system where 0° is horizontal instead of vertical.Verify the safety logic:
- When the arm is at -90° (REST), the elevator should have minimal constraints
- When the arm is at 0° (horizontal), the elevator constraint should be at the safe zone threshold
- When the arm is above 0°, the
isArmAboveSafeAngle()check should disable elevator constraintsTest the elevator at various arm angles to ensure it doesn't hit the frame or base.
245-247: subtractFrom360Degrees renamed to subtractFrom180Degrees.The method name and implementation now reflect the encoder's new discontinuity point at 180° (0.5 rotations) instead of 360° (1.0 rotation).
This change is consistent with:
- Line 322 in ArmElevatorConstants.java:
AbsoluteSensorDiscontinuityPoint = 0.5- The new angle range of [-90°, 270°] which spans 360° but is centered differently
252-253: calculateVisualizationPose adds 90° offset to arm rotation.The visualization now adds
Math.PI / 2(90°) to the arm angle. This adjusts the 3D visualization to account for the new coordinate system where 0° is horizontal.This ensures the 3D visualization in Advantage Scope or other tools displays the arm in the correct orientation relative to the robot.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
20-31: LGTM! Improved collection flow with driver assist.The refactored collection command now has a clearer structure:
- Intake coral until the transporter detects it
- Provide collection confirmation feedback
- Schedule load command if not holding algae
The addition of
IntakeAssistCommandimproves driver feedback during collection. Theunlessguard correctly prevents the command from running when coral is already collected.
41-46: Verify transporter continuous vs. pulsed behavior for coral intake.
TransporterCommands#getSetTargetStateWithPulsesCommandis defined but not used elsewhere;getIntakeCoralCommandnow applies continuous voltage. Confirm that continuous operation still reliably collects coral—if pulsed cycles are required to prevent jams or improve grip, switch back togetSetTargetStateWithPulsesCommandhere or remove the unused pulses method.src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (6)
36-38: Verify hardware wiring matches updated channels.The sensor channels have been updated from 0/1/2 to 3/4/5. Ensure that the physical wiring on the robot matches these new channel assignments to prevent sensor malfunctions.
69-70: Verify angle limits match the physical mechanism.The intake angle limits have been significantly modified:
MINIMUM_ANGLE: -12° → 0°MAXIMUM_ANGLE: 110° → -127°This represents both a sign inversion and a range change. Ensure these new limits correctly reflect the physical mechanism's range of motion and that the angle measurement direction is consistent with the motor configuration and simulation.
Test the intake at both limits to confirm:
- The angle readings match expected positions
- The mechanism doesn't hit hard stops unexpectedly
- The simulation behavior matches reality
92-93: More aggressive SysID configuration.The SysID voltages have been increased (ramp: 0.2 → 1.2 V/s, step: 0.6 → 1.5 V), which will provide better characterization data. This is appropriate for a Kraken-powered mechanism but ensure the physical mechanism can safely handle these voltages during characterization.
154-154: Improved motor tuning and API update.The feedback ratio field has been updated to match the current CTRE Phoenix 6 API (
SensorToMechanismRatio). The PID gains have been retuned:
- Added derivative gain (kD = 0.5) on the real robot for better damping
- Increased cruise velocity (2 → 3 rps) for faster intake movements
These changes should improve control response and performance.
Also applies to: 174-182, 187-187
216-216: Limit sensor auto-reset and distance sensor simulation disabled.The automatic position reset on limit sensor triggers (line 216) and the distance sensor simulation supplier (line 220) have been disabled. This suggests a shift away from automatic calibration.
Ensure there's a manual calibration workflow in place and that simulation testing accounts for the lack of distance sensor feedback.
Also applies to: 220-220
227-228: IntakeState voltages updated to match mechanism changes.The
COLLECTandEJECTstates have been updated:
COLLECT: voltage 5 → -8 (reversed and increased magnitude)EJECT: voltage -5 → 5 (reversed)This aligns with the angle limit sign changes and suggests either motor inversion or a mechanism redesign. Verify that collection and ejection work as expected with these new voltage values.
| * The highest point of the arms angular zone where the safety logic applies. | ||
| */ | ||
| static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(90); | ||
| static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(0); |
There was a problem hiding this comment.
🧩 Analysis chain
MAXIMUM_ARM_SAFE_ANGLE changed from 90° to 0°.
This moves the safe angle threshold significantly lower, which will affect when safety constraints are applied. The isArmAboveSafeAngle() check now triggers at 0° instead of 90°.
This change impacts safety logic throughout the codebase. Verify:
- The new threshold aligns with the physical constraints of the robot
- States like REST (-90°), LOAD_CORAL (-90°), and EJECT (-30°) are intentionally below the safe angle
- The elevator safety calculations in
calculateMinimumSafeElevatorHeightRotations()(ArmElevator.java:231-239) work correctly with the new threshold
Run the following script to find all usages of this constant:
🏁 Script executed:
#!/bin/bash
# Find all references to MAXIMUM_ARM_SAFE_ANGLE
rg -n -C3 'MAXIMUM_ARM_SAFE_ANGLE|isArmAboveSafeAngle' --type=javaLength of output: 2821
Arm safe angle threshold set to 0° disables dynamic elevator‐height safety for most arm positions
MAXIMUM_ARM_SAFE_ANGLE was lowered from 90° to 0° (ArmElevatorConstants.java:166), so isArmAboveSafeAngle() now returns true for any angle ≥ 0°. As a result, calculateMinimumSafeElevatorHeightRotations() (ArmElevator.java:231–239) applies only the static minimum elevator safe zone for nearly all forward arm angles, bypassing the intended dynamic height calculation. Revert MAXIMUM_ARM_SAFE_ANGLE to 90° or confirm this change won’t compromise safety.
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
around line 166, MAXIMUM_ARM_SAFE_ANGLE was changed to 0° which causes
isArmAboveSafeAngle() to be true for virtually all arm positions and disables
the dynamic elevator-height safety; restore the original safe threshold by
setting MAXIMUM_ARM_SAFE_ANGLE back to 90° (Rotation2d.fromDegrees(90)) or, if
there was an intentional behavior change, add a safety check and tests proving
no safety regression before keeping 0°.
|
|
||
| void setTargetState(IntakeConstants.IntakeState targetState) { | ||
| this.targetState = targetState; | ||
| System.out.println("Setting intake state to " + targetState.name()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace System.out.println with structured logging.
The three System.out.println calls added for debugging (lines 112, 123, 129) should be replaced with proper structured logging using the existing Logger.recordOutput infrastructure already in use throughout this file. System.out is blocking I/O, cannot be filtered by log level, and isn't captured by your telemetry system.
Apply this diff to replace the debug prints:
void setTargetState(IntakeConstants.IntakeState targetState) {
this.targetState = targetState;
- System.out.println("Setting intake state to " + targetState.name());
+ Logger.recordOutput("Intake/TargetState", targetState.name());
setTargetState(targetState.targetAngle, targetState.targetVoltage);
}
private void setTargetVoltage(double voltage) {
IntakeConstants.INTAKE_MECHANISM.setTargetVelocity(voltage);
- System.out.println("Setting intake voltage to " + voltage);
+ Logger.recordOutput("Intake/TargetVoltage", voltage);
intakeMotor.setControl(voltageRequest.withOutput(voltage));
}
private void setTargetAngle(Rotation2d targetAngle) {
- System.out.println("Setting intake angle to " + targetAngle.getDegrees() + " degrees");
+ Logger.recordOutput("Intake/TargetAngleDegrees", targetAngle.getDegrees());
angleMotor.setControl(positionRequest.withPosition(targetAngle.getRotations()));
}Also applies to: 123-123, 129-129
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/Intake.java around lines
112, 123, and 129, replace the three System.out.println debug prints with
structured logging via the existing Logger.recordOutput calls used elsewhere in
this class; specifically, remove System.out usage and call Logger.recordOutput
with a clear key (e.g., "intake/state" or similar), include the message "Setting
intake state to <state>" (or the corresponding messages at the other two sites)
substituting targetState.name() (or the relevant variable) as a formatted
argument, and ensure the messages follow the file's established logging pattern
so they are non-blocking, filterable by level, and captured by telemetry.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
57-58: Document the encoder offset values.These offset constants use unexplained magic numbers. This concern was previously raised and remains unresolved.
Based on past review comments.
166-166: Arm safe angle threshold set to 0° disables dynamic elevator‐height safety.This change was previously flagged as a critical issue. Setting
MAXIMUM_ARM_SAFE_ANGLEto 0° causesisArmAboveSafeAngle()to return true for most arm positions, bypassing dynamic elevator-height safety calculations.Based on past review comments.
209-219: Commented-out velocity filter and limit switches.These safety features remain disabled. This concern was previously raised and remains unresolved.
Based on past review comments.
275-279: Elevator software limits commented out.These safety limits remain disabled. This concern was previously raised as a major issue and remains unresolved.
Based on past review comments.
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
37-37: Consider simplifying the redundant guard.Both
.until()and.unless()use the same condition (RobotContainer.END_EFFECTOR::hasGamePiece). While safe, the.unless()guard alone would prevent unnecessary execution. The.until()guard is useful if the game piece arrives mid-execution, but the duplication is redundant for the common case.If you prefer to remove the redundancy:
- ).until(RobotContainer.END_EFFECTOR::hasGamePiece).unless(RobotContainer.END_EFFECTOR::hasGamePiece); + ).unless(RobotContainer.END_EFFECTOR::hasGamePiece);However, keeping both guards is safe and defensive, so this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/RobotContainer.java(0 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AutonomousCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(11 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/frc/trigon/robot/RobotContainer.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AutonomousCommands.java (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(19-62)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(45-171)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-45)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(32-238)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(21-120)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
🔇 Additional comments (12)
src/main/java/frc/trigon/robot/commands/commandfactories/AutonomousCommands.java (1)
93-93: LGTM!The switch from
getIntakeSequenceCommand()togetIntakeCoralCommand()simplifies the intake flow. The new public command is correctly integrated here.src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
22-23: LGTM!The simplified intake flow using
getIntakeCoralCommand().until(RobotContainer.TRANSPORTER::hasCoral)followed by confirmation is clear and direct.
47-51: LGTM!Exposing
getIntakeCoralCommand()as public is correct and enables reuse in autonomous commands. The parallel intake and transporter activation logic is sound.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (9)
34-34: Verify the sensor channel change.The reverse limit sensor channel was changed from 0 to 1. Ensure this matches the physical wiring of the sensor on the robot.
52-52: Verify the elevator gear ratio change.The gear ratio changed from 3 to 4, which will affect all position and velocity conversions for the elevator. Ensure this matches the physical gearbox configuration and that all dependent calculations remain correct.
63-67: Approve motion parameter updates; verify tuning.The arm default velocity and acceleration were updated from 0 to actual values (2.5 and 7), which fixes a critical issue that would have prevented arm motion. The elevator acceleration was reduced from 80 to 50, which may improve stability but could reduce responsiveness.
Verify that these tuning parameters meet performance requirements during physical testing.
85-86: LGTM! Arm angle range adjusted to support new states.The angle range was shifted from [0°, 360°] to [-90°, 270°] to accommodate new states like REST (-90°) and EJECT (-30°). The total range remains 360°.
198-204: Verify updated PID gains.The arm PID gains were updated significantly (kP to 70, kG to 0.39 for the real robot). Verify these gains provide stable, responsive control during physical testing.
232-233: LGTM! Enhanced telemetry.Additional rotor velocity and position signals improve observability for debugging and tuning.
264-270: Verify updated elevator PID gains.The elevator PID gains were updated (kP to 12, kD to 1, and feedforward terms adjusted). Verify these gains provide stable elevator control during physical testing, especially given the gear ratio change.
320-322: Verify encoder direction and discontinuity point changes.The encoder direction was changed to
CounterClockwise_Positiveand the discontinuity point was changed from 1.0 to 0.5. The discontinuity point of 0.5 means the encoder wraps around at 180° instead of 360°, which may cause unexpected behavior if the arm crosses this boundary.Verify:
- The direction change matches the physical encoder mounting
- The arm never crosses the 180° boundary during normal operation, or handle the wraparound correctly
336-374: Verify the new state machine definitions.The entire
ArmElevatorStateenum was replaced with a new set of states. This is a major change that affects all arm-elevator behaviors.Key concerns to verify:
- Physical safety: Ensure all angle/position combinations are mechanically safe and don't cause collisions
- Prepare states: Verify that states with prepare states (e.g.,
SCORE_L2→PREPARE_SCORE_L2) sequence correctly- Safety bypass: States with
ignoreConstraints = true(e.g.,LOAD_CORAL,COLLECT_ALGAE_FLOOR) bypass safety checks. Confirm this is intentional and document why- Coordinate validity: Several states have extreme angles (e.g.,
SCORE_NETat 70°,COLLECT_ALGAE_LOLLIPOPat 70°). Verify these are achievable and safe- Speed scalars: The
speedScalarvalues range from 0.3 to 1. Verify these provide appropriate motion speeds for each operationConsider adding unit tests that verify state transitions and safety constraints.
Co-Authored-By: Nummun14 <142012009+Nummun14@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
191-195: Soft limits remain disabled.This concern was previously flagged. The forward and reverse software soft limits are still commented out, removing safety boundaries. Please address the previous review comment regarding this issue.
210-210: Debug trigger safety concern remains unaddressed and worsened.This concern was previously flagged. The debug trigger still allows runtime position reset, and now includes
.ignoringDisable(true), which permits the reset even when the robot is disabled. This increases the risk of unsafe position resets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/deploy/pathplanner/settings.json(1 hunks)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/deploy/pathplanner/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/intake/Intake.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-99)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
🔇 Additional comments (6)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (6)
69-70: Verify angle coordinate system change is mechanically safe.The angle bounds have been fundamentally redesigned: MINIMUM_ANGLE went from -12° to -127° and MAXIMUM_ANGLE from 110° to 0°. This appears to flip the coordinate system and significantly extends the range.
With soft limits disabled (lines 191-195), ensure:
- The new range is mechanically safe and won't cause collisions
- Hardware limit switches are properly configured and tested
- The IntakeState enum values (lines 224-228) correspond to correct physical positions
92-93: LGTM: More aggressive SysID parameters.The characterization parameters are now more aggressive (6× ramp rate, 2.5× step voltage), which should provide better system identification data. Ensure characterization is performed in a controlled environment.
176-183: LGTM: Updated control gains.The PID and feedforward gains have been revised with separate simulation and real-world values. The cruise velocity calculation in simulation now uses a kV-based formula. These tuning changes should improve control performance.
Also applies to: 187-187
224-228: LGTM: IntakeState enum updated consistently.The state definitions have been updated to use the new angle coordinate system. The voltage and angle pairings appear logically consistent:
- COLLECT: negative voltage at minimum angle (extended)
- EJECT: positive voltage at maximum angle (retracted)
- REST states: zero voltage at appropriate angles
This aligns with the angle bound changes at lines 69-70.
36-38: Verify intake sensor wiringSensor channels were remapped from 0/1/2 to 3/4/5; ensure the robot’s physical DIO wiring matches these new assignments to prevent sensor misreads.
154-154: SensorToMechanismRatio usage is correct: SensorToMechanismRatio expects sensor rotations per mechanism output; INTAKE_MOTOR_GEAR_RATIO (4) and ANGLE_MOTOR_GEAR_RATIO (40) are appropriate values.
| configureIntakeMotor(); | ||
| configureAngleMotor(); | ||
| configureLimitSensor(REVERSE_LIMIT_SENSOR, REVERSE_LIMIT_SENSOR_SIMULATION_SUPPLIER, REVERSE_LIMIT_SENSOR_BOOLEAN_EVENT, MINIMUM_ANGLE); | ||
| configureLimitSensor(REVERSE_LIMIT_SENSOR, REVERSE_LIMIT_SENSOR_SIMULATION_SUPPLIER, REVERSE_LIMIT_SENSOR_BOOLEAN_EVENT, MAXIMUM_ANGLE); |
There was a problem hiding this comment.
Limit switch auto-zeroing disabled.
The automatic position reset when limit switches are triggered has been commented out (line 216). Combined with disabled soft limits (lines 191-195), this removes multiple layers of position safety:
- No automatic re-zeroing when hitting physical limits
- No software boundaries to prevent overtravel
- Manual reset via debug trigger (line 210) is now the only way to correct position drift
This increases the risk of accumulating position errors and the intake mechanism believing it's at an incorrect angle.
Consider re-enabling at least one safety layer: either restore soft limits with the new angle range or re-enable automatic position resets at limit switches.
Also applies to: 216-216
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java around
lines 140 and referencing the surrounding lines 191-195, 210, and 216, the
limit-switch auto-zero and soft limits have been disabled which removes position
safety; restore at least one safety layer by either re-enabling soft limits with
the correct angle range or re-enabling the automatic position reset in the limit
switch handler: if choosing soft limits, call the motor controller soft limit
configuration with MINIMUM_ANGLE and MAXIMUM_ANGLE and enable the soft limits
where they were commented out (lines ~191-195); if choosing auto-zero, uncomment
or re-add the limit switch event handler at line ~216 so that when the
reverse/forward limit BooleanEvent fires it sets the encoder position to the
known physical angle (use setSelectedSensorPosition or the equivalent API with
the correct offset), and keep the debug manual reset (line ~210) as a fallback;
ensure the simulation supplier and BooleanEvent used in configureLimitSensor at
line 140 are still wired to the handler and add brief comments documenting which
safety layer is active.
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java
Outdated
Show resolved
Hide resolved
| private static final Trigger DISABLED_TRIGGER = new Trigger(DriverStation::isDisabled); | ||
| private static final Executor BRAKE_MODE_EXECUTOR = Executors.newFixedThreadPool(8); | ||
| private static final LoggedNetworkBoolean ENABLE_EXTENSIVE_LOGGING = new LoggedNetworkBoolean("/SmartDashboard/EnableExtensiveLogging", RobotHardwareStats.isSimulation()); | ||
| private static final LoggedNetworkBoolean ENABLE_EXTENSIVE_LOGGING = new LoggedNetworkBoolean("/SmartDashboard/EnableExtensiveLogging", true);//TODO: AHAHAHAHAHHA |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify this is intentional and track for reversion.
Changing the default from RobotHardwareStats.isSimulation() to true enables extensive logging unconditionally on real hardware, which increases resource consumption (CPU, network bandwidth, storage). The TODO comment suggests this is temporary.
If this is for debugging, consider reverting before competition or making it configurable via NetworkTables at runtime.
Based on learnings
Revert or disable extensive logging default before deployment.
The default for ENABLE_EXTENSIVE_LOGGING was changed to true, enabling costly logging on real hardware (CPU, network, storage). Since it’s already exposed via SmartDashboard, disable it at runtime or revert the default to false before competition.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java around line 30,
the LoggedNetworkBoolean ENABLE_EXTENSIVE_LOGGING is defaulted to true which
enables heavy logging on real hardware; change its default back to false (or
programmatically force it off at startup) so extensive logging is disabled by
default while leaving the SmartDashboard key available for debugging. Update the
constructor or static initializer to use false as the default value (or call
ENABLE_EXTENSIVE_LOGGING.set(false) during robot init) and add a brief comment
reminding to enable it only for debugging.
Co-Authored-By: Nummun14 <142012009+Nummun14@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (18)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
110-110: Replace System.out.println with structured logging.These debug print statements should use
Logger.recordOutputinstead ofSystem.out.printlnfor consistency with the existing logging infrastructure in this file.Also applies to: 121-121, 127-127
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
168-172: Soft limits disabled without explanation.The forward and reverse soft limits have been commented out, removing a critical safety boundary. Combined with the removal of limit switch sensors and auto-zeroing, this creates significant position safety risks.
191-191: Distance sensor simulation disabled.The simulation supplier has been commented out, preventing coral collection detection from working in simulation.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (3)
67-72: Fix SysId elevator logging units.
linearPosition/linearVelocitymust be in meters and meters/second, but you're passing raw rotations and rotor RPS. This will produce invalid SysId characterization data for the elevator.Apply this diff to convert to proper units:
- log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) - .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); + log.motor("Elevator") + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) + .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE)));
67-72: Fix SysId elevator logging units.The elevator logging still passes raw rotations/rotor velocity to
Units.Meters.of()andUnits.MetersPerSecond.of(), which expect meters and m/s. This will produce bogus SysId fits.Convert to meters before logging:
if (SHOULD_CALIBRATE_ELEVATOR) { + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); return; }
67-73: Fix SysId elevator logging units.The elevator's
linearPositionandlinearVelocityare still logging raw rotations and rotor RPS instead of converting to meters and meters/second. This will produce incorrect SysId characterization. This issue was previously flagged.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (12)
48-49: Document the encoder offset values.
ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875and theARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETcalculation involve unexplained magic numbers. Add inline comments explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSETwas measured (encoder reading at what physical position?)- Why
-23.56°is used in the offset calculation- What the arm's zero/reference position is (horizontal? vertical?)
Apply this diff:
- private static final double ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875; - static final double ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = RobotHardwareStats.isSimulation() ? 0 : edu.wpi.first.math.util.Units.degreesToRotations(-23.56) - ANGLE_ENCODER_GRAVITY_OFFSET; + // Encoder reading when arm is pointing straight down (gravity-down position, measured in rotations) + private static final double ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875; + // Offset to align arm zero position: -23.56° converts from gravity-down to desired zero reference + // Simulation uses ideal zero (no physical offset needed) + static final double ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = RobotHardwareStats.isSimulation() + ? 0 + : edu.wpi.first.math.util.Units.degreesToRotations(-23.56) - ANGLE_ENCODER_GRAVITY_OFFSET;
152-152: Arm safe angle threshold set to 0° disables dynamic elevator height safety.
MAXIMUM_ARM_SAFE_ANGLEchanged from 90° to 0°, soisArmAboveSafeAngle()returns true for virtually all forward arm positions. This causescalculateMinimumSafeElevatorHeightRotations()(ArmElevator.java:231-239) to bypass the dynamic height calculation and apply only the static minimum for most arm angles.Revert to 90° or confirm this change is intentional and won't compromise safety:
- static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(0); + static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(90);Based on learnings: angles above 90° bypass the safety logic per previous team feedback, but lowering the threshold to 0° extends the bypass zone significantly.
192-202: Commented-out velocity filter and soft limits pose safety risks.The velocity filter (line 192) and forward/reverse software limit switches (lines 198-202) are disabled. Without software limits, the arm could exceed mechanical bounds if commanded beyond the valid angle range.
Either:
- Re-enable with updated thresholds accounting for the new offset:
config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET; config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;
- Remove the dead code if permanently disabled, or add TODO comments explaining temporary disablement and tracking re-enablement.
258-262: Elevator software limits commented out—safety hazard.Elevator soft limits are disabled, allowing the elevator to potentially crash into physical stops and damage the mechanism or cause safety issues.
Strongly recommend re-enabling with appropriate thresholds:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = 0; // Minimum position + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS);If disabled for debugging, add a TODO comment and re-enable before deployment.
48-49: Document encoder offset values.The offset constants remain undocumented magic numbers. Add comments explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSETwas measured- What
-23.56°represents inARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET- The arm's intended zero position
As per coding guidelines from previous review.
152-152: Arm safe angle threshold disables elevator safety logic.
MAXIMUM_ARM_SAFE_ANGLE = 0°meansisArmAboveSafeAngle()returns true for nearly all arm positions, bypassing dynamic elevator height calculations incalculateMinimumSafeElevatorHeightRotations(). This could allow unsafe elevator/arm combinations.Restore the safe threshold:
- static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(0); + static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(90);Or provide analysis proving 0° is safe for all intended operations.
192-202: Re-enable arm software limit switches.Commented-out soft limits leave the arm vulnerable to exceeding mechanical bounds. Re-enable with the updated angle range or document why they're disabled.
-// config.Feedback.VelocityFilterTimeConstant = 0.2; - -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); - -// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270); + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET; + + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;
258-262: Re-enable elevator software limit switches.Disabled elevator limits risk crashing into hard stops, potentially damaging the mechanism. Re-enable before competition.
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = 0; + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS) / ELEVATOR_GEAR_RATIO;
48-49: Document the encoder offset values.The magic numbers
ANGLE_ENCODER_GRAVITY_OFFSET = -0.059326171875and-23.56°inARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETneed inline comments explaining:
- How the gravity offset was measured
- What the -23.56° offset represents physically
- The arm's intended zero position
This issue was previously flagged.
152-152: Arm safe angle threshold disables dynamic elevator safety.
MAXIMUM_ARM_SAFE_ANGLEchanged from 90° to 0°. As previously flagged, this causesisArmAboveSafeAngle()to return true for virtually all forward arm positions, which bypasses the dynamic elevator-height safety calculation incalculateMinimumSafeElevatorHeightRotations(). This is a critical safety regression. Revert to 90° or provide justification and additional safety verification.Based on learnings: The team's design has angles above 90° bypassing safety logic, but changing the threshold to 0° extends this bypass to nearly all positions.
192-202: Commented-out velocity filter and limit switches.The velocity filter (line 192) and forward/reverse software limit switches (lines 198-202) remain commented out. Without these safety controls, the arm could exceed mechanical bounds. Either re-enable with proper thresholds accounting for
ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET, or document why they're disabled and when they'll be restored. This issue was previously flagged.
258-262: Elevator software limits disabled.Elevator software limits remain commented out. This is particularly dangerous for the elevator, as it could crash into physical stops and cause mechanism damage or safety hazards. Strongly recommend re-enabling these limits before any testing. This critical issue was previously flagged.
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (2)
5-5: Remove unused import of SwerveSetpoint inSwerve.java:5.
4-5: Remove unused SwerveSetpoint import
The importcom.pathplanner.lib.util.swerve.SwerveSetpointisn’t referenced in Swerve.java; remove it.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (3)
21-21: Document the calibration flag and consider a more robust approach.The
SHOULD_CALIBRATE_ELEVATORflag manually switches between arm and elevator calibration. This approach risks deployment errors if the flag isn't toggled correctly before running SysId.Consider one of these approaches:
- Add a clear inline comment explaining this is a manual toggle for SysId runs
- Use a dashboard-selectable option to choose the calibration target at runtime
- Create separate SysId command factories for arm vs. elevator
If keeping the flag, add:
- private static final boolean SHOULD_CALIBRATE_ELEVATOR = true; + /** + * Manual toggle: set to true for elevator SysId, false for arm SysId. + * IMPORTANT: Verify this matches your intended calibration target before running SysId. + */ + private static final boolean SHOULD_CALIBRATE_ELEVATOR = true;
21-21: Make calibration flag configurable or remove after testing.Hardcoding
SHOULD_CALIBRATE_ELEVATOR = truemeans the arm won't be calibrated when this code is deployed. Consider making this a dashboard-configurable boolean or removing it entirely after calibration is complete.Apply this diff to make it configurable:
- private static final boolean SHOULD_CALIBRATE_ELEVATOR = true; + private static final boolean SHOULD_CALIBRATE_ELEVATOR = SmartDashboard.getBoolean("ArmElevator/CalibrateElevator", false);Or add a TODO if this is temporary:
- private static final boolean SHOULD_CALIBRATE_ELEVATOR = true; + // TODO: Remove after elevator calibration is complete + private static final boolean SHOULD_CALIBRATE_ELEVATOR = true;
21-21: Make calibration target configurable.The
SHOULD_CALIBRATE_ELEVATORflag is hardcoded. Consider making this a dashboard-configurable value or a command-line argument so you can switch calibration targets without recompiling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(7 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
👮 Files not reviewed due to content moderation or server errors (3)
- src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java
- src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
- src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (9)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (2)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-108)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-170)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-108)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(22-354)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-170)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-99)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
src/main/java/lib/commands/WheelRadiusCharacterizationCommand.java (1)
WheelRadiusCharacterizationCommand(25-197)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(11-36)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)
🔇 Additional comments (95)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
21-21: Verify disabling autonomous scoring is intentional.
SHOULD_SCORE_AUTONOMOUSLYhas been changed fromtruetofalse, which disables autonomous scoring behavior by default. Ensure this aligns with current robot requirements and competition strategy.
47-47: Simplified manual scoring preparation.The manual scoring path now uses
getPrepareArmElevatorIfWontHitReefinstead ofgetAutonomouslyPrepareScoreCommand, removing autonomous driving logic from the manual workflow. This simplifies the operator-controlled scoring sequence by focusing solely on arm/elevator preparation without robot movement.src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
34-34: LGTM!The new distance sensor logging provides useful telemetry for monitoring game piece detection. The scaled value uses the calibration constants defined in
EndEffectorConstants.src/main/java/frc/trigon/robot/RobotContainer.java (1)
87-87: Improved end effector default behavior.The end effector now uses a dynamic default command that automatically switches between
HOLD_CORAL(when holding a game piece) andREST(when empty). This provides better default behavior compared to the previous staticRESTstate.src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
29-35: Smart default command implementation.The new
getDefaultCommand()provides intelligent default behavior by holding coral when detected and resting when empty. This prevents accidental game piece loss while maintaining safe idle behavior.src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (5)
21-21: Verify distance sensor wiring matches new channel.The distance sensor channel changed from 4 to 0. Ensure the physical wiring on the robot matches this new channel assignment to prevent detection failures.
26-26: Verify DutyCycle sensor hardware is installed.The sensor type changed from
createDigitalSensortocreateDutyCycleSensor, which requires different hardware. Confirm that a DutyCycle-compatible distance sensor (e.g., analog sensor with PWM output) is physically installed at channel 0.
49-52: Distance sensor calibration constants added.New scaling constants convert raw DutyCycle sensor readings to centimeters. The formula is:
distance_cm = (raw_value * 0.0002) - 200. Ensure these constants match your sensor's datasheet specifications.
69-70: Verify motor configuration changes align with mechanism behavior.Two critical motor configuration changes:
- NeutralMode: Changed from
BraketoCoast, allowing the end effector to backdrive when unpowered- Inverted: Changed from
Clockwise_PositivetoCounterClockwise_Positive, reversing the positive voltage directionVerify that:
- Coast mode is appropriate for your mechanism (won't drop game pieces when idle)
- The inversion change produces correct rotation direction for intake/ejection
99-99: HOLD_CORAL state provides gentle retention.The new
HOLD_CORAL(-0.5)state applies a low holding voltage to maintain coral grip without excessive motor current. This complements the new smart default command that switches between holding and resting.src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (6)
80-85: Verify the drive motor PID/FF tuning values.The drive motor control gains have changed significantly:
- kP reduced from 50 to 2 (25× reduction)
- kS reduced from 5.25 to 0.24025 (~22× reduction)
- kV added: 0.91963 (new feedforward term)
- kA added: 0.069151 (new feedforward term)
These changes suggest either:
- New characterization data from sysid
- Hardware modifications (gearing, wheel diameter, motor type)
- Correction of previously incorrect values
The addition of kV and kA feedforward terms is good practice for velocity control. However, ensure these values were obtained from proper sysid characterization and have been tested under load.
Consider verifying:
- These values match the latest sysid characterization results
- The robot drives smoothly with these gains under various loads
- The step voltage reduction to 2V (line 34) didn't compromise characterization quality
34-34: Verify sysid step voltage reduction
Sysid step voltage for DRIVE_MOTOR_SYSID_CONFIG dropped from 8 V to 2 V, lower than battery voltage (~12 V) and other subsystems’ configs (e.g., climber at 5 V). Ensure this change is intentional and still provides adequate signal-to-noise ratio for friction (kS) and acceleration (kA) identification.
32-36: LGTM! Lower SysID voltage improves safety.Reducing the step voltage from 8V to 2V during system identification improves safety and can yield more accurate characterization at lower speeds, reducing the risk of wheel slip.
80-85: LGTM! PID/FF gains updated from characterization.The updated gains reflect typical post-characterization tuning:
- kP reduced from 50 to 2 (less aggressive response)
- kS, kV, kA now populated with non-zero values from system identification
Simulation values correctly preserved with their separate tuning.
34-34: Verify the reduced SysId voltage is intentional.The SysId voltage has been reduced from 8V to 2V. This lower voltage is safer for initial characterization but may provide less accurate results due to lower signal-to-noise ratio and potential issues with overcoming static friction. Ensure this change was intentional and that the characterization at 2V provides sufficient data quality.
80-85: Confirm and document new drive motor control gainsNo evidence found of SysId characterization or tuning documentation for the updated kP, kS, kV, and kA values. Please:
- Attach or reference the SysId tuning report used to derive these gains
- Validate on the robot: ensure smooth drive behavior and accurate setpoint tracking under both normal and aggressive maneuvers
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (8)
31-34: LGTM: Encoder offsets calibrated.The encoder offsets have been updated from placeholder zeros to calibrated values. This is expected after physical alignment or initial hardware setup.
36-39: Verify wheel diameter impact
- Confirm the 0.076 m wheel diameter matches the actual hardware
- Validate odometry and velocity calculations using the new diameter
- Recalculate and update drive feedforward gains (kS=0.24025, kV=0.91963, kA=0.069151) in SwerveModuleConstants.java to reflect the diameter change
31-34: LGTM! Encoder offsets calibrated.The steer encoder offsets have been updated from placeholder zeros to measured calibration values. This is standard practice after aligning each module's encoder zero position with its mechanical zero.
102-104: LGTM! Gyro mount pose calibrated.The gyro mount pose configuration has been updated to reflect the actual physical mounting orientation of the IMU on the robot:
- Yaw offset of ~52° indicates the gyro is mounted at an angle relative to the robot frame
- Small pitch/roll corrections account for minor mounting imperfections
These corrections will improve odometry accuracy by properly transforming gyro readings into the robot's coordinate frame.
36-39: Verify downstream impacts of wheel diameter changeThe wheel diameter was reduced from 0.1 m to 0.076 m; ensure any max‐speed limits, odometry conversions, feedforward/feedback gains, or autonomous path parameters referencing wheel diameter have been updated or retuned.
31-34: LGTM! Steer encoder offsets calibrated.The steer encoder offsets have been updated from placeholder zeros to specific measured values. This is expected when physically calibrating the robot's swerve modules.
102-104: LGTM! Gyro mount pose configured.The gyro mount pose has been updated with specific orientation values. This calibration ensures the gyro readings are correctly transformed to match the robot's coordinate frame.
36-39: Validate wheel diameter reduction
The wheel diameter was changed from 100 mm to 76 mm (24% smaller), which directly affects your encoder-to-distance conversions and drive kinematics. Confirm:
- The 76 mm value was measured on the actual wheels
- Odometry/pose estimation has been recalibrated and validated with the new diameter
- Autonomous paths, speed limits, and distance-based routines have been tested for accuracy with this change
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (2)
51-55: Limit sensor updates removed consistently with constant changes.The removal of
FORWARD_LIMIT_SENSORandREVERSE_LIMIT_SENSORupdate calls is consistent with their removal fromIntakeConstants.java. However, this eliminates runtime feedback from physical limit switches, which compounds the safety concerns created by disabling soft limits in the constants file.Verify that the intake mechanism has alternative physical protections (hard stops, current limiting) since both software limits and limit switch feedback have been removed.
134-134: Visualization rotation direction corrected.Negating the Y-axis rotation adjusts the visualization to properly represent the intake's physical orientation.
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (6)
82-83: SysId parameters made more aggressive.The voltage ramp rate increased 6x (0.2→1.2 V/s) and maximum voltage increased 2.5x (0.6→1.5 V). This will produce faster characterization runs and potentially better data, but monitor the mechanism during SysId to ensure it handles the increased aggressiveness safely.
131-131: API naming updated to match Phoenix 6 conventions.The rename from
RotorToSensorRatiotoSensorToMechanismRatioaligns with updated Phoenix 6 API naming.
153-165: Control gains updated for new angle range.The PID, feedforward, and motion magic parameters have been retuned. Key changes:
- Real hardware: kP=30, kD=0.5, kS=0.61, kV=4, kG=0.3
- Motion magic cruise velocity reduced to 3 rot/s (more conservative)
These values appear reasonable but should be validated through testing across the full angle range.
Test the intake motion across the full angle range (-127° to 0°) to verify:
- Smooth motion without oscillation
- Adequate speed at cruise velocity
- Proper gravity compensation at all angles
187-187: Debug trigger removed - safety improvement.Commenting out the debug position reset trigger removes the previously flagged safety risk of accidentally resetting the motor position during operation.
34-34: Verify distance sensor wiring on DIO channel 5
Code now references channel 5 only; ensure the physical sensor is connected to DIO 5.
195-199: Verify all IntakeState usages with new voltage/angle mappings.
Confirm default commands and runtime checks now produce correct behavior in:
- RobotContainer.setDefaultCommand(IntakeConstants.IntakeState.REST)
- SimulationFieldHandler.atState checks for COLLECT and EJECT
- IntakeCommands.getSetTargetStateCommand in all CommandFactories (CoralCollection, CoralEjection, Climb, AlgaeManipulation)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (34)
48-49: LGTM!The conditional SysId configuration selection correctly delegates to the calibration flag.
52-57: LGTM!Good catch—follower motors now correctly match the brake state of their masters, ensuring consistent mechanical behavior across the entire mechanism.
82-85: LGTM!The mechanism visualization correctly applies the position offset to align the reference display with the actual arm position.
107-113: LGTM!The conditional voltage routing correctly implements the calibration flag behavior.
153-155: LGTM!Centralizing the offset application ensures all arm angle readings are consistently adjusted.
196-196: LGTM!Speed scaling is correctly applied before setting the target arm state.
209-212: LGTM!The offset is correctly removed when commanding the motor (since the encoder feedback includes the offset), maintaining consistency in the control loop.
219-219: LGTM!Correctly uses the dedicated
elevatorPositionRequestfor elevator positioning.
222-239: LGTM!The safety angle calculations correctly account for the new coordinate system (−90° to 270°). The ±90° adjustments in the trigonometric calculations align with the shifted reference frame.
249-255: LGTM!The visualization orientation correctly adds a 90° offset to align with the physical arm orientation in the new coordinate system.
257-267: LGTM!The speed scaling helper methods eliminate code duplication and correctly scale motion parameters proportionally while maintaining the jerk relationship (10× acceleration).
135-137: Verify reversed arm angle calculation.The code now uses
subtractFrom180Degrees(180°−angle) instead of mirroring around 180° (360°−angle). Confirm this new reversal axis matches your intended behavior for allisStateReversedstates.
52-57: LGTM!Correctly applying brake mode to follower motors ensures consistent behavior across the mechanism.
107-113: LGTM!The conditional SysId drive logic correctly routes voltage to the subsystem being calibrated.
153-155: LGTM!The offset correctly adjusts the encoder reading to match the intended arm coordinate system.
82-89: LGTM!Mechanism visualization correctly incorporates the arm position offset for both current and target positions.
249-255: LGTM!The 90-degree visualization offset aligns the 3D representation with the updated arm coordinate system.
222-229: LGTM!The updated safe angle calculation correctly handles the new coordinate system, returning -90° as a conservative lower bound when the elevator is too high for the arm to reach safely.
231-239: Verify elevator safety calculation with new angle system.Adding 90° to the arm angle before computing
getCos()fundamentally changes the elevator height constraint calculation. Ensure this produces correct minimum safe heights across the full arm range (-90° to 270°).Consider adding unit tests for edge cases:
- Arm at -90° (should allow low elevator positions)
- Arm at 0° (transition point for safety logic)
- Arm at 90° (vertical, critical for safety)
- Arm at 270° (maximum angle)
187-207: LGTM!Extracting speed scaling to helper methods improves code clarity and maintainability.
209-212: LGTM!The offset handling is correct: added when reading encoder position and subtracted when commanding motor position, ensuring consistent coordinate systems.
218-220: LGTM!Using separate motion magic requests for arm and elevator allows independent speed scaling and prevents interference between the two subsystems.
257-261: LGTM!Speed scaling helper maintains the correct relationship between velocity, acceleration, and jerk (10x).
263-267: LGTM!Elevator speed scaling correctly maintains motion profile relationships.
47-49: LGTM!SysId config selection correctly matches the calibration flag.
33-38: LGTM!Separate elevator motion magic request enables independent tuning and speed scaling.
132-137: Verify reversed angle math in atStateSubtraction from 180° (instead of 360°) changes reversed targets. Confirm for all calls with isStateReversed=true (SimulationFieldHandler.java:129–135; AutonomousCommands.java:133) that the resulting arm angles fall within the new –90° to 270° range and match the expected physical positions.
48-48: LGTM!The config selection logic correctly routes to the appropriate SysId configuration based on the calibration flag.
52-57: LGTM!Applying brake mode to both master and follower motors ensures consistent behavior across the motor groups.
107-113: LGTM!The voltage routing correctly targets the subsystem being calibrated.
153-155: LGTM!The arm position offset correctly applies the new gravity-based calibration reference.
209-212: LGTM!The offset is correctly subtracted when commanding the motor, maintaining consistency with the offset addition in
getCurrentArmAngle().
257-267: LGTM!The speed scaling helpers correctly apply the scalar to all motion parameters while maintaining the 10x jerk-to-acceleration ratio.
135-135: Confirm reversed angle calculation semanticsThe code now uses
subtractFrom180Degrees(180° − θ) instead of the previous 360° mirror, yielding very different results (e.g. 90°→90° vs. 90°→270°). Verify that mirroring around 180° matches your mechanical and control requirements.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (29)
44-44: Verify elevator PID gains were retuned for the new gear ratio.
ELEVATOR_GEAR_RATIOchanged from 3 to 4 (33% increase), which significantly affects the relationship between motor output and elevator motion. Confirm that the elevator PID gains (lines 247-253) were re-characterized for this new mechanical configuration.The velocity and acceleration limits may also need adjustment—verify through testing that motion profiles remain smooth and don't exceed mechanical limits.
54-58: LGTM—significant motion profile tuning applied.The updated velocity and acceleration limits represent major performance tuning:
- Arm real-robot acceleration increased from 0 to 7
- Elevator real-robot acceleration increased from 20 to 50
These changes should improve responsiveness but verify through testing that the mechanism remains mechanically sound under the new profiles.
76-77: LGTM—coordinate system updated to match physical constraints.The angle bounds shifted from [0°, 360°] to [−90°, 270°], representing the same 360° range with a new reference frame that better aligns with the physical mechanism orientation.
100-110: LGTM—more conservative SysId voltage limits.Reducing the step voltage targets (arm: 3V → 1.5V, elevator: 3V → 2V) makes characterization safer, reducing the risk of mechanism damage during SysId runs.
181-187: LGTM—substantial arm control tuning applied.The PID gains were significantly updated:
- kP doubled (34 → 70) for more aggressive position control
- kG increased 3.5× (0.1117 → 0.39) for better gravity compensation
These changes should be validated through testing to ensure stability and smooth motion.
215-216: LGTM!The additional rotor velocity and position signals support the SysId characterization logging implemented in ArmElevator.java.
247-253: LGTM—elevator control substantially retuned.The PID gains were significantly updated with the gear ratio change (3 → 4):
- kP increased 3.4× (3.5 → 12)
- kD increased 2.5× (0.4 → 1)
- kA disabled (feedforward removed)
These changes must be validated through testing to ensure stability with the new mechanical configuration.
279-280: LGTM!The rotor velocity signal supports SysId characterization, and the position reset provides consistent initialization on startup.
304-306: LGTM—encoder configuration aligns with new coordinate system.The sensor direction change and discontinuity point adjustment (1.0 → 0.5) align with the shifted angle reference frame and ensure continuous readings across the working range [−90°, 270°].
316-338: Verify the reworked state machine.
- Constraint-bypassing states: LOAD_CORAL (line 324), COLLECT_ALGAE_FLOOR (line 336)
- Manually confirm:
- All target angles/positions are mechanically achievable
- Prepare→execute state chains run in the intended order
- Speed scalars (especially 0.02) yield safe, smooth motion
- Ignoring constraints on these states won’t cause collisions
28-28: Verify elevator follower motor CAN ID.CAN ID changed from 18 to 17. Ensure the physical motor is connected to ID 17 and update any hardware documentation.
44-44: Verify elevator gear ratio change.Gear ratio changed from 3 to 4, which affects all elevator kinematics. Confirm this matches the actual gearbox installed on the robot. If this corrects a previous error, consider re-characterizing the elevator with SysId.
54-58: LGTM!Replacing zero acceleration/velocity values with realistic limits enables proper arm motion on the real robot. The simulation values remain higher for faster testing.
76-77: LGTM!The updated angle range (-90° to 270°) aligns with the new coordinate system and state definitions.
100-110: LGTM!Lower SysId voltage limits provide more conservative characterization, reducing risk of mechanism damage during testing.
181-187: LGTM!Non-zero PID gains for the real robot enable proper arm control. Previous zero values would have prevented motion.
215-216: LGTM!Registering rotor velocity and position signals enables more comprehensive debugging and system identification.
247-253: LGTM!Updated elevator PID and feedforward gains reflect characterization results and should improve control performance.
279-280: LGTM!Registering rotor velocity and zeroing position on initialization are correct practices for elevator control.
304-306: LGTM!Encoder configuration changes align with the updated coordinate system and 180-degree reversal logic.
316-337: Confirm speedScalar for REST_WITH_CORAL/REST_WITH_ALGAE
These states use a 0.02 speedScalar (2% of default arm speed), which may produce under 0.1 RPS movement. Ensure this extremely low speed is intentional for safe handling—otherwise consider increasing it (e.g., to 0.1–0.2) for practical responsiveness.
28-28: LGTM!Elevator follower motor ID updated to 17, consistent with hardware remapping.
44-44: LGTM - verify elevator behavior.The elevator gear ratio changed from 3 to 4, which affects all position/velocity calculations. Ensure the new ratio has been tested and all elevator position constants in the state machine (lines 316-337) are calibrated for this gearbox.
54-58: LGTM!Motion profile parameters updated based on physical testing. The arm acceleration increase from 0 to 7 on the real robot will improve responsiveness.
76-77: LGTM - verify mechanical limits.The arm angle range shifted from [0°, 360°] to [-90°, 270°]. This aligns with the new offset logic and the
subtractFrom180Degreesimplementation. Confirm these bounds match your physical hard stops.
100-110: LGTM!Reduced SysId voltage limits (arm: 1.5V, elevator: 2V) are a safer approach for characterization testing.
181-187: LGTM!Updated PID and feedforward gains from characterization. The significant kG increase (0.11 → 0.39) suggests the arm has more mass or different center of gravity than initially modeled.
247-253: LGTM!Elevator PID and feedforward gains updated from characterization. The increased kP (3.5 → 12) and kD (0.4 → 1) will provide more aggressive position tracking.
304-306: LGTM - encoder discontinuity matches new range.The encoder discontinuity point moved to 0.5 (180°), which aligns with the new [-90°, 270°] angle range and the
subtractFrom180Degreesimplementation. The sensor direction change likely reflects physical mounting orientation.
| MINIMUM_ANGLE = Rotation2d.fromDegrees(-127), | ||
| MAXIMUM_ANGLE = Rotation2d.fromDegrees(0); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify mechanical clearances for dramatically expanded angle range.
The angle bounds have changed drastically:
MINIMUM_ANGLE: -12° → -127° (expanded by 115°)MAXIMUM_ANGLE: 110° → 0° (reduced by 110°)
This represents a fundamental change to the intake's operational envelope. At -127°, the intake may collide with the robot frame, strain cables, or interfere with other mechanisms.
Physically test the intake mechanism across the full new range (-127° to 0°) to verify:
- No collisions with robot frame or other subsystems
- No cable strain or connector stress
- Adequate structural support at extreme angles
- Intended functionality at all positions
🏁 Script executed:
#!/bin/bash
# Description: Check if there are test files or comments documenting the angle range change
rg -n -A 3 -B 3 "(MINIMUM_ANGLE|MAXIMUM_ANGLE|127|angle.*range)" --type java -g '*Test*.java' -g '*Constants*.java'Length of output: 6185
Enable software angle limits and validate mechanical range
- Uncomment and configure
config.SoftwareLimitSwitch.ReverseSoftLimitEnable = trueandForwardSoftLimitEnable = truewith thresholdsMINIMUM_ANGLE.getRotations()/MAXIMUM_ANGLE.getRotations()inIntakeConstants.java(lines 169–172) - Physically test the intake across its full new range (−127° to 0°) to confirm no collisions, cable strain, or structural interference
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java around
lines 61–62 and update at lines 169–172, enable and set the motor controller
software limits: uncomment and set
config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true and
config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true, and assign the
thresholds to MINIMUM_ANGLE.getRotations() and MAXIMUM_ANGLE.getRotations()
respectively; after making the changes, build and deploy, then physically
exercise the intake through the full range (−127° to 0°) and verify there are no
collisions, cable strain, or structural interference.
|
|
||
| import com.pathplanner.lib.util.DriveFeedforwards; | ||
|
|
||
| import com.pathplanner.lib.util.swerve.SwerveSetpoint; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the unused import.
The SwerveSetpoint class is imported but not used anywhere in the visible code.
Please confirm whether this import is:
- Needed for upcoming changes
- Used elsewhere in the codebase but the import was missing
- Dead code that should be removed
🏁 Script executed:
#!/bin/bash
# Search for SwerveSetpoint usage in this file
rg -n 'SwerveSetpoint' src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javaLength of output: 56
🏁 Script executed:
#!/bin/bash
# Search for SwerveSetpoint usage across the repository
rg -nP '\bSwerveSetpoint\b' --glob '*.java'Length of output: 117
Remove unused import
The import com.pathplanner.lib.util.swerve.SwerveSetpoint is not referenced; delete this line.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java around line 5,
remove the unused import statement
"com.pathplanner.lib.util.swerve.SwerveSetpoint" because it is not referenced
anywhere in the file; simply delete that import line and save the file to
eliminate the unused-import warning.
| config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792); | ||
| config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891); | ||
| config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180); |
There was a problem hiding this comment.
Fix inconsistent units for gyro mount pose configuration.
There's a units inconsistency in the gyro mount pose configuration:
- Line 102:
MountPoseYawusesUnits.degreesToRotations()(converts to rotations/revolutions) - Lines 103-104:
MountPosePitchandMountPoseRolluseUnits.degreesToRadians()(converts to radians)
According to CTRE Phoenix 6 documentation, the MountPose configuration expects all three values in the same units (typically rotations for the API). This inconsistency will cause incorrect gyro orientation compensation.
Apply this diff to use consistent units:
- config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792);
- config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891);
- config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180);
+ config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792);
+ config.MountPose.MountPosePitch = Units.degreesToRotations(-0.087891);
+ config.MountPose.MountPoseRoll = Units.degreesToRotations(-0.659180);Or if radians are preferred:
- config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792);
- config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891);
- config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180);
+ config.MountPose.MountPoseYaw = Units.degreesToRadians(-52.198792);
+ config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891);
+ config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180);Verify which unit system the CTRE API expects:
#!/bin/bash
# Search for CTRE Pigeon2 MountPose documentation or examples
rg -i "MountPose" --type java -C 5Co-Authored-By: Nummun14 <142012009+Nummun14@users.noreply.github.com>
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/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
42-42: Simulation bug: collection detection always true.In simulation, getScaledValue() returns sensorInputs.value (0 or 1), and both are < 6 cm, so COLLECTION_DETECTION_BOOLEAN_EVENT is always true. Return realistic distances in cm in sim.
- private static final DoubleSupplier DISTANCE_SENSOR_SIMULATION_SUPPLIER = () -> (SimulationFieldHandler.isHoldingCoral() && SimulationFieldHandler.isCoralInEndEffector()) || SimulationFieldHandler.isHoldingAlgae() ? 1 : 0; + private static final DoubleSupplier DISTANCE_SENSOR_SIMULATION_SUPPLIER = () -> { + // Near object ~3 cm, otherwise far ~100 cm + final boolean hasObject = (SimulationFieldHandler.isHoldingCoral() && SimulationFieldHandler.isCoralInEndEffector()) + || SimulationFieldHandler.isHoldingAlgae(); + return hasObject ? 3.0 : 100.0; + };Also applies to: 55-57
♻️ Duplicate comments (4)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
67-73: Fix SysId elevator logging units.The previous critical issue remains unresolved:
linearPosition/linearVelocitymust be in meters and m/s, but you're passing raw rotations (line 69) and rotor RPS (line 70). This will produce invalid SysId characterization fits for the elevator.Apply this diff to convert to proper units:
- if (SHOULD_CALIBRATE_ELEVATOR) { - log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) - .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); - return; - } + if (SHOULD_CALIBRATE_ELEVATOR) { + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); + log.motor("Elevator") + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) + .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); + return; + }src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (3)
153-153: Arm safe-angle at 0° likely disables dynamic elevator safety.Setting MAXIMUM_ARM_SAFE_ANGLE to 0° makes isArmAboveSafeAngle() true for nearly all forward poses, bypassing dynamic elevator-height safety. Revert to 90° or add equivalent protections/tests before keeping 0°.
Based on learnings
193-204: Arm software limits are disabled — restore or justify.Forward/reverse soft limits are commented out. Re-enable with thresholds that account for ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET, or add a clear TODO with owner/ticket and rationale. Running without these invites hard-stop crashes.
259-264: Elevator software limits disabled — high risk.Forward/reverse soft limits are commented out. Strongly recommend re-enabling using MAXIMUM_ELEVATOR_HEIGHT_METERS (converted to sensor units) and your reverse reset position. If temporarily disabled, add TODOs and re-enable before field use.
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (2)
29-35: Default should react to hasGamePiece changes continuously.ConditionalCommand samples once at init; it won’t switch when hasGamePiece flips. Prefer a Run command that sets the target each cycle.
-import edu.wpi.first.wpilibj2.command.ConditionalCommand; +import edu.wpi.first.wpilibj2.command.Commands; ... - public static Command getDefaultCommand() { - return new ConditionalCommand( - getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL), - getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.REST), - RobotContainer.END_EFFECTOR::hasGamePiece - ); - } + public static Command getDefaultCommand() { + return Commands.run( + () -> RobotContainer.END_EFFECTOR.setTargetState( + RobotContainer.END_EFFECTOR.hasGamePiece() + ? EndEffectorConstants.EndEffectorState.HOLD_CORAL + : EndEffectorConstants.EndEffectorState.REST + ), + RobotContainer.END_EFFECTOR + ); + }
21-27: End action is empty — confirm desired behavior.StartEndCommand does not stop the motor on end. If you want a defined safe state between commands, use END_EFFECTOR::stop.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
20-31: Prefer composing the load step rather than scheduling inside an InstantCommand.Scheduling from inside a command makes cancellation and tracing harder. Consider returning a composed sequence:
- Intake until transporter has coral
- Rumble
- If not holding algae, then run getLoadCoralCommand()
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
49-57: Calibrate and document distance-sensor scaling.Slope 0.0002 and intercept −200 imply specific units from the duty-cycle IO. Briefly document the measurement procedure, units, and sample readings, and verify the threshold of 6 cm matches reality.
Also applies to: 88-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(6 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/RobotContainer.java
- src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java
🧬 Code graph analysis (6)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (2)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(19-109)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (4)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-45)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(23-355)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (2)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)
🔇 Additional comments (20)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (14)
21-21: LGTM! Calibration flag is clear.The calibration flag provides a clean way to switch between arm and elevator SysId characterization.
47-49: LGTM! Config selection logic is correct.The method correctly returns the appropriate SysId configuration based on the calibration flag.
52-57: Good fix! Follower motors now have synchronized brake mode.Previously, follower motors were not having their brake mode set, which could lead to inconsistent mechanical behavior. This ensures both master and follower motors have synchronized brake modes.
74-78: LGTM! Arm SysId logging units are correct.The arm logging correctly uses angular units (rotations and RPS) for the rotational arm mechanism, with proper gear ratio compensation for velocity.
82-89: LGTM! Mechanism visualization uses consistent offsets.The arm mechanism visualization correctly applies offsets to both current and target positions for consistency.
107-113: LGTM! SysId voltage routing is correct.The voltage routing correctly directs test voltages to the mechanism being characterized based on the calibration flag.
153-155: LGTM! Arm angle offset applied correctly.The arm position offset compensates for mechanical mounting and encoder reference point differences, aligning the encoder reading with the actual arm angle.
195-202: LGTM! Arm state setting applies speed scaling correctly.The method properly scales motion profile parameters before setting the target angle, maintaining consistent behavior with the reversed state logic.
209-212: LGTM! Arm angle offset compensation is bidirectional.The offset is correctly subtracted when commanding motor position (inverse of the addition in
getCurrentArmAngle), ensuring the motor control works in encoder reference frame.
218-220: LGTM! Elevator position request correctly uses renamed variable.The rename from
positionRequesttoelevatorPositionRequestimproves code clarity.
222-229: LGTM! Safe angle calculation handles edge cases and adjusts reference frame.The NaN check provides a safe fallback (-90° = arm down), and the 90-degree reference frame adjustment aligns with the corresponding elevator safety calculation.
231-239: LGTM! Elevator safety calculation uses consistent angle offset.The +90° offset (line 232) is the complement of the -90° offset in
calculateMinimumArmSafeAngle(line 228), maintaining mathematical consistency between arm and elevator safety constraints.
257-267: LGTM! Speed scaling implementation is clean.The speed scaling methods properly scale all motion profile parameters (velocity, acceleration, jerk) by the same factor, maintaining profile shape while allowing variable-speed states.
132-137: Validate the revised reversed-angle formula (180° − angle)
The mirroring logic was changed from360° − angleto180° − angle(subtractFrom180Degrees), which alters outputs (e.g., REST –90°→270° instead of 90°). Confirm that all reversed states (COLLECT_ALGAE_*, SCORE_L4.prepareState, etc.) still map to the intended physical arm positions.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (3)
37-42: CANivore bus usage consistency looks good.All arm/elevator motors and the angle encoder are on the same CANivore bus via RobotConstants.CANIVORE_NAME. Good for consistent remote sensor routing.
55-56: Double-check Motion Magic units and realism for accel values.ARM_DEFAULT_MAXIMUM_ACCELERATION is 67.2344 (sim) vs 4 (real). Ensure these are in the expected Phoenix6 units for your selected sensor (rotations/s² after ratios) and are realistic for both environments to avoid overshoot or sluggish response.
Also applies to: 59-59
305-308: Verify CANcoder direction, offset, and discontinuity interplay.With CounterClockwise_Positive, MagnetOffset = −0.0625 rotations, and discontinuity at 0.5, confirm:
- Increasing arm angle corresponds to increasing encoder rotations.
- The wrap point does not cross frequently-used poses.
- Offsets align with ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET assumptions.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
49-53: New public getIntakeCoralCommand() is clear and cohesive.Encapsulating Intake+Transporter collection improves reuse across teleop/auto.
37-39: Load flow end condition looks sound.Ending on hasGamePiece, then returning arm to REST until at-state, with final unless(hasGamePiece) prevents redundant actions.
Confirm default EndEffector behavior after this sequence returns (e.g., HOLD_CORAL via default command).
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
70-75: Confirm motor inversion/neutral mode vs state voltages.With CounterClockwise_Positive and NeutralMode.Coast, ensure:
- Negative HOLD_CORAL voltage aids retention, not ejection.
- Coasting at REST won’t let the piece slip unintentionally when default command toggles to REST.
Also applies to: 92-101
| PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.41, null, false, 1), | ||
| REST(Rotation2d.fromDegrees(-90), 0.603, null, false, 0.7), |
There was a problem hiding this comment.
SCORE_L4 targets exceed maximum elevator height (1.41 m > 1.382 m).
Both PREPARE_SCORE_L4 and SCORE_L4 command 1.41 m, above MAXIMUM_ELEVATOR_HEIGHT_METERS. This can overdrive the mechanism without soft limits.
Apply this diff to clamp within limits:
- PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.41, null, false, 1),
+ PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.382, null, false, 1),
...
- SCORE_L4(Rotation2d.fromDegrees(25), 1.41, PREPARE_SCORE_L4, false, 1),
+ SCORE_L4(Rotation2d.fromDegrees(25), 1.382, PREPARE_SCORE_L4, false, 1),Also applies to: 331-331, 62-68
🤖 Prompt for AI Agents
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
lines 320-321 (also check lines 331 and 62-68): PREPARE_SCORE_L4 and SCORE_L4
use 1.41 m which exceeds MAXIMUM_ELEVATOR_HEIGHT_METERS (1.382 m); update those
entries to clamp the elevator target to the defined maximum (e.g., replace
literal 1.41 with a value computed as Math.min(1.41,
MAXIMUM_ELEVATOR_HEIGHT_METERS) or directly set to
MAXIMUM_ELEVATOR_HEIGHT_METERS when the literal is greater) and apply the same
clamp to the other occurrences at the noted lines so no constant exceeds the
maximum allowed height.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
68-72: Fix SysId elevator units.
linearPosition/linearVelocitystill log raw rotations, so any elevator SysId run will produce nonsense fits. Convert to meters before publishing.Apply this diff:
- log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) + final double elevatorPositionMeters = getCurrentElevatorPositionMeters(); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); + log.motor("Elevator") + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE)));src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
320-333: Clamp L4 elevator setpoints to the defined maximum.Both PREPARE_SCORE_L4 and SCORE_L4 command 1.5 m while
MAXIMUM_ELEVATOR_HEIGHT_METERSis 1.382 m, and the software limits are still disabled. This will drive the elevator past its mechanical range.Apply this diff:
- PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.5, null, false, 1), + PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), MAXIMUM_ELEVATOR_HEIGHT_METERS, null, false, 1), ... - SCORE_L4(Rotation2d.fromDegrees(20), 1.5, PREPARE_SCORE_L4, false, 1), + SCORE_L4(Rotation2d.fromDegrees(20), MAXIMUM_ELEVATOR_HEIGHT_METERS, PREPARE_SCORE_L4, false, 1),
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
113-125: Consider usingParallelCommandGroupinstead ofParallelRaceGroupfor semantic clarity.Both
getScoreInNetCommand()(lines 113-117) andgetAtonomouslyScoreInNetCommand()(lines 119-125) useParallelRaceGroupto coordinate multiple commands. However, examining the child commands:
getFlippableOverridableArmCommand()returns aStartEndCommand(infinite duration)runWhen(EndEffectorCommands.getSetTargetStateCommand(...))wraps anotherStartEndCommand(infinite duration)getDriveToNetCommand()ends with a closed-loop drive command (infinite duration)Since all child commands are infinite and never complete naturally, the race condition will never resolve on its own. Termination is handled by outer
untilclauses (line 59 and line 102). While functionally equivalent toParallelCommandGroupin this context,ParallelRaceGroupimplies that one command completing should terminate the group, which is misleading here.Apply this diff to improve semantic clarity in
getScoreInNetCommand():private static Command getScoreInNetCommand() { - return new ParallelRaceGroup( + return new ParallelCommandGroup( GeneralCommands.getFlippableOverridableArmCommand(ArmElevatorConstants.ArmElevatorState.SCORE_NET, false, AlgaeManipulationCommands::shouldReverseNetScore), GeneralCommands.runWhen(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_ALGAE), OperatorConstants.CONTINUE_TRIGGER) ); }And similarly for
getAtonomouslyScoreInNetCommand():private static Command getAtonomouslyScoreInNetCommand() { - return new ParallelRaceGroup( + return new ParallelCommandGroup( GeneralCommands.getFlippableOverridableArmCommand(ArmElevatorConstants.ArmElevatorState.SCORE_NET, false, AlgaeManipulationCommands::shouldReverseNetScore), GeneralCommands.runWhen(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_ALGAE), OperatorConstants.CONTINUE_TRIGGER), getDriveToNetCommand() ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(11 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(6 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(4 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (7)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(19-109)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (9)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(31-267)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-99)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(16-79)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(12-37)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(19-109)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-45)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(12-45)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(21-120)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (4)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(20-251)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-99)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (3)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(23-356)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-172)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (3)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(18-213)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)
🔇 Additional comments (16)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (4)
64-65: Verify pulse timing adjustments are tested.The pulse timing has been reduced significantly (voltage application time by 20%, wait time by 40%). Ensure these faster pulse cycles have been tested with actual coral handling to confirm they don't cause slippage or degraded performance.
35-38: Encapsulation approved—no external references detected. MakingGEAR_RATIO,MOTOR_AMOUNT, andGEARBOXprivate is safe given there are no usages outsideTransporterConstants.
25-25: Confirm beam break channel change is propagatedNo hardcoded references to channel 3 remain in the code; ensure the physical wiring has been updated to use channel 1.
106-109: Major behavioral changes to transporter states—verify thoroughly.The target voltages for all non-REST states now reverse the right motor’s direction and, in one case, alter its magnitude:
- COLLECT: right 5 → –4 V, left 5 V
- ALIGN_CORAL: right 5 → –5 V, left 6 V
- HOLD_CORAL: right 1 → –1 V, left 1 V
- EJECT: right –5 → 5 V, left –5 V
These reversals fundamentally change coral intake, alignment, and ejection behavior. Ensure they align with the updated state-machine and have been validated with real-world testing.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (7)
22-23: LGTM! Simplified intake flow.The intake sequence now clearly separates collection (until coral detected) from confirmation, making the flow easier to follow.
30-30: LGTM! Appropriate parallel command composition.Running the intake assist command alongside the main sequence is the correct approach for concurrent behavior.
52-57: LGTM! Clean public API.The simplified public
getIntakeCoralCommand()provides a clear entry point for intake operations, replacing the previous private helper logic. The parallel execution of intake and transporter is appropriate.
52-57: LGTM on the public API exposure!The visibility change from private to public for
getIntakeCoralCommand()and the simplified logic (replacing multiple private helpers) makes the intake flow more maintainable and testable. The parallel execution of intake and transporter COLLECT states is clear and straightforward.
37-42: Correct.unless()guard coverage: The.unless(RobotContainer.END_EFFECTOR::hasGamePiece)at the end applies to the entire load-and-rest sequence—if a piece is already held, neither phase runs—and no conflicting patterns were found.
52-57: Confirm continuous vs pulsed transporter behaviorTransporterCommands.getSetTargetStateCommand now drives continuously rather than the previous pulsed cycle (getSetTargetStateWithPulsesCommand: apply voltage for PULSE_VOLTAGE_APPLIED_TIME_SECONDS, wait for PULSE_WAIT_TIME_SECONDS, repeated). Verify this change has been tested to ensure reliable coral collection.
22-30: Incorrect guard removal –.until(RobotContainer.TRANSPORTER::hasCoral)remains ongetIntakeCoralCommand.Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (3)
18-24: Confirm simulation guard removal for Coral ejection commandRemoving the
onlyIf(SimulationFieldHandler::isHoldingCoral)guard allows the ejection command to run regardless of simulation state—isHoldingCoral()is still used in multiple simulation suppliers—potentially desyncing simulation behavior. Verify this change is intentional and safe.
18-24: Removal of the simulation-only guard is safe: ejection is now gated byRobotContainer.TRANSPORTER.hasCoral()orRobotContainer.INTAKE.hasCoral().
18-24: Verify removal of simulation-only guard is intentional.The
.onlyIf(SimulationFieldHandler::isHoldingCoral)guard was removed fromgetCoralEjectionCommand(). This guard previously restricted coral ejection to simulation environments only. Removing it enables ejection on the real robot.Confirm that this change is intentional and that the ejection command is safe to execute on physical hardware without the simulation guard.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
98-98: Clean separation of autonomous and manual scoring paths.The conditional selection between
getAtonomouslyScoreInNetCommand()andgetScoreInNetCommand()based onCoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLYprovides a clear separation between autonomous and operator-controlled scoring modes. This approach maintains code clarity and makes the scoring behavior explicit.
52-52: Verify and document the 0.5 s settling delay
This floor-algae command adds a 0.5 s post-detection wait via.raceWith(…WaitUntil…andThen(WaitCommand(0.5))), whereas other collection commands simply use.until(…). Confirm the END_EFFECTOR sensor bounce requires this delay, document the rationale, and ensure it doesn’t degrade cycle time.
| new InstantCommand( | ||
| () -> { | ||
| if (!AlgaeManipulationCommands.isHoldingAlgae()) | ||
| getLoadCoralCommand().schedule(); | ||
| } | ||
| ) |
There was a problem hiding this comment.
Avoid manual .schedule() inside commands.
Calling .schedule() on a command inside an InstantCommand bypasses the normal command composition patterns and can lead to issues with command lifecycle management, cancellation, and resource tracking.
Apply this diff to use proper command composition:
- new InstantCommand(
- () -> {
- if (!AlgaeManipulationCommands.isHoldingAlgae())
- getLoadCoralCommand().schedule();
- }
- )
+ Commands.either(
+ getLoadCoralCommand(),
+ Commands.none(),
+ () -> !AlgaeManipulationCommands.isHoldingAlgae()
+ )🤖 Prompt for AI Agents
In the file
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
around lines 24 to 29, the code manually schedules the getLoadCoralCommand()
inside an InstantCommand. This is not the recommended approach as it bypasses
the normal command composition patterns and can lead to issues with command
lifecycle management, cancellation, and resource tracking. Instead, refactor the
code to use proper command composition by removing the manual .schedule() call
and allowing the command framework to handle the scheduling and execution of the
getLoadCoralCommand().
| ).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen( | ||
| new ParallelCommandGroup( | ||
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING), | ||
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL) | ||
| ).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING)) | ||
| ).unless(RobotContainer.END_EFFECTOR::hasGamePiece); |
There was a problem hiding this comment.
Add timeout to prevent indefinite blocking.
The .until(RobotContainer.END_EFFECTOR::hasGamePiece) at line 37 could block indefinitely if the sensor fails to detect the coral or the game piece is not properly loaded. This would hang the command sequence and impact user experience.
Apply this diff to add a timeout:
- ).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen(
+ ).until(RobotContainer.END_EFFECTOR::hasGamePiece).withTimeout(2.0).andThen(Consider defining the timeout constant in CoralCollectionCommands or an appropriate constants file for maintainability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen( | |
| new ParallelCommandGroup( | |
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING), | |
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL) | |
| ).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING)) | |
| ).unless(RobotContainer.END_EFFECTOR::hasGamePiece); | |
| ).until(RobotContainer.END_EFFECTOR::hasGamePiece).withTimeout(2.0).andThen( | |
| new ParallelCommandGroup( | |
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING), | |
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL) | |
| ).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING)) | |
| ).unless(RobotContainer.END_EFFECTOR::hasGamePiece); |
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
around lines 37 to 42, the .until(RobotContainer.END_EFFECTOR::hasGamePiece) can
block indefinitely; add a timeout to the waiting predicates (e.g.,
.withTimeout(??, TimeUnit.SECONDS) or use .raceWith(new
WaitCommand(timeoutSeconds))) so the command chain fails or continues after a
bounded period, and likewise add a timeout to the
ParallelCommandGroup.until(...) that waits for the ARM_ELEVATOR state; define a
timeout constant (e.g., CORAL_COLLECTION_TIMEOUT_SECONDS) either as a private
static final in CoralCollectionCommands or in an existing constants file and use
that constant for both timeouts to keep maintainability.
Logic error: unless guard contradicts until condition.
The until(RobotContainer.END_EFFECTOR::hasGamePiece) on line 37 waits for the end effector to detect a game piece. Once this condition is true, execution proceeds to the andThen chain. However, the unless(RobotContainer.END_EFFECTOR::hasGamePiece) guard on line 42 prevents the entire andThen sequence from running when hasGamePiece() is true.
Since we can only reach the andThen after hasGamePiece() becomes true, the unless guard will always prevent the rest-after-loading sequence from executing. This means the arm elevator will never reach REST_AFTER_LOADING and the end effector will never hold the coral.
Remove the unless guard to fix the logic:
).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen(
new ParallelCommandGroup(
ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING),
EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL)
).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING))
- ).unless(RobotContainer.END_EFFECTOR::hasGamePiece);
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen( | |
| new ParallelCommandGroup( | |
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING), | |
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL) | |
| ).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING)) | |
| ).unless(RobotContainer.END_EFFECTOR::hasGamePiece); | |
| ).until(RobotContainer.END_EFFECTOR::hasGamePiece).andThen( | |
| new ParallelCommandGroup( | |
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING), | |
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.HOLD_CORAL) | |
| ).until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.REST_AFTER_LOADING)) | |
| ); |
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
around lines 37 to 42, the command chain uses
until(RobotContainer.END_EFFECTOR::hasGamePiece) followed by an andThen(...)
that is then wrapped with unless(RobotContainer.END_EFFECTOR::hasGamePiece),
which contradicts the until condition and prevents the sequence from ever
running; remove the trailing unless(RobotContainer.END_EFFECTOR::hasGamePiece)
so the andThen sequence executes after the until condition becomes true,
allowing the arm to move to REST_AFTER_LOADING and the end effector to enter
HOLD_CORAL as intended.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
114-114: Replace System.out.println with structured logging.This issue was previously flagged. The
System.out.printlnstatements should be replaced withLogger.recordOutputcalls to ensure non-blocking operation and proper telemetry capture.Also applies to: 125-125, 131-131
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
67-73: Fix SysId elevator logging units.
linearPosition/linearVelocitymust be in meters and m/s, but you're passing raw rotations and rotor RPS. That will produce bogus SysId fits for the elevator.Apply this diff to fix the units:
- log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); + log.motor("Elevator") + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond))src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
153-153: Arm safe angle threshold set to 0° disables dynamic elevator‐height safety for most arm positions.
MAXIMUM_ARM_SAFE_ANGLEwas lowered from 90° to 0°, soisArmAboveSafeAngle()now returns true for any angle ≥ 0°. As a result,calculateMinimumSafeElevatorHeightRotations()(ArmElevator.java:241–249) applies only the static minimum elevator safe zone for nearly all forward arm angles, bypassing the intended dynamic height calculation.Revert
MAXIMUM_ARM_SAFE_ANGLEto 90° or confirm this change won't compromise safety:- static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(0); + static final Rotation2d MAXIMUM_ARM_SAFE_ANGLE = Rotation2d.fromDegrees(90);
199-203: Re-enable arm software limits.Commented-out forward/reverse software limit switches remove critical safety bounds. Without these limits, the arm could exceed mechanical stops if commanded beyond the angle range.
Apply this diff to restore the limits with the updated angle range:
-// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); - -// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270); + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET; + + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;
259-263: Re-enable elevator software limits.Commented-out elevator software limits are especially concerning as the elevator could crash into physical stops, potentially damaging the mechanism or causing safety hazards.
Apply this diff to restore the limits:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = 0; + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS);Note: You'll need to add a helper method or inline the conversion since
metersToRotationsis defined in the subsystem class, not the constants class.
320-320: SCORE_L4 and SCORE_NET targets exceed maximum elevator height.
PREPARE_SCORE_L4(1.5m),SCORE_L4(1.5m), andSCORE_NET(1.636m) all exceedMAXIMUM_ELEVATOR_HEIGHT_METERS(1.382m). This can overdrive the mechanism without soft limits.Apply this diff to clamp within limits:
- PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.5, null, false, 1), + PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), MAXIMUM_ELEVATOR_HEIGHT_METERS, null, false, 1), ... - SCORE_L4(Rotation2d.fromDegrees(-10), 1.5, PREPARE_SCORE_L4, false, 0.8), + SCORE_L4(Rotation2d.fromDegrees(-10), MAXIMUM_ELEVATOR_HEIGHT_METERS, PREPARE_SCORE_L4, false, 0.8), - SCORE_NET(Rotation2d.fromDegrees(70), 1.636, null, false, 0.3), + SCORE_NET(Rotation2d.fromDegrees(70), MAXIMUM_ELEVATOR_HEIGHT_METERS, null, false, 0.3),Also applies to: 332-332, 333-333
🧹 Nitpick comments (7)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
108-110: Add safeguards and logging for position resets.This public method resets the encoder position without any checks or logging. Consider:
- Add a JavaDoc comment explaining when this should be called (e.g., during initialization, after homing to a limit switch)
- Log the reset operation via
Logger.recordOutput("Intake/PositionReset", true)for debugging- Consider whether this should only be callable when the intake is at a known physical position (e.g., at a limit switch)
Apply this diff to add logging:
public void resetIntakePosition() { + Logger.recordOutput("Intake/PositionReset", true); intakeMotor.setPosition(0); }src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
49-49: Debounce time reduced by 50%.The debounce time decreased from 0.2 to 0.1 seconds, making the sensor more responsive. However, this may increase susceptibility to false positives from sensor noise or transient readings. Ensure this has been tested under actual operating conditions.
src/main/java/lib/hardware/misc/servo/ServoIO.java (1)
30-31: Add JavaDoc documentation for the new stop() method.The new
stop()method lacks documentation explaining its purpose and contract. Consider adding JavaDoc to clarify what stop is intended to do and how subclasses should override it.Apply this diff to add documentation:
+ /** + * Stops the servo. Subclasses should override this to implement hardware-specific stop behavior. + */ protected void stop() { }src/main/java/lib/hardware/misc/servo/Servo.java (3)
41-44: Consider using a small epsilon for floating-point comparison.Exact equality (
== 0) on doubles can miss near-zero values due to floating-point precision. While explicit 0 values will work, consider using a small epsilon threshold (e.g.,Math.abs(targetSpeed) < 1e-6) for more robust comparison.Apply this diff to use epsilon comparison:
- if (targetSpeed == 0) { + if (Math.abs(targetSpeed) < 1e-6) { stop(); return; }
58-61: Consider using a small epsilon for floating-point comparison.Similar to
setTargetSpeed, exact equality (== 0) on doubles can miss near-zero values. Consider using a small epsilon threshold for more robust comparison.Apply this diff to use epsilon comparison:
- if (value == 0) { + if (Math.abs(value) < 1e-6) { stop(); return; }
65-67: Add JavaDoc documentation for the new public method.The new
stop()method lacks documentation. Add JavaDoc to explain what stop does and how it differs from setting speed/value to 0.Apply this diff to add documentation:
+ /** + * Stops the servo by disabling it (on real hardware) or setting its target speed to 0 (in simulation). + * This is called automatically when setting target speed or value to 0. + */ public void stop() { servoIO.stop(); }src/main/java/lib/hardware/misc/servo/io/SimulationServoIO.java (1)
30-33: Consider whether targetScaledPosition should also be reset.The stop() method only resets
targetSpeed, buttargetScaledPositionremains unchanged. This matches the existing pattern inset(double)(Line 27), but it creates a potential inconsistency where the servo is "stopped" by speed but still has a position target. Consider whether both should be reset for a complete stop state.Apply this diff if complete state reset is desired:
@Override protected void stop() { this.targetSpeed = 0; + this.targetScaledPosition = 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/constants/CameraConstants.java(2 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/climber/Climber.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(6 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(3 hunks)src/main/java/lib/hardware/misc/servo/Servo.java(2 hunks)src/main/java/lib/hardware/misc/servo/ServoIO.java(1 hunks)src/main/java/lib/hardware/misc/servo/io/RealServoIO.java(1 hunks)src/main/java/lib/hardware/misc/servo/io/SimulationServoIO.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java
- src/main/java/frc/trigon/robot/RobotContainer.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (1)
ClimbCommands(17-54)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (3)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(18-213)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(23-356)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)
src/main/java/frc/trigon/robot/constants/CameraConstants.java (1)
src/main/java/frc/trigon/robot/poseestimation/apriltagcamera/AprilTagCameraConstants.java (1)
AprilTagCameraConstants(17-66)
🔇 Additional comments (29)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
138-138: Verify the rotation sign flip produces correct visualization.The Y-axis rotation was changed from positive to negative. Ensure that this change correctly represents the intake's physical orientation in the 3D visualization and that the telemetry displays match the robot's actual position.
Please verify that the intake visualization now displays correctly in your telemetry system (e.g., AdvantageScope) and that the orientation matches the physical robot.
src/main/java/frc/trigon/robot/subsystems/climber/Climber.java (2)
31-31: LGTM: Clean delegation to dedicated stop method.The refactor to delegate servo stopping to
stopServos()improves code organization and aligns with the newstop()capability on servo hardware.
108-111: Good addition, but note inconsistency with updatePeriodically().The new
stopServos()method improves code clarity by explicitly stopping both servos. However, there's an inconsistency: this method stops both servos, butupdatePeriodically()(line 48) doesn't update the left servo. This mismatch could lead to the left servo being in an inconsistent state.src/main/java/frc/trigon/robot/constants/CameraConstants.java (3)
27-34: Verify the coordinate swap and orientation changes match physical camera placement.The left and right reef tag camera transforms have significant changes:
- Translation coordinates swapped: X and Y values exchanged (e.g., left changed from
(-0.129, 0.2032)to(-0.2032, 0.129))- 180° roll rotation added to both cameras'
Rotation3dThese changes suggest the cameras have been physically repositioned or their mounting orientation changed. Since these constants are critical for accurate pose estimation, incorrect values will lead to navigation and autonomous failures.
Please verify:
- The new translation coordinates accurately reflect the physical camera positions measured from the robot center
- The 180° roll rotation correctly represents the camera mounting orientation
- Test pose estimation with AprilTags to confirm the transformations produce accurate results
42-46: LGTM! Good typo fix and consistent renaming.The changes improve code clarity:
- Renamed constant from
INTAKE_SIDE_REEF_TAG_CAMERAtoFRONT_REEF_TAG_CAMERAfor better semantics- Fixed camera name string from
"IntakeSideReefTagCameraCamera"(duplicate "Camera") to"FrontReefTagCamera"- Updated transform reference to match the renamed constant
23-26: All old constant references removed Verified thatROBOT_CENTER_TO_INTAKE_REEF_TAG_CAMERAno longer appears in any Java file outsideCameraConstants.java.src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (5)
9-9: LGTM: CANivore integration added.The addition of
RobotConstants.CANIVORE_NAMEproperly integrates the motor with the CANivore network as shown in the relevant code snippets.Also applies to: 26-26
42-42: Simulation and detection logic updated correctly.The simulation supplier and detection predicate now properly use scaled values. In simulation, the supplier returns 1 (game piece present, < 6 cm threshold) or 10 (no game piece, > 6 cm threshold), which correctly triggers detection. The change from
getBinaryValue()togetScaledValue() < COLLECTION_DETECTION_DISTANCE_CENTIMETRESaligns with the sensor type change to duty cycle.Note: This approval assumes the scaling constants flagged in the previous comment are corrected.
Also applies to: 56-56
70-71: Verify motor configuration changes align with mechanical design.Two significant motor configuration changes:
NeutralMode:Brake→Coast- allows free spinning when unpowered instead of providing resistanceInverted:Clockwise_Positive→CounterClockwise_Positive- reverses motor directionThese changes fundamentally alter the end effector's behavior. Confirm with the mechanical team that these changes are intentional and have been tested.
92-101: Verify large voltage changes for EndEffector states.Several state voltages changed significantly:
SCORE_CORAL: 4V → 1.5V (62.5% reduction)COLLECT_ALGAE: -4V → -7V (75% increase in magnitude)SCORE_ALGAE: 4V → 10V (150% increase)- New state:
HOLD_CORALat -0.5VHOLD_ALGAE: -4V → -7V (75% increase in magnitude)These changes will substantially impact scoring speed, collection force, and game piece handling. Ensure these values have been tested and validated for the intended game piece manipulation behavior.
22-22: Verify scaling constants against actual sensor output. No definitions or docs found for the sensor’s duty-cycle range; ensure(value * 0.0002) - 200maps raw readings (0–1) to the intended 0–10 cm range.src/main/java/lib/hardware/misc/servo/io/RealServoIO.java (1)
39-42: LGTM!The stop() implementation correctly disables the servo hardware, which is the appropriate behavior for stopping a real servo.
src/main/java/lib/hardware/misc/servo/io/SimulationServoIO.java (1)
30-33: LGTM!The stop() implementation correctly sets the target speed to 0 for simulation, matching the expected behavior for stopping a servo in simulation mode.
src/main/java/lib/hardware/misc/servo/Servo.java (1)
41-44: Verify zero-speed to stop() behavior change Replace direct 0-speed commands withstop()(disabling the servo), and audit allsetTargetSpeed(...)andset(...)call sites to ensure no code relies on “speed = 0” versus “servo disabled”.src/main/java/frc/trigon/robot/constants/OperatorConstants.java (4)
8-8: LGTM!The
ClimbCommandsimport is properly used for gating score triggers when climbing.
22-22: LGTM!Changing the right stick exponent from 1 to 2 provides finer control at lower stick deflections while maintaining full range at higher deflections.
67-69: LGTM!The new reset triggers are correctly wired to
resetElevatorPosition()andresetIntakePosition()in RobotContainer.java.
84-103: LGTM!The climb-gating logic correctly prevents scoring operations during climb by checking
!ClimbCommands.isClimbing().src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (5)
21-21: LGTM!The
SHOULD_CALIBRATE_ELEVATORflag allows toggling between arm and elevator calibration modes for SysId characterization.
33-38: LGTM!Renaming
positionRequesttoelevatorPositionRequestimproves clarity by distinguishing it from the arm'sarmPositionRequest.
115-117: LGTM!The
resetElevatorPosition()method is correctly wired to the reset trigger in RobotContainer.java.
267-277: LGTM!The speed-scaling helpers correctly adjust motion parameters based on the state's
speedScalarfield.
255-257: Angle reversal logic is correct.subtractFrom180Degreesmaps the new range [-90°, 270°] onto itself, so reversed angles remain within bounds and behave as intended.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
37-50: LGTM!The motor/encoder constructors correctly reference
RobotConstants.CANIVORE_NAME, and the offset values align with the new calibration scheme.
45-45: LGTM!Changing
ELEVATOR_GEAR_RATIOfrom 3 to 4 reflects a hardware gearing update.
55-59: LGTM!The velocity and acceleration tuning values provide reasonable default motion parameters for both simulation and real robot.
77-78: LGTM!The updated arm angle range of [-90°, 270°] aligns with the new offset scheme and supports the full physical range of motion.
182-188: LGTM!The arm master PID/feedforward gains are reasonable for the updated gear ratio and motion profile.
248-254: LGTM!The elevator master PID/feedforward gains are reasonable for the updated gear ratio and motion profile.
| motor.update(); | ||
| rightServo.update(); | ||
| leftServo.update(); | ||
| // leftServo.update(); |
There was a problem hiding this comment.
Critical: Left servo not updated periodically.
The left servo's update() call is commented out, which means its state won't synchronize with hardware, and control commands won't execute. This creates an asymmetry with the right servo (line 47) and conflicts with stopServos() (line 109) which assumes both servos are operational.
If this is temporary (testing, missing hardware), add a TODO comment explaining when/why it will be re-enabled. Otherwise, either re-enable it or remove the leftServo entirely.
Apply this diff to re-enable the left servo update:
- // leftServo.update();
+ leftServo.update();Or if temporarily disabled, add a TODO:
- // leftServo.update();
+ // TODO: disabled pending left servo hardware installation; re-enable when servo connected
+ // leftServo.update();🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/climber/Climber.java around line
48, the left servo.update() call is commented out, leaving the left servo
unsynchronized with hardware; either re-enable the update call to match the
right servo or, if this was intentional for temporary testing, restore the call
but add a clear TODO comment explaining why it is disabled and when it will be
re-enabled (or remove the left servo entirely if not used). Ensure symmetry with
the rightServo.update() call at line 47 and that stopServos() at line 109
correctly handles both servos after your change.
| public void setServoPowers(double power) { | ||
| rightServo.setTargetSpeed(power); | ||
| leftServo.setTargetSpeed(-power); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify public visibility is required.
The method visibility appears to have changed to public, but no external callers are evident in the provided context. If this method is only used internally (line 96), keep it private to reduce the public API surface.
Run the following script to check for external usage:
🏁 Script executed:
#!/bin/bash
# Description: Search for external calls to setServoPowers outside Climber.java
# Search for calls to setServoPowers in other files
rg -n --type=java -g '!Climber.java' 'setServoPowers' -A 2 -B 2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for external calls to setServoPowers in other Java files
rg -nP 'setServoPowers\s*\(' -g '*.java' -g '!src/main/java/frc/trigon/robot/subsystems/climber/Climber.java'Length of output: 221
Make setServoPowers private
This method is only called within Climber, so reduce the public API surface by changing its visibility.
src/main/java/frc/trigon/robot/subsystems/climber/Climber.java:103
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/climber/Climber.java around lines
103 to 106, change the method visibility of setServoPowers from public to
private since it is only used internally; update the method signature to
private, verify there are no external references or tests depending on it, and
run a build to ensure visibility change causes no compilation issues.
There was a problem hiding this comment.
Actionable comments posted: 0
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/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
112-132: Inconsistent drive command scheduling between net and processor scoring.The processor scoring command (Lines 127-132) uses
finallyDoto schedulegetFieldRelativeDriveCommand(), but the net scoring command (Lines 112-117) does not. Since both commands are part of the sameSelectCommandstructure and have similar lifecycles, this inconsistency could lead to unexpected behavior differences.Expected behavior:
- After scoring in the processor, the drive command is scheduled, returning control to the driver
- After scoring in the net, no drive command is scheduled (current robot state continues)
Please verify whether:
- Net scoring should also schedule the drive command for consistency, or
- There is a specific reason processor scoring requires this but net scoring does not
If both should schedule the drive command, apply this diff:
private static Command getScoreInNetCommand() { return new ParallelRaceGroup( GeneralCommands.getFlippableOverridableArmCommand(ArmElevatorConstants.ArmElevatorState.SCORE_NET, false, AlgaeManipulationCommands::shouldReverseNetScore), GeneralCommands.runWhen(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_ALGAE), OperatorConstants.CONTINUE_TRIGGER) - ); + ).finallyDo(GeneralCommands.getFieldRelativeDriveCommand()::schedule); }
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
98-99: Consider extracting the conditional logic to reduce duplication.The ternary operators duplicate the
SHOULD_SCORE_AUTONOMOUSLYcheck for both scoring paths.Consider extracting a helper method to improve readability:
+ private static Command getAutonomousAwareCommand(Command autonomousCommand, Command manualCommand) { + return CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? autonomousCommand : manualCommand; + } + private static Command getScoreAlgaeCommand() { return new SelectCommand<>( Map.of( 0, getHoldAlgaeCommand(), - 1, CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? getAtonomouslyScoreInNetCommand() : getScoreInNetCommand(), - 2, CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? getAtonomouslyScoreInProcessorCommand() : getScoreInProcessorCommand() + 1, getAutonomousAwareCommand(getAtonomouslyScoreInNetCommand(), getScoreInNetCommand()), + 2, getAutonomousAwareCommand(getAtonomouslyScoreInProcessorCommand(), getScoreInProcessorCommand()) ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(20-251)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(12-37)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-104)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(16-79)
🔇 Additional comments (4)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (4)
52-52: LGTM!The addition of a 0.5-second delay after game piece detection provides a safety margin to ensure the algae is fully secured before proceeding. This is a sensible improvement over the previous immediate transition.
134-140: LGTM! Consistent with non-autonomous processor scoring.The autonomous processor scoring command correctly includes the drive-to-processor behavior and maintains consistency by scheduling the field-relative drive command in
finallyDo, matching the pattern established in the non-autonomous version.Note: The same
SHOULD_SCORE_AUTONOMOUSLYtoggle concern mentioned for net scoring (Lines 119-125) applies here as well.
198-198: LGTM! Good cleanup practice.Adding
finallyDoto resetSHOULD_COLLECT_FROM_LOLLIPOPensures the flag is properly cleaned up when the command ends, is interrupted, or is cancelled. This prevents the flag from remaining set incorrectly and is consistent with similar cleanup patterns elsewhere in the file.
119-125: VerifySHOULD_SCORE_AUTONOMOUSLYdynamic interruption
BothgetDriveToNetCommand()(line 153) andgetDriveToProcessorCommand()(line 162) are wrapped in.onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY)inside aParallelRaceGroup. Flipping the flag to false mid-execution will cancel the entire race (arm and end-effector commands). Confirm this is intended. If not, consider:
- Removing the
.onlyWhile(...)from those drive commands- Capturing the flag’s value at command creation instead of checking it dynamically
- Using
ParallelCommandGroupso drive completion doesn’t terminate other commands
Co-Authored-By: Nummun14 <142012009+Nummun14@users.noreply.github.com>
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/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
53-64: Add automated tests forgetAutonomouslyPrepareScoreCommand.Cover key timing scenarios:
- Arm‐angle gating via
.until(isPrepareArmAngleAboveCurrentArmAngle).unless(() -> REEF_CHOOSER.getScoringLevel() == ScoringLevel.L1)skip behavior- Load delays and arm prep overlap in parallel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(19-67)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-104)
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
42-51: LGTM! Explicit load step improves manual scoring flow.The addition of
getLoadCoralCommand()at line 44 ensures coral is properly loaded from the transporter to the end effector before arm preparation. The.unless(END_EFFECTOR::hasGamePiece)guard ingetLoadCoralCommand()safely handles cases where coral is already loaded.The operator confirmation via
CONTINUE_TRIGGER(line 45) provides appropriate manual control before executing the final scoring motion.
| public static Command getScoreInReefCommand(boolean shouldScoreRight) { | ||
| return new SequentialCommandGroup( | ||
| CoralCollectionCommands.getLoadCoralCommand(), | ||
| new ConditionalCommand( | ||
| getAutonomouslyScoreCommand(shouldScoreRight), | ||
| getScoreCommand(shouldScoreRight), | ||
| () -> SHOULD_SCORE_AUTONOMOUSLY && REEF_CHOOSER.getScoringLevel() != ScoringLevel.L1 | ||
| ) | ||
| return new ConditionalCommand( | ||
| getAutonomouslyScoreCommand(shouldScoreRight), | ||
| getScoreCommand(shouldScoreRight), | ||
| () -> SHOULD_SCORE_AUTONOMOUSLY && REEF_CHOOSER.getScoringLevel() != ScoringLevel.L1 | ||
| ).onlyIf(CoralCollectionCommands::hasCoral); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify edge case: holding algae with coral in transporter.
The hasCoral() check at line 29 could return true when holding algae if the transporter contains coral. In this scenario:
- The guard passes (coral detected in transporter)
getLoadCoralCommand()skips due to.unless(END_EFFECTOR::hasGamePiece)(occupied by algae)- The scoring sequence proceeds but attempts to score algae instead of coral
While teleop triggers filter by !isHoldingAlgae(), this edge case could occur if getScoreInReefCommand is invoked directly in autonomous routines.
Consider tightening the hasCoral() implementation to return false when holding algae:
// In CoralCollectionCommands.java
static boolean hasCoral() {
return !AlgaeManipulationCommands.isHoldingAlgae()
&& (RobotContainer.END_EFFECTOR.hasGamePiece() || RobotContainer.TRANSPORTER.hasCoral());
}Run the following script to verify all call sites and confirm this edge case is properly handled:
🏁 Script executed:
#!/bin/bash
# Search for direct calls to getScoreInReefCommand that might bypass trigger guards
rg -nP '\bgetScoreInReefCommand\s*\(' --type=java -C3Length of output: 2172
🏁 Script executed:
#!/bin/bash
rg -nP '\bstatic\s+boolean\s+hasCoral\b' -C5 --type javaLength of output: 1221
Exclude algae in hasCoral() guard
In CoralCollectionCommands.java (line 64), change hasCoral() to:
static boolean hasCoral() {
return !AlgaeManipulationCommands.isHoldingAlgae()
&& (RobotContainer.END_EFFECTOR.hasGamePiece() || RobotContainer.TRANSPORTER.hasCoral());
}so it returns false when holding algae, preventing algae from being scored.
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
around lines 24-30, update the guard method used
(CoralCollectionCommands.hasCoral at line ~64 in CoralCollectionCommands.java)
so it returns false when the robot is holding algae; change hasCoral() to first
check AlgaeManipulationCommands.isHoldingAlgae() and return false if true,
otherwise return whether the end effector or transporter currently has coral
(i.e., !AlgaeManipulationCommands.isHoldingAlgae() &&
(RobotContainer.END_EFFECTOR.hasGamePiece() ||
RobotContainer.TRANSPORTER.hasCoral())); ensure any necessary imports or static
references are present.
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 (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
119-125: Fix the typo in the method name.The method name contains a spelling error: "Atonomously" should be "Autonomously" (missing the 'u').
Apply this diff:
-private static Command getAtonomouslyScoreInNetCommand() { +private static Command getAutonomouslyScoreInNetCommand() {Don't forget to update the call site on Line 98.
134-140: Fix the typo in the method name.Same spelling error as the net scoring method: "Atonomously" should be "Autonomously".
Apply this diff:
-private static Command getAtonomouslyScoreInProcessorCommand() { +private static Command getAutonomouslyScoreInProcessorCommand() {Don't forget to update the call site on Line 99.
src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (1)
148-154: Critical: Units mismatch in rotation difference calculation.Line 152 mixes radians and degrees:
Math.atan2()returns radians but is subtracted frompose.getRotation().getDegrees()which returns degrees. This corrupts the distance rating calculation and will cause incorrect object tracking.Apply this diff to fix the units mismatch:
- final double rotationDifferenceDegrees = Math.abs(pose.getRotation().getDegrees() - Math.atan2(yDifference, xDifference)); + final double rotationDifferenceDegrees = Math.abs(pose.getRotation().getDegrees() - Math.toDegrees(Math.atan2(yDifference, xDifference)));
♻️ Duplicate comments (12)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (3)
5-5: Remove unused import.The unused import flagged in the previous review remains unaddressed.
5-5: Remove unused import.This issue was previously flagged and remains unresolved. The
SwerveSetpointimport is not referenced anywhere in the file.Apply this diff:
-import com.pathplanner.lib.util.swerve.SwerveSetpoint;
5-5: Remove unused import.The import
com.pathplanner.lib.util.swerve.SwerveSetpointis not referenced anywhere in the file.Apply this diff:
-import com.pathplanner.lib.util.swerve.SwerveSetpoint;src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (3)
102-104: Fix inconsistent units for gyro mount pose configuration.The units inconsistency flagged in the previous review remains unaddressed. Line 102 uses
degreesToRotations()while lines 103-104 usedegreesToRadians(). This will cause incorrect gyro orientation compensation.
102-104: Fix inconsistent units for gyro mount pose configuration.This critical issue was previously flagged and remains unresolved. Line 102 uses
Units.degreesToRotations()for yaw, while lines 103-104 useUnits.degreesToRadians()for pitch and roll. According to CTRE Phoenix 6 documentation, all three mount pose values must use the same units (typically rotations).Apply this diff to use consistent units (rotations):
- config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792); - config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891); - config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180); + config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792); + config.MountPose.MountPosePitch = Units.degreesToRotations(-0.087891); + config.MountPose.MountPoseRoll = Units.degreesToRotations(-0.659180);
102-104: Fix inconsistent units for gyro mount pose configuration.There's a units inconsistency in the gyro mount pose configuration:
- Line 102:
MountPoseYawusesUnits.degreesToRotations()(converts to rotations/revolutions)- Lines 103-104:
MountPosePitchandMountPoseRolluseUnits.degreesToRadians()(converts to radians)According to CTRE Phoenix 6 documentation, the
MountPoseconfiguration expects all three values in the same units (typically rotations for the API). This inconsistency will cause incorrect gyro orientation compensation.Apply this diff to use consistent units:
- config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792); - config.MountPose.MountPosePitch = Units.degreesToRadians(-0.087891); - config.MountPose.MountPoseRoll = Units.degreesToRadians(-0.659180); + config.MountPose.MountPoseYaw = Units.degreesToRotations(-52.198792); + config.MountPose.MountPosePitch = Units.degreesToRotations(-0.087891); + config.MountPose.MountPoseRoll = Units.degreesToRotations(-0.659180);src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
61-76: Restore intake soft limits for the new −127°…0° envelope.
MINIMUM_ANGLE now dives 115° deeper, yet both TalonFX soft limits stay commented out, leaving the arm with zero electronic end‑stops (and no auto-zero fallback). A bad target or drift will now smash into the hard stop/cabling. Please re-enable the forward/reverse soft limits (thresholds = MINIMUM_ANGLE / MAXIMUM_ANGLE) or add an equivalent safety layer and confirm the hardware can actually traverse this range before merging.-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = MINIMUM_ANGLE.getRotations(); -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = MAXIMUM_ANGLE.getRotations(); + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = MINIMUM_ANGLE.getRotations(); + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = MAXIMUM_ANGLE.getRotations();Also applies to: 171-175
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (1)
67-73: Fix SysId elevator logging units.
linearPosition/linearVelocitymust be in meters and m/s, but you're passing raw rotations and rotor RPS. This will produce incorrect SysId characterization.Apply this diff to fix the units:
if (SHOULD_CALIBRATE_ELEVATOR) { + final double elevatorPositionMeters = rotationsToMeters(getElevatorPositionRotations()); + final double elevatorVelocityMetersPerSecond = rotationsToMeters( + elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO + ); log.motor("Elevator") - .linearPosition(Units.Meters.of(getElevatorPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(elevatorMasterMotor.getSignal(TalonFXSignal.ROTOR_VELOCITY) / ArmElevatorConstants.ELEVATOR_GEAR_RATIO)) + .linearPosition(Units.Meters.of(elevatorPositionMeters)) + .linearVelocity(Units.MetersPerSecond.of(elevatorVelocityMetersPerSecond)) .voltage(Units.Volts.of(elevatorMasterMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); return; }src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (4)
157-157: Safe angle threshold set to 0° disables dynamic elevator safety.
MAXIMUM_ARM_SAFE_ANGLEwas changed from 90° to 0°, causingisArmAboveSafeAngle()to return true for nearly all forward arm positions (≥ 0°). This bypasses the dynamic elevator-height safety calculation incalculateMinimumSafeElevatorHeightRotations(), applying only the static minimum safe zone. This was flagged as critical in a previous review.Verify this change doesn't compromise safety or revert to 90°.
202-212: Re-enable software limit switches for arm safety.The velocity filter (line 202) and forward/reverse software limit switches (lines 208-212) remain commented out from a previous review. Without soft limits, the arm could exceed mechanical bounds if commanded beyond the angle range (-90° to 270°).
Either re-enable these safety controls with updated thresholds or remove the dead code with a documented justification.
268-272: Re-enable elevator software limits to prevent crashes.Elevator software limits remain disabled, which is more critical than the arm case. The elevator could crash into physical stops, potentially causing mechanism damage or safety hazards.
Strongly recommend re-enabling limits with proper thresholds:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = 0; + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + final double maxHeightRotations = lib.utilities.Conversions.distanceToRotations( + MAXIMUM_ELEVATOR_HEIGHT_METERS, + DRUM_DIAMETER_METERS + ); + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = maxHeightRotations;
326-349: Fix elevator heights exceeding maximum limits.Multiple states specify elevator heights exceeding
MAXIMUM_ELEVATOR_HEIGHT_METERS(1.382m):
PREPARE_SCORE_L4: 1.5m (line 329)SCORE_L4: 1.5m (line 342)SCORE_NET: 1.644m (line 343)While safety logic will clamp these values, the constants should reflect valid positions to avoid confusion and potential issues if the safety logic is bypassed.
Apply this diff to clamp within limits:
- PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.5, null, false, 1), + PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.382, null, false, 1), PREPARE_REST(Rotation2d.fromDegrees(-90), 0.603, null, false, 0.8), REST(Rotation2d.fromDegrees(-90), 0.534, PREPARE_REST, true, 0.8), ... - SCORE_L4(Rotation2d.fromDegrees(-10), 1.5, PREPARE_SCORE_L4, false, 0.8), - SCORE_NET(Rotation2d.fromDegrees(70), 1.644, null, false, 0.3), + SCORE_L4(Rotation2d.fromDegrees(-10), 1.382, PREPARE_SCORE_L4, false, 0.8), + SCORE_NET(Rotation2d.fromDegrees(70), 1.382, null, false, 0.3),
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java (1)
157-170: Servo configuration is now a no-op.The
configureServos()method body is entirely commented out, making it an empty method that's still called in the static block (line 106). While this is consistent with the commented servo constants (lines 53-58), if servos are not being used, consider removing the method call from the static initializer to avoid confusion.src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
52-52: Consider extracting the 0.1-second delay as a named constant.The 0.1-second wait after detecting a game piece serves as a debounce/confirmation period but is currently a magic number.
Extract this value to improve readability:
+private static final double GAME_PIECE_DETECTION_CONFIRMATION_SECONDS = 0.1; + ... -getInitiateFloorAlgaeCollectionCommand().raceWith(new WaitUntilCommand(RobotContainer.END_EFFECTOR::hasGamePiece).andThen(new WaitCommand(0.1))), +getInitiateFloorAlgaeCollectionCommand().raceWith(new WaitUntilCommand(RobotContainer.END_EFFECTOR::hasGamePiece).andThen(new WaitCommand(GAME_PIECE_DETECTION_CONFIRMATION_SECONDS))),
98-99: Consider refactoring the conditional logic for better readability.Embedding ternary operators directly in
Map.of()is valid but may reduce clarity.Refactor to a helper method:
+private static Command getNetScoringCommand() { + return CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? + getAtonomouslyScoreInNetCommand() : + getScoreInNetCommand(); +} + +private static Command getProcessorScoringCommand() { + return CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? + getAtonomouslyScoreInProcessorCommand() : + getScoreInProcessorCommand(); +} + private static Command getScoreAlgaeCommand() { return new SelectCommand<>( Map.of( 0, getHoldAlgaeCommand(), - 1, CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? getAtonomouslyScoreInNetCommand() : getScoreInNetCommand(), - 2, CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY ? getAtonomouslyScoreInProcessorCommand() : getScoreInProcessorCommand() + 1, getNetScoringCommand(), + 2, getProcessorScoringCommand() ),src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
34-34: Consider the trade-off of reduced SysID voltage.The SysID characterization voltage was reduced from 8V to 2V. While this is safer and reduces the risk of wheel slip during characterization, it may produce less accurate feedforward gains at higher operating speeds.
If characterization results seem inaccurate at full speed, consider increasing the voltage to 4-6V for better high-speed characterization while maintaining safety.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
53-54: Document the encoder offset values.The offset constants still lack inline documentation explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSET = -0.0625was measured- Why
-23.56°is subtracted inARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET- The intended zero position of the arm
A previous review requested this documentation, which was marked as addressed but appears to be missing in the current code.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/deploy/pathplanner/paths/Example Path.path(1 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java(5 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(5 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AutonomousCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/CameraConstants.java(2 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(6 hunks)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(6 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(7 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/deploy/pathplanner/paths/Example Path.path
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
- src/main/java/frc/trigon/robot/constants/OperatorConstants.java
👮 Files not reviewed due to content moderation or server errors (3)
- src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java
- src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
- src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.javasrc/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (12)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(27-366)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/intake/Intake.java (1)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-213)
src/main/java/frc/trigon/robot/commands/commandfactories/AutonomousCommands.java (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(19-73)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(16-79)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)
src/main/java/frc/trigon/robot/constants/CameraConstants.java (1)
src/main/java/frc/trigon/robot/poseestimation/apriltagcamera/AprilTagCameraConstants.java (1)
AprilTagCameraConstants(17-66)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-173)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(20-251)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(27-366)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(12-37)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(19-109)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-104)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (3)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(18-213)
🔇 Additional comments (67)
src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java (3)
29-32: Channel conflict resolved.The previous channel conflict where both REVERSE_LIMIT_SENSOR_CHANNEL and LEFT_SERVO_CHANNEL were assigned to channel 8 has been resolved. All channels now have unique values (8, 0, 1, 4), eliminating the hardware conflict.
53-58: Clarify servo usage intent.The servo PWM constants are commented out, and servo configuration (lines 157-170) is also disabled, yet the servo objects (lines 40-42) are still instantiated and
ClimberState(line 184) retains atargetServoPowerfield. This suggests the servos may still be used with default PWM values, or the functionality is temporarily disabled.Please clarify whether:
- Servos should use default PWM boundaries
- Servo functionality is intentionally disabled pending future work
- The servo objects and related code should be removed entirely
113-113: Verify motor direction change is intentional and tested.The motor inversion has been changed from
CounterClockwise_PositivetoClockwise_Positive, which reverses the motor's direction. This affects all climber movements, position control, and potentially limit switch logic.Ensure this change is intentional and that all climber movements have been tested to confirm they operate correctly in the new direction.
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (3)
26-32: Verify the PID tuning adjustments.The PID controller parameters have been updated for the real robot. Ensure these values were thoroughly tested and produce the desired behavior for intake assistance.
176-205: LGTM! New assist mode and enum restructuring.The
AssistModeenum has been properly extended with:
- New
isAlternatefield to categorize assist modes- New
ALTERNATE_ALIGNmode that combines alignment assistance with alternate control logic- All existing modes correctly categorized
The structure is clean and allows for better code reuse between similar assist modes.
118-125: Verify non-assisted axis input scalingConfirm that when
shouldAssistXorshouldAssistYis false, bypassingMath.cbrtstill provides acceptable driver control, since the previous implementation applied the cube root to both axes unconditionally. Test manual control (e.g., inASSIST_ROTATIONmode) to ensure the change in response curve is intentional and feels correct.src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (3)
112-117: LGTM!The restructure to
ParallelRaceGroupcorrectly coordinates the arm movement with the end effector action, triggered byCONTINUE_TRIGGER. The race pattern allows the parent command's.until()clause to control termination.
127-132: Verify the safety of scheduling commands infinallyDo.Using
finallyDoto schedule another command is an unusual pattern.finallyDoexecutes when the command ends (normally, interrupted, or cancelled), and scheduling a new command at this point could lead to unexpected timing or conflict with the command scheduler's state.Confirm this is the intended behavior and that it doesn't cause race conditions. Consider whether using
.andThen()or scheduling the drive command from the parent command would be more predictable.
198-198: LGTM!Good cleanup practice. Resetting
SHOULD_COLLECT_FROM_LOLLIPOPinfinallyDoensures the flag is cleared when the command ends, consistent with the pattern used forIS_HOLDING_ALGAEon Line 60.src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (11)
64-64: LGTM!The translation PID constant update from 4.2 to 5 appears to be a reasonable tuning adjustment.
108-108: LGTM!The gyro signal registration correctly uses the odometry frequency for the YAW signal, ensuring proper threading and update rates.
36-39: Verify and test new wheel diameterAll diameter references are parameterized via Conversions; confirm odometry, velocity, and autonomous routines have been validated with the new 0.076 m value.
31-34: LGTM! Encoder calibration applied.The steering encoder offsets have been properly calibrated for all four swerve modules. These values are typical for absolute encoder offset corrections.
64-64: LGTM! Translation PID tuning applied.The translation PID proportional gain has been increased from 4.2 to 5, which should provide slightly more aggressive position correction.
108-108: LGTM! Gyro signal registration improves odometry.Registering the yaw signal at the odometry frequency enables threaded signal updates, which should improve pose estimation accuracy and reduce latency.
36-39: Verify physical wheel diameter: Changed from 0.05 m×2 (100 mm) to 0.038 m×2 (76 mm) in SwerveConstants (lines 36–39); confirm this matches your actual wheels to avoid odometry and path-planning errors.
31-34: LGTM!The encoder offset calibration values have been updated to match the physical hardware configuration.
36-39: LGTM!The wheel diameter measurements have been corrected from 0.05m (50mm) to 0.038m (38mm) radius, which significantly affects odometry calculations. Ensure this matches the actual physical wheel radius.
64-64: LGTM!The translation PID proportional gain has been increased from 4.2 to 5, providing more aggressive position tracking.
108-108: LGTM!The gyro YAW signal is now properly registered for threaded updates at the odometry frequency, which should improve pose estimation accuracy.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (11)
80-85: LGTM!The drive motor gains appear to have been re-characterized. The new values are more realistic than the previous ones (especially kS which was previously 5.25V, unrealistically high for a modern motor).
Changes:
- kP: 50 → 1 (more reasonable proportional gain)
- kS: 5.25 → 0.24025 (realistic static friction voltage)
- kV: 0.8774 → 0.91963 (slight velocity gain adjustment)
- kA: 0.020691 → 0.069151 (increased acceleration gain)
101-101: Verify the increased steer motor current limit.The steer motor current limit was increased from 30A to 50A (67% increase). While this allows more torque for faster/more aggressive steering, it also increases thermal stress on the motors.
Confirm that:
- The motors can handle sustained operation at 50A without overheating
- The breakers/PDH can supply this increased current
- This change was necessary for performance (e.g., faster module rotation)
108-108: LGTM!Reducing the steer motor kP from 65 to 40 should make steering less aggressive and reduce oscillations, which is a sensible tuning adjustment.
34-34: Verify the reduced SysID voltage is sufficient.The maximum SysID voltage has been reduced from 8V to 2V. While lower voltages improve safety during characterization, 2V may be insufficient to overcome friction and static loads, potentially resulting in poor characterization data.
Ensure this voltage produces measurable and consistent motion across all modules.
80-85: LGTM! Verify the re-characterized drive motor gains.The drive motor PID and feedforward gains have been significantly updated, likely from a fresh system identification run:
- kP reduced from 50 to 1 (98% reduction suggests previous tuning was far too aggressive)
- kS reduced from 5.25 to 0.24025 (more accurate friction characterization)
- kV increased slightly from 0.8774 to 0.91963
- kA increased from 0.020691 to 0.069151
These changes should improve drive motor control. Verify through testing that:
- Velocity tracking is smooth without oscillations
- Acceleration/deceleration is controlled
- The robot drives straight without veering
101-101: Verify the increased steer motor current limit.The stator current limit has been increased from 30A to 50A (67% increase). While this allows more steering torque, ensure:
- The Falcon 500 motor can safely handle sustained 50A operation
- Wiring and connections are rated for the increased current
- The battery/PDP can supply the additional current during peak demand
108-108: LGTM! Steer motor tuning adjustment.The steer motor proportional gain has been reduced from 65 to 40, which should reduce oscillations and provide smoother steering control.
34-34: LGTM!The SysID characterization voltage has been reduced from 8V to 2V. This is a safer approach that reduces the risk of damage during characterization, though it will result in slower test execution.
80-85: Verify the re-characterized drive motor gains are accurate.The drive motor PID and feedforward gains have changed dramatically:
- kP: 50 → 1 (50× reduction in proportional gain)
- kS: 5.25 → 0.24025 (21× reduction in static friction compensation)
- kV: 0.8774 → 0.91963 (5% increase in velocity feedforward)
- kA: 0.020691 → 0.069151 (3.3× increase in acceleration feedforward)
These changes suggest a complete re-characterization was performed. The new kP of 1 is quite low for closed-loop velocity control and may result in sluggish response or steady-state error.
Verify these gains:
- Confirm they were derived from SysID characterization data
- Test the drive motor response to ensure adequate tracking performance
- Monitor for steady-state velocity errors during autonomous paths
- Verify the extremely low kP doesn't cause excessive settling time
101-101: Verify thermal safety with increased stator current limit.The steer motor stator current limit has been increased from 30A to 50A (67% increase). While this allows for more powerful steering corrections, it could lead to motor overheating during sustained operation.
Ensure:
- The Falcon 500 motor can handle 50A continuous stator current without exceeding thermal limits
- Monitor motor temperature during extended operation
- Consider whether the increased current is necessary or if mechanical improvements could reduce the required torque
108-108: LGTM, but verify steering response.The steer motor proportional gain has been reduced from 65 to 40 (38% reduction), which will result in less aggressive steering corrections. This may improve stability but could also slow down module alignment.
Test the steering response to ensure:
- Modules still align quickly enough during rapid direction changes
- No noticeable lag in steering response during teleoperated driving
- Autonomous paths execute smoothly without steering-induced oscillation
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (3)
308-308: Harmonize pose prediction usage and verify 200 ms lookahead
- Translation PIDs now use
getPredictedRobotFuturePose(0.2), while rotation and other PID controllers still use the current pose—confirm this inconsistency is intentional.- Verify stability tests cover potential overshoot or oscillations with a 200 ms lookahead and consider renaming
calculateSelfRelativePIDToPoseSpeedsto reflect its use of predicted poses.
308-308: Verify predictive pose control behaviorThe PID controllers now use a 0.2 s constant-velocity pose prediction (getPredictedRobotFuturePose).
- Validate stability (no oscillations or overshoot) when tracking under varying accelerations and rotations
- Confirm the prediction accuracy under typical drive profiles
- Ensure a 0.2 s lookahead suits your loop timing and actuator latency
307-320: Verify 200 ms pose prediction for stabilityThe PID now uses a 0.2 s extrapolated pose (constant‐velocity assumption), which can cause overshoot or oscillation during acceleration/deceleration. Test:
- Sudden direction changes
- Autonomous path-following for oscillations/overshoot
- That 0.2 s matches your control-loop latency
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
31-31: LGTM! Scoring position fine-tuned.The 0.05 meter adjustment to the scoring position X transform is a reasonable calibration refinement.
src/main/java/frc/trigon/robot/constants/CameraConstants.java (5)
42-46: LGTM! Improved naming consistency.The camera rename from
INTAKE_SIDE_REEF_TAG_CAMERAtoFRONT_REEF_TAG_CAMERAimproves clarity and fixes the duplicate "Camera" suffix in the old name string ("IntakeSideReefTagCameraCamera").
23-26: Rename verified
No remaining occurrences of the old camera constant; the rename is consistent.
20-21: Confirm physical mounting of object detection camera matches updated transform (CameraConstants.java:20–21).
- Y moved from +0.195 m to –0.195 m (38 cm lateral shift), Z lowered from 0.7156 m to 0.62 m, roll added at 15°, yaw tweaked to 29.2°.
If the camera wasn’t physically repositioned to these values, object detection will be inaccurate.
28-29: Verify left reef tag camera transform values
In CameraConstants.java (lines 28–29), translation was changed from(-0.129, 0.2032, 0.1258)to(-0.2032, 0.129, 0.1258)and roll flipped from 0° to 180°. Confirm these values accurately reflect the camera’s physical mounting and coordinate‐frame orientation.
32-33: Confirm RIGHT_REEF_TAG_CAMERA transform matches physical mount.
- Translation and rotation now mirror LEFT_REEF_TAG_CAMERA (Translation3d(-0.2032, -0.129, 0.1258), Rotation3d(0, 60 °, 180 °)).
- Verify these values against the actual right-side camera position and orientation on the robot.
src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (2)
25-37: All instantiation sites updated to the new constructor signature
The only usage in RobotContainer.java correctly passes the deletion threshold, DistanceCalculationMethod enum, game-piece type, and camera. No further changes required.
121-137: No additional null checks needed for removal
removeObject(null)simply callsknownObjectPositions.remove(null), which is safe on a standardMapand won’t throw an NPE whengetClosestObjectToPositionreturnsnull.Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (11)
9-9: LGTM!The import is correctly added to access
RobotConstants.CANIVORE_NAME.
22-22: Verify the sensor channel matches the physical wiring.The distance sensor channel has been changed from 4 to 0. Ensure this matches the actual DIO port connection on the robot.
26-26: LGTM!Adding the CANivore name ensures the motor is properly configured on the correct CAN bus.
27-27: Verify the sensor type change aligns with hardware.The sensor has been changed from digital to duty cycle. This is a significant change that enables analog distance readings rather than binary on/off detection. Confirm that:
- The physical sensor connected to DIO 0 is a duty cycle sensor (e.g., an analog distance sensor with PWM output)
- The sensor's duty cycle output range is compatible with the scaling constants defined at lines 51-52
42-42: LGTM!The simulation supplier correctly returns 1 cm (when holding a game piece) or 10 cm (when not holding), which properly triggers the collection detection threshold of 5 cm defined at line 53.
49-53: Verify scaling constants match the sensor's datasheet.The scaling constants (slope: 0.0002, intercept: -200) convert raw duty cycle values to distance in centimeters. These values suggest the sensor's raw output range is approximately 1000-1250 for distances of 0-50 cm. Confirm these constants are calibrated correctly by:
- Consulting the sensor's datasheet for the duty cycle-to-distance mapping
- Testing with known distances to validate the conversion accuracy
54-57: LGTM!The collection detection now correctly uses the scaled distance value with a 5 cm threshold, which is appropriate for the duty cycle sensor. The 0.16-second debounce helps prevent false triggering.
71-71: Verify the motor direction reversal is coordinated with all commands.The motor inversion has been changed, which reverses the direction for all voltage commands. Ensure that:
- The direction change is intentional and correctly oriented for game piece handling
- All
EndEffectorStatevoltage values (lines 93-101) have been adjusted accordingly- Game piece collection, scoring, and ejection all function correctly in the intended directions
87-90: LGTM!The distance sensor configuration correctly applies the scaling constants for the duty cycle sensor and sets up the simulation supplier.
92-108: Verify the voltage adjustments achieve the intended game piece handling.Multiple
EndEffectorStatevoltages have been modified:
SCORE_CORAL: 4V → 1.5V (62.5% reduction)COLLECT_ALGAE: -4V → -11V (175% increase in magnitude)SCORE_ALGAE: 4V → 10V (150% increase)HOLD_CORAL: changed to -0.5VHOLD_ALGAE: -4V → -6V (50% increase in magnitude)These changes, combined with the motor inversion at line 71, significantly alter the end effector's behavior. Test all game piece operations to confirm:
- Coral and algae are collected, held, and scored reliably
- Game pieces are not damaged by excessive voltage
- The voltage levels provide sufficient torque without exceeding safe current limits
70-70: Validate end effector neutral mode change
Ensure changing MotorOutput.NeutralMode from Brake to Coast for the end-effector motor doesn’t allow game pieces to slip when idle, during stops, or on power loss.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java (8)
21-21: LGTM!The
SHOULD_CALIBRATE_ELEVATORflag provides a clean way to switch between arm and elevator SysId characterization modes.
115-117: LGTM!The
resetElevatorPosition()method provides a useful manual calibration capability for the elevator encoder.
213-222: Verify arm safety logic doesn't create deadlock.The safety check at lines 216-219 prevents arm movement when
minimumSafeAngle > max(currentAngle, targetAngle). If the arm is below the safe angle while the elevator is in the safe zone, this could prevent the arm from moving up to escape the constraint, creating a deadlock condition.Verify that the elevator can always adjust first to allow the arm to move to a safe position. Test the sequence:
- Arm at -90° (below safe angle)
- Elevator at minimum safe zone
- Command arm to move to 0° or higher
232-238: LGTM, but verify the updated safe angle calculation.The calculation now uses
kCCW_90deg(subtracted) instead ofkZero(added), and the fallback changed from -90° to -100°. This aligns with the updated angle reference frame, but ensure the safe angle constraints still prevent collisions.Test edge cases where the elevator is at minimum safe zone height to confirm the arm cannot collide with frame components.
241-249: Safe elevator height calculation depends on MAXIMUM_ARM_SAFE_ANGLE.The calculation at line 242 adds 90° to the arm angle before computing cosine. Combined with
MAXIMUM_ARM_SAFE_ANGLEnow being 0° (changed in Constants),isArmAboveSafeAngle()returns true for most arm positions, bypassing the dynamic height calculation and using only the static minimum safe zone. Verify this doesn't compromise safety.
255-257: LGTM!The helper method now mirrors angles around 180° instead of 360°, consistent with the updated state reversal logic throughout the file.
267-277: LGTM!The speed scaling helpers cleanly adjust motion parameters based on each state's
speedScalar, enabling different movement speeds for different operations.
139-141: Add unit tests for reversed‐angle mapping. Verify that subtractFrom180Degrees produces correct mirrored angles (e.g., 0°→180°, 90°→90°, 180°→0°) and that atState(target, true) returns the expected results for reversed states.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
41-45: LGTM!Centralizing the CANivore name in
RobotConstantsimproves maintainability and consistency across the codebase.
49-49: LGTM!The gear ratio change from 3 to 4 reflects a hardware modification. The elevator will be slower (by 25%) but have more torque, and PID gains have been retuned accordingly.
81-82: LGTM!The angle range shift from (0°, 360°) to (-90°, 270°) changes the coordinate system reference, likely to make -90° represent the arm pointing down and 0° horizontal forward.
165-167: LGTM!The breaking event trigger provides good protection against arm motor damage by stopping the motor when stator current exceeds 70A for more than 0.1 seconds.
257-263: LGTM!The elevator PID and feedforward gains have been retuned for the new gear ratio of 4, with increased kP and kD for tighter control.
314-316: LGTM!The encoder direction and discontinuity point changes align with the updated angle coordinate system and 180° mirroring logic.
| if (assistMode.isAlternate) | ||
| return calculateAlternateAssistTranslationPower(assistMode, selfRelativeJoystickPower, xPIDOutput, yPIDOutput); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good refactor to centralize alternate mode logic.
Using the isAlternate field instead of direct enum comparison makes the code more maintainable and extensible. This allows multiple assist modes (like the new ALTERNATE_ALIGN) to share the alternate assist behavior.
Also applies to: 146-146
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java
around lines 99-100 (and similarly at line 146), replace any direct enum
comparisons to check for alternate assist modes with the assistMode.isAlternate
boolean and route through the centralized alternate-assist method; specifically,
when assistMode.isAlternate is true return
calculateAlternateAssistTranslationPower(assistMode, selfRelativeJoystickPower,
xPIDOutput, yPIDOutput) (and apply the same pattern at the other occurrence) so
all alternate-mode logic is handled in one place.
Co-Authored-By: Nummun14 <142012009+Nummun14@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
170-174: Re-enable intake angle soft limits.The TalonFX soft limits are still commented out even though the motion envelope expanded to −127°…0°. This removes the last layer of protection against over-rotation if the absolute position drifts or a command misbehaves. Please restore the limits (thresholds at
MINIMUM_ANGLE/MAXIMUM_ANGLE) or document an equivalent safeguard.+ config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = MINIMUM_ANGLE.getRotations(); + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = MAXIMUM_ANGLE.getRotations();src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (9)
53-54: Document the encoder offset values.The
ANGLE_ENCODER_GRAVITY_OFFSET = -0.0625(≈ -22.5°) and theARM_POSITION_OFFSET_FROM_GRAVITY_OFFSETcalculation still lack inline comments explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSETwas measured (encoder reading at gravity-down position)- Why
-23.56°is subtracted- The arm's intended zero position (horizontal? vertical?)
Add concise inline comments:
+// Encoder reading when arm points straight down (gravity-down calibration position) private static final double ANGLE_ENCODER_GRAVITY_OFFSET = -0.0625; +// Offset to align arm zero position to horizontal (measured offset from gravity-down to horizontal = -23.56°) +// Simulation uses ideal zero; real robot applies physical calibration offset static final double ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET = RobotHardwareStats.isSimulation() ? 0 : edu.wpi.first.math.util.Units.degreesToRotations(-23.56) - ANGLE_ENCODER_GRAVITY_OFFSET;
157-157: Arm safe angle threshold at 0° disables dynamic elevator-height safety.
MAXIMUM_ARM_SAFE_ANGLEremains at 0° (previously flagged as critical). This causesisArmAboveSafeAngle()to return true for virtually all forward arm angles (≥ 0°), which bypasses the dynamic elevator-height safety calculation incalculateMinimumSafeElevatorHeightRotations(). This significantly weakens safety protections. Restore to 90° or provide clear justification and alternative safety measures.
202-202: Document or remove commented velocity filter.The velocity filter configuration is commented out without explanation. Either document why it's disabled (e.g., "TODO: Re-enable after testing shows improved performance") or remove the dead code entirely.
208-212: Re-enable arm software limits.Arm software limits remain commented out (previously flagged as major). Without these limits, the arm can be commanded beyond safe mechanical bounds (-90° to 270°), risking damage or injury. Re-enable the limits using the updated angle range:
-// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = Conversions.degreesToRotations(270); -// -// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = Conversions.degreesToRotations(-270); + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(270) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET; + + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = edu.wpi.first.math.util.Units.degreesToRotations(-90) - ARM_POSITION_OFFSET_FROM_GRAVITY_OFFSET;
268-272: Re-enable elevator software limits.Elevator software limits remain commented out (previously flagged as major). This is more critical than the arm limits because the elevator can physically crash into mechanical stops at high speed, causing significant damage. Re-enable the limits:
-// config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = ELEVATOR_REVERSE_LIMIT_RESET_POSITION_ROTATIONS; -// -// config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; -// config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ReverseSoftLimitEnable = true; + config.SoftwareLimitSwitch.ReverseSoftLimitThreshold = 0; // Minimum position + + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = MAXIMUM_ELEVATOR_HEIGHT_METERS / (Math.PI * DRUM_DIAMETER_METERS) * ELEVATOR_GEAR_RATIO;
53-54: Document the offset calculation.Although a previous review comment was marked as addressed, the magic numbers still lack inline documentation explaining:
- How
ANGLE_ENCODER_GRAVITY_OFFSET = -0.0625was obtained (measurement procedure with arm in gravity-down position)- Why
-23.56°is subtracted in the offset calculation- What the arm's intended zero/reference position is (horizontal, vertical, etc.)
Based on past review comments: This was previously flagged and marked as addressed but the documentation is still missing.
157-157: Critical: Safe angle threshold still disables dynamic safety.
MAXIMUM_ARM_SAFE_ANGLEremains at 0°, which causesisArmAboveSafeAngle()to return true for virtually all arm positions ≥ 0°. This disables the dynamic elevator-height safety calculation incalculateMinimumSafeElevatorHeightRotations(), potentially compromising collision prevention.Based on past review comments: This critical issue was previously flagged but remains unresolved.
202-202: Re-enable software limits and velocity filter.The velocity filter and forward/reverse software limit switches remain commented out without explanation. Software limits are critical safety controls that prevent the arm from exceeding mechanical bounds and potentially causing damage.
Based on past review comments: This major safety issue was previously flagged but remains unresolved. If these controls are intentionally disabled for testing, add TODO comments with justification and re-enable before deployment.
Also applies to: 208-212
268-272: Critical: Re-enable elevator software limits.Elevator software limits remain commented out. This is more critical than the arm limits because the elevator can crash into physical hard stops at both ends, potentially causing mechanism damage or safety hazards.
Based on past review comments: This critical safety issue was previously flagged but remains unresolved. Strongly recommend re-enabling before any testing or deployment.
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
7-7: Remove unused import.The
CoralPlacingCommandsimport is not used anywhere in this file.Apply this diff to remove the unused import:
-import frc.trigon.robot.commands.commandfactories.CoralPlacingCommands;src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (2)
118-127: Simplify redundant cube root calculations.Lines 123-124 recompute
Math.cbrt(joystickValue.getX())andMath.cbrt(joystickValue.getY()), which are already stored inxJoystickPowerandyJoystickPoweron lines 120-121.Apply this diff to simplify:
final double - xPower = assistMode.shouldAssistX ? calculateAlternateAssistPower(xPIDOutput, Math.cbrt(joystickValue.getX()), xJoystickPower) : xJoystickPower, - yPower = assistMode.shouldAssistY ? calculateAlternateAssistPower(yPIDOutput, Math.cbrt(joystickValue.getY()), yJoystickPower) : yJoystickPower; + xPower = assistMode.shouldAssistX ? calculateAlternateAssistPower(xPIDOutput, xJoystickPower, xJoystickPower) : xJoystickPower, + yPower = assistMode.shouldAssistY ? calculateAlternateAssistPower(yPIDOutput, yJoystickPower, yJoystickPower) : yJoystickPower;
175-204: Good addition of ALTERNATE_ALIGN mode and enum structure.The new
ALTERNATE_ALIGNenum constant and the addition of theisAlternatefield properly extend the assist mode functionality. The refactor allows multiple modes to share alternate assist behavior while maintaining clarity through the boolean field.Optional: Consider adding more detailed documentation for
ALTERNATE_ALIGNto clarify that it provides Y and theta assist without X movement, similar to the documentation for other modes.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
289-290: LGTM with note.The rotor velocity signal registration improves telemetry. The commented-out position reset on line 290 should be documented if it's intentionally preserved across restarts, or removed if it's dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java(5 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(10 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/intake/Intake.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/intake/Intake.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(19-109)
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (1)
ClimbCommands(17-54)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (6)
src/main/java/frc/trigon/robot/constants/RobotConstants.java (1)
RobotConstants(7-17)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (3)
src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(18-213)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
🔇 Additional comments (38)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (2)
35-35: LGTM! Good observability addition.Logging the distance sensor value provides valuable runtime visibility into the sensor's behavior, which will help with debugging collection detection issues.
61-61: Verify the ejecting threshold is correctly tuned.The threshold has been lowered from 2V to 1V, which now classifies
SCORE_CORAL(1.5V) as an ejecting action. This change appears intentional and logically correct, as scoring coral is indeed an ejecting operation.Please confirm this threshold aligns with the desired behavior across all EndEffectorState values and any downstream logic that depends on
isEjecting().src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
26-32: Verify PID tuning changes through testing.The PID constraints have been adjusted, which will affect the assist behavior:
- X and THETA: Slightly reduced max velocity (2.8→2.65), increased acceleration (5→5.5)
- Y: Reduced max velocity (2.8→2.65) and significantly reduced acceleration (5→4)
Ensure these changes have been tested and provide the expected assist behavior.
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (9)
8-8: LGTM!The import is necessary for the climbing check added on line 98.
22-22: LGTM!Increasing the exponent from 1 to 2 provides a quadratic response curve, giving the driver finer control at small stick deflections while maintaining responsiveness at larger deflections. This is a common and beneficial tuning adjustment.
67-69: LGTM!The new reset triggers use distinct key combinations (
r+mfor elevator,r+nfor intake) that don't conflict with existing bindings. Good addition for operator control during testing and debugging.
85-85: LGTM!Removing
finalis necessary to allow the climbing check augmentation on line 98. While this sacrifices immutability, the trade-off is acceptable given the need to conditionally extend the trigger logic.
98-99: LGTM!Excellent safety addition. This prevents scoring commands from executing during climb operations, avoiding dangerous subsystem conflicts. The implementation correctly gates both algae and coral scoring paths using the
ClimbCommands.isClimbing()state tracker.
8-8: LGTM!The import is necessary for the climb state check added on line 98.
22-22: LGTM!Increasing the right stick exponent from 1 to 2 provides finer control near the center position by applying a quadratic response curve.
67-69: LGTM!The new reset triggers provide manual position reset capabilities for the elevator and intake subsystems, with distinct key combinations to prevent accidental activation.
84-99: LGTM - Addresses TODO for climb safety.The modification correctly gates score triggers based on climb state, preventing scoring operations during climb sequences. The
finalmodifier was appropriately removed to enable the additional gating logic.Based on learnings: This addresses the TODO comment in ClimbCommands.java (line 16) about making score triggers not work while climbing.
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (26)
13-16: LGTM!Centralizing the CANivore name through
RobotConstants.CANIVORE_NAMEimproves maintainability and ensures consistent CAN bus configuration across all motors and encoders.Also applies to: 41-45
59-63: LGTM - verify tuning through testing.The arm velocity/acceleration parameters changing from 0 to non-zero for the real robot indicates this is initial tuning. The elevator acceleration increasing from 20 to 50 (2.5×) is significant. Ensure these values are validated through testing and don't cause mechanical stress or safety issues.
71-71: Verify maximum elevator height against mechanical limits.The maximum elevator height increased from 1.382m to 1.644m (+26.2cm, +19%). Confirm this new limit is within the mechanism's physical constraints and won't cause the elevator to exceed safe operating range or collide with hard stops.
81-82: LGTM!Expanding the arm angle range to (-90°, 270°) allows the arm to operate below horizontal, which aligns with the new state machine targets (REST, LOAD_CORAL at -90°, EJECT at -30°). The 270° maximum is appropriate for the mechanism's physical constraints.
107-113: LGTM!Reducing system identification voltages (arm: 3V → 1.5V, elevator: 3V → 2V) is a safer approach that minimizes risk of mechanical damage or unexpected behavior during characterization runs.
165-175: LGTM!The breaking-arm detection trigger using a 70A stator current threshold with 0.1s debounce is a solid safety mechanism. Automatically stopping the arm motor when excessive current is detected helps prevent mechanical damage from binding or collisions.
189-189: Verify feedback sensor change from Fused to Remote CANcoder.Changing from
FusedCANcodertoRemoteCANcoderdisables sensor fusion with the motor's internal rotor position.RemoteCANcoderrelies solely on the external absolute encoder. Confirm this change is intentional and doesn't negatively impact control performance, especially during rapid movements where fusion helps with latency compensation.
191-200: LGTM!The PID and feedforward gain updates reflect system retuning. The addition of
StaticFeedforwardSign = UseClosedLoopSignon line 200 is appropriate for an arm mechanism where the direction of gravity compensation depends on the control direction.
225-226: LGTM!Adding rotor velocity and position signal registrations enables additional telemetry for debugging and performance monitoring.
257-267: LGTM!The elevator PID and feedforward gain updates reflect system retuning. The
GravityType = Elevator_Staticconfiguration is appropriate for a vertical elevator mechanism.
289-290: Verify commented-out elevator position reset.Line 290's commented
setPosition(0)might affect elevator initialization. If the elevator doesn't reset to zero at startup, subsequent position commands could be offset. Document why this is commented or re-enable if position reset is needed during initialization.
314-314: LGTM!The sensor direction change to
CounterClockwise_Positivealigns with the updated arm angle conventions and geometry.
316-316: Verify absolute sensor discontinuity point.Changing
AbsoluteSensorDiscontinuityPointfrom its default to 0.5 rotations (180°) affects wraparound behavior. Confirm this aligns with the arm's mechanical zero position and won't cause unexpected jumps at the ±180° boundary, especially given the expanded angle range of -90° to 270°.
326-349: LGTM - state machine overhaul.The comprehensive state machine redesign with new targets is well-structured. Key observations:
- PREPARE states use appropriate intermediate positions
- Elevator heights (including SCORE_L4 at 1.5m and SCORE_NET at 1.644m) are now within the updated maximum of 1.644m, resolving the previous critical issue
- Speed scalars provide appropriate motion profiles for different operations
Ensure all states are tested thoroughly, especially the new algae collection states and the scoring sequences.
49-49: Confirm elevator gear ratio change
The ratio was updated from 3 to 4 (≈33%); ensure this matches the actual hardware update, adjust the elevator simulation (line 97) and all state-machine setpoints, and manually search for any remaining “3” literals to verify no old assumptions persist.
13-16: LGTM!The new imports are necessary for the breaking arm detection trigger (lines 165-176) and CANivore bus configuration.
41-45: LGTM!Centralizing the CANivore bus name through
RobotConstants.CANIVORE_NAMEimproves configuration consistency and maintainability across all arm and elevator motors and encoders.
49-49: Verify the gear ratio update.The elevator gear ratio changed from 3 to 4. Ensure this matches the physical mechanism and that all dependent calculations (position conversions, velocity limits, etc.) are consistent with this change.
59-63: LGTM!The updated velocity and acceleration limits reflect tuning from real robot testing. Note that the elevator acceleration increased significantly (20 → 50), which will result in faster but potentially more aggressive motion profiles.
71-71: LGTM - Operating ranges updated.The expanded elevator height limit (1.644 m) and revised arm angle bounds (-90° to 270°) accommodate the new state machine requirements and resolve previous issues where target positions exceeded limits.
Also applies to: 81-82
165-176: LGTM - Good safety addition.The breaking arm detection trigger provides hardware protection by monitoring stator current and automatically stopping the arm motor when excessive current (>70A for >100ms) is detected.
189-189: Verify sensor source change.The feedback sensor source changed from
FusedCANcoder(sensor fusion) toRemoteCANcoder(external only). Confirm this change aligns with your sensor reliability requirements and doesn't introduce single-point-of-failure risks.
191-197: LGTM!The updated PID and feedforward gains reflect system identification and tuning results. The addition of gravity compensation (
kG = 0.35) is particularly important for stable arm control.
225-226: LGTM!Adding rotor velocity and position signal registration improves observability for debugging and tuning.
257-263: LGTM!The updated elevator PID and feedforward gains reflect system identification results. The gravity compensation (
kG = 0.41015625) is essential for elevator control.
314-314: Verify discontinuity point alignment.The
AbsoluteSensorDiscontinuityPointchanged to 0.5 (wrapping at 180°). Ensure this aligns with the arm's mechanical range (-90° to 270°) and doesn't cause unexpected behavior at the wrap boundary.Also applies to: 316-316
| PREPARE_SCORE_L1(Rotation2d.fromDegrees(-8), 0.5, null, false, 1), | ||
| PREPARE_SCORE_L2(Rotation2d.fromDegrees(60), 0.1, null, false, 1), | ||
| PREPARE_SCORE_L3(Rotation2d.fromDegrees(60), 0.6, null, false, 1), | ||
| PREPARE_SCORE_L4(Rotation2d.fromDegrees(50), 1.5, null, false, 1), | ||
| PREPARE_REST(Rotation2d.fromDegrees(-90), 0.603, null, false, 0.8), | ||
| REST(Rotation2d.fromDegrees(-90), 0.534, PREPARE_REST, true, 0.8), | ||
| REST_AFTER_LOADING(Rotation2d.fromDegrees(-90), 0.603, null, true, 0.7), | ||
| REST_WITH_CORAL(Rotation2d.fromDegrees(90), 0.603, null, false, 0.8), | ||
| REST_WITH_ALGAE(Rotation2d.fromDegrees(90), 0.603, null, false, 0.3), | ||
| REST_FOR_CLIMB(Rotation2d.fromDegrees(90), 0.603, null, false, 0.7), | ||
| LOAD_CORAL(Rotation2d.fromDegrees(-90), 0.5, null, true, 0.7), | ||
| UNLOAD_CORAL(Rotation2d.fromDegrees(-90), 0.603, null, false, 0.7), | ||
| EJECT(Rotation2d.fromDegrees(-30), 0.603, null, false, 0.7), | ||
| SCORE_L1(Rotation2d.fromDegrees(-13), PREPARE_SCORE_L1.targetPositionMeters, null, false, 1), | ||
| SCORE_L2(Rotation2d.fromDegrees(25), PREPARE_SCORE_L2.targetPositionMeters, PREPARE_SCORE_L2, false, 0.8), | ||
| SCORE_L3(Rotation2d.fromDegrees(25), PREPARE_SCORE_L3.targetPositionMeters, PREPARE_SCORE_L3, false, 0.8), | ||
| SCORE_L4(Rotation2d.fromDegrees(-10), 1.5, PREPARE_SCORE_L4, false, 0.8), | ||
| SCORE_NET(Rotation2d.fromDegrees(70), 1.644, null, false, 0.3), | ||
| SCORE_PROCESSOR(Rotation2d.fromDegrees(0), 0.603, null, false, 0.7), | ||
| COLLECT_ALGAE_L2(Rotation2d.fromDegrees(0), 0.603, null, false, 1), | ||
| COLLECT_ALGAE_L3(Rotation2d.fromDegrees(0), 0.953, null, false, 1), | ||
| PREPARE_COLLECT_ALGAE_FLOOR(Rotation2d.fromDegrees(-30), 0.4, null, false, 1), | ||
| COLLECT_ALGAE_FLOOR(Rotation2d.fromDegrees(-30), 0.13, PREPARE_COLLECT_ALGAE_FLOOR, true, 1), | ||
| COLLECT_ALGAE_LOLLIPOP(Rotation2d.fromDegrees(0), 0.15, null, false, 1); |
There was a problem hiding this comment.
Verify SCORE_NET at maximum elevator height.
The state machine has been comprehensively updated. Most states are within limits, but SCORE_NET (line 343) uses 1.644 m, which exactly equals MAXIMUM_ELEVATOR_HEIGHT_METERS. With software limits disabled, this leaves no safety margin. Consider either:
- Reducing
SCORE_NETheight to 1.64 m (leaving a small margin) - Re-enabling software limits with appropriate thresholds
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java
around lines 326 to 349, SCORE_NET currently uses 1.644 m which equals
MAXIMUM_ELEVATOR_HEIGHT_METERS leaving no safety margin when software limits are
disabled; change SCORE_NET target height to a slightly lower value (e.g., 1.64
m) or re-enable software limits with appropriate thresholds so the elevator
never commands the exact max, and update any related constants/comments to
reflect the chosen approach.
| DISTANCE_SENSOR_SCALING_SLOPE = 0.0002, | ||
| DISTANCE_SENSOR_SCALING_INTERCEPT_POINT = -200; | ||
| private static final double COLLECTION_DETECTION_DISTANCE_CENTIMETRES = 35; | ||
| static final BooleanEvent COLLECTION_DETECTION_BOOLEAN_EVENT = new BooleanEvent( | ||
| CommandScheduler.getInstance().getActiveButtonLoop(), | ||
| DISTANCE_SENSOR::getBinaryValue | ||
| () -> DISTANCE_SENSOR.getScaledValue() < COLLECTION_DETECTION_DISTANCE_CENTIMETRES | ||
| ).debounce(COLLECTION_DETECTION_DEBOUNCE_TIME_SECONDS); |
There was a problem hiding this comment.
Fix distance sensor scaling constants.
On real hardware SimpleSensor#getScaledValue() applies raw * 0.0002 - 200. For any plausible raw duty-cycle reading (<1 175 000), the result stays near −200, so Line 110’s < 35 cm condition is always true. That breaks coral detection by permanently reporting “object present”. Re-establish a realistic slope/intercept (or zero-offset) based on measured sensor outputs so the scaled value only drops below 35 cm when a game piece is actually within range.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java around
lines 105–111, the distance sensor scaling constants (slope 0.0002 and intercept
-200) produce scaled values ≈ -200 for any realistic raw reading so the
COLLECTION_DETECTION_BOOLEAN_EVENT (rawScaled < 35 cm) is always true; replace
these magic numbers with a realistic calibration or zero-offset: perform a
two-point calibration from measured raw readings to known distances (or set
intercept = 0 if sensor already outputs distance-like values), compute slope and
intercept from those measurements, update DISTANCE_SENSOR_SCALING_SLOPE and
DISTANCE_SENSOR_SCALING_INTERCEPT_POINT accordingly, and then verify/adjust
COLLECTION_DETECTION_DISTANCE_CENTIMETRES so the boolean only becomes true when
an object is actually within range.
Summary by CodeRabbit
New Features
Improvements
Sensors
Drive