Climb Commands #20
Climb Commands #20
Conversation
WalkthroughAdds a climb trigger and toggle binding; refactors arm subsystem package to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Driver as Driver/Operator
participant OC as OperatorConstants
participant RC as RobotContainer
participant CC as ClimbCommands
participant AE as ArmElevator
participant IN as Intake
Driver->>OC: Press back or C
OC->>RC: CLIMB_TRIGGER active
RC->>CC: toggleOnTrue(getClimbCommand())
note right of CC: Toggle starts/stops climb command
rect rgb(235,245,255)
CC->>AE: setState(REST_FOR_CLIMB)
CC->>IN: setState(REST_FOR_CLIMB)
end
alt Climb running
CC-->>CC: IS_CLIMBING = true
else Toggled off / finished
CC-->>CC: IS_CLIMBING = false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 1
🧹 Nitpick comments (8)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
221-223: Avoid targeting the exact forward soft limit for REST_FOR_CLIMBCommanding MAXIMUM_ANGLE can cause the mechanism to sit on the forward soft/limit and chatter. Use a small margin below max.
- REST_FOR_CLIMB(0, MAXIMUM_ANGLE), + REST_FOR_CLIMB(0, Rotation2d.fromDegrees(108)), // small margin below 110°Optional: extract a named SAFE_MAX_ANGLE constant and reuse across states for consistency.
src/main/java/frc/trigon/robot/RobotContainer.java (1)
15-15: Prefer explicit imports over wildcard for command factoriesKeeps dependencies clear and avoids accidental symbol clashes as packages evolve.
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (2)
177-179: Avoid commanding exactly 0 m for REST_FOR_CLIMBTargeting the hard bottom risks sitting on the reverse limit and mechanical stress. Use the defined safe margin.
- REST_FOR_CLIMB(0, 0, 0.5), + REST_FOR_CLIMB(MINIMUM_ELEVATOR_SAFE_ZONE_METERS, MINIMUM_ELEVATOR_SAFE_ZONE_METERS, 0.5),
184-187: Ground collection at 0 m—consider a small margin above bottomIf ground contact isn’t strictly required, target the safe zone to avoid limit switch contact; otherwise, acknowledge intentional use.
- COLLECT_ALGAE_FROM_GROUND(0, 0, 1), + COLLECT_ALGAE_FROM_GROUND(MINIMUM_ELEVATOR_SAFE_ZONE_METERS, MINIMUM_ELEVATOR_SAFE_ZONE_METERS, 1),src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (4)
22-22: Avoid public mutable static; rename and narrow scope for climb state flag.Public, mutable, ALL_CAPS boolean is misleading (looks like a constant) and leaks global state. Prefer private volatile with an accessor (also easier to gate triggers).
Proposed change:
-public static boolean IS_CLIMBING = false;//TODO: Make score triggers not work while climbing +private static volatile boolean isClimbing = false; // TODO: Gate score triggers while climbingAdd these helpers inside the class (to wire in RobotContainer/OperatorConstants):
public static boolean isClimbing() { return isClimbing; } public static java.util.function.BooleanSupplier isClimbingSupplier() { return () -> isClimbing; } public static java.util.function.BooleanSupplier notClimbingSupplier() { return () -> !isClimbing; }Example gating pattern (outside this file): someTrigger.and(ClimbCommands.notClimbingSupplier()).
Please confirm triggers that must be disabled while climbing are gated accordingly.
24-24: Stale TODO.“Set other component positions” is now implemented via getSetSubsystemsToRestForClimbCommand(); update/remove the TODO.
32-32: Use deadlineWith instead of alongWith for background tasks.LED animation and “rest-for-climb” setters are continuous; alongWith requires them to finish, which isn’t intended. deadlineWith keeps them running in the background and cancels them when the main sequence ends.
- ).alongWith(getSetSubsystemsToRestForClimbCommand(), getClimbLEDCommand()).finallyDo(() -> IS_CLIMBING = false); + ).deadlineWith(getSetSubsystemsToRestForClimbCommand(), getClimbLEDCommand()).finallyDo(() -> isClimbing = false);Note: This also updates the variable name if you apply the refactor suggested on Line 22.
50-56: Non-finishing “rest-for-climb” commands: verify intent and consider prepare-first.Arm/Elevator/Intake “set target state” commands here (per provided snippets) don’t self-finish; they continuously hold targets. That’s fine when used as background tasks (prefer deadlineWith as suggested above). Optionally, consider using the subsystems’ “prepare” commands first if a staged motion is safer.
Based on relevant code snippets (ArmCommands/ElevatorCommands/IntakeCommands).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
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/commands/commandfactories/ClimbCommands.java (1)
ClimbCommands(21-57)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-199)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-233)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
240-241: Verify 120° REST_FOR_CLIMB vs safety zoneMAXIMUM_ARM_SAFE_ANGLE is 90°. REST_FOR_CLIMB at 120° exceeds that. Confirm safety logic permits this during climb and that geometry clears elevator/climber at that angle. If needed, gate safety checks with the climbing state.
src/main/java/frc/trigon/robot/RobotContainer.java (1)
110-111: Climb toggle binding looks good; ensure conflicts are resolvedtoggleOnTrue is appropriate for ClimbCommands.getClimbCommand(). However, due to the trigger overlaps noted in OperatorConstants, this may still fight with whileTrue ejection/collection commands requiring the same subsystems.
Optional: gate scoring/ejection while climbing to satisfy the TODO in ClimbCommands. For example:
- EJECT_CORAL_TRIGGER.and(() -> !ClimbCommands.IS_CLIMBING)
- SCORE_CORAL_*_TRIGGER.and(() -> !ClimbCommands.IS_CLIMBING)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (3)
10-17: Imports look correct and necessary.New Arm/Elevator/Intake command/constant imports are used below. No issues.
26-31: Confirm cancellation semantics for the final manual-adjust step.The last step (getAdjustClimbManuallyCommand) doesn’t end on its own, making the whole sequence non-terminating by design. Ensure this command is bound with whileTrue/whileHeld (or otherwise cancelable), not toggleOnTrue.
35-37: LED animation hook LGTM.Animation selection via constants is clean and keeps policy out of the factory.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (2)
22-33: Add timeouts to PREPARE/CLIMB stages to avoid indefinite hangs if sensors misbehaveA stuck hasCage()/atTargetState() will stall the sequence; manual adjust is never reached unless canceled. Add conservative timeouts.
Proposed change:
- ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.PREPARE_FOR_CLIMB) - .until(() -> RobotContainer.CLIMBER.hasCage() || OperatorConstants.CONTINUE_TRIGGER.getAsBoolean()), - ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.CLIMB) - .until(RobotContainer.CLIMBER::atTargetState), + ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.PREPARE_FOR_CLIMB) + .until(() -> RobotContainer.CLIMBER.hasCage() || OperatorConstants.CONTINUE_TRIGGER.getAsBoolean()) + .withTimeout(3.0), + ClimberCommands.getSetTargetStateCommand(ClimberConstants.ClimberState.CLIMB) + .until(RobotContainer.CLIMBER::atTargetState) + .withTimeout(8.0),Verify behavior on-rig with and without cage detection to confirm the fallbacks are acceptable.
35-36: Accessor looks goodPublic isClimbing() pairs well with trigger gating and telemetry.
Consider pushing this as a wpilib Trigger: new Trigger(ClimbCommands::isClimbing) to compose directly in bindings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/LEDConstants.java(0 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/frc/trigon/robot/constants/LEDConstants.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (7)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-199)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-233)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-124)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
59-59: Simplified confirmation command is clean.Dropping the LED parallel command and using an InstantCommand to trigger controller rumble keeps the operator feedback while removing the extra command plumbing.
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (2)
9-16: LGTM on new subsystem importsImports align with new REST_FOR_CLIMB usage.
50-56: Safety: use prepare-then-target for Arm/Elevator; Intake lacks prepareIntakeCommands has no
getPrepare…API—ensure direct set toREST_FOR_CLIMBis safe or implement a prepare step.
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java
Show resolved
Hide resolved
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 (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
33-37: Add a timeout to avoid a hang if the arm never reaches EJECT.
If sensors/constraints prevent reaching target, the sequence blocks and end‑effector never ejects. Add a safety timeout.Apply this diff:
- ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.EJECT).until(RobotContainer.ARM_ELEVATOR::atTargetState), + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.EJECT) + .until(RobotContainer.ARM_ELEVATOR::atTargetState) + .withTimeout(1.5),src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
39-41: Don’t schedule child commands from inside a group; compose them.
SchedulinggetAlignCoralCommand()can outlive the parent, causing lifecycle/requirements conflicts. Compose it so the scheduler manages ownership.Apply this diff:
- new InstantCommand(() -> getAlignCoralCommand().schedule()).alongWith(getCollectionConfirmationCommand()) + getAlignCoralCommand().alongWith(getCollectionConfirmationCommand())
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
30-35: Remove redundant termination condition.
.until(hasGamePiece)and.onlyWhile(!hasGamePiece)duplicate each other. Keep one.Apply this diff:
- ).until(RobotContainer.END_EFFECTOR::hasGamePiece).onlyWhile(() -> !RobotContainer.END_EFFECTOR.hasGamePiece()); + ).until(RobotContainer.END_EFFECTOR::hasGamePiece);src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (1)
33-35: Gate SCORE_CORAL triggers to prevent scoring while climbing
In RobotContainer.java (lines 107–108), chain.and(() -> !ClimbCommands.isClimbing())before.whileTrueon both SCORE_CORAL triggers:- OperatorConstants.SCORE_CORAL_LEFT_TRIGGER.whileTrue(CoralPlacingCommands.getScoreInReefCommand(false)); + OperatorConstants.SCORE_CORAL_LEFT_TRIGGER + .and(() -> !ClimbCommands.isClimbing()) + .whileTrue(CoralPlacingCommands.getScoreInReefCommand(false)); - OperatorConstants.SCORE_CORAL_RIGHT_TRIGGER.whileTrue(CoralPlacingCommands.getScoreInReefCommand(true)); + OperatorConstants.SCORE_CORAL_RIGHT_TRIGGER + .and(() -> !ClimbCommands.isClimbing()) + .whileTrue(CoralPlacingCommands.getScoreInReefCommand(true));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java(2 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/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java
- src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/RobotContainer.java
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (3)
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)
ArmElevatorConstants(28-365)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
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)
ArmElevatorConstants(28-365)
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (7)
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)
ArmElevatorConstants(28-365)src/main/java/frc/trigon/robot/subsystems/climber/ClimberCommands.java (1)
ClimberCommands(12-46)src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java (1)
ClimberConstants(26-168)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-233)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-67)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (2)
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)
ArmElevatorConstants(28-365)
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-365)
🔇 Additional comments (7)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
8-9: Import migration to armelevator looks correct.
No functional change; aligns with package move.src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
9-10: Import migration to armelevator looks good.
58-60: Rumble-only confirmation is fine.
Keeps user feedback without LED coupling.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (2)
1-1: Package move to armelevator acknowledged.
337-337: REST_FOR_CLIMB at 180° — revalidate safety envelope.
At 180° the arm is beyond MAXIMUM_ARM_SAFE_ANGLE (90°). Confirm elevator height (0.603 m) satisfies your safety logic and there’s no mechanical interference.Would you like a quick checklist for on‑robot validation?
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java (2)
29-31: Good: flag reset on finish/interruption.
finallyDoensuresIS_CLIMBINGis cleared in all exits.
18-18: Make IS_CLIMBING volatile (or AtomicBoolean) for cross-thread visibility.
Prevents stale reads in triggers/telemetry; matches prior feedback.Apply this diff:
- private static boolean IS_CLIMBING = false;//TODO: Make score triggers not work while climbing + private static volatile boolean IS_CLIMBING = false; // TODO: Make score triggers not work while climbing
src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Behavior Changes
Refactor