Launch calc#84
Open
arcadeArchitect wants to merge 13 commits into
Open
Conversation
Contributor
arcadeArchitect
commented
Mar 24, 2026
arcadeArchitect
commented
- fix odometry
- add auto shooting -> uses odometry to accurately score in hub using extrapolated data points
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughSwitched pose sourcing from estimated to odometry across navigation and launch code; refactored LaunchCalculator and launcher constants; added launch-aware drive command and telemetry; removed turret from auto-score; tightened vision tunables and transforms; adjusted motor current/soft-limits, motion tuning, and controller bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant Driver as Driver
participant RC as RobotContainer
participant DC as DriveCommands
participant LC as LaunchCalculator
participant V as Vision
participant RS as RobotState
participant F as Flywheel
participant H as ShooterHood
participant I as Indexer
Driver->>RC: bumper + joystick (launch)
RC->>DC: joystickDriveWhileLaunching(...)
DC->>RS: read odometry pose & angular velocity
DC->>LC: request LaunchingParameters(robotToLauncher)
LC->>V: read camera observations & robotToCamera tunables
V-->>LC: cameraToTarget / observations
LC-->>DC: launch setpoints (angle, speed, TOF)
DC->>F: setVelocity(autoInput)
DC->>H: setPosition(autoInput)
F->>RS: update leftFlywheelAtSetpoint(flag)
RS-->>I: allow/deny indexing based on leftFlywheelAtSetpoint
DC-->>RS: publish robotSetpointVelocity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java (1)
117-169:⚠️ Potential issue | 🟠 MajorReset
cameraToTargeton every update.This field is only assigned inside the single-tag branch. When a cycle only has multitag/no-target results, the reused
VisionIOInputsinstance keeps the previous single-tag value and downstream aim code can read a stale target vector. Initialize it to an explicit no-target value before iterating unread results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java` around lines 117 - 169, In updateInputs, reset inputs.cameraToTarget to an explicit "no-target" sentinel before iterating camera.getAllUnreadResults() so a prior single-tag value can't be reused; e.g., set VisionIOInputs.cameraToTarget to null or a dedicated NO_TARGET instance at the start of updateInputs(), then let the single-tag branch continue to assign it when a target is present.src/main/java/org/team5924/frc2026/util/LaunchCalculator.java (1)
345-351:⚠️ Potential issue | 🟠 MajorDon't hardcode the launcher velocity to zero.
This removes the TOF lookahead compensation for robot motion, so the new odometry/extrapolation path only behaves correctly for stationary shots. Reconnect this to the actual field-relative or setpoint velocity before relying on moving auto-shots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java` around lines 345 - 351, The code currently creates a zeroed robot velocity (var robotVelocity = new ChassisSpeeds()) which disables TOF lookahead compensation; replace that with the real field/setpoint velocity (e.g., call RobotState.getInstance().getFieldSetpointVelocity() or the appropriate getter) so launcherVelocity computation uses actual motion; keep robotAngle and the GeomUtil.transformVelocity(robotVelocity, robotToLauncher.getTranslation().toTranslation2d(), robotAngle) path intact so moving autonomous shots are compensated correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/commands/drive/DriveCommands.java`:
- Around line 206-250: The launch code uses
LaunchCalculator.getInstance().getParameters() unguarded, so stale/invalid
solutions can drive turning or make atLaunchGoal() true; fix by gating use with
parameters.isValid(): in joystickDriveWhileLaunching() check
parameters.isValid() before using the returned drive angle (fall back to normal
joystickDrive behavior when false), and in atLaunchGoal() return false
immediately if !parameters.isValid() (so launch gating only succeeds for valid
solutions). Ensure all accesses to parameters.driveAngle() / angle-based math
and any launch gating logic first verify
LaunchCalculator.getInstance().getParameters().isValid().
In `@src/main/java/org/team5924/frc2026/Robot.java`:
- Around line 129-133: The low-battery reset (disabledTimer.reset() and
lowBatteryAlert.set(false)) was placed in the Robot constructor behind a
DriverStation.isEnabled() check so it only ran once at startup; move that logic
out of the constructor and into the robot's disabled entry point so
disabledTimer measures actual disabled time. Remove the
DriverStation.isEnabled() block from the constructor and instead call
disabledTimer.reset() and lowBatteryAlert.set(false) inside disabledInit() (or
the method that runs when the robot transitions to disabled) so the timer and
alert are reset each time the robot becomes disabled.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 352-371: The right-bumper Commands.runOnce handlers currently list
indexer as a required subsystem though the lambdas only touch intakePivot and
intake; change the required subsystem from indexer to intake (i.e., update the
Commands.runOnce(...) calls associated with driveController.rightBumper() to
pass intake instead of indexer) so the correct subsystem is
claimed—alternatively, if the bumper should also control the indexer, explicitly
set its state in the lambdas (modify the same Commands.runOnce usages,
intakePivot, intake, indexer, driveController.rightBumper()).
In `@src/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.java`:
- Around line 75-77: The readiness bit is being published before the AUTO target
is refreshed, causing a one-cycle stale isAtSetpoint; ensure you update
autoInput (via setAutoInput) before computing/publishing isAtSetpoint and before
clamping logic—move the call that pulls the new speed from LaunchCalculator so
setAutoInput(autoSpeed) runs first, then recompute any setpoint/clamp and
finally publish readiness; apply the same ordering fix where similar AUTO
refresh logic appears (refer to the block around lines 153-168).
In
`@src/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHood.java`:
- Around line 83-87: AUTO currently writes to the autoInput field directly
(bypassing the clamp in setAutoInput), which allows commands outside
GeneralShooterHood.MIN_POSITION_RADS/MAX_POSITION_RADS; change every direct
assignment to autoInput (e.g., the ones referenced at lines around 184-186) to
call setAutoInput(value) instead, and additionally ensure any final usage of
autoInput (where the command is applied to the actuator/motor) clamps again or
reads via a getter that returns MathUtil.clamp(autoInput,
GeneralShooterHood.MIN_POSITION_RADS, GeneralShooterHood.MAX_POSITION_RADS) to
guarantee bounds are enforced at the point of actuation.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.java`:
- Line 27: Field name `a` in class VisionIO is unclear; rename it to a
descriptive boolean such as hasTarget or isValid (choose the name that matches
its semantics) and update all usages accordingly. Locate the declaration "public
boolean a = true;" in VisionIO and change the identifier, update any constructor
assignments, getters/setters, method references, and tests to use the new name,
and add a brief Javadoc comment on the field describing its purpose. Ensure
compilation by updating imports/references and run tests to verify behavior.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Around line 314-316: The cache latestParameters is single-valued but
getParameters(boolean isLeft) is called for both robotToLauncherLeft and
robotToLauncherRight, so results get clobbered between sides; modify the cache
to be side-aware (e.g., add latestParametersLeft and latestParametersRight or
use a Map<Boolean, LaunchingParameters> keyed by isLeft or by the Transform) and
update getParameters(boolean isLeft) to read/write the appropriate per-side
cache entry (references: getParameters(boolean), latestParameters,
robotToLauncherLeft, robotToLauncherRight); ensure any other
getParameters(Transform) paths consult the same per-side cache semantics so
simultaneous calls for both sides return correct cached values.
- Around line 162-183: The code is setting the boolean passing true even though
the passing lookup tables (passing*Map) are commented out/empty, causing later
passing*Map.get(...) calls to return invalid data; update the logic around the
boolean passing (the variable named passing and the map population blocks like
hoodAngleMap, flywheelSpeedMap, timeOfFlightMap) to only set passing = true when
the corresponding passing maps are actually populated (e.g., check that the
relevant maps contain entries or that containsKey(distance) before using
passingMap.get(...)), or default passing = false when those maps are empty;
ensure every use site (the code that calls passing*Map.get(...) and any code
that branches on passing) guards with map.containsKey(...) or checks
map.isEmpty() so no get() is called on missing data.
---
Outside diff comments:
In
`@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java`:
- Around line 117-169: In updateInputs, reset inputs.cameraToTarget to an
explicit "no-target" sentinel before iterating camera.getAllUnreadResults() so a
prior single-tag value can't be reused; e.g., set VisionIOInputs.cameraToTarget
to null or a dedicated NO_TARGET instance at the start of updateInputs(), then
let the single-tag branch continue to assign it when a target is present.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Around line 345-351: The code currently creates a zeroed robot velocity (var
robotVelocity = new ChassisSpeeds()) which disables TOF lookahead compensation;
replace that with the real field/setpoint velocity (e.g., call
RobotState.getInstance().getFieldSetpointVelocity() or the appropriate getter)
so launcherVelocity computation uses actual motion; keep robotAngle and the
GeomUtil.transformVelocity(robotVelocity,
robotToLauncher.getTranslation().toTranslation2d(), robotAngle) path intact so
moving autonomous shots are compensated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f94398d6-d24c-4cf3-af73-6b8ce17f1478
📒 Files selected for processing (18)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/Robot.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/RobotState.javasrc/main/java/org/team5924/frc2026/commands/AutoBuilder.javasrc/main/java/org/team5924/frc2026/commands/drive/DriveCommands.javasrc/main/java/org/team5924/frc2026/commands/drive/DriveToPose.javasrc/main/java/org/team5924/frc2026/commands/shooter/AutoScoreCommands.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/intakePivot/IntakePivotIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHood.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/hopper/Hopper.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/indexer/Indexer.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.javasrc/main/java/org/team5924/frc2026/util/LaunchCalculator.javasrc/main/java/org/team5924/frc2026/util/LauncherConstants.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.java (1)
185-188:⚠️ Potential issue | 🟡 Minor
updateSetpointStatebypassessetAutoInputclamping.Line 186 directly assigns to
autoInput, bypassing theMathUtil.clamp(...)guard insetAutoInput(). If a largechangevalue is passed,autoInputcould exceed the 0–150 bounds.♻️ Proposed fix
public void updateSetpointState(double change) { - autoInput = inputs.setpointVelocityRotationsPerSec + change; + setAutoInput(inputs.setpointVelocityRotationsPerSec + change); setGoalState(FlywheelState.SETPOINT); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.java` around lines 185 - 188, The method updateSetpointState currently assigns autoInput directly which bypasses the clamping in setAutoInput and can allow values outside 0–150; change updateSetpointState to compute the new value (inputs.setpointVelocityRotationsPerSec + change) and call setAutoInput(...) with that value (or otherwise apply MathUtil.clamp(min=0, max=150, ...) to that computed value) before calling setGoalState(FlywheelState.SETPOINT) so autoInput is always constrained.src/main/java/org/team5924/frc2026/util/LaunchCalculator.java (1)
156-184:⚠️ Potential issue | 🟡 MinorLookup table range doesn't cover declared min/max distances.
minDistanceis set to0.9andmaxDistanceto4.9, but the lookup tables only have entries from2.388to4.470. Distances outside this range will extrapolate, which can produce unreliable hood angles and flywheel speeds for close-range shots (< 2.388m).If close-range shooting is expected, consider either:
- Adding calibration data points for the lower distance range, or
- Updating
minDistanceto reflect the actual calibrated range soisValidreturns false for uncalibrated distances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java` around lines 156 - 184, The lookup tables (hoodAngleMap, flywheelSpeedMap, timeOfFlightMap) only contain entries from ~2.388 to 4.470 but minDistance/minDistance and maxDistance are set to 0.9 and 4.9, so calls to isValid will allow distances outside the calibrated range and cause unreliable extrapolation; fix by either adding calibrated entries for the lower range (add hoodAngleMap, flywheelSpeedMap, timeOfFlightMap keys for distances < 2.388) or by tightening the validated range (set minDistance to the lowest calibrated key, e.g. 2.388, and optionally adjust passingMinDistance/passingMaxDistance accordingly) and ensure isValid uses those adjusted min/max values.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/util/LaunchCalculator.java (2)
19-19: Unused import.
Rotationfromedu.wpi.first.units.Unitsis imported but never used in this file.-import static edu.wpi.first.units.Units.Rotation;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java` at line 19, Remove the unused static import Rotation from edu.wpi.first.units.Units in LaunchCalculator to clean up imports; open the LaunchCalculator class and delete the line "import static edu.wpi.first.units.Units.Rotation;" (or your IDE's unused import quick-fix) so the file no longer contains an unused symbol.
453-454: Stray semicolon.Line 454 contains an isolated semicolon statement that does nothing.
private static Rotation2d getDriveAngleWithLauncherOffset( Pose2d robotPose, Translation2d target, Transform3d robotToLauncher) { - ;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java` around lines 453 - 454, The method with signature containing Pose2d robotPose, Translation2d target, Transform3d robotToLauncher in LaunchCalculator has a stray isolated semicolon inside its body; remove that lone ";" so the method body begins/continues correctly (i.e., delete the empty statement in the method following the parameter list) and ensure the surrounding code/brace structure remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 441-461: The leftBumper trigger is being composed after binding,
so the gate (.and(() ->
LaunchCalculator.getInstance().getParameters().isValid())) does not affect the
previously bound handlers; change the code to compose the trigger first (e.g.,
capture driveController.leftBumper().and(() ->
LaunchCalculator.getInstance().getParameters().isValid()) into a local Trigger
variable) and then call .onTrue(...) with
AutoScoreCommands.runTrackTargetCommand(shooterHoodLeft, flywheelLeft,
true).alongWith(Commands.runOnce(() ->
indexer.setGoalState(IndexerState.INDEXING), indexer)) and .onFalse(...) with
Commands.runOnce(() -> { flywheelLeft.setGoalState(FlywheelState.OFF);
indexer.setGoalState(IndexerState.OFF); }, flywheelLeft, indexer) on that
composed trigger so both true and false handlers are properly gated.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.java`:
- Line 27: The cameraToTarget field in VisionIO (used in VisionIOInputs) can
carry stale data across cycles; in VisionIOPhotonVision.updateInputs() reset the
per-cycle flag (hasCameraToTarget) to false at the start of the method and only
set it to true when you actually assign cameraToTarget in the single-tag branch,
and ensure VisionIOInputs.cameraToTarget is initialized/cleared on creation so
consumers never see last-cycle values when no new single-tag result arrived.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Line 351: The code creates robotVelocity as a zero ChassisSpeeds instance
which disables velocity-based lookahead; replace or restore the real
field-relative velocity by using
RobotState.getInstance().getFieldSetpointVelocity() (or another valid source)
when computing lookahead in LaunchCalculator, or if zeroing is intentional add a
clear TODO comment explaining why; specifically update the variable
robotVelocity (currently set via new ChassisSpeeds()) so the subsequent
time-of-flight compensation uses the actual chassis velocity rather than
always-zero values.
---
Outside diff comments:
In `@src/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.java`:
- Around line 185-188: The method updateSetpointState currently assigns
autoInput directly which bypasses the clamping in setAutoInput and can allow
values outside 0–150; change updateSetpointState to compute the new value
(inputs.setpointVelocityRotationsPerSec + change) and call setAutoInput(...)
with that value (or otherwise apply MathUtil.clamp(min=0, max=150, ...) to that
computed value) before calling setGoalState(FlywheelState.SETPOINT) so autoInput
is always constrained.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Around line 156-184: The lookup tables (hoodAngleMap, flywheelSpeedMap,
timeOfFlightMap) only contain entries from ~2.388 to 4.470 but
minDistance/minDistance and maxDistance are set to 0.9 and 4.9, so calls to
isValid will allow distances outside the calibrated range and cause unreliable
extrapolation; fix by either adding calibrated entries for the lower range (add
hoodAngleMap, flywheelSpeedMap, timeOfFlightMap keys for distances < 2.388) or
by tightening the validated range (set minDistance to the lowest calibrated key,
e.g. 2.388, and optionally adjust passingMinDistance/passingMaxDistance
accordingly) and ensure isValid uses those adjusted min/max values.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Line 19: Remove the unused static import Rotation from
edu.wpi.first.units.Units in LaunchCalculator to clean up imports; open the
LaunchCalculator class and delete the line "import static
edu.wpi.first.units.Units.Rotation;" (or your IDE's unused import quick-fix) so
the file no longer contains an unused symbol.
- Around line 453-454: The method with signature containing Pose2d robotPose,
Translation2d target, Transform3d robotToLauncher in LaunchCalculator has a
stray isolated semicolon inside its body; remove that lone ";" so the method
body begins/continues correctly (i.e., delete the empty statement in the method
following the parameter list) and ensure the surrounding code/brace structure
remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7a70711-966a-4cd1-80eb-a3d84b571dc8
⛔ Files ignored due to path filters (1)
src/main/java/org/team5924/frc2026/generated/TunerConstants.javais excluded by!**/generated/**
📒 Files selected for processing (7)
src/main/java/org/team5924/frc2026/Robot.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/commands/drive/DriveCommands.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHood.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.javasrc/main/java/org/team5924/frc2026/util/LaunchCalculator.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/team5924/frc2026/Robot.java
- src/main/java/org/team5924/frc2026/commands/drive/DriveCommands.java
- src/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHood.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
441-461:⚠️ Potential issue | 🟠 MajorThe
.onFalse()is bound to rawleftBumper(), not the composed trigger.The
.onTrue()is correctly gated by the composed trigger (lines 441-443), but the.onFalse()at lines 452-461 creates a new rawleftBumper()trigger. This means the OFF command fires on every bumper release, even when the validity check failed and the scoring command never started.🛡️ Proposed fix: bind both handlers to the same composed trigger
- driveController - .leftBumper() - .and(() -> LaunchCalculator.getInstance().getParameters().isValid()) + var autoScoreTrigger = + driveController + .leftBumper() + .and(() -> LaunchCalculator.getInstance().getParameters().isValid()); + autoScoreTrigger .onTrue( AutoScoreCommands.runTrackTargetCommand(shooterHoodLeft, flywheelLeft, true) .alongWith( Commands.runOnce( () -> { indexer.setGoalState(IndexerState.INDEXING); }, indexer))); - driveController - .leftBumper() - .onFalse( + autoScoreTrigger.onFalse( Commands.runOnce( () -> { flywheelLeft.setGoalState(FlywheelState.OFF); indexer.setGoalState(IndexerState.OFF); }, flywheelLeft, indexer));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/RobotContainer.java` around lines 441 - 461, The off-handler is attached to a fresh raw driveController.leftBumper() instead of the composed trigger that includes LaunchCalculator.getInstance().getParameters().isValid(), so the OFF command fires on any bumper release; fix by creating and reusing the composed trigger (e.g., store driveController.leftBumper().and(() -> LaunchCalculator.getInstance().getParameters().isValid()) in a local variable) and then call that trigger's onTrue(...) with AutoScoreCommands.runTrackTargetCommand(...) and onFalse(...) with the Commands.runOnce(...) that sets flywheelLeft and indexer to OFF so both handlers are bound to the same trigger.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java (1)
82-113: Consider extracting duplicate Transform3d construction.The Transform3d construction logic is duplicated between the constructor (lines 82-91) and
updateLoggedTunableNumbers()(lines 98-107). Extract to a private helper method to reduce duplication and risk of divergence.♻️ Proposed refactor
+ private Transform3d buildRobotToCamera() { + return new Transform3d( + new Translation3d( + Units.inchesToMeters(xInches.getAsDouble()), + Units.inchesToMeters(yInches.getAsDouble()), + Units.inchesToMeters(zInches.getAsDouble())), + new Rotation3d( + Units.degreesToRadians(rollDegrees.getAsDouble()), + Units.degreesToRadians(pitchDegrees.getAsDouble()), + Units.degreesToRadians(yawDegrees.getAsDouble()))); + } + // In constructor: - this.robotToCamera = - new Transform3d( - new Translation3d( - Units.inchesToMeters(xInches.getAsDouble()), - Units.inchesToMeters(yInches.getAsDouble()), - Units.inchesToMeters(zInches.getAsDouble())), - new Rotation3d( - Units.degreesToRadians(rollDegrees.getAsDouble()), - Units.degreesToRadians(pitchDegrees.getAsDouble()), - Units.degreesToRadians(yawDegrees.getAsDouble()))); + this.robotToCamera = buildRobotToCamera(); // In updateLoggedTunableNumbers: LoggedTunableNumber.ifChanged( hashCode(), - () -> - robotToCamera = - new Transform3d(...), + () -> robotToCamera = buildRobotToCamera(), xInches, yInches, zInches, rollDegrees, pitchDegrees, yawDegrees);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java` around lines 82 - 113, Extract the duplicated Transform3d construction into a private helper method (e.g., buildRobotToCameraTransform()) and replace both the constructor assignment to robotToCamera and the lambda passed to LoggedTunableNumber.ifChanged in updateLoggedTunableNumbers() with calls to that helper; the helper should read xInches, yInches, zInches, rollDegrees, pitchDegrees, yawDegrees and return the new Transform3d so robotToCamera = buildRobotToCameraTransform() is used in both places to remove duplication.src/main/java/org/team5924/frc2026/util/LaunchCalculator.java (1)
185-258: Consider removing or documenting commented-out calibration data.This large block of commented-out code (along with lines 260-299) reduces readability. If this data is kept for reference during tuning, add a brief comment explaining its purpose. Otherwise, consider removing it and relying on version control history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java` around lines 185 - 258, The large commented-out calibration block in LaunchCalculator (the maps hoodAngleMap, flywheelSpeedMap, timeOfFlightMap and passingHoodAngleMap, passingFlywheelSpeedMap, passingTimeOfFlightMap plus the minDistance/maxDistance/passingMinDistance/passingMaxDistance/phaseDelay values) should either be removed or documented; locate that commented block and either delete it or prepend a short explanatory comment stating it is archival calibration/tuning data kept for offline reference with instructions on where to find the canonical data (or how to re-enable it), so the code remains readable while preserving context for future tuning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java`:
- Around line 182-188: The assignment to inputs.cameraToTarget currently runs
even when tagPose.isPresent() is false, causing cameraToTarget to be set from
Transform3d.kZero; update the code so that the block that constructs
cameraToTargetRotation, computes Units.radiansToDegrees(...), and assigns
inputs.cameraToTarget only executes inside the existing if (tagPose.isPresent())
branch (or add an explicit guard checking tagPose.isPresent()), so you don't
populate inputs.cameraToTarget with zero values unless that behavior is
intentionally documented; refer to symbols cameraToTarget, Transform3d.kZero,
tagPose.isPresent(), Rotation3d, and inputs.cameraToTarget when making the
change.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 441-461: The off-handler is attached to a fresh raw
driveController.leftBumper() instead of the composed trigger that includes
LaunchCalculator.getInstance().getParameters().isValid(), so the OFF command
fires on any bumper release; fix by creating and reusing the composed trigger
(e.g., store driveController.leftBumper().and(() ->
LaunchCalculator.getInstance().getParameters().isValid()) in a local variable)
and then call that trigger's onTrue(...) with
AutoScoreCommands.runTrackTargetCommand(...) and onFalse(...) with the
Commands.runOnce(...) that sets flywheelLeft and indexer to OFF so both handlers
are bound to the same trigger.
---
Nitpick comments:
In
`@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java`:
- Around line 82-113: Extract the duplicated Transform3d construction into a
private helper method (e.g., buildRobotToCameraTransform()) and replace both the
constructor assignment to robotToCamera and the lambda passed to
LoggedTunableNumber.ifChanged in updateLoggedTunableNumbers() with calls to that
helper; the helper should read xInches, yInches, zInches, rollDegrees,
pitchDegrees, yawDegrees and return the new Transform3d so robotToCamera =
buildRobotToCameraTransform() is used in both places to remove duplication.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Around line 185-258: The large commented-out calibration block in
LaunchCalculator (the maps hoodAngleMap, flywheelSpeedMap, timeOfFlightMap and
passingHoodAngleMap, passingFlywheelSpeedMap, passingTimeOfFlightMap plus the
minDistance/maxDistance/passingMinDistance/passingMaxDistance/phaseDelay values)
should either be removed or documented; locate that commented block and either
delete it or prepend a short explanatory comment stating it is archival
calibration/tuning data kept for offline reference with instructions on where to
find the canonical data (or how to re-enable it), so the code remains readable
while preserving context for future tuning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69dc1dc3-500a-4bf6-9877-b3d1219338e8
📒 Files selected for processing (4)
src/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.javasrc/main/java/org/team5924/frc2026/util/LaunchCalculator.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
374-439: Consider removing or organizing commented-out code.There are ~65 lines of commented-out alternative binding implementations. If these are kept for reference/quick switching, consider:
- Moving them to a separate documentation file or code comments at the top
- Using feature flags or config constants to switch behavior programmatically
If they're no longer needed, removing them would improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/RobotContainer.java` around lines 374 - 439, The file contains ~65 lines of commented-out alternative button bindings (references: driveController, Commands.runOnce, flywheelLeft, flywheelRight, shooterHoodLeft, indexer, LaunchCalculator) which clutters readability; either remove these dead blocks or move them to a separate place (e.g., a new DESIGN.md or a dedicated commented-bindings utility) and, if you need quick toggles, convert them into switchable implementations controlled by a boolean FEATURE flag or config constant so the active binding logic remains concise in RobotContainer.src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java (1)
97-108: Minor indentation issue inupdateRobotToCamera().The method body has unnecessary extra indentation (8 spaces vs expected 4), making it inconsistent with the rest of the class.
♻️ Suggested formatting fix
private void updateRobotToCamera() { - robotToCamera = - new Transform3d( - new Translation3d( - Units.inchesToMeters(xInches.getAsDouble()), - Units.inchesToMeters(yInches.getAsDouble()), - Units.inchesToMeters(zInches.getAsDouble())), - new Rotation3d( - Units.degreesToRadians(rollDegrees.getAsDouble()), - Units.degreesToRadians(pitchDegrees.getAsDouble()), - Units.degreesToRadians(yawDegrees.getAsDouble()))); + robotToCamera = + new Transform3d( + new Translation3d( + Units.inchesToMeters(xInches.getAsDouble()), + Units.inchesToMeters(yInches.getAsDouble()), + Units.inchesToMeters(zInches.getAsDouble())), + new Rotation3d( + Units.degreesToRadians(rollDegrees.getAsDouble()), + Units.degreesToRadians(pitchDegrees.getAsDouble()), + Units.degreesToRadians(yawDegrees.getAsDouble()))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java` around lines 97 - 108, The method updateRobotToCamera() has its method body over-indented; adjust the indentation so the body aligns with standard class formatting (reduce the extra 8-space indent to the class's normal 4-space indent) for the robotToCamera assignment and the nested Transform3d/Translation3d/Rotation3d constructors to match the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 374-439: The file contains ~65 lines of commented-out alternative
button bindings (references: driveController, Commands.runOnce, flywheelLeft,
flywheelRight, shooterHoodLeft, indexer, LaunchCalculator) which clutters
readability; either remove these dead blocks or move them to a separate place
(e.g., a new DESIGN.md or a dedicated commented-bindings utility) and, if you
need quick toggles, convert them into switchable implementations controlled by a
boolean FEATURE flag or config constant so the active binding logic remains
concise in RobotContainer.
In
`@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java`:
- Around line 97-108: The method updateRobotToCamera() has its method body
over-indented; adjust the indentation so the body aligns with standard class
formatting (reduce the extra 8-space indent to the class's normal 4-space
indent) for the robotToCamera assignment and the nested
Transform3d/Translation3d/Rotation3d constructors to match the rest of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2d5b29d-1d4f-4fc9-9ea6-34bba058a2ab
📒 Files selected for processing (2)
src/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java
Contributor
✅ Actions performedReviews paused. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.