Improved Swerve #26
Improved Swerve #26
Conversation
|
Warning Rate limit exceeded@ShmayaR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 47 seconds before requesting another review. ⌛ 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 (2)
WalkthroughSetpoint generation and force/current-based control were removed from the swerve subsystem. Calls now convert ChassisSpeeds directly to SwerveModuleState[] and set those on modules using voltage-based velocity control. Method signatures and callers were updated accordingly. Drive motor configuration gains were adjusted, and a configuration method was made public. Changes
Sequence Diagram(s)sequenceDiagram
auton/teleop Command->>Swerve: selfRelativeDrive(targetSpeeds)
Swerve->>Swerve: kinematics: targetSpeeds → SwerveModuleState[]
Swerve->>SwerveModule: setTargetState(state) loop over modules
SwerveModule->>SwerveModule: configure VelocityVoltage request
SwerveModule->>Drive Motor: apply velocity (voltage-based)
note over Swerve,SwerveModule: Setpoint generator and force/current paths removed
sequenceDiagram
participant SysId as SysID Routine
participant Swerve as Swerve
participant Module as SwerveModule[*]
SysId->>Swerve: sysIDDrive(voltage)
Swerve->>Module: setDriveMotorTargetVoltage(voltage) loop
Module->>Drive Motor: VelocityVoltage control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (2)
66-69: Possible deadlock: unlock not guaranteed on exceptionWrap lock/unlock in try/finally to prevent a stuck lock if
updateHardware()throws.- Phoenix6SignalThread.SIGNALS_LOCK.lock(); - updateHardware(); - Phoenix6SignalThread.SIGNALS_LOCK.unlock(); + Phoenix6SignalThread.SIGNALS_LOCK.lock(); + try { + updateHardware(); + } finally { + Phoenix6SignalThread.SIGNALS_LOCK.unlock(); + }
334-345: Snapshot consistency risk between gyro and module arrays
odometryUpdatesis derived from a fresh gyro snapshot, while module arrays were captured earlier inside the lock. If the gyro has more entries than the module arrays, indexing can throw. Capture all threaded signals under the same critical section or store the gyro snapshot duringupdateHardware().Suggested fix directions:
- Fetch
odometryUpdatesYawDegreesinside the locked section together with module arrays, or- Persist a gyro snapshot during
updateHardware()and use that here.src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java (2)
112-119: Javadoc mismatch (states “meters” but returns radians)Method name and implementation return radians; fix the comment.
- /** - * Gets the position of the drive wheel in meters. We don't use a {@link Rotation2d} because this function returns distance, not rotations. - * - * @return the position of the drive wheel in meters - */ + /** + * Gets the position of the drive wheel in radians (unwrapped). + * Not using {@link Rotation2d} to avoid angle wrapping. + * + * @return drive wheel position in radians + */
206-214: Fix remote sensor ID and avoid CAN ID collision
File src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
- In
configureSteerMotor(), use the encoder’s ID for fused feedback:-steerMotor.applyConfiguration(SwerveModuleConstants.generateSteerMotorConfiguration(steerMotor.getID())); +steerMotor.applyConfiguration(SwerveModuleConstants.generateSteerMotorConfiguration(steerEncoder.getID()));
- In the constructor, assign a distinct CANcoderEncoder ID (not
moduleID + 4) so it doesn’t collide with the steer motor’s device ID.
🧹 Nitpick comments (7)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (4)
49-56: Param name misleads: using voltage while namedtargetCurrentThis now sets voltage. Rename for clarity and clamp to saturation to avoid accidental >12V.
- public void sysIDDrive(double targetCurrent) { + public void sysIDDrive(double targetVoltage) { SwerveModuleState[] a = SwerveConstants.KINEMATICS.toSwerveModuleStates(new ChassisSpeeds(0, 0, 2)); for (int i = 0; i < 4; i++) { - swerveModules[i].setDriveMotorTargetVoltage(targetCurrent); + final double clamped = + Math.max(-SwerveModuleConstants.VOLTAGE_COMPENSATION_SATURATION, + Math.min(SwerveModuleConstants.VOLTAGE_COMPENSATION_SATURATION, targetVoltage)); + swerveModules[i].setDriveMotorTargetVoltage(clamped); swerveModules[i].setTargetAngle(a[i].angle); } }Please confirm no external tooling depends on the old param name in logs/UI.
184-192: Feedforward computed but unused
targetForcesNmis calculated and then ignored. Either plumb it into the module control path (e.g., feedforward in velocity request) or remove the parameter to avoid confusion.- public void selfRelativeDriveWithoutSetpointGeneration(ChassisSpeeds targetSpeeds, DriveFeedforwards feedforwards) { + public void selfRelativeDriveWithoutSetpointGeneration(ChassisSpeeds targetSpeeds) { final SwerveModuleState[] swerveModuleStates = SwerveConstants.KINEMATICS.toSwerveModuleStates(targetSpeeds); - final double[] targetForcesNm; - if (feedforwards == null) - targetForcesNm = new double[]{0, 0, 0, 0}; - else - targetForcesNm = feedforwards.linearForcesNewtons(); setTargetModuleStates(swerveModuleStates); }If you prefer wiring feedforward, say so and I can sketch the mapping approach.
278-286: Add wheel-speed desaturation (and optionally keep the “still → stop” behavior)To avoid commanding unreachable speeds, desaturate before applying. If you still want neutral-stop, re‑enable it here.
- public void selfRelativeDrive(ChassisSpeeds targetSpeeds) { - final SwerveModuleState[] swerveModuleStates = SwerveConstants.KINEMATICS.toSwerveModuleStates(targetSpeeds); - setTargetModuleStates(swerveModuleStates); - } + public void selfRelativeDrive(ChassisSpeeds targetSpeeds) { +// if (isStill(targetSpeeds)) { stop(); return; } + final SwerveModuleState[] swerveModuleStates = SwerveConstants.KINEMATICS.toSwerveModuleStates(targetSpeeds); + edu.wpi.first.math.kinematics.SwerveDriveKinematics.desaturateWheelSpeeds( + swerveModuleStates, SwerveConstants.MAXIMUM_SPEED_METERS_PER_SECOND); + setTargetModuleStates(swerveModuleStates); + }Confirm if you intentionally removed the neutral-stop to keep modules “armed” at zero speeds.
132-138: Angle “at target” ignores velocity toleranceYou record
isAngleStillbut don’t use it. Consider requiring both within-tolerance and “still” for consistency with translation checks.- return atTargetAngle; + return atTargetAngle && isAngleStill;src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
61-61: Visibility change:generateDriveMotorConfiguration()Making this public is fine if used outside the package. If not, keep package‑private to reduce API surface.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java (2)
27-27: 1 kHz control update request is excessive
new VelocityVoltage(...).withUpdateFreqHz(1000)can create unnecessary CAN load. 100–200 Hz is typical; main loop is 50 Hz.- private final VelocityVoltage driveVelocityRequest = new VelocityVoltage(0).withUpdateFreqHz(1000); + private final VelocityVoltage driveVelocityRequest = new VelocityVoltage(0).withUpdateFreqHz(100);If you truly need >200 Hz, please share CAN bus utilization metrics.
128-133: Defensive check on threaded odometry arrays
latestOdometryDrivePositions/latestOdometrySteerPositionscan be null or shorter than the requested index on the first few cycles.- public SwerveModulePosition getOdometryPosition(int odometryUpdateIndex) { - return new SwerveModulePosition( - driveWheelRotationsToMeters(latestOdometryDrivePositions[odometryUpdateIndex]), - Rotation2d.fromRotations(latestOdometrySteerPositions[odometryUpdateIndex]) - ); - } + public SwerveModulePosition getOdometryPosition(int odometryUpdateIndex) { + if (latestOdometryDrivePositions == null || latestOdometrySteerPositions == null + || odometryUpdateIndex >= latestOdometryDrivePositions.length + || odometryUpdateIndex >= latestOdometrySteerPositions.length) { + return new SwerveModulePosition(0, getCurrentAngle()); + } + return new SwerveModulePosition( + driveWheelRotationsToMeters(latestOdometryDrivePositions[odometryUpdateIndex]), + Rotation2d.fromRotations(latestOdometrySteerPositions[odometryUpdateIndex]) + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
SwerveConstants(19-110)
🔇 Additional comments (1)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
80-86: Drive Slot0 gains (lines 80–86) need verification
- Real-robot values set kP=50, kI=0, kD=0, kS=5.25 V, kV=0 V⋅s/m, kA=0 V⋅s²/m—zero kV/kA and a high kS suggest a swap or mis-configuration.
- Verify these against your latest SysId JSON and CTRE Phoenix6 unit conventions; correct sim↔real values as needed. I can help calculate the expected kV from your max wheel speed and diameter.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
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/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
36-36: Remove unused setpoint API Delete thepreviousSetpointfield and itsresetSetpoint()method from Swerve.java, and remove the call toresetSetpoint()in MotorSubsystem.java (line 37).
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (2)
177-185: Address commented-outisStillcheck.The
isStillcheck is commented out. Either restore this logic with justification or remove the dead code to maintain clarity.If the check is no longer needed, apply this diff:
public void selfRelativeDriveWithoutSetpointGeneration(ChassisSpeeds targetSpeeds) { -// if (isStill(targetSpeeds)) { -// stop(); -// return; -// } - final SwerveModuleState[] swerveModuleStates = SwerveConstants.KINEMATICS.toSwerveModuleStates(targetSpeeds); setTargetModuleStates(swerveModuleStates); }
272-275: Address commented-outisStillcheck.The
isStillcheck is commented out here as well. Either restore this logic with justification or remove the dead code.If the check is no longer needed, apply this diff:
public void selfRelativeDrive(ChassisSpeeds targetSpeeds) { -// if (isStill(targetSpeeds) { -// stop(); -// return; -// } - final SwerveModuleState[] swerveModuleStates = SwerveConstants.KINEMATICS.toSwerveModuleStates(targetSpeeds); setTargetModuleStates(swerveModuleStates); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/CommandConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
SwerveConstants(19-110)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/commands/CommandConstants.java (1)
51-51: LGTM! Method signature update applied correctly.Both lambda invocations have been correctly updated to match the new
selfRelativeDriveWithoutSetpointGeneration(ChassisSpeeds)signature by removing the trailingnullargument.Also applies to: 57-57
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (2)
277-278: Verify performance impact of removing setpoint generation.The removal of
SwerveSetpointGeneratoris a significant architectural change that may affect acceleration limiting and drive smoothness. Ensure that:
- The robot still respects physical acceleration limits
- Module state transitions remain smooth without setpoint interpolation
- Performance testing confirms no degradation in drive quality
Consider testing the robot under various driving scenarios (quick direction changes, high-speed maneuvers, precise positioning) to validate that direct state control maintains acceptable performance characteristics.
281-284: LGTM! Simplified module state setting. The loop and new setTargetState implementation correctly handle both open- and closed-loop velocity control without explicit feedforward, so dropping the force parameter is safe.
| public void sysIDDrive(double targetCurrent) { | ||
| SwerveModuleState[] a = SwerveConstants.KINEMATICS.toSwerveModuleStates(new ChassisSpeeds(0, 0, 2)); | ||
| for (int i = 0; i < 4; i++) { | ||
| swerveModules[i].setDriveMotorTargetCurrent(targetCurrent); | ||
| swerveModules[i].setDriveMotorTargetVoltage(targetCurrent); | ||
| swerveModules[i].setTargetAngle(a[i].angle); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename parameter targetCurrent to reflect voltage-based control.
The method now uses setDriveMotorTargetVoltage, but the parameter is still named targetCurrent, which is misleading.
Apply this diff to rename the parameter:
@Override
- public void sysIDDrive(double targetCurrent) {
+ public void sysIDDrive(double targetVoltage) {
SwerveModuleState[] a = SwerveConstants.KINEMATICS.toSwerveModuleStates(new ChassisSpeeds(0, 0, 2));
for (int i = 0; i < 4; i++) {
- swerveModules[i].setDriveMotorTargetVoltage(targetCurrent);
+ swerveModules[i].setDriveMotorTargetVoltage(targetVoltage);
swerveModules[i].setTargetAngle(a[i].angle);
}
}📝 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.
| public void sysIDDrive(double targetCurrent) { | |
| SwerveModuleState[] a = SwerveConstants.KINEMATICS.toSwerveModuleStates(new ChassisSpeeds(0, 0, 2)); | |
| for (int i = 0; i < 4; i++) { | |
| swerveModules[i].setDriveMotorTargetCurrent(targetCurrent); | |
| swerveModules[i].setDriveMotorTargetVoltage(targetCurrent); | |
| swerveModules[i].setTargetAngle(a[i].angle); | |
| } | |
| } | |
| @Override | |
| public void sysIDDrive(double targetVoltage) { | |
| SwerveModuleState[] a = SwerveConstants.KINEMATICS.toSwerveModuleStates(new ChassisSpeeds(0, 0, 2)); | |
| for (int i = 0; i < 4; i++) { | |
| swerveModules[i].setDriveMotorTargetVoltage(targetVoltage); | |
| swerveModules[i].setTargetAngle(a[i].angle); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java around lines 50
to 56, the method parameter name targetCurrent is misleading because the method
calls setDriveMotorTargetVoltage; rename the parameter to targetVoltage in the
method signature and replace all usages inside the method (and update any
references/call sites in the codebase) to use targetVoltage so the name
correctly reflects voltage-based control.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
49-55: Rename parameter to reflect voltage control.This method now applies a voltage target, so the
targetCurrentname is misleading—same feedback I left earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/commands/CommandConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
SwerveConstants(19-110)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (1)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)
| private void setTargetModuleStates(SwerveModuleState[] swerveModuleStates) { | ||
| for (int i = 0; i < swerveModules.length; i++) | ||
| swerveModules[i].setTargetState(swerveModuleStates[i], targetForcesNm[i]); | ||
| swerveModules[i].setTargetState(swerveModuleStates[i]); | ||
| } |
There was a problem hiding this comment.
Desaturate wheel speeds before commanding modules.
Pulling raw states from toSwerveModuleStates can exceed the module’s max velocity whenever translation and rotation are combined, so we need to clamp them prior to issuing targets or the real robot will saturate and fall short of requested motions. Please reintroduce desaturation inside setTargetModuleStates.
- private void setTargetModuleStates(SwerveModuleState[] swerveModuleStates) {
- for (int i = 0; i < swerveModules.length; i++)
- swerveModules[i].setTargetState(swerveModuleStates[i]);
+ private void setTargetModuleStates(SwerveModuleState[] swerveModuleStates) {
+ SwerveDriveKinematics.desaturateWheelSpeeds(
+ swerveModuleStates,
+ AutonomousConstants.ROBOT_CONFIG.moduleConfig.maxDriveVelocityMPS
+ );
+ for (int i = 0; i < swerveModules.length; i++)
+ swerveModules[i].setTargetState(swerveModuleStates[i]);
}Add the corresponding import edu.wpi.first.math.kinematics.SwerveDriveKinematics;.
📝 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.
| private void setTargetModuleStates(SwerveModuleState[] swerveModuleStates) { | |
| for (int i = 0; i < swerveModules.length; i++) | |
| swerveModules[i].setTargetState(swerveModuleStates[i], targetForcesNm[i]); | |
| swerveModules[i].setTargetState(swerveModuleStates[i]); | |
| } | |
| private void setTargetModuleStates(SwerveModuleState[] swerveModuleStates) { | |
| // Clamp wheel speeds to the robot’s max before commanding | |
| SwerveDriveKinematics.desaturateWheelSpeeds( | |
| swerveModuleStates, | |
| AutonomousConstants.ROBOT_CONFIG.moduleConfig.maxDriveVelocityMPS | |
| ); | |
| for (int i = 0; i < swerveModules.length; i++) | |
| swerveModules[i].setTargetState(swerveModuleStates[i]); | |
| } |
Co-Authored-By: Nummun14 <[email protected]>
…-TRIGON/RobotCodeOffSeason2025 into voltage-based-swerve
2 participants
Summary by CodeRabbit