Coral Placing Commands #15
Conversation
…ng-TRIGON/RobotCodeOffSeason2025 into coral-scoring-commands
|
@CodeRabbit full reveiw |
|
@levyishai I'll conduct a full review of the PR changes for you. ✅ Actions performedFull review triggered. |
WalkthroughAdds reef-scoring workflow: new CoralPlacingCommands and ReefChooser, reef enums/poses/transforms in FieldConstants, operator reef triggers, prepare-state APIs and supplier-based command overloads for arm/elevator, load/eject command updates, autonomous path/planning constants, and wiring across commands/subsystems for autonomous vs manual scoring flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator
participant ReefChooser
participant CoralPlacing as CoralPlacingCommands
participant Drive as Swerve
participant Elevator
participant Arm
participant Gripper
Operator->>ReefChooser: set level / clock / side (triggers/HID)
Operator->>CoralPlacing: request scoring command
CoralPlacing->>ReefChooser: getScoringLevel(), calculateTargetScoringPose()
alt SHOULD_SCORE_AUTONOMOUSLY == true
CoralPlacing->>Drive: driveTo(targetPose)
par prepare mechanisms
CoralPlacing->>Elevator: prepareState(...) / setTargetState(...)
CoralPlacing->>Arm: setPrepareState(...) / setTargetState(...)
and autonomous release
CoralPlacing->>Gripper: release when conditions met
end
else Manual path
CoralPlacing->>Elevator: setTargetState(...)
CoralPlacing->>Arm: setTargetState(...)
Operator->>Gripper: manual release
end
CoralPlacing-->>Operator: complete
sequenceDiagram
autonumber
participant System
participant ReefChooser
participant HID
System->>ReefChooser: new ReefChooser(port)
ReefChooser-->>ReefChooser: wait 3s -> configureBindings()
ReefChooser->>HID: bind HID buttons (clock positions)
ReefChooser->>System: expose triggers for levels/side
HID-->>ReefChooser: update internal state on input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 11
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/elevator/Elevator.java (1)
49-55: Unit mismatch in SysId logging (rotations reported as meters).linearPosition and linearVelocity should be in meters/ m·s⁻¹. You’re logging rotations.
Apply:
- log.motor("Elevator") - .linearPosition(Units.Meters.of(getPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(masterMotor.getSignal(TalonFXSignal.VELOCITY))) + log.motor("Elevator") + .linearPosition(Units.Meters.of(getPositionMeters())) + .linearVelocity(Units.MetersPerSecond.of( + rotationsToMeters(masterMotor.getSignal(TalonFXSignal.VELOCITY)) + ))
🧹 Nitpick comments (9)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
86-86: Consider exposing or centralizing elevator tolerance.0.01 m may be tighter than field reality; if this drives gating logic elsewhere, consider making it configurable or at least record it to logs for tuning.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
214-221: Guard L1 transforms.L1 uses NaN transforms; ensure calculateTargetPlacingPosition is never called for L1 or throw with a clear message to avoid propagating NaNs.
public FlippablePose2d calculateTargetPlacingPosition(FieldConstants.ReefClockPosition reefClockPosition, FieldConstants.ReefSide reefSide) { + if (this == L1) { + throw new IllegalStateException("L1 placing pose is not defined; this method should not be called for L1."); + }
40-47: Race/Repeat semantics: verify WaitUntilChangeCommand usage.The repeated race between setting elevator state and waiting for chooser changes can thrash if chooser toggles quickly. Consider debouncing chooser updates or gating on equality with current target before repeating.
128-136: Distance calculation: OK but consider caching pose.You call calculateClosestScoringPose twice in close succession in this class; consider caching per cycle in RobotContainer if used frequently.
src/main/java/frc/trigon/robot/misc/ReefChooser.java (3)
3-5: Replace magic 3s delay with HID-connected gate (avoid early scheduling from static init).Scheduling from the constructor with a fixed
WaitCommand(3)is brittle and may run before devices are available or the scheduler is fully ready. Gate on the HID actually being connected instead.Apply:
-import edu.wpi.first.wpilibj2.command.WaitCommand; +import edu.wpi.first.wpilibj2.command.WaitUntilCommand; +import edu.wpi.first.wpilibj.DriverStation; @@ - new WaitCommand(3).andThen(this::configureBindings).ignoringDisable(true).schedule(); + new WaitUntilCommand(() -> DriverStation.isJoystickConnected(port)) + .andThen(this::configureBindings) + .ignoringDisable(true) + .schedule();Verify on both sim and real hardware that bindings appear immediately when the device enumerates (no arbitrary delay).
Also applies to: 25-26
86-89: Harden face-index mapping against future misuse.This relies on enum order and assumes 0–11. Add a guard so accidental out-of-range calls fail fast.
- private void setFaceFromIndex(int index) { + private void setFaceFromIndex(int index) { + final int faces = FieldConstants.REEF_CLOCK_POSITIONS * FieldConstants.ReefSide.values().length; + if (index < 0 || index >= faces) + throw new IllegalArgumentException("Invalid reef face index: " + index); clockPosition = FieldConstants.ReefClockPosition.values()[index / 2]; reefSide = FieldConstants.ReefSide.values()[index % 2]; }
115-129: Remove or wire the unused level increment/decrement helpers.Dead code adds maintenance overhead. Either bind them to buttons or drop them for now.
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
20-22: RenameI_HATE_YOUand use a Set for clarity/perf.Professional naming helps maintenance; a
Setcommunicates intent and gives O(1) membership tests.- private static final List<Integer> I_HATE_YOU = List.of( - 13, 12, 16, 15, 14, 4, 5, 3, 2, 1 - ); + private static final java.util.Set<Integer> IGNORED_TAG_IDS = java.util.Set.of( + 13, 12, 16, 15, 14, 4, 5, 3, 2, 1 + ); @@ - if (!I_HATE_YOU.contains(aprilTag.ID)) + if (!IGNORED_TAG_IDS.contains(aprilTag.ID)) tagIdToPose.put(aprilTag.ID, aprilTag.pose.transformBy(TAG_OFFSET));Also applies to: 61-63
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
15-18: Be cautious with static initialization order forREEF_CHOOSER.Since
ReefChooserschedules work in its constructor, creating it here can run before the scheduler/devices are ready. If you adopt theWaitUntilCommandgate suggested in ReefChooser, this is fine; otherwise, consider lazy-init or invoking binding from robot/container init.Also applies to: 25-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(3 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 26-26: constructor ConditionalCommand in class ConditionalCommand cannot be applied to given types; required: Command,Command,BooleanSupplier; found: ProxyCommand,()->...
[error] 41-41: cannot find symbol: method getLoadCoralCommand() in CoralCollectionCommands
[error] 43-43: no suitable method found for getSetTargetStateCommand(REEF_CHOOSER::getElevatorState)
[error] 51-51: cannot find symbol: method getLoadCoralCommand() in CoralCollectionCommands
[error] 63-63: no suitable method found for ArmCommands.getSetTargetStateCommand()
[error] 64-64: cannot find symbol: method atState(ElevatorState) on ELEVATOR
[error] 65-65: invalid method reference: Elevator::isCloseEnoughToOpenGripper not found
[error] 72-72: package GripperConstants does not exist
[error] 73-73: cannot find symbol: GripperCommands
[error] 74-74: cannot find symbol: method atState(ElevatorState) on ELEVATOR
[error] 85-85: cannot find symbol: GripperCommands.getPrepareForStateCommand(REEF_CHOOSER::getGripperState)
[error] 91-91: cannot find symbol: GripperCommands.getSetTargetStateCommand(REEF_CHOOSER::getGripperState)
[error] 97-97: cannot find symbol: GripperCommands.getPrepareForStateCommand(REEF_CHOOSER::getGripperState).until(OperatorConstants.CONTINUE_TRIGGER)
[error] 98-98: cannot find symbol: GripperCommands.getSetTargetStateCommand(REEF_CHOOSER::getGripperState)
[error] 104-104: cannot find symbol: GripperCommands.getPrepareForScoringInL4Command(REEF_CHOOSER::calculateTargetScoringPose)
[error] 107-107: cannot find symbol: GripperCommands.getScoreInL4Command(REEF_CHOOSER::calculateTargetScoringPose)
[error] 113-113: no suitable method found for ElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER::getElevatorState)
[error] 114-114: cannot find symbol: shouldScoreRight
[error] 122-122: package AutonomousConstants does not exist
🔇 Additional comments (11)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
113-120: Hardware gains are zero on real hardware — confirm intent or provide safe defaultskP/kI/kD/kS/kV/kA/kG are set to 0 when RobotHardwareStats.isSimulation() is false; MotionMagic will not regulate with these values.
- Add an inline comment if zeros are intentional.
- Otherwise initialize safe starter gains or load Slot0 gains from WPILib Preferences.
File: src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (lines 113–120)
config.Slot0.kP = RobotHardwareStats.isSimulation() ? 3.5 : 0; config.Slot0.kI = RobotHardwareStats.isSimulation() ? 0 : 0; config.Slot0.kD = RobotHardwareStats.isSimulation() ? 0.4 : 0; config.Slot0.kS = RobotHardwareStats.isSimulation() ? 0.016165 : 0; config.Slot0.kV = RobotHardwareStats.isSimulation() ? 0.4766 : 0; config.Slot0.kA = RobotHardwareStats.isSimulation() ? 0.014239 : 0; config.Slot0.kG = RobotHardwareStats.isSimulation() ? 0.58202 : 0;Need a template to load Slot0 gains from WPILib Preferences?
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
79-82: atTargetState() looks good.Simple, readable tolerance check using constants.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
111-116: Reference to AutonomousConstants without import and potential capture issue.Add the import (see earlier diff). Also ensure shouldScoreRight is effectively final (method param is fine).
95-101: .until(OperatorConstants.CONTINUE_TRIGGER) compatibility.If CONTINUE_TRIGGER isn’t a BooleanSupplier, use
.until(OperatorConstants.CONTINUE_TRIGGER::getAsBoolean).
139-156: Distance-minimization loop: looks good.Nearest clock-angle scoring pose selection is clean and efficient.
118-126: Signature verified — confirm constraint constant typeSwerveCommands.getDriveToPoseCommand(Supplier, PathConstraints) exists so the Supplier you pass is correct; confirm AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS is a PathConstraints instance (and that calculateClosestScoringPose returns FlippablePose2d).
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
44-54: API surface looks solid.Pose, arm, and elevator state accessors are cohesive and side-effect free.
src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
84-90: AllReefClockPositionentries setisFacingDriverStation = true— likely incorrect.The flip logic in
ReefSide.shouldFlipYTransform()depends on this flag. With alltrue, flipping degenerates to side-only behavior and may produce wrong Y transforms on half the reef.Proposed mapping (please verify against field geometry):
- REEF_12_OCLOCK(true), - REEF_2_OCLOCK(true), - REEF_4_OCLOCK(true), - REEF_6_OCLOCK(true), - REEF_8_OCLOCK(true), - REEF_10_OCLOCK(true); + REEF_12_OCLOCK(true), + REEF_2_OCLOCK(true), + REEF_4_OCLOCK(true), + REEF_6_OCLOCK(false), + REEF_8_OCLOCK(false), + REEF_10_OCLOCK(false);If orientation differs by alliance or mirroring, consider deriving this from pose/angle instead of hard-coding.
Also applies to: 92-99, 78-81
29-36: ConfirmRotation2d.unaryMinus()availability in your WPILib version.If you target an older release, replace with
Rotation2d.fromRadians(-AUTO_FIND_CORAL_POSE_LEFT_ROTATION.getRadians()).
39-47: Clock-angle math is clean and self-consistent.Using
REEF_CLOCK_POSITION_DIFFERENCE.times(-ordinal())and exposing a precomputedREEF_CLOCK_ANGLESarray is tidy.Also applies to: 101-111
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
47-59: Trigger mapping looks good and covers the intended UX.Direct binds for L1–L4, clock positions, and side selection are clear and discoverable.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (9)
3-19: Missing imports for referenced classes.The file references several classes that are not imported, which will cause compilation failures.
Add the missing import statements:
import edu.wpi.first.wpilibj2.command.*; +import frc.trigon.robot.commands.commandclasses.GeneralCommands; import frc.trigon.robot.RobotContainer; import frc.trigon.robot.commands.commandclasses.WaitUntilChangeCommand; import frc.trigon.robot.constants.FieldConstants; import frc.trigon.robot.constants.OperatorConstants; +import frc.trigon.robot.constants.AutonomousConstants; import frc.trigon.robot.misc.ReefChooser; import frc.trigon.robot.subsystems.arm.ArmCommands; import frc.trigon.robot.subsystems.arm.ArmConstants; import frc.trigon.robot.subsystems.elevator.ElevatorCommands; import frc.trigon.robot.subsystems.elevator.ElevatorConstants; +import frc.trigon.robot.subsystems.swerve.SwerveCommands; +import frc.trigon.robot.commands.commandfactories.CoralCollectionCommands;
24-30: Fix ConditionalCommand constructor parameter order.The
getContinuousConditionalCommandmethod is likely expecting the same parameter order asConditionalCommand, but the condition logic appears to be incorrect for the intended behavior.Based on the naming and logic, this should run autonomous when the condition is true. Apply this fix:
- return GeneralCommands.getContinuousConditionalCommand( - getAutonomouslyScoreInReefFromGripperCommand(shouldScoreRight).asProxy(), - getManuallyScoreInReefFromGripperCommand().asProxy(), - () -> SHOULD_SCORE_AUTONOMOUSLY && REEF_CHOOSER.getScoringLevel() != ScoringLevel.L1 - ).until(() -> REEF_CHOOSER.getScoringLevel() == ScoringLevel.L1); + return GeneralCommands.getContinuousConditionalCommand( + getAutonomouslyScoreInReefFromGripperCommand(shouldScoreRight).asProxy(), + getManuallyScoreInReefFromGripperCommand().asProxy(), + () -> SHOULD_SCORE_AUTONOMOUSLY && REEF_CHOOSER.getScoringLevel() != ScoringLevel.L1 + ).until(() -> REEF_CHOOSER.getScoringLevel() == ScoringLevel.L1);
32-39: Fix method call and parameter type mismatch.
CoralCollectionCommands.getLoadCoralCommand()doesn't exist, andElevatorCommands.getSetTargetStateCommand()expects a value, not a method reference.Apply this fix:
- return CoralCollectionCommands.getLoadCoralCommand().asProxy().andThen( + return CoralCollectionCommands.getCoralCollectionCommand().asProxy().andThen( new ParallelCommandGroup( - ElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER::getElevatorState).raceWith(new WaitUntilChangeCommand<>(REEF_CHOOSER::getElevatorState)).repeatedly(), + ElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER.getElevatorState()).raceWith(new WaitUntilChangeCommand<>(REEF_CHOOSER::getElevatorState)).repeatedly(), getArmScoringSequenceCommand() ).asProxy() );
41-51: Fix method call and remove trailing andThen().Same issues as the manual command, plus a stray
.andThen()with no arguments.Apply this fix:
- CoralCollectionCommands.getLoadCoralCommand().asProxy().andThen( + CoralCollectionCommands.getCoralCollectionCommand().asProxy().andThen( new ParallelCommandGroup( getOpenElevatorWhenCloseToReefCommand(shouldScoreRight), getAutoGripperScoringSequenceCommand(shouldScoreRight) ).asProxy().raceWith(new WaitUntilChangeCommand<>(REEF_CHOOSER::getElevatorState)).repeatedly() ), getAutonomousDriveToReefThenManualDriveCommand(shouldScoreRight) - ).andThen(); + );
74-100: Fix references to non-existent GripperCommands class.These methods reference
GripperCommandswhich doesn't exist in the codebase.These methods need to be updated to use existing APIs or marked as TODOs until the
GripperCommandsclass is implemented:private static Command scoreFromGripperReefChooserCommand(boolean shouldScoreRight) { + // TODO: Implement when GripperCommands class is available + return Commands.none(); + /* return new SequentialCommandGroup( - GripperCommands.getPrepareForStateCommand(REEF_CHOOSER::getGripperState).raceWith( + // GripperCommands.getPrepareForStateCommand(REEF_CHOOSER::getGripperState).raceWith( new SequentialCommandGroup( new WaitUntilCommand(() -> canAutonomouslyReleaseFromGripper(shouldScoreRight)), new WaitUntilChangeCommand<>(RobotContainer.ROBOT_POSE_ESTIMATOR::getEstimatedRobotPose) - ) - ).until(OperatorConstants.CONTINUE_TRIGGER), - GripperCommands.getSetTargetStateCommand(REEF_CHOOSER::getGripperState).finallyDo(OperatorConstants.REEF_CHOOSER::switchReefSide) + // ) + // ).until(OperatorConstants.CONTINUE_TRIGGER), + // GripperCommands.getSetTargetStateCommand(REEF_CHOOSER::getGripperState).finallyDo(OperatorConstants.REEF_CHOOSER::switchReefSide) ); + */ }Apply similar fixes to the other methods that reference
GripperCommands.
102-107: Fix parameter type mismatch and missing AutonomousConstants.The method expects a value, not a method reference, and
AutonomousConstantsis missing.Apply this fix:
return GeneralCommands.runWhen( - ElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER::getElevatorState), + ElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER.getElevatorState()), () -> calculateDistanceToTargetScoringPose(shouldScoreRight) < AutonomousConstants.MINIMUM_DISTANCE_FROM_REEF_TO_OPEN_ELEVATOR );
213-233: Use explicit enum mapping instead of ordinal-based logic.The current implementation uses ordinal-based mapping which is brittle and error-prone. The mapping appears to be off by one.
Replace with explicit enum mapping:
private ElevatorConstants.ElevatorState determineElevatorState() { - return switch (ordinal()) { - case 0 -> ElevatorConstants.ElevatorState.REST; - case 1 -> ElevatorConstants.ElevatorState.SCORE_L1; - case 2 -> ElevatorConstants.ElevatorState.SCORE_L2; - case 3 -> ElevatorConstants.ElevatorState.SCORE_L3; - case 4 -> ElevatorConstants.ElevatorState.SCORE_L4; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; + return switch (this) { + case L1 -> ElevatorConstants.ElevatorState.SCORE_L1; + case L2 -> ElevatorConstants.ElevatorState.SCORE_L2; + case L3 -> ElevatorConstants.ElevatorState.SCORE_L3; + case L4 -> ElevatorConstants.ElevatorState.SCORE_L4; + }; } private ArmConstants.ArmState determineArmState() { - return switch (ordinal()) { - case 0 -> null; - case 1 -> ArmConstants.ArmState.SCORE_L1; - case 2 -> ArmConstants.ArmState.SCORE_L2; - case 3 -> ArmConstants.ArmState.SCORE_L3; - case 4 -> ArmConstants.ArmState.SCORE_L4; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; + return switch (this) { + case L1 -> ArmConstants.ArmState.SCORE_L1; + case L2 -> ArmConstants.ArmState.SCORE_L2; + case L3 -> ArmConstants.ArmState.SCORE_L3; + case L4 -> ArmConstants.ArmState.SCORE_L4; + }; }
235-239: Fix incorrect level calculation logic.The current logic incorrectly maps L1 (ordinal 0) to level 1, but other levels are off by one.
Apply this fix:
private int calculateLevel() { - if (ordinal() == 0) - return 1; - return ordinal(); + return ordinal() + 1; }
53-59: Fix compilation errors in arm scoring sequence.Multiple syntax and API issues need to be addressed.
Based on the pipeline errors and other similar patterns, apply this fix:
return new SequentialCommandGroup( - ArmCommands.getSetTargetStateCommand() - .unless(() -> RobotContainer.ELEVATOR.atState(REEF_CHOOSER.getElevatorState()) || REEF_CHOOSER.getScoringLevel() == ScoringLevel.L2 || REEF_CHOOSER.getScoringLevel() == ScoringLevel.L1) + ArmCommands.getSetTargetStateCommand(REEF_CHOOSER.getArmState()) + .unless(() -> RobotContainer.ELEVATOR.atTargetState() + || REEF_CHOOSER.getScoringLevel() == ScoringLevel.L2 + || REEF_CHOOSER.getScoringLevel() == ScoringLevel.L1), scoreFromGripperReefChooserCommand() );
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
109-117: Consider uncommenting autonomous drive command.The autonomous drive functionality is commented out, which may be intentional for testing but should be addressed.
If this functionality is needed, uncomment and fix the method:
-// private static Command getAutonomousDriveToReefThenManualDriveCommand(boolean shouldScoreRight) { -// return new SequentialCommandGroup( -// SwerveCommands.getDriveToPoseCommand( -// () -> CoralPlacingCommands.calculateClosestScoringPose(shouldScoreRight), -// AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS -// ), -// GeneralCommands.getFieldRelativeDriveCommand() -// ); -// } + private static Command getAutonomousDriveToReefThenManualDriveCommand(boolean shouldScoreRight) { + // TODO: Implement when SwerveCommands.getDriveToPoseCommand is available + return GeneralCommands.getFieldRelativeDriveCommand(); + /* + return new SequentialCommandGroup( + SwerveCommands.getDriveToPoseCommand( + () -> CoralPlacingCommands.calculateClosestScoringPose(shouldScoreRight), + AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS + ), + GeneralCommands.getFieldRelativeDriveCommand() + ); + */ + }
📜 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(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T09:14:50.990Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:50.990Z
Learning: The Swerve subsystem's atPose method is designed to accept FlippablePose2d directly, not Pose2d. The method signature is `public boolean atPose(FlippablePose2d pose2d)` and it handles the internal conversion to Pose2d by calling pose2d.get().
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-11T09:14:50.990Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:50.990Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (13)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(45-119)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(16-113)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-60)src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
ReefChooser(16-130)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(12-50)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-255)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(10-35)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(27-188)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(15-65)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(17-52)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 56-56: gradlew build: ')' expected at CoralPlacingCommands.java:56 (likely missing semicolon after chained '.unless(...)' call).
[error] 57-57: gradlew build: ';' expected at CoralPlacingCommands.java:57 (likely due to the previous line missing a semicolon).
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
153-157: LGTM - atPose method accepts FlippablePose2d directly.Based on the retrieved learnings, the Swerve subsystem's
atPosemethod acceptsFlippablePose2ddirectly and handles the internal conversion.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Outdated
Show resolved
Hide resolved
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/subsystems/elevator/Elevator.java (1)
51-53: Fix SysId logging units (rotations ≠ meters).You're reporting position/velocity in meters but feeding rotations/rotations-per-second. This corrupts SysId data and derived gains.
Apply:
- .linearPosition(Units.Meters.of(getPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(masterMotor.getSignal(TalonFXSignal.VELOCITY))) + .linearPosition(Units.Meters.of(getPositionMeters())) + .linearVelocity(Units.MetersPerSecond.of(rotationsToMeters(masterMotor.getSignal(TalonFXSignal.VELOCITY))))
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (4)
39-46: Also apply brake/stop to follower motor.Keeps both motors’ neutral/drive states consistent; avoids asymmetric drag.
public void setBrake(boolean brake) { - masterMotor.setBrake(brake); + masterMotor.setBrake(brake); + ElevatorConstants.FOLLOWER_MOTOR.setBrake(brake); } ... public void stop() { - masterMotor.stopMotor(); + masterMotor.stopMotor(); + ElevatorConstants.FOLLOWER_MOTOR.stopMotor(); }
78-85: LGTM; minor API polish and tolerance edge-case.Consider param rename to avoid shadowing and allow equality in tolerance.
- public boolean atState(ElevatorConstants.ElevatorState targetState) { - return targetState == this.targetState && atTargetState(); + public boolean atState(ElevatorConstants.ElevatorState expectedState) { + return expectedState == this.targetState && atTargetState(); } ... - return currentToTargetStateDifferenceMeters < ElevatorConstants.HEIGHT_TOLERANCE_METERS; + return currentToTargetStateDifferenceMeters <= ElevatorConstants.HEIGHT_TOLERANCE_METERS;
111-113: Use >= for stage limit transition.Prevents a 1-step “deadband” exactly at the limit.
- private boolean isSecondStageComponentLimitReached() { - return getPositionMeters() > ElevatorConstants.SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS; - } + private boolean isSecondStageComponentLimitReached() { + return getPositionMeters() >= ElevatorConstants.SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS; + }
123-127: Clamp speed scalar and confirm dynamic-request mutation semantics.Avoid accidental overspeed; Phoenix 6 allows mutating DynamicMotionMagicVoltage fields, but confirm your version. If uncertain, rebuild the request with withVelocity/withAcceleration.
- private void scalePositionRequestSpeed(double speedScalar) { - positionRequest.Velocity = ElevatorConstants.DEFAULT_MAXIMUM_VELOCITY * speedScalar; - positionRequest.Acceleration = ElevatorConstants.DEFAULT_MAXIMUM_ACCELERATION * speedScalar; + private void scalePositionRequestSpeed(double speedScalar) { + final double clamped = Math.max(0.0, Math.min(1.0, speedScalar)); + positionRequest.Velocity = ElevatorConstants.DEFAULT_MAXIMUM_VELOCITY * clamped; + positionRequest.Acceleration = ElevatorConstants.DEFAULT_MAXIMUM_ACCELERATION * clamped; positionRequest.Jerk = positionRequest.Acceleration * 10; }Add (outside this hunk) if you prefer a helper:
edu.wpi.first.math.MathUtil.clamp.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(27-188)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
- src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-263)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(13-61)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(12-37)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-199)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(45-119)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
16-16: LGTM: AutoLogOutput import added correctlyNo issues.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
11-14: LGTM: required imports addedNo concerns.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
237-237: Confirm LOAD_CORAL hardware polarity and enum-ordering safety
- Verify that applying –4 V for LOAD_CORAL aligns with your intake motor’s polarity so
hasGamePiecereflects actual intake both in simulation and on the robot.- Ensure no code relies on
ArmState.ordinal(),ArmState.values()[…], or any array/List indexing based on the enum’s declaration order.
| public static Command getLoadCoralCommand() { | ||
| return new ParallelCommandGroup( | ||
| ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.LOAD_CORAL), | ||
| ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL) | ||
| ).until(RobotContainer.ARM::hasGamePiece); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Cancellation stops rollers; likely to drop coral on detection
ArmCommands.getSetTargetStateCommand(...) calls Arm.stop() on end; Arm.stop() halts both the arm and end-effector motors (see Arm.java Lines 49–52). When .until(ARM::hasGamePiece) triggers, the rollers stop immediately, risking loss of the newly acquired coral. Also, there’s no timeout if the sensor never trips.
Apply a minimal safety guard now (timeout):
- ).until(RobotContainer.ARM::hasGamePiece);
+ )
+ .until(RobotContainer.ARM::hasGamePiece)
+ .withTimeout(2.5);Recommended follow-ups (requires small helpers in ArmCommands):
- Keep rollers at a gentle hold after detection instead of stopping.
- Split “arm positioning” from “roller control” so end behavior doesn’t cut power to rollers.
Example helper additions (in ArmCommands.java):
// New: keep only the arm positioning running; do not stop rollers on end.
public static Command getSetArmStateCommand(ArmConstants.ArmState targetState, boolean reversed) {
return new FunctionalCommand(
() -> {}, // init handled elsewhere
() -> RobotContainer.ARM.setArmState(targetState, reversed),
interrupted -> RobotContainer.ARM.setArmState(ArmConstants.ArmState.REST), // or no-op
() -> false,
RobotContainer.ARM
);
}
// New: hold rollers at specified voltage (no auto-stop on end).
public static Command getSetEndEffectorHoldCommand(double holdVoltage) {
return new StartEndCommand(
() -> RobotContainer.ARM.setEndEffectorState(new ArmConstants.ArmState(ArmConstants.ArmState.REST.targetAngle, holdVoltage)),
() -> {}, // keep holding after this command unless another command takes control
RobotContainer.ARM
);
}Then refactor getLoadCoralCommand:
- return new ParallelCommandGroup(
- ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.LOAD_CORAL),
- ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL)
- ).until(RobotContainer.ARM::hasGamePiece);
+ final double HOLD_VOLTAGE = -1.5; // tune as needed
+ return new ParallelCommandGroup(
+ ArmCommands.getSetArmStateCommand(ArmConstants.ArmState.LOAD_CORAL, false),
+ ArmCommands.getSetEndEffectorHoldCommand(ArmConstants.ArmState.LOAD_CORAL.targetEndEffectorVoltage),
+ ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL)
+ )
+ .until(RobotContainer.ARM::hasGamePiece)
+ .andThen(ArmCommands.getSetEndEffectorHoldCommand(HOLD_VOLTAGE))
+ .withTimeout(2.5);If adding helpers isn’t desirable now, at least keep the current rollers running briefly post-detection via an InstantCommand that bumps the end-effector voltage (requires a public API to set it without stopping).
To confirm the stop-on-cancel behavior:
🏁 Script executed:
#!/bin/bash
# Verify ArmCommands end behavior and Arm.stop() effect
rg -nP 'FunctionalCommand\(|ExecuteEndCommand\(' src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java -C3
rg -n 'void stop\(\)' src/main/java/frc/trigon/robot/subsystems/arm/Arm.java -n -C3Length of output: 886
Critical: cancellation currently stops rollers — add timeout and decouple roller control
Verified: getSetTargetStateCommand(...) ends by calling RobotContainer.ARM.stop(), and Arm.stop() calls armMasterMotor.stopMotor() and endEffectorMotor.stopMotor() (ArmCommands.java ~lines 40–44; Arm.java ~lines 48–52). .until(...) will therefore cut power to the rollers on detection, and there is no timeout if the sensor never trips.
- Immediate fix (apply now, in CoralCollectionCommands.getLoadCoralCommand): add a short timeout so rollers aren’t cut off instantly.
- ).until(RobotContainer.ARM::hasGamePiece);
+ )
+ .until(RobotContainer.ARM::hasGamePiece)
+ .withTimeout(2.5);- Required follow-up (must address before release): decouple arm positioning from end-effector control so cancelling/ending an arm-position command does not stop the rollers.
- Change ArmCommands.getSetTargetStateCommand(...) to avoid calling RobotContainer.ARM.stop() for end-effect behavior (or call a function that only stops the arm motor).
- Add a separate command to control/hold the end-effector (rollers) so you can explicitly stop or hold rollers after detection.
Files/locations to update:
- Immediate: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (getLoadCoralCommand).
- Follow-up: src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (FunctionalCommand end handler) and src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (stop implementation lines shown above).
📝 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.
| public static Command getLoadCoralCommand() { | |
| return new ParallelCommandGroup( | |
| ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.LOAD_CORAL), | |
| ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL) | |
| ).until(RobotContainer.ARM::hasGamePiece); | |
| } | |
| public static Command getLoadCoralCommand() { | |
| return new ParallelCommandGroup( | |
| ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.LOAD_CORAL), | |
| ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL) | |
| ) | |
| .until(RobotContainer.ARM::hasGamePiece) | |
| .withTimeout(2.5); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
around lines 22–27, the ParallelCommandGroup used for getLoadCoralCommand is
being cancelled by .until(...) which triggers Arm.stop() and cuts power to the
rollers immediately; add a short timeout to the command (e.g., withTimeout(0.5)
on the ParallelCommandGroup) so the rollers are allowed to run briefly before
the until predicate can cancel them, and keep the existing
.until(RobotContainer.ARM::hasGamePiece) behavior; as a required follow-up
before release, refactor ArmCommands.getSetTargetStateCommand(...) to avoid
calling RobotContainer.ARM.stop() for end-effector behavior and add a separate
command/subsystem to control/hold the rollers so ending/cancelling arm position
commands does not stop the rollers.
| @AutoLogOutput(key = "Arm/HasCoral") | ||
| public boolean hasGamePiece() { | ||
| return ArmConstants.COLLECTION_DETECTION_BOOLEAN_EVENT.getAsBoolean(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Method name vs log key mismatch; also confirm sensor polarity
- The method is hasGamePiece() but the log key is Arm/HasCoral. Consider aligning the name or key for clarity (e.g., hasCoral() or key "Arm/HasGamePiece").
- BooleanEvent wraps DISTANCE_SENSOR::getBinaryValue with debounce. Ensure the sensor is active-high for “coral present”; the sim supplier returns 0 when holding which may invert semantics if getBinaryValue treats nonzero as true.
Quick checks:
🏁 Script executed:
#!/bin/bash
# Confirm any callers expect "coral" semantics
rg -nP '\bhasGamePiece\s*\(' -C2
# Verify sim vs HW polarity comments/usages near the distance sensor
rg -nP 'DISTANCE_SENSOR|COLLECTION_DETECTION_BOOLEAN_EVENT|SimulationFieldHandler\.isHoldingGamePiece' -n -C3Length of output: 13502
Align log key with method name & correct simulation polarity
- The log key on Arm.java line 108 (
"Arm/HasCoral") doesn’t match hasGamePiece(): change it to"Arm/HasGamePiece"(or rename the method to hasCoral()). - In ArmConstants.java line 94 the simulation supplier uses
isHoldingGamePiece() ? 0 : 1, which inverts detection; flip to? 1 : 0so a held game piece yields true.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/arm/Arm.java around lines 108–111
the AutoLogOutput key is mislabeled "Arm/HasCoral" while the method is
hasGamePiece(); update the key to "Arm/HasGamePiece" (or rename the method to
hasCoral if you prefer consistent naming). Also in
src/main/java/frc/trigon/robot/constants/ArmConstants.java around line 94 the
simulation supplier inverts detection by returning isHoldingGamePiece() ? 0 : 1;
flip the ternary to isHoldingGamePiece() ? 1 : 0 so that a held game piece maps
to true in simulation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(45-119)
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
21-27: Supplier is polled every cycle; confirm this is intended (dynamic vs. capture-on-schedule).ExecuteEndCommand will call
targetState.get()repeatedly while scheduled. If you want the state captured once at schedule time (so operator changes mid-execution don’t retarget), prefer a deferred creation:Option A (capture at schedule time):
public static Command getSetTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { - return new ExecuteEndCommand( - () -> RobotContainer.ELEVATOR.setTargetState(targetState.get()), - RobotContainer.ELEVATOR::stop, - RobotContainer.ELEVATOR - ); + return edu.wpi.first.wpilibj2.command.Commands.defer( + () -> getSetTargetStateCommand(targetState.get()), + RobotContainer.ELEVATOR + ); }Note: requires
import edu.wpi.first.wpilibj2.command.Commands;If the “always follow the supplier” behavior is desired, keep as-is but please confirm.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (3)
87-87: Confirm tolerance choice and wire it to closed-loop error where applicable.0.01 m (~10 mm) may be tighter than the mechanism’s repeatability. Verify it aligns with Motion Magic/allowable error and sensor quantization; otherwise expect “never at target” flicker.
175-186: Double-check heights and prepare-state intent.
- SCORE_L1 and SCORE_L2 both use 0.203 m. Is that intentional? If not, update L2.
- All prepareStatePositionMeters equal targetPositionMeters, which makes “prepare” equivalent to “target.” If the intent was a staging/safe transit height (e.g., 0.603 m before L3/L4), consider setting distinct prepare values.
188-195: Document the new field for clarity.Add brief Javadoc explaining when prepareStatePositionMeters is used relative to targetPositionMeters and how speedScalar should be chosen.
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (2)
21-27: Resolve Supplier once to avoid state thrash; optionally add a “dynamic” variant.Re-evaluating targetState.get() every cycle can cause oscillations if the Supplier is HID/NT-backed. Capture once and delegate to the fixed-state overload.
- public static Command getSetTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { - return new ExecuteEndCommand( - () -> RobotContainer.ELEVATOR.setTargetState(targetState.get()), - RobotContainer.ELEVATOR::stop, - RobotContainer.ELEVATOR - ); - } + public static Command getSetTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { + final var resolved = targetState.get(); + return getSetTargetStateCommand(resolved); + }If you truly need live re-evaluation, keep the current form but provide a clearly named method like getSetTargetStateCommandDynamic(Supplier<...>) and use it intentionally.
45-51: Same concern for prepare-state: capture once or offer a dynamic variant.Current code re-reads the Supplier every cycle.
- public static Command getSetPrepareTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { - return new ExecuteEndCommand( - () -> RobotContainer.ELEVATOR.setPrepareState(targetState.get()), - RobotContainer.ELEVATOR::stop, - RobotContainer.ELEVATOR - ); - } + public static Command getSetPrepareTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { + final var resolved = targetState.get(); + return getSetPrepareTargetStateCommand(resolved); + }Also ensure callers add a finish condition when these are used in sequences (e.g., .until(ELEVATOR::atPreparedTargetState)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
- src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(45-119)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (2)
9-9: LGTM: needed import.
53-58: LGTM; matches the existing pattern.Fixed-state prepare command is clear and consistent with the target-state overload.
Please confirm sequence callers gate completion (e.g., until atPreparedTargetState) so these don’t run indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
31-34: Add a timeout on the prepare phase to prevent deadlocks if the sensor never reports “at prepare.”If
atPrepareAngle()never becomes true (sensor fault, saturation, soft-limit), the sequence never advances and EJECT never engages. Add a small timeout as a safety net.return new SequentialCommandGroup( - ArmCommands.getPrepareForStateCommand(ArmConstants.ArmState.EJECT).until(RobotContainer.ARM::atPrepareAngle), + ArmCommands.getPrepareForStateCommand(ArmConstants.ArmState.EJECT) + .until(RobotContainer.ARM::atPrepareAngle) + .withTimeout(1.0), ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.EJECT) );Optionally, make the timeout a named constant in ArmConstants to tune on‑robot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
- src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
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.248Z
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/arm/ArmConstants.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (2)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(14-70)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-259)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (4)
237-237: New coral loading state added.The addition of
LOAD_CORALstate with appropriate angles and negative voltage for intake operation looks correct and aligns with the PR's objective to add coral placing functionality.
253-256: Constructor refactored to include prepare angle.The three-parameter constructor properly initializes all fields including the new
prepareAngle. This enables the arm to have different intermediate positioning states before reaching the target angle, which is useful for coordinated multi-subsystem operations.
250-250: New public field added for prepare angle.The
prepareAnglefield is properly declared as public final, maintaining consistency with the existingtargetAngleandtargetEndEffectorVoltagefields.
235-247: Confirm identical prepare/target angles are intentional.prepareAngle is actively used: Arm.atPrepareAngle() (src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:108–112), setTargetState()/setTargetAngle(...) uses prepareAngle (src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:157–163), and CoralEjectionCommands waits on atPrepareAngle (src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java:32). Many ArmState entries in ArmConstants (src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java:235–247; fields at 249–256) set prepareAngle == targetAngle, making the prepare step a no-op — confirm this is intentional or adjust prepareAngle values/command flow.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
32-32: Approve: gate on prepare angle before EJECT.Confirmed in src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java: EJECT(Rotation2d.fromDegrees(60), Rotation2d.fromDegrees(60), 4) — prepare == target, so the change is correct and future‑proofs behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
5-5: Imports OK; ConditionalCommand will be removable after refactor.If you adopt the supplier‑polling rest command below, ConditionalCommand becomes unused.
Also applies to: 13-13
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
234-249: Validate target vs prepare angles per level.All prepareAngle values equal targetAngle. If the intent is distinct “preposition then finalize,” consider setting different prepareAngle values where needed (e.g., safer approach angles for L2/L4).
Using your prior note that angles >90° bypass isArmAboveSafeAngle() checks, please confirm REST_WITH_CORAL=180° and SCORE_L2=180° are desired under that safety model.
251-257: New prepareAngle field — add brief Javadoc for semantics.Document when prepareAngle is honored (e.g., by Arm.setPrepareState) and its frame/units to avoid misuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
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.248Z
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/arm/ArmConstants.java
🔇 Additional comments (6)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
28-30: LGTM on import regrouping.No behavioral change; matches nearby subsystem import style.
78-78: Default Arm “rest” won’t adapt when coral state changes.ArmCommands.getRestCommand() currently uses ConditionalCommand, which evaluates once and never reselects. If hasGamePiece() flips while this default is running, the arm won’t switch between REST and REST_WITH_CORAL. Please refactor getRestCommand() to be supplier‑driven and continuously poll (see ArmCommands comments), then keep this binding as-is.
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (4)
66-66: Good: prepare-state path uses setPrepareState().Matches the new ArmState.prepareAngle concept and keeps this command non‑finishing.
38-40: Supplier is dereferenced at construction — dynamic selection is lost.This pre-resolves the state and breaks chooser-driven runtime behavior. Align with Elevator by polling the supplier each execute.
Apply this diff:
- public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState) { - return getSetTargetStateCommand(targetState.get(), false); - } + public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState) { + return new FunctionalCommand( + () -> { /* no-op init */ }, + () -> { + var state = targetState.get(); + RobotContainer.ARM.setEndEffectorState(state); + RobotContainer.ARM.setArmState(state, false); + }, + interrupted -> RobotContainer.ARM.stop(), + () -> false, + RobotContainer.ARM + ); + }
56-58: Same issue: Supplier pre-resolved for prepare-state overload.Evaluate the supplier at runtime so chooser updates take effect.
Apply this diff:
- public static Command getPrepareForStateCommand(Supplier<ArmConstants.ArmState> targetState) { - return getPrepareForStateCommand(targetState.get(), false); - } + public static Command getPrepareForStateCommand(Supplier<ArmConstants.ArmState> targetState) { + return new ExecuteEndCommand( + () -> RobotContainer.ARM.setPrepareState(targetState.get(), false), + RobotContainer.ARM::stop, + RobotContainer.ARM + ); + }
72-78: getRestCommand should be reactive, not one-shot.ConditionalCommand picks once at init and never re-evaluates; default command won’t follow hasGamePiece() changes. Make it supplier‑driven so it adapts live.
Apply this diff (relies on the supplier‑polling fix above):
- public static Command getRestCommand() { - return new ConditionalCommand( - getSetTargetStateCommand(ArmConstants.ArmState.REST_WITH_CORAL), - getSetTargetStateCommand(ArmConstants.ArmState.REST), - RobotContainer.ARM::hasGamePiece - ); - } + public static Command getRestCommand() { + return getSetTargetStateCommand( + () -> RobotContainer.ARM.hasGamePiece() + ? ArmConstants.ArmState.REST_WITH_CORAL + : ArmConstants.ArmState.REST + ); + }Run to confirm call sites and default binding:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (2)
21-27: Prefer deferring Supplier evaluation to schedule-time (avoid continuous re-setting).Today, setTargetState(targetState.get()) runs every execute cycle. If you intend to sample once when scheduled (common for “go to state” commands), wrap with DeferredCommand.
Apply:
- public static Command getSetTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { - return new ExecuteEndCommand( - () -> RobotContainer.ELEVATOR.setTargetState(targetState.get()), - RobotContainer.ELEVATOR::stop, - RobotContainer.ELEVATOR - ); - } + public static Command getSetTargetStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { + return new edu.wpi.first.wpilibj2.command.DeferredCommand( + () -> getSetTargetStateCommand(targetState.get()), + java.util.Set.of(RobotContainer.ELEVATOR) + ); + }
45-51: Same defer note for prepare-state Supplier.Mirror the schedule-time sampling to avoid repeatedly calling prepareState when not needed.
- public static Command getPrepareStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { - return new ExecuteEndCommand( - () -> RobotContainer.ELEVATOR.prepareState(targetState.get()), - RobotContainer.ELEVATOR::stop, - RobotContainer.ELEVATOR - ); - } + public static Command getPrepareStateCommand(Supplier<ElevatorConstants.ElevatorState> targetState) { + return new edu.wpi.first.wpilibj2.command.DeferredCommand( + () -> getPrepareStateCommand(targetState.get()), + java.util.Set.of(RobotContainer.ELEVATOR) + ); + }src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
61-66: Remove unused local or use it.robotOrientationOnField is unused.
- final Rotation2d robotOrientationOnField = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation();
24-26: SHOULD_SCORE_AUTONOMOUSLY is unused.Either wire it to gate autonomous pathing, or drop it.
55-59: calculateDistanceToTargetScoringPose is unused.Delete or use for gating/telemetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: The Swerve subsystem's atPose method is designed to accept FlippablePose2d directly, not Pose2d. The method signature is `public boolean atPose(FlippablePose2d pose2d)` and it handles the internal conversion to Pose2d by calling pose2d.get().
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-197)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (10)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(16-113)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-60)src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
ReefChooser(16-130)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-79)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(15-65)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 11-11: cannot find symbol: import frc.trigon.robot.constants.AutonomousConstants; symbol: class AutonomousConstants
🔇 Additional comments (7)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (2)
9-9: Import for Supplier is correct.Needed for the new overloads; no issues.
53-58: Prepare-state fixed overload looks good.src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (2)
80-92: New “at…” queries are correct and cohesive.Tolerance-based checks use HEIGHT_TOLERANCE_METERS; comparisons look right.
116-121: prepareState implementation matches contract.Speed scaling + prepareStatePositionMeters target is appropriate; package-private visibility aligns with ElevatorCommands in same package.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
169-173: Level calculation off-by-one.Return ordinal()+1 so L1→1, L2→2, etc.
- private int calculateLevel() { - if (ordinal() == 0) - return 1; - return ordinal(); - } + private int calculateLevel() { + return ordinal() + 1; + }
158-166: Same mapping bug for Arm state.- private ArmConstants.ArmState determineArmState() { - return switch (ordinal()) { - case 0 -> ArmConstants.ArmState.REST; - case 1 -> ArmConstants.ArmState.SCORE_L1; - case 2 -> ArmConstants.ArmState.SCORE_L2; - case 3 -> ArmConstants.ArmState.SCORE_L3; - case 4 -> ArmConstants.ArmState.SCORE_L4; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; - } + private ArmConstants.ArmState determineArmState() { + return switch (this) { + case L1 -> ArmConstants.ArmState.SCORE_L1; + case L2 -> ArmConstants.ArmState.SCORE_L2; + case L3 -> ArmConstants.ArmState.SCORE_L3; + case L4 -> ArmConstants.ArmState.SCORE_L4; + }; + }
147-156: Enum-to-state mapping is wrong (L1→REST, etc.).Map explicitly by constant.
- private ElevatorConstants.ElevatorState determineElevatorState() { - return switch (ordinal()) { - case 0 -> ElevatorConstants.ElevatorState.REST; - case 1 -> ElevatorConstants.ElevatorState.SCORE_L1; - case 2 -> ElevatorConstants.ElevatorState.SCORE_L2; - case 3 -> ElevatorConstants.ElevatorState.SCORE_L3; - case 4 -> ElevatorConstants.ElevatorState.SCORE_L4; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; - } + private ElevatorConstants.ElevatorState determineElevatorState() { + return switch (this) { + case L1 -> ElevatorConstants.ElevatorState.SCORE_L1; + case L2 -> ElevatorConstants.ElevatorState.SCORE_L2; + case L3 -> ElevatorConstants.ElevatorState.SCORE_L3; + case L4 -> ElevatorConstants.ElevatorState.SCORE_L4; + }; + }
| import frc.trigon.robot.constants.AutonomousConstants; | ||
| import frc.trigon.robot.constants.FieldConstants; |
There was a problem hiding this comment.
Build break: AutonomousConstants not found.
Either add the constants class or stop depending on it here. Quick unblock: inline a local PathConstraints and drop the import.
-import frc.trigon.robot.constants.AutonomousConstants;
+import com.pathplanner.lib.path.PathConstraints;
@@
- AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS
+ DRIVE_TO_REEF_CONSTRAINTS
),Add near Line 26:
+ // TODO: tune these; placeholder to unblock compilation if AutonomousConstants is absent
+ private static final PathConstraints DRIVE_TO_REEF_CONSTRAINTS = new PathConstraints(3.0, 2.0, 8.0, 10.0);Alternatively, add frc.trigon.robot.constants.AutonomousConstants with DRIVE_TO_REEF_CONSTRAINTS.
Also applies to: 47-52
🧰 Tools
🪛 GitHub Actions: Java CI with Gradle
[error] 11-11: cannot find symbol: import frc.trigon.robot.constants.AutonomousConstants; symbol: class AutonomousConstants
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
around lines 11-12 and 26 (and also affecting lines 47-52): the import of
frc.trigon.robot.constants.AutonomousConstants causes a build break because the
class is missing; either remove the import and inline a local PathConstraints
instance (named e.g. DRIVE_TO_REEF_CONSTRAINTS) near line 26 and use that in
place of AutonomousConstants references, or create the missing
frc.trigon.robot.constants.AutonomousConstants class exposing
DRIVE_TO_REEF_CONSTRAINTS; update all uses at lines ~47-52 to reference the new
local constant or the newly-added class accordingly.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
61-85: Avoid mixed flipping (pre‑flipped translation + non‑flipping FlippablePose2d)Pre‑flipping only the translation (line 64) and returning a non‑flipping FlippablePose2d (line 84) risks wrong heading on red. Prefer constructing an unflipped blue‑reference Pose2d and returning a FlippablePose2d that flips both translation and rotation.
Apply:
- final Translation2d reefCenterPosition = new FlippableTranslation2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, true).get(); + final Translation2d reefCenterPosition = FieldConstants.BLUE_REEF_CENTER_TRANSLATION; @@ - return new FlippablePose2d(closestScoringPose.transformBy(shouldScoreRight ? scoringPoseToRightBranch : scoringPoseToRightBranch.inverse()), false); + return new FlippablePose2d( + closestScoringPose.transformBy(shouldScoreRight ? scoringPoseToRightBranch : scoringPoseToRightBranch.inverse()), + true + );Please sanity‑check robot heading on red alliance at a couple of clock positions after this change.
45-53: Manual-drive step never ends by itself (acceptable if gated by CONTINUE)Since step 2 is a continuous drive command, rely on the upstream
.until(OperatorConstants.CONTINUE_TRIGGER)to end the parallel prep. If operator gating is not guaranteed, consider ParallelRaceGroup with an explicit WaitUntilCommand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: The Swerve subsystem's atPose method is designed to accept FlippablePose2d directly, not Pose2d. The method signature is `public boolean atPose(FlippablePose2d pose2d)` and it handles the internal conversion to Pose2d by calling pose2d.get().
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-18T22:46:49.248Z
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.248Z
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/arm/ArmConstants.java
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/lib/commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-260)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (8)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(16-113)src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
ReefChooser(16-130)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 11-11: Compilation error: cannot find symbol - import frc.trigon.robot.constants.AutonomousConstants; AutonomousConstants class not found in frc.trigon.robot.constants
🔇 Additional comments (9)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
235-249: Prep-angle wiring looks consistent; new states defined correctlyThree-arg ctor use and prepareAngle field integration across all enum constants is coherent.
241-246: Verify L1–L4 target/prepare angles vs safety windowSCORE_L1 has target 110° (prepare 110°) and safety cutoff is 90°. Per team learning, >90° bypasses safety—confirm this is intentional for on‑field operation.
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (2)
87-87: Good: explicit height toleranceHEIGHT_TOLERANCE_METERS = 0.01 adds clarity for atTarget checks.
175-196: Enum now carries prepare-state height; ctor/signature alignAll constants pass (target, prepare, speedScalar) and the new field is stored; no API inconsistencies spotted.
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (3)
38-45: Supplier variant now evaluates at runtime (good)FunctionalCommand polls Supplier inside lambdas; fixes earlier pre‑resolution pitfall.
61-67: Prepare-state Supplier variant consistent with patternExecuteEndCommand form matches Elevator’s semantics.
81-87: Nice: REST auto‑selects with/without coralConditionalCommand keeps default behavior smart.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
94-97: L1 uses NaN/null → NPE/NaN propagation in pose calcPassing null Rotation2d and NaN transforms will crash in Transform2d and downstream math.
Apply:
- L1(Double.NaN, Double.NaN, null), + L1(FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS, + FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS, + Rotation2d.fromDegrees(0)),
11-12: Build break: AutonomousConstants not foundRemove this import and inline a PathConstraints constant (or add the missing class). Also add the missing GeneralCommands import.
Apply:
-import frc.trigon.robot.constants.AutonomousConstants; +import com.pathplanner.lib.path.PathConstraints; +import frc.trigon.robot.commands.commandclasses.GeneralCommands;Define a local constraints constant (tune values as needed):
public class CoralPlacingCommands { public static boolean SHOULD_SCORE_AUTONOMOUSLY = true; private static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER; + // TODO: tune these + private static final PathConstraints DRIVE_TO_REEF_CONSTRAINTS = new PathConstraints(3.0, 2.0, 8.0, 10.0);Use it:
- AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS + DRIVE_TO_REEF_CONSTRAINTS
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
5-5: Clean up unused import if REST change appliedIf you replace ConditionalCommand in getRestCommand(), drop this import.
-import edu.wpi.first.wpilibj2.command.ConditionalCommand;src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
40-42: Use explicit 360.0 rather than Conversions.DEGREES_PER_ROTATIONS for clarity.This constant currently equals 360; the intent is clearer with a literal and avoids surprising coupling.
- public static final Rotation2d REEF_CLOCK_POSITION_DIFFERENCE = Rotation2d.fromDegrees(Conversions.DEGREES_PER_ROTATIONS / REEF_CLOCK_POSITIONS); + public static final Rotation2d REEF_CLOCK_POSITION_DIFFERENCE = Rotation2d.fromDegrees(360.0 / REEF_CLOCK_POSITIONS);
43-48: Document transform directions (field frame).Clarify X/Y sign semantics (toward reef/toward driver) to prevent misuse downstream.
Add a short comment describing positive X/Y directions relative to field origin and whether these offsets are toward a target or from it. No code changes required.
60-69: Prefer Set.contains and Map return type; keep method cohesive.Reflect the earlier rename and expose interface types. Also, keep the exclusion list name aligned.
- private static HashMap<Integer, Pose3d> fieldLayoutToTagIdToPoseMap() { - final HashMap<Integer, Pose3d> tagIdToPose = new HashMap<>(); + private static Map<Integer, Pose3d> fieldLayoutToTagIdToPoseMap() { + final Map<Integer, Pose3d> tagIdToPose = new HashMap<>(); for (AprilTag aprilTag : APRIL_TAG_FIELD_LAYOUT.getTags()) { - if (!I_HATE_YOU.contains(aprilTag.ID)) + if (!EXCLUDED_APRIL_TAG_IDS.contains(aprilTag.ID)) tagIdToPose.put(aprilTag.ID, aprilTag.pose.transformBy(TAG_OFFSET)); } return tagIdToPose; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/lib/commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-260)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)
🔇 Additional comments (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (3)
61-67: LGTM: runtime‑polled prepare supplierCommand polls suppliers during execute and stops cleanly. Matches intended continuous “prepare” behavior.
75-76: LGTM: prepare overload now uses setPrepareStateConsistent with ArmState.prepareAngle semantics.
13-13: LGTM: Supplier importRequired for new supplier overloads.
src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
70-83: XOR logic depends on correct isFacingDriverStation; confirm truth table.The XOR is fine, but correctness hinges on the per‑position facing flag. Any wrong flag will flip the Y transform incorrectly.
Add a quick unit test enumerating all 6 positions × 2 sides, asserting expected flip decisions for your field convention. I can draft it if helpful.
111-113: No change required — Rotation2d.times(double) exists in WPILib 2025.
The existing expression REEF_CLOCK_POSITION_DIFFERENCE.times(-ordinal()) is valid; the suggested Rotation2d.fromDegrees(...) rewrite is unnecessary.
31-37: Verify LEFT/RIGHT pose mirroring & rotationRIGHT is defined as FIELD_WIDTH_METERS - AUTO_FIND_CORAL_POSE_LEFT_Y with AUTO_FIND_CORAL_POSE_LEFT_ROTATION.unaryMinus() while also passing 'true' to FlippablePose2d — confirm FlippablePose2d's alliance flip does not also invert rotation (if it does, remove the extra unaryMinus), and verify (FIELD_WIDTH - y) + rotation.negation produces an exact mirror on both alliances. File: src/main/java/frc/trigon/robot/constants/FieldConstants.java lines 31–36.
| 13, 12, 16, 15, 14, 4, 5, 3, 2, 1 | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename magic list and document why these tags are excluded.
Name is unprofessional and unclear. Prefer a descriptive, intention‑revealing name and a Set for faster contains().
- private static final List<Integer> I_HATE_YOU = List.of(
- 13, 12, 16, 15, 14, 4, 5, 3, 2, 1
- );
+ // AprilTag IDs intentionally ignored by vision (ex: unreliable positions or occluded)
+ private static final Set<Integer> EXCLUDED_APRIL_TAG_IDS = Set.of(
+ 13, 12, 16, 15, 14, 4, 5, 3, 2, 1
+ );Additional imports required:
+import java.util.Set;📝 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.
| 13, 12, 16, 15, 14, 4, 5, 3, 2, 1 | |
| ); | |
| import java.util.Set; | |
| // AprilTag IDs intentionally ignored by vision (ex: unreliable positions or occluded) | |
| private static final Set<Integer> EXCLUDED_APRIL_TAG_IDS = Set.of( | |
| 13, 12, 16, 15, 14, 4, 5, 3, 2, 1 | |
| ); |
| public static final HashMap<Integer, Pose3d> TAG_ID_TO_POSE = fieldLayoutToTagIdToPoseMap(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose TAG map as an unmodifiable Map, not a concrete HashMap.
Avoid exposing mutability and concrete types from API.
- public static final HashMap<Integer, Pose3d> TAG_ID_TO_POSE = fieldLayoutToTagIdToPoseMap();
+ public static final Map<Integer, Pose3d> TAG_ID_TO_POSE =
+ java.util.Collections.unmodifiableMap(fieldLayoutToTagIdToPoseMap());Additional imports:
+import java.util.Map;
+import java.util.Collections;📝 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.
| public static final HashMap<Integer, Pose3d> TAG_ID_TO_POSE = fieldLayoutToTagIdToPoseMap(); | |
| import java.util.Map; | |
| import java.util.Collections; | |
| public static final Map<Integer, Pose3d> TAG_ID_TO_POSE = | |
| Collections.unmodifiableMap(fieldLayoutToTagIdToPoseMap()); |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/constants/FieldConstants.java around lines
28-29, the public TAG_ID_TO_POSE is currently declared as a concrete HashMap
which exposes mutability; change the exposed type to Map<Integer, Pose3d> and
initialize it with an unmodifiable map (e.g., wrap the result of
fieldLayoutToTagIdToPoseMap() with Collections.unmodifiableMap or have that
helper return an unmodifiable Map), and add the required imports (java.util.Map
and java.util.Collections) so callers see an immutable Map API rather than a
mutable HashMap.
| REEF_12_OCLOCK(true), | ||
| REEF_2_OCLOCK(true), | ||
| REEF_4_OCLOCK(true), | ||
| REEF_6_OCLOCK(true), | ||
| REEF_8_OCLOCK(true), | ||
| REEF_10_OCLOCK(true); | ||
|
|
||
| public final Rotation2d clockAngle; | ||
| public final boolean isFacingDriverStation; | ||
| public final int clockPosition; | ||
|
|
||
| ReefClockPosition(boolean isFacingDriverStation) { | ||
| this.clockAngle = calculateClockAngle(); | ||
| this.isFacingDriverStation = isFacingDriverStation; | ||
| this.clockPosition = ordinal() == 0 ? 12 : ordinal() * 2; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Likely bug: all ReefClockPosition entries set isFacingDriverStation=true.
With all true, shouldFlipYTransform degenerates to !doesFlipYTransformWhenFacingDriverStation, ignoring the position entirely. That’s almost certainly wrong.
Proposed fixes (pick one):
- Provide accurate booleans per clock position (half should be false).
- Or derive facing programmatically from clockAngle and field convention to avoid hardcoding.
Would you like me to wire a data‑driven derivation using the reef center-to‑driver vector?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "Locating FieldConstants.java..."
file=$(rg --files | rg "FieldConstants.java" || true)
if [ -z "$file" ]; then
file=$(find . -type f -name "FieldConstants.java" -print -quit || true)
fi
if [ -n "$file" ] && [ -f "$file" ]; then
echo "Found: $file"
echo "---- File head (1..260) ----"
sed -n '1,260p' "$file" || true
else
echo "FieldConstants.java not found in repo."
fi
echo
echo "---- Search: isFacingDriverStation ----"
rg -n --hidden -S "isFacingDriverStation" || true
echo
echo "---- Search: ReefClockPosition ----"
rg -n --hidden -S "ReefClockPosition" || true
echo
echo "---- Search: REEF_*OCLOCK enum lines ----"
rg -n --hidden -S "REEF_.*OCLOCK" -n -C2 || true
echo
echo "---- Search: calculateClockAngle ----"
rg -n --hidden -S "calculateClockAngle" || true
echo
echo "---- Search: shouldFlipYTransform|doesFlipYTransformWhenFacingDriverStation|flipYTransform ----"
rg -n --hidden -S "shouldFlipYTransform|doesFlipYTransformWhenFacingDriverStation|flipYTransform|flip.*Y.*Transform" || true
echo
echo "---- Search: clockAngle|clockPosition usages ----"
rg -n --hidden -S "clockAngle|clockPosition" || trueLength of output: 17592
Fix required: ReefClockPosition enum sets every isFacingDriverStation=true (breaks flipping logic).
All REEF_*_OCLOCK entries are constructed with isFacingDriverStation=true; as a result ReefSide.shouldFlipYTransform(...) simplifies to !doesFlipYTransformWhenFacingDriverStation and ignores the clock position. This is incorrect.
Location: src/main/java/frc/trigon/robot/constants/FieldConstants.java (lines 86–101).
- Set the correct boolean for each clock position (about half should be false) so shouldFlipYTransform depends on position.
- Or derive facing programmatically from clockAngle and the reef center→driver vector to avoid hardcoding.
Can implement the data‑driven derivation if desired.
| public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState, Supplier<Boolean> isStateReversed) { | ||
| return new FunctionalCommand( | ||
| () -> RobotContainer.ARM.setEndEffectorState(targetState.get()), | ||
| () -> RobotContainer.ARM.setArmState(targetState.get(), isStateReversed.get()), | ||
| interrupted -> RobotContainer.ARM.stop(), | ||
| () -> false, | ||
| RobotContainer.ARM | ||
| ); } |
There was a problem hiding this comment.
Fix supplier variant: end‑effector can desync when supplier changes mid‑run
You only set the end‑effector once in initialize(). If targetState supplier changes while the command runs, execute() updates the arm angle but keeps the old roller voltage. Update both each cycle (or capture once via Commands.defer if static selection is desired).
Apply this diff:
- public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState, Supplier<Boolean> isStateReversed) {
- return new FunctionalCommand(
- () -> RobotContainer.ARM.setEndEffectorState(targetState.get()),
- () -> RobotContainer.ARM.setArmState(targetState.get(), isStateReversed.get()),
- interrupted -> RobotContainer.ARM.stop(),
- () -> false,
- RobotContainer.ARM
- ); }
+ public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState, Supplier<Boolean> isStateReversed) {
+ return new FunctionalCommand(
+ () -> { },
+ () -> {
+ var state = targetState.get();
+ RobotContainer.ARM.setEndEffectorState(state);
+ RobotContainer.ARM.setArmState(state, isStateReversed.get());
+ },
+ interrupted -> RobotContainer.ARM.stop(),
+ () -> false,
+ RobotContainer.ARM
+ );
+ }📝 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.
| public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState, Supplier<Boolean> isStateReversed) { | |
| return new FunctionalCommand( | |
| () -> RobotContainer.ARM.setEndEffectorState(targetState.get()), | |
| () -> RobotContainer.ARM.setArmState(targetState.get(), isStateReversed.get()), | |
| interrupted -> RobotContainer.ARM.stop(), | |
| () -> false, | |
| RobotContainer.ARM | |
| ); } | |
| public static Command getSetTargetStateCommand(Supplier<ArmConstants.ArmState> targetState, Supplier<Boolean> isStateReversed) { | |
| return new FunctionalCommand( | |
| () -> { }, | |
| () -> { | |
| var state = targetState.get(); | |
| RobotContainer.ARM.setEndEffectorState(state); | |
| RobotContainer.ARM.setArmState(state, isStateReversed.get()); | |
| }, | |
| interrupted -> RobotContainer.ARM.stop(), | |
| () -> false, | |
| RobotContainer.ARM | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java around lines
38 to 45, the FunctionalCommand currently sets the end‑effector only in
initialize(), which can desync if the targetState Supplier changes while the
command is running; change the command so both the end‑effector and arm state
are updated each cycle by moving the end‑effector call into the execute lambda
(call RobotContainer.ARM.setEndEffectorState(targetState.get()) and
RobotContainer.ARM.setArmState(targetState.get(), isStateReversed.get()) inside
execute), keep initialize empty (or use Commands.defer(...) to capture the value
once if static behavior is desired), and leave end conditions, interrupt
handling, and requirements unchanged.
| public static Command getRestCommand() { | ||
| return new ConditionalCommand( | ||
| getSetTargetStateCommand(ArmConstants.ArmState.REST_WITH_CORAL), | ||
| getSetTargetStateCommand(ArmConstants.ArmState.REST), | ||
| RobotContainer.ARM::hasGamePiece | ||
| ); | ||
| } |
There was a problem hiding this comment.
Default REST won’t react to hasGamePiece changes
ConditionalCommand evaluates only at schedule time; as a default command it won’t switch REST↔REST_WITH_CORAL when possession changes. Poll each cycle instead.
Apply this diff:
- public static Command getRestCommand() {
- return new ConditionalCommand(
- getSetTargetStateCommand(ArmConstants.ArmState.REST_WITH_CORAL),
- getSetTargetStateCommand(ArmConstants.ArmState.REST),
- RobotContainer.ARM::hasGamePiece
- );
- }
+ public static Command getRestCommand() {
+ return new ExecuteEndCommand(
+ () -> {
+ var state = RobotContainer.ARM.hasGamePiece()
+ ? ArmConstants.ArmState.REST_WITH_CORAL
+ : ArmConstants.ArmState.REST;
+ RobotContainer.ARM.setEndEffectorState(state);
+ RobotContainer.ARM.setArmState(state, false);
+ },
+ RobotContainer.ARM::stop,
+ RobotContainer.ARM
+ );
+ }If you apply this, remove the now‑unused ConditionalCommand import.
📝 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.
| public static Command getRestCommand() { | |
| return new ConditionalCommand( | |
| getSetTargetStateCommand(ArmConstants.ArmState.REST_WITH_CORAL), | |
| getSetTargetStateCommand(ArmConstants.ArmState.REST), | |
| RobotContainer.ARM::hasGamePiece | |
| ); | |
| } | |
| public static Command getRestCommand() { | |
| return new ExecuteEndCommand( | |
| () -> { | |
| var state = RobotContainer.ARM.hasGamePiece() | |
| ? ArmConstants.ArmState.REST_WITH_CORAL | |
| : ArmConstants.ArmState.REST; | |
| RobotContainer.ARM.setEndEffectorState(state); | |
| RobotContainer.ARM.setArmState(state, false); | |
| }, | |
| RobotContainer.ARM::stop, | |
| RobotContainer.ARM | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java around lines
81 to 87, the current getRestCommand returns a ConditionalCommand which only
evaluates at schedule time so the default command won't switch between REST and
REST_WITH_CORAL when RobotContainer.ARM::hasGamePiece changes; replace the
ConditionalCommand with a Command that re-evaluates each execute cycle (e.g.,
use a Perpetual or functional command that checks
RobotContainer.ARM.hasGamePiece() each cycle and calls the appropriate
getSetTargetStateCommand for REST or REST_WITH_CORAL), update getRestCommand to
return that polling command, and remove the now-unused ConditionalCommand
import.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
63-66: Remove unused variable and reuse existing constant.
- robotOrientationOnField is unused.
- Prefer FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION for consistency.
- final Rotation2d robotOrientationOnField = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d reefCenterPosition = new FlippableTranslation2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, true).get(); + final Translation2d reefCenterPosition = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get();
100-101: Javadoc nit: “gripper state” → “arm state.”- * as well as a different elevator and gripper state. + * as well as a different elevator and arm state.
24-26: Public mutable flag is unused; remove or wire it.Dead public state invites misuse. If not needed here, drop it.
- public static boolean SHOULD_SCORE_AUTONOMOUSLY = true; + // Removed unused global flag; re-introduce only when actually used.
📜 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(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: The Swerve subsystem's atPose method is designed to accept FlippablePose2d directly, not Pose2d. The method signature is `public boolean atPose(FlippablePose2d pose2d)` and it handles the internal conversion to Pose2d by calling pose2d.get().
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (12)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-60)src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
ReefChooser(16-130)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(32-260)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-197)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(15-65)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 11-11: cannot find symbol: AutonomousConstants; import frc.trigon.robot.constants.AutonomousConstants cannot be resolved. Ensure the class exists and is on the classpath.
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
151-169: Avoid ordinal-based state mapping; switch on the enum constant.Using level/ordinal is brittle. Map directly on this.
- private ElevatorConstants.ElevatorState determineElevatorState() { - return switch (level) { + private ElevatorConstants.ElevatorState determineElevatorState() { + return switch (this) { case 1 -> ElevatorConstants.ElevatorState.SCORE_L1; case 2 -> ElevatorConstants.ElevatorState.SCORE_L2; case 3 -> ElevatorConstants.ElevatorState.SCORE_L3; case 4 -> ElevatorConstants.ElevatorState.SCORE_L4; default -> throw new IllegalStateException("Unexpected value: " + ordinal()); }; } @@ - private ArmConstants.ArmState determineArmState() { - return switch (level) { + private ArmConstants.ArmState determineArmState() { + return switch (this) { case 1 -> ArmConstants.ArmState.SCORE_L1; case 2 -> ArmConstants.ArmState.SCORE_L2; case 3 -> ArmConstants.ArmState.SCORE_L3; case 4 -> ArmConstants.ArmState.SCORE_L4; default -> throw new IllegalStateException("Unexpected value: " + ordinal()); }; }Optional follow-up: set elevatorState/armState explicitly in each enum constant and delete these helpers.
104-107: L1 enum uses NaN/null → guaranteed NPE in calculateTargetPlacingPosition.Transform2d requires non-null Rotation2d and finite numbers.
- L1(Double.NaN, Double.NaN, null), + L1(FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS, + FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS, + Rotation2d.fromDegrees(0)),
| import edu.wpi.first.wpilibj2.command.ParallelCommandGroup; | ||
| import edu.wpi.first.wpilibj2.command.SequentialCommandGroup; | ||
| import frc.trigon.robot.RobotContainer; | ||
| import frc.trigon.robot.constants.AutonomousConstants; |
There was a problem hiding this comment.
Fix build: replace missing AutonomousConstants with local PathConstraints.
AutonomousConstants isn’t on classpath (pipeline error). Inline a local PathConstraints and drop the import/usage.
-import frc.trigon.robot.constants.AutonomousConstants;
+import com.pathplanner.lib.path.PathConstraints;
@@
public class CoralPlacingCommands {
- public static boolean SHOULD_SCORE_AUTONOMOUSLY = true;
+ public static boolean SHOULD_SCORE_AUTONOMOUSLY = true;
private static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER;
+ // TODO: tune values or move to a restored AutonomousConstants
+ private static final PathConstraints DRIVE_TO_REEF_CONSTRAINTS = new PathConstraints(3.0, 2.0, 8.0, 10.0);
@@
SwerveCommands.getDriveToPoseCommand(
() -> CoralPlacingCommands.calculateClosestScoringPose(shouldScoreRight),
- AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS
+ DRIVE_TO_REEF_CONSTRAINTS
),Also applies to: 47-52
🧰 Tools
🪛 GitHub Actions: Java CI with Gradle
[error] 11-11: cannot find symbol: AutonomousConstants; import frc.trigon.robot.constants.AutonomousConstants cannot be resolved. Ensure the class exists and is on the classpath.
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
around line 11 (and usages around lines 47-52), remove the nonexistent import of
frc.trigon.robot.constants.AutonomousConstants and replace references to
AutonomousConstants with a local PathConstraints definition inside this file;
add a small local static PathConstraints (or equivalent) instance/values that
mirror the fields used (max velocity, max acceleration, etc.), then update the
code that referenced AutonomousConstants to use this local PathConstraints
instance and drop the import line.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
125-130: Javadoc nit: mentions “gripper state” but enum exposes arm/elevator.Update wording to avoid confusion.
Apply:
- * as well as a different elevator and gripper state. + * as well as different elevator and arm states.
80-87: Minor: reuse cached flippable reef center to avoid reallocation.Functionally identical, a bit cleaner.
- final Translation2d reefCenterPosition = new FlippableTranslation2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, true).get(); + final Translation2d reefCenterPosition = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-18T22:46:49.248Z
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.248Z
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/arm/ArmConstants.java
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: The Swerve subsystem's atPose method is designed to accept FlippablePose2d directly, not Pose2d. The method signature is `public boolean atPose(FlippablePose2d pose2d)` and it handles the internal conversion to Pose2d by calling pose2d.get().
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (9)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-60)src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
ReefChooser(16-130)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 12-12: cannot find symbol: import frc.trigon.robot.constants.AutonomousConstants; symbol: class AutonomousConstants
🔇 Additional comments (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
120-120: ANGLE_TOLERANCE widened to 5° — verify system-level effects.Confirm this larger tolerance doesn’t prematurely report “atPrepareAngle/atTargetAngle,” especially around reef‑safety interlocks and elevator gating.
235-258: New ArmState.prepareAngle API looks consistent; values for L2–L4 > 90° align with safety bypass.Mapping reads sane and fits the team’s pattern (angles above 90° bypass safe‑angle logic per your prior design). No code issues spotted.
Please sanity‑check SCORE_L2–L4 prepareAngle=100° against collision margins and elevator clearances on‑bot.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
109-113: Prep completion predicate: confirm “closest scoring pose” is intended vs operator-selected pose.If the intent is to wait until the operator’s chosen clock position, consider using REEF_CHOOSER.calculateTargetScoringPose() instead of the nearest pose.
12-12: Build break: AutonomousConstants missing + missing imports; inline PathConstraints and import GeneralCommands.Replace the missing AutonomousConstants usage with a local PathConstraints and add the missing import for GeneralCommands.
Apply:
@@ -import frc.trigon.robot.constants.AutonomousConstants; +import com.pathplanner.lib.path.PathConstraints; +import frc.trigon.robot.commands.commandclasses.GeneralCommands; @@ public class CoralPlacingCommands { public static boolean SHOULD_SCORE_AUTONOMOUSLY = true; private static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER; + // TODO: tune or move back to AutonomousConstants when restored + private static final PathConstraints DRIVE_TO_REEF_CONSTRAINTS = new PathConstraints(3.0, 2.0, 8.0, 10.0); @@ private static Command getAutonomousDriveToReefThenManualDriveCommand(boolean shouldScoreRight) { return new SequentialCommandGroup( SwerveCommands.getDriveToPoseCommand( () -> CoralPlacingCommands.calculateClosestScoringPose(shouldScoreRight), - AutonomousConstants.DRIVE_TO_REEF_CONSTRAINTS + DRIVE_TO_REEF_CONSTRAINTS ), GeneralCommands.getFieldRelativeDriveCommand() ); }Also applies to: 24-27, 64-71
115-123: Reverse‑scoring condition is inverted (fires when facing reef).Engage reverse when robot faces away from reef (|error| ≥ 90°).
Apply:
- return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); + return robotRotationFacingReef.getCos() < 0;
131-135: L1 uses NaN/null → NPE/NaN propagation in pose math.Provide valid transforms and rotation. Temporary safe choice: reuse L2 transform until L1 is tuned.
Apply:
- L1(Double.NaN, Double.NaN, null), + L1(FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS, + FieldConstants.REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS, + Rotation2d.fromDegrees(0)),
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (5)
30-30: Calibrate the feedforward scalar before deployment.The TODO comment indicates that the
FEEDFORWARD_SCALARvalue of 0.5 needs calibration. This value affects path-following performance and should be tuned based on robot characteristics.Would you like me to open an issue to track the calibration of this feedforward scalar value?
38-44: Consider extracting duplicate coral pose constants.The coral pose constants (position and rotation) are duplicated from
FieldConstants.java. This violates DRY principles and could lead to inconsistencies if values need to be updated.- private static final double - AUTO_FIND_CORAL_POSE_X = 3.3, - AUTO_FIND_CORAL_POSE_LEFT_Y = 5.5; - private static final Rotation2d AUTO_FIND_CORAL_POSE_LEFT_ROTATION = Rotation2d.fromDegrees(130); - public static final FlippablePose2d - AUTO_FIND_CORAL_POSE_LEFT = new FlippablePose2d(AUTO_FIND_CORAL_POSE_X, AUTO_FIND_CORAL_POSE_LEFT_Y, AUTO_FIND_CORAL_POSE_LEFT_ROTATION, true), - AUTO_FIND_CORAL_POSE_RIGHT = new FlippablePose2d(AUTO_FIND_CORAL_POSE_X, FieldConstants.FIELD_WIDTH_METERS - AUTO_FIND_CORAL_POSE_LEFT_Y, AUTO_FIND_CORAL_POSE_LEFT_ROTATION.unaryMinus(), true); + // Reference the coral poses directly from FieldConstants to avoid duplication + public static final FlippablePose2d + AUTO_FIND_CORAL_POSE_LEFT = FieldConstants.AUTO_FIND_CORAL_POSE_LEFT, + AUTO_FIND_CORAL_POSE_RIGHT = FieldConstants.AUTO_FIND_CORAL_POSE_RIGHT;
102-104: Implement the registerCommands method.The TODO comment indicates that command registration is not yet implemented. This is essential for autonomous functionality.
Would you like me to help implement the command registration based on the autonomous commands being developed in this PR?
74-79: Consider adding initialization status tracking.The init() method performs critical setup but doesn't provide feedback on success or failure. Consider adding logging or status tracking.
public static void init() { + System.out.println("Initializing AutonomousConstants..."); Pathfinding.setPathfinder(new LocalADStarAK()); PathfindingCommand.warmupCommand().schedule(); configureAutoBuilder(); registerCommands(); + System.out.println("AutonomousConstants initialization complete"); }
66-69: Unused field AUTO_PATH_FOLLOWING_CONTROLLER.The controller is initialized but only used internally in configureAutoBuilder(). Consider whether it needs to be a field or could be a local variable.
- private static final PPHolonomicDriveController AUTO_PATH_FOLLOWING_CONTROLLER = new PPHolonomicDriveController( - AUTO_TRANSLATION_PID_CONSTANTS, - AUTO_ROTATION_PID_CONSTANTS - ); // ... other code ... private static void configureAutoBuilder() { + final PPHolonomicDriveController autoPathFollowingController = new PPHolonomicDriveController( + AUTO_TRANSLATION_PID_CONSTANTS, + AUTO_ROTATION_PID_CONSTANTS + ); AutoBuilder.configure( RobotContainer.ROBOT_POSE_ESTIMATOR::getEstimatedRobotPose, RobotContainer.ROBOT_POSE_ESTIMATOR::resetPose, RobotContainer.SWERVE::getSelfRelativeVelocity, (chassisSpeeds -> RobotContainer.SWERVE.drivePathPlanner(chassisSpeeds, true)), - AUTO_PATH_FOLLOWING_CONTROLLER, + autoPathFollowingController, ROBOT_CONFIG, Flippable::isRedAlliance, RobotContainer.SWERVE ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(41-115)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/utilities/LocalADStarAK.java (1)
LocalADStarAK(19-150)src/main/java/lib/utilities/flippable/Flippable.java (1)
Flippable(19-87)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (2)
29-29: Verify path constraints are appropriate for the robot.The DRIVE_TO_REEF_CONSTRAINTS specify relatively conservative speeds (2.5 m/s, 4 m/s²). Ensure these align with the robot's actual capabilities and competition strategy.
Are these constraint values (2.5 m/s max velocity, 4 m/s² max acceleration) aligned with your robot's capabilities and autonomous strategy requirements?
75-76: Verify PathfindingCommand warmup doesn't interfere with robot initialization.PathfindingCommand.warmupCommand().schedule() is present in AutonomousConstants.init() (AutonomousConstants.java:76) and PathPlannerConstants.init() (PathPlannerConstants.java:55); only PathPlannerConstants.init() is actually called from RobotContainer.initializeGeneralSystems() (RobotContainer.java:93) during RobotContainer construction. Confirm warmupCommand() is non‑blocking and does not require subsystems/CommandScheduler to be fully initialized or the robot to be enabled; if it can block or needs enabled state, schedule it later (e.g., robotInit/autonomousInit) or guard/mark it with .ignoringDisable(true) as appropriate.
| AUTO_TRANSLATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ? | ||
| new PIDConstants(0, 0, 0) : | ||
| new PIDConstants(0, 0, 0), | ||
| AUTO_ROTATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ? | ||
| new PIDConstants(0, 0, 0) : | ||
| new PIDConstants(0, 0, 0); |
There was a problem hiding this comment.
PID constants are all zeros - will disable path-following control.
Both translation and rotation PID constants are set to (0, 0, 0) for both simulation and real hardware. This effectively disables the path-following controller, making the robot unable to follow autonomous paths.
Apply this diff to use appropriate PID values:
private static final PIDConstants
AUTO_TRANSLATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ?
- new PIDConstants(0, 0, 0) :
- new PIDConstants(0, 0, 0),
+ new PIDConstants(5.0, 0, 0) : // Start with reasonable P-gain for simulation
+ new PIDConstants(5.0, 0, 0), // Tune these values during testing
AUTO_ROTATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ?
- new PIDConstants(0, 0, 0) :
- new PIDConstants(0, 0, 0);
+ new PIDConstants(3.0, 0, 0) : // Start with reasonable P-gain for simulation
+ new PIDConstants(3.0, 0, 0); // Tune these values during testing🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/constants/AutonomousConstants.java around
lines 50–55 the AUTO_TRANSLATION_PID_CONSTANTS and AUTO_ROTATION_PID_CONSTANTS
are initialized to (0,0,0) for both simulation and real hardware which disables
closed-loop path following; replace these zero tuples with sensible non-zero PID
triples (keep the RobotHardwareStats.isSimulation() conditional) — for example
set translation to something like (1.0, 0.0, 0.1) for real hardware and a
slightly lower/smoother sim value (0.8, 0.0, 0.08), and set rotation to
something like (3.0, 0.0, 0.2) real / (2.5, 0.0, 0.15) sim; ensure you construct
new PIDConstants(...) with those values and document that they should be
validated/tuned on the real robot.
| private static RobotConfig getRobotConfig() { | ||
| try { | ||
| return RobotConfig.fromGUISettings(); | ||
| } catch (IOException | ParseException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add fallback configuration for RobotConfig loading failures.
The current implementation throws a RuntimeException if configuration loading fails, which could prevent the robot from starting. Consider providing a safe default configuration.
Apply this diff to add fallback handling:
private static RobotConfig getRobotConfig() {
try {
return RobotConfig.fromGUISettings();
} catch (IOException | ParseException e) {
- throw new RuntimeException(e);
+ System.err.println("Failed to load robot config from GUI: " + e.getMessage());
+ // Return a default configuration to allow robot to function
+ return new RobotConfig(
+ 70, // Robot mass in kg
+ 6.0, // Moment of inertia
+ new RobotConfig.ModuleConfig(
+ 0.05, // Wheel radius in meters
+ 4.5, // Max module speed
+ 1.2, // Coefficient of friction
+ new DCMotor(12, 5.0, 2.0, 2.0, 1, 1), // Drive motor
+ 6.75, // Drive current limit
+ new DCMotor(12, 5.0, 2.0, 2.0, 1, 1), // Steer motor
+ 20 // Steer current limit
+ ),
+ 0.5, 0.5 // Module locations
+ );
}
}📝 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.
| private static RobotConfig getRobotConfig() { | |
| try { | |
| return RobotConfig.fromGUISettings(); | |
| } catch (IOException | ParseException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } | |
| private static RobotConfig getRobotConfig() { | |
| try { | |
| return RobotConfig.fromGUISettings(); | |
| } catch (IOException | ParseException e) { | |
| System.err.println("Failed to load robot config from GUI: " + e.getMessage()); | |
| // Return a default configuration to allow robot to function | |
| return new RobotConfig( | |
| 70, // Robot mass in kg | |
| 6.0, // Moment of inertia | |
| new RobotConfig.ModuleConfig( | |
| 0.05, // Wheel radius in meters | |
| 4.5, // Max module speed | |
| 1.2, // Coefficient of friction | |
| new DCMotor(12, 5.0, 2.0, 2.0, 1, 1), // Drive motor | |
| 6.75, // Drive current limit | |
| new DCMotor(12, 5.0, 2.0, 2.0, 1, 1), // Steer motor | |
| 20 // Steer current limit | |
| ), | |
| 0.5, 0.5 // Module locations | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/constants/AutonomousConstants.java around
lines 94 to 100, the getRobotConfig method currently throws a RuntimeException
when RobotConfig.fromGUISettings() fails; replace that by catching the
IOException/ParseException, log the failure (with the exception) and return a
safe default RobotConfig instead of rethrowing so the robot can still start; use
an existing RobotConfig.getDefault() or construct a conservative default config
(safe speeds, timeouts, disabled/autonomous flags as appropriate) and return it.
Summary by CodeRabbit
New Features
Bug Fixes
Chores