Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds PathPlanner vendor dependency and assets, introduces a new CommandSwerveDrivetrain subsystem with simulation, auto, and vision integration, updates Robot and RobotContainer for logging and control bindings, and adds driver station joystick mappings. No existing APIs are modified; new public class and field are introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Driver as Driver (Joystick)
participant RC as RobotContainer
participant CDS as CommandSwerveDrivetrain
participant PP as PathPlanner AutoBuilder
rect rgb(240,248,255)
note over RC: Initialization
RC->>CDS: create drivetrain (constructors)
RC->>PP: configureAutoBuilder(...)
end
alt Teleop enabled
Driver->>RC: joystick axes/buttons
RC->>CDS: applyRequest(SwerveRequest.Drive(...))
CDS->>CDS: periodic() update & operator perspective
CDS->>Swerve: set module states
else Disabled
RC->>CDS: Idle (brake/neutral per binding)
end
opt Vision updates
Note over CDS: addVisionMeasurement(...)
CDS->>CDS: fuse pose (odometry + vision)
end
rect rgb(245,255,245)
note over RC: Autonomous
RC->>PP: getSelectedAuto()
PP-->>RC: Command (trajectory follower)
RC->>CDS: execute generated commands
CDS->>Swerve: follow path
end
sequenceDiagram
autonumber
participant Robot as Robot
participant DLM as DataLogManager
participant Epi as Epilogue
Robot->>DLM: start()
Robot->>Epi: bind(this)
Note over Robot: @Logged annotation active
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/main/java/frc/robot/RobotContainer.java (2)
40-40: Encapsulate drivetrain fieldExpose via a getter instead of a public field to avoid leaking implementation details.
- public final CommandSwerveDrivetrain drivetrain = TunerConstants.createDrivetrain(); + private final CommandSwerveDrivetrain drivetrain = TunerConstants.createDrivetrain();Add a getter (outside this hunk):
public CommandSwerveDrivetrain getDrivetrain() { return drivetrain; }
79-82: Handle null autonomous selectionautoChooser.getSelected() can return null. Ensure caller handles this gracefully or provide a safe fallback command.
src/main/java/frc/robot/subsystems/CommandSwerveDrivetrain.java (5)
121-145: AutoBuilder wiring LGTM; consider externalizing gainsConfiguration and feedforward hooks look correct. Consider moving the PID gains (10/0/0 and 7/0/0) to constants or config for easier tuning without code changes.
145-148: Improve error reporting contentInclude the exception message to aid troubleshooting.
- DriverStation.reportError( - "Failed to load PathPlanner config and configure AutoBuilder", ex.getStackTrace()); + DriverStation.reportError( + "Failed to load PathPlanner config and configure AutoBuilder: " + ex.getMessage(), + ex.getStackTrace());
183-199: Notifier lifecycle: optional cleanupConsider closing m_simNotifier when shutting down to prevent background thread leaks in long-running sims/tests.
Add:
public void close() { if (m_simNotifier != null) { m_simNotifier.close(); m_simNotifier = null; } }
207-210: Clarify timestamp timebase for visionYou convert FPGA timestamps to current time. Ensure callers pass FPGA time; otherwise this double-converts and skews timestamps. Add Javadoc note stating the expected timebase (FPGA).
Also applies to: 225-232
238-244: Defensive copies for array gettersReturn copies to prevent external mutation of internal arrays.
- public SwerveModuleState[] getModuleStates() { - return getState().ModuleStates; - } + public SwerveModuleState[] getModuleStates() { + return getState().ModuleStates.clone(); + } - public SwerveModuleState[] getModuleTargets() { - return getState().ModuleTargets; - } + public SwerveModuleState[] getModuleTargets() { + return getState().ModuleTargets.clone(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/java/frc/robot/generated/TunerConstants.javais excluded by!**/generated/**
📒 Files selected for processing (8)
simgui-ds.json(1 hunks)src/main/deploy/pathplanner/navgrid.json(1 hunks)src/main/deploy/pathplanner/paths/Example Path.path(1 hunks)src/main/deploy/pathplanner/settings.json(1 hunks)src/main/java/frc/robot/Robot.java(1 hunks)src/main/java/frc/robot/RobotContainer.java(1 hunks)src/main/java/frc/robot/subsystems/CommandSwerveDrivetrain.java(1 hunks)vendordeps/PathplannerLib-2025.2.7.json(1 hunks)
🔇 Additional comments (4)
src/main/deploy/pathplanner/paths/Example Path.path (1)
35-42: Verify constraints sourceuseDefaultConstraints is true, so globalConstraints are ignored. Confirm this is intended and that settings.json has the desired defaults.
Also applies to: 53-54
src/main/java/frc/robot/RobotContainer.java (1)
55-77: Drive bindings look correctAxis directions and deadbands align with WPILib conventions; idle-on-disable is a good touch.
src/main/java/frc/robot/subsystems/CommandSwerveDrivetrain.java (2)
157-159: applyRequest helper is cleanSimple and idiomatic; no issues.
170-180: Alliance perspective application is robustApplies once or on disable; avoids mid-match flips. Good pattern.
| autoChooser = AutoBuilder.buildAutoChooser("Tests"); | ||
| SmartDashboard.putData("Auto Mode", autoChooser); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Default auto likely mismatched
The chooser uses "Tests" as default. The repo adds "Example Path.path". Align the default to an existing auto to avoid null selection.
- autoChooser = AutoBuilder.buildAutoChooser("Tests");
+ autoChooser = AutoBuilder.buildAutoChooser("Example Path");🏁 Script executed:
#!/bin/bash
# Locate AutoBuilder file
rg --files-with-matches 'class AutoBuilder' -t java
# Show buildAutoChooser signature and surrounding lines
rg -nP --type=java -C5 'buildAutoChooser\('
# Search for 'Tests' literal in Java sources
rg -n '"Tests"' -t java
# Search for 'Example Path' literal in Java sources
rg -n '"Example Path"' -t java
# List any .path files in the repository
find . -type f -name "*.path"Length of output: 965
Align default auto chooser to an existing path
Replace "Tests" with "Example Path" since only Example Path.path is present:
- autoChooser = AutoBuilder.buildAutoChooser("Tests");
+ autoChooser = AutoBuilder.buildAutoChooser("Example Path");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| autoChooser = AutoBuilder.buildAutoChooser("Tests"); | |
| SmartDashboard.putData("Auto Mode", autoChooser); | |
| autoChooser = AutoBuilder.buildAutoChooser("Example Path"); | |
| SmartDashboard.putData("Auto Mode", autoChooser); |
🤖 Prompt for AI Agents
In src/main/java/frc/robot/RobotContainer.java around lines 46 to 48, the auto
chooser default string "Tests" does not match any existing path; change the
argument passed to AutoBuilder.buildAutoChooser from "Tests" to "Example Path"
so the chooser defaults to the existing Example Path.path, then keep putting the
chooser to SmartDashboard as before.
JosephTLockwood
force-pushed
the
2-Logging
branch
from
e1c58c5 to
066dbd5
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores