Alpha autos - basic structure for an auto builder and drive to pose commands#33
Conversation
… to newest version
…ith a drivetopose and alliancefliputil
|
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. WalkthroughAdds and removes many path-planner assets and navgrid data; introduces DriveToPose and AutoBuilder commands; extends RobotState with estimatedPose, gyroOffset and resetPose; adds AllianceFlipUtil and wiring for shooter/intake subsystems and named auto commands; updates Drive.periodic to set estimatedPose; updates vendordeps manifests and a few formatting/stub utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant AutoBuilder
participant RobotState
participant DriveToPose
participant PID as PID(drive,theta)
participant Drive
Operator->>AutoBuilder: basicDriveAuto()
AutoBuilder->>RobotState: getEstimatedPose()
RobotState-->>AutoBuilder: current Pose
AutoBuilder->>RobotState: resetPose(flipped target)
AutoBuilder->>DriveToPose: start(target suppliers)
DriveToPose->>PID: reset()
loop control loop
DriveToPose->>RobotState: getEstimatedPose()
RobotState-->>DriveToPose: current pose
DriveToPose->>PID: compute drive & theta outputs
PID-->>DriveToPose: driveSpeed, turnSpeed
DriveToPose->>Drive: runVelocity(chassisSpeeds)
end
DriveToPose->>Drive: stop()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/commands/AutoBuilder.java`:
- Around line 53-60: The DriveToPose call uses the same supplier for both target
and robot (RobotState.getInstance().getEstimatedPose()), so the error is always
zero and the command does nothing; update the target pose supplier (the second
argument to DriveToPose) to return a different pose than the robot supplier—for
example compute RobotState.getInstance().getEstimatedPose() and add a forward
Translation2d (respecting AllianceFlipUtil.shouldFlip()) or use
Pose2d.plus/relative transform to shift the desired X distance—so the PID sees a
nonzero error and the command drives for the 0.6s timeout.
- Around line 44-51: The resetPose call is using
RobotState.getInstance().getEstimatedPose() which currently returns
Pose2d.kZero, so the translation is always (0,0); replace that source with the
live/odometry pose (or fix getEstimatedPose so it returns live data) so
resetPose is given the robot's actual position. Concretely, change the argument
to use the RobotState method that provides the current/odometry pose (e.g.,
getCurrentPose(), getLatestPose(), or the odometry getter your codebase exposes)
instead of getEstimatedPose(), keep the AllianceFlipUtil.apply(...) and
Rotation2d.kPi inversion as-is, and ensure resetPose receives a non-zero Pose2d
representing the robot's real pose.
In `@src/main/java/org/team5924/frc2026/commands/AutoScoreCommands.java`:
- Around line 41-44: Change the autoScore method to return a WPILib Command
instead of Runnable and replace the UnsupportedOperationException with a safe
no-op Command implementation; specifically, update the signature public static
Runnable autoScore(Drive drive, SuperShooter shooter, BooleanSupplier s) to
return Command, remove the throw, and return a no-op Command (e.g.,
Commands.none() or an InstantCommand that does nothing and declares no subsystem
requirements) so callers can compose it safely with .andThen(), .alongWith(),
etc., while keeping the Drive, SuperShooter and BooleanSupplier parameters for
the real implementation later.
In `@src/main/java/org/team5924/frc2026/commands/drive/DriveToPose.java`:
- Around line 35-36: The feedforward suppliers linearFF and omegaFF stored by
the three-argument DriveToPose constructor are never used in execute(), so apply
them when computing the commanded chassis velocity: in DriveToPose.execute() add
the linearFF.get() (or linearFF.get().getX()/getY() as appropriate for
Translation2d) into the translation component and omegaFF.getAsDouble() into the
angular/omega component of the Pose/ChassisSpeeds sent to the drivetrain or to
the method that sets output; ensure you reference the class fields linearFF and
omegaFF (and not the default lambdas) and combine their values with the existing
PID/pose-based velocity before calling the drivetrain output function so
AutoBuilder.veryBasicAuto() produces non-zero motion.
- Around line 86-101: The PID calls are inverted: replace
drivePID.calculate(distanceError, 0) with either drivePID.calculate(0,
distanceError) or negate the PID output so driveSpeed drives toward the target
(ensure driveSpeed is positive when distanceError > 0), and stop precomputing
angleError; call thetaPID.calculate(current.getRotation().getRadians(),
targetPose.getRotation().getRadians()) (so enableContinuousInput on thetaPID can
wrap correctly) instead of thetaPID.calculate(angleError, 0); keep using
errorTranslation.getAngle(), Translation2d and
ChassisSpeeds.fromFieldRelativeSpeeds and then call drive.runVelocity(speeds).
In `@src/main/java/org/team5924/frc2026/RobotState.java`:
- Around line 50-57: estimatedPose is never updated so getEstimatedPose() always
returns Pose2d.kZero; update the field whenever the robot pose is reset or the
drive estimator updates: in RobotState.resetPose(Pose2d) assign estimatedPose =
pose in addition to setting odometryPose and gyroOffset, and ensure
Drive.periodic() (where the SwerveDrivePoseEstimator is stepped and
setOdometryPose(...) is called) also writes the estimator result back to
RobotState. Use the existing symbols estimatedPose, getEstimatedPose(),
resetPose(Pose2d), Drive.periodic(), SwerveDrivePoseEstimator and
setOdometryPose(...) to locate where to assign the current estimated pose.
In `@src/main/java/org/team5924/frc2026/util/AllianceFlipUtil.java`:
- Around line 26-32: The applyX and applyY methods in AllianceFlipUtil are
subtracting meter-based coordinates from inch constants (651.2 and 317.7);
replace those inch literals with their meter equivalents (approximately 16.54
and 8.07) or compute meters via conversion (e.g., 651.2 * 0.0254 and 317.7 *
0.0254) and use named constants (e.g., FIELD_LENGTH_METERS, FIELD_WIDTH_METERS)
so applyX, applyY and shouldFlip operate in meters consistently.
In `@vendordeps/photonlib.json`:
- Around line 7-11: The mavenUrls and jsonUrl in vendordeps/photonlib.json point
to endpoints that return 404/302; update these entries so they reference the
correct, accessible PhotonVision Maven endpoints (or remove invalid entries) and
ensure the jsonUrl resolves with HTTP 200. Specifically, verify and correct the
"mavenUrls" array and the "jsonUrl" value in photonlib.json (follow redirects to
the final URL if needed), test them with a simple HTTP request (curl/wget) to
confirm 200 OK, and then commit the corrected URLs so the build can fetch
artifacts successfully.
🧹 Nitpick comments (7)
src/main/deploy/pathplanner/paths/Depot to Hub.path (1)
14-15: Consider usinglinkedNamefor shared waypoints across paths.The AI summary notes several paths share endpoints like "Hub" and "Depot" (e.g.,
Hub to Depot.path,Hub to Climb.path,Outpost to Hub.path). SettinglinkedNameon shared waypoints ensures that if you adjust a location in one path, PathPlanner propagates the change to all paths referencing it — reducing the risk of coordinate drift between related paths.Also applies to: 27-28
src/main/java/org/team5924/frc2026/commands/AutoScoreCommands.java (1)
25-39: LGTM — utility class pattern is appropriate here.Private constructor correctly prevents instantiation. The commented-out plan on line 26 outlines a reasonable future signature.
The TODO on line 26 outlines a fuller
autoScoresignature returningCommandwith driver inputs, rumble, and alignment overrides. Would you like me to open an issue to track this, or generate a skeleton implementation wired intoDriveToPoseandSuperShooter?vendordeps/Phoenix6-26.1.1.json (1)
1-449: Validate JSON structure.The JSON structure appears well-formed, but it's good practice to validate it programmatically to catch any subtle syntax issues.
#!/bin/bash # Description: Validate JSON structure of the vendordep file echo "Validating JSON structure..." if jq empty vendordeps/Phoenix6-26.1.1.json 2>/dev/null; then echo "✓ JSON is valid" jq '.' vendordeps/Phoenix6-26.1.1.json > /dev/null else echo "✗ JSON validation failed" exit 1 fisrc/main/java/org/team5924/frc2026/Constants.java (1)
53-57: Field and method share the namedisableHAL— consider disambiguating.Java allows a field and method to share a name, so this compiles. However, it harms readability — at a call site,
Constants.disableHAL(the field) andConstants.disableHAL()(the method) differ by only parentheses, which is easy to mis-read or mis-type. A small rename avoids the ambiguity.♻️ Suggested rename
- public static boolean disableHAL = false; + public static boolean halDisabled = false; public static void disableHAL() { - disableHAL = true; + halDisabled = true; }Then update the read-site in
AllianceFlipUtilaccordingly.src/main/java/org/team5924/frc2026/RobotState.java (1)
42-42:gyroOffsetshould beprivate.The field has default (package-private) access, exposing it to other classes in the package. Since it's an internal detail of the pose-reset math, it should be
private.♻️ Proposed fix
- Rotation2d gyroOffset = Rotation2d.kZero; + private Rotation2d gyroOffset = Rotation2d.kZero;src/main/java/org/team5924/frc2026/commands/drive/DriveToPose.java (1)
39-45: Consider usingLoggedTunableNumberfor PID gains and tolerances.Hardcoded PID gains and tolerances make tuning inconvenient during development and testing. Using
LoggedTunableNumber(consistent with how the rest of this codebase tunes values at runtime) would make iterating on these values much easier.src/main/deploy/pathplanner/autos/Example Auto - Left.auto (1)
6-49: Link shared waypoints between consecutive paths to ensure continuous transitions.All 7 referenced paths exist. However, endpoint continuity verification reveals coordinate mismatches between several sequential paths that could cause minor position drift during autonomous execution:
- "Left to Outpost" → "Outpost to Hub": Δ (0.025, 0.005)
- "Depot to Hub" → "Hub to Neutral": Δ (0.022, 0.022)
- "Hub to Neutral" → "Neutral to Hub": Δ (0.095, 0.000) — largest discrepancy
- "Neutral to Hub" → "Hub to Climb": Δ (0.014, 0.058)
Use
linkedNameto link the shared waypoints between these consecutive paths. For example, if "Left to Outpost" and "Outpost to Hub" share an "Outpost" waypoint, setlinkedName: "Outpost"on the final waypoint of the first path and the initial waypoint of the second path.
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/drive/DriveToPose.java`:
- Around line 38-41: The drivePID and thetaPID can produce unbounded outputs;
clamp their outputs to safe physical limits instead of relying on integrator
bounds — in the DriveToPose class, add constants (e.g., MAX_DRIVE_SPEED,
MAX_TURN_RATE) and in execute() after computing driveSpeed =
drivePID.calculate(...) and turnSpeed = thetaPID.calculate(...), clamp those
values to the max/min limits (use MathUtil.clamp or equivalent) before
constructing the ChassisSpeeds/issuing motor commands so neither drivePID nor
thetaPID can command unsafe velocities.
In `@src/main/java/org/team5924/frc2026/RobotState.java`:
- Around line 50-58: The estimatedPose in RobotState is only set in resetPose
and never refreshed; update RobotState from the pose estimator each loop by
adding a call in Drive.periodic() after the SwerveDrivePoseEstimator is updated
to call RobotState.getInstance().setEstimatedPose(...) using the estimator's
current pose (e.g., poseEstimator.getEstimatedPosition()/getEstimatedPose
method), so RobotState.estimatedPose stays in sync with the
SwerveDrivePoseEstimator for autonomous commands.
🧹 Nitpick comments (7)
src/main/java/org/team5924/frc2026/util/AllianceFlipUtil.java (3)
27-33: Extract field dimension constants for readability and reuse.The inch values
651.2and317.7appear as magic numbers. Extracting them as named constants (or their meter equivalents) improves clarity and makes it easy to update if the field spec changes.♻️ Suggested refactor
public class AllianceFlipUtil { + private static final double FIELD_LENGTH_METERS = Units.inchesToMeters(651.2); + private static final double FIELD_WIDTH_METERS = Units.inchesToMeters(317.7); + public static double applyX(double x) { - return shouldFlip() ? Units.inchesToMeters(651.2) - x : x; + return shouldFlip() ? FIELD_LENGTH_METERS - x : x; } public static double applyY(double y) { - return shouldFlip() ? Units.inchesToMeters(317.7) - y : y; + return shouldFlip() ? FIELD_WIDTH_METERS - y : y; }
49-53:DriverStation.getAlliance()called twice — store in a local variable.
shouldFlip()callsDriverStation.getAlliance()twice (.isPresent()then.get()). This is a minor redundancy and a theoretical TOCTOU gap. Using a localOptionalis cleaner.♻️ Suggested refactor
public static boolean shouldFlip() { - return !Constants.disableHAL - && DriverStation.getAlliance().isPresent() - && DriverStation.getAlliance().get() == DriverStation.Alliance.Red; + if (Constants.disableHAL) return false; + var alliance = DriverStation.getAlliance(); + return alliance.isPresent() && alliance.get() == DriverStation.Alliance.Red; }
43-47: RedundantshouldFlip()calls when flipping aPose2d.When
apply(Pose2d)determines that flipping is needed and then callsapply(Translation2d)→applyX/applyYandapply(Rotation2d), each of those re-evaluatesshouldFlip()(up to 4 extra calls involvingDriverStationqueries). This works correctly today, but if you refactorshouldFlip()per the above suggestion, consider also adding a private overload that accepts a pre-computed boolean to avoid the redundancy.src/main/java/org/team5924/frc2026/RobotState.java (2)
42-42:gyroOffsetis package-private — consider making itprivate.Every other mutable field in
RobotStateisprivate.gyroOffsetlacks an access modifier, making it package-private and exposing it to direct mutation from any class in the same package. Since it's only mutated internally byresetPose, marking itprivatewould be more consistent and safer.♻️ Suggested refactor
- Rotation2d gyroOffset = Rotation2d.kZero; + private Rotation2d gyroOffset = Rotation2d.kZero;
48-50: Minor inconsistency:new Pose2d()vsPose2d.kZero.
odometryPoseis initialized withnew Pose2d()whileestimatedPoseusesPose2d.kZero. They're semantically identical, but using the same style keeps things consistent.src/main/java/org/team5924/frc2026/commands/drive/DriveToPose.java (2)
33-33: Defaultrobotsupplier is eagerly bound then overwritten in chained constructors.The field initializer
RobotState.getInstance()::getEstimatedPosecaptures the singleton immediately, but constructors 2 and 3 replace it. This works, but the unnecessary eager initialization obscures intent. Consider deferring the default assignment into the primary constructor body to make the contract clearer.♻️ Suggested refactor
- private Supplier<Pose2d> robot = RobotState.getInstance()::getEstimatedPose; + private Supplier<Pose2d> robot; // ... public DriveToPose(Drive drive, Supplier<Pose2d> target) { this.drive = drive; this.target = target; + this.robot = RobotState.getInstance()::getEstimatedPose; addRequirements(drive); thetaPID.enableContinuousInput(-Math.PI, Math.PI); }
111-121: Consider adding a timeout safeguard for autonomous use.
isFinished()only returnstruewhen both tolerances are met. If the robot is blocked, stalled, or the target is unreachable, this command runs indefinitely — which is particularly dangerous during the 15-second autonomous period where every second counts. While the caller can chain.withTimeout(), it's easy to forget. You could document this expectation or add a default maximum duration.
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/climb/AutoClimbCommands.java`:
- Around line 27-30: The autoClimb method in AutoClimbCommands currently throws
an UnsupportedOperationException with the wrong message and has incorrect
indentation; update the thrown exception in AutoClimbCommands.autoClimb to read
"Unimplemented method 'autoClimb'" (matching the method name) and re-indent the
method body to be one level inside the class (same style as AutoScoreCommands)
so the method block aligns with other class members.
In `@src/main/java/org/team5924/frc2026/commands/shooter/AutoScoreCommands.java`:
- Around line 43-46: The autoScore method in class AutoScoreCommands is
misaligned at column 0; re-indent the entire method declaration and body (public
static Command autoScore(Drive drive, SuperShooter shooter, BooleanSupplier s) {
... }) to match the existing class member indentation level used for other
methods/fields in AutoScoreCommands so the method sits within the class body
consistently.
🧹 Nitpick comments (2)
src/main/java/org/team5924/frc2026/commands/shooter/AutoScoreCommands.java (1)
28-40: TODO and commented-out scaffold noted.The commented-out signature and TODO are fine for a scaffold PR. Happy to help flesh out the implementation when you're ready.
src/main/java/org/team5924/frc2026/commands/climb/AutoClimbCommands.java (1)
3-7: Imports appear to be copied fromAutoScoreCommandswithout cleanup.
SuperShooterandDoubleSupplierare not relevant to a climb command class. These should be removed (or replaced with the actual climb subsystem import when the implementation is ready) to avoid confusion about this file's dependencies.Proposed fix
import java.util.function.BooleanSupplier; -import java.util.function.DoubleSupplier; import org.team5924.frc2026.subsystems.drive.Drive; -import org.team5924.frc2026.subsystems.superShooter.SuperShooter;
…the first two basic autos
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/main/deploy/pathplanner/autos/Gather`, Shoot, + Climb Auto.auto:
- Around line 31-48: The auto sequence misses a shoot action after returning to
the hub: after the path step with pathName "Outpost to Hub" add a shooter step
(e.g., a "Run Shooter" or "named" action that triggers the shooter) before the
next path step with pathName "Hub to Climb" so the gathered piece is scored;
locate the sequence entries around the "Outpost to Hub" and "Hub to Climb" path
steps and insert the shooter command before the existing "Run L1 Climb" named
action.
In `@src/main/java/org/team5924/frc2026/commands/shooter/AutoScoreCommands.java`:
- Around line 39-42: autoScore currently throws UnsupportedOperationException
and will crash when invoked (e.g., by the "Run Shooter" command in
RobotContainer); replace the exception with a no-op command return to avoid
runtime crashes until the real implementation is ready. Specifically, in the
autoScore(Drive drive, SuperShooter shooter) method remove the throw and return
Commands.none() (or another appropriate no-op from wpilib) and add the import
for edu.wpi.first.wpilibj2.command.Commands so the code compiles and calling
"Run Shooter" is safe.
In `@src/main/java/org/team5924/frc2026/RobotContainer.java`:
- Around line 152-158: The registered named command currently calls
Commands.runOnce which sets shooter.setGoalState(ShooterState.AUTO_SHOOTING) and
then calls AutoScoreCommands.autoScore(drive, shooter) but discards its returned
Command; instead build and register a composite Command that first sets the
shooter state and then schedules/executes the Command returned by
AutoScoreCommands.autoScore(drive, shooter) (e.g. use Commands.sequence or
.andThen to chain the runOnce that calls shooter.setGoalState(...) with
AutoScoreCommands.autoScore(drive, shooter)) so the auto-score Command is
returned to NamedCommands.registerCommand and actually runs.
- Around line 148-172: The auto chooser is built before the named commands are
registered, so PathPlanner will not resolve "Run Shooter", "Run L1 Climb", and
"Run Intake"; move all NamedCommands.registerCommand(...) calls (the three
registerCommand blocks) to occur before the autoChooser initialization (the call
to AutoBuilder.buildAutoChooser() that assigns autoChooser) so the commands are
registered prior to AutoBuilder.buildAutoChooser() loading/parsing .auto files.
- Around line 118-122: Create a new IntakeIOSim class that extends
GenericRollerSystemIOSim and implements IntakeIO (mirroring ShooterRollerIOSim),
implement any required IO methods/constructors to simulate roller behavior, and
replace the no-op anonymous IntakeIO in RobotContainer (the new Intake(new
IntakeIO() {})) with new Intake(new IntakeIOSim()) so the Intake subsystem uses
the simulator; ensure class name IntakeIOSim and constructor match other *IOSim
patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/team5924/frc2026/commands/drive/DriveToPose.java`:
- Around line 40-48: The theta PID tunables (thetaKp/thetaKi/thetaKd) are
registered with the same dashboard keys as driveKp/driveKi/driveKd causing
entangled gains, and the PIDController instances (drivePID/thetaPID) only read
values at construction so dashboard changes are ignored; fix by giving theta*
tunables unique keys (e.g., "Drive/Theta/Kp" etc.) and update the PIDController
gains at runtime (e.g., in DriveToPose.execute()) using
LoggedTunableNumber.ifChanged with a callback that calls drivePID.setPID(...)
and thetaPID.setPID(...); reference the LoggedTunableNumber fields
(driveKp/driveKi/driveKd, thetaKp/thetaKi/thetaKd), the PIDController fields
(drivePID/thetaPID), and update point (execute()).
🧹 Nitpick comments (4)
src/main/java/org/team5924/frc2026/commands/AutoBuilder.java (1)
35-38: Unused fields:shooterandintake.These fields are injected but never referenced. The PR description notes that autonomous scoring is not yet implemented — if that's the plan, a brief
// TODOon each would clarify intent and prevent accidental removal.src/main/java/org/team5924/frc2026/util/AllianceFlipUtil.java (3)
26-31: Field and method share the namedisableHAL— confusing API.The
public static boolean disableHALfield and thepublic static void disableHAL()method have the same name. This compiles but is confusing for callers:AllianceFlipUtil.disableHAL(field) vsAllianceFlipUtil.disableHAL()(method). Also, the field ispublicand directly writable, bypassing the setter.Consider renaming one or making the field private:
Suggested fix
- public static boolean disableHAL = false; + private static boolean halDisabled = false; public static void disableHAL() { - disableHAL = true; + halDisabled = true; } ... public static boolean shouldFlip() { - return !disableHAL + return !halDisabled && DriverStation.getAlliance().isPresent() && DriverStation.getAlliance().get() == DriverStation.Alliance.Red; }
33-39: Extract field-dimension constants.
651.2and317.7(inches) are magic numbers representing the 2026 field dimensions. Extract them into named constants for clarity and single-point-of-change.Suggested refactor
public class AllianceFlipUtil { + private static final double FIELD_LENGTH_METERS = Units.inchesToMeters(651.2); + private static final double FIELD_WIDTH_METERS = Units.inchesToMeters(317.7); + ... public static double applyX(double x) { - return shouldFlip() ? Units.inchesToMeters(651.2) - x : x; + return shouldFlip() ? FIELD_LENGTH_METERS - x : x; } public static double applyY(double y) { - return shouldFlip() ? Units.inchesToMeters(317.7) - y : y; + return shouldFlip() ? FIELD_WIDTH_METERS - y : y; }
24-24: Unused import:Constants.
org.team5924.frc2026.Constantsis imported but not referenced anywhere in this file.
2 participants
Im just trying to get feedback from coderabbit but I still need to make the auto score mechanism.