Hopper elevator#86
Closed
maiKuneho wants to merge 29 commits into
Closed
Conversation
…ts, refine constants, relocate subsystem current/goal states into respective subsystem files, add booleans in RobotContainer.java to switch between real/io implementations
Launch calc
Subsystem deprecation
Contributor
Summary by CodeRabbit
WalkthroughThe PR consolidates multi-instance shooter hardware (left/right flywheel, hood, turret) into unified single instances, removes the turret subsystem entirely, introduces a new HopperElevator subsystem to replace turret elevator logic, refactors RobotState to track odometry-based poses instead of subsystem states, adds launching functionality with drive-angle closed-loop control in DriveCommands, updates vision camera calibration parameters, and modifies Constants with revised device IDs and mechanism configurations. Changes
Sequence DiagramsequenceDiagram
participant Driver
participant DriveCommands as DriveCommands<br/>(Launching)
participant LaunchCalculator
participant RobotState
participant Drive
participant Flywheel
participant ShooterHood
Driver->>DriveCommands: Trigger joystickDriveWhileLaunching()
DriveCommands->>LaunchCalculator: getParameters()
LaunchCalculator->>RobotState: getOdometryPose()
RobotState-->>LaunchCalculator: robotPose
LaunchCalculator->>LaunchCalculator: Compute hood angle,<br/>flywheel speed,<br/>turret angle
LaunchCalculator-->>DriveCommands: LaunchingParameters
DriveCommands->>DriveCommands: Compute angular velocity<br/>using kP/kD gains<br/>and yaw error
DriveCommands->>DriveCommands: Compute polar velocity<br/>constraints using<br/>lookahead
DriveCommands->>Drive: runVelocity(vx, vy, omega)
DriveCommands->>Flywheel: setAutoInput(speed)
DriveCommands->>ShooterHood: setAutoInput(angle)
Flywheel->>Flywheel: Execute AUTO state<br/>using speed target
ShooterHood->>ShooterHood: Execute AUTO state<br/>using angle target
DriveCommands->>DriveCommands: atLaunchGoal()?<br/>Check yaw tolerance
DriveCommands-->>Driver: Ready to launch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/team5924/frc2026/util/LaunchCalculator.java (2)
335-357:⚠️ Potential issue | 🟠 MajorThe new zeroed chassis speed disables flight-time compensation.
robotVelocityis now alwaysnew ChassisSpeeds(), so the iterative lookahead never applies anyoffsetX/offsetY. Shots taken while the robot is moving will be aimed as if the launcher were stationary.🤖 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 335 - 357, The robotVelocity was incorrectly hardcoded to new ChassisSpeeds(), which zeroes launcherVelocity and prevents flight-time compensation; restore the real field-relative robot velocity (use RobotState.getInstance().getFieldSetpointVelocity() or the prior method that returns current chassis speeds), then compute launcherVelocity exactly as before with DriverStation.isAutonomous() ? robotVelocity : GeomUtil.transformVelocity(robotVelocity, robotToLauncher.getTranslation().toTranslation2d(), robotAngle) so the iterative lookahead (using launcherVelocity.vxMetersPerSecond / vyMetersPerSecond) produces nonzero offsetX/offsetY when the robot is moving; also guard against null/optional returns from RobotState.getInstance() if needed.
152-157:⚠️ Potential issue | 🔴 CriticalPassing mode still dereferences empty lookup tables.
All
passing*Mappopulation is commented out above, butpassingcan still becometrueonce the robot crosseshubCenter. At that point thepassingTimeOfFlightMap,passingHoodAngleMap, andpassingFlywheelSpeedMaplookups returnnulland this method will blow up.Also applies to: 224-235, 308-347, 371-395
🤖 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 152 - 157, The code can set passing=true when crossing hubCenter but all passing*Map populations are commented out, causing NPEs when calling passingTimeOfFlightMap/passingHoodAngleMap/passingFlywheelSpeedMap; fix by guarding all lookups and assignments so they never dereference a null map entry: in methods that use passing and the maps (e.g., the static init logic and the calculation methods referenced by passing, hubCenter, passingTimeOfFlightMap, passingHoodAngleMap, passingFlywheelSpeedMap, timeOfFlightMap, hoodAngleMap, flywheelSpeedMap) add null-safe checks (map != null && map.containsKey(key) or value != null) and if the passing maps are empty or missing fall back to the non-passing maps or sensible defaults, or prevent toggling passing to true unless the passing maps are populated; apply the same null-safe pattern to all affected blocks (around the hubCenter transition and the lookup points noted).src/main/java/org/team5924/frc2026/RobotContainer.java (1)
341-350:⚠️ Potential issue | 🔴 CriticalUse
onFalse(...)for the intake shutoff binding.Both
rightBumper()bindings currently use.onTrue(), causing both commands to fire when the button is pressed. The shutdown command (lines 341–350) then immediately cancels the intake startup command, leaving the intake stowed/off instead of running. Change the second binding to.onFalse()so intake activates on press and deactivates on release—matching the working pattern already used forleftBumper()(lines 373–382).Suggested fix
driveController .rightBumper() - .onTrue( + .onFalse( Commands.runOnce( () -> { intakePivot.setGoalState(IntakePivotState.STOW); intake.setGoalState(IntakeState.OFF); }, intakePivot, intake));🤖 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 341 - 350, The second rightBumper binding currently uses .onTrue(), causing both startup and shutdown commands to run on press; change that call to .onFalse() so the lambda that sets intakePivot.setGoalState(IntakePivotState.STOW) and intake.setGoalState(IntakeState.OFF) runs on button release instead of press—follow the same pattern as the leftBumper() binding; update the driveController.rightBumper().onTrue(...) invocation to driveController.rightBumper().onFalse(...) for the intake shutdown command that references intakePivot and intake.
🧹 Nitpick comments (7)
.vscode/settings.json (1)
65-65: Consider avoiding hard-pinning-Xmx4Gin repo-shared workspace settings.This can be heavy on lower-memory dev machines. If this is only needed for some contributors, consider documenting it in
README(or moving to user settings) and keeping a lower default here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/settings.json at line 65, The workspace setting "java.jdt.ls.vmargs" currently hard-pins "-Xmx4G", which can be too large for low-memory machines; remove or lower that flag in .vscode/settings.json (e.g., drop the -Xmx4G or replace it with a safer default like -Xmx1G), and instead document the recommended JVM args in the README or instruct contributors to add the higher -Xmx value in their personal VS Code user settings if they need it; update the README note to include the exact flag string and rationale so maintainers can opt into the larger heap locally rather than forcing it in the repo-shared "java.jdt.ls.vmargs".src/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.java (1)
42-44: Consider adding Javadoc for theTranslationRotationrecord.The rotation units (degrees vs radians) are not documented. Based on usage in
VisionIOPhotonVision, these are degrees, but a brief Javadoc comment would clarify this for future maintainers.📝 Suggested documentation
+ /** Represents translation and rotation (roll, pitch, yaw in degrees). */ public static record TranslationRotation( Translation3d translation, double roll, double pitch, double yaw) {}🤖 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/VisionIO.java` around lines 42 - 44, Add Javadoc to the TranslationRotation record to document its purpose and explicitly state that roll, pitch, and yaw are in degrees (as used in VisionIOPhotonVision); update the comment above the record TranslationRotation to describe the translation field (Translation3d) and each rotation component (roll, pitch, yaw) with units and any conventions (e.g., axis order or right-hand rule) so future maintainers know how VisionIOPhotonVision expects and produces these values.src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java (1)
54-54: Remove commented-out code.The line
// this.robotToCamera = robotToCamera;is dead code and should be removed to keep the codebase clean.🧹 Proposed fix
public VisionIOPhotonVision(String name, Transform3d robotToCamera) { camera = new PhotonCamera(name); - // this.robotToCamera = robotToCamera; rollDegrees =🤖 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` at line 54, Remove the dead commented-out assignment in VisionIOPhotonVision by deleting the line "// this.robotToCamera = robotToCamera;" in the class (remove any leftover commented assignment referencing robotToCamera) so the codebase has no commented-out, unused lines; ensure no other references rely on that commented line before removing.src/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOSim.java (1)
28-30: Remove or replace commented-out stub with a TODO.Commented-out code adds clutter. If follower simulation support is planned, use a TODO comment to track the work instead.
🧹 Option 1: Remove commented code
public IntakeIOSim() { super( DCMotor.getKrakenX60Foc(1), Constants.Intake.SIM_MOI, Constants.Intake.MOTOR_TO_MECHANISM); } - - // `@Override` - // public void updateFollowerInputs() {} }📝 Option 2: Replace with TODO
public IntakeIOSim() { super( DCMotor.getKrakenX60Foc(1), Constants.Intake.SIM_MOI, Constants.Intake.MOTOR_TO_MECHANISM); } - - // `@Override` - // public void updateFollowerInputs() {} + + // TODO: Implement updateFollowerInputs() for follower motor simulation }🤖 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/rollers/intake/IntakeIOSim.java` around lines 28 - 30, The commented-out stub for updateFollowerInputs in IntakeIOSim should be removed or replaced with a clear TODO; locate the IntakeIOSim class and either delete the commented lines "// `@Override` // public void updateFollowerInputs() {}" to reduce clutter, or replace them with a single-line TODO comment referencing the method name (e.g., "TODO: implement updateFollowerInputs simulation/follower support") so the intent is tracked without leaving commented code; ensure formatting and spacing follow existing file style.src/main/java/org/team5924/frc2026/subsystems/rollers/intake/Intake.java (1)
45-48: Remove emptyperiodic()override.This override only delegates to
super.periodic()without adding any logic. It can be safely removed as the parent class method will be called automatically.🧹 Proposed cleanup
- `@Override` - public void periodic() { - super.periodic(); - } - public void setGoalState(IntakeState goalState) {🤖 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/rollers/intake/Intake.java` around lines 45 - 48, The Intake class contains an empty periodic() override that only calls super.periodic(); remove the override method from Intake (delete the periodic() method in class Intake) so the parent class's periodic() runs without a redundant override.src/main/java/org/team5924/frc2026/subsystems/flywheel/FlywheelIOSim.java (1)
52-55: Hardcoded loop bound assumes 4-motor array size.The loop iterates exactly 4 times to populate
motorConnectedandtempCelsiusarrays. If the array size inFlywheelIOInputsis changed, this will cause anArrayIndexOutOfBoundsException.Consider using the array length directly:
♻️ Proposed fix
- for (int i = 0; i < 4; ++i) { + for (int i = 0; i < inputs.motorConnected.length; ++i) { inputs.motorConnected[i] = true; inputs.tempCelsius[i] = 25.0; }🤖 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/FlywheelIOSim.java` around lines 52 - 55, The loop in FlywheelIOSim hardcodes 4 iterations causing potential ArrayIndexOutOfBoundsException if FlywheelIOInputs arrays change; update the loop to iterate using the arrays' actual length (e.g., use inputs.motorConnected.length or inputs.tempCelsius.length) so both inputs.motorConnected and inputs.tempCelsius are populated safely for whatever size those arrays currently have.src/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOTalonFX.java (1)
23-28: Follower field is stored but never used after initialization.The
followerinstance is created (which configures the TalonFX in follower mode via its constructor), but the field is never accessed afterward. This works because CTRE follower mode operates independently aftersetControl(new Follower(...))is called.However, the follower motor's telemetry (temperature, current, connection status) is never captured since
follower.updateInputs()is never called. Consider whether follower diagnostics should be logged for monitoring motor health.🤖 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/rollers/intake/IntakeIOTalonFX.java` around lines 23 - 28, The follower instance (follower of type IntakeFollowerIOTalonFX) is constructed but never used, so its telemetry (temperature/current/connection) is never collected; either remove the field if you intentionally don’t monitor it or call its update method from the primary subsystem telemetry path—specifically, add a call to follower.updateInputs() inside IntakeIOTalonFX.updateInputs() (or whatever input-polling method is used) and forward the follower’s telemetry into your existing input/telemetry structure so the follower motor health is logged; if opting to remove, delete the follower field and constructor instantiation instead.
🤖 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 261-274: The early return when driveLauncherCORMaxErrorDeg.get() <
driveLauncherCORMinErrorDeg.get() causes no new chassis command or setpoint
velocity to be published; instead remove the return and treat the
tunables-as-invalid case by setting a safe corScalar (e.g., 1.0) and continue to
compute/apply the chassis speeds and update RobotState.robotSetpointVelocity so
the drivetrain output and RobotState remain in sync; modify the block around
driveLauncherCORMaxErrorDeg/driveLauncherCORMinErrorDeg and the corScalar
calculation in DriveCommands (the code using parameters.driveAngle(),
RobotState.getInstance().getRotation(), and corScalar) to ensure a fallback path
publishes the chassis command and updates RobotState rather than returning
early.
In `@src/main/java/org/team5924/frc2026/Robot.java`:
- Around line 129-133: The low battery alert reset currently in robotInit()
checks DriverStation.isEnabled(), which is false at startup, so move the
disabledTimer.reset() and lowBatteryAlert.set(false) logic out of robotInit()
into the enabled-mode init methods (e.g., teleopInit() and autonomousInit()) so
the timer and alert are correctly reset when the robot transitions to enabled;
update references to disabledTimer and lowBatteryAlert in those methods and
remove the ineffective check from robotInit().
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 73-87: Add a HopperElevator field and a realHopperElevator boolean
to RobotContainer and instantiate it in all three init branches (REAL, SIM,
default) following the same pattern used for other subsystems (e.g., hopper,
intake): declare private final HopperElevator hopperElevator; add private final
boolean realHopperElevator; set realHopperElevator based on the environment like
the other realX flags, and in each branch (REAL, SIM, default) assign
hopperElevator to the platform-specific implementation (or a simulation stub)
consistent with how hopper and intake are constructed so its periodic() will be
scheduled.
- Around line 384-392: Replace the two separate commands with a single
continuous default command on shooterHood: remove the Commands.runOnce(...)
default and the driveController.rightStick().onTrue(...) binding, and instead
call shooterHood.setDefaultCommand(Commands.run( () -> {
shooterHood.setGoalState(ShooterHood.ShooterHoodState.MANUAL);
shooterHood.setInput(() -> driveController.getRightY()); }, shooterHood )); so
the MANUAL goal is set and the input supplier is refreshed every cycle (this
mirrors the patterns used in ManualShooterCommands and IntakePivotCommands and
prevents handleManualState() from reading a stale input).
In `@src/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.java`:
- Around line 145-160: The flag being published is too generic — change the
logic in handleCurrentState/isAtSetpoint so
RobotState.setFlywheelAtSetpoint(...) only becomes true when the flywheel is
both in a feedable mode (whitelist the feedable enums, e.g., FlywheelState.AUTO
and FlywheelState.MANUAL_SETPOINT) and the measured velocity matches the
intended target; to do that, compute the target by calling
getTargetVelocityRotationsPerSec() (ensure autoInput is refreshed before
comparison), then call EqualsUtil.epsilonEquals(...) and only set
RobotState.setFlywheelAtSetpoint(isReady) when goalState is one of the allowed
feedable states (do not set it for OFF or open-loop/idle states); apply the same
whitelist-based check to the other identical location around the later block
(lines ~169-171) that currently publishes the generic setpoint flag.
In
`@src/main/java/org/team5924/frc2026/subsystems/hopperElevator/HopperElevator.java`:
- Around line 123-134: The code allows HopperElevatorState.MOVING to be accepted
and even sets this.goalState and currentState before rejecting it; change
setGoalState to validate and reject MOVING before mutating any fields (i.e., if
goalState == HopperElevatorState.MOVING, log/report error and return
immediately), avoid fall-through in the switch, and only set this.goalState and
currentState when a legitimate state change is accepted; also ensure
lastStateChange is updated whenever you actually change currentState/goalState
so timeout tracking measures real transitions (refer to setGoalState,
this.goalState, currentState, lastStateChange, and HopperElevatorState.MOVING).
- Around line 89-95: The isAtSetpoint() method is comparing
inputs.setpointMeters to inputs.positionRads using
Constants.HopperElevator.EPSILON_RADS, which mixes linear and angular units;
change the comparison so both sides use the same units (either compare a
positionMeters field to setpointMeters using an EPSILON_METERS tolerance, or
convert setpointMeters to radians and compare to inputs.positionRads using
EPSILON_RADS). Update the referenced symbols (inputs.setpointMeters,
inputs.positionRads, Constants.HopperElevator.EPSILON_RADS and/or add
Constants.HopperElevator.EPSILON_METERS) and keep the existing timeout check
(ENABLE_TIMEOUT / timeSinceLastStateChange / STATE_TIMEOUT) unchanged.
- Around line 97-121: The code never drives the elevator from joystick input
because MANUAL ends up treated like MOVING (using goalState.getHeightMeters())
and handleManualState() only stops on deadzone without commanding motion; update
handleCurrentState / the MOVING branch to detect when goalState ==
HopperElevatorState.MANUAL and delegate to manual handling (call
handleManualState or inline manual logic) instead of calling
io.setHeight(goalState.getHeightMeters()), and change handleManualState() to
actively command the actuator when outside the deadzone (use the live input
value scaled to the appropriate output method on io — e.g., percent/voltage
setter or a max-volts tunable — rather than using the ManualVolts tunable), and
apply the same fix in the other similar block mentioned (lines 123-134) so
manual input actually drives the mechanism.
- Around line 53-56: STOW and EXTENDED enum presets are both initialized to 0.0
via LoggedTunableNumber, so set realistic non-zero defaults: update the enum
entry for STOW(new LoggedTunableNumber("HopperElevator/StowHeightMeters", X))
and EXTENDED(new LoggedTunableNumber("HopperElevator/ExtendedHeightMeters", Y))
to meaningful heights (replace X and Y with measured default stow and extended
heights in meters), leaving the NetworkTables keys unchanged; no other logic
changes needed (leave MANUAL and MOVING as-is).
In
`@src/main/java/org/team5924/frc2026/subsystems/hopperElevator/HopperElevatorIOTalonFX.java`:
- Around line 64-69: The dashboard tunable keys and alert labels in
HopperElevatorIOTalonFX still use IntakePivot/ShooterHood names; rename the
LoggedTunableNumber keys (e.g., motionCruiseVelocity, motionAcceleration,
motionJerk) to use a unique prefix like "HopperElevator/..." and update any
alert creation/update calls that reference "ShooterHood" or "IntakePivot" (the
init/update alerts further down in this class) so they use "HopperElevator" (or
another hopper-specific name) to avoid colliding with other subsystems; ensure
you update all similar tunables and alert strings in this file (the blocks
around the other tunable definitions and the init/update alert code) to the new
prefix.
- Around line 98-106: The gravity type for the elevator is set incorrectly to
Arm_Cosine which makes kG vary with angle; change the GravityType assignment on
the Slot0Configs instance (slot0Configs.GravityType) from
GravityTypeValue.Arm_Cosine to GravityTypeValue.Elevator_Static so the kG
feedforward remains constant for the linear elevator (update the assignment
where slot0Configs is configured in HopperElevatorIOTalonFX).
- Around line 168-172: The current startup code in HopperElevatorIOTalonFX calls
BaseStatusSignal.waitForAll(0.5, cancoderAbsolutePosition) but then
unconditionally calls cancoder.setPosition(0.0) and talon.setPosition(0.0),
discarding the absolute reading; instead, use the cancoderAbsolutePosition value
to initialize the Talon sensor or only zero when you have a physical reference:
read the absolute encoder value from cancoderAbsolutePosition after waitForAll
and convert it into the Talon sensor units (use the existing conversion logic
used elsewhere) and call talon.setPosition(convertedValue); only call
cancoder.setPosition(0.0) / talon.setPosition(0.0) unconditionally if a verified
homing condition exists (e.g., an atStowed limit switch or the value lies within
an expected deadband), otherwise preserve the absolute reading so subsequent
moves aren’t offset.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.java`:
- Line 27: Remove the stray debug field or give it a meaningful name: locate the
public boolean field declared as "public boolean a = true;" in the VisionIO
class and either delete it if unused, or rename it to a descriptive identifier
(e.g., isEnabled, debugMode, or useVisionProcessing) and update all references
within VisionIO accordingly so intent is clear and no single-letter field
remains.
In `@src/main/java/org/team5924/frc2026/util/LauncherConstants.java`:
- Around line 26-30: The field robotToLauncher in class LauncherConstants is
intended as a constant but is missing the final modifier; update the declaration
of robotToLauncher (public static Transform3d robotToLauncher) to be public
static final Transform3d robotToLauncher so the Transform3d instance cannot be
reassigned at runtime, leaving the existing initializer (new Transform3d(...))
unchanged.
---
Outside diff comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 341-350: The second rightBumper binding currently uses .onTrue(),
causing both startup and shutdown commands to run on press; change that call to
.onFalse() so the lambda that sets
intakePivot.setGoalState(IntakePivotState.STOW) and
intake.setGoalState(IntakeState.OFF) runs on button release instead of
press—follow the same pattern as the leftBumper() binding; update the
driveController.rightBumper().onTrue(...) invocation to
driveController.rightBumper().onFalse(...) for the intake shutdown command that
references intakePivot and intake.
In `@src/main/java/org/team5924/frc2026/util/LaunchCalculator.java`:
- Around line 335-357: The robotVelocity was incorrectly hardcoded to new
ChassisSpeeds(), which zeroes launcherVelocity and prevents flight-time
compensation; restore the real field-relative robot velocity (use
RobotState.getInstance().getFieldSetpointVelocity() or the prior method that
returns current chassis speeds), then compute launcherVelocity exactly as before
with DriverStation.isAutonomous() ? robotVelocity :
GeomUtil.transformVelocity(robotVelocity,
robotToLauncher.getTranslation().toTranslation2d(), robotAngle) so the iterative
lookahead (using launcherVelocity.vxMetersPerSecond / vyMetersPerSecond)
produces nonzero offsetX/offsetY when the robot is moving; also guard against
null/optional returns from RobotState.getInstance() if needed.
- Around line 152-157: The code can set passing=true when crossing hubCenter but
all passing*Map populations are commented out, causing NPEs when calling
passingTimeOfFlightMap/passingHoodAngleMap/passingFlywheelSpeedMap; fix by
guarding all lookups and assignments so they never dereference a null map entry:
in methods that use passing and the maps (e.g., the static init logic and the
calculation methods referenced by passing, hubCenter, passingTimeOfFlightMap,
passingHoodAngleMap, passingFlywheelSpeedMap, timeOfFlightMap, hoodAngleMap,
flywheelSpeedMap) add null-safe checks (map != null && map.containsKey(key) or
value != null) and if the passing maps are empty or missing fall back to the
non-passing maps or sensible defaults, or prevent toggling passing to true
unless the passing maps are populated; apply the same null-safe pattern to all
affected blocks (around the hubCenter transition and the lookup points noted).
---
Nitpick comments:
In @.vscode/settings.json:
- Line 65: The workspace setting "java.jdt.ls.vmargs" currently hard-pins
"-Xmx4G", which can be too large for low-memory machines; remove or lower that
flag in .vscode/settings.json (e.g., drop the -Xmx4G or replace it with a safer
default like -Xmx1G), and instead document the recommended JVM args in the
README or instruct contributors to add the higher -Xmx value in their personal
VS Code user settings if they need it; update the README note to include the
exact flag string and rationale so maintainers can opt into the larger heap
locally rather than forcing it in the repo-shared "java.jdt.ls.vmargs".
In `@src/main/java/org/team5924/frc2026/subsystems/flywheel/FlywheelIOSim.java`:
- Around line 52-55: The loop in FlywheelIOSim hardcodes 4 iterations causing
potential ArrayIndexOutOfBoundsException if FlywheelIOInputs arrays change;
update the loop to iterate using the arrays' actual length (e.g., use
inputs.motorConnected.length or inputs.tempCelsius.length) so both
inputs.motorConnected and inputs.tempCelsius are populated safely for whatever
size those arrays currently have.
In `@src/main/java/org/team5924/frc2026/subsystems/rollers/intake/Intake.java`:
- Around line 45-48: The Intake class contains an empty periodic() override that
only calls super.periodic(); remove the override method from Intake (delete the
periodic() method in class Intake) so the parent class's periodic() runs without
a redundant override.
In
`@src/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOSim.java`:
- Around line 28-30: The commented-out stub for updateFollowerInputs in
IntakeIOSim should be removed or replaced with a clear TODO; locate the
IntakeIOSim class and either delete the commented lines "// `@Override` // public
void updateFollowerInputs() {}" to reduce clutter, or replace them with a
single-line TODO comment referencing the method name (e.g., "TODO: implement
updateFollowerInputs simulation/follower support") so the intent is tracked
without leaving commented code; ensure formatting and spacing follow existing
file style.
In
`@src/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOTalonFX.java`:
- Around line 23-28: The follower instance (follower of type
IntakeFollowerIOTalonFX) is constructed but never used, so its telemetry
(temperature/current/connection) is never collected; either remove the field if
you intentionally don’t monitor it or call its update method from the primary
subsystem telemetry path—specifically, add a call to follower.updateInputs()
inside IntakeIOTalonFX.updateInputs() (or whatever input-polling method is used)
and forward the follower’s telemetry into your existing input/telemetry
structure so the follower motor health is logged; if opting to remove, delete
the follower field and constructor instantiation instead.
In `@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIO.java`:
- Around line 42-44: Add Javadoc to the TranslationRotation record to document
its purpose and explicitly state that roll, pitch, and yaw are in degrees (as
used in VisionIOPhotonVision); update the comment above the record
TranslationRotation to describe the translation field (Translation3d) and each
rotation component (roll, pitch, yaw) with units and any conventions (e.g., axis
order or right-hand rule) so future maintainers know how VisionIOPhotonVision
expects and produces these values.
In
`@src/main/java/org/team5924/frc2026/subsystems/vision/VisionIOPhotonVision.java`:
- Line 54: Remove the dead commented-out assignment in VisionIOPhotonVision by
deleting the line "// this.robotToCamera = robotToCamera;" in the class (remove
any leftover commented assignment referencing robotToCamera) so the codebase has
no commented-out, unused lines; ensure no other references rely on that
commented line before removing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81108b4f-ea27-420c-a5c7-cebf98b2ab92
📒 Files selected for processing (40)
.vscode/settings.jsonsrc/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/commands/shooter/ManualShooterCommands.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/Flywheel.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/FlywheelIO.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/FlywheelIOSim.javasrc/main/java/org/team5924/frc2026/subsystems/flywheel/FlywheelIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/hopperElevator/HopperElevator.javasrc/main/java/org/team5924/frc2026/subsystems/hopperElevator/HopperElevatorIO.javasrc/main/java/org/team5924/frc2026/subsystems/hopperElevator/HopperElevatorIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/intakePivot/IntakePivot.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/pivots/shooterHood/ShooterHoodIOSim.javasrc/main/java/org/team5924/frc2026/subsystems/pivots/shooterHood/ShooterHoodIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/generic/GenericRollerIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/hopper/Hopper.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/hopper/HopperIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/indexer/Indexer.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/indexer/IndexerIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/intake/Intake.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeFollowerIO.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeFollowerIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOSim.javasrc/main/java/org/team5924/frc2026/subsystems/rollers/intake/IntakeIOTalonFX.javasrc/main/java/org/team5924/frc2026/subsystems/turret/Turret.javasrc/main/java/org/team5924/frc2026/subsystems/turret/TurretIOSim.javasrc/main/java/org/team5924/frc2026/subsystems/turret/TurretIOTalonFX.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
💤 Files with no reviewable changes (5)
- src/main/java/org/team5924/frc2026/subsystems/rollers/hopper/HopperIOTalonFX.java
- src/main/java/org/team5924/frc2026/subsystems/rollers/indexer/IndexerIOTalonFX.java
- src/main/java/org/team5924/frc2026/subsystems/turret/TurretIOTalonFX.java
- src/main/java/org/team5924/frc2026/subsystems/turret/Turret.java
- src/main/java/org/team5924/frc2026/subsystems/turret/TurretIOSim.java
Comment on lines
73
to
+87
| public class RobotContainer { | ||
| // Subsystems | ||
| private final Drive drive; | ||
| private SwerveDriveSimulation driveSimulation = null; | ||
| private Vision vision; | ||
|
|
||
| private final Vision vision; | ||
|
|
||
| private final Intake intake; | ||
| private final IntakePivot intakePivot; | ||
| private final Hopper hopper; | ||
| private final Indexer indexer; | ||
|
|
||
| private final ShooterHood shooterHoodRight; | ||
| private final Flywheel flywheelRight; | ||
| private final Turret turretRight; | ||
| private final ShooterHood shooterHood; | ||
| private final Flywheel flywheel; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the RobotContainer file and check its full contents
find . -name "RobotContainer.java" -type fRepository: Team5924/GoldenGateRobotics2026
Length of output: 131
🏁 Script executed:
# Also look for HopperElevator in the codebase
rg -l "HopperElevator" --type javaRepository: Team5924/GoldenGateRobotics2026
Length of output: 378
🏁 Script executed:
# Read the full RobotContainer.java file to check for HopperElevator construction
wc -l ./src/main/java/org/team5924/frc2026/RobotContainer.javaRepository: Team5924/GoldenGateRobotics2026
Length of output: 135
🏁 Script executed:
# Read the full RobotContainer file
cat -n ./src/main/java/org/team5924/frc2026/RobotContainer.javaRepository: Team5924/GoldenGateRobotics2026
Length of output: 19793
🏁 Script executed:
# Search for HopperElevator instantiation (new HopperElevator)
rg "new HopperElevator" --type java -B 2 -A 2Repository: Team5924/GoldenGateRobotics2026
Length of output: 708
🏁 Script executed:
# Check if there are any other Robot container-like classes or factories that might instantiate HopperElevator
rg "HopperElevator" --type java -B 2 -A 2Repository: Team5924/GoldenGateRobotics2026
Length of output: 19426
Add HopperElevator to the composition root.
HopperElevator is not instantiated anywhere in the codebase. Without construction in RobotContainer, the subsystem's periodic() method never executes and the implementation becomes dead code. Add a HopperElevator field, a corresponding realHopperElevator flag, and instantiate it in all three initialization branches (REAL, SIM, and default) following the same pattern as other subsystems.
🤖 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 73 - 87,
Add a HopperElevator field and a realHopperElevator boolean to RobotContainer
and instantiate it in all three init branches (REAL, SIM, default) following the
same pattern used for other subsystems (e.g., hopper, intake): declare private
final HopperElevator hopperElevator; add private final boolean
realHopperElevator; set realHopperElevator based on the environment like the
other realX flags, and in each branch (REAL, SIM, default) assign hopperElevator
to the platform-specific implementation (or a simulation stub) consistent with
how hopper and intake are constructed so its periodic() will be scheduled.
| @AutoLog | ||
| public static class VisionIOInputs { | ||
| public boolean connected = false; | ||
| public boolean a = true; |
Contributor
There was a problem hiding this comment.
Remove or rename the debug field a.
The field public boolean a = true; has an unclear single-letter name and appears to be leftover debug or placeholder code. If this field is needed, give it a meaningful name that describes its purpose. If it's not needed, remove it before merging.
🤖 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/VisionIO.java` at line
27, Remove the stray debug field or give it a meaningful name: locate the public
boolean field declared as "public boolean a = true;" in the VisionIO class and
either delete it if unused, or rename it to a descriptive identifier (e.g.,
isEnabled, debugMode, or useVisionProcessing) and update all references within
VisionIO accordingly so intent is clear and no single-letter field remains.
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.
No description provided.