Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughUpdates build metadata, adds intake motor subsystem with button-controlled voltage output, modifies swerve drive encoder offset calculations and motor inversion configuration, introduces AngularVelocity type for turn velocity measurement, and refactors button bindings to include gyro reset functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR implements prototype board-specific configurations and refactoring for swerve drive modules. The changes include hardware calibration values, code quality improvements, and a prototype intake motor integration for testing.
Changes:
- Refactored position wrapping logic in OptixSpark to use WPILib's MathUtil.inputModulus
- Updated swerve module configuration with hardware-specific calibration offsets and motor inversions
- Changed turnVelocityRadPerSec type from double to AngularVelocity for better type safety
- Added prototype intake motor integration with button bindings for testing
- Adjusted encoder sync interval and tunables path structure
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| OptixSpark.java | Simplified position wrapping logic using MathUtil.inputModulus, removed min==max validation, added explicit this qualifiers |
| MiscUtils.java | Added normalize() utility method for Rotation2d normalization |
| SwerveModuleSim.java | Updated to use typed AngularVelocity for turn velocity instead of raw doubles |
| SwerveModuleSpark.java | Added position wrapping configuration, turn motor inversion support, refactored encoder initialization, and type-safe velocity handling |
| SwerveModuleIO.java | Changed turnVelocityRadPerSec from double to AngularVelocity type |
| SwerveModule.java | Updated tunable parameter paths with leading slash, changed offset override default to true |
| Swerve.java | Reordered periodic() method to call gyro update before odometry, reduced encoder sync interval to 7 seconds |
| SwerveConfig.java | Updated CANCODER_OFFSET values for prototype hardware, added TURN_INVERTED configuration array |
| ButtonBindings.java | Added intake motor control bindings and Runnable overload for switchCommands method |
| Robot.java | Initialized prototype intake motor as static field for testing |
| BuildConstants.java | Updated build metadata |
| simgui-ds.json | Changed joystick GUID configuration |
Comments suppressed due to low confidence (1)
Template/src/main/java/frc/robot/utils/OptixSpark.java:352
- The removal of the min==max check creates a potential issue. If min and max are equal, this will set wrapMin and wrapMax to the same value, which could cause division by zero or incorrect behavior in the positionError method (line 478) where errorBound is calculated as (wrapMax - wrapMin) / 2.0. Consider adding validation to handle or reject the case where min equals max.
double a = Math.min(min, max);
double b = Math.max(min, max);
this.wrapEnabled = true;
this.wrapMin = a;
this.wrapMax = b;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setModuleStates(desiredStates); | ||
|
|
||
| if (getIsStopped() && (Timer.getTimestamp() - lastEncoderSyncTime) > 20.0) { | ||
| if (getIsStopped() && (Timer.getTimestamp() - lastEncoderSyncTime) > 7.0) { |
There was a problem hiding this comment.
The encoder sync interval was reduced from 20.0 seconds to 7.0 seconds. While this will cause more frequent encoder syncing, ensure this frequency is appropriate and won't cause performance issues or wear on hardware. Document the rationale for this specific timeout value.
| intakeMotor = OptixSpark.ofSparkMax(36); | ||
| intakeMotor.setInverted(true); | ||
| intakeMotor.apply(); |
There was a problem hiding this comment.
The intakeMotor is initialized as a public static field but is not managed as a proper subsystem. This bypasses WPILib's command-based framework benefits like proper requirements management and lifecycle hooks. Consider creating a proper IntakeSubsystem that extends SubsystemBase, or at minimum, ensure the motor is properly stopped in appropriate lifecycle methods (e.g., disabledInit, autonomousExit).
| Rotation2d.fromRadians(2.746), | ||
| Rotation2d.fromRadians(-1.161), | ||
| Rotation2d.fromRadians(0), | ||
| Rotation2d.fromRadians(-9.779+Math.PI), |
There was a problem hiding this comment.
The CANCODER_OFFSET value for module 0 uses the expression (-9.779+Math.PI) which evaluates to approximately -6.6374. Consider replacing this with the computed value or adding a comment explaining why this specific mathematical expression is used. Having the expression inline makes it less clear what the intended offset value is.
| Rotation2d.fromRadians(-9.779+Math.PI), | |
| // Module 0 offset: (-9.779 + Math.PI) rad ≈ -6.6374 rad | |
| Rotation2d.fromRadians(-6.6374), |
| Bind.button(ctl.b()).switchCommands(() -> { | ||
| Robot.intakeMotor.setVoltage(7.5); | ||
| }, () -> { | ||
| Robot.intakeMotor.setVoltage(0); | ||
| }); |
There was a problem hiding this comment.
The switchCommands binding implementation has a logic issue. The Commands.either() will evaluate the condition when the button is pressed, and the switchState toggle happens immediately, so the command will alternate each press. However, using runOnce() means each command only runs once when the button is pressed, not continuously. This is likely not the intended behavior for motor control. Consider using Commands.run() for continuous execution while the motor should be on, or implement a proper toggle that maintains state between button presses.
| Bind.button(ctl.b()).switchCommands(() -> { | |
| Robot.intakeMotor.setVoltage(7.5); | |
| }, () -> { | |
| Robot.intakeMotor.setVoltage(0); | |
| }); | |
| ctl.b().toggleOnTrue( | |
| Commands.startEnd( | |
| () -> Robot.intakeMotor.setVoltage(7.5), | |
| () -> Robot.intakeMotor.setVoltage(0))); |
| import frc.robot.config.RobotConfig.Input; | ||
| import frc.robot.config.RobotConfig.RobotType; | ||
| import frc.robot.utils.MiscUtils; | ||
| import frc.robot.utils.OptixSpark; |
There was a problem hiding this comment.
The import for OptixSpark is added but never used in this file. The only reference to OptixSpark is through Robot.intakeMotor, which doesn't require the import. Consider removing this unused import.
| import frc.robot.utils.OptixSpark; |
| public static Rotation2d normalize(Rotation2d rotation) { | ||
| double currentRads = rotation.getRadians(); | ||
| return Rotation2d.fromRadians(MathUtil.angleModulus(currentRads)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The new normalize method is added but never used in this PR. The method applies angleModulus to normalize a Rotation2d to the range [-π, π]. This appears to be dead code unless there's a planned future use. Consider removing it if it's not needed, or adding usage if it is intended to be used.
| public static Rotation2d normalize(Rotation2d rotation) { | |
| double currentRads = rotation.getRadians(); | |
| return Rotation2d.fromRadians(MathUtil.angleModulus(currentRads)); | |
| } |
| tunableTurnKI = new LoggedNetworkNumber("/Tuning/Swerve/Module " + name + "/Turn/kI", turnConfig.kI); | ||
| tunableTurnKD = new LoggedNetworkNumber("/Tuning/Swerve/Module " + name + "/Turn/kD", turnConfig.kD); | ||
|
|
||
| tunableOffsetOverride = new LoggedNetworkBoolean("/Tuning/Swerve/Module " + name + "/Offset Override", true); |
There was a problem hiding this comment.
The tunableOffsetOverride default value changed from false to true. This means offset override will be enabled by default, which could affect the behavior on robot startup. Ensure this change is intentional and that it won't cause unexpected behavior when the robot is initialized.
Summary by CodeRabbit
New Features
Improvements
Configuration Updates
✏️ Tip: You can customize this high-level summary in your review settings.