Skip to content

Conversation

@Nummun14
Copy link
Member

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

Walkthrough

RobotContainer now delegates autonomous selection to GeneralAutonomousCommands.getAutonomousCommand(). The removed AutonomousCommands file was replaced by new GeneralAutonomousCommands and SafeAutonomousDriveCommands classes that provide composed autonomous flows, pose/pose-reset utilities, and trench/alliance-zone-aware drive builders. AutonomousConstants adds multiple LoggedDashboardChoosers, new path constraints, timeouts, and updated PID/feedforward values. FieldConstants adds many FlippablePose2d positions and adjusts zone lengths. Vision-driven game-piece clustering and GamePieceAutoDriveCommand were added; an older GamePieceAutoDriveCommand implementation and AutonomousCommands were removed. Multiple subsystem constants, pathplanner/navgrid, and intake/assist APIs were also updated.

🚥 Pre-merge checks | ❌ 3
❌ Failed checks (1 warning, 2 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Autonomous' is too vague and does not clearly summarize the substantial changes made to autonomous command handling and robot constants. Use a more descriptive title such as 'Refactor autonomous command handling to centralized GeneralAutonomousCommands' or 'Restructure autonomous system with new command factory pattern'.
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to verify relatedness to the changeset. Add a description explaining the major refactoring: removal of AutonomousCommands, introduction of GeneralAutonomousCommands, autonomous chooser migration to constants, and supporting infrastructure changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Comment on lines 105 to 118
private static void initializeAutoChoosers() {
configureClimbPositionChooser();
configureAutonomousPositionChooser(FIRST_AUTONOMOUS_CHOOSER);
configureAutonomousPositionChooser(SECOND_AUTONOMOUS_CHOOSER);
configureAutonomousPositionChooser(THIRD_AUTONOMOUS_CHOOSER);
}

private static void configureAutonomousPositionChooser(LoggedDashboardChooser<Command> firstAutonomousChooser) {
firstAutonomousChooser.addOption("Depot", AutonomousCommands.getCollectFromDepotCommand());
firstAutonomousChooser.addOption("Score", AutonomousCommands.getScoreCommand());
firstAutonomousChooser.addOption("CollectFromNeutralZone", AutonomousCommands.getCollectFromNeutralZoneCommand());
firstAutonomousChooser.addOption("Delivery", AutonomousCommands.getDeliveryCommand());
firstAutonomousChooser.addDefaultOption("Nothing", null);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Rename firstAutonomousChooser to reflect general usage.
The method configures all three choosers, so firstAutonomousChooser is misleading; consider autonomousChooser or similar. As per coding guidelines, Variable, function, and class names must be descriptive and intention-revealing.

Comment on lines 34 to 42
public static final FlippablePose2d
LEFT_CLIMB_POSITION = new FlippablePose2d(1.45, 4.25, Rotation2d.fromDegrees(0), true),
RIGHT_CLIMB_POSITION = new FlippablePose2d(LEFT_CLIMB_POSITION.get().getX(), 3.28, Rotation2d.fromDegrees(0), true),
CENTER_CLIMB_POSITION = new FlippablePose2d((LEFT_CLIMB_POSITION.get().getX() + LEFT_CLIMB_POSITION.get().getX()) / 2, LEFT_CLIMB_POSITION.get().getY(), Rotation2d.fromDegrees(0), true),
DEPOT_POSITION = new FlippablePose2d(0.9, 6, Rotation2d.fromDegrees(180), true),
LEFT_INTAKE_POSITION = new FlippablePose2d(6.2, 7, Rotation2d.fromDegrees(-30), true),
RIGHT_INTAKE_POSITION = new FlippablePose2d(LEFT_INTAKE_POSITION.get().getX(), FIELD_WIDTH_METERS - LEFT_INTAKE_POSITION.get().getY(), Rotation2d.fromDegrees(30), true),
LEFT_IDEAL_SHOOTING_POSITION = new FlippablePose2d(2.7, 5.8, Rotation2d.fromDegrees(0), true),
RIGHT_IDEAL_SHOOTING_POSITION = new FlippablePose2d(LEFT_IDEAL_SHOOTING_POSITION.get().getX(), FIELD_WIDTH_METERS - LEFT_IDEAL_SHOOTING_POSITION.get().getY(), Rotation2d.fromDegrees(0), true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

CENTER_CLIMB_POSITION currently matches LEFT_CLIMB_POSITION.
Line 37 averages the left X with itself and reuses the left Y, so the center selection will resolve to the left climb. If the intent is a midpoint, compute the center using the left/right climb Y values (or the field-width midpoint) and keep X as the shared value.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

final Translation2d fieldRelativeJoystickPosition = getFieldRelativeJoystickPosition();

final Rotation2d joystickAngle = fieldRelativeJoystickPosition.getAngle();
final Rotation2d joystickAngle = fieldRelativeJoystickPosition.getNorm() < 1e-6 ? new Rotation2d() : fieldRelativeJoystickPosition.getAngle();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Extract the joystick epsilon into a named constant.
1e-6 is a magic number; define a constant (or reuse an existing deadband constant) to make intent explicit and adjustable.

As per coding guidelines, magic numbers and strings should be named constants.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Comment on lines +32 to +34
public int getSize() {
return 1;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

getSize always returns 1.
This breaks cluster scoring/consumers once size > 1. Store the size (or the list) in the instance and return the real count.

Comment on lines +36 to +44
private Translation2d calculateCentroid(List<Translation2d> pieces) {
double sumX = 0;
double sumY = 0;
for (Translation2d piece : pieces) {
sumX += piece.getX();
sumY += piece.getY();
}
return new Translation2d(sumX / pieces.size(), sumY / pieces.size());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard against empty clusters.
pieces.size() can be 0, which will divide by zero. Validate in the constructor or here and fail fast with a clear error.

public static final PIDController GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER = RobotHardwareStats.isSimulation() ?
new PIDController(0.5, 0, 0) :
new PIDController(2.4, 0, 0);
public static final double AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS = 0.01;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Confirm the 0.01m intake-open distance is intentional.
This threshold is extremely small and may be below sensor noise, which could prevent the “close enough” condition from ever triggering.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Comment on lines +96 to +120
private GamePieceCluster findBestCluster() {
List<Translation2d> allObjects = OBJECT_POSE_ESTIMATOR.getObjectsOnField();
Pose2d robotPose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose();

if (allObjects.isEmpty()) return null;

GamePieceCluster bestCluster = null;
double maxScore = -Double.MAX_VALUE;

for (Translation2d seed : allObjects) {
if (isOutOfBounds(seed)) continue;

List<Translation2d> clusterPieces = getNeighbors(seed, allObjects);
if (clusterPieces.isEmpty()) continue;

GamePieceCluster cluster = new GamePieceCluster(clusterPieces, robotPose);
double score = calculateClusterScore(clusterPieces.size(), cluster.getDistanceToRobot());

if (score > maxScore) {
maxScore = score;
bestCluster = cluster;
}
}
return bestCluster;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

O(n²) complexity in findBestCluster for each seed.

For every seed, getNeighbors iterates all objects again. With typical vision counts this is fine, but consider early termination or caching if object counts grow.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Comment on lines 104 to 111
private static FlippablePose2d getTrenchExitPose(FlippablePose2d targetPose) {
final FlippablePose2d targetTrenchExitPose = isRight() ?
getClosestPoseToPose(targetPose, FieldConstants.RIGHT_TRENCH_ENTRY_POSITION_FROM_ALLIANCE_ZONE, FieldConstants.RIGHT_TRENCH_ENTRY_POSITION_FROM_NEUTRAL_ZONE) :
getClosestPoseToPose(targetPose, FieldConstants.LEFT_TRENCH_ENTRY_POSITION_FROM_ALLIANCE_ZONE, FieldConstants.LEFT_TRENCH_ENTRY_POSITION_FROM_NEUTRAL_ZONE);
if (RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees() > 90 || RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees() < -90)
return new FlippablePose2d(targetTrenchExitPose.getBlueObject().getTranslation(), Math.PI, true);
return targetTrenchExitPose;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

isRight() called at runtime here - correct, but verify heading adjustment logic.

The rotation adjustment at lines 108-109 checks if the heading is outside ±90°. However, the condition > 90 || < -90 doesn't account for the discontinuity at ±180°. Consider using absolute value: Math.abs(rotation.getDegrees()) > 90.

Comment on lines 117 to 119
if (RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees() > 90)
return new FlippablePose2d(targetTrenchEntryPose.getBlueObject().getTranslation(), Math.PI, true);
return targetTrenchEntryPose;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Inconsistent rotation check compared to getTrenchExitPose().

Line 117 only checks > 90 but not < -90, while line 108 checks both. This asymmetry could cause different behavior depending on which quadrant the robot is facing.

-if (RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees() > 90)
+if (Math.abs(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees()) > 90)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees() > 90)
return new FlippablePose2d(targetTrenchEntryPose.getBlueObject().getTranslation(), Math.PI, true);
return targetTrenchEntryPose;
if (Math.abs(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getDegrees()) > 90)
return new FlippablePose2d(targetTrenchEntryPose.getBlueObject().getTranslation(), Math.PI, true);
return targetTrenchEntryPose;

Comment on lines 122 to 133
private static FlippablePose2d getClosestPoseToPose(FlippablePose2d pose, FlippablePose2d... poses) {
FlippablePose2d closestPose = null;
double closestDistance = Double.MAX_VALUE;
for (FlippablePose2d candidatePose : poses) {
final double distance = pose.get().getTranslation().getDistance(candidatePose.get().getTranslation());
if (distance < closestDistance) {
closestDistance = distance;
closestPose = candidatePose;
}
}
return closestPose;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

getClosestPoseToPose can return null if called with empty varargs.

If poses is empty, closestPose remains null. Consider adding a guard or documenting the precondition.

Comment on lines +37 to +41
public static LoggedDashboardChooser<Supplier<Command>>
FIRST_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("FirstAutonomousChooser", new SendableChooser<>()),
SECOND_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("SecondAutonomousChooser", new SendableChooser<>()),
THIRD_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("ThirdAutonomousChooser", new SendableChooser<>());
public static LoggedDashboardChooser<FlippablePose2d> CLIMB_POSITION_CHOOSER = new LoggedDashboardChooser<>("ClimbChooser", new SendableChooser<>());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Mark chooser fields as final.

These choosers are initialized once at declaration and their references never change. Mark them final to prevent accidental reassignment:

-public static LoggedDashboardChooser<Supplier<Command>>
+public static final LoggedDashboardChooser<Supplier<Command>>
         FIRST_AUTONOMOUS_CHOOSER = ...

Same applies to CLIMB_POSITION_CHOOSER on line 41.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public static LoggedDashboardChooser<Supplier<Command>>
FIRST_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("FirstAutonomousChooser", new SendableChooser<>()),
SECOND_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("SecondAutonomousChooser", new SendableChooser<>()),
THIRD_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("ThirdAutonomousChooser", new SendableChooser<>());
public static LoggedDashboardChooser<FlippablePose2d> CLIMB_POSITION_CHOOSER = new LoggedDashboardChooser<>("ClimbChooser", new SendableChooser<>());
public static final LoggedDashboardChooser<Supplier<Command>>
FIRST_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("FirstAutonomousChooser", new SendableChooser<>()),
SECOND_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("SecondAutonomousChooser", new SendableChooser<>()),
THIRD_AUTONOMOUS_CHOOSER = new LoggedDashboardChooser<>("ThirdAutonomousChooser", new SendableChooser<>());
public static final LoggedDashboardChooser<FlippablePose2d> CLIMB_POSITION_CHOOSER = new LoggedDashboardChooser<>("ClimbChooser", new SendableChooser<>());

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

AutonomousConstants.FIRST_AUTONOMOUS_CHOOSER.get() == null ? new InstantCommand() : AutonomousConstants.FIRST_AUTONOMOUS_CHOOSER.get().get(),
AutonomousConstants.SECOND_AUTONOMOUS_CHOOSER.get() == null ? new InstantCommand() : AutonomousConstants.SECOND_AUTONOMOUS_CHOOSER.get().get(),
AutonomousConstants.THIRD_AUTONOMOUS_CHOOSER.get() == null ? new InstantCommand() : AutonomousConstants.THIRD_AUTONOMOUS_CHOOSER.get().get(),
getClimbCommand(AutonomousConstants.CLIMB_POSITION_CHOOSER.get()).onlyIf(() -> AutonomousConstants.CLIMB_POSITION_CHOOSER.get() != null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Climb position captured at construction time; null-check runs at runtime.

getClimbCommand receives the position value immediately, but onlyIf re-reads the chooser later. If the selection changes between command construction and execution, behavior is inconsistent.

Fix by changing getClimbCommand to accept a Supplier<FlippablePose2d>:

getClimbCommand(() -> AutonomousConstants.CLIMB_POSITION_CHOOSER.get())
    .onlyIf(() -> AutonomousConstants.CLIMB_POSITION_CHOOSER.get() != null)

Then update getClimbCommand signature at line 73 accordingly.

Comment on lines 135 to 137
public static Command getDriveToPoseCommand(Supplier<FlippablePose2d> targetPose, PathConstraints constraints, double endVelocity) {
return new DeferredCommand(() -> getCurrentDriveToPoseCommand(targetPose.get(), constraints, endVelocity), Set.of(RobotContainer.SWERVE));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Missing Javadoc for public method.

The new overloaded getDriveToPoseCommand is a public API but lacks documentation. The endVelocity parameter's purpose and expected units should be documented for clarity.

/**
 * Creates a command that drives the swerve to the target pose using path planning.
 *
 * `@param` targetPose  the target pose supplier
 * `@param` constraints the path constraints
 * `@param` endVelocity the target velocity at the end of the path (m/s)
 * `@return` the command
 */

Comment on lines +142 to +143
AutoBuilder.pathfindToPose(targetPose.get(), constraints, endVelocity),
getPIDToPoseCommand(targetPose).onlyIf(() -> endVelocity < 1e-3)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider naming the velocity threshold constant.

The 1e-3 threshold represents "effectively zero velocity" for deciding whether to apply the final PID correction. While the value is reasonable, a named constant would improve readability.

♻️ Optional refactor
+private static final double ZERO_VELOCITY_THRESHOLD = 1e-3;
+
 private static Command getCurrentDriveToPoseCommand(FlippablePose2d targetPose, PathConstraints constraints, double endVelocity) {
     return new SequentialCommandGroup(
             new InstantCommand(() -> RobotContainer.SWERVE.initializeDrive(true)),
             AutoBuilder.pathfindToPose(targetPose.get(), constraints, endVelocity),
-            getPIDToPoseCommand(targetPose).onlyIf(() -> endVelocity < 1e-3)
+            getPIDToPoseCommand(targetPose).onlyIf(() -> endVelocity < ZERO_VELOCITY_THRESHOLD)
     );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
AutoBuilder.pathfindToPose(targetPose.get(), constraints, endVelocity),
getPIDToPoseCommand(targetPose).onlyIf(() -> endVelocity < 1e-3)
private static final double ZERO_VELOCITY_THRESHOLD = 1e-3;
private static Command getCurrentDriveToPoseCommand(FlippablePose2d targetPose, PathConstraints constraints, double endVelocity) {
return new SequentialCommandGroup(
new InstantCommand(() -> RobotContainer.SWERVE.initializeDrive(true)),
AutoBuilder.pathfindToPose(targetPose.get(), constraints, endVelocity),
getPIDToPoseCommand(targetPose).onlyIf(() -> endVelocity < ZERO_VELOCITY_THRESHOLD)
);
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/frc/trigon/robot/subsystems/turret/TurretConstants.java (1)

116-117: ⚠️ Potential issue | 🟡 Minor

Reconcile or document the MotionMagic sim/real discrepancy. The sim acceleration (90) is 60× the sim cruise velocity (1.5), while real values are equal (5/5). This will create very different dynamics during tuning and may produce non-transferable gains. Either align the ratios or add a comment explaining the intentional difference for physics simulation.

Comment on lines +98 to 100
private double calculateAssistPower(double pidOutput, double joystickValue) {
return joystickValue + pidOutput;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Result may exceed valid output range.

Adding joystickValue (up to ±1) to pidOutput (clamped to ±1) can produce values outside [-1, 1]. If the downstream drive command doesn't handle this, you may get unexpected saturation behavior.

Consider clamping the final result:

return MathUtil.clamp(joystickValue + pidOutput, -1, 1);

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 15

return;
}

double maxDeltaDeg = GamePieceAutoDriveConstants.MAX_INTAKE_ROTATION_RATE_DEG_PER_SEC * 0.02;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Magic number: 0.02 should be a named constant.

This assumes a 20ms loop period. Extract to a constant (e.g., LOOP_PERIOD_SECONDS) for clarity and easier tuning if the loop rate changes.

Comment on lines +168 to +182
private double getXControllerOutput() {
if (!shouldDrive()) return 0.0;
Translation2d error = getTranslationError();
if (error == null) return 0.0;
double output = AutonomousConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.calculate(-error.getX());
return output * getIntakeSpeedScale();
}

private double getYControllerOutput() {
if (!shouldDrive()) return 0.0;
Translation2d error = getTranslationError();
if (error == null) return 0.0;
double output = AutonomousConstants.GAME_PIECE_AUTO_DRIVE_Y_PID_CONTROLLER.calculate(-error.getY());
return output * getIntakeSpeedScale();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Redundant getTranslationError() calls between X and Y output methods.

Both getXControllerOutput and getYControllerOutput independently call getTranslationError(), which computes the same value. Consider caching the error in createTargetUpdateCommand alongside the cluster update to avoid redundant pose lookups each cycle.

Comment on lines +191 to +197
private boolean shouldDrive() {
Translation2d error = getTranslationError();
if (error == null) return false;
if (error.getNorm() <= AutonomousConstants.AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS)
return false;
return true;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Brace style: omit braces for single-statement if.

Per team standard, single-line control flow should not use braces.

Suggested fix
     private boolean shouldDrive() {
         Translation2d error = getTranslationError();
-        if (error == null) return false;
-        if (error.getNorm() <= AutonomousConstants.AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS)
-            return false;
-        return true;
+        if (error == null) return false;
+        return error.getNorm() > AutonomousConstants.AUTO_COLLECTION_INTAKE_OPEN_CHECK_DISTANCE_METERS;
     }

Based on learnings: "In Java files across the repository, follow the team standard to omit braces for single-line if statements."

Comment on lines +234 to +237
private static double getEffectiveWallX() {
return GamePieceAutoDriveConstants.ALLIANCE_WALL_X_METERS
+ 0.05;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Hardcoded 0.05 margin should use a named constant.

getEffectiveWallX() adds a hardcoded 0.05 safety margin. This should be a constant in GamePieceAutoDriveConstants (e.g., WALL_SAFETY_MARGIN_METERS) for consistency and easier tuning.

Comment on lines +85 to 92
private double calculateThetaPower(boolean shouldAssist) {
final double joystickValue = CommandConstants.calculateDriveStickAxisValue(OperatorConstants.DRIVER_CONTROLLER.getRightX());

if (hasNoTrackedGamePiece())
if (!shouldAssist || hasNoTrackedGamePiece())
return joystickValue;

return calculateThetaAssistPower(assistScalar, joystickValue, distanceFromTrackedGamePiece.getAngle().unaryMinus());
return calculateThetaAssistPower(joystickValue, distanceFromTrackedGamePiece.getAngle().unaryMinus());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Wrong axis scaling method for rotation.

Line 86 uses calculateDriveStickAxisValue for the rotation stick (getRightX), but this method is intended for translation axes. Based on the CommandConstants documentation, this should use calculateRotationStickAxisValue which applies the correct rotation-specific divider and shift power.

- final double joystickValue = CommandConstants.calculateDriveStickAxisValue(OperatorConstants.DRIVER_CONTROLLER.getRightX());
+ final double joystickValue = CommandConstants.calculateRotationStickAxisValue(OperatorConstants.DRIVER_CONTROLLER.getRightX());

Comment on lines +82 to +93
private static FlippablePose2d getClosestPoseToPose(FlippablePose2d pose, FlippablePose2d... poses) {
FlippablePose2d closestPose = null;
double closestDistance = Double.MAX_VALUE;
for (FlippablePose2d candidatePose : poses) {
final double distance = pose.get().getTranslation().getDistance(candidatePose.get().getTranslation());
if (distance < closestDistance) {
closestDistance = distance;
closestPose = candidatePose;
}
}
return closestPose;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential NPE: getClosestPoseToPose returns null for empty array.

If poses is empty, closestPose remains null. Callers could NPE. Consider defensive validation or documentation.

Comment on lines +116 to +119
firstAutonomousChooser.addOption("Depot", () -> GeneralAutonomousCommands.getCollectFromDepotCommand(true, 6));
firstAutonomousChooser.addOption("Score", () -> GeneralAutonomousCommands.getScoreCommand(4));
firstAutonomousChooser.addOption("CollectFromNeutralZone", () -> GeneralAutonomousCommands.getCollectFromNeutralZoneCommand(true, 1.5));
firstAutonomousChooser.addOption("Delivery", () -> GeneralAutonomousCommands.getDeliveryCommand(true, 6));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use the defined timeout constants instead of hardcoded values.

The timeout values here duplicate constants defined at lines 48-52, and there's a mismatch: getDeliveryCommand uses 6 but DELIVERY_TIMEOUT_SECONDS is 8.

Replace with:

  • Line 116: DEPOT_COLLECTION_TIMEOUT_SECONDS (6)
  • Line 117: SCORING_TIMEOUT_SECONDS (6)
  • Line 118: NEUTRAL_ZONE_COLLECTION_TIMEOUT_SECONDS (1.5)
  • Line 119: DELIVERY_TIMEOUT_SECONDS (8) — note this changes behavior
Suggested fix
-        firstAutonomousChooser.addOption("Depot", () -> GeneralAutonomousCommands.getCollectFromDepotCommand(true, 6));
-        firstAutonomousChooser.addOption("Score", () -> GeneralAutonomousCommands.getScoreCommand(4));
-        firstAutonomousChooser.addOption("CollectFromNeutralZone", () -> GeneralAutonomousCommands.getCollectFromNeutralZoneCommand(true, 1.5));
-        firstAutonomousChooser.addOption("Delivery", () -> GeneralAutonomousCommands.getDeliveryCommand(true, 6));
+        firstAutonomousChooser.addOption("Depot", () -> GeneralAutonomousCommands.getCollectFromDepotCommand(true, DEPOT_COLLECTION_TIMEOUT_SECONDS));
+        firstAutonomousChooser.addOption("Score", () -> GeneralAutonomousCommands.getScoreCommand(SCORING_TIMEOUT_SECONDS));
+        firstAutonomousChooser.addOption("CollectFromNeutralZone", () -> GeneralAutonomousCommands.getCollectFromNeutralZoneCommand(true, NEUTRAL_ZONE_COLLECTION_TIMEOUT_SECONDS));
+        firstAutonomousChooser.addOption("Delivery", () -> GeneralAutonomousCommands.getDeliveryCommand(true, DELIVERY_TIMEOUT_SECONDS));


private static void configureAutonomousPositionChooser(LoggedDashboardChooser<Supplier<Command>> firstAutonomousChooser) {
firstAutonomousChooser.addOption("Depot", () -> GeneralAutonomousCommands.getCollectFromDepotCommand(true, 6));
firstAutonomousChooser.addOption("Score", () -> GeneralAutonomousCommands.getScoreCommand(4));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

SCORING_TIMEOUT_SECONDS is 6, but getScoreCommand uses 4.

Either update the constant to 4 or use the constant here. This inconsistency suggests the constant may not reflect the intended value.

Comment on lines +71 to +76
for (int i = 0; i <8; i++) {
final SimulatedGamePiece currentHeldFuel = new SimulatedGamePiece(0, 0);
SimulationFieldHandler.addHeldFuel(currentHeldFuel);
}

initializeDepotFuel(0.31, 5.96);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Magic numbers should be named constants.

  • 8 on line 71: What does this represent? Initial held fuel capacity?
  • 0.31, 5.96 on line 76: Depot center coordinates need descriptive constant names.

Also minor: missing space in i <8i < 8.

+    private static final int INITIAL_HELD_FUEL_COUNT = 8;
+    private static final double
+            DEPOT_CENTER_X_METERS = 0.31,
+            DEPOT_CENTER_Y_METERS = 5.96;
+
     // In initializeFuel():
-        for (int i = 0; i <8; i++) {
+        for (int i = 0; i < INITIAL_HELD_FUEL_COUNT; i++) {
             final SimulatedGamePiece currentHeldFuel = new SimulatedGamePiece(0, 0);
             SimulationFieldHandler.addHeldFuel(currentHeldFuel);
         }

-        initializeDepotFuel(0.31, 5.96);
+        initializeDepotFuel(DEPOT_CENTER_X_METERS, DEPOT_CENTER_Y_METERS);

Comment on lines +109 to 127
public boolean atTargetShootingCalculationsAngle(boolean useWideTolerance) {
return atFieldRelativeAngle(shootingCalculations.getTargetShootingState().targetFieldRelativeYaw(), useWideTolerance);
}

public boolean atTargetAngle(boolean useWideTolerance) {
return Math.abs(targetSelfRelativeAngle.minus(getCurrentSelfRelativeAngle()).getRadians())
< (useWideTolerance ? TurretConstants.WIDE_TOLERANCE.getRadians() : TurretConstants.NORMAL_TOLERANCE.getRadians());
return atSelfRelativeAngle(targetSelfRelativeAngle, useWideTolerance);
}

public boolean atFieldRelativeAngle(Rotation2d fieldRelativeAngle, boolean useWideTolerance) {
final double differenceRadians = Math.abs(fieldRelativeAngle.minus(getCurrentFieldRelativeAngle()).getRadians());
final double toleranceRadians = useWideTolerance ? TurretConstants.WIDE_TOLERANCE.getRadians() : TurretConstants.NORMAL_TOLERANCE.getRadians();
return differenceRadians < toleranceRadians;
}

public boolean atSelfRelativeAngle(Rotation2d selfRelativeAngle, boolean useWideTolerance) {
final double differenceRadians = Math.abs(selfRelativeAngle.minus(getCurrentSelfRelativeAngle()).getRadians());
final double toleranceRadians = useWideTolerance ? TurretConstants.WIDE_TOLERANCE.getRadians() : TurretConstants.NORMAL_TOLERANCE.getRadians();
return differenceRadians < toleranceRadians;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Rename new boolean predicates to question-form names.
Lines 109-127 introduce boolean methods with “at…” names; per team conventions, booleans should read as questions (e.g., isAt…). Consider renaming these new methods (and updating call sites) to align with that standard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants