Algae Commands #17
Algae Commands #17
Conversation
|
Warning Rate limit exceeded@Strflightmight09 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughReplaces PathPlannerConstants with AutonomousConstants, adds AlgaeManipulationCommands and new operator triggers/bindings, moves arm/elevator to an armelevator package, extends field/simulation for algae (including ALGAE_ON_FIELD), adds flippable Pose3d utility, and introduces flippable/override arm command helpers and related state changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Driver
participant Operator as OperatorConstants
participant Algae as AlgaeManipulationCommands
participant Swerve
participant Arm as ArmElevator
participant Intake
note over Driver,Operator: Floor algae collection flow
Driver->>Operator: FLOOR_ALGAE_COLLECTION_TRIGGER
Operator->>Algae: getFloorAlgaeCollectionCommand()
activate Algae
Algae->>Intake: start collection
Algae->>Arm: set algae-collection state
Algae->>Swerve: drive/align as needed
Algae-->>Operator: await score selection / complete
deactivate Algae
sequenceDiagram
autonumber
actor Driver
participant Operator as OperatorConstants
participant Algae as AlgaeManipulationCommands
participant ReefChooser
participant Swerve
participant Arm as ArmElevator
note over Driver,Operator: Reef algae collection flow
Driver->>Operator: REEF_ALGAE_COLLECTION_TRIGGER
Operator->>Algae: getReefAlgaeCollectionCommand()
activate Algae
Algae->>ReefChooser: select clock-face & scoring pose
Algae->>Swerve: align to reef target
Algae->>Arm: set algae-collection state
Algae-->>Operator: await stop/score trigger
deactivate Algae
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
32-38: PID constants are all zeros - needs calibration.Both simulation and real robot PID constants for translation and rotation are set to (0, 0, 0), which means the autonomous path following won't work properly.
These PID constants need to be tuned for both simulation and real hardware:
AUTO_TRANSLATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ? - new PIDConstants(0, 0, 0) : - new PIDConstants(0, 0, 0), + new PIDConstants(5.0, 0, 0) : // Example values for simulation + new PIDConstants(4.5, 0, 0.1), // Example values for real robot AUTO_ROTATION_PID_CONSTANTS = RobotHardwareStats.isSimulation() ? - new PIDConstants(0, 0, 0) : - new PIDConstants(0, 0, 0); + new PIDConstants(4.0, 0, 0) : // Example values for simulation + new PIDConstants(3.5, 0, 0.2); // Example values for real robot
🧹 Nitpick comments (12)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
86-87: Address the TODO comment for command registration.The
registerCommands()method is empty with a TODO. This should be implemented to register named commands for autonomous routines.Would you like me to help implement the command registration or create an issue to track this task?
src/main/java/frc/trigon/robot/commands/commandclasses/GamePieceAutoDriveCommand.java (1)
50-50: Verify the logging output correctness.The X distance logging uses
getSetpoint().positionfrom the profiled PID controller. This logs the setpoint position rather than the actual distance, which might be misleading.Consider logging the actual distance instead:
-Logger.recordOutput("GamePieceAutoDrive/XDistanceFromTrackedGamePiece", AutonomousConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position); +Logger.recordOutput("GamePieceAutoDrive/XDistanceSetpoint", AutonomousConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position);src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
8-18: Consider extracting common LED animation parameters.Both
CORAL_COLLECTION_CONFIRMATION_ANIMATION_SETTINGSandALGAE_COLLECTION_CONFIRMATION_ANIMATION_SETTINGSshare the same flow speed (0.5) and direction (false). Consider extracting these as named constants to improve maintainability.public class LEDConstants { //TODO: Implement LEDConstants + private static final double COLLECTION_CONFIRMATION_FLOW_SPEED = 0.5; + private static final boolean COLLECTION_CONFIRMATION_FORWARD_DIRECTION = false; + public static LEDStripAnimationSettings.ColorFlowSettings CORAL_COLLECTION_CONFIRMATION_ANIMATION_SETTINGS = new LEDStripAnimationSettings.ColorFlowSettings( Color.kGreen, - 0.5, - false + COLLECTION_CONFIRMATION_FLOW_SPEED, + COLLECTION_CONFIRMATION_FORWARD_DIRECTION ), ALGAE_COLLECTION_CONFIRMATION_ANIMATION_SETTINGS = new LEDStripAnimationSettings.ColorFlowSettings( Color.kPurple, - 0.5, - false + COLLECTION_CONFIRMATION_FLOW_SPEED, + COLLECTION_CONFIRMATION_FORWARD_DIRECTION );src/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
44-48: Consider extracting game piece check into a reusable predicate.Multiple triggers use
RobotContainer.ARM::hasGamePieceas a condition. Consider extracting this into a named trigger or predicate for better maintainability and clarity.+ private static final Trigger HAS_GAME_PIECE_TRIGGER = new Trigger(RobotContainer.ARM::hasGamePiece); + public static final Trigger - SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or((DRIVER_CONTROLLER.rightStick().and(RobotContainer.ARM::hasGamePiece))), - SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or((DRIVER_CONTROLLER.leftStick().and(RobotContainer.ARM::hasGamePiece))), - EJECT_ALGAE_TRIGGER = OPERATOR_CONTROLLER.p().and(RobotContainer.ARM::hasGamePiece), - STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER = DRIVER_CONTROLLER.x().and(RobotContainer.ARM::hasGamePiece), + SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or(DRIVER_CONTROLLER.rightStick().and(HAS_GAME_PIECE_TRIGGER)), + SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or(DRIVER_CONTROLLER.leftStick().and(HAS_GAME_PIECE_TRIGGER)), + EJECT_ALGAE_TRIGGER = OPERATOR_CONTROLLER.p().and(HAS_GAME_PIECE_TRIGGER), + STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER = DRIVER_CONTROLLER.x().and(HAS_GAME_PIECE_TRIGGER), ALGAE_COLLECTION_TRIGGER = DRIVER_CONTROLLER.a().or(OPERATOR_CONTROLLER.a());
44-48: Remove redundant parentheses for cleaner code.The outer parentheses in the
.and()expressions are unnecessary and can be removed.public static final Trigger - SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or((DRIVER_CONTROLLER.rightStick().and(RobotContainer.ARM::hasGamePiece))), - SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or((DRIVER_CONTROLLER.leftStick().and(RobotContainer.ARM::hasGamePiece))), + SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or(DRIVER_CONTROLLER.rightStick().and(RobotContainer.ARM::hasGamePiece)), + SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or(DRIVER_CONTROLLER.leftStick().and(RobotContainer.ARM::hasGamePiece)), EJECT_ALGAE_TRIGGER = OPERATOR_CONTROLLER.p().and(RobotContainer.ARM::hasGamePiece), STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER = DRIVER_CONTROLLER.x().and(RobotContainer.ARM::hasGamePiece), ALGAE_COLLECTION_TRIGGER = DRIVER_CONTROLLER.a().or(OPERATOR_CONTROLLER.a());src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
29-34: Fix typo in method name:getAlgaeCollectionCommandCommandThe method name has a redundant "Command" suffix. It should be
getAlgaeCollectionCommandfor consistency with other command factory methods.Apply this diff to fix the method name:
- public static Command getAlgaeCollectionCommandCommand() { + public static Command getAlgaeCollectionCommand() {Note: You'll also need to update the reference to this method in
RobotContainer(line 116 ofRobotContainer.java).
103-111: Use switch expression for clearer intentThe cascade of if statements can be replaced with a switch expression for better readability.
Apply this refactor for improved clarity:
private static int getAlgaeScoreMethodSelector() { - if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) - return 1; - if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) - return 2; - if (OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean()) - return 3; - return 0; + return switch (true) { + case true when OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean() -> 1; + case true when OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean() -> 2; + case true when OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean() -> 3; + default -> 0; + }; }Actually, since switch on boolean expressions isn't directly supported, keep the current implementation but consider using an enum for better type safety:
public enum AlgaeScoreMethod { HOLD(0), SCORE_NET(1), SCORE_PROCESSOR(2), EJECT(3); public final int value; AlgaeScoreMethod(int value) { this.value = value; } }
115-116: Address the TODO: Scoring level dependencyThe TODO indicates that the collection state should depend on the target scoring level, but it's currently hardcoded to L3.
The command is currently hardcoded to collect from L3. Would you like me to implement dynamic level selection based on the target scoring level, or open an issue to track this requirement?
134-153: Potential NPE if pose estimator returns nullThe method doesn't check if
getEstimatedRobotPose()returns null before accessing its translation.Add null safety check:
private static FlippablePose2d calculateClosestAlgaeCollectionPose() { + final Pose2d robotPose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + if (robotPose == null) { + // Return a default pose or handle appropriately + return new FlippablePose2d(new Pose2d(), false); + } - final Translation2d robotPositionOnField = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); + final Translation2d robotPositionOnField = robotPose.getTranslation();
141-141: Uninitialized pose could cause issuesThe
closestScoringPoseis initialized with an empty constructor, which may not represent a valid pose if no closer pose is found (though this is unlikely given the loop logic).Initialize with a more meaningful default or use
Optional:- Pose2d closestScoringPose = new Pose2d(); + Pose2d closestScoringPose = null; for (final Rotation2d targetRotation : reefClockAngles) { final Pose2d reefCenterAtTargetRotation = new Pose2d(reefCenterPosition, targetRotation); final Pose2d currentScoringPose = reefCenterAtTargetRotation.transformBy(reefCenterToBranchScoringPose); final double distanceFromCurrentScoringPoseMeters = currentScoringPose.getTranslation().getDistance(robotPositionOnField); if (distanceFromCurrentScoringPoseMeters < distanceFromClosestScoringPoseMeters) { distanceFromClosestScoringPoseMeters = distanceFromCurrentScoringPoseMeters; closestScoringPose = currentScoringPose; } } + + if (closestScoringPose == null) { + // This should never happen if REEF_CLOCK_ANGLES is non-empty + throw new IllegalStateException("No closest scoring pose found"); + }
73-73: Verify the commented-out condition logicThe
//comments suggest incomplete implementation forSHOULD_SCORE_AUTONOMOUSLYcheck.The autonomous scoring condition is commented out. Do you need this condition implemented? I can help add the
SHOULD_SCORE_AUTONOMOUSLYflag and properly integrate it into the command logic.Also applies to: 85-85
32-32: Track TODOs for implementationMultiple TODOs indicate incomplete functionality that should be tracked.
I noticed these TODOs in the implementation:
- Line 32: "add coral unloading if needed"
- Line 115: "make this depend on the target scoring level"
Would you like me to open GitHub issues to track these requirements?
Also applies to: 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/CommandConstants.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/GamePieceAutoDriveCommand.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/constants/LEDConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.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(5 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(27-189)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-119)
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)
src/main/java/frc/trigon/robot/RobotContainer.java (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(27-167)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-50)
src/main/java/frc/trigon/robot/commands/commandclasses/GamePieceAutoDriveCommand.java (3)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-119)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (16)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-119)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(15-87)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-50)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(12-66)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-257)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-189)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableRotation2d.java (1)
FlippableRotation2d(9-67)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-119)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/frc/trigon/robot/subsystems/swerve/Swerve.java (3)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
SwerveModuleConstants(16-125)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
SwerveConstants(19-110)
src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(25-88)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)
🔇 Additional comments (33)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (2)
25-25: LGTM! The class rename from PathPlannerConstants to AutonomousConstants improves semantic clarity.The rename better reflects the class's broader purpose beyond just PathPlanner configuration, as it contains general autonomous functionality.
29-29: New DRIVE_TO_REEF_CONSTRAINTS looks reasonable for reef operations.The constraints (2.5 m/s velocity, 4 m/s² acceleration, 450°/s angular velocity, 900°/s² angular acceleration) appear appropriate for precise reef positioning. These conservative values should provide good control during algae manipulation.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (2)
12-12: LGTM! Import path updated correctly.The import change from
PathPlannerConstantstoAutonomousConstantsis consistent with the rename.
71-71: Current limit reference updated correctly.The drive motor current limit now correctly references
AutonomousConstants.ROBOT_CONFIG.moduleConfig.driveCurrentLimit.src/main/java/frc/trigon/robot/commands/CommandConstants.java (2)
10-10: Import updated consistently.The import change aligns with the project-wide migration to
AutonomousConstants.
48-48: Module locations reference updated correctly.The wheel radius characterization command now correctly uses
AutonomousConstants.ROBOT_CONFIG.moduleLocations.src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (2)
11-11: Import updated consistently.The import aligns with the rename from
PathPlannerConstantstoAutonomousConstants.
47-59: Swerve configuration correctly references AutonomousConstants.All references to robot configuration (kinematics, max speeds) have been properly updated to use
AutonomousConstants.ROBOT_CONFIG.src/main/java/frc/trigon/robot/commands/commandclasses/GamePieceAutoDriveCommand.java (2)
9-9: Import updates are consistent.Both the main import and the removal of
PathPlannerConstantsalign with the project-wide migration.Also applies to: 14-14
56-59: PID controller references updated correctly.All references to the game piece auto-drive PID controllers and the intake open check distance have been properly migrated to use
AutonomousConstants.Also applies to: 67-67
src/main/java/frc/trigon/robot/constants/FieldConstants.java (5)
35-35: Make TAG_ID_TO_POSE immutable and narrow the exposed type; rename the ignore list.Public mutable collections invite accidental mutation. Expose
Map<Integer, Pose3d>and return an unmodifiable map. Also, renameI_HATE_YOUto a professional name.
[ suggest_essential_refactor ]Apply:
- private static final List<Integer> I_HATE_YOU = List.of( + private static final List<Integer> IGNORED_TAG_IDS = List.of( //Tags to ignore ); @@ - public static final HashMap<Integer, Pose3d> TAG_ID_TO_POSE = fieldLayoutToTagIDToPoseMap(); + public static final Map<Integer, Pose3d> TAG_ID_TO_POSE = fieldLayoutToTagIDToPoseMap(); @@ - 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 (!IGNORED_TAG_IDS.contains(aprilTag.ID)) tagIDToPose.put(aprilTag.ID, aprilTag.pose.transformBy(TAG_OFFSET)); - return tagIDToPose; + return java.util.Collections.unmodifiableMap(tagIDToPose); }Also applies to: 47-54
25-25: Avoid exposing a mutable array for REEF_CLOCK_ANGLES.Expose via an accessor that returns a defensive copy, or publish an unmodifiable List.
[ suggest_recommended_refactor ]Minimal change:
- public static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); + private static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); + public static Rotation2d[] getReefClockAngles() { return REEF_CLOCK_ANGLES.clone(); }
24-24: Confirm angle direction matches field convention.
calculateClockAngle()yields 0, -60, -120, … degrees (clockwise). WPILibRotation2dis CCW-positive. Verify downstream math expects clockwise offsets.
[ request_verification ]
26-31: Provide alliance-aware reef center helpers.
BLUE_REEF_CENTER_TRANSLATIONis blue-only. Add a helper that flips for red viaFlippingUtil.
[ suggest_optional_refactor ]Example:
+ public static Translation2d getAllianceReefCenter() { + return FlippingUtil.flipFieldPosition(BLUE_REEF_CENTER_TRANSLATION); + }
58-64:isFacingDriverStationis hardcoded true for all positions—likely incorrect.Some reef faces should be false. Set accurate values or derive from field geometry.
[ raise_minor_issue ]
[ request_verification ]src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
108-111: Good addition: telemetry for hasGamePiece.
@AutoLogOutput("Arm/HasGamePiece")is appropriate and consistent with existing logs.
[ approve_code_changes ]src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
46-50: Verify LEDCommands overload; single-arg call may not compile.
LEDCommands.getAnimateCommand(settings)requires LED strips or a board in the visible API. If no single-arg overload exists, pass the LED targets or add an overload.
[ raise_minor_issue ]
[ request_verification ]Run to confirm:
#!/bin/bash rg -nC2 -P 'class\s+LEDCommands\b' lib | sed -n '1,160p' rg -nP '\bgetAnimateCommand\s*\(' libsrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (2)
78-80: LGTM: simple, correct tolerance check.
atState()uses meters andTOLERANCE_METERS; fits the new state flow.
[ approve_code_changes ]
48-54: Fix logging units: meters vs rotations.
linearPosition(Units.Meters.of(getPositionRotations()))should use meters.
[ raise_minor_issue ]- .linearPosition(Units.Meters.of(getPositionRotations())) + .linearPosition(Units.Meters.of(getPositionMeters()))src/main/java/frc/trigon/robot/RobotContainer.java (2)
95-96: Good: initialize AutonomousConstants before using AutoBuilder.
AutonomousConstants.init()ininitializeGeneralSystems()precedesbuildAutoChooser().
[ approve_code_changes ]
[ request_verification ]
If you ever reorder, keepinit()beforeAutoBuilder.buildAutoChooser().
105-106: Consider renaming API to avoid “CommandCommand”.
AlgaeManipulationCommands.getAlgaeCollectionCommandCommand()is awkward. PrefergetAlgaeCollectionCommand(); update call sites accordingly.
[ suggest_nitpick_refactor ]
[ request_verification ]Check definition/callers:
#!/bin/bash rg -nP '\bgetAlgaeCollectionCommandCommand\b' -C2 rg -nP 'class\s+AlgaeManipulationCommands\b' -C3src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (6)
16-16: LGTM! Import refactoring properly reflects architectural change.The change from
PathPlannerConstantstoAutonomousConstantsis consistently applied throughout the file.
28-29: LGTM! Logging imports properly organized.Good job organizing the AutoLogOutput and Logger imports together for better code organization.
38-38: LGTM! Consistent use of AutonomousConstants for module count.The DriveFeedforwards initialization correctly uses the new constant location.
160-160: Verify FEEDFORWARD_SCALAR value and units.The feedforward scaling uses
AutonomousConstants.FEEDFORWARD_SCALAR. According to the constants file, this is set to 0.5 with a TODO comment indicating it needs calibration.Since this value directly affects path following performance, please ensure it's properly calibrated before deployment.
194-194: LGTM! Consistent module count reference in resetSetpoint.The method correctly uses the new constant location for module count.
35-35: Verify ROBOT_CONFIG compatibility in setpointGenerator.ast-grep returned no matches for a ROBOT_CONFIG declaration — confirm AutonomousConstants.ROBOT_CONFIG exposes the same fields/types that were provided by PathPlannerConstants.ROBOT_CONFIG and that it matches what SwerveSetpointGenerator expects. Location: src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java — initializer:
private final SwerveSetpointGenerator setpointGenerator = new SwerveSetpointGenerator(AutonomousConstants.ROBOT_CONFIG, SwerveModuleConstants.MAXIMUM_MODULE_ROTATIONAL_SPEED_RADIANS_PER_SECOND);src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
245-247: LGTM! Well-structured algae manipulation states.The new algae states (
COLLECT_ALGAE_FROM_REEF,COLLECT_ALGAE_GROUND,EJECT_ALGAE) provide clear separation of concerns for different collection sources and ejection. The voltage values appropriately differentiate between intake (-4V) and ejection (4V) operations.src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (4)
5-8: Good practice: Explicit imports improve code clarity.Replacing wildcard imports with explicit imports for Phoenix6 signal types improves code readability and makes dependencies more transparent.
113-119: Style improvement: Consistent formatting of ternary operators.The added spacing around ternary operators improves readability while maintaining the same functionality.
179-179: Verify SCORE_PROCESSOR position (0.605 m) vs REST_WITH_ALGAE (0.603 m).
2 mm difference is within the 0.05 m (5 cm) tolerance — these states may be indistinguishable; increase separation, tighten/document the tolerance, or confirm the 2 mm is intentional.
Files: src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (enum) and src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (usage).
95-95: Verify TOLERANCE_METERS (0.05 m) is appropriate for all elevator states.TOLERANCE_METERS is defined in src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java and used in src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java as the position-complete check; confirm 0.05 m meets accuracy requirements for SCORE_PROCESSOR (0.605 m) and other targets — if not, tighten it or add state-specific tolerances.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
42-46: Unsafe controller input access in lambdaThe lambda expressions directly access
OperatorConstants.DRIVER_CONTROLLER.getLeftY()and.getLeftX()which will only evaluate once when the supplier is created, not continuously during command execution.Wrap the controller methods in the lambda to ensure they're evaluated on each call:
- () -> fieldRelativePowersToSelfRelativeXPower(OperatorConstants.DRIVER_CONTROLLER.getLeftY(), OperatorConstants.DRIVER_CONTROLLER.getLeftX()), + () -> fieldRelativePowersToSelfRelativeXPower(() -> OperatorConstants.DRIVER_CONTROLLER.getLeftY(), () -> OperatorConstants.DRIVER_CONTROLLER.getLeftX()),Also update the method signature:
- private static double fieldRelativePowersToSelfRelativeXPower(double xPower, double yPower) { + private static double fieldRelativePowersToSelfRelativeXPower(DoubleSupplier xPower, DoubleSupplier yPower) { final Rotation2d robotHeading = RobotContainer.SWERVE.getDriveRelativeAngle(); - final double xValue = CommandConstants.calculateDriveStickAxisValue(xPower); - final double yValue = CommandConstants.calculateDriveStickAxisValue(yPower); + final double xValue = CommandConstants.calculateDriveStickAxisValue(xPower.getAsDouble()); + final double yValue = CommandConstants.calculateDriveStickAxisValue(yPower.getAsDouble());Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
|
//TODO: have algae collection just be one button, and the mode depending on the target scoring level |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (3)
176-186: Replace magic number 0.603 with the named constant.Leverage SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS to keep enum targets DRY and consistent.
- REST(0.603, 0.603, 0.7), + REST(SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, + SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, 0.7), @@ - COLLECT_ALGAE_FROM_L2(0.603, 0.603, 1), + COLLECT_ALGAE_FROM_L2(SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, + SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, 1), @@ - REST_WITH_ALGAE(0.603, 0.603, 0.3), + REST_WITH_ALGAE(SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, + SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, 0.3),
105-105: Sim limit sensor never trips.REVERSE_LIMIT_SENSOR_SIMULATION_SUPPLIER returns 0, so auto‑zero on the reverse limit can’t be exercised in sim. If you plan to test zeroing in simulation, expose a setter or tie this to ElevatorSimulation state.
I can sketch a small sim hook that asserts the limit when rotations <= 0. Would you like that?
88-90: Scope/rename ambiguous TOLERANCE_METERSElevatorConstants defines HEIGHT_TOLERANCE_METERS = 0.01 and TOLERANCE_METERS = 0.05 (src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java:106). LEDAutoSetupCommand also declares TOLERANCE_METERS = 0.1 (src/main/java/frc/trigon/robot/commands/commandclasses/LEDAutoSetupCommand.java:22). These separate symbols cause ambiguity — make intent explicit by renaming.
Proposed non-breaking rename in ElevatorConstants:
- static final double TOLERANCE_METERS = 0.05; + static final double GOAL_TOLERANCE_METERS = 0.05;Also consider renaming the LED constant to LED_TOLERANCE_METERS (or similarly scoped name) if it’s unrelated.
Run to find/update usages:
rg -nP '\bTOLERANCE_METERS\b' -C2src/main/java/frc/trigon/robot/constants/FieldConstants.java (2)
29-32: Prefer grouping XY offsets as Translation2d to prevent mismatch.Two doubles are easier to desync at call sites. Keep the doubles for back‑compat, but add typed offsets now.
Add alongside the constants:
public static final Translation2d REEF_CENTER_TO_TARGET_SCORING_OFFSET = new Translation2d( REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS, REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS); public static final Translation2d REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_OFFSET = new Translation2d( REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS, REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_Y_TRANSFORM_METERS);
25-27: Document angle convention and ordering to avoid misuse.State explicitly that 0 deg is 12 o’clock and positions step clockwise by −60°: [0, −60, −120, −180, −240, −300].
Add brief Javadoc:
/** * Angle step between reef clock positions (60°). * Convention: 0° at 12 o'clock; positive is CCW (WPILib). We step CW by using negative multiples. */ public static final Rotation2d REEF_CLOCK_POSITION_DIFFERENCE = ...And on calculateClockAngle():
/** Returns 0°, -60°, -120°, ... by stepping CW from 12 o'clock. */ private Rotation2d calculateClockAngle() { return REEF_CLOCK_POSITION_DIFFERENCE.times(-ordinal()); }Please confirm this matches how alignment expects angles (CW vs CCW).
Also applies to: 111-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
- src/main/java/frc/trigon/robot/RobotContainer.java
- src/main/java/frc/trigon/robot/constants/AutonomousConstants.java
- src/main/java/frc/trigon/robot/constants/OperatorConstants.java
- src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
87-87: No-op formatting change.Blank line is fine; no action needed.
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
28-28: Confirm BLUE_REEF_CENTER_TRANSLATION against the 2025 welded layout.4.48945 m looks precise; just sanity‑check it matches the current WPILib field origin and flipping convention.
If helpful, print it in sim and validate that FLIPPABLE_REEF_CENTER_TRANSLATION resolves correctly for both alliances at runtime.
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
247-249: EJECT_ALGAE at 90° sits on the safety boundary; add margin.MAXIMUM_ARM_SAFE_ANGLE is 90° (Line 118) and ANGLE_TOLERANCE is 5° (Line 119). Targeting exactly 90° risks chattering/violations. Nudge down a few degrees.
- EJECT_ALGAE(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90), 4); + EJECT_ALGAE(Rotation2d.fromDegrees(85), Rotation2d.fromDegrees(85), 4);Please verify in sim that intake ejection still clears reliably at 85°.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (3)
155-158: Include eject in the “button pressed” helper (if kept).If you keep isScoreAlgaeButtonPressed(), include the eject trigger.
- return OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean() || - OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean(); + return OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean() || + OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean() || + OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean();
36-51: Auto‑align race end conditions: consider deadlineWith for clearer intent.Using raceWith(new WaitCommand(1).andThen(WaitUntil(hasGamePiece))) ends alignment as soon as either finishes, which may be 1s timeout + hasGamePiece early. If the desired behavior is “drive until hasGamePiece or overridden,” use deadlineWith to make the waits the deadline.
- ).raceWith( - new WaitCommand(1).andThen(new WaitUntilCommand(RobotContainer.ARM::hasGamePiece)), - new WaitUntilCommand(OperatorConstants.STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER) - ); + ).deadlineWith( + new WaitCommand(1).andThen(new WaitUntilCommand(RobotContainer.ARM::hasGamePiece)) + ).raceWith( + new WaitUntilCommand(OperatorConstants.STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER) + );
29-34: Verified enums present; prefer composing the score command instead of scheduling
- Verified: ArmConstants.ArmState.COLLECT_ALGAE_FROM_REEF and ElevatorConstants.ElevatorState.COLLECT_ALGAE_FROM_L3 exist (AlgaeManipulationCommands.java lines 113–116). getScoreAlgaeCommand().schedule() is invoked inside an InstantCommand (lines 29–34).
- Recommendation: remove the InstantCommand(() -> getScoreAlgaeCommand().schedule()) and compose getScoreAlgaeCommand() directly into the command group (e.g., include it in the Sequential/Parallel group or use andThen/alongWith) to avoid manual scheduling side‑effects and lifecycle/race issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (16)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-118)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)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-199)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableRotation2d.java (1)
FlippableRotation2d(9-67)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/AlgaeManipulationCommands.java
[error] 115-115: Compilation error: cannot find symbol ArmConstants.ArmState.COLLECT_ALGAE_FROM_REEF. (likely missing enum value or incorrect reference)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
53-63: Simplify SelectCommand control flow; avoid raceWith + repeatedly + custom WaitUntilChange.The current chain is brittle and can terminate unexpectedly. Track selector changes and use a single until condition.
- ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::isScoreAlgaeButtonPressed)).repeatedly().until(() -> !RobotContainer.ARM.hasGamePiece()); + ).until(() -> !RobotContainer.ARM.hasGamePiece() || hasScoreMethodChanged());Add the following helpers (anywhere private static is appropriate in this class):
// Helper state for selector change detection private static int lastScoreMethod = -1; private static boolean hasScoreMethodChanged() { final int current = getAlgaeScoreMethodSelector(); final boolean changed = current != lastScoreMethod; lastScoreMethod = current; return changed; }
31-33: Don’t schedule inside an InstantCommand; include the command directly.Scheduling breaks lifecycle/interrupt semantics of the group. Compose directly.
- getInitiateAlgaeCollectionCommand().until(RobotContainer.ARM::hasGamePiece), - new InstantCommand(() -> getScoreAlgaeCommand().schedule()).alongWith(getAlgaeCollectionConfirmationCommand()) //TODO: add coral unloading if needed + getInitiateAlgaeCollectionCommand().until(RobotContainer.ARM::hasGamePiece), + getScoreAlgaeCommand().alongWith(getAlgaeCollectionConfirmationCommand()) //TODO: add coral unloading if needed
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
88-88: Two tolerance constants — de-dupe or clarify scope.
HEIGHT_TOLERANCE_METERS = 0.01 (used in Elevator.atTargetState / atPreparedTargetState — src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java:86,91) vs TOLERANCE_METERS = 0.05 (defined src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java:106); there's also a local TOLERANCE_METERS = 0.1 in src/main/java/frc/trigon/robot/commands/commandclasses/LEDAutoSetupCommand.java:22. Consolidate into a single clearly-named constant (e.g., POSITION_TOLERANCE_METERS) or rename to express distinct semantics (e.g., HEIGHT_POSITION_TOLERANCE_METERS vs SAFETY_TOLERANCE_METERS); remove any unused duplicate.src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
238-252: New ArmState entries look reasonable; align naming with Elevator and confirm voltage directions
- Minor consistency nit: Arm uses FLOOR while Elevator uses GROUND for L1 algae collection. Pick one (recommend GROUND) to avoid confusion across subsystems.
- Verify end-effector voltage signs: LOAD/collect use negative, unload/score/eject use positive. Confirm these match hardware polarity.
Apply if you want naming consistency:
- COLLECT_ALGAE_FLOOR(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0), -4), + COLLECT_ALGAE_GROUND(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0), -4),And update references (ScoringLevel.determineArmAlgaeCollectionState, any call sites).
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
26-26: Avoid exposing REEF_CHOOSER; use OperatorConstants.REEF_CHOOSER directlyMaking this package‑private to let other factories reach into it increases coupling. Keep it private here and have other classes use OperatorConstants.REEF_CHOOSER.
- static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER; + private static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER;Outside this file (AlgaeManipulationCommands), switch to OperatorConstants.REEF_CHOOSER (see its review comment).
195-203: Algae elevator state mapping: consider naming consistency GROUND vs FLOORMinor: Elevator uses COLLECT_ALGAE_GROUND while Arm uses COLLECT_ALGAE_FLOOR. Standardize to one term across subsystems to reduce operator/docs ambiguity.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (4)
104-112: Default scoring method should follow target level (single‑button UX in PR objectives)When no manual override is pressed, pick by level: L4→Net, L3/L2→Processor, L1→Eject.
- private static int getAlgaeScoreMethodSelector() { - if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) - return 1; - if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) - return 2; - if (OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean()) - return 3; - return 0; - } + private static int getAlgaeScoreMethodSelector() { + if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) return 1; + if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) return 2; + if (OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean()) return 3; + // Default by target level (REEF_CHOOSER) + return switch (OperatorConstants.REEF_CHOOSER.getScoringLevel()) { + case L4 -> 1; // Net + case L3, L2 -> 2; // Processor + case L1 -> 3; // Eject + }; + }
114-118: Avoid reaching through CoralPlacingCommands; use OperatorConstants.REEF_CHOOSER directlyThis removes cross‑factory coupling and lets you keep REEF_CHOOSER private in CoralPlacingCommands.
- return new ParallelCommandGroup( - ArmCommands.getSetTargetStateCommand(CoralPlacingCommands.REEF_CHOOSER.getArmAlgaeCollectionState()), - ElevatorCommands.getSetTargetStateCommand(CoralPlacingCommands.REEF_CHOOSER.getElevatorAlgaeCollectionState()) - ); + return new ParallelCommandGroup( + ArmCommands.getSetTargetStateCommand(OperatorConstants.REEF_CHOOSER.getArmAlgaeCollectionState()), + ElevatorCommands.getSetTargetStateCommand(OperatorConstants.REEF_CHOOSER.getElevatorAlgaeCollectionState()) + );
66-75: Optional: avoid per‑cycle allocations for fixed target headingsMinor GC nit: wrap the FlippableRotation2d constants once and capture in a lambda, rather than constructing new instances every loop.
// At top of file private static final FlippableRotation2d NET_HEADING = new FlippableRotation2d(Rotation2d.k180deg, true); private static final FlippableRotation2d PROCESSOR_HEADING = new FlippableRotation2d(Rotation2d.kCW_90deg, true); // In lambdas () -> NET_HEADING () -> PROCESSOR_HEADINGAlso applies to: 78-87
29-35: Rename getAlgaeCollectionCommandCommand → getAlgaeCollectionCommandRename the method declaration in src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (currently at the top of the file) and update the RobotContainer binding in src/main/java/frc/trigon/robot/RobotContainer.java (line ~104):
-OperatorConstants.ALGAE_COLLECTION_TRIGGER.whileTrue(AlgaeManipulationCommands.getAlgaeCollectionCommandCommand()); +OperatorConstants.ALGAE_COLLECTION_TRIGGER.whileTrue(AlgaeManipulationCommands.getAlgaeCollectionCommand());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(8 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (12)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-118)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)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/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(21-70)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-229)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (5)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-201)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-263)
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-201)
🔇 Additional comments (9)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (2)
90-90: Replace hard-coded 6.8 rotations with geometry-derived limit; clamp SCORE_NET to MAX-ε.6.8 mechanism rotations ≈ 1.71 m (> MAXIMUM_ELEVATOR_HEIGHT_METERS = 1.382 m). This risks overtravel if the soft limit is honored. Derive limits from drum geometry and avoid commanding exactly the soft limit.
Apply:
@@ - static final double DRUM_DIAMETER_METERS = DRUM_RADIUS_METERS * 2; + static final double DRUM_DIAMETER_METERS = DRUM_RADIUS_METERS * 2; + // Unit conversions between mechanism rotations and linear travel. + static double rotationsToMeters(double rotations) { + return rotations * (Math.PI * DRUM_DIAMETER_METERS); + } + static double metersToRotations(double meters) { + return meters / (Math.PI * DRUM_DIAMETER_METERS); + } @@ - config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = + metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS); @@ - SCORE_NET(1.382, 1.382, 0.3), + SCORE_NET(MAXIMUM_ELEVATOR_HEIGHT_METERS - HEIGHT_TOLERANCE_METERS, + MAXIMUM_ELEVATOR_HEIGHT_METERS - HEIGHT_TOLERANCE_METERS, 0.3),Quick sanity check:
#!/bin/bash python - <<'PY' import math R=0.04 circ=2*math.pi*R print("6.8 rots -> meters:", 6.8*circ) print("Max height meters:", 1.382) print("Max height -> rots:", 1.382/circ) PYAlso applies to: 138-140, 188-189
178-187: Enum renames/additions for algae — verify call sites & replace magic 0.603 with constant
- File: src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (≈ lines 178–187). Ensure any old names (COLLECT_ALGAE_FROM_*) are updated to the new enum values.
- Replace repeated 0.603 with SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS.
Suggested diff:
- COLLECT_ALGAE_L2(0.603, 0.603, 1), + COLLECT_ALGAE_L2(SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, + SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS, 1),
Confirm ground at 0 m is safe: ensure software safety gating prevents bottoming when the arm is in unsafe poses.
Verify call sites (run from repo root and paste results):
rg -nP '\b(COLLECT_ALGAE_FROM_(L2|L3|GROUND)|COLLECT_ALGAE_(L2|L3|GROUND|LOLLIPOP))\b' --hidden --no-ignore-vcs -SIf rg reports "No files were searched" or yields nothing, run:
git grep -nE 'COLLECT_ALGAE_FROM_|COLLECT_ALGAE_' || rg -n 'COLLECT_ALGAE_'src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
40-42: Good migration to coral‑specific state suppliersUsing REEF_CHOOSER coral getters clarifies intent and keeps scoring flows cohesive.
Also applies to: 50-52, 58-60
137-164: State grouping per material is clear; good encapsulationSplitting elevator/arm states into coral vs algae collection reads well and prevents cross‑mixing. No change requested.
215-223: Algae arm state mapping: LGTMMapping L1→FLOOR, L2→L2, L3→L3, L4→LOLLIPOP matches the PR objective.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (3)
129-133: Confirmation feedback: LGTMRumble + LED animation on successful collection is a nice touch; composition is safe and bounded.
29-35: Don’t schedule child commands inside an InstantCommand; compose them directlyScheduling breaks lifecycle/interrupt semantics of the enclosing group. Compose the scoring command directly.
- new InstantCommand(() -> getScoreAlgaeCommand().schedule()).alongWith(getAlgaeCollectionConfirmationCommand()) //TODO: add coral unloading if needed + getScoreAlgaeCommand().alongWith(getAlgaeCollectionConfirmationCommand()) //TODO: add coral unloading if neededAlso consider renaming this method for clarity:
- public static Command getAlgaeCollectionCommandCommand() { + public static Command getAlgaeCollectionCommand() {Update RobotContainer binding accordingly (see verification below).
54-64: Selector change detection ignores EJECT; scoring mode may never update to EJECTThe race condition tracks only net/processor presses, so switching to EJECT won’t retrigger selection. Track the selector int instead.
- ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::isScoreAlgaeButtonPressed)).repeatedly().until(() -> !RobotContainer.ARM.hasGamePiece()); + ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::getAlgaeScoreMethodSelector)) + .repeatedly() + .until(() -> !RobotContainer.ARM.hasGamePiece());Optionally, drop repeated().raceWith() complexity and use a simple state-change helper; happy to provide if you prefer.
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
48-62: LGTM — verify no remaining getArmState/getElevatorState usagesNew getters look good; I couldn't confirm in the sandbox (ripgrep skipped files). Run locally to ensure no old generic getters remain:
rg -nP '\bgetArmState\s*(|\bgetElevatorState\s*(' -tjava -C2
git grep -n -e 'getArmState(' -e 'getElevatorState(' || trueIf none found, mark resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
29-35: Rename method: getAlgaeCollectionCommandCommand → getAlgaeCollectionCommandTypo/verbosity in the public API. Recommend a simple rename for clarity.
- public static Command getAlgaeCollectionCommandCommand() { + public static Command getAlgaeCollectionCommand() {Note: Update RobotContainer binding accordingly (see separate comment).
156-159: If you keep this helper, include EJECT; otherwise remove it after adopting selector-based changeAs written, EJECT_ALGAE_TRIGGER is ignored. Either remove this method (preferred after the change above) or fix it.
- private static boolean isScoreAlgaeButtonPressed() { - return OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean() || - OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean(); - } + private static boolean isScoreAlgaeButtonPressed() { + return OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean() + || OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean() + || OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean(); + }
37-52: Consider removing the 1s delay before honoring hasGamePiece
new WaitCommand(1).andThen(new WaitUntilCommand(RobotContainer.ARM::hasGamePiece))defers stopping auto-align for 1s even if collection completes immediately, potentially fighting the scoring drive for ~1s.Swap to a direct WaitUntil to end alignment as soon as the piece is detected (you already debounce collection at the sensor level).
- ).raceWith( - new WaitCommand(1).andThen(new WaitUntilCommand(RobotContainer.ARM::hasGamePiece)), - new WaitUntilCommand(OperatorConstants.STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER) - ) + ).raceWith( + new WaitUntilCommand(RobotContainer.ARM::hasGamePiece), + new WaitUntilCommand(OperatorConstants.STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER) + )
104-112: Map “single button” behavior to target level by default (manual triggers override)Per PR objective, default the scoring method when no manual override is pressed:
- L4 → net
- L3/L2 → processor
- L1 → eject
private static int getAlgaeScoreMethodSelector() { - if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) + // Manual overrides + if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) return 1; if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) return 2; if (OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean()) return 3; - return 0; + // Default based on target scoring level + return switch (CoralPlacingCommands.REEF_CHOOSER.getTargetScoringLevel()) { + case L4 -> 1; // net + case L3, L2 -> 2; // processor + case L1 -> 3; // eject + }; }If
getTargetScoringLevel()differs, tweak the call accordingly.
135-154: Duplicate pose-search logic with CoralPlacingCommands.calculateClosestScoringPoseThis repeats the “iterate reef angles, pick nearest, apply transform” pattern. Consider extracting a shared helper to avoid drift in transforms/tolerances.
161-167: Math OK; consider y component too for fine approachThe field→self X projection is correct. If you later want lateral nudging during align, expose Y as well.
66-76: No persistent swerve “tug‑of‑war”; optional explicit gating recommended.
- getAlignToReefCommand is proxied and raced with a WaitUntil(RobotContainer.ARM::hasGamePiece), so alignment is cancelled when the arm acquires a piece; scoring is scheduled by the InstantCommand only after that event and scoring swerve commands are proxied too — the proxied/explicit scheduling means the scoring command will take the SWERVE requirement (interrupting the align command) rather than causing ongoing alternating drive outputs. (frcdocs.wpi.edu)
- Optional (recommended): make the intent explicit to avoid any single‑frame overlap — gate align with !RobotContainer.ARM.hasGamePiece() and gate scoring drives with RobotContainer.ARM.hasGamePiece(), or set a short “alignEnded” flag. Files: src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (getAlignToReefCommand, getScoreInNetCommand, getScoreInProcessorCommand).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T17:39:27.429Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.429Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (13)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-118)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)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-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-201)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-229)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
33-34: Instant schedule pattern OK here; prevent duplicate re-schedules from whileTrueUsing InstantCommand(() -> cmd.schedule()) to decouple lifecycles is fine (per your stated preference). However, when bound with whileTrue, the outer group can finish quickly (esp. when auto-align is disabled) and be re-scheduled repeatedly, causing repeated schedule() calls for the scoring command.
Switch the binding to onTrue so scoring is scheduled once per press.
Outside this file (RobotContainer.java):
- OperatorConstants.ALGAE_COLLECTION_TRIGGER.whileTrue(AlgaeManipulationCommands.getAlgaeCollectionCommandCommand()); + OperatorConstants.ALGAE_COLLECTION_TRIGGER.onTrue(AlgaeManipulationCommands.getAlgaeCollectionCommand());Please confirm in sim/logs that no “command already scheduled” warnings appear when holding the trigger.
54-64: Make selection reactive to ALL methods (incl. Eject); watch selector value, not a booleanCurrently the “reselect” race watches only net/processor triggers; pressing Eject won’t trigger a reselection, so the SelectCommand may not switch to the ejection branch until another event occurs. Track the integer selector instead so any change (hold/net/processor/eject) restarts the select.
- ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::isScoreAlgaeButtonPressed)).repeatedly().until(() -> !RobotContainer.ARM.hasGamePiece()); + ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::getAlgaeScoreMethodSelector)) + .repeatedly() + .until(() -> !RobotContainer.ARM.hasGamePiece());
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (7)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-118)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-201)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(32-263)
🔇 Additional comments (11)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (11)
50-52: Using coral-specific state suppliers + reversal supplier — LGTM
60-62: Manual score path updated to coral-specific states — LGTM
68-70: Prepare path aligned to coral states with reversal — LGTM
137-143: State split (coral vs algae) in ScoringLevel fields is a good API evolution
160-164: Constructor initializes all new state fields — LGTM
185-193: determineElevatorCoralState mapping is correct per levels
195-203: Algae collection elevator mapping matches PR objective (L1=floor, L2/L3 levels, L4=lollipop)
205-213: determineArmCoralState mapping — LGTM
215-223: Algae collection arm mapping aligns with intended behaviors
26-26: Do not make REEF_CHOOSER private — it's referenced outside this class.
AlgaeManipulationCommands uses CoralPlacingCommands.REEF_CHOOSER (src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java:115-116); making it private would break those call sites. Leave visibility or provide a controlled accessor/alternative.Likely an incorrect or invalid review comment.
113-114: Use preallocated Rotation2d constants
WPILib’s Rotation2d class provides static preallocated constants k180deg and kZero; prefer these over creating new Rotation2d instances or using fromDegrees to avoid unnecessary allocations. (github.wpilib.org)Likely an incorrect or invalid review comment.
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: 1
🧹 Nitpick comments (6)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (6)
28-28: Consider renaming the redundant method nameThe method name
getAlgaeCollectionCommandCommand()has redundant "Command" suffix repetition.- public static Command getAlgaeCollectionCommandCommand() { + public static Command getAlgaeCollectionCommand() {
43-46: Complex self-relative drive calculationThe nested function calls for converting field-relative to self-relative powers may introduce calculation errors. Consider extracting this logic to a dedicated helper method for clarity and testability.
SwerveCommands.getClosedLoopSelfRelativeDriveCommand( - () -> fieldRelativePowersToSelfRelativeXPower(OperatorConstants.DRIVER_CONTROLLER.getLeftY(), OperatorConstants.DRIVER_CONTROLLER.getLeftX()), - () -> 0, + AlgaeManipulationCommands::getSelfRelativeXPower, + AlgaeManipulationCommands::getSelfRelativeYPower, () -> calculateClosestAlgaeCollectionPose().getRotation() ) + private static double getSelfRelativeXPower() { + return fieldRelativePowersToSelfRelativeXPower( + OperatorConstants.DRIVER_CONTROLLER.getLeftY(), + OperatorConstants.DRIVER_CONTROLLER.getLeftX() + ); + } + + private static double getSelfRelativeYPower() { + // Y power is intentionally zero for reef alignment + return 0; + }
48-49: Race condition with multiple wait conditionsThe command races with both a time-based wait AND a game piece detection. If the 1-second timer expires first, the game piece wait never executes. Consider using
raceWithon individual conditions or documenting the intended priority.).raceWith( - new WaitCommand(1).andThen(new WaitUntilCommand(RobotContainer.ARM::hasGamePiece)), + new WaitCommand(1), // Minimum alignment time + new WaitUntilCommand(RobotContainer.ARM::hasGamePiece), // Stop aligning when piece detected new WaitUntilCommand(OperatorConstants.STOP_ALGAE_AUTO_ALIGN_OVERRIDE_TRIGGER) - ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY).asProxy(); + ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY).asProxy();
56-60: Magic numbers in selector mappingThe integer values (0, 1, 2, 3) used for the
SelectCommandmapping should be constants or an enum for better maintainability.+ private static final int HOLD_MODE = 0; + private static final int SCORE_NET_MODE = 1; + private static final int SCORE_PROCESSOR_MODE = 2; + private static final int EJECT_MODE = 3; + private static Command getScoreAlgaeCommand() { return new SelectCommand<>( Map.of( - 0, getHoldAlgaeCommand(), - 1, getScoreInNetCommand(), - 2, getScoreInProcessorCommand(), - 3, getEjectAlgaeCommand() + HOLD_MODE, getHoldAlgaeCommand(), + SCORE_NET_MODE, getScoreInNetCommand(), + SCORE_PROCESSOR_MODE, getScoreInProcessorCommand(), + EJECT_MODE, getEjectAlgaeCommand() ), AlgaeManipulationCommands::getAlgaeScoreMethodSelector private static int getAlgaeScoreMethodSelector() { if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) - return 1; + return SCORE_NET_MODE; if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) - return 2; + return SCORE_PROCESSOR_MODE; if (OperatorConstants.EJECT_ALGAE_TRIGGER.getAsBoolean()) - return 3; - return 0; + return EJECT_MODE; + return HOLD_MODE; }
134-153: Extract pose calculation constantsThe pose calculation logic contains inline transformations and magic values that should be extracted for better maintainability.
private static FlippablePose2d calculateClosestAlgaeCollectionPose() { final Translation2d robotPositionOnField = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); final Translation2d reefCenterPosition = new FlippableTranslation2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, true).get(); final Rotation2d[] reefClockAngles = FieldConstants.REEF_CLOCK_ANGLES; final Transform2d reefCenterToBranchScoringPose = new Transform2d(FieldConstants.REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS, 0, new Rotation2d()); - double distanceFromClosestScoringPoseMeters = Double.POSITIVE_INFINITY; - Pose2d closestScoringPose = new Pose2d(); + Pose2d closestScoringPose = findClosestReefPose(robotPositionOnField, reefCenterPosition, reefClockAngles, reefCenterToBranchScoringPose); + + return new FlippablePose2d(closestScoringPose.rotateBy(CoralPlacingCommands.shouldReverseScore() ? Rotation2d.k180deg : new Rotation2d()), false); + } + + private static Pose2d findClosestReefPose(Translation2d robotPosition, Translation2d reefCenter, Rotation2d[] clockAngles, Transform2d centerToPose) { + double minDistance = Double.POSITIVE_INFINITY; + Pose2d closestPose = new Pose2d(); - for (final Rotation2d targetRotation : reefClockAngles) { - final Pose2d reefCenterAtTargetRotation = new Pose2d(reefCenterPosition, targetRotation); - final Pose2d currentScoringPose = reefCenterAtTargetRotation.transformBy(reefCenterToBranchScoringPose); - final double distanceFromCurrentScoringPoseMeters = currentScoringPose.getTranslation().getDistance(robotPositionOnField); - if (distanceFromCurrentScoringPoseMeters < distanceFromClosestScoringPoseMeters) { - distanceFromClosestScoringPoseMeters = distanceFromCurrentScoringPoseMeters; - closestScoringPose = currentScoringPose; + for (final Rotation2d targetRotation : clockAngles) { + final Pose2d reefCenterAtTargetRotation = new Pose2d(reefCenter, targetRotation); + final Pose2d currentPose = reefCenterAtTargetRotation.transformBy(centerToPose); + final double distance = currentPose.getTranslation().getDistance(robotPosition); + if (distance < minDistance) { + minDistance = distance; + closestPose = currentPose; } } - - return new FlippablePose2d(closestScoringPose.rotateBy(CoralPlacingCommands.shouldReverseScore() ? Rotation2d.k180deg : new Rotation2d()), false); + return closestPose; }
130-130: TODO comment indicates incomplete LED implementationThe TODO comment suggests LED functionality is not fully implemented yet.
Would you like me to help implement the LED animation for algae collection confirmation or open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T17:39:27.429Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.429Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (18)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-118)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-115)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-66)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-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-201)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableRotation2d.java (1)
FlippableRotation2d(9-67)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(21-70)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-229)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (3)
31-32: Good decoupling patternThis follows the previously acknowledged pattern of using
InstantCommandwithschedule()to decouple command lifecycles, allowing the score command to run independently.
115-117: Dynamic state selection correctly implements PR objectivesThe implementation properly uses the REEF_CHOOSER to get the appropriate arm and elevator states based on the selected scoring level, aligning with the PR objective of having collection behavior depend on the target scoring level.
33-33: Verify .asProxy() lifecycle when used with .alongWith().asProxy() detaches a command from its parent; composing a proxied command with .alongWith(getAlignToReefCommand()) (src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java) can change cancellation/termination semantics. Confirm getAlignToReefCommand() does not return an asProxy() — if it does, return a non-proxied command for this composition or restructure so the parent retains lifecycle control.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/constants/OperatorConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/constants/OperatorConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
26-26: Avoid drift: derive the step angle from enum length (not a hard-coded count).Computing from ReefClockPosition.values().length keeps the math correct if enum entries change.
- 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(Conversions.DEGREES_PER_ROTATIONS / ReefClockPosition.values().length);src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (1)
47-47: Fix logging: label says X distance, but you log the controller setpointLog the actual X distance; if you also want the setpoint, log it under a distinct key.
-Logger.recordOutput("GamePieceAutoDrive/XDistanceFromTrackedGamePiece", AutonomousConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position); +Logger.recordOutput("GamePieceAutoDrive/XDistanceFromTrackedGamePiece", distanceFromTrackedGamePiece.getX()); +Logger.recordOutput("GamePieceAutoDrive/XControllerSetpoint", AutonomousConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position);src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
36-37: Add a timeout to avoid hanging if detection never flips.Chain a timeout to the load step so the robot doesn’t stall if the distance sensor doesn’t trip.
- ).until(RobotContainer.ARM::hasGamePiece); + ).until(RobotContainer.ARM::hasGamePiece).withTimeout(2.0);src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
106-108: Consolidate tolerance constants.TOLERANCE_METERS (0.05) and ELEVATOR_POSITION_TOLERANCE_METERS (0.02) overlap semantically. Consider a single clearly named tolerance per purpose to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/frc/trigon/robot/constants/OperatorConstants.java
- src/main/java/frc/trigon/robot/RobotContainer.java
- src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (3)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-123)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-123)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/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)
🔇 Additional comments (11)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
27-27: Do not expose a mutable array via public API; make it private and return a clone.Public static final arrays are mutable; a stray write corrupts global state.
- public static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); + private static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles();Add an accessor elsewhere in this class:
public static Rotation2d[] getReefClockAngles() { return REEF_CLOCK_ANGLES.clone(); }Check and update call sites that reference FieldConstants.REEF_CLOCK_ANGLES:
#!/bin/bash rg -nP --type=java -C3 '\bFieldConstants\.REEF_CLOCK_ANGLES\b'
29-32: Add the missing Y transform for algae (or alias to scoring Y).Callers likely expect both X and Y; currently only X is defined.
public static final double REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS = 1.3, REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS = 0.17, - REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6; + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6, + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_Y_TRANSFORM_METERS = REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS;
28-28: LGTM: Center translation is clear and flippable-friendly.Using field width/2 for Y pairs well with FlippableTranslation2d.
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (2)
9-9: Migration to AutonomousConstants looks rightSwitching to AutonomousConstants improves cohesion and centralizes tuning.
64-64: Complete the TODO: require intake be in COLLECT before advancing toward the game pieceMatch prior guidance: drive only if intake is actively collecting.
-return distanceFromTrackedGamePiece != null && - (distanceFromTrackedGamePiece.getNorm() > AutonomousConstants.AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS);//TODO: If intake is open +return distanceFromTrackedGamePiece != null + && distanceFromTrackedGamePiece.getNorm() > AutonomousConstants.AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS + && RobotContainer.INTAKE.atState(IntakeConstants.IntakeState.COLLECT);Add import:
import frc.trigon.robot.subsystems.intake.IntakeConstants;src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
121-124: Telemetry key rename: ensure dashboards/consumers updated (or add a temporary alias).The rename to "Arm/HasGamePiece" is good. If anything still reads "Arm/HasCoral", update it or add a temporary alias to avoid breaking dashboards.
Optionally add this alias in updatePeriodically:
Logger.recordOutput("Arm/HasCoral", hasGamePiece());src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
70-70: LGTM: LED confirmation animation selection.Correct constant usage; matches LEDConstants.
39-44: Unload can release before arm is positioned; add a prepare phase and timeouts.As previously noted, set a prepare angle first, then enable eject to avoid premature release. Also add timeouts for robustness.
- public static Command getUnloadCoralCommand() { - return new ParallelCommandGroup( - ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.UNLOAD_CORAL), - ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.UNLOAD_CORAL) - ).until(() -> !RobotContainer.ARM.hasGamePiece()); - } + public static Command getUnloadCoralCommand() { + return new SequentialCommandGroup( + new ParallelCommandGroup( + ArmCommands.getPrepareForStateCommand(ArmConstants.ArmState.UNLOAD_CORAL), + ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.UNLOAD_CORAL) + ).until(RobotContainer.ARM::atPrepareAngle).withTimeout(1.0), + new ParallelCommandGroup( + ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.UNLOAD_CORAL), + ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.UNLOAD_CORAL) + ).until(() -> !RobotContainer.ARM.hasGamePiece()).withTimeout(1.5) + ); + }src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (2)
88-91: Derive forward soft limit from meters (don’t hard‑code 6.8 rotations).This was previously flagged and still applies. Use drum geometry to derive the limit from MAXIMUM_ELEVATOR_HEIGHT_METERS to prevent overtravel.
@@ - static final double DRUM_DIAMETER_METERS = DRUM_RADIUS_METERS * 2; + static final double DRUM_DIAMETER_METERS = DRUM_RADIUS_METERS * 2; + + // Unit conversions between mechanism rotations (post SensorToMechanismRatio) and linear travel. + static double rotationsToMeters(double rotations) { + return rotations * (2 * Math.PI * DRUM_RADIUS_METERS); + } + static double metersToRotations(double meters) { + return meters / (2 * Math.PI * DRUM_RADIUS_METERS); + }And update the config to use the derived limit:
@@ - config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; - config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = + metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS);
176-191: LGTM on new states; confirm cross‑module consistency.Added UNLOAD_CORAL, COLLECT_ALGAE_LOLLIPOP, SCORE_PROCESSOR, and renamed algae states look consistent. Ensure all command factories/choosers reference the updated names.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
242-242: Align naming: GROUND vs FLOOR for algae collection.ArmState uses COLLECT_ALGAE_FLOOR while ElevatorState uses COLLECT_ALGAE_GROUND. Pick one term repo‑wide to avoid confusion (recommend “GROUND”).
Run to locate references:
Also applies to: 251-256
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
25-26: Guard against drift between REEF_CLOCK_POSITIONS and enum countAdd a runtime check to fail fast if someone changes the enum without updating the constant. Avoid computing from values() in a field initializer to prevent static-init cycles.
Add this static block anywhere after the static fields:
static { int count = ReefClockPosition.values().length; if (REEF_CLOCK_POSITIONS != count) { throw new IllegalStateException("REEF_CLOCK_POSITIONS (" + REEF_CLOCK_POSITIONS + ") != ReefClockPosition.values().length (" + count + ")"); } }src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
176-191: Sanity-check enum heights against soft limits.SCORE_NET is at 1.382 m (top). With the new soft limit using MAX - tolerance, ensure controllers can settle without fighting the limit. Consider setting SCORE_NET slightly below MAX or increasing HEIGHT_TOLERANCE_METERS if needed.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (3)
102-112: Net scoring reversal heuristic is stubbed.shouldReverseNetScore() always false; verify desired behavior before field testing.
145-153: Looping on lollipop toggle is fine; consider deadline vs race if you need to hold until choice changes.Current race+repeatedly is acceptable; a ParallelDeadlineGroup with WaitUntilChange can be clearer.
207-213: Compute Y component too or clarify intent.Helper only returns self-relative X. If you need Y elsewhere, add symmetrical Y conversion to avoid duplicating math.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
111-115: Rotation flip on reverse is correctUsing 180° flip when reversing aligns the branch-facing pose. Optionally use Rotation2d.kZero for symmetry over new Rotation2d().
- shouldReverseScore() ? Rotation2d.k180deg : new Rotation2d() + shouldReverseScore() ? Rotation2d.k180deg : Rotation2d.kZerosrc/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
29-33: Global button-state flags are fine; document semanticsThese are used for mutual exclusion between left/right score buttons. Consider a brief comment on intended behavior and reset semantics.
81-98: createScoreTrigger works; minor gating nitFlag updates (onTrue/onFalse) occur on the base stick trigger before algae/coral gating, which may lock out the opposite stick even when the gated trigger won’t fire. If that’s undesirable, apply the algae/coral predicate before the onTrue/onFalse handlers.
- if (isAlgaeCommand) - return scoreTrigger.and(() -> AlgaeManipulationCommands.isHoldingAlgae()); - return scoreTrigger.and(() -> !AlgaeManipulationCommands.isHoldingAlgae()); + final Trigger gated = isAlgaeCommand + ? scoreTrigger.and(AlgaeManipulationCommands::isHoldingAlgae) + : scoreTrigger.and(() -> !AlgaeManipulationCommands.isHoldingAlgae()); + return gated;Then attach onTrue/onFalse to gated instead of the base stick trigger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(8 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java(2 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(2 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-21T17:39:27.460Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.460Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (7)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (4)
src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-99)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-267)
src/main/java/frc/trigon/robot/RobotContainer.java (6)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-99)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(18-78)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(30-218)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)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(17-117)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-99)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-226)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)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-202)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-267)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (7)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-99)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-117)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-202)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-267)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(30-218)
🔇 Additional comments (25)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (2)
27-27: Avoid exposing a mutable array; make it private and return a cloneApply:
- public static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); + private static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles();Add accessor (outside this hunk):
public static Rotation2d[] getReefClockAngles() { return REEF_CLOCK_ANGLES.clone(); }Remember to update call sites to use FieldConstants.getReefClockAngles().
29-32: Add missing algae Y transform (or explicitly alias it)Many call sites expect both X and Y. If algae Y equals scoring Y, alias it; otherwise set the measured value.
public static final double REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS = 1.3, REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS = 0.17, - REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6; + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6, + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_Y_TRANSFORM_METERS = REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS;src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
139-141: Replace hard-coded forward soft limit (6.8 rotations) with derived value from max height.6.8 rotations ≈ 1.71 m (with 0.04 m drum radius), which can exceed MAXIMUM_ELEVATOR_HEIGHT_METERS (1.382 m). Compute the limit from meters to avoid overtravel.
Apply:
@@ - config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; - config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = 6.8; + config.SoftwareLimitSwitch.ForwardSoftLimitEnable = true; + // Keep a small headroom so targets at MAX don't trip the soft-limit edge. + config.SoftwareLimitSwitch.ForwardSoftLimitThreshold = metersToRotations(MAXIMUM_ELEVATOR_HEIGHT_METERS - HEIGHT_TOLERANCE_METERS);Add helpers (near other constants):
@@ static final double HEIGHT_TOLERANCE_METERS = 0.01; static final double SECOND_ELEVATOR_COMPONENT_EXTENDED_LENGTH_METERS = 0.603; static final double DRUM_DIAMETER_METERS = DRUM_RADIUS_METERS * 2; + // Unit conversions between mechanism rotations (post SensorToMechanismRatio) and linear travel. + static double rotationsToMeters(double rotations) { + return rotations * (2 * Math.PI * DRUM_RADIUS_METERS); + } + static double metersToRotations(double meters) { + return meters / (2 * Math.PI * DRUM_RADIUS_METERS); + }src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
29-34: Load coral sequence end condition looks good.Ending when the arm detects a game piece is appropriate for this phase.
src/main/java/frc/trigon/robot/RobotContainer.java (2)
95-96: Good: initialize AutonomousConstants once at startup.This ensures PathPlanner setup and command registration before bindings.
103-114: Bindings look coherent with new algae workflows.Toggle-on-true for algae collection and explicit lollipop toggle are consistent with the flows introduced.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (2)
155-167: This depends on dynamic flip override—ensure GeneralCommands fix is applied.Once getFlippableOverridableArmCommand uses Suppliers, arm reversal will update live during collection.
186-200: Pose generation looks sound.Using nearest reef branch and flipping 180° on reverse is reasonable.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (11)
26-26: Making REEF_CHOOSER package-private is fineVisibility here makes sense given cross-package usage.
51-53: Correct state selection for autonomous scoringUsing REEF_CHOOSER coral states and shouldReverseScore supplier is appropriate.
61-63: LGTM: manual scoring path mirrors autonomous state targetsConsistent with autonomous flow; only the gating condition differs.
69-71: LGTM: prepare commands now use coral target statesPrepare routing matches the execute routing; good cohesion.
138-144: New per-level state fields are sensibleSplitting coral vs algae collection states improves clarity for command factories.
161-164: Eagerly computing per-level mappings is finePrecomputing in the enum constructor avoids runtime branching in hot paths.
186-194: Elevator coral state mapping per level looks correctL1→SCORE_L1, …, L4→SCORE_L4 matches constants.
196-202: Elevator algae collection mapping is consistentL1–L2→COLLECT_ALGAE_L2, L3–L4→COLLECT_ALGAE_L3 aligns with constants.
204-212: Arm coral state mapping per level looks correctState selection matches ArmConstants per level.
214-220: Arm algae collection mapping is consistentL1–L2→COLLECT_ALGAE_L2, L3–L4→COLLECT_ALGAE_L3 as intended.
36-45: Fix compile-time constant and keep the correct 90° thresholdRotation2d.kCW_90deg doesn’t exist in WPILib. Use a 90.0-degree literal (or Rotation2d.fromDegrees(90)) and retain the absolute heading-error check.
Apply:
static boolean shouldReverseScore() { - final Pose2d robotPose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); - final Rotation2d robotRotation = robotPose.getRotation(); - final Translation2d robotTranslation = robotPose.getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return Math.abs(robotRotationFacingReef.getDegrees()) > Rotation2d.kCW_90deg.getDegrees(); + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get() + .minus(pose.getTranslation()) + .getAngle(); + final double headingErrorDeg = pose.getRotation().minus(toReef).getDegrees(); + return Math.abs(headingErrorDeg) > 90.0; }Optional: add hysteresis (e.g., enter >95°, exit <85°) to avoid chatter near the threshold.
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (6)
43-43: POV-up for reset heading is reasonableMatches common driver ergonomics.
47-47: Continue trigger composition looks goodCombines both sticks or operator 'k'; confirm this matches driver workflow.
56-57: Algae scoring via right/left score buttons is clearRight→net, left→processor is intuitive given the createScoreTrigger gating.
63-65: Utility triggers are fineStart to flip arm and equals for sim spawn are acceptable defaults.
53-61: Decouple STOP_REEF_ALGAE_ALIGN_TRIGGER from RobotContainer to break the cycleUse an algae-held predicate or move this trigger to RobotContainer. Minimal decoupling fix below uses AlgaeManipulationCommands.isHoldingAlgae().
- STOP_REEF_ALGAE_ALIGN_TRIGGER = DRIVER_CONTROLLER.x().and(RobotContainer.ARM::hasGamePiece), + STOP_REEF_ALGAE_ALIGN_TRIGGER = DRIVER_CONTROLLER.x().and(AlgaeManipulationCommands::isHoldingAlgae),Alternative (preferred): define STOP_REEF_ALGAE_ALIGN_TRIGGER in RobotContainer where ARM is available, and keep OperatorConstants free of subsystem references.
6-6: Remove RobotContainer import to avoid circular static initializationOperatorConstants is referenced by RobotContainer, and importing RobotContainer here (used below in STOP_REEF_ALGAE_ALIGN_TRIGGER) reintroduces a circular dependency that can cause static init issues.
Apply:
-import frc.trigon.robot.RobotContainer;Then decouple STOP_REEF_ALGAE_ALIGN_TRIGGER from RobotContainer (see next comment).
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java
Outdated
Show resolved
Hide resolved
| REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS = 0.17, | ||
| REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6; | ||
|
|
||
| public static final FlippablePose2d FLIPPABLE_PROCESSOR_SCORE_POSE = new FlippablePose2d(0, 0, Rotation2d.kCW_90deg, true);//TODO: Find values |
There was a problem hiding this comment.
🧩 Analysis chain
Don’t ship a placeholder pose at (0,0); verify Rotation2d.kCW_90deg exists
- A public constant with (0,0) can route the robot to field origin if accidentally used. Either keep it private behind a getter that throws until measured, or ensure there are no usages yet.
- Also confirm WPILib exposes Rotation2d.kCW_90deg; if not, use Rotation2d.fromDegrees(-90).
If kCW_90deg is unavailable, apply:
- public static final FlippablePose2d FLIPPABLE_PROCESSOR_SCORE_POSE = new FlippablePose2d(0, 0, Rotation2d.kCW_90deg, true);//TODO: Find values
+ public static final FlippablePose2d FLIPPABLE_PROCESSOR_SCORE_POSE = new FlippablePose2d(0, 0, Rotation2d.fromDegrees(-90), true); // TODO: measure real pose before useSearch for accidental usages:
Confirm the WPILib constant:
🏁 Script executed:
#!/bin/bash
rg -nP --type=java -C3 '\bFLIPPABLE_PROCESSOR_SCORE_POSE\b'Length of output: 2388
🌐 Web query:
Does WPILib (2025 season) provide Rotation2d.kCW_90deg (or similar) as a predefined constant?
💡 Result:
Yes — WPILib's Rotation2d includes predefined constants such as kCW_90deg (also kCCW_90deg, kZero, kPi, etc.). [1][2]
Sources:
[1] WPILib Rotation2d API (fields list).
[2] WPILib geometry docs (Rotation2d description).
Make FLIPPABLE_PROCESSOR_SCORE_POSE private/unusable until real values are set
- Currently used in AlgaeManipulationCommands (src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java:119,121); exposing a (0,0) pose risks routing the robot to the field origin. Wrap it behind a getter that throws (or remove usages) until measured.
- Rotation2d.kCW_90deg is a valid WPILib constant—no change needed there.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/constants/FieldConstants.java around line 50,
FLIPPABLE_PROCESSOR_SCORE_POSE is a public constant set to (0,0) and must not be
usable until real values are measured; make the field private and provide a
public getter that throws an IllegalStateException (with a clear message) if the
pose is still the placeholder, or remove/replace usages. Then update
AlgaeManipulationCommands
(src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
lines ~119 and ~121) to stop referencing the constant directly — either call the
new getter inside a try/catch and handle the exception by skipping/aborting the
command or refactor to use a measured pose passed in from a configuration or
test stub.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Show resolved
Hide resolved
|
Todo: add auto algae scoring |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
102-112: Net scoring heading: implement shouldReverseNetScore()Heading is hardcoded (TODO). Reuse the existing reef-facing heuristic for now.
Apply:
- () -> new FlippableRotation2d(shouldReverseNetScore() ? Rotation2d.kZero : Rotation2d.k180deg, true) + () -> new FlippableRotation2d(shouldReverseNetScore() ? Rotation2d.kZero : Rotation2d.k180deg, true)And replace the method body (see lines 141–143).
141-143: Implement shouldReverseNetScore() using current reef-facing heuristicPrevents inconsistent headings during net scoring until a dedicated net heuristic exists.
Apply:
- private static boolean shouldReverseNetScore() {//TODO: Implement - return false; - } + private static boolean shouldReverseNetScore() { + return CoralPlacingCommands.shouldReverseScore(); + }
145-153: Mode toggle loop: consider removing raceWith(WaitUntilChangeCommand) in favor of a simple untilUsing repeatedly().raceWith(WaitUntilChangeCommand) for lollipop vs floor is more complex than needed.
Apply:
- ).raceWith( - new WaitUntilChangeCommand<>(() -> SHOULD_COLLECT_FROM_LOLLIPOP) - ).repeatedly(); + ).until(() -> false).unless(() -> false); // keep running until parent endsOr, if you want to break on toggle changes, mirror the selector-change helper pattern used for scoring.
169-177: Reef collection state tracking is good; consider flippable override parityArm set uses shouldReverseScore(), but doesn’t honor the operator flip override like floor/lollipop flow via GeneralCommands.getFlippableOverridableArmCommand. If intentional, ignore; otherwise, switch to the overridable variant for UX parity.
215-217: Implement reef-relative Y offset for alignment PIDFills the TODO so ALIGN_TO_REEF_Y_CONTROLLER has a meaningful measurement.
Apply:
- private static double calculateReefRelativeYOffset() {//TODO: Implement - return 0; - } + private static double calculateReefRelativeYOffset() { + final Pose2d robot = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Pose2d target = calculateClosestAlgaeCollectionPose().get(); + final Rotation2d branchHeading = target.getRotation(); + final Rotation2d toBranchFrame = new Rotation2d(-branchHeading.getRadians()); + final Translation2d errorInBranchFrame = target.getTranslation() + .minus(robot.getTranslation()) + .rotateBy(toBranchFrame); + return errorInBranchFrame.getY(); + }
34-38: PID constants duplicated for sim/real; consider centralizing in AutonomousConstantsBoth branches use (3,0,0). Move to AutonomousConstants for tuning consistency with other auto PIDs.
63-65: Parallel align runs only when SHOULD_SCORE_AUTONOMOUSLY; consider separate toggle for algaeYou’re piggybacking on CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY. If operator needs independent control for algae auto-align/score, add a dedicated flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-21T17:39:27.460Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.460Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (13)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(17-117)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(13-99)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/FlippableRotation2d.java (1)
FlippableRotation2d(9-67)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(18-78)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-226)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (5)
53-53: Avoid out-of-band scheduling (or document intent) — make floor vs reef behavior consistentFloor flow schedules scoring via InstantCommand, reef flow composes scoring directly. If decoupling is intended, mirror that pattern in reef; otherwise, remove schedule() here for consistent lifecycle management.
Apply if you prefer in-group lifecycle:
- new InstantCommand(() -> getScoreAlgaeCommand().schedule()).alongWith(getAlgaeCollectionConfirmationCommand()) + getScoreAlgaeCommand().asProxy().alongWith(getAlgaeCollectionConfirmationCommand())If you intentionally want “fire-and-forget” here, please add a brief comment clarifying the interruption semantics difference vs reef per operator UX.
84-93: Simplify SelectCommand control flow; track selector changes directlyThe chain raceWith(WaitUntilChangeCommand(isScoreAlgaeButtonPressed)).repeatedly().until(...) is brittle and only watches a boolean, not which score method is selected.
Apply:
- ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::isScoreAlgaeButtonPressed)).repeatedly().until(() -> !RobotContainer.ARM.hasGamePiece() && !isScoreAlgaeButtonPressed()); + ).until(() -> !RobotContainer.ARM.hasGamePiece() || hasScoreMethodChanged());Add helpers (outside this hunk):
private static int lastScoreMethod = -1; private static boolean hasScoreMethodChanged() { final int current = getAlgaeScoreMethodSelector(); if (current != lastScoreMethod) { lastScoreMethod = current; return true; } return false; }
207-213: Field→self transform only returns X; add Y or rename method to reflect X-onlyEither compute both components and return a pair/Translation2d, or add a Y helper for symmetry. Current naming suggests more than X.
Option A (preferred): return both components:
- private static double fieldRelativePowersToSelfRelativeXPower(double xPower, double yPower) { + private static Translation2d fieldRelativePowersToSelfRelative(double xPower, double yPower) { final Rotation2d robotHeading = RobotContainer.SWERVE.getDriveRelativeAngle(); final double xValue = CommandConstants.calculateDriveStickAxisValue(xPower); final double yValue = CommandConstants.calculateDriveStickAxisValue(yPower); - - return (xValue * robotHeading.getCos()) + (yValue * robotHeading.getSin()); + final double selfX = (xValue * robotHeading.getCos()) + (yValue * robotHeading.getSin()); + final double selfY = (-xValue * robotHeading.getSin()) + (yValue * robotHeading.getCos()); + return new Translation2d(selfX, selfY); }Then use .getX() in the align command, and consider using .getY() when manual Y is desired.
Option B: rename to fieldRelativeToSelfRelativeX(...) to clarify scope.
67-82: Y-alignment PID never acts (offset always 0) — implement reef-relative Y errorcalculateReefRelativeYOffset() returns 0, so the PID outputs 0 and alignment can’t correct lateral error.
Apply:
- () -> ALIGN_TO_REEF_Y_CONTROLLER.calculate(calculateReefRelativeYOffset()), + () -> ALIGN_TO_REEF_Y_CONTROLLER.calculate(calculateReefRelativeYOffset()),And implement calculateReefRelativeYOffset() below (see comment at lines 215–217).
186-200: Pose2d.nearest(List) is available in WPILib 2025; no fallback loop needed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/RobotContainer.java (3)
15-16: Avoid wildcard import from commandfactories.Wildcard imports can hide type collisions and make code browsing harder. Prefer explicit imports.
Apply:
-import frc.trigon.robot.commands.commandfactories.*; +import frc.trigon.robot.commands.commandfactories.GeneralCommands; +import frc.trigon.robot.commands.commandfactories.AlgaeManipulationCommands; +import frc.trigon.robot.commands.commandfactories.CoralCollectionCommands; +import frc.trigon.robot.commands.commandfactories.CoralPlacingCommands; +import frc.trigon.robot.commands.commandfactories.CoralEjectionCommands;
100-100: Remove commented binding or add a TODO note.Dead commented code adds noise; either delete or leave a clear TODO explaining when it returns.
103-104: Consolidate to a single algae-collection button (per PR goals).You currently bind separate toggles for floor vs reef. Use a single trigger that selects the command from the target level (via REEF_CHOOSER arm algae collection state).
Apply:
- OperatorConstants.FLOOR_ALGAE_COLLECTION_TRIGGER.toggleOnTrue(AlgaeManipulationCommands.getFloorAlgaeCollectionCommand()); - OperatorConstants.REEF_ALGAE_COLLECTION_TRIGGER.toggleOnTrue(AlgaeManipulationCommands.getReefAlgaeCollectionCommand()); + // Single-button algae control: floor when chooser indicates floor; otherwise reef. + OperatorConstants.REEF_ALGAE_COLLECTION_TRIGGER.toggleOnTrue( + edu.wpi.first.wpilibj2.command.Commands.either( + AlgaeManipulationCommands.getFloorAlgaeCollectionCommand(), + AlgaeManipulationCommands.getReefAlgaeCollectionCommand(), + () -> OperatorConstants.REEF_CHOOSER.getArmAlgaeCollectionState() == frc.trigon.robot.subsystems.arm.ArmConstants.ArmState.COLLECT_ALGAE_FLOOR + ) + );Please verify the chooser/state method names and that both commands self-terminate as intended when the game piece is no longer held. Based on OperatorConstants and AlgaeManipulationCommands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/RobotContainer.java (6)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(18-90)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(34-279)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-207)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/RobotContainer.java (3)
95-95: Good: Initialize PathPlanner via AutonomousConstants early.Ensures AutoBuilder/config is ready before building the chooser. LGTM.
106-106: Coral collection binding: LGTM.Continuous hold-to-run pattern is appropriate here.
109-109: Eject coral binding: LGTM.Fits the expected hold-to-eject behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/RobotContainer.java (4)
15-16: Wildcard factory import is fine; optional: switch to explicit imports for clarityKeeps compile clean; if you want stricter readability and IDE assistance, prefer explicit imports.
100-100: Commented D‑Pad drive bindingIf permanently unused, consider removing to avoid confusion; otherwise leave a TODO explaining why it’s disabled.
103-104: Two separate algae collection toggles; align later with “single button” objectiveWorks for now. To match the PR goal (one button that adapts to target level/score vs eject), plan to consolidate behind a SelectCommand reading the active target level.
112-112: Flip‑override UX: note interaction with getResetFlipArmOverrideCommand()Many algae flows call GeneralCommands.getResetFlipArmOverrideCommand() at start, which will clear this manual toggle. If intentional, add a brief comment; if not, adjust those commands.
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
347-349: Confirm safe motion to COLLECT_ALGAE_LOLLIPOP and add PREPARE variant if needed
70° @ 0.3 m is within MAXIMUM_ARM_SAFE_ANGLE (90°). If approach or exit into this pose feels constraint-limited or jerky, mirror the floor flow by introducing a PREPARE_COLLECT_ALGAE_LOLLIPOP enum entry (ignoreConstraints=true, referenced as prepareState) to ensure a smooth approach/retreat arc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(5 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
- src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/RobotContainer.java (8)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(18-90)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(34-279)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-69)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(17-39)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-208)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)
🔇 Additional comments (4)
src/main/java/frc/trigon/robot/RobotContainer.java (4)
95-96: AutonomousConstants.init() swap looks correctGood move centralizing PathPlanner init into AutonomousConstants.
106-106: Coral collection binding LGTMHolds command while pressed and cancels cleanly.
111-111: Gate coral spawn to simulation onlyThis should not register on real hardware. Wrap the binding with RobotHardwareStats.isSimulation().
Apply:
- OperatorConstants.SPAWN_CORAL_IN_SIMULATION_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())))); + if (frc.trigon.robot.misc.RobotHardwareStats.isSimulation()) { + OperatorConstants.SPAWN_CORAL_IN_SIMULATION_TRIGGER.onTrue( + new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose()))) + ); + }
113-114: A‑button overlap: gate lollipop toggle to only when holding algaeA also selects L1 when not holding algae (see OperatorConstants). Gate this toggle with isHoldingAlgae to avoid unintended mode switches.
Apply:
- OperatorConstants.LOLLIPOP_ALGAE_TOGGLE_TRIGGER.onTrue(new InstantCommand(AlgaeManipulationCommands::toggleLollipopCollection)); + OperatorConstants.LOLLIPOP_ALGAE_TOGGLE_TRIGGER + .and(AlgaeManipulationCommands::isHoldingAlgae) + .onTrue(new InstantCommand(AlgaeManipulationCommands::toggleLollipopCollection));
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: 4
🧹 Nitpick comments (9)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
90-90: UNLOAD_CORAL shares the same voltage/sign as SCORE_CORAL/EJECT — confirm intent or differentiate.If UNLOAD_CORAL is a distinct behavior (backfeed to intake vs front eject), consider using a different magnitude or guard to avoid accidental equivalence in behavior/telemetry.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (5)
97-106: Simplify SelectCommand looping; current raceWith+repeatedly chain is brittle.Prefer a straight loop with an explicit until or a deadline group to avoid premature termination.
Apply (one option):
- ).raceWith(new WaitUntilChangeCommand<>(AlgaeManipulationCommands::isScoreAlgaeButtonPressed)).repeatedly(); + ).repeatedly() + .until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() || !isScoreAlgaeButtonPressed());
98-103: Map “L1” default to an eject, not hold, to match spec.Swap 0→getEjectAlgaeCommand() and add the method below.
Apply:
return new SelectCommand<>( Map.of( - 0, getHoldAlgaeCommand(), + 0, getEjectAlgaeCommand(), 1, getScoreInNetCommand(), 2, getScoreInProcessorCommand() ), AlgaeManipulationCommands::getAlgaeScoreMethodSelector )Add:
+ private static Command getEjectAlgaeCommand() { + return new ParallelCommandGroup( + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.REST_WITH_ALGAE), + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.EJECT) + ); + }
174-177: Unify reversal logic with coral scoring.Delegate to CoralPlacingCommands.shouldReverseScore() for consistency.
Apply:
- private static boolean shouldReverseNetScore() { - final Rotation2d swerveAngle = RobotContainer.SWERVE.getDriveRelativeAngle(); - return swerveAngle.getDegrees() < 90 && swerveAngle.getDegrees() > -90; - } + private static boolean shouldReverseNetScore() { + return CoralPlacingCommands.shouldReverseScore(); + }
179-187: Toggling flow: simplify the race and avoid repeated().One WaitUntilChange is enough; repeated() can re-enter unexpectedly.
Apply:
- ).raceWith( - new WaitUntilChangeCommand<>(() -> SHOULD_COLLECT_FROM_LOLLIPOP) - ).repeatedly(); + ).raceWith(new WaitUntilChangeCommand<>(() -> SHOULD_COLLECT_FROM_LOLLIPOP));
266-272: Helper returns only X component; consider returning both or rename to avoid misuse.Returning a Translation2d would prevent future call‑site mistakes.
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (3)
73-77: Logic OK; prefer clearer XOR and re-verify mid-run flip behavior.Semantics look right and are read live via Supplier. For readability, use != instead of ^.
- ArmElevatorCommands.getPrepareForStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()) : - ArmElevatorCommands.getSetTargetStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()); + ArmElevatorCommands.getPrepareForStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean()) : + ArmElevatorCommands.getSetTargetStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean());Flipping while a command is active will change isStateReversed mid-execution. Please verify on-bot that states with prepare phases (e.g., COLLECT_ALGAE_FLOOR) don’t need the prepare stage to be re-run on flip; otherwise we should wrap with a restart-on-change pattern.
79-83: Add convenience overload for API symmetry.You have (Supplier, isPrepare, shouldStartFlipped) and (ArmElevatorState, isPrepare). Add (Supplier, isPrepare) that defaults to the override supplier.
public static Command getFlippableOverridableArmCommand(Supplier<ArmElevatorConstants.ArmElevatorState> targetState, boolean isPrepareState, BooleanSupplier shouldStartFlipped) { return isPrepareState ? ArmElevatorCommands.getPrepareForStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()) : ArmElevatorCommands.getSetTargetStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()); } + + public static Command getFlippableOverridableArmCommand(Supplier<ArmElevatorConstants.ArmElevatorState> targetState, boolean isPrepareState) { + return isPrepareState + ? ArmElevatorCommands.getPrepareForStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE) + : ArmElevatorCommands.getSetTargetStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE); + }
85-89: Avoid boolean flag parameter; split into two named factories.Boolean flags harm readability at call sites. Consider two methods:
- getFlippableOverridableArmPrepareCommand(...)
- getFlippableOverridableArmSetTargetCommand(...)
If you prefer keeping this API, add brief Javadoc explaining isPrepareState.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(5 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java(2 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-21T17:39:27.460Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.460Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.javasrc/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (21)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(43-127)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-119)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-233)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)src/main/java/lib/utilities/flippable/FlippablePose2d.java (1)
FlippablePose2d(11-58)src/main/java/lib/utilities/flippable/FlippableRotation2d.java (1)
FlippableRotation2d(9-67)src/main/java/lib/utilities/flippable/FlippableTranslation2d.java (1)
FlippableTranslation2d(9-35)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-76)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-203)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(34-279)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)
🔇 Additional comments (15)
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
51-53: Accessor addition LGTM.Publicly exposing the algae-collection elevator state mirrors the existing API and keeps call sites clean.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (3)
26-30: Conditional “fire-and-forget” schedule is OK here.Scheduling load only when not holding algae matches the intended decoupling.
Based on learnings
71-74: LED confirmation change LGTM.Uses the new, specific coral confirmation setting.
42-47: Add a timeout/fallback to avoid spinning forever if sensors disagree.If END_EFFECTOR loses the piece but INTAKE doesn’t detect it, this loop won’t end.
Apply:
public static Command getUnloadCoralCommand() { return new ParallelCommandGroup( ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) - ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()); + ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()) + .withTimeout(2.0); }src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (5)
26-26: Use of OperatorConstants.REEF_CHOOSER is correct.Reduces duplication and matches earlier feedback.
116-124: Fix compile error and inverted logic in shouldReverseScore().Rotation2d.kCW_90deg/kCCW_90deg do not exist; logic should reverse when |heading error| > 90°.
Apply:
- static boolean shouldReverseScore() { - final Rotation2d robotRotation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d robotTranslation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); - } + static boolean shouldReverseScore() { + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get() + .minus(pose.getTranslation()) + .getAngle(); + final double headingErrorDeg = pose.getRotation().minus(toReef).getDegrees(); + return Math.abs(headingErrorDeg) > 90.0; + }
138-141: New algae-collection state on ScoringLevel: good addition.Keeps level→state mapping co-located with scoring state.
159-160: Constructor wiring LGTM.Both elevator states are initialized lazily via helpers.
191-197: Level→algae-collection mapping LGTM.L1/2→L2 and L3/4→L3 makes sense for reach constraints.
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (6)
76-95: Remove .asProxy() — it drops Swerve ownership and risks concurrent drive commands.Keep requirements to avoid conflicts with other driving commands while aligning.
Apply:
).raceWith( new WaitUntilCommand(RobotContainer.END_EFFECTOR::hasGamePiece), new WaitUntilCommand(OperatorConstants.STOP_REEF_ALGAE_ALIGN_TRIGGER) - ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY).asProxy(); + ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY);
115-120: Don’t race net scoring; use the arm sequence as the deadline.Race group can end early when drive finishes, aborting the arm/eject.
Apply:
- return new ParallelRaceGroup( + return new ParallelDeadlineGroup( getArmNetSequenceCommand(AlgaeManipulationCommands::shouldReverseNetScore), getDriveToNetCommand() );
143-155: Remove .asProxy() in net drive; keep Swerve exclusive.Proxying can overlap with other Swerve commands.
Apply:
- ).asProxy().onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY); + ).onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY);
157-164: Same here: drop .asProxy() in processor drive.Avoid requirement loss mid-drive.
Apply:
- ).asProxy() - .until(() -> RobotContainer.SWERVE.atPose(FieldConstants.FLIPPABLE_PROCESSOR_SCORE_POSE)) + ) + .until(() -> RobotContainer.SWERVE.atPose(FieldConstants.FLIPPABLE_PROCESSOR_SCORE_POSE))
166-172: Implement level-based fallback for single-button behavior (PR objective).Defaulting to 0 breaks the “one button by level” flow: L4→net, L3/L2→processor, L1→eject/hold.
Apply:
private static int getAlgaeScoreMethodSelector() { if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) return 1; if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) return 2; - return 0; + return switch (OperatorConstants.REEF_CHOOSER.getScoringLevel()) { + case L4 -> 1; // net + case L3, L2 -> 2; // processor + case L1 -> 0; // eject/hold + }; }
209-215: Remove .asProxy() from lollipop intake assist.Keep intake/transporter requirements to prevent actuator conflicts.
Apply:
- ).asProxy(); + );
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
83-87: Undefined SAFE_DISTANCE_FROM_SCORING_POSE_METERS breaks compilationLine 86 references
SAFE_DISTANCE_FROM_SCORING_POSE_METERS, but that constant is no longer defined anywhere in this class (see the Gradle failure). Please restore the constant (with the intended value) or replace the usage with an existing field/constant so the code compiles again.
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
26-29: Fire-and-forget scheduling is fine; simplify the condition.Current pattern is OK for decoupling lifecycles. You can drop the inline if and use
onlyIf(...)for clarity.Apply:
- new InstantCommand( - () -> { - if (!AlgaeManipulationCommands.isHoldingAlgae()) - getLoadCoralCommand().schedule(); - } - ) + new InstantCommand(() -> getLoadCoralCommand().schedule()) + .onlyIf(() -> !AlgaeManipulationCommands.isHoldingAlgae())Also consider replacing
onlyWhile(!hasGamePiece)ingetLoadCoralCommand()withonlyIf(!hasGamePiece)to avoid redundant gating with.until(hasGamePiece). Based on learnings.
73-73: Bound the LED confirmation so it’s visible and non-blocking.Without a bound, this animation can be canceled immediately by outer
.until(...)or run longer than intended. Tie it to the rumble duration.Apply:
- LEDCommands.getAnimateCommand(LEDConstants.CORAL_COLLECTION_CONFIRMATION_ANIMATION_SETTINGS) //TODO: add LEDs + LEDCommands.getAnimateCommand(LEDConstants.CORAL_COLLECTION_CONFIRMATION_ANIMATION_SETTINGS) + .withTimeout(OperatorConstants.RUMBLE_DURATION_SECONDS) // keeps it brief and consistentAlternatively, schedule the LED in an
InstantCommand(...schedule())if you want it to survive parent cancellation. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(5 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/frc/trigon/robot/misc/ReefChooser.java
- src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java
- src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T17:39:27.460Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.460Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (7)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(34-278)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-367)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-91)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-367)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 86-86: cannot find symbol: SAFE_DISTANCE_FROM_SCORING_POSE_METERS
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
167-175: Reverse-orientation check still inverted and uses nonexistent constantsThe predicate should flip when
|heading error| > 90°, but this implementation returns true only when the error is within ±90°. On top of that,Rotation2d.kCW_90deg/kCCW_90degare not WPILib constants, so this code will not compile. The earlier review comment calling this out still applies—please fix both the logic and the constants.static boolean shouldReverseScore() { - final Rotation2d robotRotation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d robotTranslation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION + .get() + .minus(pose.getTranslation()) + .getAngle(); + final double headingErrorDeg = pose.getRotation().minus(toReef).getDegrees(); + return Math.abs(headingErrorDeg) > 90.0; }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (4)
59-59: Replace “UNTETSTED” banner with TODO + add a quick check.Either remove the banner or add a brief TODO and a unit/sim check ensuring debounce gating actually delays execution.
67-73: Debounced runWhen: logic fixed; simplify construction.Works as intended, but you can avoid setDebounceTime and the extra SequentialCommandGroup.
Apply:
- public static Command runWhen(Command command, BooleanSupplier condition, double debounceTimeSeconds) { - final Debouncer debouncer = new Debouncer(0, Debouncer.DebounceType.kRising); - return new SequentialCommandGroup( - new InstantCommand(() -> debouncer.setDebounceTime(debounceTimeSeconds)), - runWhen(command, () -> debouncer.calculate(condition.getAsBoolean())) - ); - } + public static Command runWhen(Command command, BooleanSupplier condition, double debounceTimeSeconds) { + final Debouncer debouncer = new Debouncer(debounceTimeSeconds, Debouncer.DebounceType.kRising); + return new WaitUntilCommand(() -> debouncer.calculate(condition.getAsBoolean())) + .andThen(command); + }
79-84: Minor: prefer != over ^ for boolean XOR.Same semantics; a touch clearer to most readers.
- ArmElevatorCommands.getPrepareForStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()) : - ArmElevatorCommands.getSetTargetStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()); + ArmElevatorCommands.getPrepareForStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean()) : + ArmElevatorCommands.getSetTargetStateCommand(() -> targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean());- ArmElevatorCommands.getPrepareForStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()) : - ArmElevatorCommands.getSetTargetStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE ^ shouldStartFlipped.getAsBoolean()); + ArmElevatorCommands.getPrepareForStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean()) : + ArmElevatorCommands.getSetTargetStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE != shouldStartFlipped.getAsBoolean());Also applies to: 85-90
91-95: Optional API symmetry: add Supplier overload without shouldStartFlipped.For parity with the non‑Supplier version, consider:
public static Command getFlippableOverridableArmCommand(Supplier<ArmElevatorConstants.ArmElevatorState> targetState, boolean isPrepareState) { return isPrepareState ? ArmElevatorCommands.getPrepareForStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE) : ArmElevatorCommands.getSetTargetStateCommand(targetState, () -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE); }
📜 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(6 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java(3 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/constants/FieldConstants.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-91)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-121)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-367)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (3)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-91)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-367)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(13-174)
🔇 Additional comments (6)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
167-175: Fix the reverse-score heading check and constants.
Rotation2d.kCW_90deg/kCCW_90degdon’t exist in WPILib, so this method won’t compile, and the comparison currently returnstruewhile you’re still roughly facing the reef instead of when you’re actually behind it. Compute the heading error and trip the reversal once the absolute error exceeds 90°.- final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); + final double headingErrorDegrees = robotRotation.minus(robotRotationRelativeToReef).getDegrees(); + return Math.abs(headingErrorDegrees) > 90.0;src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (2)
3-3: Imports look good.New imports are appropriate for the added functionality.
Also applies to: 8-9, 13-13
75-77: Let reset run while Disabled.Make the override reset available in Disabled (operator usability).
- public static Command getResetFlipArmOverrideCommand() { - return new InstantCommand(() -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE = false); - } + public static Command getResetFlipArmOverrideCommand() { + return new InstantCommand(() -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE = false) + .ignoringDisable(true); + }src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
26-35: Clock constants hygiene + missing algae Y transform + defensive array.
- Make REEF_CLOCK_ANGLES defensive (array is mutable).
- Derive positions count from enum length to prevent drift if the enum changes.
- Add missing Y transform for algae collection (alias to scoring Y if identical).
- public static final int REEF_CLOCK_POSITIONS = 6; - public static final Rotation2d REEF_CLOCK_POSITION_DIFFERENCE = Rotation2d.fromDegrees(Conversions.DEGREES_PER_ROTATIONS / REEF_CLOCK_POSITIONS); - public static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); + // Keep the constant if you want the name, but derive it from the enum to avoid drift. + private static final int REEF_CLOCK_POSITIONS = ReefClockPosition.values().length; + public static final Rotation2d REEF_CLOCK_POSITION_DIFFERENCE = + Rotation2d.fromDegrees(Conversions.DEGREES_PER_ROTATIONS / ReefClockPosition.values().length); + // Defensive: don't expose mutable array + private static final Rotation2d[] REEF_CLOCK_ANGLES = ReefClockPosition.getClockAngles(); @@ - public static final double - REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS = 1.3, - REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS = 0.17, - REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6, - REEF_CENTER_TO_TARGET_NO_HIT_REEF_POSITION_X_TRANSFORM_METERS = REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS + 0.3; + public static final double + REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS = 1.3, + REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS = 0.17, + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_X_TRANSFORM_METERS = 1.6, + REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_Y_TRANSFORM_METERS = REEF_CENTER_TO_TARGET_SCORING_POSITION_Y_TRANSFORM_METERS, + REEF_CENTER_TO_TARGET_NO_HIT_REEF_POSITION_X_TRANSFORM_METERS = REEF_CENTER_TO_TARGET_SCORING_POSITION_X_TRANSFORM_METERS + 0.3;Add accessor (outside this hunk):
public static Rotation2d[] getReefClockAngles() { return REEF_CLOCK_ANGLES.clone(); }Verification (updates may be needed at call sites):
#!/bin/bash # Find usages that need to switch to the accessor rg -nP --type=java '\bFieldConstants\.REEF_CLOCK_ANGLES\b|REEF_CENTER_TO_TARGET_ALGAE_COLLECTION_POSITION_Y_TRANSFORM_METERS'
52-54: Replace static flippable fields with lazy getters to stop double‑flip and class‑init cycle.Constructing FlippablePose2d from SimulatedGamePieceConstants.*.get() (already alliance‑flipped) with shouldFlipWhenRedAlliance=true flips twice on red and also pulls SimulatedGamePieceConstants during FieldConstants static init. Make these lazy and set flipping to false.
- public static final FlippablePose2d - FLIPPABLE_PROCESSOR_SCORE_POSE = new FlippablePose2d(SimulatedGamePieceConstants.PROCESSOR_LOCATION.get().toPose2d(), true), - FLIPPABLE_NET_SCORE_POSE = new FlippablePose2d(SimulatedGamePieceConstants.NET_MINIMUM_X_LOCATION.get().toPose2d(), true); + // Lazily resolved via getters to avoid static init cycles and double-flip.Add getters (outside this hunk):
public static FlippablePose2d getProcessorScorePose() { return new FlippablePose2d( frc.trigon.robot.misc.simulatedfield.SimulatedGamePieceConstants.PROCESSOR_LOCATION.get().toPose2d(), /* shouldFlipWhenRedAlliance= */ false ); } public static FlippablePose2d getNetScorePose() { return new FlippablePose2d( frc.trigon.robot.misc.simulatedfield.SimulatedGamePieceConstants.NET_MINIMUM_X_LOCATION.get().toPose2d(), /* shouldFlipWhenRedAlliance= */ false ); }Update call sites to use FieldConstants.getProcessorScorePose()/getNetScorePose(). This remains compatible with Swerve.atPose(FlippablePose2d). Based on learnings.
#!/bin/bash # Locate call sites to update rg -nP --type=java '\bFieldConstants\.FLIPPABLE_(PROCESSOR|NET)_SCORE_POSE\b' -C2
8-8: Break circular static initialization and avoid alliance double‑flip.This import is only used by the static initializers below, creating a circular init risk with SimulatedGamePieceConstants (it references FieldConstants) and causes double‑flipping by wrapping an already‑flipped pose in another flippable. Remove the static dependency here and switch to lazy getters (see suggestion on Lines 52‑54).
- import frc.trigon.robot.misc.simulatedfield.SimulatedGamePieceConstants; + // SimulatedGamePieceConstants referenced via FQCN inside lazy getters to avoid static init cycles.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
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 (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (6)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(34-278)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-91)
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
40-45: Blocker: unload can hang and releases before arm is positioned; make it a deadline with timeout and sequence release after arm.As written, end‑effector “UNLOAD_CORAL” starts immediately and may dump before the arm reaches its unload pose; also no timeout means it can run forever on sensor misread. Prior reviews flagged the same. Fix by (1) sequencing arm → end‑effector, (2) using a ParallelDeadlineGroup with a WaitUntil + timeout, and (3) gating on having a piece.
Apply:
- public static Command getUnloadCoralCommand() { - return new ParallelCommandGroup( - ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), - EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) - ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()); - } + public static Command getUnloadCoralCommand() { + return new ParallelDeadlineGroup( + new WaitUntilCommand(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()) + .withTimeout(1.5), + new SequentialCommandGroup( + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) + ) + ).onlyIf(RobotContainer.END_EFFECTOR::hasGamePiece); + }If transfer proves flaky, consider temporarily assisting intake during unload as a parallel non‑deadline participant.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.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/commands/commandfactories/CoralPlacingCommands.java (1)
115-119: Replace Rotation2d.k180deg (likely undefined) with fromDegrees(180) (compile fix).WPILib doesn’t expose k180deg; use a factory.
- shouldReverseScore() ? Rotation2d.k180deg : new Rotation2d() + shouldReverseScore() ? Rotation2d.fromDegrees(180) : new Rotation2d()Apply in both calculateClosestScoringPose(...) and calculateClosestNoHitReefPose(...).
Also applies to: 144-148
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
96-99: Use the existing FLIPPABLE_REEF_CENTER_TRANSLATION constant.Avoid reconstructing the same FlippableTranslation2d; improves clarity and consistency.
- final Translation2d reefCenterPosition = new FlippableTranslation2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, true).get(); + final Translation2d reefCenterPosition = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get();Apply in both pose calculators.
Also applies to: 125-129
161-164: Guard against null prepareState in isReadyToScore (L1).L1’s SCORE_L1.prepareState is null; today you avoid the autonomous path for L1, but a defensive fallback is cheap.
- return RobotContainer.ARM_ELEVATOR.atState(REEF_CHOOSER.getArmElevatorState().prepareState, shouldReverseScore()) + final var state = REEF_CHOOSER.getArmElevatorState(); + final var prep = state.prepareState != null ? state.prepareState : state; + return RobotContainer.ARM_ELEVATOR.atState(prep, shouldReverseScore()) && RobotContainer.SWERVE.atPose(calculateClosestScoringPose(shouldScoreRight));
📜 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(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(11-91)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-121)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-367)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (5)
25-25: Limit REEF_CHOOSER visibility to private (agreeing with prior suggestion).Make the field private to keep encapsulation tight and avoid unintended package access.
- static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER; + private static final ReefChooser REEF_CHOOSER = OperatorConstants.REEF_CHOOSER;
42-44: LGTM: unified flippable/override arm command usage.Using GeneralCommands.getFlippableOverridableArmCommand(...) cleans up flip/override handling and resolves the earlier duplication pattern.
Also applies to: 52-54
60-66: Prepare-arm gating looks correct and safer.Conditionally pre‑positioning the arm only when angle is above current or when beyond SAFE_DISTANCE_FROM_SCORING_POSE_METERS is a good collision-avoidance guard.
Please confirm SAFE_DISTANCE_FROM_SCORING_POSE_METERS (2.2 m) was validated on the sim/field model for L2/L3 where reach is longest.
Also applies to: 82-86
166-174: Reverse-orientation check is inverted and uses non-existent constants (compile error).This will misclassify when to flip and won’t compile due to Rotation2d.kCW_90deg/kCCW_90deg.
- static boolean shouldReverseScore() { - final Rotation2d robotRotation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d robotTranslation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); - } + static boolean shouldReverseScore() { + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION + .get() + .minus(pose.getTranslation()) + .getAngle(); + final double headingErrorDeg = pose.getRotation().minus(toReef).getDegrees(); + return Math.abs(headingErrorDeg) > 90.0; + }Optional: add small hysteresis (e.g., enter >95°, exit <85°) to avoid chatter near 90°.
188-191: Algae collection state mapping doesn’t match level behavior (L1 floor, L2/L3 processor, L4 lollipop).Current switch maps 1→L2 and 4→L3; this breaks the operator’s “single button” logic.
- private ArmElevatorConstants.ArmElevatorState determineArmElevatorAlgaeCollectionState() { - return switch (level) { - case 1, 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; - case 3, 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; - } + private ArmElevatorConstants.ArmElevatorState determineArmElevatorAlgaeCollectionState() { + return switch (level) { + case 1 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR; // eject/floor + case 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; // processor + case 3 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; // processor + case 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP; // net/lollipop + default -> throw new IllegalStateException("Unexpected value: " + ordinal()); + }; + }Also applies to: 209-210, 223-229
Co-Authored-By: Nummun14 <[email protected]>
Co-Authored-By: Nummun14 <[email protected]>
Co-Authored-By: Nummun14 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (16)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
110-110: Guard sim-only coral spawn bindingBind SPAWN_CORAL_IN_SIMULATION_TRIGGER only when running in simulation to avoid confusing behavior on hardware.
- OperatorConstants.SPAWN_CORAL_IN_SIMULATION_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose()))); + if (frc.trigon.robot.misc.RobotHardwareStats.isSimulation()) { + OperatorConstants.SPAWN_CORAL_IN_SIMULATION_TRIGGER.onTrue( + new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())) + ); + }
112-112: Gate lollipop toggle to only act when holding algae (resolve A-button overlap with L1 selection)Prevents unintended toggles when A is used to set L1.
- OperatorConstants.LOLLIPOP_ALGAE_TOGGLE_TRIGGER.onTrue(new InstantCommand(AlgaeManipulationCommands::toggleLollipopCollection)); + OperatorConstants.LOLLIPOP_ALGAE_TOGGLE_TRIGGER + .and(AlgaeManipulationCommands::isHoldingAlgae) + .onTrue(new InstantCommand(AlgaeManipulationCommands::toggleLollipopCollection));src/main/java/frc/trigon/robot/constants/OperatorConstants.java (3)
29-31: Remove stick-state flags; compose triggers directly (and decouple from algae state)Static flags can desync; derive mutual exclusion from current inputs and drop algae coupling here.
- private static boolean - IS_LEFT_SCORE_BUTTON_PRESSED = false, - IS_RIGHT_SCORE_BUTTON_PRESSED = false; @@ - private static Trigger createScoreTrigger(boolean isRight, boolean isAlgaeCommand) { - final Trigger scoreTrigger; - - if (isRight) - scoreTrigger = DRIVER_CONTROLLER.rightStick() - .and(() -> !IS_LEFT_SCORE_BUTTON_PRESSED) - .onTrue(new InstantCommand(() -> IS_RIGHT_SCORE_BUTTON_PRESSED = true)) - .onFalse(new InstantCommand(() -> IS_RIGHT_SCORE_BUTTON_PRESSED = false)); - else - scoreTrigger = DRIVER_CONTROLLER.leftStick() - .and(() -> !IS_RIGHT_SCORE_BUTTON_PRESSED) - .onTrue(new InstantCommand(() -> IS_LEFT_SCORE_BUTTON_PRESSED = true)) - .onFalse(new InstantCommand(() -> IS_LEFT_SCORE_BUTTON_PRESSED = false)); - - if (isAlgaeCommand) - return scoreTrigger.and(AlgaeManipulationCommands::isHoldingAlgae); - return scoreTrigger.and(() -> !AlgaeManipulationCommands.isHoldingAlgae()); - } + private static Trigger createScoreTrigger(boolean isRight) { + final Trigger right = DRIVER_CONTROLLER.rightStick(); + final Trigger left = DRIVER_CONTROLLER.leftStick(); + final Trigger primary = isRight ? right : left; + final Trigger other = isRight ? left : right; + // Fires only when the chosen stick is pressed and the other is not + return primary.and(other.negate()); + }Also applies to: 80-97
55-60: Make constants “dumb”: remove algae/coral gating hereLet command factories decide game-piece constraints.
- SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or(createScoreTrigger(true, true)), - SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or(createScoreTrigger(false, true)), + SCORE_ALGAE_IN_NET_TRIGGER = OPERATOR_CONTROLLER.n().or(createScoreTrigger(true)), + SCORE_ALGAE_IN_PROCESSOR_TRIGGER = OPERATOR_CONTROLLER.j().or(createScoreTrigger(false)), @@ - SCORE_CORAL_RIGHT_TRIGGER = createScoreTrigger(true, false), - SCORE_CORAL_LEFT_TRIGGER = createScoreTrigger(false, false), + SCORE_CORAL_RIGHT_TRIGGER = createScoreTrigger(true), + SCORE_CORAL_LEFT_TRIGGER = createScoreTrigger(false),
64-65: Resolve A-button duplication cleanlyEither move the lollipop toggle off Driver A (preferred) or accept gating only at binding sites. Suggest rebinding to an operator key to fully eliminate conflict.
- LOLLIPOP_ALGAE_TOGGLE_TRIGGER = DRIVER_CONTROLLER.a(); + LOLLIPOP_ALGAE_TOGGLE_TRIGGER = OPERATOR_CONTROLLER.b(); @@ - SET_TARGET_SCORING_REEF_LEVEL_L1_TRIGGER = OPERATOR_CONTROLLER.numpad0().or(DRIVER_CONTROLLER.a().and(() -> !AlgaeManipulationCommands.isHoldingAlgae())), + SET_TARGET_SCORING_REEF_LEVEL_L1_TRIGGER = OPERATOR_CONTROLLER.numpad0().or(DRIVER_CONTROLLER.a()),Note: With the RobotContainer gating added, either approach works; prefer keeping this constants class decoupled from command state.
Also applies to: 67-67
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (7)
161-164: Unify net reverse decisionDelegate to the existing reef reverse heuristic to keep behavior consistent.
- final Rotation2d swerveAngle = RobotContainer.SWERVE.getDriveRelativeAngle(); - return swerveAngle.getDegrees() < 90 && swerveAngle.getDegrees() > -90; + return CoralPlacingCommands.shouldReverseScore();
93-94: Don’t proxy Swerve in align; keep requirements to avoid drive conflictsDrop asProxy() so only one command owns Swerve during align.
- ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY).asProxy(); + ).onlyIf(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY);
114-120: Avoid ParallelRaceGroup for net scoring — can terminate earlyUse ParallelCommandGroup (or Deadline with arm as deadline) so drive/elevator don’t abort prematurely.
- return new ParallelRaceGroup( + return new ParallelCommandGroup( GeneralCommands.getFlippableOverridableArmCommand(ArmElevatorConstants.ArmElevatorState.SCORE_NET, false, AlgaeManipulationCommands::shouldReverseNetScore), GeneralCommands.runWhen(EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_ALGAE), OperatorConstants.CONTINUE_TRIGGER), getDriveToNetCommand() );
130-141: Retain Swerve ownership during net approach (remove asProxy)Proxying drops requirements and invites concurrent drives.
- ).asProxy().onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY); + ).onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY);
145-151: Same here for processor approach — remove asProxyKeep exclusive Swerve control.
- ).asProxy() - .until(() -> RobotContainer.SWERVE.atPose(FieldConstants.FLIPPABLE_PROCESSOR_SCORE_POSE)) + ) + .until(() -> RobotContainer.SWERVE.atPose(FieldConstants.FLIPPABLE_PROCESSOR_SCORE_POSE)) .onlyWhile(() -> CoralPlacingCommands.SHOULD_SCORE_AUTONOMOUSLY);
196-202: Don’t proxy intake/transporter in lollipop sequenceKeep subsystem requirements to prevent actuator conflicts.
- ).asProxy(); + );
153-159: Implement single‑button fallback mapping by target level (PR objective)When neither algae score trigger is pressed, select score mode by target level: L4→net, L3/L2→processor, L1→hold/eject.
private static int getAlgaeScoreMethodSelector() { if (OperatorConstants.SCORE_ALGAE_IN_NET_TRIGGER.getAsBoolean()) return 1; if (OperatorConstants.SCORE_ALGAE_IN_PROCESSOR_TRIGGER.getAsBoolean()) return 2; - return 0; + return switch (OperatorConstants.REEF_CHOOSER.getScoringLevel()) { + case L4 -> 1; + case L3, L2 -> 2; + case L1 -> 0; + }; }src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
62-64: Keep Swerve requirement in autonomous drive stepsRemove asProxy() to avoid concurrent drive commands during prepare/score.
- getAutonomousDriveToNoHitReefPose(shouldScoreRight).asProxy().onlyWhile(() -> !isPrepareArmAngleAboveCurrentArmAngle()), - getAutonomousDriveToReef(shouldScoreRight).asProxy() + getAutonomousDriveToNoHitReefPose(shouldScoreRight).onlyWhile(() -> !isPrepareArmAngleAboveCurrentArmAngle()), + getAutonomousDriveToReef(shouldScoreRight)
166-174: Fix reverse-orientation check and invalid Rotation2d constantsUse absolute heading error > 90°; remove non-existent kCW_90deg/kCCW_90deg.
- static boolean shouldReverseScore() { - final Rotation2d robotRotation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d robotTranslation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); - } + static boolean shouldReverseScore() { + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get() + .minus(pose.getTranslation()) + .getAngle(); + return Math.abs(pose.getRotation().minus(toReef).getDegrees()) > 90.0; + }
223-229: Map algae collection states per level (L1→floor, L2→L2, L3→L3, L4→lollipop)Aligns with PR goals and operator expectations.
- return switch (level) { - case 1, 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; - case 3, 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; + return switch (level) { + case 1 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR; + case 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; + case 3 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; + case 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP; + default -> throw new IllegalStateException("Unexpected value: " + ordinal()); + };src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
75-77: Allow flip-override reset while disabledLet operators clear the override in Disabled.
- public static Command getResetFlipArmOverrideCommand() { - return new InstantCommand(() -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE = false); - } + public static Command getResetFlipArmOverrideCommand() { + return new InstantCommand(() -> OperatorConstants.SHOULD_FLIP_ARM_OVERRIDE = false) + .ignoringDisable(true); + }
🧹 Nitpick comments (1)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
127-135: Trim the algae-state check duplicationThe long OR chain is starting to sprawl; every new algae state now requires touching this method in two places. Wrapping the allowable states in an
EnumSet(and looping over it for the flipped check) would keep this logic centralized and harder to forget when more states arrive.+ private static final EnumSet<ArmElevatorConstants.ArmElevatorState> ALGAE_COLLECTION_STATES = + EnumSet.of( + ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2, + ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3, + ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR, + ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP + ); + private static boolean isCollectingAlgae() { - return RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2, true) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3, true) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR, true) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP) - || RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP, true); + return ALGAE_COLLECTION_STATES.stream().anyMatch(state -> + RobotContainer.ARM_ELEVATOR.atState(state) || RobotContainer.ARM_ELEVATOR.atState(state, true)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/frc/trigon/robot/RobotContainer.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(9 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(4 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(2 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.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: 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/misc/simulatedfield/SimulationFieldHandler.java
📚 Learning: 2025-09-21T17:39:27.460Z
Learnt from: Nummun14
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java:23-28
Timestamp: 2025-09-21T17:39:27.460Z
Learning: In FRC command-based programming, using InstantCommand with schedule() inside a command group is acceptable when you want to decouple command lifecycles - allowing part of a sequence to be interrupted while ensuring another part runs to completion independently. This creates "fire-and-forget" behavior where scheduled commands continue regardless of the parent command's cancellation.
Applied to files:
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java
🧬 Code graph analysis (9)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (2)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)
src/main/java/frc/trigon/robot/RobotContainer.java (4)
src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(18-213)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(33-265)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(13-174)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (11)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/subsystems/intake/IntakeCommands.java (1)
IntakeCommands(11-37)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-233)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterCommands.java (1)
TransporterCommands(9-44)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(21-120)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(33-265)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (3)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(33-265)
src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (16)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
CommandConstants(23-122)src/main/java/frc/trigon/robot/commands/commandclasses/WaitUntilChangeCommand.java (1)
WaitUntilChangeCommand(7-31)src/main/java/frc/trigon/robot/constants/AutonomousConstants.java (1)
AutonomousConstants(27-104)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-121)src/main/java/frc/trigon/robot/constants/LEDConstants.java (1)
LEDConstants(6-26)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)src/main/java/lib/hardware/misc/leds/LEDCommands.java (1)
LEDCommands(15-409)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/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(18-73)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(23-235)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-121)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
53-55: Nice telemetry hook for ejection stateThe additional
AutoLogOutputmakes it straightforward to correlate end-effector voltage with algae/coral release flows. Thanks for wiring this up.src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
347-350: Confirm processor height changeDropping
SCORE_PROCESSORfrom 0.603 m to 0.1 m is a ~½ m reduction in elevator extension. That will put the wrist much closer to the frame and could miss the processor opening entirely if unintentional. Can you double-check this against the mech model/sim before we rely on it?src/main/java/frc/trigon/robot/RobotContainer.java (1)
111-111: Flip-override UX checkMany algae flows call GeneralCommands.getResetFlipArmOverrideCommand(), which will wipe the manual toggle bound here. Confirm this reset is intentional and documented; otherwise, consider removing the resets or applying them only at specific transitions.
| public static Command getUnloadCoralCommand() { | ||
| return new ParallelCommandGroup( | ||
| ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), | ||
| EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) | ||
| ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()); | ||
| } |
There was a problem hiding this comment.
Add timeout/deadline to prevent unload loop from hanging.
This ParallelCommandGroup never terminates if either sensor fails to report “coral transferred,” so both subsystems keep running indefinitely and anything waiting on unload will stall. ParallelCommandGroup waits for all members to finish, and the until decorator only interrupts when the predicate flips true—if it never does, the group runs forever.(docs.wpilib.org) Convert this to a ParallelDeadlineGroup with a WaitUntilCommand plus timeout (and keep the existing condition as the deadline) so the actuators stop deterministically even when handoff fails.
- return new ParallelCommandGroup(
- ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL),
- EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL)
- ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral());
+ return new ParallelDeadlineGroup(
+ new WaitUntilCommand(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral())
+ .withTimeout(1.5),
+ ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL),
+ EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL)
+ ).onlyIf(RobotContainer.END_EFFECTOR::hasGamePiece);import edu.wpi.first.wpilibj2.command.ParallelDeadlineGroup;
import edu.wpi.first.wpilibj2.command.WaitUntilCommand;🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
around lines 40 to 45, replace the ParallelCommandGroup that uses .until(...)
with a ParallelDeadlineGroup that uses a WaitUntilCommand constructed from the
existing predicate as the deadline and also apply a sensible timeout (e.g., 1–3
seconds) so the group always stops if the sensor predicate never becomes true;
add imports for ParallelDeadlineGroup and WaitUntilCommand, make the
WaitUntilCommand.deadline use () -> !RobotContainer.END_EFFECTOR.hasGamePiece()
&& RobotContainer.INTAKE.hasCoral(), wrap the existing two subsystem commands as
the parallel commands, and set withTimeout(...) on the deadline so actuators
stop deterministically on timeout or when the predicate becomes true.
Co-Authored-By: Nummun14 <[email protected]>
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 (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
115-119: Use Rotation2d.fromDegrees(180) for compatibility.k180deg may not exist across WPILib versions. Replace with fromDegrees(180).
- shouldReverseScore() ? Rotation2d.k180deg : new Rotation2d() + shouldReverseScore() ? Rotation2d.fromDegrees(180) : new Rotation2d()Also applies to: 144-148
161-164: Null prepareState can NPE for L1; check against SCORE_L1.When prepareState is null (e.g., SCORE_L1), atState(null, ...) may blow up. Use the state itself in that case.
- private static boolean isReadyToScore(boolean shouldScoreRight) { - return RobotContainer.ARM_ELEVATOR.atState(REEF_CHOOSER.getArmElevatorState().prepareState, shouldReverseScore()) - && RobotContainer.SWERVE.atPose(calculateClosestScoringPose(shouldScoreRight)); - } + private static boolean isReadyToScore(boolean shouldScoreRight) { + final var state = REEF_CHOOSER.getArmElevatorState(); + final var checkState = state.prepareState != null ? state.prepareState : state; + return RobotContainer.ARM_ELEVATOR.atState(checkState, shouldReverseScore()) + && RobotContainer.SWERVE.atPose(calculateClosestScoringPose(shouldScoreRight)); + }
♻️ Duplicate comments (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
166-174: Reverse-orientation check is inverted; also avoid non-existent constants.Current logic returns true inside ±90°. It should reverse when |heading error| > 90°. Also kCW_90deg/kCCW_90deg are not guaranteed constants.
-static boolean shouldReverseScore() { - final Rotation2d robotRotation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); - final Translation2d robotTranslation = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getTranslation(); - final Translation2d reefCenterTranslation = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get(); - final Translation2d difference = reefCenterTranslation.minus(robotTranslation); - final Rotation2d robotRotationRelativeToReef = difference.getAngle(); - final Rotation2d robotRotationFacingReef = robotRotation.minus(robotRotationRelativeToReef); - return robotRotationFacingReef.getDegrees() > Rotation2d.kCW_90deg.getDegrees() && robotRotationFacingReef.getDegrees() < Rotation2d.kCCW_90deg.getDegrees(); -} +static boolean shouldReverseScore() { + final Pose2d pose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); + final Rotation2d toReef = FieldConstants.FLIPPABLE_REEF_CENTER_TRANSLATION.get() + .minus(pose.getTranslation()) + .getAngle(); + final double headingErrorDeg = pose.getRotation().minus(toReef).getDegrees(); + return Math.abs(headingErrorDeg) > 90.0; +}Optionally add 85/95° hysteresis if you see chatter.
223-229: Map algae collection states per level (L1 floor, L2/L3 processor, L4 lollipop).Current switch maps 1–2 to L2, 3–4 to L3; this contradicts the PR plan and constants.
- private ArmElevatorConstants.ArmElevatorState determineArmElevatorAlgaeCollectionState() { - return switch (level) { - case 1, 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; - case 3, 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; - default -> throw new IllegalStateException("Unexpected value: " + ordinal()); - }; - } + private ArmElevatorConstants.ArmElevatorState determineArmElevatorAlgaeCollectionState() { + return switch (level) { + case 1 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_FLOOR; + case 2 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L2; + case 3 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_L3; + case 4 -> ArmElevatorConstants.ArmElevatorState.COLLECT_ALGAE_LOLLIPOP; + default -> throw new IllegalStateException("Unexpected value: " + ordinal()); + }; + }src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
24-27: Gate is fine; but getLoadCoralCommand can run indefinitely if sensor never trips.Schedule once is okay, but without a deadline/timeout, motors may thrash forever on bad sensor. Convert to a deadline with timeout and gate on supply presence.
@@ - public static Command getLoadCoralCommand() { - return new ParallelCommandGroup( - ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.LOAD_CORAL), - EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.LOAD_CORAL) - ).until(RobotContainer.END_EFFECTOR::hasGamePiece).onlyWhile(() -> !RobotContainer.END_EFFECTOR.hasGamePiece()); - } + public static Command getLoadCoralCommand() { + return new ParallelDeadlineGroup( + new WaitUntilCommand(RobotContainer.END_EFFECTOR::hasGamePiece).withTimeout(1.5), + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.LOAD_CORAL), + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.LOAD_CORAL) + ).onlyIf(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() + && (RobotContainer.INTAKE.hasCoral() || RobotContainer.TRANSPORTER.hasCoral())); + }Add:
+import edu.wpi.first.wpilibj2.command.ParallelDeadlineGroup; +import edu.wpi.first.wpilibj2.command.WaitUntilCommand;
40-45: Unload can also hang; switch to deadline + timeout and guard on current hold.Prevents indefinite running if transfer sensors fail.
- public static Command getUnloadCoralCommand() { - return new ParallelCommandGroup( - ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), - EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) - ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()); - } + public static Command getUnloadCoralCommand() { + return new ParallelDeadlineGroup( + new WaitUntilCommand(() -> !RobotContainer.END_EFFECTOR.hasGamePiece() && RobotContainer.INTAKE.hasCoral()) + .withTimeout(1.5), + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.UNLOAD_CORAL), + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.UNLOAD_CORAL) + ).onlyIf(RobotContainer.END_EFFECTOR::hasGamePiece); + }
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
82-86: Name vs behavior; consider tiny hysteresis for the distance check.runWhen(...) waits until arm is above prepare angle or distance > SAFE_DISTANCE; that’s fine. For stability, consider 0.1–0.2 m hysteresis to avoid re-arming if distance jitters around the threshold on sim/real.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
47-49: One-shot confirmation is currently orphaned; wire rumble on acquisition or remove helper.getCollectionConfirmationCommand() is unused; consider andThen(getCollectionConfirmationCommand()) after hasCoral, or drop the method.
- private static Command getIntakeSequenceCommand() { - return getInitiateCollectionCommand().until(RobotContainer.TRANSPORTER::hasCoral); - } + private static Command getIntakeSequenceCommand() { + return getInitiateCollectionCommand() + .until(RobotContainer.TRANSPORTER::hasCoral) + .andThen(getCollectionConfirmationCommand()); + }Also applies to: 65-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (8)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-102)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(21-120)src/main/java/frc/trigon/robot/commands/commandfactories/AlgaeManipulationCommands.java (1)
AlgaeManipulationCommands(33-265)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (6)
src/main/java/frc/trigon/robot/subsystems/armelevator/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-366)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(12-98)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(42-126)src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
GeneralCommands(19-96)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(18-121)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (4)
25-25: Good: centralizing ReefChooser access.Using OperatorConstants.REEF_CHOOSER avoids duplicate instances.
35-35: Correct gate before scoring sequence.onlyIf(...) prevents scheduling without a piece.
42-42: Arm command now flippable/overridable — nice integration.Passing shouldReverseScore as a supplier keeps the decision dynamic.
Also applies to: 52-52
188-191: Algae collection state alongside scoring state — good expansion.This aligns with the PR’s single-button algae collection concept.
Also applies to: 209-210
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
9-10: Package move to armelevator — good.Imports updated correctly.
Co-Authored-By: Nummun14 <[email protected]>
Did most of the logic, kinda need sim and target states to implement and test the rest
Summary by CodeRabbit
New Features
Changes
Refactor