86 drive heading#87
Conversation
WalkthroughAdds PID-based heading snap and coral-station lock/unlock commands, updates RobotContainer bindings to use those commands and a slow-mode constant, renames CoralInAndOut package to lowercase, and refreshes build metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Driver
participant RobotContainer
participant Drive
participant PID as PIDController
Driver->>RobotContainer: press right trigger
RobotContainer->>Drive: lockOnCoralStation(right=true)
Drive->>Drive: setDesiredHeading(targetHeading)
Drive->>Drive: setSnapToHeading(true)
loop snap-to-heading enabled
Drive->>PID: compute heading error
PID-->>Drive: angular correction (omega)
Drive->>Drive: updateSpeedsWithDesiredHeading(apply omega)
end
Driver->>RobotContainer: press/release left trigger
RobotContainer->>Drive: unlockRotation()
Drive->>Drive: setSnapToHeading(false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 2
🧹 Nitpick comments (5)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (2)
147-161: Public snap state API is clear; consider using Rotation2d for type safety.Optional: make desiredHeading a Rotation2d and expose getters to avoid rad/deg mix-ups.
410-413: Snap overrides driver omega; confirm intended behavior.With snapToHeading true, joystick rotation is ignored. If you want manual override within tolerance, gate the override with joystick magnitude.
src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java (3)
75-80: Duplicate alliance logic; reuse helper consistently.You already provide isFlipped(). Replace local re-computations elsewhere (e.g., joystickDrive, turnTo* helpers) to reduce drift.
86-96: getCoralStationRotation OK; consider returning Rotation2d.Returning a Rotation2d avoids sign/radian mistakes and reads better at call sites.
359-414: Legacy turnToLeft/Right duplicate snapping math.Now that getCoralStationRotation exists and Drive handles snapping, consider removing or delegating these to reduce duplication.
Also applies to: 416-469
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
vendordeps/AdvantageKit.jsonis excluded by none and included by nonevendordeps/PathplannerLib-2025.2.7.jsonis excluded by none and included by nonevendordeps/Phoenix6-frc2025-latest.jsonis excluded by none and included by none
📒 Files selected for processing (13)
src/main/java/org/team5924/frc2025/BuildConstants.java(1 hunks)src/main/java/org/team5924/frc2025/Constants.java(1 hunks)src/main/java/org/team5924/frc2025/RobotContainer.java(3 hunks)src/main/java/org/team5924/frc2025/RobotState.java(1 hunks)src/main/java/org/team5924/frc2025/commands/coralInAndOut/RunIntake.java(1 hunks)src/main/java/org/team5924/frc2025/commands/coralInAndOut/RunShooter.java(1 hunks)src/main/java/org/team5924/frc2025/commands/coralInAndOut/TeleopShoot.java(1 hunks)src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java(3 hunks)src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOut.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIO.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOKrakenFOC.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOSim.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java (1)
src/main/java/org/team5924/frc2025/Constants.java (1)
Constants(42-284)
src/main/java/org/team5924/frc2025/RobotContainer.java (4)
src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOKrakenFOC.java (1)
CoralInAndOutIOKrakenFOC(27-119)src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOSim.java (1)
CoralInAndOutIOSim(23-31)src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java (1)
DriveCommands(47-470)src/main/java/org/team5924/frc2025/Constants.java (1)
Constants(42-284)
🔇 Additional comments (13)
src/main/java/org/team5924/frc2025/BuildConstants.java (1)
24-30: LGTM! Auto-generated build metadata.These changes are automatically generated build constants that reflect the current repository state, branch name, and build timestamp. The updates are expected and appropriate for a build constants file.
src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOSim.java (1)
17-17: Package rename follows Java naming conventions.The package rename from
CoralInAndOuttocoralInAndOutcorrectly follows Java conventions where package names should be lowercase.src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIO.java (1)
17-17: Package rename follows Java naming conventions.Consistent with the broader refactoring to use lowercase package names per Java conventions.
src/main/java/org/team5924/frc2025/RobotState.java (1)
28-28: Import updated to match package rename.The import correctly reflects the new lowercase package structure.
src/main/java/org/team5924/frc2025/Constants.java (1)
59-59: New slow mode constant looks good.The
SLOW_MODE_MULTIconstant is well-named, appropriately typed, and the value of 0.5 is reasonable for slow mode operation (50% speed reduction).src/main/java/org/team5924/frc2025/commands/coralInAndOut/RunShooter.java (1)
21-22: Imports updated to match package rename.Both import statements correctly reference the new lowercase package structure.
src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOut.java (1)
17-17: Package rename applied consistently.The core subsystem class correctly uses the new lowercase package name per Java conventions.
src/main/java/org/team5924/frc2025/commands/coralInAndOut/RunIntake.java (1)
21-22: Imports updated to match package rename.Import statements correctly reference the new lowercase package structure.
src/main/java/org/team5924/frc2025/commands/coralInAndOut/TeleopShoot.java (1)
23-24: Imports updated to match package rename.Import statements correctly reference the new lowercase package structure, completing the consistent refactoring across all affected files.
src/main/java/org/team5924/frc2025/subsystems/rollers/CoralInAndOut/CoralInAndOutIOKrakenFOC.java (1)
17-17: Package path rename looks good; verify folder casing on disk.Ensure the directory is actually renamed to rollers/coralInAndOut to match the new package on case-sensitive filesystems and CI agents. Also confirm all imports/usages were updated across the repo.
src/main/java/org/team5924/frc2025/RobotContainer.java (3)
53-58: CoralInAndOut import path rename LGTM.Matches the package change; build should resolve.
220-229: Slow mode uses Constants.SLOW_MODE_MULTI = 0.5 — good centralization.Change is clear and consistent across axes.
266-274: Trigger orchestration depends on lock/unlock being one-shot.Given current DriveCommands.lockOnCoralStation/unlockRotation use run(), locking persists after release. After switching to runOnce (see suggested diff), this binding set works as intended: press to lock, release both triggers to unlock.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (1)
147-151: Make fields private and add proper documentation.The heading-related fields should be private to encapsulate internal state. Additionally,
headingPidshould be final since it's never reassigned, and all fields should have javadoc comments explaining their purpose.Apply this diff:
- // in radians - private double desiredHeading = 0.0; - private boolean snapToHeading = false; - - PIDController headingPid = new PIDController(3, 0, 0); + /** Desired heading in radians for snap-to-heading control. */ + private double desiredHeading = 0.0; + + /** Whether snap-to-heading control is currently enabled. */ + private boolean snapToHeading = false; + + /** PID controller for heading correction with continuous input enabled. */ + private final PIDController headingPid = new PIDController(3, 0, 0);
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (1)
153-163: Add javadoc and consider normalizing the heading input.These public methods lack documentation and
setDesiredHeadingdoesn't validate or normalize the input angle, which could lead to unexpected behavior if values outside the [-π, π] range are passed.Apply this diff:
+ /** Toggles snap-to-heading control on/off. */ public boolean toggleSnapToHeading() { return snapToHeading = !snapToHeading; } + /** + * Enables or disables snap-to-heading control. + * @param snap true to enable, false to disable + */ public void setSnapToHeading(boolean snap) { snapToHeading = snap; } + /** + * Sets the desired heading for snap-to-heading control. + * @param heading the target heading in radians + */ public void setDesiredHeading(double heading) { - desiredHeading = heading; + desiredHeading = MathUtil.angleModulus(heading); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/team5924/frc2025/BuildConstants.java(1 hunks)src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/team5924/frc2025/commands/drive/DriveCommands.java
- src/main/java/org/team5924/frc2025/BuildConstants.java
🔇 Additional comments (3)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (3)
34-34: LGTM!The imports are necessary for the heading snapping implementation.
Also applies to: 36-36
252-252: LGTM!Continuous input is correctly enabled for the heading PID controller, which properly handles angle wrap-around at ±π.
415-417: LGTM!The heading snapping integration is correctly implemented with a conditional check to only apply it when enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/team5924/frc2025/BuildConstants.java (1)
24-29: Consider excluding auto-generated build files from version control.This file is explicitly marked as automatically generated (Line 19) and contains build metadata that changes with every build. Committing it creates noisy diffs, frequent merge conflicts, and makes code review harder. Additionally,
DIRTY = 1(Line 30) indicates uncommitted changes, which shouldn't be reflected in a committed build file.Add this file to
.gitignore:+# Auto-generated build constants +src/main/java/org/team5924/frc2025/BuildConstants.javaThen remove it from the repository:
git rm --cached src/main/java/org/team5924/frc2025/BuildConstants.javaThe build system will regenerate this file automatically when needed, ensuring each developer and CI build has accurate metadata without polluting version control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/team5924/frc2025/BuildConstants.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (1)
384-402: Fix critical PID usage error and extract magic number.Some issues from the previous review have been addressed (passing raw angle to PID, consistent object creation), but two remain:
Line 399: Calling
headingPid.close()every cycle is incorrect. Theclose()method is for resource cleanup and should only be called once when the controller is no longer needed. Calling it repeatedly will cause the PID controller to malfunction.Line 388: The tolerance value
Math.toRadians(3.0)is a magic number and should be a named constant.Apply this diff:
+ private static final double HEADING_TOLERANCE_RAD = Math.toRadians(3.0); // ±3 degrees + /** * rotates the speeds towards the desired heading with a ±3 degree tolerance * * @param speeds input speeds that will be updated * @param targetHeading the desired heading (rotation) * @return updated speeds */ public ChassisSpeeds updateSpeedsWithDesiredHeading(ChassisSpeeds speeds, double targetHeading) { - // tolerance of ±3 deg double currentHeading = getRotation().getRadians(); double error = MathUtil.angleModulus(targetHeading - currentHeading); - boolean isWithinTolerance = Math.abs(error) <= Math.toRadians(3.0); + boolean isWithinTolerance = Math.abs(error) <= HEADING_TOLERANCE_RAD; if (isWithinTolerance) return new ChassisSpeeds( speeds.vxMetersPerSecond, speeds.vyMetersPerSecond, 0.0); // within tolerance; don't rotate - // calculate omega double omega = headingPid.calculate(currentHeading, targetHeading); omega = MathUtil.clamp(omega, -getMaxAngularSpeedRadPerSec(), getMaxAngularSpeedRadPerSec()); - headingPid.close(); return new ChassisSpeeds(speeds.vxMetersPerSecond, speeds.vyMetersPerSecond, omega); }
🧹 Nitpick comments (3)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (3)
147-149: Make fields private.These fields should be
privateinstead of package-private for better encapsulation.Apply this diff:
- // in radians - private double desiredHeading = 0.0; - private boolean snapToHeading = false; + // in radians + private double desiredHeading = 0.0; + private boolean snapToHeading = false;
151-151: Extract PID gains to named constants.The PID gains
(3, 0, 0)are magic numbers and should be extracted to named constants for maintainability and tuning.Apply this diff:
+ private static final double HEADING_KP = 3.0; + private static final double HEADING_KI = 0.0; + private static final double HEADING_KD = 0.0; + - PIDController headingPid = new PIDController(3, 0, 0); + private PIDController headingPid = new PIDController(HEADING_KP, HEADING_KI, HEADING_KD);
161-163: Consider adding validation for heading parameter.The method accepts any double value without validation. Consider adding a check or documentation about expected range (e.g., radians in [-π, π]).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (3)
src/main/java/org/team5924/frc2025/subsystems/drive/Drive.java (3)
34-36: LGTM!The imports are appropriate for the heading snap functionality.
252-252: LGTM!Enabling continuous input for the heading PID controller is correct for angle control.
412-414: LGTM!The integration of heading snap into
runVelocityis clean and correct.
2 participants
drive heading, slow mode multiplier updated to 0.5
Summary by CodeRabbit
New Features
Refactor