Simulation #11
Simulation #11
Conversation
* updated position for arm and intake * Update IntakeConstants.java
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
Outdated
Show resolved
Hide resolved
|
@CodeRabbit full review |
|
@levyishai I'll perform a full review of all the changes in this PR. ✅ Actions performedFull review triggered. |
WalkthroughIntroduces type-specific simulation for coral and algae, renames/shared estimators to coral-specific, adds controller bindings, splits collection/ejection/scoring by type, adds dynamic coral scoring locations, extends subsystems with velocity/pose helpers, updates triggers, and wires simulation periodic updates. Multiple public API changes across simulation handlers, constants, and commands. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Driver as Driver/Operator
participant RC as RobotContainer
participant Sim as SimulationFieldHandler
participant Arm as Arm
participant Intake as Intake
participant Trans as Transporter
participant Score as SimulationScoringHandler
Driver->>RC: Trigger: Spawn Coral
RC->>Sim: updateCoralSpawning(robotPose)
Note over Sim: Adds/positions a coral near robot
Driver->>RC: Trigger: Collect Coral
RC->>Sim: update() (periodic)
Sim->>Arm: isCollectingCoral()
Arm-->>Sim: collection pose, velocities
Sim->>Sim: Set HELD_CORAL_INDEX / updateLoad()
Note over Sim: Tracks transfer to end-effector
Driver->>RC: Trigger: Eject Coral
RC->>Sim: update() (periodic)
Sim->>Intake: calculateLinearIntakeVelocity()
Sim->>Arm: calculateLinearArmAndEndEffectorVelocity()
Sim->>Sim: release held coral with velocities
Sim->>Score: checkGamePieceScored()
alt Coral
Score->>Score: Compare to CORAL_SCORING_LOCATIONS
Score->>Sim: Mark scored, remove location
else Algae
Score->>Score: Compare to PROCESSOR_LOCATION
Score->>Sim: Mark scored, remove algae
end
sequenceDiagram
autonumber
participant RC as RobotContainer
participant Auto as CoralAutoDriveCommand
participant Est as CORAL_POSE_ESTIMATOR
participant Swerve as Swerve
RC->>Auto: Schedule CoralAutoDriveCommand
loop While target exists
Auto->>Est: getClosestObjectToRobot()
Est-->>Auto: Tracked coral pose(s)
Auto->>Swerve: Drive toward target (angle, distance)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
120-122: Why make getPositionMeters() public? Confirm API surface and intended consumers.This was previously private (see prior comment). If external consumers (e.g., Arm/visualization) truly need it, fine—otherwise prefer narrowing to package-private or exposing a typed telemetry/supplier to avoid tight coupling.
Run to find external usages and validate exposure scope:
#!/bin/bash # Find all external usages of getPositionMeters() rg -n --type=java -C2 '\bgetPositionMeters\s*\('src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java (2)
33-35: API/Javadoc alignment and prior guidance.Varargs velocity is fine, but prior feedback suggested modeling release via a Transform of velocity. If that guideline still stands, consider an overload taking a Transform3d or clearly document that velocity is summed and transformed internally.
47-52: Bug: release() without velocity leaves stale velocityAtRelease.A zero-velocity drop should reset velocityAtRelease; otherwise the piece continues with the previous ejection velocity.
public void release() { poseAtRelease = fieldRelativePose; timestampAtRelease = Timer.getTimestamp(); + velocityAtRelease = new Translation3d(); // zero out for pure drop updateIsTouchingGround(); }src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
126-133: Incorrect linear velocity calculation for arm movementPer the past review comment, the arm velocity calculation is fundamentally different from wheel velocity. The arm's rotational motion should be converted to linear velocity at the end effector position using the arm length and angular velocity.
The current implementation incorrectly treats arm motor velocity as if it were wheel velocity. For a rotating arm, the linear velocity at the end effector should be calculated as:
linear_velocity = angular_velocity × arm_lengthApply this diff to fix the calculation:
public Translation3d calculateLinearArmVelocity() { - double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); + // Convert rotations/sec to radians/sec and multiply by arm length + double angularVelocityRadPerSec = armMasterMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI; + double linearVelocityMetersPerSec = angularVelocityRadPerSec * ArmConstants.ARM_LENGTH_METERS; return new Translation3d( - Math.cos(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * velocityRotationsPerSecond, - Math.sin(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * velocityRotationsPerSecond, + Math.cos(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * linearVelocityMetersPerSec, + Math.sin(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * linearVelocityMetersPerSec, 0 ); }Note: This requires
ARM_LENGTH_METERSto be defined inArmConstants.
🧹 Nitpick comments (12)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
68-71: Verify pose units and Z-axis sign for COLLECTED_CORAL_POSE.WPILib Pose3d/Translation3d expect meters with Z up. Values look non-metric and Z is negative (below floor). Please confirm units and coordinate frame; if these are inches or an offset, convert and/or document. Consider centralizing in FieldConstants to avoid drift.
Example if these were inches (adjust as needed):
- public static final Pose3d COLLECTED_CORAL_POSE = new Pose3d( - new Translation3d(-24.11, 8.98, -20.68), - new Rotation3d() - ); + public static final Pose3d COLLECTED_CORAL_POSE = new Pose3d( + new Translation3d( + Units.inchesToMeters(-24.11), + Units.inchesToMeters(8.98), + Units.inchesToMeters(-20.68) // verify if Z should be positive (up) + ), + new Rotation3d() + );src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
97-103: Avoid duplicate calls to getFieldRelativeVelocity().Cache once to prevent redundant computation and minor logging noise.
- public Translation3d getFieldRelativeVelocity3d() { - return new Translation3d( - getFieldRelativeVelocity().vxMetersPerSecond, - getFieldRelativeVelocity().vyMetersPerSecond, - 0 - ); - } + public Translation3d getFieldRelativeVelocity3d() { + final ChassisSpeeds frv = getFieldRelativeVelocity(); + return new Translation3d(frv.vxMetersPerSecond, frv.vyMetersPerSecond, 0); + }src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
7-8: Avoid wildcard geometry importPrefer explicit imports to reduce accidental symbol collisions and keep the API surface clear.
20-21: Rename and use a Set for O(1) lookupsThe name is non-professional and List.contains is O(n). Use an immutable Set and a clear name.
- private static final List<Integer> I_HATE_YOU = List.of( - 1, 2, 3, 4, 5, 12, 13, 14, 15, 16 - ); + // Tags intentionally ignored in layout → explain why (broken, unreliable, etc.) + private static final java.util.Set<Integer> IGNORED_TAG_IDS = java.util.Set.of( + 1, 2, 3, 4, 5, 12, 13, 14, 15, 16 + );And update usage:
- if (!I_HATE_YOU.contains(aprilTag.ID)) + if (!IGNORED_TAG_IDS.contains(aprilTag.ID)) tagIDToPose.put(aprilTag.ID, aprilTag.pose.transformBy(TAG_OFFSET));
56-69: XOR flip logic: add a unit test to lock behaviorThe XOR is concise but easy to regress if flags change. Add a small test matrix for LEFT/RIGHT × each clock position.
I can draft a parameterized test asserting expected flips for each clock position if you share the intended truth table.
src/main/java/frc/trigon/robot/misc/objectdetectioncamera/io/SimulationObjectDetectionCameraIO.java (1)
83-84: Handle unknown object IDs explicitly; don’t default to algaeDefaulting every non-coral to algae risks false positives when new types are added. Use an explicit switch and return empty for unknown IDs (optionally log).
- final ArrayList<SimulatedGamePiece> gamePiecesOnField = objectID == SimulatedGamePieceConstants.GamePieceType.CORAL.id ? SimulationFieldHandler.getSimulatedCoral() : SimulationFieldHandler.getSimulatedAlgae(); + final ArrayList<SimulatedGamePiece> gamePiecesOnField; + switch (objectID) { + case SimulatedGamePieceConstants.GamePieceType.CORAL.id: + gamePiecesOnField = SimulationFieldHandler.getSimulatedCoral(); + break; + case SimulatedGamePieceConstants.GamePieceType.ALGAE.id: + gamePiecesOnField = SimulationFieldHandler.getSimulatedAlgae(); + break; + default: + org.littletonrobotics.junction.Logger.recordOutput(hostname + "/UnknownObjectID", objectID); + return new ArrayList<>(); + }src/main/java/frc/trigon/robot/RobotContainer.java (1)
45-50: Rename remaining VOTSL_POSE_ESTIMATOR alias to CORAL_POSE_ESTIMATOR; consider ALGAE_POSE_ESTIMATORNo occurrences of OBJECT_POSE_ESTIMATOR or GamePieceType.GAME_PIECE_TYPE were found. CoralAutoDriveCommand still declares a local alias:
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java —
private static final ObjectPoseEstimator VOTSL_POSE_ESTIMATOR = RobotContainer.CORAL_POSE_ESTIMATOR; (used at ~lines 27, 40, 69). Rename or remove this alias and use RobotContainer.CORAL_POSE_ESTIMATOR directly for consistency.
- If algae tracking is planned, add ALGAE_POSE_ESTIMATOR mirrored from CORAL_POSE_ESTIMATOR.
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (1)
64-64: TODO comment needs clarificationThe TODO comment about checking if intake is open appears incomplete. This condition should be implemented to prevent driving towards game pieces when the intake cannot collect them.
Would you like me to help implement the intake open check condition or create an issue to track this?
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (3)
90-95: Minor: avoid indexOf in a loopindexOf inside the loop is O(n). Use an indexed loop and return i.
- for (SimulatedGamePiece gamePiece : gamePieceList) - if (gamePiece.getDistanceFromPoseMeters(collectionPose) <= intakeTolerance) - return gamePieceList.indexOf(gamePiece); + for (int i = 0; i < gamePieceList.size(); i++) { + if (gamePieceList.get(i).getDistanceFromPoseMeters(collectionPose) <= intakeTolerance) + return i; + }
24-31: Clarify mutability of returned listsgetSimulatedCoral/Algae return live, mutable lists. If external mutation is unintended, return unmodifiable views; otherwise, add javadoc stating callers may mutate.
Possible change (API-impacting):
-import java.util.ArrayList; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; ... - public static ArrayList<SimulatedGamePiece> getSimulatedCoral() { - return CORAL_ON_FIELD; + public static List<SimulatedGamePiece> getSimulatedCoral() { + return Collections.unmodifiableList(CORAL_ON_FIELD); } ... - public static ArrayList<SimulatedGamePiece> getSimulatedAlgae() { - return ALGAE_ON_FIELD; + public static List<SimulatedGamePiece> getSimulatedAlgae() { + return Collections.unmodifiableList(ALGAE_ON_FIELD); }
137-142: Verify frame conventions for held posesCORAL uses TransporterConstants.COLLECTED_CORAL_POSE while ALGAE uses RobotContainer.ARM.calculateAlgaeCollectionPose(). Confirm both are robot-relative (as assumed by calculateHeldGamePieceFieldRelativePose). If not, apply appropriate transforms or rename to make frames explicit.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
80-89: Nit: method name grammarcalculatedCoralScoringLocations() reads like past tense. Prefer calculateCoralScoringLocations().
- public static final ArrayList<FlippablePose3d> CORAL_SCORING_LOCATIONS = calculatedCoralScoringLocations(); + public static final ArrayList<FlippablePose3d> CORAL_SCORING_LOCATIONS = calculateCoralScoringLocations(); ... - private static ArrayList<FlippablePose3d> calculatedCoralScoringLocations() { + private static ArrayList<FlippablePose3d> calculateCoralScoringLocations() {Also applies to: 66-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/frc/trigon/robot/RobotContainer.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java(4 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java(4 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/io/SimulationObjectDetectionCameraIO.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java(3 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(2 hunks)
🔇 Additional comments (24)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
28-35: Clock geometry constants look good; verify angle sign matches field conventionAngles progress as 0, -60, -120, … via
times(-ordinal()). If you expect CCW (+) around the reef, flip the sign.Run-time sanity check suggestion: log
REEF_CLOCK_ANGLESin degrees and confirm the sequence matches your intended ordering (12→0°, 2→±60°, …).src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (4)
15-15: Import simplification looks good.The import statement for Logger has been moved but remains functional. This is a minor organizational change with no impact on functionality.
73-74: LGTM! Consistent update to use CORAL_POSE_ESTIMATOR.The references to
CORAL_POSE_ESTIMATORin bothgetTrackGamePieceCommandandcalculateDistanceFromTrackedCGamePieceare consistent with the overall refactoring from generic object tracking to coral-specific tracking.Also applies to: 80-80
56-56: Minor Javadoc formatting improvement.The whitespace alignment in the
@paramdocumentation is a minor formatting change that improves consistency.
46-46: No action needed — CORAL_POSE_ESTIMATOR is initialized in RobotContainer.
RobotContainer declarespublic static final ObjectPoseEstimator CORAL_POSE_ESTIMATOR = new ObjectPoseEstimator(...)at RobotContainer.java:45–50, so the usage in IntakeAssistCommand is safe.src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (5)
7-7: Import simplification using wildcard is acceptable.The switch to wildcard import for geometry classes simplifies the imports and is appropriate given the multiple geometry classes used in this file.
110-110: Visualization origin change - verify visual alignment.The ARM_VISUALIZATION_ORIGIN_POINT translation has been modified from
(0, 0.0954, 0.9517)to(0, -0.0954, 0.3573). This represents a significant shift in the Y and Z coordinates that will affect the arm's visual representation.Ensure that this new origin point correctly aligns with the robot's actual arm mounting position in the simulation visualization.
114-117: New algae transform looks correct.The
ARM_TO_HELD_ALGAEtransform with translation(0, 0, -0.5855)and zero rotation properly defines the algae holding position relative to the arm.
125-125: Wheel radius constant addition is appropriate.The addition of
WHEEL_RADIUS_METERScalculated from 1.5 inches is properly implemented using the utility conversion function.
91-91: Verified — holding logic covers all defined game-piece types.SimulatedGamePieceConstants.GamePieceType only defines ALGAE and CORAL (src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java), so isHoldingCoral() || isHoldingAlgae() covers all enum types.
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
87-87: Distance sensor simulation aligned with coral-specific logic.The update to use
SimulationFieldHandler.isHoldingCoral()for the distance sensor simulation is consistent with the PR's coral-focused intake functionality.
96-97: Confirm intake visualization origin & large rotation
- INTAKE_VISUALIZATION_ORIGIN_POINT changed:
- Translation: (0, 0.29449, 0.32349) → (0.3234, 0, 0.2944)
- Rotation: (0, MINIMUM_ANGLE.getRadians(), 0) → (0, -2.28, 0) (−2.28 rad = −130.6°)
- Verify this large rotation and the swapped translation axes are intentional and use the same coordinate/units convention as other subsystem visualizations.
- Check/align with: src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java:95, src/main/java/frc/trigon/robot/subsystems/intake/Intake.java:116, src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java:109, src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:195, src/main/java/frc/trigon/robot/subsystems/climber/ClimberConstants.java:71, src/main/java/frc/trigon/robot/subsystems/climber/Climber.java:114, src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java:69,73, src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java:89,93
- If intended, add an inline comment documenting rationale/units; if not, revert or adjust.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java (2)
29-35: Parameter name mismatch in checkAlgaeScoredThe parameter on line 30 should be
ALGAE_SCORING_TOLERANCE_METERSinstead ofALGAE_SCORING_METERSto maintain naming consistency with the coral scoring method.Apply this diff to fix the parameter name:
private static void checkAlgaeScored(SimulatedGamePiece algae) { - if (!isGamePieceScored(algae, SimulatedGamePieceConstants.PROCESSOR_LOCATION, SimulatedGamePieceConstants.ALGAE_SCORING_METERS)) + if (!isGamePieceScored(algae, SimulatedGamePieceConstants.PROCESSOR_LOCATION, SimulatedGamePieceConstants.ALGAE_SCORING_TOLERANCE_METERS)) return;Likely an incorrect or invalid review comment.
17-17: Confirm mutating a static CORAL_SCORING_LOCATIONS is intentional.Declaration: public static final ArrayList CORAL_SCORING_LOCATIONS = calculatedCoralScoringLocations(); (src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java:66). It is modified (remove) in SimulationScoringHandler (src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java:17,23). Make a defensive copy before mutating or make the field immutable if shared-state mutation is not intended.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (3)
85-88: Good implementation of algae collection pose calculationThe new method properly chains transformations to calculate the algae collection pose.
192-192: Good integration of elevator position in visualizationThe visualization pose now correctly incorporates the elevator's vertical position.
117-133: ARM_LENGTH_METERS is defined — no change required.Defined in src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (line 69: ARM_LENGTH_METERS = 0.67); referenced at lines 80 and 101.
src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (3)
27-27: Renamed pose estimator reference is correctly updatedThe conditional now properly uses
VOTSL_POSE_ESTIMATORwhich is aliased toCORAL_POSE_ESTIMATOR.
57-57: Method reference updated correctlyThe method reference properly uses the new class name
CoralAutoDriveCommand.
17-18: Verify CORAL_POSE_ESTIMATOR initialization — OKRobotContainer declares
public static final ObjectPoseEstimator CORAL_POSE_ESTIMATOR = new ObjectPoseEstimator(...)(src/main/java/frc/trigon/robot/RobotContainer.java, lines 44–48).src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
59-61: Logging split by type looks goodSeparate logs for corals and algae improve debugging.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (3)
31-46: Sanity-check starting Z/rotation vs type originCoral starts at z=0.15 with vertical rotation; algae at z=0.5. Verify these agree with GamePieceType origin heights (ALGAE 0.2, CORAL 0.06) and intended on-field setups.
91-118: Transform composition for scoring locations is solidLevel/clock-face vectors, branch transform, and alignment compose correctly and are easy to extend.
66-71: Confirm PROCESSOR pose + feeder coordinates and alliance‑flipping
- SimulatedGamePieceConstants.PROCESSOR_LOCATION is a non‑flippable Pose3d set to (0,0,0) and is used by SimulationScoringHandler.checkAlgaeScored (SimulationScoringHandler.java:30). Source the real processor pose from FieldConstants or make it a FlippablePose3d so it flips with alliance.
- LEFT_FEEDER_POSITION / RIGHT_FEEDER_POSITION are FlippableTranslation2d (SimulatedGamePieceConstants.java:68–70) and thus already support flipping; verify the numeric coordinates or reference FieldConstants if canonical feeder positions exist.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java
Show resolved
Hide resolved
|
@ShmayaR some fire CRs here |
✅ Actions performedFull review triggered. |
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 (4)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
50-54: Telemetry units bug: rotations logged as meters; velocity likely in rot/s.linearPosition should use meters, and velocity should be converted to m/s.
- .linearPosition(Units.Meters.of(getPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(masterMotor.getSignal(TalonFXSignal.VELOCITY))) + .linearPosition(Units.Meters.of(getPositionMeters())) + .linearVelocity(Units.MetersPerSecond.of( + rotationsToMeters(masterMotor.getSignal(TalonFXSignal.VELOCITY)) + ))src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (2)
51-59: Guard against null race in supplier usage (add WaitUntil + null-safe lambdas).Between the predicate and these lambdas,
distanceFromTrackedGamePiece.get()can turn null and NPE. Gate on non-null and make the suppliers null-safe.Apply:
public static Command getDriveToGamePieceCommand(Supplier<Translation2d> distanceFromTrackedGamePiece) { return new SequentialCommandGroup( - new InstantCommand(() -> PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.reset(distanceFromTrackedGamePiece.get().getX(), RobotContainer.SWERVE.getSelfRelativeVelocity().vxMetersPerSecond)), + new WaitUntilCommand(() -> distanceFromTrackedGamePiece.get() != null), + new InstantCommand(() -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + if (d != null) { + PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.reset( + d.getX(), RobotContainer.SWERVE.getSelfRelativeVelocity().vxMetersPerSecond); + } + }), SwerveCommands.getClosedLoopSelfRelativeDriveCommand( - () -> PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.calculate(distanceFromTrackedGamePiece.get().getX()), - () -> PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_Y_PID_CONTROLLER.calculate(distanceFromTrackedGamePiece.get().getY()), + () -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + return d == null ? 0 : PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.calculate(d.getX()); + }, + () -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + return d == null ? 0 : PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_Y_PID_CONTROLLER.calculate(d.getY()); + }, CoralAutoDriveCommand::calculateTargetAngle ) ); }
67-76: Avoid returning null rotation; provide a safe fallback.Returning null can break consumers. Return current robot heading when no object is tracked.
public static FlippableRotation2d calculateTargetAngle() { final Pose2d robotPose = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose(); final Translation2d trackedObjectFieldRelativePosition = VOTSL_POSE_ESTIMATOR.getClosestObjectToRobot(); - if (trackedObjectFieldRelativePosition == null) - return null; + if (trackedObjectFieldRelativePosition == null) + return new FlippableRotation2d(robotPose.getRotation(), false);src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
60-69: Null-safety: gate PID reset and suppliers to avoid NPEs.
distanceFromTrackedGamePiece.get()can be null at command start; add a wait and null-safe suppliers.public static Command getAssistIntakeCommand(AssistMode assistMode, Supplier<Translation2d> distanceFromTrackedGamePiece) { return new SequentialCommandGroup( - new InstantCommand(() -> resetPIDControllers(distanceFromTrackedGamePiece.get())), + new WaitUntilCommand(() -> distanceFromTrackedGamePiece.get() != null), + new InstantCommand(() -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + if (d != null) resetPIDControllers(d); + }), SwerveCommands.getClosedLoopSelfRelativeDriveCommand( - () -> calculateTranslationPower(assistMode, distanceFromTrackedGamePiece.get()).getX(), - () -> calculateTranslationPower(assistMode, distanceFromTrackedGamePiece.get()).getY(), - () -> calculateThetaPower(assistMode, distanceFromTrackedGamePiece.get()) + () -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + return d == null ? 0 : calculateTranslationPower(assistMode, d).getX(); + }, + () -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + return d == null ? 0 : calculateTranslationPower(assistMode, d).getY(); + }, + () -> { + final Translation2d d = distanceFromTrackedGamePiece.get(); + return d == null ? 0 : calculateThetaPower(assistMode, d); + } ) ); }
♻️ Duplicate comments (9)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java (3)
33-35: API shape: consider Transform3d for release to carry rotation with velocity.Matches prior feedback; improves expressiveness if spin at release matters.
35-41: Bug: Translation3d.plus() is non-mutating — velocity sum is always zero.Assign the result while summing.
- final Translation3d fieldRelativeReleaseVelocity = new Translation3d(); - for (Translation3d velocityComponent : fieldRelativeReleaseVelocities) - fieldRelativeReleaseVelocity.plus(velocityComponent); - - velocityAtRelease = fieldRelativeReleaseVelocity; + Translation3d summed = new Translation3d(); + for (Translation3d v : fieldRelativeReleaseVelocities) + summed = summed.plus(v); + velocityAtRelease = summed;
47-52: No-arg release keeps stale velocity — likely unintended motion after “drop.”Explicitly zero or delegate to varargs with zero vector so gravity-only drop works deterministically.
- public void release() { - poseAtRelease = fieldRelativePose; - timestampAtRelease = Timer.getTimestamp(); - - updateIsTouchingGround(); - } + public void release() { + release(new Translation3d()); + }src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
129-132: CORAL_COLLECTION_POSE units/Z sign already flagged earlier.See prior review about inches passed as meters and negative Z placing the pose below field level. Please address that first.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java (2)
10-14: Use else-if to avoid redundant checks.This was already flagged; reiterating to change the second
iftoelse if.
16-25: Avoid removing from list during enhanced-for (ConcurrentModificationException).Already flagged: iterate by index descending or use an Iterator and
iterator.remove().src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
117-124: Fix end-effector linear velocity conversion and prefer Rotation2d trig.Convert rotations/s to m/s using circumference; use
Rotation2dtrig for clarity.public Translation3d calculateLinearEndEffectorVelocity() { - double velocityRotationsPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * ArmConstants.WHEEL_RADIUS_METERS; - return new Translation3d( - Math.cos(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * velocityRotationsPerSecond, - Math.sin(RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()) * velocityRotationsPerSecond, - 0 - ); + final double velocityMetersPerSecond = + endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; + final Rotation2d robotRot = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + return new Translation3d(robotRot.cos() * velocityMetersPerSecond, + robotRot.sin() * velocityMetersPerSecond, + 0); }src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (2)
63-68: Bug: algae loop uses coral index/list; also O(n^2) indexOf in both loopsSwap to indexed for-loops, use the correct held index/list, and avoid indexOf.
- for (SimulatedGamePiece coral : CORAL_ON_FIELD) - coral.updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == CORAL_ON_FIELD.indexOf(coral)); - for (SimulatedGamePiece algae : ALGAE_ON_FIELD) - algae.updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == CORAL_ON_FIELD.indexOf(algae)); + for (int i = 0; i < CORAL_ON_FIELD.size(); i++) + CORAL_ON_FIELD.get(i).updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == i); + for (int i = 0; i < ALGAE_ON_FIELD.size(); i++) + ALGAE_ON_FIELD.get(i).updatePeriodically(HELD_ALGAE_INDEX != null && HELD_ALGAE_INDEX == i);
105-123: Bug: coral ejection gated by “collect”; gate by “eject” (and optionally at caller)Use isEjectingCoral(), mirroring algae path; also gate the call site.
private static void updateEjection() { - if (HELD_CORAL_INDEX != null) - updateCoralEjection(); + if (HELD_CORAL_INDEX != null && isEjectingCoral()) + updateCoralEjection(); @@ private static void updateCoralEjection() { final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX); - - if (isCollectingCoral()) { + if (isEjectingCoral()) { heldCoral.release(RobotContainer.ARM.calculateLinearEndEffectorVelocity(), RobotContainer.ARM.calculateLinearArmVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d()); HELD_CORAL_INDEX = null; } }
🧹 Nitpick comments (13)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
54-61: Minor typos in mechanism display names."Tranporter" → "Transporter" in UI strings to keep dashboards clean.
- "TranporterRightMotorMechanism", + "TransporterRightMotorMechanism", ... - "TranporterLeftMotorMechanism", + "TransporterLeftMotorMechanism",src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
97-103: Avoid recomputing field-relative velocity 3 times.Cache once; cheaper and clearer.
- public Translation3d getFieldRelativeVelocity3d() { - return new Translation3d( - getFieldRelativeVelocity().vxMetersPerSecond, - getFieldRelativeVelocity().vyMetersPerSecond, - 0 - ); - } + public Translation3d getFieldRelativeVelocity3d() { + final ChassisSpeeds v = getFieldRelativeVelocity(); + return new Translation3d(v.vxMetersPerSecond, v.vyMetersPerSecond, 0); + }src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java (1)
38-49: Logging keys still use GamePieceAutoDrive prefix.Rename to reflect the new class for easier telemetry filtering.
-Logger.recordOutput("GamePieceAutoDrive/DistanceFromTrackedGamePiece", distanceFromTrackedGamePiece); -Logger.recordOutput("GamePieceAutoDrive/XDistanceFromTrackedGamePiece", PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position); +Logger.recordOutput("CoralAutoDrive/DistanceFromTrackedGamePiece", distanceFromTrackedGamePiece); +Logger.recordOutput("CoralAutoDrive/XDistanceFromTrackedGamePiece", PathPlannerConstants.GAME_PIECE_AUTO_DRIVE_X_PID_CONTROLLER.getSetpoint().position);src/main/java/frc/trigon/robot/misc/objectdetectioncamera/io/SimulationObjectDetectionCameraIO.java (1)
83-95: Prefer explicit mapping from objectID to list (future-proof, clearer failure for unknown IDs).Current ternary funnels any non-CORAL to algae. If more types are added, this silently misroutes. Use an enum lookup/switch and handle default.
- final ArrayList<SimulatedGamePiece> gamePiecesOnField = objectID == SimulatedGamePieceConstants.GamePieceType.CORAL.id ? SimulationFieldHandler.getSimulatedCoral() : SimulationFieldHandler.getSimulatedAlgae(); + final ArrayList<SimulatedGamePiece> gamePiecesOnField = switch ( + SimulatedGamePieceConstants.GamePieceType.fromID(objectID) + ) { + case CORAL -> SimulationFieldHandler.getSimulatedCoral(); + case ALGAE -> SimulationFieldHandler.getSimulatedAlgae(); + default -> new ArrayList<>(); + };If
fromIDisn’t available, add it toGamePieceTypeor map via a small helper here.src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (2)
71-76: Reset stale distance when target is lost.Keep state coherent: set
distanceFromTrackedGamePiece = nullif no target.return new RunCommand(() -> { - if (RobotContainer.CORAL_POSE_ESTIMATOR.getClosestObjectToRobot() != null) - distanceFromTrackedGamePiece = calculateDistanceFromTrackedCGamePiece(); + if (RobotContainer.CORAL_POSE_ESTIMATOR.getClosestObjectToRobot() != null) { + distanceFromTrackedGamePiece = calculateDistanceFromTrackedCGamePiece(); + } else { + distanceFromTrackedGamePiece = null; + } });
78-88: Name consistency: fix typo “CGamePiece”.Rename to
calculateDistanceFromTrackedGamePiecefor consistency with usages elsewhere.src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
129-132: Make CORAL_COLLECTION_POSE a true constant.Mark it
finalto prevent accidental reassignment.-public static Pose3d CORAL_COLLECTION_POSE = new Pose3d( +public static final Pose3d CORAL_COLLECTION_POSE = new Pose3d( new Translation3d(-23.58, 23.73, -20.22), new Rotation3d() );Optionally combine with the units/sign fix suggested in the earlier comment.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (2)
90-95: Prefer nearest piece within tolerance; avoid extra indexOfSelect the closest candidate and return its index in one pass.
- private static Integer getIndexOfCollectedGamePiece(Pose3d collectionPose, ArrayList<SimulatedGamePiece> gamePieceList, double intakeTolerance) { - for (SimulatedGamePiece gamePiece : gamePieceList) - if (gamePiece.getDistanceFromPoseMeters(collectionPose) <= intakeTolerance) - return gamePieceList.indexOf(gamePiece); - return null; - } + private static Integer getIndexOfCollectedGamePiece(Pose3d collectionPose, ArrayList<SimulatedGamePiece> gamePieceList, double intakeTolerance) { + int bestIndex = -1; + double bestDist = Double.POSITIVE_INFINITY; + for (int i = 0; i < gamePieceList.size(); i++) { + final double d = gamePieceList.get(i).getDistanceFromPoseMeters(collectionPose); + if (d <= intakeTolerance && d < bestDist) { + bestDist = d; + bestIndex = i; + } + } + return bestIndex >= 0 ? bestIndex : null; + }
144-150: Guard against stale held index (OOB) to avoid crashAdd a bounds check before get(i).
private static void updateHeldGamePiecePose(Pose3d robotRelativeHeldGamePiecePose, ArrayList<SimulatedGamePiece> gamePieceList, Integer heldGamePieceIndex) { if (heldGamePieceIndex == null) return; + if (heldGamePieceIndex < 0 || heldGamePieceIndex >= gamePieceList.size()) + return; final SimulatedGamePiece heldGamePiece = gamePieceList.get(heldGamePieceIndex); heldGamePiece.updatePose(calculateHeldGamePieceFieldRelativePose(robotRelativeHeldGamePiecePose)); }src/main/java/frc/trigon/robot/constants/FieldConstants.java (2)
71-100: Decouple enum from ordinal; make clockPosition explicit and verify facing flagsAvoid ordinal()-coupled semantics and set the correct isFacingDriverStation per clock.
- public enum ReefClockPosition { - REEF_12_OCLOCK(true), - REEF_2_OCLOCK(true), - REEF_4_OCLOCK(true), - REEF_6_OCLOCK(true), - REEF_8_OCLOCK(true), - REEF_10_OCLOCK(true); - - public final Rotation2d clockAngle; - public final boolean isFacingDriverStation; - public final int clockPosition; - - ReefClockPosition(boolean isFacingDriverStation) { - this.clockAngle = calculateClockAngle(); - this.isFacingDriverStation = isFacingDriverStation; - this.clockPosition = ordinal() == 0 ? 12 : ordinal() * 2; - } + public enum ReefClockPosition { + REEF_12_OCLOCK(12, /* verify */ true), + REEF_2_OCLOCK(2, /* verify */ true), + REEF_4_OCLOCK(4, /* verify */ false), + REEF_6_OCLOCK(6, /* verify */ false), + REEF_8_OCLOCK(8, /* verify */ false), + REEF_10_OCLOCK(10, /* verify */ true); + + public final Rotation2d clockAngle; + public final boolean isFacingDriverStation; + public final int clockPosition; + + ReefClockPosition(int clockPosition, boolean isFacingDriverStation) { + this.clockPosition = clockPosition; + this.isFacingDriverStation = isFacingDriverStation; + this.clockAngle = calculateClockAngle(); + } @@ - private Rotation2d calculateClockAngle() { - return REEF_CLOCK_POSITION_DIFFERENCE.times(-ordinal()); - } + private Rotation2d calculateClockAngle() { + final int stepsFrom12 = (clockPosition % 12) / 2; // 12→0, 2→1, ..., 10→5 + return REEF_CLOCK_POSITION_DIFFERENCE.times(-stepsFrom12); + } }
19-21: Rename “I_HATE_YOU” to a neutral, descriptive nameImproves readability/maintainability.
- private static final List<Integer> I_HATE_YOU = List.of( + private static final List<Integer> IGNORED_TAG_IDS = List.of( 1, 2, 3, 4, 5, 12, 13, 14, 15, 16 ); @@ - if (!I_HATE_YOU.contains(aprilTag.ID)) + if (!IGNORED_TAG_IDS.contains(aprilTag.ID)) tagIDToPose.put(aprilTag.ID, aprilTag.pose.transformBy(TAG_OFFSET));Also applies to: 50-51
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (2)
53-60: Deduplicate clock-face rotations; derive from FieldConstants to keep one source of truthCompute Z-rotation from FieldConstants.REEF_CLOCK_ANGLES; remove per-face constants.
- private static final Rotation3d - REEF_TO_2_OCLOCK_ROTATION = new Rotation3d(0, 0, Math.PI / 3), - REEF_TO_4_OCLOCK_ROTATION = new Rotation3d(0, 0, Math.PI / 3 * 2), - REEF_TO_6_OCLOCK_ROTATION = new Rotation3d(0, 0, Math.PI), - REEF_TO_8_OCLOCK_ROTATION = new Rotation3d(0, 0, Math.PI / 3 * 4), - REEF_TO_10_OCLOCK_ROTATION = new Rotation3d(0, 0, Math.PI / 3 * 5), - REEF_TO_12_OCLOCK_ROTATION = new Rotation3d(0, 0, 0); + // Derive clock-face rotation from FieldConstants.REEF_CLOCK_ANGLES @@ - switch (clockFace) { - case 2 -> reefToClockFaceRotation = REEF_TO_2_OCLOCK_ROTATION; - case 4 -> reefToClockFaceRotation = REEF_TO_4_OCLOCK_ROTATION; - case 6 -> reefToClockFaceRotation = REEF_TO_6_OCLOCK_ROTATION; - case 8 -> reefToClockFaceRotation = REEF_TO_8_OCLOCK_ROTATION; - case 10 -> reefToClockFaceRotation = REEF_TO_10_OCLOCK_ROTATION; - case 12 -> reefToClockFaceRotation = REEF_TO_12_OCLOCK_ROTATION; - default -> reefToClockFaceRotation = new Rotation3d(); - } + { + final int idx = (clockFace % 12) / 2; // 12→0, 2→1, ... + reefToClockFaceRotation = new Rotation3d(0, 0, FieldConstants.REEF_CLOCK_ANGLES[idx].getRadians()); + }Also applies to: 102-110
91-116: Validate inputs for level and clockFaceFail fast on invalid values to avoid silent misplacements.
private static FlippablePose3d calculateCoralScorePose(int level, int clockFace, boolean isLeftBranch) { + if (level < 1 || level > 4) + throw new IllegalArgumentException("level must be in [1,4]"); + if (clockFace < 2 || clockFace > 12 || (clockFace % 2) != 0) + throw new IllegalArgumentException("clockFace must be even in [2,12]");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/frc/trigon/robot/RobotContainer.java(1 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java(4 hunks)src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java(4 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/io/SimulationObjectDetectionCameraIO.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java(3 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationScoringHandler.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T08:52:03.279Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#11
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:117-124
Timestamp: 2025-09-11T08:52:03.279Z
Learning: When working with Rotation2d objects in WPILib, prefer using the built-in trigonometric methods like Rotation2d.cos() and Rotation2d.sin() instead of Math.cos(rotation.getRadians()) and Math.sin(rotation.getRadians()). This is more readable and semantically appropriate.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🔇 Additional comments (11)
src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (2)
6-8: LGTM: geometry imports are appropriate.
68-71: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo "Listing Java files under src (top 200)..."
fd -HI --type f --extension java src | sed -n '1,200p' || trueecho
echo "Searching for TransporterConstants.java (case-sensitive)..."
FILES=$(fd -HI --type f 'TransporterConstants.java' || true)if [ -z "$FILES" ]; then
echo "No TransporterConstants.java found by name."
else
echo "Found TransporterConstants.java files:"
printf '%s\n' "$FILES"
fiecho
echo "Searching repository for the symbol COLLECTED_CORAL_POSE..."
rg -n "COLLECTED_CORAL_POSE" -S || trueecho
echo "Searching for any file paths containing 'transporter'..."
fd -HI --type f 'transporter' src || trueif [ -n "$FILES" ]; then
for f in $FILES; do
echo
echo "---- $f (first 240 lines) ----"
sed -n '1,240p' "$f" | nl -ba -v1 -w3 -s' '
echo "---- end $f ----"
done
fiecho
echo "Searching for 'new Translation3d' occurrences..."
rg -n "new Translation3d" -S || trueecho
echo "Searching for Units usage (Units.inchesToMeters, inchesToMeters, Units.)..."
rg -n "Units\.inchesToMeters|inchesToMeters|Units\." -S || trueecho
echo "Searching for comments/strings mentioning 'inch' or 'meter'..."
rg -n "\binch|\bmeter" -S || true</blockquote></details> <details> <summary>src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)</summary><blockquote> `120-122`: **Visibility change: confirm external usages justify public API.** If this is consumed by Arm/visualization, public makes sense; otherwise prefer package-private. Add brief Javadoc. You can verify usages: ```shell #!/bin/bash rg -n --type=java '\bgetPositionMeters\s*\(' -C2src/main/java/frc/trigon/robot/RobotContainer.java (1)
45-50: LGTM on CORAL_POSE_ESTIMATOR wiring.Constructor args and type look consistent with the wider refactor.
Run to ensure no stale references to the old field remain:
#!/bin/bash # Expect 0 results for OBJECT_POSE_ESTIMATOR usages outside comments rg -nP --type=java '\bOBJECT_POSE_ESTIMATOR\b' -g '!**/build/**'src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
87-88: LGTM: per-type holding state.Switching to
isHoldingCoral()aligns with the new per-type flow.
95-98: Verify visualization origin orientation units.
new Rotation3d(0, -2.28, 0)is ≈ -130.6°. If intended, fine; otherwise consider degrees-to-radians conversion to avoid accidental rad/deg mix.src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (3)
91-92: LGTM: distance sensor sim now respects both piece types.Matches the broader split between coral and algae.
114-118: LGTM: transform for held algae added.This cleanly decouples visualization from held-object frame.
125-125: LGTM: explicit wheel radius constant.Using
Units.inchesToMeters(1.5)(fully-qualified) avoids import conflicts withedu.wpi.first.units.Units.src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
85-88: LGTM: algae collection pose via transform.Good reuse of visualization origin + transform for the algae frame.
190-196: Visualization: nice use of elevator Z offset.Good to see the arm pose composed with elevator position.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
136-144: Compute tangential arm-tip velocity (ω × r); update consumer and verify end-effector wheel directioncalculateLinearArmVelocity currently returns a radial vector (cos,0,sin); it must return the tangential velocity: (-sin·v, 0, cos·v). Affects calculateLinearArmAndEndEffectorVelocity (src/main/java/frc/trigon/robot/subsystems/arm/Arm.java — callers around lines ~125–129) which calls this method and also adds an end-effector contribution using a radial projection. Confirm whether the end-effector wheel velocity is intended to be radial (leave as-is) or tangential (update its projection to (-sv, 0, cv)).
- public Translation3d calculateLinearArmVelocity() { - double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); - double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; - return new Translation3d( - getAngle().getCos() * velocityMagnitude, - 0, - getAngle().getSin() * velocityMagnitude - ); - } + public Translation3d calculateLinearArmVelocity() { + final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); + final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; + final double c = getAngle().getCos(); + final double s = getAngle().getSin(); + // Tangential (robot frame, rotation about +Y): v = (-s * v, 0, c * v) + return new Translation3d(-s * tipMps, 0, c * tipMps); + }Sanity checks: θ=0° → (0,0,±v); θ=90° → (±v,0,0). Flip both components if signs are reversed vs encoder direction.
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
87-90: Document the frame: this pose is robot‑relative.Add a brief Javadoc so call sites don’t confuse robot‑relative vs field‑relative poses.
+ /** + * Robot-relative pose of the held game piece at the end effector. + */ public Pose3d calculateGamePieceCollectionPose() { return calculateVisualizationPose() .transformBy(ArmConstants.ARM_TO_HELD_GAME_PIECE); }
120-123: Fix log key casing; clarify “coral-only” semantics.
- Key casing typo: “InEndeffector” → “InEndEffector”.
- Method name reads generic, but it returns coral‑specific state; either rename (e.g., inEndEffectorCoral) or add Javadoc stating it’s coral‑only.
- @AutoLogOutput(key = "Arm/InEndeffector") + @AutoLogOutput(key = "Arm/InEndEffector") public boolean inEndEffector() { return SimulationFieldHandler.isCoralInEndEffector(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (4)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(44-121)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(16-200)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-261)
🪛 Gitleaks (8.27.2)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
[high] 120-120: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
115-118: LGTM: telemetry hook for collection detection.Good use of debounced event + AutoLogOutput.
201-206: LGTM: elevator Z offset in visualization.Using ELEVATOR.getPositionMeters() on Z matches WPILib’s X-forward, Y-left, Z-up convention.
Also applies to: 203-203
| public Translation3d calculateLinearArmAndEndEffectorVelocity() { | ||
| double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; | ||
| return calculateLinearArmVelocity().plus( | ||
| new Translation3d( | ||
| getAngle().getCos() * velocityMetersPerSecond, | ||
| getAngle().getSin() * velocityMetersPerSecond, | ||
| 0 | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
End‑effector velocity uses XY plane and returns robot frame where field frame is required.
- The added component is placed in X–Y with z=0, but the arm pitches about Y, so radial/tangential motion lies in X–Z.
- Simulation expects field‑relative velocities when releasing pieces; this method currently returns robot‑relative.
Apply this fix: compute wheel velocity along the arm (robot X–Z), sum with the arm tip’s tangential velocity (from calculateLinearArmVelocity()), then rotate XY by robot heading to return field‑relative.
- public Translation3d calculateLinearArmAndEndEffectorVelocity() {
- double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS;
- return calculateLinearArmVelocity().plus(
- new Translation3d(
- getAngle().getCos() * velocityMetersPerSecond,
- getAngle().getSin() * velocityMetersPerSecond,
- 0
- )
- );
- }
+ public Translation3d calculateLinearArmAndEndEffectorVelocity() {
+ final double endEffectorRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY);
+ final double wheelMps = endEffectorRps * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS;
+
+ // Wheel contribution along arm axis (robot frame: XZ plane)
+ final double c = getAngle().getCos();
+ final double s = getAngle().getSin();
+ final Translation3d wheelRobot = new Translation3d(c * wheelMps, 0, s * wheelMps);
+
+ // Sum with arm tip tangential velocity (robot frame)
+ final Translation3d combinedRobot = calculateLinearArmVelocity().plus(wheelRobot);
+
+ // Rotate to field frame
+ final Rotation2d heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation();
+ final Translation2d xyField = new Translation2d(combinedRobot.getX(), combinedRobot.getY()).rotateBy(heading);
+ return new Translation3d(xyField.getX(), xyField.getY(), combinedRobot.getZ());
+ }Verify call‑site expectations and SimulatedGamePiece.release(...) Javadoc:
🏁 Script executed:
#!/bin/bash
# Show call sites of these APIs and release signature/comments
rg -n -C2 "calculateLinearArmAndEndEffectorVelocity|calculateLinearArmVelocity" --type java
rg -n -C3 "void\\s+release\\(" --type java
rg -n -C2 "expects field|field[- ]relative|fieldRelative" --type javaLength of output: 19053
Fix: return field‑relative end‑effector velocity — compute wheel contribution in robot X–Z and rotate to field frame.
calculateLinearArmAndEndEffectorVelocity (src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:125–134) currently treats the wheel contribution as (cos, sin, 0) in robot XY; the arm pitches about Y so the wheel's radial/tangential motion is in robot X–Z. SimulationFieldHandler passes this value into SimulatedGamePiece.release(...), whose Javadoc requires field‑relative velocities (src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java:128,141 and src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java:33–35).
Change the method to: wheelMps = rps * 2π * radius; wheelRobot = (cos(theta)*wheelMps, 0, sin(theta)*wheelMps); combinedRobot = calculateLinearArmVelocity().plus(wheelRobot); rotate combinedRobot.X/Y by the robot heading (use the existing pose estimator or swerve pose) and return Translation3d(x_field, y_field, z_robot).
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/arm/Arm.java around lines 125–134,
the method computes the wheel/tangential contribution in robot X–Y instead of
robot X–Z and returns robot‑relative velocities; change it to compute wheelMps =
endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI *
ArmConstants.WHEEL_RADIUS_METERS, form wheelRobot = (cos(theta)*wheelMps, 0,
sin(theta)*wheelMps), add that to calculateLinearArmVelocity() to get
combinedRobot, then rotate the combinedRobot X and Y components into field frame
using the robot heading from the pose estimator / swerve pose (leave Z as robot
Z) and return a Translation3d(x_field, y_field, z_robot) so the result is
field‑relative end‑effector velocity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
129-132: CORAL_COLLECTION_POSE coordinates are now reasonable.The updated pose with translation (0.6827, 0, 0) in meters is consistent with the robot's dimensions and places the collection point approximately 68cm forward of the robot origin at ground level, which is appropriate for coral collection.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
139-147: Bug: arm tip velocity direction is radial, not tangential.For rotation about Y, tangential direction in robot frame is (sinθ, 0, -cosθ). Current code uses (cosθ, 0, sinθ), which is radial and 90° off, skewing release physics.
- public Translation3d calculateLinearArmVelocity() { - double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); - double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; - return new Translation3d( - getAngle().getCos() * velocityMagnitude, - 0, - getAngle().getSin() * velocityMagnitude - ); - } + public Translation3d calculateLinearArmVelocity() { + final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); // verify units: mechanism rps vs rotor rps/ratio + final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; + final double s = getAngle().getSin(); + final double c = getAngle().getCos(); + // Tangential direction for rotation about +Y: (sinθ, 0, -cosθ) * |v| + return new Translation3d(tipMps * s, 0, -tipMps * c); + }Would you like me to switch to CANcoder velocity (mechanism‑side) to avoid gear‑ratio ambiguity?
128-137: Bug: end‑effector contribution uses XY plane and robot frame; needs XZ and field frame.Wheel velocity should be along the arm axis in robot XZ and the final result rotated into field frame to match SimulatedGamePiece.release expectations.
- public Translation3d calculateLinearArmAndEndEffectorVelocity() { - double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; - return calculateLinearArmVelocity().plus( - new Translation3d( - getAngle().getCos() * velocityMetersPerSecond, - getAngle().getSin() * velocityMetersPerSecond, - 0 - ) - ); - } + public Translation3d calculateLinearArmAndEndEffectorVelocity() { + final double endEffRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY); + final double wheelMps = endEffRps * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; + final double s = getAngle().getSin(); + final double c = getAngle().getCos(); + + // Wheel contribution along arm axis (robot frame, XZ plane) + final Translation3d wheelRobot = new Translation3d(c * wheelMps, 0, s * wheelMps); + // Sum with arm tangential (robot frame) + final Translation3d combinedRobot = calculateLinearArmVelocity().plus(wheelRobot); + + // Rotate robot XY into field frame; keep Z as robot Z + final Rotation2d heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final Translation2d xyField = new Translation2d(combinedRobot.getX(), combinedRobot.getY()).rotateBy(heading); + return new Translation3d(xyField.getX(), xyField.getY(), combinedRobot.getZ()); + }Please confirm SimulatedGamePiece.release(...) indeed expects field‑relative velocities; if not, rotate at the call site instead.
#!/bin/bash # Verify expectations and call sites rg -n -C2 'void\s+release\(' --type java rg -n -C2 'calculateLinearArmAndEndEffectorVelocity\(' --type java
🧹 Nitpick comments (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (4)
7-7: Nit: wildcard import — OK if consistent with project style.If your style guide prefers explicit imports for readability and IDE assistance, switch back from
geometry.*. Otherwise, ignore.
91-91: Distance-sensor sim supplier: confirm expected value type/scale.
SimpleSensor.setSimulationSuppliertakes a DoubleSupplier; returning 1/0 is fine, but make it explicit 1.0/0.0 and add a short comment clarifying semantics (1 = detected). Also confirm downstream thresholds treat any >0 as “true”.- private static final DoubleSupplier DISTANCE_SENSOR_SIMULATION_SUPPLIER = () -> (SimulationFieldHandler.isHoldingCoral() && SimulationFieldHandler.isCoralInEndEffector()) || SimulationFieldHandler.isHoldingAlgae() ? 1 : 0; + private static final DoubleSupplier DISTANCE_SENSOR_SIMULATION_SUPPLIER = + () -> ((SimulationFieldHandler.isHoldingCoral() && SimulationFieldHandler.isCoralInEndEffector()) + || SimulationFieldHandler.isHoldingAlgae()) ? 1.0 : 0.0; // 1.0 = detected
113-117: Verify transform frame/signs for held-piece pose.Offset (0, 0.1, -0.5855) assumes +Z backward (arm forward = -Z). If your robot frame defines +X forward, consider documenting this here to avoid regressions. No code change required if the visualized pose matches expectations.
117-117: Angle tolerance tuning.5° feels generous for alignment/collection. If you see oscillations or premature “at target,” consider 1–2° in sim and retune on‑bot.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
123-127: Minor: logging key capitalization.Consider “Arm/InEndEffector” (capital E) for consistency with method name and other keys.
- @AutoLogOutput(key = "Arm/InEndeffector") + @AutoLogOutput(key = "Arm/InEndEffector")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(4 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
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.248Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🧬 Code graph analysis (4)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-199)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(16-200)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(16-200)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(16-200)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-265)
🪛 Gitleaks (8.27.2)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
[high] 123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (10)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
80-82: LGTM! Proper state checking implementation.The method correctly implements a tolerance-based position check using the state's target position and the defined tolerance constant. The logic is sound and follows good practices for floating-point comparisons in robotics applications.
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (5)
87-87: DISTANCE_SENSOR_SIMULATION_SUPPLIER logic looks good.The supplier correctly returns 1 when coral is held but not in the end effector (i.e., coral is in the intake), and 0 otherwise. This accurately simulates the distance sensor's behavior.
95-98: INTAKE_VISUALIZATION_ORIGIN_POINT translation updated appropriately.The updated translation (0.3234, 0, 0.2944) appears to be in meters and represents a reasonable position for the intake visualization origin relative to the robot frame.
100-104: Good addition of velocity display mechanism.Adding
MAXIMUM_DISPLAYABLE_VELOCITYand using it in theSpeedMechanism2dconstructor provides a clear visualization scale for the intake velocity in simulation/debugging tools.
133-133: WHEEL_RADIUS_METERS constant properly defined.The wheel radius of 1.5 inches (converted to ~0.0381 meters) is a reasonable value for intake wheels.
220-222: IntakeState angle assignments are intentional and consistent.REST → MINIMUM_ANGLE (8.34°); COLLECT/EJECT → MAXIMUM_ANGLE (70°). Intake.java uses targetState.targetAngle for motor control and soft/limit-switch configuration; COLLECT and EJECT intentionally share the same targetAngle but use different targetVoltage. No callers or tests were found that assume the previous mapping.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
127-127: Wheel radius: confirm dimension.Value uses 1.5 inches. Ensure this is the radius (not diameter). If wheel is 3" OD, this is correct.
241-241: New ArmState LOAD_CORAL — integration check.State is referenced by SimulationFieldHandler.isCoralLoading(); verify bindings/commands transition into/out of this state as intended and that end‑effector voltage (-4) matches the load flow.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
88-91: LGTM: collection pose via fixed transform.Clear and reusable. No issues.
114-117: LGTM: unified “has game piece” signal with debounce.Good use of BooleanEvent. No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (2)
26-32: Prevent external list mutation by returning unmodifiable viewsThese getters expose internal mutable lists, allowing external code to modify the field state directly. This breaks encapsulation and can cause bugs.
+import java.util.Collections; +import java.util.List; - public static ArrayList<SimulatedGamePiece> getSimulatedCoral() { - return CORAL_ON_FIELD; + public static List<SimulatedGamePiece> getSimulatedCoral() { + return Collections.unmodifiableList(CORAL_ON_FIELD); } - public static ArrayList<SimulatedGamePiece> getSimulatedAlgae() { - return ALGAE_ON_FIELD; + public static List<SimulatedGamePiece> getSimulatedAlgae() { + return Collections.unmodifiableList(ALGAE_ON_FIELD); }Also update callers to use
List<SimulatedGamePiece>instead ofArrayListand provide explicit mutation methods if needed (e.g.,removeSimulatedCoral(int index)).
138-142: Fix velocity frame mismatch — ARM returns robot-relative, release() expects field-relativeThe
release()method expects field-relative velocities (see JavaDoc), but ARM'scalculateLinearArmAndEndEffectorVelocity()returns robot-relative. You must rotate the arm velocity to field frame before passing it.if (HELD_ALGAE_INDEX != null && isEjectingAlgae()) { final SimulatedGamePiece heldAlgae = ALGAE_ON_FIELD.get(HELD_ALGAE_INDEX); - heldAlgae.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d()); + final Translation3d armVelRobotFrame = RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(); + final Rotation3d robotHeading = new Rotation3d(0, 0, RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()); + final Translation3d armVelFieldFrame = armVelRobotFrame.rotateBy(robotHeading); + heldAlgae.release(armVelFieldFrame, RobotContainer.SWERVE.getFieldRelativeVelocity3d()); HELD_ALGAE_INDEX = null; }src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
93-120: Add input validation for calculateCoralScorePoseThe method accepts any integer values but only handles specific cases. Invalid inputs silently return empty transforms/rotations, which could cause confusion.
private static FlippablePose3d calculateCoralScorePose(int level, int clockFace, boolean isLeftBranch) { + // Validate inputs + if (level < 1 || level > 4) { + throw new IllegalArgumentException("Level must be 1-4, got: " + level); + } + if (clockFace != 2 && clockFace != 4 && clockFace != 6 && + clockFace != 8 && clockFace != 10 && clockFace != 12) { + throw new IllegalArgumentException("Clock face must be one of {2,4,6,8,10,12}, got: " + clockFace); + } + final Translation3d reefCenterToLevelVector; final Rotation3d reefToClockFaceRotation; final Transform3d coralAlignment; switch (level) { case 1 -> reefCenterToLevelVector = REEF_CENTER_TO_L1_VECTOR; case 2 -> reefCenterToLevelVector = REEF_CENTER_TO_L2_VECTOR; case 3 -> reefCenterToLevelVector = REEF_CENTER_TO_L3_VECTOR; case 4 -> reefCenterToLevelVector = REEF_CENTER_TO_L4_VECTOR; - default -> reefCenterToLevelVector = new Translation3d(); + default -> throw new AssertionError("Unreachable after validation"); }
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (2)
70-75: Performance: avoid O(n²) indexOf() calls in hot loopCalling
indexOf()for every piece on every update is inefficient, especially as the lists grow.private static void updateGamePiecesPeriodically() { - for (SimulatedGamePiece coral : CORAL_ON_FIELD) - coral.updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == CORAL_ON_FIELD.indexOf(coral)); - for (SimulatedGamePiece algae : ALGAE_ON_FIELD) - algae.updatePeriodically(HELD_ALGAE_INDEX != null && HELD_ALGAE_INDEX == ALGAE_ON_FIELD.indexOf(algae)); + for (int i = 0; i < CORAL_ON_FIELD.size(); i++) + CORAL_ON_FIELD.get(i).updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == i); + for (int i = 0; i < ALGAE_ON_FIELD.size(); i++) + ALGAE_ON_FIELD.get(i).updatePeriodically(HELD_ALGAE_INDEX != null && HELD_ALGAE_INDEX == i); }
145-146: Add bounds check to prevent crash from stale indicesIf external code removes pieces from the lists,
HELD_CORAL_INDEXcould become out-of-bounds.private static void updateCoralEjection() { + if (HELD_CORAL_INDEX >= CORAL_ON_FIELD.size()) { + HELD_CORAL_INDEX = null; + return; + } final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX);Also apply to line 139 for algae:
if (HELD_ALGAE_INDEX != null && isEjectingAlgae()) { + if (HELD_ALGAE_INDEX >= ALGAE_ON_FIELD.size()) { + HELD_ALGAE_INDEX = null; + return; + } final SimulatedGamePiece heldAlgae = ALGAE_ON_FIELD.get(HELD_ALGAE_INDEX);And line 181 in
updateHeldGamePiecePose:private static void updateHeldGamePiecePose(Pose3d robotRelativeHeldGamePiecePose, ArrayList<SimulatedGamePiece> gamePieceList, Integer heldGamePieceIndex) { if (heldGamePieceIndex == null) return; + if (heldGamePieceIndex >= gamePieceList.size()) { + // Clear stale index + if (gamePieceList == CORAL_ON_FIELD) HELD_CORAL_INDEX = null; + if (gamePieceList == ALGAE_ON_FIELD) HELD_ALGAE_INDEX = null; + return; + }Also applies to: 177-182
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (2)
104-112: Simplify clock face rotation using FieldConstants.REEF_CLOCK_ANGLESThe switch statement duplicates angle calculations that already exist in FieldConstants.
- switch (clockFace) { - case 2 -> reefToClockFaceRotation = REEF_TO_2_OCLOCK_ROTATION; - case 4 -> reefToClockFaceRotation = REEF_TO_4_OCLOCK_ROTATION; - case 6 -> reefToClockFaceRotation = REEF_TO_6_OCLOCK_ROTATION; - case 8 -> reefToClockFaceRotation = REEF_TO_8_OCLOCK_ROTATION; - case 10 -> reefToClockFaceRotation = REEF_TO_10_OCLOCK_ROTATION; - case 12 -> reefToClockFaceRotation = REEF_TO_12_OCLOCK_ROTATION; - default -> reefToClockFaceRotation = new Rotation3d(); - } + // Map clockFace to index: 12→0, 2→1, 4→2, 6→3, 8→4, 10→5 + final int clockIndex = (clockFace == 12) ? 0 : clockFace / 2; + final Rotation3d reefToClockFaceRotation = new Rotation3d(0, 0, + FieldConstants.REEF_CLOCK_ANGLES[clockIndex].getRadians());Then remove the now-unused REEF_TO_*_OCLOCK_ROTATION constants (lines 52-58).
119-119: Break up complex transform chain for readabilityThe single-line transform chain is difficult to follow and debug.
- return new FlippablePose3d(new Pose3d(new Pose2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, new Rotation2d())).transformBy(new Transform3d(reefCenterToLevelVector.rotateBy(reefToClockFaceRotation), reefToClockFaceRotation)).transformBy(isLeftBranch ? RIGHT_BRANCH_TO_LEFT_BRANCH : new Transform3d()).transformBy(coralAlignment), true); + final Pose3d reefCenter = new Pose3d( + new Pose2d(FieldConstants.BLUE_REEF_CENTER_TRANSLATION, new Rotation2d())); + final Transform3d toClockAndLevel = new Transform3d( + reefCenterToLevelVector.rotateBy(reefToClockFaceRotation), + reefToClockFaceRotation); + final Transform3d toBranch = isLeftBranch ? RIGHT_BRANCH_TO_LEFT_BRANCH : new Transform3d(); + + final Pose3d coralPose = reefCenter + .transformBy(toClockAndLevel) + .transformBy(toBranch) + .transformBy(coralAlignment); + + return new FlippablePose3d(coralPose, true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java(2 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 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: 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/SimulatedGamePieceConstants.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/RobotContainer.java (5)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-62)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(15-37)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(9-44)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-212)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
FieldConstants(15-101)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-265)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(30-232)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(20-117)src/main/java/lib/utilities/flippable/FlippablePose3d.java (1)
FlippablePose3d(12-61)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-124)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
48-53: LGTM! Clean rename from OBJECT → CORALThe rename from
OBJECT_POSE_ESTIMATORtoCORAL_POSE_ESTIMATORcorrectly reflects its specialized purpose for coral tracking. The initialization properly usesGamePieceType.CORAL.
106-110: Commands properly wired to controller triggersThe new coral-specific command bindings are correctly structured — spawn, collection, ejection, and loading all use appropriate trigger conditions (
onTrue/whileTrue). ThePose3dwrapper in line 106 is necessary for the method signature.src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
30-45: Field placement values look reasonableThe coral and algae starting positions are symmetrically distributed at reasonable field locations. The vertical orientation for coral (
CORAL_TO_VERTICAL_POSITION_ROTATION) and heights (0.15m for coral, 0.5m for algae) appear appropriate for the game pieces.src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
41-43: Fix the trigger initialization bug — onlySPAWN_CORAL_TRIGGERgets assignedJava's comma-separated declarations with a single assignment (line 43) assign the value only to the last variable.
CORAL_COLLECTION_TRIGGERremainsnull.Apply this fix to properly initialize both triggers:
- public static final Trigger - CORAL_COLLECTION_TRIGGER = DRIVER_CONTROLLER.leftTrigger().or(OPERATOR_CONTROLLER.c()), - SPAWN_CORAL_TRIGGER = OPERATOR_CONTROLLER.equals(); + public static final Trigger + CORAL_COLLECTION_TRIGGER = DRIVER_CONTROLLER.leftTrigger().or(OPERATOR_CONTROLLER.c()); + public static final Trigger + SPAWN_CORAL_TRIGGER = OPERATOR_CONTROLLER.equals();Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
92-100: Validate feeder selection coordinates and flipping behaviorupdateCoralSpawning picks the nearest feeder by 2D distance; LEFT_FEEDER_POSITION = FlippableTranslation2d(0.923, 7.370, true) and RIGHT_FEEDER_POSITION = FlippableTranslation2d(0.923, 0.668, true) are defined in src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java — confirm these coordinates match your field layout and that FlippableTranslation2d.get() returns translations in the same field frame (including any alliance flipping) before relying on this selection.
| private static void updateCoralEjection() { | ||
| final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX); | ||
|
|
||
| if (isEjectingCoral() && !isCoralInEndEffector()) { | ||
| heldCoral.release(RobotContainer.INTAKE.calculateLinearIntakeVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation()); | ||
| HELD_CORAL_INDEX = null; | ||
| } | ||
| if (isEjectingCoral() && isCoralInEndEffector()) { | ||
| heldCoral.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation()); | ||
| HELD_CORAL_INDEX = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix velocity frame mismatch for coral ejection paths
Same issue: both coral ejection paths pass robot-relative velocities to release() which expects field-relative.
private static void updateCoralEjection() {
final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX);
+ final Rotation3d robotHeading = new Rotation3d(0, 0, RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians());
if (isEjectingCoral() && !isCoralInEndEffector()) {
- heldCoral.release(RobotContainer.INTAKE.calculateLinearIntakeVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation());
+ final Translation3d intakeVelFieldFrame = RobotContainer.INTAKE.calculateLinearIntakeVelocity().rotateBy(robotHeading);
+ heldCoral.release(intakeVelFieldFrame, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation());
HELD_CORAL_INDEX = null;
}
if (isEjectingCoral() && isCoralInEndEffector()) {
- heldCoral.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation());
+ final Translation3d armVelFieldFrame = RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity().rotateBy(robotHeading);
+ heldCoral.release(armVelFieldFrame, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation());
HELD_CORAL_INDEX = null;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void updateCoralEjection() { | |
| final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX); | |
| if (isEjectingCoral() && !isCoralInEndEffector()) { | |
| heldCoral.release(RobotContainer.INTAKE.calculateLinearIntakeVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation()); | |
| HELD_CORAL_INDEX = null; | |
| } | |
| if (isEjectingCoral() && isCoralInEndEffector()) { | |
| heldCoral.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation()); | |
| HELD_CORAL_INDEX = null; | |
| } | |
| } | |
| private static void updateCoralEjection() { | |
| final SimulatedGamePiece heldCoral = CORAL_ON_FIELD.get(HELD_CORAL_INDEX); | |
| final Rotation3d robotHeading = new Rotation3d(0, 0, RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation().getRadians()); | |
| if (isEjectingCoral() && !isCoralInEndEffector()) { | |
| final Translation3d intakeVelFieldFrame = RobotContainer.INTAKE.calculateLinearIntakeVelocity().rotateBy(robotHeading); | |
| heldCoral.release(intakeVelFieldFrame, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation()); | |
| HELD_CORAL_INDEX = null; | |
| } | |
| if (isEjectingCoral() && isCoralInEndEffector()) { | |
| final Translation3d armVelFieldFrame = RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity().rotateBy(robotHeading); | |
| heldCoral.release(armVelFieldFrame, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation()); | |
| HELD_CORAL_INDEX = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
around lines 145 to 156, the two calls to release() pass robot-relative linear
velocities returned by RobotContainer.INTAKE.calculateLinearIntakeVelocity() and
RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), but release()
expects field-relative velocities; convert those robot-relative velocity vectors
into field-relative frame before passing them to release() (e.g., rotate the
velocity by the robot heading/pose from RobotContainer.SWERVE or use an existing
SWERVE helper like toFieldRelative/transformByPose) so both ejection code paths
supply field-relative velocities and then clear HELD_CORAL_INDEX as before.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (1)
21-23: Rename and document excluded tag IDs; use a Set for O(1) lookups.Current name is unclear/unprofessional; also List.contains() is fine here but Set is cleaner.
Apply:
- private static final List<Integer> I_HATE_YOU = List.of( - 1, 2, 3, 4, 5, 12, 13, 14, 15, 16 - ); + // AprilTag IDs intentionally ignored by localization (stage/ambiguous tags) + private static final java.util.Set<Integer> EXCLUDED_TAG_IDS = java.util.Set.of( + 1, 2, 3, 4, 5, 12, 13, 14, 15, 16 + );And in Line 72 update:
- if (!I_HATE_YOU.contains(aprilTag.ID)) + if (!EXCLUDED_TAG_IDS.contains(aprilTag.ID))src/main/java/frc/trigon/robot/RobotContainer.java (1)
105-105: Gate coral spawning to simulation and simplify with runOnce().Prevents accidental use on real hardware; keeps behavior identical in sim.
Apply:
+import edu.wpi.first.wpilibj.RobotBase; @@ - OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())))); + OperatorConstants.SPAWN_CORAL_TRIGGER + .and(RobotBase::isSimulation) + .onTrue(Commands.runOnce( + () -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())) + ));src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
130-133: Logging key typo (“Endeffector”).Use consistent camel-case “EndEffector” to match other keys.
Apply:
- @AutoLogOutput(key = "Arm/InEndeffector") + @AutoLogOutput(key = "Arm/InEndEffector")
📜 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/CoralEjectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java
- src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
- src/main/java/frc/trigon/robot/constants/OperatorConstants.java
- src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
- src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.248Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/RobotContainer.java (6)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-62)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(15-37)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-63)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (1)
ObjectPoseEstimator(15-200)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-212)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)
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/subsystems/arm/Arm.java (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-212)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-265)
🪛 Gitleaks (8.28.0)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
src/main/java/frc/trigon/robot/constants/FieldConstants.java (3)
95-101: Make ReefClockPosition explicit; avoid ordinal-coupled logic; fix facing flags.Reiterating earlier feedback: relying on ordinal() is fragile; and all six facing flags = true is likely wrong. Use explicit clockPosition (12,2,4,6,8,10) and correct per-constant facing.
I can provide a targeted refactor once you confirm the correct facing for each clock.
Also applies to: 120-122
50-58: Duplicate constant block re-declared (compile error). Remove this entire block.These fields are already defined on Lines 40–49. The duplicates will prevent compilation.
Apply:
- 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(); - public static final Translation2d BLUE_REEF_CENTER_TRANSLATION = new Translation2d(4.48945, FIELD_WIDTH_METERS / 2); - 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;Note: also resolves the previously noted “extra newline.”
140-169: Duplicate enum ReefClockPosition (compile error). Keep only one definition.This second copy collides with the first; delete it.
Apply:
- public enum ReefClockPosition { - REEF_12_OCLOCK(true), - REEF_2_OCLOCK(true), - REEF_4_OCLOCK(true), - REEF_6_OCLOCK(true), - REEF_8_OCLOCK(true), - REEF_10_OCLOCK(true); - - public final Rotation2d clockAngle; - public final boolean isFacingDriverStation; - public final int clockPosition; - - ReefClockPosition(boolean isFacingDriverStation) { - this.clockAngle = calculateClockAngle(); - this.isFacingDriverStation = isFacingDriverStation; - this.clockPosition = ordinal() == 0 ? 12 : ordinal() * 2; - } - - public static Rotation2d[] getClockAngles() { - final Rotation2d[] clockAngles = new Rotation2d[ReefClockPosition.values().length]; - for (int i = 0; i < clockAngles.length; i++) - clockAngles[i] = ReefClockPosition.values()[i].clockAngle; - - return clockAngles; - } - - private Rotation2d calculateClockAngle() { - return REEF_CLOCK_POSITION_DIFFERENCE.times(-ordinal()); - } - }Also: prior review already flagged ordinal-coupling and “all true” facing flags on the surviving enum — please address there.
src/main/java/frc/trigon/robot/RobotContainer.java (3)
9-9: Imports LGTM.New imports are minimal and used.
Also applies to: 12-12, 16-16, 24-24
107-110: Controller bindings: OK.Ejection/load/score mappings are consistent with the new simulation flow.
47-52: Confirm no stale OBJECT_POSE_ESTIMATOR referencesFile: src/main/java/frc/trigon/robot/RobotContainer.java (lines 47–52). The prior rg run returned no output — inconclusive. Re-run from the repository root and confirm no hits:
rg -nP '\bOBJECT_POSE_ESTIMATOR\b'
rg -nP '\bObjectPoseEstimator\b'If both return zero matches, the rename is approved.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
11-11: Import of SimulationFieldHandler: OK.Used by inEndEffector().
88-91: Collection pose helper: OK.Transforming visualization pose by ARM_TO_HELD_GAME_PIECE is correct and keeps a single source of truth.
| public enum ReefSide { | ||
| RIGHT(true), | ||
| LEFT(false); | ||
|
|
||
| public final boolean doesFlipYTransformWhenFacingDriverStation; | ||
|
|
||
| ReefSide(boolean doesFlipYTransformWhenFacingDriverStation) { | ||
| this.doesFlipYTransformWhenFacingDriverStation = doesFlipYTransformWhenFacingDriverStation; | ||
| } | ||
|
|
||
| public boolean shouldFlipYTransform(ReefClockPosition reefClockPosition) { | ||
| return doesFlipYTransformWhenFacingDriverStation ^ reefClockPosition.isFacingDriverStation; // In Java, ^ acts as an XOR (exclusive OR) operator, which fits in this case | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate enum ReefSide (compile error). Keep only one definition.
Two identical public enums in the same type will not compile.
Apply:
- public enum ReefSide {
- RIGHT(true),
- LEFT(false);
-
- public final boolean doesFlipYTransformWhenFacingDriverStation;
-
- ReefSide(boolean doesFlipYTransformWhenFacingDriverStation) {
- this.doesFlipYTransformWhenFacingDriverStation = doesFlipYTransformWhenFacingDriverStation;
- }
-
- public boolean shouldFlipYTransform(ReefClockPosition reefClockPosition) {
- return doesFlipYTransformWhenFacingDriverStation ^ reefClockPosition.isFacingDriverStation; // In Java, ^ acts as an XOR (exclusive OR) operator, which fits in this case
- }
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public enum ReefSide { | |
| RIGHT(true), | |
| LEFT(false); | |
| public final boolean doesFlipYTransformWhenFacingDriverStation; | |
| ReefSide(boolean doesFlipYTransformWhenFacingDriverStation) { | |
| this.doesFlipYTransformWhenFacingDriverStation = doesFlipYTransformWhenFacingDriverStation; | |
| } | |
| public boolean shouldFlipYTransform(ReefClockPosition reefClockPosition) { | |
| return doesFlipYTransformWhenFacingDriverStation ^ reefClockPosition.isFacingDriverStation; // In Java, ^ acts as an XOR (exclusive OR) operator, which fits in this case | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/constants/FieldConstants.java around lines 125
to 138, there are two identical public enum ReefSide definitions causing a
duplicate-type compile error; remove the redundant enum declaration and keep
only one canonical ReefSide definition (ensure the retained enum includes the
boolean field, constructor, and shouldFlipYTransform method), then update any
imports/usages if they referenced the removed duplicate so all code points to
the single remaining enum.
| public Translation3d calculateLinearArmAndEndEffectorVelocity() { | ||
| double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; | ||
| return calculateLinearArmVelocity().plus( | ||
| new Translation3d( | ||
| getAngle().getCos() * velocityMetersPerSecond, | ||
| getAngle().getSin() * velocityMetersPerSecond, | ||
| 0 | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public Translation3d calculateLinearArmVelocity() { | ||
| double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); | ||
| double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; | ||
| return new Translation3d( | ||
| getAngle().getCos() * velocityMagnitude, | ||
| 0, | ||
| getAngle().getSin() * velocityMagnitude | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Arm/end-effector velocity is in wrong axes and frame.
- Tangential tip velocity for rotation about Y must be in X–Z: (sinθ, 0, −cosθ), not (cosθ, 0, sinθ).
- Wheel contribution should be along the arm axis (X–Z), not XY.
- Sim release expects field-relative; current methods return robot-frame.
This yields wrong ejection direction and frame mismatch in SimulationFieldHandler.release(...).
Apply:
- public Translation3d calculateLinearArmAndEndEffectorVelocity() {
- double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS;
- return calculateLinearArmVelocity().plus(
- new Translation3d(
- getAngle().getCos() * velocityMetersPerSecond,
- getAngle().getSin() * velocityMetersPerSecond,
- 0
- )
- );
- }
+ public Translation3d calculateLinearArmAndEndEffectorVelocity() {
+ // Robot-frame components
+ final double c = getAngle().getCos();
+ final double s = getAngle().getSin();
+
+ // Arm tip tangential speed (m/s): ω [rad/s] * L
+ final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); // verify rotor vs mechanism units
+ final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS;
+ final Translation3d tangentialRobot = new Translation3d(s * tipMps, 0, -c * tipMps);
+
+ // End-effector wheel linear speed along the arm axis (m/s)
+ final double wheelRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY); // verify rotor vs mechanism units
+ final double wheelMps = wheelRps * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS;
+ final Translation3d wheelRobot = new Translation3d(c * wheelMps, 0, s * wheelMps);
+
+ final Translation3d combinedRobot = tangentialRobot.plus(wheelRobot);
+
+ // Rotate XY into field frame (keep Z as-is)
+ final var heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation();
+ final var xyField = new Translation2d(combinedRobot.getX(), combinedRobot.getY()).rotateBy(heading);
+ return new Translation3d(xyField.getX(), xyField.getY(), combinedRobot.getZ());
+ }
@@
- public Translation3d calculateLinearArmVelocity() {
- double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY);
- double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS;
- return new Translation3d(
- getAngle().getCos() * velocityMagnitude,
- 0,
- getAngle().getSin() * velocityMagnitude
- );
- }
+ public Translation3d calculateLinearArmVelocity() {
+ final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); // verify rotor vs mechanism units
+ final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS;
+ final double c = getAngle().getCos();
+ final double s = getAngle().getSin();
+ final Translation3d tangentialRobot = new Translation3d(s * tipMps, 0, -c * tipMps);
+
+ final var heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation();
+ final var xyField = new Translation2d(tangentialRobot.getX(), tangentialRobot.getY()).rotateBy(heading);
+ return new Translation3d(xyField.getX(), xyField.getY(), tangentialRobot.getZ());
+ }Please verify sensor units (rotor vs mechanism) and apply gear ratios if needed:
🏁 Script executed:
#!/bin/bash
# Find all VELOCITY signal uses to confirm convention
rg -nP --type java 'getSignal\s*\(\s*TalonFXSignal\.VELOCITY\s*\)' -C2
# Check if your TalonFXMotor wrapper converts to mechanism units
rg -nP --type java 'class\s+TalonFXMotor\b' -C3
rg -nP --type java 'ARM_GEAR_RATIO|END_EFFECTOR_GEAR_RATIO' src/main/java/frc/trigon/robot/subsystems/armLength of output: 7822
🏁 Script executed:
#!/bin/bash
# Locate and display the getSignal method implementation in TalonFXMotor
rg -nP --type java 'double getSignal' -C5 src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java
rg -nP --type java 'public .*getSignal' -C5 src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.javaLength of output: 665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate Phoenix6Inputs ==="
rg -n --hidden --line-number 'class Phoenix6Inputs' -S || true
rg -n --hidden --line-number 'class .*Inputs' src/main/java -S || true
echo
echo "=== search for getSignal definitions (all matches) ==="
rg -n --type java 'getSignal\s*\(' -S || true
echo
echo "=== print TalonFXMotor (head) ==="
sed -n '1,240p' src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java || true
echo
echo "=== print Phoenix6Inputs file(s) if found ==="
for f in $(rg -n --hidden --line-number 'class Phoenix6Inputs' -S -g 'src/**' -l || true); do
echo "---- $f ----"
sed -n '1,320p' "$f"
done
echo
echo "=== ArmConstants (top portion) ==="
sed -n '1,320p' src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java || true
echo
echo "=== find SimulationFieldHandler and release usages ==="
rg -n --line-number 'class\s+SimulationFieldHandler' -S || true
rg -n --line-number 'SimulationFieldHandler' -S || true
rg -n --line-number 'release\s*\(' -S || true
echo
echo "=== where calculateLinearArmAndEndEffectorVelocity is referenced ==="
rg -n --line-number 'calculateLinearArmAndEndEffectorVelocity' -S || true
echo
echo "=== find ROBOT_POSE_ESTIMATOR usage ==="
rg -n --line-number 'ROBOT_POSE_ESTIMATOR' -S || true
echo
echo "=== show Arm.getAngle / angle utilities in Arm.java (context) ==="
sed -n '1,260p' src/main/java/frc/trigon/robot/subsystems/arm/Arm.java || trueLength of output: 55845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find TalonFXSignal definition and print it
rg -n --hidden --line-number '^public enum TalonFXSignal' -S || true
rg -n --hidden --line-number 'enum\s+TalonFXSignal' -S || true
file=$(rg -n --hidden --line-number 'enum\s+TalonFXSignal' -S -g 'src/**' -l || true)
if [ -n "$file" ]; then
echo "---- $file ----"
sed -n '1,240p' "$file"
else
echo "TalonFXSignal enum not found by rg"
rg -n --hidden --line-number 'TalonFXSignal' -S -g 'src/**' || true
fiLength of output: 1359
Correct velocity direction vectors and rotate to field frame
- calculateLinearArmVelocity must use the tangential vector (sin θ, 0, −cos θ) in X–Z, not (cos θ, 0, sin θ).
- calculateLinearArmAndEndEffectorVelocity’s wheel term should lie along the arm axis in X–Z, not in the XY‐plane.
- Both methods currently return robot‐frame velocities; SimulationFieldHandler.release expects field‐relative. Rotate the XY components by RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(), preserving Z.
- Verify TalonFXSignal.VELOCITY units given the configured Feedback.RotorToSensorRatio (mechanism vs rotor) and adjust for gear ratios if necessary.
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/arm/Arm.java around lines 135 to
154, the velocity vectors and reference frame are incorrect: change
calculateLinearArmVelocity to use the tangential direction (sinθ, 0, −cosθ) for
X–Z components (not (cosθ,0,sinθ)), make the end‑effector wheel term lie along
the arm axis in X–Z (projected onto X–Z using cos/sin of the arm angle, not into
the XY plane), and then convert both returned Translation3d values from robot
frame to field frame by rotating only the X/Y components using
RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation() while
preserving Z; finally, confirm TalonFXSignal.VELOCITY units and apply the
correct rotor→mechanism scaling (gear ratios / Feedback.RotorToSensorRatio) so
the computed linear speeds use mechanism (arm/wheel) rotations rather than raw
rotor rotations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
130-133: Make CORAL_COLLECTION_POSE a true constantDeclare final to prevent accidental runtime mutation.
- public static Pose3d CORAL_COLLECTION_POSE = new Pose3d( + public static final Pose3d CORAL_COLLECTION_POSE = new Pose3d( new Translation3d(0.6827, 0, 0), new Rotation3d() );
134-134: Duplicate wheel radius constant (diverges easily)WHEEL_RADIUS_METERS exists here and in ArmConstants. Prefer a single source (e.g., a shared EndEffectorConstants) and reference it from both.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
113-117: Held-game-piece transform likely inverted on ZTranslation z = -0.5855 places the piece below the arm origin at 0°; with ARM_VISUALIZATION_ORIGIN_POINT.z ≈ 0.3573 this can go below the field plane. Verify axes (X forward, Y left, Z up) and whether this offset should be along +X instead.
128-128: Duplicate wheel radius constantSame as IntakeConstants; consolidate to one definition.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
130-134: Log key casing nitUse “Arm/InEndEffector” for consistency with naming elsewhere.
- @AutoLogOutput(key = "Arm/InEndeffector") + @AutoLogOutput(key = "Arm/InEndEffector") public boolean inEndEffector() { return SimulationFieldHandler.isCoralInEndEffector(); }src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (3)
18-24: Held-piece tracking by index is brittleExternal list mutations can invalidate indices. Consider storing SimulatedGamePiece references instead; at minimum, add bounds checks before get(i).
70-75: Avoid O(n²) in periodic updatesindexOf inside the loop is O(n); switch to indexed for-loops.
- for (SimulatedGamePiece coral : CORAL_ON_FIELD) - coral.updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == CORAL_ON_FIELD.indexOf(coral)); - for (SimulatedGamePiece algae : ALGAE_ON_FIELD) - algae.updatePeriodically(HELD_ALGAE_INDEX != null && HELD_ALGAE_INDEX == ALGAE_ON_FIELD.indexOf(algae)); + for (int i = 0; i < CORAL_ON_FIELD.size(); i++) + CORAL_ON_FIELD.get(i).updatePeriodically(HELD_CORAL_INDEX != null && HELD_CORAL_INDEX == i); + for (int i = 0; i < ALGAE_ON_FIELD.size(); i++) + ALGAE_ON_FIELD.get(i).updatePeriodically(HELD_ALGAE_INDEX != null && HELD_ALGAE_INDEX == i);
169-175: Add bounds checks when indexing held pieceGuard against stale indices after external list mutations to avoid IndexOutOfBoundsException.
- final SimulatedGamePiece heldGamePiece = gamePieceList.get(heldGamePieceIndex); + if (heldGamePieceIndex < 0 || heldGamePieceIndex >= gamePieceList.size()) return; + final SimulatedGamePiece heldGamePiece = gamePieceList.get(heldGamePieceIndex);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/constants/FieldConstants.java(1 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/constants/FieldConstants.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.248Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (2)
src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (1)
IntakeConstants(31-233)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(20-117)src/main/java/lib/utilities/flippable/FlippablePose3d.java (1)
FlippablePose3d(12-61)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-126)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
src/main/java/frc/trigon/robot/RobotContainer.java (7)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-62)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(15-37)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-203)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-63)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (1)
ObjectPoseEstimator(15-200)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
🪛 Gitleaks (8.28.0)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (13)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
106-106: LGTM: spawn-coral trigger uses field pose correctlyPassing Pose3d constructed from the robot pose is appropriate.
48-53: Rename ripple check — all usages updated to CORAL_POSE_ESTIMATORConfirmed: no remaining OBJECT_POSE_ESTIMATOR matches; CORAL_POSE_ESTIMATOR appears in:
- src/main/java/frc/trigon/robot/RobotContainer.java:48
- src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java:18,27,40,69
- src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java:46,73,80
src/main/java/frc/trigon/robot/subsystems/intake/IntakeConstants.java (3)
88-89: Distance sensor sim condition reads wellUsing “holding coral and not in end effector” matches the transporter path. No issues.
96-99: Verify visualization rotationRotation set to -2 radians (≈ -114.6°). If this is only for visualization it’s fine; if used elsewhere, confirm this matches the real intake zero.
221-224: Confirm EJECT angle = MAXIMUM_ANGLE is intentionalEjecting at the full-extend angle changes clearance/envelope; verify this is safe with adjacent mechanisms and sim geometry.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
91-91: Distance sensor sim logic matches field statesCondition covers coral-in-end-effector and algae. Looks correct.
247-251: Angle updates for SCORE_L4/COLLECT_ALGAE_L3Values look plausible; confirm they satisfy elevator/arm safe-angle rules and do not violate MAXIMUM_ARM_SAFE_ANGLE gating in combined motions.
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (3)
46-49: LGTM: new coral triggersBindings for collection and spawn are clear.
51-54: Controller mapping checkA/B/X/Y to L1–L4 is fine; confirm driver ergonomics match team conventions.
35-44: Remove DRIVER_CONTROLLER.rightTrigger() from CONTINUE_TRIGGERCONTINUE_TRIGGER is referenced by ClimbCommands and CoralPlacingCommands; change the binding to avoid conflicting use of the driver's right trigger.
- CONTINUE_TRIGGER = DRIVER_CONTROLLER.rightTrigger().or(OPERATOR_CONTROLLER.k()), + CONTINUE_TRIGGER = OPERATOR_CONTROLLER.k(),Locations:
- src/main/java/frc/trigon/robot/constants/OperatorConstants.java:40 (declaration)
- src/main/java/frc/trigon/robot/commands/commandfactories/ClimbCommands.java:22 (uses CONTINUE_TRIGGER.getAsBoolean())
- src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:48 (uses CONTINUE_TRIGGER)
Verify RobotContainer's right-trigger binding (ejection) and test affected commands after this change.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
135-144: End-effector velocity: wrong plane and frameYou add XY (z=0) while the arm pitches about Y (motion is in X–Z). Also returns robot-frame; release() paths expect field-frame.
Apply:
- public Translation3d calculateLinearArmAndEndEffectorVelocity() { - double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; - return calculateLinearArmVelocity().plus( - new Translation3d( - getAngle().getCos() * velocityMetersPerSecond, - getAngle().getSin() * velocityMetersPerSecond, - 0 - ) - ); - } + public Translation3d calculateLinearArmAndEndEffectorVelocity() { + final double wheelRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY); + final double wheelMps = wheelRps * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; + final double c = getAngle().getCos(), s = getAngle().getSin(); + // Wheel along arm axis (robot frame, X–Z) + final Translation3d wheelRobot = new Translation3d(c * wheelMps, 0, s * wheelMps); + // Tangential tip (robot frame) + wheel + final Translation3d combinedRobot = calculateLinearArmVelocity().plus(wheelRobot); + // Rotate XY into field frame (preserve Z) + final Rotation2d heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final Translation2d xyField = new Translation2d(combinedRobot.getX(), combinedRobot.getY()).rotateBy(heading); + return new Translation3d(xyField.getX(), xyField.getY(), combinedRobot.getZ()); + }Also verify TalonFXSignal.VELOCITY units are mechanism rps (gear ratio applied).
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (2)
148-155: Critical: release() discards velocity components; rotate mechanism velocities to field frame before releasing corals.
- src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java (lines ~35–38): fieldRelativeReleaseVelocity.plus(velocityComponent); is non‑mutating — Translation3d.plus(...) returns a new instance and the result is discarded. Fix by assigning the result to accumulate, e.g.:
- fieldRelativeReleaseVelocity.plus(velocityComponent); + fieldRelativeReleaseVelocity = fieldRelativeReleaseVelocity.plus(velocityComponent);
- src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (lines 148–155): mechanism-local velocities passed to heldCoral.release(...) must be converted to field frame (or have their producers return field-frame velocities). Suggested change (unchanged from original diff):
- heldCoral.release(RobotContainer.INTAKE.calculateLinearIntakeVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation()); + { + final var heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final var v = RobotContainer.INTAKE.calculateLinearIntakeVelocity(); + final var vField2d = new edu.wpi.first.math.geometry.Translation2d(v.getX(), v.getY()).rotateBy(heading); + final var vField3d = new edu.wpi.first.math.geometry.Translation3d(vField2d.getX(), vField2d.getY(), v.getZ()); + heldCoral.release(vField3d, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), IntakeConstants.CORAL_COLLECTION_POSE.getTranslation()); + } @@ - heldCoral.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation()); + { + final var heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final var v = RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(); + final var vField2d = new edu.wpi.first.math.geometry.Translation2d(v.getX(), v.getY()).rotateBy(heading); + final var vField3d = new edu.wpi.first.math.geometry.Translation3d(vField2d.getX(), vField2d.getY(), v.getZ()); + heldCoral.release(vField3d, RobotContainer.SWERVE.getFieldRelativeVelocity3d(), RobotContainer.ARM.calculateGamePieceCollectionPose().getTranslation()); + }
138-142: Velocity frame mismatch on release — rotate arm velocity to field frame and fix release summationArm.calculateLinearArmAndEndEffectorVelocity() returns a robot-frame Translation3d while SimulatedGamePiece.release(...) expects field-frame velocities. Rotate the arm velocity to the field before calling release (or change Arm to return field-frame and keep calls unchanged). Also fix SimulatedGamePiece.release so it actually sums the incoming Translation3d components (fieldRelativeReleaseVelocity.plus(...) currently discards the result).
- Locations: SimulationFieldHandler.java — algae release (src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java:140) and coral release (…:153). SimulatedGamePiece.release — src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePiece.java (release method, ~lines 35–38).
Suggested fix (algae example — rotate arm velocity to field frame before release):
- final SimulatedGamePiece heldAlgae = ALGAE_ON_FIELD.get(HELD_ALGAE_INDEX); - heldAlgae.release(RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(), RobotContainer.SWERVE.getFieldRelativeVelocity3d()); + final SimulatedGamePiece heldAlgae = ALGAE_ON_FIELD.get(HELD_ALGAE_INDEX); + final var heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final var armVelRobot = RobotContainer.ARM.calculateLinearArmAndEndEffectorVelocity(); + final var fieldArmVel2d = new edu.wpi.first.math.geometry.Translation2d(armVelRobot.getX(), armVelRobot.getY()).rotateBy(heading); + final var armVelField3d = new edu.wpi.first.math.geometry.Translation3d(fieldArmVel2d.getX(), fieldArmVel2d.getY(), armVelRobot.getZ()); + heldAlgae.release(armVelField3d, RobotContainer.SWERVE.getFieldRelativeVelocity3d());Fix SimulatedGamePiece.release accumulation as well (assign plus result or use a mutable sum).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
106-106: Gate sim-only coral spawning and use Commands.runOnce (idiomatic WPILib)Prevents accidental use on real robot and removes superfluous InstantCommand.
@@ -import edu.wpi.first.wpilibj2.command.InstantCommand; +import edu.wpi.first.wpilibj.RobotBase; @@ - OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())))); + OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue( + Commands.runOnce(() -> + SimulationFieldHandler.updateCoralSpawning( + new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose()))) + .onlyIf(RobotBase::isSimulation) + );
109-112: Use onTrue for one‑shot sequences to avoid re‑scheduling loopswhileTrue re-schedules if the command ends while the button remains held (likely for load/score). onTrue is simpler and avoids churn.
- OperatorConstants.DRIVER_CONTROLLER.rightStick().whileTrue(CoralCollectionCommands.getLoadCoralCommand()); + OperatorConstants.DRIVER_CONTROLLER.rightStick().onTrue(CoralCollectionCommands.getLoadCoralCommand()); @@ - OperatorConstants.DRIVER_CONTROLLER.leftBumper().whileTrue(CoralPlacingCommands.getScoreInReefCommand(false)); - OperatorConstants.DRIVER_CONTROLLER.rightBumper().whileTrue(CoralPlacingCommands.getScoreInReefCommand(true)); + OperatorConstants.DRIVER_CONTROLLER.leftBumper().onTrue(CoralPlacingCommands.getScoreInReefCommand(false)); + OperatorConstants.DRIVER_CONTROLLER.rightBumper().onTrue(CoralPlacingCommands.getScoreInReefCommand(true));src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
146-148: Threshold as constant; include equality.Small polish: avoid magic number and treat ≥2.0 V as ejecting.
- public boolean isEjecting() { - return endEffectorMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE) > 2; - } + public boolean isEjecting() { + return endEffectorMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE) >= 2.0; + }Optionally expose ArmConstants.EJECT_VOLTAGE_THRESHOLD_VOLTS = 2.0 and use it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.248Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.248Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/RobotContainer.java (7)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-62)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(15-37)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-203)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-63)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (1)
ObjectPoseEstimator(15-200)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)
🔇 Additional comments (7)
src/main/java/frc/trigon/robot/RobotContainer.java (3)
108-108: Right‑trigger conflict resolved by moving ejection to back() — LGTMThis removes the CONTINUE vs EJECT double‑bind from the previous review.
25-25: SimulationFieldHandler.update() runs in simulationPeriodic — verifiedSimulationFieldHandler.update() is invoked exactly once in src/main/java/frc/trigon/robot/Robot.java (simulationPeriodic(), lines 62–63).
48-53: CORAL_POSE_ESTIMATOR rename verified — no remaining OBJECT_POSE_ESTIMATOR referencesSearch shows zero matches for OBJECT_POSE_ESTIMATOR; CORAL_POSE_ESTIMATOR is referenced in RobotContainer.java, CoralAutoDriveCommand.java, and IntakeAssistCommand.java — CORAL-only tracking is used at those call sites.
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (4)
11-11: Import usage is fine.SimulationFieldHandler is required for InEndEffector; no issues.
130-134: OK to source sim state here.Method name and AutoLogOutput align with SimulationFieldHandler semantics.
88-91: Approve — collection pose derivation is correct; verify in simcalculateGamePieceCollectionPose uses calculateVisualizationPose().transformBy(ArmConstants.ARM_TO_HELD_GAME_PIECE) (src/main/java/frc/trigon/robot/subsystems/arm/Arm.java). ARM_TO_HELD_GAME_PIECE is Translation3d(0, 0.1, -0.5855) with zero rotation (src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java); SimulationFieldHandler uses this pose for held-piece placement. Approving the change — verify in the simulator/field visualization that the offset matches the physical end effector before merging.
135-144: Fix frame/axis: compute robot X–Z tangential + wheel along arm, then rotate XY to field.
- calculateLinearArmAndEndEffectorVelocity currently returns robot‑frame velocities and projects the end‑effector wheel into XY; SimulationFieldHandler expects field‑relative velocities.
- calculateLinearArmVelocity returns a radial vector (cos,sin) instead of the tangential ω×r — both methods need correction.
Apply:
- public Translation3d calculateLinearArmAndEndEffectorVelocity() { - double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; - return calculateLinearArmVelocity().plus( - new Translation3d( - getAngle().getCos() * velocityMetersPerSecond, - getAngle().getSin() * velocityMetersPerSecond, - 0 - ) - ); - } + public Translation3d calculateLinearArmAndEndEffectorVelocity() { + // Robot-frame components + final double c = getAngle().getCos(); + final double s = getAngle().getSin(); + + // Wheel linear speed along arm axis (mechanism rps → m/s) + final double wheelRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY); + final double wheelMps = wheelRps * 2 * Math.PI * ArmConstants.WHEEL_RADIUS_METERS; + final Translation3d wheelRobot = new Translation3d(c * wheelMps, 0, s * wheelMps); + + // Arm tip tangential speed: ωL in robot X–Z (ω about +Y) + final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); + final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; + final Translation3d tangentialRobot = new Translation3d(s * tipMps, 0, -c * tipMps); + + final Translation3d combinedRobot = tangentialRobot.plus(wheelRobot); + + // Rotate XY into field frame; keep Z in robot frame + final Rotation2d heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); + final Translation2d xyField = new Translation2d(combinedRobot.getX(), combinedRobot.getY()).rotateBy(heading); + return new Translation3d(xyField.getX(), xyField.getY(), combinedRobot.getZ()); + }Files to edit: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java — update calculateLinearArmAndEndEffectorVelocity() (and fix/replace calculateLinearArmVelocity() usage or implementation).
| public Translation3d calculateLinearArmVelocity() { | ||
| double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); | ||
| double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS; | ||
| return new Translation3d( | ||
| getAngle().getCos() * velocityMagnitude, | ||
| 0, | ||
| getAngle().getSin() * velocityMagnitude | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Tangential direction is wrong and frame mismatch.
You’re returning a radial vector (cos,0,sin) in robot frame; tangential for rotation about Y is (sin,0,−cos)·(ωL) and release expects field‑relative.
Apply:
- public Translation3d calculateLinearArmVelocity() {
- double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY);
- double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS;
- return new Translation3d(
- getAngle().getCos() * velocityMagnitude,
- 0,
- getAngle().getSin() * velocityMagnitude
- );
- }
+ public Translation3d calculateLinearArmVelocity() {
+ final double armRps = armMasterMotor.getSignal(TalonFXSignal.VELOCITY);
+ final double tipMps = armRps * 2 * Math.PI * ArmConstants.ARM_LENGTH_METERS;
+ final double c = getAngle().getCos(), s = getAngle().getSin();
+ // Robot-frame tangential velocity (ω × r)
+ final Translation3d tangentialRobot = new Translation3d(s * tipMps, 0, -c * tipMps);
+ // Rotate XY into field frame; keep Z as robot Z
+ final Rotation2d heading = RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation();
+ final Translation2d xyField = new Translation2d(tangentialRobot.getX(), tangentialRobot.getY()).rotateBy(heading);
+ return new Translation3d(xyField.getX(), xyField.getY(), tangentialRobot.getZ());
+ }🏁 Script executed:
#!/bin/bash
# Sanity-check orientation at key angles (0°, 90°) by grepping logs/usages
rg -n "calculateLinearArmVelocity|calculateLinearArmAndEndEffectorVelocity" -C2
# Confirm no other code assumes robot-frame return
rg -nP "calculateLinearArmVelocity\\s*\\(" -C2 --type java | rg -v "Arm\\.java"Length of output: 2894
🏁 Script executed:
#!/bin/bash
# Inspect SimulatedGamePiece.release signature and callers, plus Arm/Swerve implementations
echo "---- grep for SimulatedGamePiece definitions ----"
rg -n "class .*SimulatedGamePiece|interface .*SimulatedGamePiece|record .*SimulatedGamePiece" -S || true
echo
echo "---- grep for release( definitions ----"
rg -n "^\s*public .*release\s*\(" -S || rg -n "release\s*\(" -S || true
echo
# show files that look like SimulatedGamePiece
files=$(rg -n "class .*SimulatedGamePiece|interface .*SimulatedGamePiece|record .*SimulatedGamePiece" -S --files || true)
if [ -n "$files" ]; then
for f in $files; do
echo "---- $f ----"
sed -n '1,300p' "$f"
done
fi
echo
echo "---- SimulationFieldHandler.java (first 260 lines) ----"
sed -n '1,260p' src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java || true
echo
echo "---- Arm.java (first 300 lines) ----"
sed -n '1,300p' src/main/java/frc/trigon/robot/subsystems/arm/Arm.java || true
echo
echo "---- Search for getFieldRelativeVelocity3d ----"
rg -n "getFieldRelativeVelocity3d" -S || true
files2=$(rg -n "getFieldRelativeVelocity3d" -S --files || true)
if [ -n "$files2" ]; then
for f in $files2; do
echo "---- $f ----"
sed -n '1,260p' "$f"
done
fiLength of output: 20821
Fix tangential direction and return field‑relative arm velocities.
- calculateLinearArmVelocity() currently returns a radial robot‑frame vector (cos,0,sin)*speed; compute the robot‑frame tangential velocity via ω × r (e.g., (sin,0,−cos)*tipSpeed) and rotate the XY components into the field frame using RobotContainer.ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose().getRotation(); return a field‑relative Translation3d.
- calculateLinearArmAndEndEffectorVelocity() must also rotate the end‑effector wheel (radial) velocity into the same field frame before summing.
- Reason: SimulationFieldHandler passes Arm.calculateLinearArmAndEndEffectorVelocity() into SimulatedGamePiece.release(Translation3d... fieldRelativeReleaseVelocities) which expects field‑relative velocities.
Files: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (calculateLinearArmVelocity, calculateLinearArmAndEndEffectorVelocity); src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java / SimulatedGamePiece.java.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
106-106: Prefer Commands.runOnce over new InstantCommand.Functionally equivalent, reads cleaner and avoids an extra type import.
-OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())))); +OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue( + Commands.runOnce(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose()))) +);src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (3)
32-37: Drop asProxy and consider a timeout.Inside a composed group, asProxy isn’t needed and can obscure requirements. Add a timeout to prevent hanging if detection fails.
- return new ParallelCommandGroup( + return new ParallelCommandGroup( ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.LOAD_CORAL), ElevatorCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.LOAD_CORAL) - ).until(RobotContainer.ARM::hasGamePiece).asProxy(); + ).until(RobotContainer.ARM::hasGamePiece).withTimeout(2.0);
39-44: Don’t schedule align via InstantCommand; remove redundant outer until.
- Compose align+feedback directly;
- The outer .until(INTAKE::hasCoral) duplicates the inner condition and can prematurely end the sequence.
- return new SequentialCommandGroup( - getInitiateCollectionCommand().until(RobotContainer.INTAKE::hasCoral), - new InstantCommand(() -> getAlignCoralCommand().schedule()).alongWith(getCollectionConfirmationCommand()) - ).until(RobotContainer.INTAKE::hasCoral); + return new SequentialCommandGroup( + getInitiateCollectionCommand().until(RobotContainer.INTAKE::hasCoral), + getAlignCoralCommand().alongWith(getCollectionConfirmationCommand()) + );
29-30: Remove commented dead code.Keeps the command factory clean.
- // new IntakeAssistCommand(OperatorConstants.DEFAULT_INTAKE_ASSIST_MODE)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
48-48: Rename KeyboardController.equals() to avoid confusion with Object.equalsA zero‑arg equals() returning Trigger exists at src/main/java/lib/hardware/misc/KeyboardController.java:211–213, so SPAWN_CORAL_TRIGGER = OPERATOR_CONTROLLER.equals(); (src/main/java/frc/trigon/robot/constants/OperatorConstants.java:48) compiles; still recommend renaming to a clearer API (e.g., equalsKey(), equalSign(), toTrigger()) or add an explicit alias and update the call.
51-54: Rebind L1–L4 to numpad + A/B/X/Y — no conflicting A/B/X/Y bindings found.Repo scan found no other references to DRIVER_CONTROLLER.a()/b()/x()/y(); safe to bind. Optional: extract a small helper to map reef level → trigger to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/OperatorConstants.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(15-88)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(29-263)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java (1)
ElevatorCommands(11-59)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(28-198)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-126)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
🔇 Additional comments (5)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
109-112: Bindings look good; re‑check no double‑mapped “continue/eject” remains.Given a prior conflict on rightTrigger, just validating nothing else overlaps these bumpers/back.
#!/bin/bash # Ensure rightTrigger isn't still part of CONTINUE or eject paths. rg -nP --type=java -C2 'rightTrigger\(|CONTINUE_TRIGGER|EJECT' src
48-53: CORAL_POSE_ESTIMATOR rename verified — no remaining OBJECT_POSE_ESTIMATOR usages.
Declared in src/main/java/frc/trigon/robot/RobotContainer.java; used in src/main/java/frc/trigon/robot/commands/commandclasses/CoralAutoDriveCommand.java and src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java. No other OBJECT_POSE_ESTIMATOR occurrences found.src/main/java/frc/trigon/robot/constants/OperatorConstants.java (3)
47-47: LGTM: Coral collection on LT or ‘C’.Combination makes sense and matches common “collect/hold” ergonomics.
36-36: Remap to right-stick — confirmed no duplicate binding; update driver docs.
- rightStick() only appears at src/main/java/frc/trigon/robot/constants/OperatorConstants.java:36 (RESET_HEADING_TRIGGER = DRIVER_CONTROLLER.rightStick()).
- DRIVER_CONTROLLER.y() remains at the same file: line 54 (SET_TARGET_SCORING_REEF_LEVEL_L4_TRIGGER = OPERATOR_CONTROLLER.numpad3().or(DRIVER_CONTROLLER.y())).
Update driver docs to reflect the new mapping.
40-40: Incorrect — CONTINUE_TRIGGER is used as a command completion condition, not a RobotContainer binding.
CONTINUE_TRIGGER is consumed via CONTINUE_TRIGGER.getAsBoolean() in ClimbCommands and passed to .until(...) in CoralPlacingCommands; there are no .onTrue/whileTrue bindings for it, so the "use .onTrue instead of whileTrue" recommendation does not apply.Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
106-106: Prefer runOnce to InstantCommand for one-shot spawn.
Functionally identical, slightly cleaner.- OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue(new InstantCommand(() -> SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())))); + OperatorConstants.SPAWN_CORAL_TRIGGER.onTrue(Commands.runOnce(() -> + SimulationFieldHandler.updateCoralSpawning(new Pose3d(ROBOT_POSE_ESTIMATOR.getEstimatedRobotPose())) + ));src/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
47-52: New coral-related triggers are sensible; minor nit on equals() naming.
Bindings look good. Method name OPERATOR_CONTROLLER.equals() is potentially confusing (overlaps Object.equals semantics).Consider renaming in KeyboardController to equalsKey()/equalSign() and update here accordingly.
41-44: SysID arrow-key bindings are defined but not invoked — no immediate conflict.configureSysIDBindings is defined and wires the arrow-key SysID triggers in src/main/java/frc/trigon/robot/RobotContainer.java (≈lines 114–117), but no call sites were found — SysID bindings are currently inactive during normal reef-side operation. Optional: if you plan to enable SysID at runtime, gate these bindings behind an explicit “SysID mode” flag or move them to unused buttons to avoid overlap with reef-side controls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/constants/OperatorConstants.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/RobotContainer.java (6)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
CoralEjectionCommands(15-37)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
CoralPlacingCommands(24-203)src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
OperatorConstants(10-66)src/main/java/frc/trigon/robot/misc/objectdetectioncamera/ObjectPoseEstimator.java (1)
ObjectPoseEstimator(15-200)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulatedGamePieceConstants.java (1)
SimulatedGamePieceConstants(12-141)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)
🔇 Additional comments (4)
src/main/java/frc/trigon/robot/RobotContainer.java (2)
109-111: Eject/score bindings: resolved the prior rightTrigger conflict.
Eject moved to DRIVER back; score on LB/RB is clear. Good.
48-53: Coral-only pose estimator rename/init looks correct — confirm no stale references.
Renaming to CORAL_POSE_ESTIMATOR and constraining to GamePieceType.CORAL matches the PR’s per-type shift.
Sandbox rg returned no output — run locally to confirm no leftover references:rg -nP '\bOBJECT_POSE_ESTIMATOR\b' -Ssrc/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
53-57: ABXY → L1–L4 mapping reads well.
Clear, mnemonic pairing. No issues.
35-44: Controller mapping changes LGTM — DRIVER_CONTROLLER.rightTrigger() not used elsewhere.
RESET_HEADING and CONTINUE mappings are reasonable.
src/main/java/frc/trigon/robot/constants/OperatorConstants.java:40
Summary by CodeRabbit
New Features
Changes
Bug Fixes