Implement Vision#48
Merged
Merged
Conversation
…clare in RobotContainer constructor
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Vision subsystem (real + sim PhotonVision integrations), Vision I/O types and implementations, camera constants and sim properties, vision filtering/reporting and std‑dev computation, integrates Vision into RobotContainer for modes, updates field geometry constants, and tweaks sim GUI joystick GUID/useGamepad. Changes
Sequence Diagram(s)sequenceDiagram
participant Photon as PhotonVision Camera
participant IO as VisionIO (real / sim)
participant Vision as Vision Subsystem
participant Consumer as VisionConsumer (RobotState / consumer)
Photon->>IO: camera results (multitag / single-tag)
IO->>IO: build PoseObservation[] (timestamp, pose, ambiguity, tagCount, distance)
IO->>Vision: updateInputs(inputs)
Vision->>Vision: aggregate observations across cameras
Vision->>Vision: filter observations (ambiguity, z-error, field bounds)
Vision->>Vision: compute std-devs (baseline + camera + megatag factors)
Vision->>Consumer: accept(pose, timestamp, stdDevMatrix)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![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: 4
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 76-86: The Vision camera-indexing is misaligned because FRONT_LEFT
is commented out: adjust the code so cameraStdDevFactors aligns with the cameras
passed to the Vision constructor (or make the factor explicit per camera).
Either (A) modify the Vision constructor/signature and Vision.periodic to accept
and use a per-camera std-dev factor alongside each VisionIO instance (e.g., pass
a factor with each VisionIOPhotonVision and use that factor instead of
VisionConstants.cameraStdDevFactors[cameraIndex]), or (B) restore index
alignment by instantiating a no-op VisionIO for FRONT_LEFT (using
VisionConstants.FRONT_LEFT_NAME/TRANSFORM) so the order remains [FL, FR, BL,
BR], leaving VisionConstants.cameraStdDevFactors unchanged; update references in
Vision.periodic to use the new per-camera source or preserved indices
accordingly.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java`:
- Line 63: Rename the misspelled constant SIM_ARDUCAM_PROPERIES in
VisionConstants to SIM_ARDUCAM_PROPERTIES; update the declaration in class
VisionConstants and every reference/use of SIM_ARDUCAM_PROPERIES across the
codebase to the new name to avoid breaking references (search for
SIM_ARDUCAM_PROPERIES, change to SIM_ARDUCAM_PROPERTIES in usages such as any
imports, static references, or tests).
- Line 37: The constant CAMERA_Z_LEFT in VisionConstants uses integer division
(9 / 16) so it evaluates to 0; change the expression passed to
Units.inchesToMeters to use floating-point division (e.g., 9 + 9.0/16 or the
decimal 9.5625) so CAMERA_Z_LEFT computes the correct inches-to-meters value.
- Around line 50-53: BACK_LEFT_TRANSFORM currently constructs its Translation3d
with raw inch values (-14.25, 13) instead of meter-converted values; update the
translation to use Units.inchesToMeters(...) for each numeric inch literal (or
replace with the existing CAMERA_X / CAMERA_Y constants if those represent the
same positions) so the Transform3d uses meters like the other transforms; adjust
the construction of BACK_LEFT_TRANSFORM (and keep CAMERA_Z_LEFT and CAMERA_PITCH
as-is) to ensure all translation components are in meters.
🧹 Nitpick comments (6)
src/main/java/org/team5924/frc2026/RobotState.java (1)
42-43: Remove the commented-out annotation instead of leaving dead code.The
@AutoLogOutputannotation has been replaced by an explicitLogger.recordOutputcall inDrive.java(Line 307), so this comment serves no purpose and should be deleted.♻️ Suggested fix
- // `@AutoLogOutput`(key = "RobotState/OdometryPose") `@Getter` `@Setter` private Pose2d odometryPose = new Pose2d();src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java (1)
306-307: CachegetPose()in a local variable to avoid the redundant call.
getPose()is called twice in succession. While it's lightweight, a local variable is cleaner and makes the intent explicit.♻️ Suggested fix
- RobotState.getInstance().setOdometryPose(getPose()); - Logger.recordOutput("RobotState/OdometryPose", getPose()); + Pose2d currentPose = getPose(); + RobotState.getInstance().setOdometryPose(currentPose); + Logger.recordOutput("RobotState/OdometryPose", currentPose);src/main/java/org/team5924/frc2026/Constants.java (1)
249-253: DuplicatefieldWidthconstant and inconsistent naming.
fieldWidth(Line 253) is an exact duplicate ofFIELD_WIDTH(Line 249) — both resolve tofield.getFieldWidth(). Additionally,faceLengthandfieldWidthdon't followUPPER_SNAKE_CASEconvention used by the otherpublic static finalfields in this class.Consider removing the duplicate and renaming
faceLength:♻️ Proposed cleanup
public static final double FIELD_WIDTH = field.getFieldWidth(); public static final double FIELD_LENGTH = field.getFieldLength(); - public static final double faceLength = Units.inchesToMeters(36.792600); - public static final double fieldWidth = field.getFieldWidth(); + public static final double FACE_LENGTH = Units.inchesToMeters(36.792600); public static final Translation2d blueCenter =src/main/java/org/team5924/frc2026/RobotContainer.java (1)
104-126: Minor indentation:new Vision(is misaligned.The body of the
Visionconstructor call isn't indented relative tovision =.🔧 Suggested fix
vision = - new Vision( - drive::addVisionMeasurement, - new VisionIOPhotonVisionSim( + new Vision( + drive::addVisionMeasurement, + new VisionIOPhotonVisionSim(src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVisionSim.java (1)
62-66:visionSim.update()is called once per camera per periodic cycle.Since
visionSimis a shared static instance andVision.periodic()callsupdateInputson each camera sequentially,visionSim.update(poseSupplier.get())runs 4 times per cycle with the same pose. This is functionally safe but redundant.One option is to separate the sim update from per-camera input reading, e.g., by updating the sim once externally (in
RobotContainer.updateSimulation()), but this is a minor optimization.src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java (1)
109-113: Simplify array conversion.The manual loop can be replaced with the
toArraymethod:♻️ Proposed simplification
// Save pose observations to inputs object - inputs.poseObservations = new PoseObservation[poseObservations.size()]; - for (int i = 0; i < poseObservations.size(); i++) { - inputs.poseObservations[i] = poseObservations.get(i); - } + inputs.poseObservations = poseObservations.toArray(new PoseObservation[0]);
…2026 into vision-rework
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/RobotContainer.java (2)
71-71:visionshould befinal.Every other subsystem field (
drive,shooter,shooterHood, etc.) is declaredfinal, preventing accidental reassignment after construction.visionshould follow the same pattern.♻️ Proposed fix
- private Vision vision; + private final Vision vision;🤖 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` at line 71, The field declaration for the Vision subsystem (private Vision vision) in RobotContainer should be made immutable like the other subsystems: change the field to be declared final and update any constructors or initialization code to assign it exactly once (e.g., in the RobotContainer constructor or an initializer) so there are no further reassignments; look for the RobotContainer class and the vision symbol to locate where to add the final modifier and ensure initialization sites (constructors/initializers) set it once.
249-265: TODO note: SIM joystick axes are commented out.The SIM-specific axis mapping (
getRawAxis(0)/getRawAxis(2)) is commented out and replaced with a single unconditionaljoystickDrivebinding. This is fine for competition hardware, but the TODO should be tracked — otherwise simulation driving will use the wrong axes and the dead-stick issue will resurface when the SIM branch is revisited.Do you want me to open a new issue to track restoring the SIM-specific axis mapping?
🤖 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 249 - 265, The SIM-specific joystick axis mapping was commented out; restore a conditional that checks Constants.currentMode == Constants.Mode.SIM and, when true, set drive.setDefaultCommand to DriveCommands.joystickDrive using driveController.getRawAxis(0) and getRawAxis(2) (the original SIM mapping), otherwise use the current mapping that calls getLeftX/getRightX; keep the TODO comment and add or reference a tracking issue/ID in the comment so the SIM branch is not lost again.
🤖 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 138-160: The Vision simulation instances are being given the
odometry supplier (drive::getPose) instead of the ground-truth physics pose,
causing a circular dependency; update the constructor calls that create new
VisionIOPhotonVisionSim (used in Vision) to pass the simulated pose supplier
driveSimulation::getSimulatedDriveTrainPose (or the equivalent ground-truth
method on your DriveSimulation class) instead of drive::getPose so
VisionIOPhotonVisionSim.updateInputs()/visionSim.update(...) receives the true
simulated pose for each camera.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Line 71: The field declaration for the Vision subsystem (private Vision
vision) in RobotContainer should be made immutable like the other subsystems:
change the field to be declared final and update any constructors or
initialization code to assign it exactly once (e.g., in the RobotContainer
constructor or an initializer) so there are no further reassignments; look for
the RobotContainer class and the vision symbol to locate where to add the final
modifier and ensure initialization sites (constructors/initializers) set it
once.
- Around line 249-265: The SIM-specific joystick axis mapping was commented out;
restore a conditional that checks Constants.currentMode == Constants.Mode.SIM
and, when true, set drive.setDefaultCommand to DriveCommands.joystickDrive using
driveController.getRawAxis(0) and getRawAxis(2) (the original SIM mapping),
otherwise use the current mapping that calls getLeftX/getRightX; keep the TODO
comment and add or reference a tracking issue/ID in the comment so the SIM
branch is not lost again.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Constants.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/RobotState.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/team5924/frc2026/Constants.java
- src/main/java/org/team5924/frc2026/RobotState.java
thatnerdjack
requested changes
Feb 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
250-266: Clean up the commented SIM-drive branch.The TODO + commented block makes control intent unclear. If unified axes are now correct, remove the dead branch; if not, restore it behind a clear mode guard.
🤖 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 250 - 266, Remove the dead commented SIM branch or restore it behind a clear mode check: in RobotContainer update the drive.setDefaultCommand usage around DriveCommands.joystickDrive so there is no stale TODO/comment noise—either delete the commented lines referencing Constants.currentMode and Constants.Mode.SIM and keep the current unified axes (driveController.getLeftY(), getLeftX(), getRightX()), or reintroduce the conditional using Constants.currentMode == Constants.Mode.SIM to select the alternate joystick axes (driveController.getLeftY(), getRawAxis(0), getRawAxis(2)) and remove the TODO; ensure only one clear, active branch remains around drive.setDefaultCommand.src/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java (1)
33-33: Calibration TODOs should be tracked to closure before competition tuning.Lines 33, 67-68, and 76-77 indicate placeholder extrinsics/noise/latency values. These directly affect estimator behavior, so this should be captured as a concrete follow-up task with owners/dates.
I can draft a calibration checklist issue template (extrinsics + latency/noise characterization) if you want.
Also applies to: 67-68, 76-77
🤖 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/VisionConstants.java` at line 33, The TODO placeholders for camera extrinsics, measurement noise, and latency in the VisionConstants class must be tracked to closure: create a concrete issue (calibration checklist covering extrinsics, latency, and noise characterization) with an owner and target date, then update the TODO comment in VisionConstants to reference that issue ID/owner/date and mark the current values as "PLACEHOLDER until calibration complete"; ensure the constants related to extrinsics, noise, and latency in VisionConstants are clearly labeled (e.g., the extrinsics constants, noise stddev constants, and latency constants) so callers know they are provisional, and either link to the calibration doc or replace with verified calibrated values once the issue is resolved.
🤖 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 179-185: The code creates four dummy VisionIO instances when
constructing Vision (vision = new Vision(drive::addVisionMeasurement, new
VisionIO(){}, new VisionIO(){}, new VisionIO(){}, new VisionIO(){})), which
leads to permanently disconnected cameras in replay/default mode; change the
construction so that when running in replay/default mode (i.e., when there is no
real vision source) you pass zero VisionIO implementations to the Vision
constructor instead of creating fake ones—locate the Vision construction site
(variable vision and the constructor taking drive::addVisionMeasurement and
VisionIO instances) and conditionally call new
Vision(drive::addVisionMeasurement) or otherwise supply an empty list/array of
VisionIOs depending on the Vision API rather than instantiating new VisionIO()
placeholders.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 250-266: Remove the dead commented SIM branch or restore it behind
a clear mode check: in RobotContainer update the drive.setDefaultCommand usage
around DriveCommands.joystickDrive so there is no stale TODO/comment
noise—either delete the commented lines referencing Constants.currentMode and
Constants.Mode.SIM and keep the current unified axes
(driveController.getLeftY(), getLeftX(), getRightX()), or reintroduce the
conditional using Constants.currentMode == Constants.Mode.SIM to select the
alternate joystick axes (driveController.getLeftY(), getRawAxis(0),
getRawAxis(2)) and remove the TODO; ensure only one clear, active branch remains
around drive.setDefaultCommand.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java`:
- Line 33: The TODO placeholders for camera extrinsics, measurement noise, and
latency in the VisionConstants class must be tracked to closure: create a
concrete issue (calibration checklist covering extrinsics, latency, and noise
characterization) with an owner and target date, then update the TODO comment in
VisionConstants to reference that issue ID/owner/date and mark the current
values as "PLACEHOLDER until calibration complete"; ensure the constants related
to extrinsics, noise, and latency in VisionConstants are clearly labeled (e.g.,
the extrinsics constants, noise stddev constants, and latency constants) so
callers know they are provisional, and either link to the calibration doc or
replace with verified calibrated values once the issue is resolved.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/subsystems/vision/Vision.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/Constants.java (1)
253-259: Inconsistent naming convention for constants.The constants
faceLength,fieldWidth,blueCenter, andredCenteruse camelCase, while other constants in the same class use UPPER_SNAKE_CASE (e.g.,FIELD_BORDER_MARGIN,FIELD_WIDTH). Java constants should consistently use UPPER_SNAKE_CASE.Additionally,
fieldWidth(line 254) is a duplicate ofFIELD_WIDTH(line 250)—both derive fromfield.getFieldWidth().♻️ Proposed fix for naming consistency and duplication
- public static final double faceLength = Units.inchesToMeters(36.792600); - public static final double fieldWidth = field.getFieldWidth(); - public static final Translation2d blueCenter = + public static final double FACE_LENGTH = Units.inchesToMeters(36.792600); + public static final Translation2d BLUE_CENTER = new Translation2d(Units.inchesToMeters(176.745), Units.inchesToMeters(158.5)); - public static final Translation2d redCenter = + public static final Translation2d RED_CENTER = new Translation2d(Units.inchesToMeters(514.13), Units.inchesToMeters(158.5));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Constants.java` around lines 253 - 259, The constants faceLength, fieldWidth, blueCenter, and redCenter violate the class's UPPER_SNAKE_CASE convention and fieldWidth duplicates FIELD_WIDTH; rename them to FACE_LENGTH, remove the duplicate fieldWidth (use existing FIELD_WIDTH wherever needed), and rename blueCenter/redCenter to BLUE_CENTER and RED_CENTER; update their initializers to exactly match the current expressions (e.g., FACE_LENGTH = Units.inchesToMeters(36.792600) and BLUE_CENTER/RED_CENTER = new Translation2d(Units.inchesToMeters(...), Units.inchesToMeters(...))) and replace any references to the old camelCase symbols with the new UPPER_SNAKE_CASE names.
🤖 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/Robot.java`:
- Around line 115-116: The CameraServer.startAutomaticCapture() call in
Robot.java is commented out which disables the driver camera stream; either
re-enable that call if you have a separate USB driver camera so
Elastic/dashboard receives the feed, or add a clarifying comment above the
commented line explaining that PhotonVision now handles all camera streaming
(and why the driver camera capture is intentionally disabled). Locate the
CameraServer.startAutomaticCapture() reference in the Robot class and either
uncomment it or replace the commented-out line with a clear explanatory comment
referencing PhotonVision/AprilTag cameras and the intent.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 231-247: Uncomment and re-enable the default teleop drive command
in RobotContainer by restoring the
drive.setDefaultCommand(DriveCommands.joystickDrive(...)) call (using
driveController lambdas that read getLeftY/getLeftX/getRightX) so the robot
responds to joystick input, and also re-enable the slow-mode (Y button) and
angle-locked (A button) bindings you previously commented out; if you
intentionally disabled them for isolated testing, add a clear comment in
RobotContainer above the block referencing Constants.currentMode (or the SIM
conditional) explaining that drive input is intentionally disabled and when to
re-enable it before competition.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/Constants.java`:
- Around line 253-259: The constants faceLength, fieldWidth, blueCenter, and
redCenter violate the class's UPPER_SNAKE_CASE convention and fieldWidth
duplicates FIELD_WIDTH; rename them to FACE_LENGTH, remove the duplicate
fieldWidth (use existing FIELD_WIDTH wherever needed), and rename
blueCenter/redCenter to BLUE_CENTER and RED_CENTER; update their initializers to
exactly match the current expressions (e.g., FACE_LENGTH =
Units.inchesToMeters(36.792600) and BLUE_CENTER/RED_CENTER = new
Translation2d(Units.inchesToMeters(...), Units.inchesToMeters(...))) and replace
any references to the old camelCase symbols with the new UPPER_SNAKE_CASE names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa641007-ad75-4db3-8bb3-f65f437bc832
⛔ Files ignored due to path filters (1)
src/main/java/org/team5924/frc2026/generated/TunerConstants.javais excluded by!**/generated/**
📒 Files selected for processing (5)
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/subsystems/vision/Vision.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/subsystems/vision/Vision.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java (3)
50-51: Remove or implement commented-out code.The
IGNORE_IDSdeclaration is commented out with an empty list. Either implement this feature if needed, or remove the dead code to keep the file clean.🤖 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/VisionConstants.java` around lines 50 - 51, Remove the dead commented-out IGNORE_IDS declaration or implement it: either delete the two commented lines for IGNORE_IDS to clean up VisionConstants, or uncomment and properly initialize public static final ArrayList<Integer> IGNORE_IDS = new ArrayList<>(List.of()); and add any missing imports (java.util.ArrayList, java.util.List) and a brief comment explaining its purpose so the constant is meaningful; reference the IGNORE_IDS symbol and update any code that expects it accordingly.
72-78: Non-final static fields are mutable.These threshold/baseline fields (
maxAmbiguity,maxZError,linearStdDevBaseline,angularStdDevBaseline) are declared as non-final, making them mutable at runtime. If this is intentional for runtime tuning (e.g., via dashboard), this is fine. If they should be constants, consider addingfinal.🤖 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/VisionConstants.java` around lines 72 - 78, The four threshold/baseline fields in VisionConstants (maxAmbiguity, maxZError, linearStdDevBaseline, angularStdDevBaseline) are declared mutable; if they are intended to be constants, make them immutable by adding the final modifier (declare as public static final double ...) in VisionConstants.java; if instead they must remain runtime-tunable (e.g., updated via dashboard), leave them mutable but add a comment documenting that intent and consider providing controlled accessors or a dedicated tuning mechanism rather than public mutable fields.
54-69: TODO: Simulation camera properties need realistic values.
SIM_ARDUCAM_PROPERTIESuses zero calibration error and zero latency (lines 58-60), which makes simulation unrealistically optimistic. This won't surface timing or accuracy issues that would occur on real hardware.Would you like me to open an issue to track updating these simulation parameters with realistic values based on camera testing data?
🤖 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/VisionConstants.java` around lines 54 - 69, SIM_ARDUCAM_PROPERTIES and SIM_THRIFTYCAM_PROPERTIES use unrealistic simulation parameters (zero calibration error and zero latency for ARDUCAM) which will hide timing/accuracy bugs; update the SimCameraProperties for SIM_ARDUCAM_PROPERTIES (and adjust SIM_THRIFTYCAM_PROPERTIES if needed) by replacing setCalibError(0, 0), setAvgLatencyMs(0) and setLatencyStdDevMs(0) with realistic measured values from camera characterization (use measured pixel reprojection error for setCalibError and measured mean/std latency in ms for setAvgLatencyMs/setLatencyStdDevMs), keep FPS and calibration resolution but add a short comment referencing the measurement source; if measurements are unavailable, populate conservative placeholder values (e.g., non-zero error and latency) and remove the TODO or change it to a tracked issue so future testing can refine them.
🤖 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/VisionConstants.java`:
- Around line 28-48: The file defines BACK_RIGHT_NAME and BACK_LEFT_NAME but no
corresponding transforms; either delete the unused BACK_RIGHT_NAME and
BACK_LEFT_NAME constants to avoid dead code, or add BACK_RIGHT_TRANSFORM and
BACK_LEFT_TRANSFORM definitions analogous to FRONT_RIGHT_TRANSFORM and
FRONT_LEFT_TRANSFORM (create new Transform3d instances using Translation3d with
Units.inchesToMeters and Rotation3d with Units.degreesToRadians), ensuring the
symbol names BACK_RIGHT_TRANSFORM and BACK_LEFT_TRANSFORM match usage
expectations; update any references to these constants accordingly.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java`:
- Around line 50-51: Remove the dead commented-out IGNORE_IDS declaration or
implement it: either delete the two commented lines for IGNORE_IDS to clean up
VisionConstants, or uncomment and properly initialize public static final
ArrayList<Integer> IGNORE_IDS = new ArrayList<>(List.of()); and add any missing
imports (java.util.ArrayList, java.util.List) and a brief comment explaining its
purpose so the constant is meaningful; reference the IGNORE_IDS symbol and
update any code that expects it accordingly.
- Around line 72-78: The four threshold/baseline fields in VisionConstants
(maxAmbiguity, maxZError, linearStdDevBaseline, angularStdDevBaseline) are
declared mutable; if they are intended to be constants, make them immutable by
adding the final modifier (declare as public static final double ...) in
VisionConstants.java; if instead they must remain runtime-tunable (e.g., updated
via dashboard), leave them mutable but add a comment documenting that intent and
consider providing controlled accessors or a dedicated tuning mechanism rather
than public mutable fields.
- Around line 54-69: SIM_ARDUCAM_PROPERTIES and SIM_THRIFTYCAM_PROPERTIES use
unrealistic simulation parameters (zero calibration error and zero latency for
ARDUCAM) which will hide timing/accuracy bugs; update the SimCameraProperties
for SIM_ARDUCAM_PROPERTIES (and adjust SIM_THRIFTYCAM_PROPERTIES if needed) by
replacing setCalibError(0, 0), setAvgLatencyMs(0) and setLatencyStdDevMs(0) with
realistic measured values from camera characterization (use measured pixel
reprojection error for setCalibError and measured mean/std latency in ms for
setAvgLatencyMs/setLatencyStdDevMs), keep FPS and calibration resolution but add
a short comment referencing the measurement source; if measurements are
unavailable, populate conservative placeholder values (e.g., non-zero error and
latency) and remove the TODO or change it to a tracked issue so future testing
can refine them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04150909-5e40-4c35-8d73-cd5d13fc3387
📒 Files selected for processing (2)
src/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/subsystems/vision/VisionConstants.java
…2026 into vision-rework
thatnerdjack
approved these changes
Mar 18, 2026
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
2 participants
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.
implements AprilTag odometry and vision simulation (+maplesim) using PhotonVision