Alpha autos - new auto routines using autoFactory in autoBuilder#53
Open
michaelwillson415 wants to merge 27 commits into
Open
Alpha autos - new auto routines using autoFactory in autoBuilder#53michaelwillson415 wants to merge 27 commits into
michaelwillson415 wants to merge 27 commits into
Conversation
created choreolib and autofactory implementation into auto builder
Contributor
|
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. 📒 Files selected for processing (3)
WalkthroughWires Choreo trajectory tooling into the robot: adds a global AutoFactory, reworks auto selection to use a starting-position chooser and AutoBuilder flows, extends Drive with PID-based followChoreoTrajectory, adds Choreo vendordeps and multiple .chor/.traj assets, and removes two placeholder auto command classes. Changes
Sequence DiagramsequenceDiagram
participant Driver as Driver/Dashboard
participant RC as RobotContainer
participant AB as AutoBuilder
participant AF as AutoFactory
participant Traj as TrajectoryStore
participant Drive as Drive
Driver->>RC: choose starting position
RC->>AB: setStartingPosition(supplier)
RC->>AB: select auto (scoreAndClimb / scorePickupAndClimb)
AB->>AF: request trajectory(s) for start
AF->>Traj: load .traj file(s)
Traj-->>AF: stream SwerveSample(s)
AF-->>AB: provide samples
loop for each SwerveSample
AB->>Drive: followChoreoTrajectory(sample)
Drive->>Drive: compute PID (x,y,heading)
Drive->>Drive: runVelocity(chassis speeds)
end
Drive-->>RC: update pose / auto completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![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: 7
🤖 Fix all issues with AI agents
In `@src/main/deploy/choreo/NewPath.traj`:
- Around line 22-28: The trajectory file NewPath.traj currently has null config
and empty waypoints/samples which will cause AutoFactory.trajectory("NewPath")
to fail or no-op; open the Choreo GUI (or use the Choreo CLI) and
generate/export a valid trajectory so the "trajectory" object contains a
populated config, waypoints and samples (non-empty "samples" array), then
re-save the updated NewPath.traj before deploying.
In `@src/main/deploy/choreo/test-path.chor`:
- Around line 78-83: The codegen.root field currently contains a hardcoded
machine-specific absolute path
("codegen.root":"..\\C:\\Users\\wills\\Development"); change this to a relative
or configurable value: replace the absolute path with a relative path (e.g. a
project-relative folder) or read from an environment/config variable so other
devs/CI can resolve it, and remove the embedded "C:\\" segment; update the
codegen block ("codegen.root") accordingly and, if necessary, add a
fallback/default in the build/deploy config so codegen.root is not
runtime-dependent on a local developer path.
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 79-84: The sequence reuses the "startingPositionToHub" trajectory
after running "HubtoDepot", which misaligns the robot pose; replace the call to
Robot.mAutoFactory.trajectoryCmd("startingPositionToHub") inside the
Commands.parallel block with a trajectory that begins at the depot (e.g.,
"DepotToHub" or another depot-starting trajectory), or add a new DepotToHub
trajectory in the auto factory and call
Robot.mAutoFactory.trajectoryCmd("DepotToHub") so the trajectory follower starts
from the robot's actual pose after HubtoDepot.
- Line 80: The trajectory name string is mis-cased; update the call to
Robot.mAutoFactory.trajectoryCmd("HubtoDepot") to use the PascalCase filename
expected (e.g., "HubToDepot") so it matches the actual .traj filename and other
names like "HubToClimb"; change the string literal in the AutoBuilder line that
composes Robot.mAutoFactory.trajectoryCmd(...) (the same line that also uses
AutoCommands.intake(intake)) to the correctly-cased "HubToDepot".
- Line 62: AutoCommands.score(shooter) calls use Commands.run() which never
finishes, so wrap each AutoCommands.score(shooter) invocation with a timeout to
let the sequence continue: locate the three usages of
AutoCommands.score(shooter) (currently passed into Commands.sequence(...) in
AutoBuilder) and append .withTimeout(<appropriateSeconds>) to each call (adjust
duration as needed for your scoring routine) so the sequence can advance past
the scoring step.
In `@src/main/java/org/team5924/frc2026/commands/AutoCommands.java`:
- Around line 31-32: Remove the informal "idk????" comment in AutoCommands and
replace it with a clear, actionable comment or TODO that explains the intent:
e.g., note that the command requires turret and rotation data and specify what
remains to be decided or implemented (e.g., "TODO: confirm data source for
turret and rotation (vision vs odometry) and implement fallback behavior");
target the comment near the existing shooter/turret note in the AutoCommands
class so reviewers know what needs follow-up.
In `@src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java`:
- Around line 347-361: The followChoreoTrajectory method is building
field-relative ChassisSpeeds but calls runVelocity which expects robot-relative
speeds; convert the computed field-relative speeds into robot-relative using
ChassisSpeeds.fromFieldRelativeSpeeds with the current robot rotation
(pose.getRotation()) before calling runVelocity. Locate followChoreoTrajectory,
the generated ChassisSpeeds variable, and replace the direct runVelocity(speeds)
call with a conversion to robot-relative speeds via
ChassisSpeeds.fromFieldRelativeSpeeds(...) (using sample.vx, sample.vy,
sample.omega plus the PID outputs) and pass that converted result to
runVelocity.
🧹 Nitpick comments (5)
src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java (1)
109-134: PID gains are captured once at initialization and never updated from tunables.The
xController,yController, andheadingControllerare constructed usingget()values from theLoggedTunableNumberfields, but there is noLoggedTunableNumber.ifChanged(...)call to push updated gains into the PID controllers when they are tuned at runtime. Without this, the tunable numbers are effectively dead config.♻️ Add ifChanged to periodic() or followChoreoTrajectory
Add the following to
periodic()(or at the start offollowChoreoTrajectory):LoggedTunableNumber.ifChanged( hashCode(), values -> { xController.setPID(values[0], values[1], values[2]); yController.setPID(values[3], values[4], values[5]); headingController.setPID(values[6], values[7], values[8]); }, xControllerKp, xControllerKi, xControllerKd, yControllerKp, yControllerKi, yControllerKd, headingControllerKp, headingControllerKi, headingControllerKd);src/main/java/org/team5924/frc2026/Robot.java (1)
41-41: Public static mutable field creates global state and temporal coupling.
Robot.mAutoFactoryisnulluntilRobotContainer's constructor callsconfigureAutoFactory(). Any code referencing it before that point (e.g., static initializers inAutoBuilder) will get aNullPointerException. Consider making this a private field with a static getter, or better yet, pass theAutoFactoryinstance directly to consumers that need it (likeAutoBuilder) via constructor/method parameters.♻️ Minimal improvement: add null-safety
- public static AutoFactory mAutoFactory; + private static AutoFactory mAutoFactory; + + public static AutoFactory getAutoFactory() { + if (mAutoFactory == null) { + throw new IllegalStateException("AutoFactory has not been initialized yet"); + } + return mAutoFactory; + } + + public static void setAutoFactory(AutoFactory factory) { + mAutoFactory = factory; + }src/main/java/org/team5924/frc2026/commands/AutoCommands.java (1)
27-29:Commands.run()continuously sets state every cycle — consider ifCommands.runOnce()suffices, or if the intent is to hold the subsystem requirement.
Commands.run()will callsetGoalState(INTAKE)every 20ms. If the purpose is just to set the state and hold the subsystem (preventing default commands),Commands.startRun(() -> intake.setGoalState(IntakeState.INTAKE), () -> {})or usingCommands.run()is fine. However, note the inconsistency withRobotContainer.javaLine 172 where the equivalentNamedCommandsversion usesCommands.runOnce(). Also, since these commands have no end condition, they will run indefinitely — ensure they are composed with.withTimeout()or.until()in the auto sequence.src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (2)
57-57: Static access toRobot.mAutoFactorycreates tight coupling.Accessing a mutable public static field from
RobotmakesAutoBuilderhard to test and tightly couples it to theRobotlifecycle. Consider injectingAutoFactorythrough the constructor (alongside the other subsystems) instead.Also applies to: 73-73
30-33: Remove unuseddrivefield or add clarification.The
drivesubsystem is injected via@RequiredArgsConstructorbut is not referenced in eitherscoreAndClimbAuto()orscorePickupAndClimbAuto()— trajectory commands are sourced fromRobot.mAutoFactory. If this field is not planned for future use, remove it to eliminate dead code; otherwise, add a comment clarifying its intended purpose.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/deploy/choreo/RightStartToHub.traj`:
- Around line 37-38: Trajectory files (e.g., RightStartToHub.traj and the six
others: ClimbToHub, DepotIntake, DepotToHub, HubToClimb, HubToDepot,
LeftStartToHub) declare mass = 68.0388555 and inertia = 6.0 which conflicts with
Drive.java constants ROBOT_MASS_KG = 52.16 and ROBOT_MOI = 4.39; fix by making
the values consistent: either update Drive.java (change ROBOT_MASS_KG and
ROBOT_MOI to match the trajectory mass/inertia and remove the TODO) or
regenerate all trajectory files using the current Drive.java parameters so the
trajectory JSONs reflect ROBOT_MASS_KG and ROBOT_MOI. Ensure the same corrected
numeric values are applied across all seven trajectory files and in Drive.java.
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 37-40: AutoBuilder appears unused so either delete the class or
finish it: if you keep AutoBuilder, initialize the field startingPosition to a
sensible default (e.g., "Left" or a value from config) and validate it in
scoreAndClimbAuto() and scorePickupAndClimbAuto() to avoid building trajectories
like "nullToHub"; add or ensure a public setStartingPosition(String) is called
from RobotContainer (or the code that selects autos) before any auto routine
runs, and add a guard that throws or logs a clear error if startingPosition is
null/invalid. Ensure references to the methods scoreAndClimbAuto(),
scorePickupAndClimbAuto(), and the startingPosition field are updated
accordingly so the class is either removed or fully functional.
In `@src/main/java/org/team5924/frc2026/subsystems/drive/Drive.java`:
- Around line 109-134: PID gains from LoggedTunableNumber are only read at field
initialization so runtime dashboard changes are ignored; update the
PIDController instances in periodic() by calling
LoggedTunableNumber.ifChanged(...) (use hashCode() as the key) and in its
callback call xController.setPID(...), yController.setPID(...), and
headingController.setPID(...) with the current values from
xControllerKp/xControllerKi/xControllerKd,
yControllerKp/yControllerKi/yControllerKd, and
headingControllerKp/headingControllerKi/headingControllerKd so controllers
receive updated gains.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (2)
42-60: Remove commented-out dead code.The old
basicDriveAuto()implementation is entirely commented out. It adds noise without value — the git history preserves it if needed.
33-33: Remove unuseddrivefield from AutoBuilder or add it to command requirements.The
drivesubsystem is injected via@RequiredArgsConstructorbut neitherscoreAndClimbAuto()norscorePickupAndClimbAuto()uses it. All trajectory commands are sourced fromRobot.mAutoFactory.trajectoryCmd(), which doesn't reference the field. If the auto commands internally require drive and need it scheduled as a requirement, add.addRequirements(drive)to the command chain; otherwise, remove the unused dependency.
…ve, implement autobuilder into robotcontainer with a way to select starting position
michaelwillson415
dismissed
coderabbitai[bot]’s stale review
The merge-base changed after approval.
thatnerdjack
requested changes
Feb 15, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 60-73: The trajectory name is built by concatenating the Integer
startingPosition with "ToHub" causing names like "1ToHub" that don't match
available trajectories; in scoreAndClimbAuto update the code that produces the
name for Robot.mAutoFactory.trajectoryCmd(...) to map startingPosition (values
1,3,5) to position strings ("Left","Middle","Right") before concatenation (or
change startingPosition to a String and set it to those names), then use e.g.
mappedPosition + "ToHub" and "HubToClimb" consistently when calling
trajectoryCmd so it matches the actual trajectory filenames.
- Around line 37-38: The code builds trajectory names from the integer
startingPosition (field startingPosition) which yields names like "1ToHub" that
don't match deployed files (e.g., LeftStartToHub.traj, RightStartToHub.traj)
causing trajectoryCmd() to fail in scoreAndClimbAuto() and
scorePickupAndClimbAuto(); change the name resolution to map the integers to the
correct filenames (e.g., 1 -> "LeftStartToHub", 5 -> "RightStartToHub", and 3 ->
"MiddleStartToHub" or handle missing middle by returning a sensible default or
error), update wherever startingPosition is used to request trajectories, and
stop baking the dashboard chooser value into built commands in RobotContainer —
either read the chooser at command execution time or rebuild the auto commands
when the chooser changes so runtime changes to the dashboard affect selected
autos.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 144-146: The REPLAY/default branch incorrectly constructs the
intake with a simulation implementation (new IntakeIOSim()) which will run
physics during replay; change the intake instantiation in RobotContainer so it
uses the no-op anonymous IntakeIO (e.g., new Intake(new IntakeIO() {}))
consistent with how shooterRoller and beam break are constructed (refer to the
intake, IntakeIOSim, and IntakeIO symbols) to avoid simulated behavior in replay
logs.
In
`@src/main/java/org/team5924/frc2026/subsystems/rollers/hopperAgitator/Hopper.java`:
- Around line 37-38: The enum states ON and SPIT in Hopper (using
LoggedTunableNumber with names "HopperAgitatorOnVoltage" and
"HopperAgitatorSpitVoltage") both default to 0.0 so the agitator never moves;
update these to sensible non-zero default voltages or, if they are intentionally
placeholders, add a TODO comment on the ON and SPIT enum entries (similar to
ShooterRollerState) so they aren't forgotten and reviewers know to set real
defaults later. Ensure you modify the LoggedTunableNumber initializers for ON
and SPIT (or annotate them) so the intent is clear.
- Around line 64-71: The periodic() method incorrectly leaves allBroken true
when beamBreakIOs is empty, causing inputs.isFull to be true with no sensors;
update periodic() so it checks for sensors first (e.g., if beamBreakIOs.length
== 0 set inputs.isFull = false) otherwise compute allBroken from the loop as
before using beamBreakIOs and beamBreakInputs and then assign inputs.isFull;
reference the periodic() method, beamBreakIOs, beamBreakInputs, allBroken, and
inputs.isFull when making the change.
In
`@src/main/java/org/team5924/frc2026/subsystems/rollers/shooterRoller/ShooterRoller.java`:
- Line 23: The import for BeamBreakIOInputsAutoLogged is pointing to the wrong
package; update the import in ShooterRoller.java to reference the generated
class in the sensors package (replace
org.team5924.frc2026.subsystems.beamBreak.BeamBreakIOInputsAutoLogged with
org.team5924.frc2026.subsystems.sensors.BeamBreakIOInputsAutoLogged) so the
class generated by the `@AutoLog` on BeamBreakIO is correctly resolved; ensure no
other references use the old package name and rebuild to verify compilation.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (1)
85-89:DepotIntaketrajectory is run twice in sequence — is this intentional?The deadline runs
DepotIntake→DepotIntakesequentially while the intake command runs. If this is an intentional back-and-forth motion, a brief comment would clarify intent. If not, this may be a copy-paste error.src/main/java/org/team5924/frc2026/RobotContainer.java (1)
156-176: Remove NamedCommands registrations — these are dead code after migration to Choreo autos.These
NamedCommandsregistrations are PathPlanner event markers that are never consumed. The codebase migrates to Choreo-based autos viaAutoFactory(inAutoBuilder.java), and theautoChooseronly populates withAutoBuildercommands and SysId routines. No PathPlanner event markers reference these registered commands anywhere in the codebase, and the.pathfiles in the deploy directory are unused. Remove these three registrations to clean up dead code and avoid confusion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 85-89: The sequence inside Commands.deadline in AutoBuilder
currently calls Robot.mAutoFactory.trajectoryCmd("DepotIntake") twice in a row;
confirm intended behavior and fix accordingly: if the duplicate is accidental,
remove the second Robot.mAutoFactory.trajectoryCmd("DepotIntake") call (keeping
the single trajectory + AutoCommands.intake(intake)); if the robot should
perform a different follow-up trajectory, replace the second call with the
correct trajectory name (e.g., trajectoryCmd("SomeOtherTrajectory")); if the
repeated run is intentional, add a concise comment above this Commands.sequence
explaining why DepotIntake is executed twice to clarify intent for reviewers.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 180-189: Starting position is read once during RobotContainer
construction (startingPosition.get()) so dashboard changes after boot are
ignored; instead, modify the code to defer reading the chooser until auto
selection/execution by passing the LoggedDashboardChooser (or a
Supplier<Integer>) into AutoBuilder or by calling
AutoBuilder.setStartingPosition(...) at the moment the auto command is selected,
and update how autoBuilder.scoreAndClimbAuto() and
autoBuilder.scorePickupAndClimbAuto() are created so they query the chooser
dynamically (or are rebuilt) rather than capturing a fixed value during
construction; locate LoggedDashboardChooser, AutoBuilder.setStartingPosition,
and the calls to autoBuilder.scoreAndClimbAuto()/scorePickupAndClimbAuto() to
implement this change.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/subsystems/rollers/shooterRoller/ShooterRollerIOKrakenFOC.java (1)
33-36: Redundant override — delegates entirely tosuper.This override adds no behavior. Unless it's a deliberate hook for future customization or required by a framework, it can be removed. The same pattern appears in
IntakeIOKrakenFOCandHopperKrakenFOC, so if it's a team convention, consider documenting why.src/main/java/org/team5924/frc2026/RobotContainer.java (1)
333-336:configureAutoFactoryassigns to a public static field onRobot.Using
Robot.mAutoFactoryas a public static mutable field is a code smell — it creates a hidden global dependency betweenRobotContainer,AutoBuilder, andRobot. Consider passing theAutoFactoryinstance via constructor injection intoAutoBuilderinstead.
…er, fixed supplier for startingPosition
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 70-78: The auto routines (scoreAndClimbAuto and
scorePickupAndClimbAuto) never reset ShooterState or IntakeState back to OFF
after the timed run, so add explicit reset commands after the shooting/intake
steps: locate scoreAndClimbAuto and append a command after the Commands.run(...,
shooter).withTimeout(1.0) that calls shooter.setGoalState(ShooterState.OFF) (use
Commands.run or Commands.runOnce bound to the shooter) and similarly in
scorePickupAndClimbAuto append a command after the intake deadline that calls
intake.setGoalState(IntakeState.OFF) (or update RobotState accordingly); ensure
these reset commands are part of the returned Commands.sequence so they always
run when the prior step completes/cancels.
- Around line 80-95: The trajectory names built in scorePickupAndClimbAuto() use
startingPositionSupplier.get() + "ToHub" (and similar elsewhere) which yields
"LeftToHub"/"RightToHub"/"MidToHub" but the deployed files are named
"LeftStartToHub"/"RightStartToHub" and there is no "Mid" file; update the name
construction in scorePickupAndClimbAuto() (and any other uses of
startingPositionSupplier.get() + "ToHub"/"Start" patterns) to use
startingPositionSupplier.get() + "StartToHub" so it matches deployed files, and
either add a MidStartToHub.traj to deployment or remove/validate the "Mid"
starting option to avoid missing-file runtime errors.
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
156-176: NamedCommands are registered but shooter/intake states are never reset to OFF.The "Run Shooter" and "Run Intake" named commands set states (
AUTO_SHOOTING,INTAKE) but there's no corresponding command to turn them off. If these named commands are triggered by Choreo event markers, the shooter/intake will remain active indefinitely after the marker fires. Consider registering complementary "Stop Shooter" / "Stop Intake" commands, or ensure the auto sequence explicitly resets states.src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (2)
50-68: Remove commented-out dead code.The entire
basicDriveAuto()method is commented out. If it's no longer needed after the Choreo migration, remove it to keep the file clean. It can always be recovered from version control.
34-34: Class nameAutoBuildershadows PathPlanner's well-knownAutoBuilder.PathPlanner's
com.pathplanner.lib.auto.AutoBuilderis a widely recognized class in FRC. Naming your custom classAutoBuilderin the same project (which still depends on PathPlanner forNamedCommands) will cause confusion. Consider renaming to something likeAutoRoutinesorAutoSequences.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
155-175:⚠️ Potential issue | 🟡 Minor
NamedCommandsregister fire-and-forget state changes that are never reset.
"Run Shooter"setsAUTO_SHOOTINGand"Run Intake"setsINTAKE, both viarunOnce(instant). Neither has a corresponding "stop" command registered. If these named commands are triggered (e.g., from a PathPlanner path still referencing them), the subsystems will stay in those states indefinitely.If these are vestigial from the PathPlanner migration, consider removing them. If still needed, register corresponding "Stop Shooter" / "Stop Intake" named commands.
🤖 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 155 - 175, The named commands "Run Shooter" and "Run Intake" (registered via NamedCommands.registerCommand using Commands.runOnce and calling shooter.setGoalState(ShooterState.AUTO_SHOOTING) and intake.setGoalState(IntakeState.INTAKE)) leave subsystems in non-default states with no reset; either remove these vestigial registrations if unused by PathPlanner, or add matching "Stop Shooter" and "Stop Intake" NamedCommands that call shooter.setGoalState(...) and intake.setGoalState(...) to transition each subsystem back to its safe/default state (e.g., ShooterState.IDLE / IntakeState.STOP or your subsystems' neutral states) so any triggers can be undone.
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (3)
84-87: Misleading indentation: intake-OFF command appears nested insideCommands.deadlinebut is actually aCommands.sequenceargument.Line 87 is indented as if it belongs to the
Commands.deadline(...)block, but the deadline closes on line 86 (the double))afterintake). Functionally correct, but will confuse readers.✏️ Suggested formatting
Commands.deadline( Robot.mAutoFactory.trajectoryCmd("DepotIntake"), Commands.run(() -> intake.setGoalState(IntakeState.INTAKE), intake)), - Commands.runOnce(() -> intake.setGoalState(IntakeState.OFF), intake), + Commands.runOnce(() -> intake.setGoalState(IntakeState.OFF), intake), Robot.mAutoFactory.trajectoryCmd("DepotToHub"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` around lines 84 - 87, The indentation for the intake OFF command is misleading: reformat the block so the closing of Commands.deadline(...) is visually clear and the Commands.runOnce(() -> intake.setGoalState(IntakeState.OFF), intake) line is aligned as the next argument of the surrounding Commands.sequence (not nested inside Commands.deadline); locate the snippet around Commands.deadline, Commands.run and Commands.runOnce and adjust whitespace/indentation so the matching parentheses for Commands.deadline(...) are obvious and the intake.setGoalState(IntakeState.OFF) call is visually at the same level as the sequence arguments.
45-63: Remove commented-out dead code.The entire
basicDriveAuto()method is commented out. Since it's been replaced by the new auto routines and is tracked in version control, it should be removed 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/commands/AutoBuilder.java` around lines 45 - 63, Remove the commented-out dead method basicDriveAuto and its entire block (the commented Commands.runOnce... andDriveToPose sequence) from AutoBuilder; simply delete the commented lines referencing basicDriveAuto, RobotState, DriveToPose and AllianceFlipUtil so the file no longer contains that inactive legacy code.
65-75:scoreAndClimbAutoduplicates the logic ofstartToHub— consider reusing the helper.Lines 67–68 replicate the same
resetOdometry+trajectoryCmdpattern thatstartToHubencapsulates. Additionally,scoreAndClimbAutodoesn't handle the"Mid"case at all (no trajectory exists for "Mid"), unlikescorePickupAndClimbAutowhich delegates tostartToHub.♻️ Proposed refactor
public Command scoreAndClimbAuto() { return Commands.sequence( - Robot.mAutoFactory.resetOdometry(startingPositionSupplier.get() + "StartToHub"), - Robot.mAutoFactory.trajectoryCmd(startingPositionSupplier.get() + "StartToHub"), + startToHub(startingPositionSupplier.get()), Commands.run(() -> shooter.setGoalState(ShooterState.AUTO_SHOOTING), shooter) .withTimeout(1.0), Commands.runOnce(() -> shooter.setGoalState(ShooterState.OFF), shooter),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` around lines 65 - 75, Replace the duplicated resetOdometry+trajectoryCmd sequence in scoreAndClimbAuto by delegating to the existing startToHub helper (which encapsulates Robot.mAutoFactory.resetOdometry(...) and Robot.mAutoFactory.trajectoryCmd(...)), and remove the explicit calls to Robot.mAutoFactory.resetOdometry(startingPositionSupplier.get() + "StartToHub") and trajectoryCmd(startingPositionSupplier.get() + "StartToHub"); also ensure scoreAndClimbAuto follows the same "Mid" handling as scorePickupAndClimbAuto by letting startToHub handle the "Mid" case so no trajectory is assumed missing.
🤖 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/AutoBuilder.java`:
- Around line 97-106: The startToHub method inconsistently uses
startingPositionSupplier.get() for trajectory names while taking
startingPosition as a parameter; update the method to use the startingPosition
parameter for building the trajectory keys (replace
startingPositionSupplier.get() with startingPosition in the
Robot.mAutoFactory.resetOdometry(...) and Robot.mAutoFactory.trajectoryCmd(...)
calls) so the parameter is honored consistently by startToHub(String
startingPosition).
- Around line 98-100: The code in AutoBuilder uses reference equality for
strings and returns null; change the condition to use value equality (e.g.,
"Mid".equals(startingPosition)) to avoid incorrect comparisons and NPEs, and
instead of returning null return a no-op Command (for example Commands.none() or
an InstantCommand/WaitCommand) so callers like Commands.sequence(...) won't
throw a NullPointerException; update the branch in AutoBuilder where
startingPosition is checked and ensure the returned type is a valid Command
object.
---
Outside diff comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 155-175: The named commands "Run Shooter" and "Run Intake"
(registered via NamedCommands.registerCommand using Commands.runOnce and calling
shooter.setGoalState(ShooterState.AUTO_SHOOTING) and
intake.setGoalState(IntakeState.INTAKE)) leave subsystems in non-default states
with no reset; either remove these vestigial registrations if unused by
PathPlanner, or add matching "Stop Shooter" and "Stop Intake" NamedCommands that
call shooter.setGoalState(...) and intake.setGoalState(...) to transition each
subsystem back to its safe/default state (e.g., ShooterState.IDLE /
IntakeState.STOP or your subsystems' neutral states) so any triggers can be
undone.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 65-95: The auto commands call startingPositionSupplier.get()
during construction, so dashboard changes after boot are ignored; fix by
deferring command construction to runtime: wrap each returned sequence in
Commands.defer(() -> /* build sequence */ , Set.of(drive, shooter, intake)) so
startingPositionSupplier.get() (used in scoreAndClimbAuto(),
scorePickupAndClimbAuto(), and the startToHub/Robot.mAutoFactory.trajectoryCmd
calls) is invoked at execution time; ensure the deferred supplier builds and
returns the same Commands.sequence structure and include the involved subsystems
(drive, shooter, intake, etc.) in the second argument to defer.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 178-188: The default starting position registered via
LoggedDashboardChooser<String> startingPosition uses addDefaultOption("Middle",
"Mid"), which maps to "Mid" and causes AutoBuilder to look for a MidStartToHub
trajectory that doesn't exist; fix by either adding the missing MidStartToHub
trajectory to deploy or change the default option to an existing start key
(e.g., calling startingPosition.addDefaultOption("Left", "Left") or
startingPosition.addDefaultOption("Right", "Right")) so
AutoBuilder.setStartingPosition(startingPosition::get) returns a valid
trajectory name used by autoChooser and auto routines.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 84-87: The indentation for the intake OFF command is misleading:
reformat the block so the closing of Commands.deadline(...) is visually clear
and the Commands.runOnce(() -> intake.setGoalState(IntakeState.OFF), intake)
line is aligned as the next argument of the surrounding Commands.sequence (not
nested inside Commands.deadline); locate the snippet around Commands.deadline,
Commands.run and Commands.runOnce and adjust whitespace/indentation so the
matching parentheses for Commands.deadline(...) are obvious and the
intake.setGoalState(IntakeState.OFF) call is visually at the same level as the
sequence arguments.
- Around line 45-63: Remove the commented-out dead method basicDriveAuto and its
entire block (the commented Commands.runOnce... andDriveToPose sequence) from
AutoBuilder; simply delete the commented lines referencing basicDriveAuto,
RobotState, DriveToPose and AllianceFlipUtil so the file no longer contains that
inactive legacy code.
- Around line 65-75: Replace the duplicated resetOdometry+trajectoryCmd sequence
in scoreAndClimbAuto by delegating to the existing startToHub helper (which
encapsulates Robot.mAutoFactory.resetOdometry(...) and
Robot.mAutoFactory.trajectoryCmd(...)), and remove the explicit calls to
Robot.mAutoFactory.resetOdometry(startingPositionSupplier.get() + "StartToHub")
and trajectoryCmd(startingPositionSupplier.get() + "StartToHub"); also ensure
scoreAndClimbAuto follows the same "Mid" handling as scorePickupAndClimbAuto by
letting startToHub handle the "Mid" case so no trajectory is assumed missing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/AutoBuilder.java`:
- Around line 84-88: Adjust the indentation so that the Commands.runOnce(() ->
intake.setGoalState(IntakeState.OFF), intake) line is aligned with other
sequence-level arguments (same indentation as
Robot.mAutoFactory.trajectoryCmd("DepotIntake") and
Robot.mAutoFactory.trajectoryCmd("DepotToHub")) rather than the inner deadline
block; locate the block using Commands.sequence, Commands.deadline, Commands.run
and Commands.runOnce in AutoBuilder.java and re-indent the runOnce line to the
sequence-argument level to avoid implying it's an argument to Commands.deadline.
- Around line 65-75: The scoreAndClimbAuto() method builds a trajectory name
directly using startingPositionSupplier.get() + "StartToHub", which fails for
"Mid" and duplicates logic; change scoreAndClimbAuto() to call the existing
startToHub() helper (and use its reset/trajectory behavior) instead of
constructing the trajectory name yourself so that startToHub() can return
Commands.none() for "Mid" and avoid runtime failures — update references inside
scoreAndClimbAuto() to use startToHub() (and keep subsequent shooting/cleanup
commands as-is).
- Around line 39-43: startingPositionSupplier is nullable and calling
startingPositionSupplier.get() can NPE; initialize it with a safe default (e.g.,
Supplier<String> that returns a default like "UNKNOWN" or empty string) and/or
add a null-guard in the auto-building methods that call
startingPositionSupplier.get() to avoid NPEs; also validate arguments in
setStartingPosition(Supplier<String>) to reject null (throw
IllegalArgumentException) so startingPositionSupplier is never set to null after
initialization. Ensure you update the places that call
startingPositionSupplier.get() (the auto methods that currently invoke
startingPositionSupplier.get()) to rely on the safe default or perform a guard
and use that safe default value.
- Around line 67-68: The calls to startingPositionSupplier.get() are being
evaluated eagerly inside scoreAndClimbAuto() and scorePickupAndClimbAuto(), so
change the creation of the commands that use
Robot.mAutoFactory.resetOdometry(...) and Robot.mAutoFactory.trajectoryCmd(...)
to defer evaluation until execution; wrap those expressions in a lambda or use
Commands.defer() so that startingPositionSupplier.get() is called at runtime
rather than during RobotContainer construction (update the return values of
scoreAndClimbAuto and scorePickupAndClimbAuto to return deferred commands that
call startingPositionSupplier.get() when executed).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/Robot.java (1)
48-48: Consider initialization safety for staticmAutoFactoryfield.The
public static AutoFactory mAutoFactoryfield is initialized byRobotContainer.configureAutoFactory()after subsystem construction. This works but relies on careful initialization order. If any code attempts to accessmAutoFactorybeforeconfigureAutoFactory()is called, it will benull.This pattern is common in FRC codebases, but consider adding a null check or lazy initialization if this becomes problematic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Robot.java` at line 48, The static public field mAutoFactory can be null if accessed before RobotContainer.configureAutoFactory() runs; change access to go through a lazy-initializing getter (e.g., getAutoFactory()) that checks if mAutoFactory is null and, if so, calls RobotContainer.configureAutoFactory() (and assigns the result), optionally making mAutoFactory private and synchronizing the getter for thread-safety, or add null-checks where mAutoFactory is read and throw a clear IllegalStateException if not initialized.
🤖 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`:
- Line 236: The build fails because AutoBuilder is being constructed with a
non-existent variable `shooter`; replace `shooter` with the actual SuperShooter
instance (e.g., `superShooterRight` or `superShooterLeft`) so the call to the
AutoBuilder constructor uses a real symbol; update the call where `var
autoBuilder = new AutoBuilder(drive, shooter, intake);` to pass the correct
`SuperShooter` variable (for example `superShooterRight`) that matches the
AutoBuilder constructor signature.
- Line 39: There is a duplicate import of
org.team5924.frc2026.subsystems.SuperShooter.ShooterState; remove the redundant
import so ShooterState is only imported once; locate the two identical import
statements for ShooterState in RobotContainer.java and delete one of them (keep
a single import line for ShooterState).
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/Robot.java`:
- Line 48: The static public field mAutoFactory can be null if accessed before
RobotContainer.configureAutoFactory() runs; change access to go through a
lazy-initializing getter (e.g., getAutoFactory()) that checks if mAutoFactory is
null and, if so, calls RobotContainer.configureAutoFactory() (and assigns the
result), optionally making mAutoFactory private and synchronizing the getter for
thread-safety, or add null-checks where mAutoFactory is read and throw a clear
IllegalStateException if not initialized.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Robot.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/commands/AutoBuilder.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (1)
48-66:⚠️ Potential issue | 🟠 MajorEager evaluation: starting position is baked in at command construction, not execution.
startingPositionSupplier.get()on line 54 is called whenscoreAndClimbAuto()is invoked (typically duringRobotContainerconstruction), not when the command actually runs. If the driver changes the starting position on the dashboard after initialization, the change won't take effect.Wrap the command with
Commands.defer()to evaluate the supplier at execution time:🔧 Proposed fix using deferred execution
public Command scoreAndClimbAuto() { if (startingPositionSupplier == null) { throw new IllegalStateException( "starting position must be set before building auto commands"); } - return Commands.sequence( - startToHub(startingPositionSupplier.get()), + return Commands.defer(() -> Commands.sequence( + startToHub(startingPositionSupplier.get()), Commands.parallel( Commands.run(() -> superShooterLeft.setGoalState(ShooterState.AUTO_SHOOTING), superShooterLeft), Commands.run(() -> superShooterRight.setGoalState(ShooterState.AUTO_SHOOTING), superShooterRight) ).withTimeout(1.0), Commands.parallel( Commands.runOnce(() -> superShooterLeft.setGoalState(ShooterState.OFF), superShooterLeft), Commands.runOnce(() -> superShooterRight.setGoalState(ShooterState.OFF), superShooterRight) ), Robot.mAutoFactory.trajectoryCmd("HubToClimb") - ); + ), Set.of(drive, superShooterLeft, superShooterRight)); }Note: Add
import java.util.Set;and pass the required subsystems toCommands.defer().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` around lines 48 - 66, The command builder scoreAndClimbAuto eagerly calls startingPositionSupplier.get() at construction via startToHub(startingPositionSupplier.get()), so wrap the sequence in Commands.defer(...) to defer evaluating startingPositionSupplier until command execution; replace the direct Commands.sequence(...) return with Commands.defer(() -> Commands.sequence(startToHub(startingPositionSupplier.get()), ...), /* pass required subsystems as a Set */) and ensure you import java.util.Set and include any subsystems used (e.g., superShooterLeft, superShooterRight, climb, drivetrain/auto factory if needed) in the Set parameter to Commands.defer.
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (1)
33-37: Unuseddrivefield.The
drivefield is declared and injected via@RequiredArgsConstructorbut is never used in this class. If it's intended for future use (e.g., climb integration), consider adding a// TODOcomment. Otherwise, remove it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` around lines 33 - 37, The private final Drive drive field in AutoBuilder is declared (injected via `@RequiredArgsConstructor`) but never used; either remove the unused field to avoid confusion or, if you intend to use it later, add a clear TODO comment next to the declaration (e.g., "// TODO: use Drive for future auto/climb integration") and keep the field; update the constructor wiring accordingly (remove from constructor if deleting the field) so there are no unused constructor parameters left.
🤖 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/AutoBuilder.java`:
- Around line 68-100: scorePickupAndClimbAuto eagerly builds Commands.sequence
causing subsystem requirement conflicts and repeats shooter on/off blocks; wrap
the sequence in Commands.defer to defer evaluation (use Commands.defer(() ->
Commands.sequence(...), Set.of(drive, superShooterLeft, superShooterRight,
intake)) similar to scoreAndClimbAuto) and extract the duplicated shooter
patterns into helper methods like shootersOn() and shootersOff() that return the
parallel on-withTimeout and parallel off commands respectively; replace the two
duplicated blocks in scorePickupAndClimbAuto with shootersOn() and
shootersOff(), and apply the same refactor to scoreAndClimbAuto so both use
Commands.defer and the new helper methods referencing superShooterLeft,
superShooterRight, intake, and drive.
---
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 48-66: The command builder scoreAndClimbAuto eagerly calls
startingPositionSupplier.get() at construction via
startToHub(startingPositionSupplier.get()), so wrap the sequence in
Commands.defer(...) to defer evaluating startingPositionSupplier until command
execution; replace the direct Commands.sequence(...) return with
Commands.defer(() ->
Commands.sequence(startToHub(startingPositionSupplier.get()), ...), /* pass
required subsystems as a Set */) and ensure you import java.util.Set and include
any subsystems used (e.g., superShooterLeft, superShooterRight, climb,
drivetrain/auto factory if needed) in the Set parameter to Commands.defer.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 33-37: The private final Drive drive field in AutoBuilder is
declared (injected via `@RequiredArgsConstructor`) but never used; either remove
the unused field to avoid confusion or, if you intend to use it later, add a
clear TODO comment next to the declaration (e.g., "// TODO: use Drive for future
auto/climb integration") and keep the field; update the constructor wiring
accordingly (remove from constructor if deleting the field) so there are no
unused constructor parameters left.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/commands/AutoBuilder.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/RobotContainer.java
…utos so the starting position can be changed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2026/RobotContainer.java (1)
108-109: Public static mutable field — acceptable but consider encapsulation.
autoFactoryas apublic staticmutable field enables direct access fromAutoBuilder, but violates encapsulation. This pattern is common in FRC codebases for simplicity. If you want to improve this later, consider a static getter or passing the factory as a dependency.🤖 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 108 - 109, autoFactory is declared as a public static mutable field on RobotContainer which exposes internal state to AutoBuilder; change its visibility to private static and provide a static accessor method (e.g., getAutoFactory()) or update AutoBuilder to accept an AutoFactory instance via constructor or method injection so the factory is passed as a dependency rather than accessed directly as RobotContainer.autoFactory.
🤖 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/AutoBuilder.java`:
- Around line 61-78: The Commands.defer call in scorePickupAndClimbAuto() is
missing the Drive subsystem in its requirements set which can cause command
conflicts; update the requirements Set.of(superShooterLeft, superShooterRight,
intake) to also include drive (the Drive subsystem instance used elsewhere) so
the deferred sequence properly declares Drive as a required subsystem—modify the
requirements for Commands.defer in scorePickupAndClimbAuto() to match the
pattern used in scoreAndClimbAuto() by adding drive to the Set.of(...) list.
- Around line 47-59: The deferred command returned by scoreAndClimbAuto uses
RobotContainer.autoFactory.trajectoryCmd("HubToClimb") which requires the Drive
subsystem, but Commands.defer(...) currently only lists superShooterLeft and
superShooterRight; add Drive to the AutoBuilder (e.g., inject a Drive drive
field via the constructor or otherwise obtain a reference) and include that
Drive instance in the requirements Set passed to Commands.defer so the deferred
command properly reserves Drive when scheduled; update any RobotContainer
instantiation of AutoBuilder to pass the Drive dependency.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 108-109: autoFactory is declared as a public static mutable field
on RobotContainer which exposes internal state to AutoBuilder; change its
visibility to private static and provide a static accessor method (e.g.,
getAutoFactory()) or update AutoBuilder to accept an AutoFactory instance via
constructor or method injection so the factory is passed as a dependency rather
than accessed directly as RobotContainer.autoFactory.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Robot.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/commands/AutoBuilder.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2026/Robot.java
Comment on lines
+47
to
+59
| public Command scoreAndClimbAuto() { | ||
| if (startingPositionSupplier == null) { | ||
| throw new IllegalStateException( | ||
| "starting position must be set before building auto commands"); | ||
| } | ||
| return Commands.defer(() -> Commands.sequence( | ||
| startToHub(startingPositionSupplier.get()), | ||
| shootersOn(), | ||
| shootersOff(), | ||
| RobotContainer.autoFactory.trajectoryCmd("HubToClimb") | ||
| // Commands.run(() -> climb.setGoalState(ClimbState.L1_CLIMB), climb) | ||
| ), Set.of(superShooterLeft, superShooterRight)); | ||
| } |
Contributor
There was a problem hiding this comment.
Commands.defer() requirements set is missing the Drive subsystem.
The deferred sequence includes RobotContainer.autoFactory.trajectoryCmd("HubToClimb") which requires the Drive subsystem internally. However, the requirements set on line 58 only includes the shooters. This could cause scheduling conflicts if another command tries to use drive while this deferred command is scheduled but hasn't yet created its actual command.
To fix this, AutoBuilder would need access to the Drive subsystem, either passed via constructor or accessed statically.
🛡️ Proposed fix — add Drive to AutoBuilder and requirements
Add drive to the constructor (update @RequiredArgsConstructor fields):
+ private final Drive drive;
private final SuperShooter superShooterLeft;
private final SuperShooter superShooterRight;
private final Intake intake;Update the defer requirements:
- ), Set.of(superShooterLeft, superShooterRight));
+ ), Set.of(drive, superShooterLeft, superShooterRight));And update the instantiation in RobotContainer:
- var autoBuilder = new AutoBuilder(superShooterLeft, superShooterRight, intake);
+ var autoBuilder = new AutoBuilder(drive, superShooterLeft, superShooterRight, 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/commands/AutoBuilder.java` around lines 47
- 59, The deferred command returned by scoreAndClimbAuto uses
RobotContainer.autoFactory.trajectoryCmd("HubToClimb") which requires the Drive
subsystem, but Commands.defer(...) currently only lists superShooterLeft and
superShooterRight; add Drive to the AutoBuilder (e.g., inject a Drive drive
field via the constructor or otherwise obtain a reference) and include that
Drive instance in the requirements Set passed to Commands.defer so the deferred
command properly reserves Drive when scheduled; update any RobotContainer
instantiation of AutoBuilder to pass the Drive dependency.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (2)
61-78:⚠️ Potential issue | 🟡 MinorSame missing
Drivesubsystem issue in defer requirements.Line 77's requirements set should also include
drivefor the same reason asscoreAndClimbAuto().- ), Set.of(superShooterLeft, superShooterRight, intake)); + ), Set.of(drive, superShooterLeft, superShooterRight, 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/commands/AutoBuilder.java` around lines 61 - 78, The deferred command in scorePickupAndClimbAuto() is missing the Drive subsystem in its requirements set; update the final Set.of(...) passed to Commands.defer in scorePickupAndClimbAuto() to include drive (e.g., Set.of(superShooterLeft, superShooterRight, intake, drive)) similar to how scoreAndClimbAuto() includes the drive requirement so the trajectory commands and drive-dependent actions have exclusive access.
47-59:⚠️ Potential issue | 🟡 Minor
Commands.defer()requirements set is missing theDrivesubsystem.The deferred sequence includes
RobotContainer.autoFactory.trajectoryCmd("HubToClimb")which requires theDrivesubsystem. However, line 58 only includes the shooters in the requirements set. This can cause scheduling conflicts if another command uses drive while this deferred command is scheduled.Add
Driveas a constructor parameter and include it in the requirements:`@RequiredArgsConstructor` public class AutoBuilder { + private final Drive drive; private final SuperShooter superShooterLeft; private final SuperShooter superShooterRight; // private final Climb climb; private final Intake intake;- ), Set.of(superShooterLeft, superShooterRight)); + ), Set.of(drive, superShooterLeft, superShooterRight));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` around lines 47 - 59, The deferred command built in scoreAndClimbAuto() uses RobotContainer.autoFactory.trajectoryCmd("HubToClimb") which requires the Drive subsystem, but the Commands.defer(...) requirements Set only includes superShooterLeft/superShooterRight; add Drive to the class by accepting a Drive parameter in the constructor (e.g., Drive drive) and store it on the instance, then include that drive instance in the requirements Set passed to Commands.defer (e.g., Set.of(superShooterLeft, superShooterRight, drive)) so the Drive subsystem is reserved for the deferred sequence.
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (1)
27-27: Unused import:Driveis imported but not used.The
Driveclass is imported but there's no field or local usage in this class. Either remove the import or addDriveas a constructor parameter if it's needed for theCommands.defer()requirements (see related comment below).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java` at line 27, The AutoBuilder class imports Drive but never uses it; remove the unused import "import org.team5924.frc2026.subsystems.drive.Drive;" from AutoBuilder to eliminate the dead dependency, or if Drive is required for command construction, add a Drive parameter to AutoBuilder's constructor and store it for use when building deferred commands (see Commands.defer()) so the import becomes necessary; update any references in the class to use the stored Drive instance.src/main/java/org/team5924/frc2026/RobotContainer.java (1)
108-108: Consider encapsulating the staticautoFactoryfield.Public static mutable fields reduce encapsulation and make dependencies harder to track. Consider either:
- Making it private with a static getter
- Passing
autoFactorytoAutoBuildervia constructor injectionThis would improve testability and make the dependency explicit.
- public static AutoFactory autoFactory; + private static AutoFactory autoFactory; + + public static AutoFactory getAutoFactory() { + return autoFactory; + }🤖 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 108, The public static field autoFactory in RobotContainer should be encapsulated: change the field to private (private static AutoFactory autoFactory) and expose it via a public static getter method (e.g., getAutoFactory()), or alternatively remove the static dependency and inject AutoFactory into AutoBuilder through a constructor parameter (modify AutoBuilder to accept an AutoFactory in its constructor and update call sites in RobotContainer to pass the instance). Update all usages of RobotContainer.autoFactory to use RobotContainer.getAutoFactory() or the injected instance in AutoBuilder to ensure no code directly accesses the mutable static field.src/main/java/org/team5924/frc2026/Robot.java (1)
19-19: Unused import:choreo.auto.AutoFactoryis not used in this file.The
AutoFactoryclass is instantiated and used inRobotContainer.java, not inRobot.java. This import appears to be unnecessary here.-import choreo.auto.AutoFactory;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/team5924/frc2026/Robot.java` at line 19, The import choreo.auto.AutoFactory in Robot.java is unused and should be removed to avoid unused-import warnings; open Robot.java, delete the line importing AutoFactory (the class is used in RobotContainer.java, not Robot.java), save and rebuild to confirm the compiler/IDE no longer reports the unused import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 61-78: The deferred command in scorePickupAndClimbAuto() is
missing the Drive subsystem in its requirements set; update the final
Set.of(...) passed to Commands.defer in scorePickupAndClimbAuto() to include
drive (e.g., Set.of(superShooterLeft, superShooterRight, intake, drive)) similar
to how scoreAndClimbAuto() includes the drive requirement so the trajectory
commands and drive-dependent actions have exclusive access.
- Around line 47-59: The deferred command built in scoreAndClimbAuto() uses
RobotContainer.autoFactory.trajectoryCmd("HubToClimb") which requires the Drive
subsystem, but the Commands.defer(...) requirements Set only includes
superShooterLeft/superShooterRight; add Drive to the class by accepting a Drive
parameter in the constructor (e.g., Drive drive) and store it on the instance,
then include that drive instance in the requirements Set passed to
Commands.defer (e.g., Set.of(superShooterLeft, superShooterRight, drive)) so the
Drive subsystem is reserved for the deferred sequence.
---
Nitpick comments:
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Line 27: The AutoBuilder class imports Drive but never uses it; remove the
unused import "import org.team5924.frc2026.subsystems.drive.Drive;" from
AutoBuilder to eliminate the dead dependency, or if Drive is required for
command construction, add a Drive parameter to AutoBuilder's constructor and
store it for use when building deferred commands (see Commands.defer()) so the
import becomes necessary; update any references in the class to use the stored
Drive instance.
In `@src/main/java/org/team5924/frc2026/Robot.java`:
- Line 19: The import choreo.auto.AutoFactory in Robot.java is unused and should
be removed to avoid unused-import warnings; open Robot.java, delete the line
importing AutoFactory (the class is used in RobotContainer.java, not
Robot.java), save and rebuild to confirm the compiler/IDE no longer reports the
unused import.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Line 108: The public static field autoFactory in RobotContainer should be
encapsulated: change the field to private (private static AutoFactory
autoFactory) and expose it via a public static getter method (e.g.,
getAutoFactory()), or alternatively remove the static dependency and inject
AutoFactory into AutoBuilder through a constructor parameter (modify AutoBuilder
to accept an AutoFactory in its constructor and update call sites in
RobotContainer to pass the instance). Update all usages of
RobotContainer.autoFactory to use RobotContainer.getAutoFactory() or the
injected instance in AutoBuilder to ensure no code directly accesses the mutable
static field.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2026/Robot.javasrc/main/java/org/team5924/frc2026/RobotContainer.javasrc/main/java/org/team5924/frc2026/commands/AutoBuilder.java
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.