-
Notifications
You must be signed in to change notification settings - Fork 9
Choreo routines #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Choreo routines #199
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces Choreo autonomous routines, which is a great addition. My review focuses on ensuring the new code is robust, maintainable, and free of bugs. I've found a critical compilation error in Robot.java due to an extra brace, along with a few high-severity issues related to shared state in PID controllers and incorrect use of units in the simulation code. There are also several medium-severity suggestions to improve code quality, such as removing unused code, fixing formatting, and improving encapsulation. Please review the detailed comments for suggestions on how to address these points.
src/main/java/frc/robot/Robot.java
Outdated
| public void autonomousInit() { | ||
| Shuffleboard.startRecording(); | ||
| if (SubsystemConstants.DRIVEBASE_ENABLED && AutoLogic.getSelectedAuto() != null) { | ||
| AutoLogic.getSelectedAuto().schedule(); | ||
| } | ||
| //AutoLogic.getSelectedAuto().schedule(); | ||
|
|
||
|
|
||
| new Routines().runRoutine().schedule(); | ||
|
|
||
|
|
||
| if (subsystems.climbPivotSubsystem != null) { | ||
| subsystems.climbPivotSubsystem.moveCompleteFalse(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has a few issues:
- Critical Syntax Error: There's an extra closing brace
}on line 181 that will cause a compilation failure. - High Severity Logic Change: The
if (subsystems.climbPivotSubsystem != null)block has been nested inside the outerifblock. This is likely unintentional and changes the behavior of the autonomous initialization, asmoveCompleteFalse()will now only be called if a drivebase is enabled and an auto is selected. - Readability: The code contains commented-out lines and excessive whitespace, which hurts readability.
I've provided a suggestion that fixes all these issues by cleaning up the code, removing the extra brace, and restoring the original logic for the conditional blocks.
public void autonomousInit() {
Shuffleboard.startRecording();
if (SubsystemConstants.DRIVEBASE_ENABLED && AutoLogic.getSelectedAuto() != null) {
new Routines().runRoutine().schedule();
}
if (subsystems.climbPivotSubsystem != null) {
subsystems.climbPivotSubsystem.moveCompleteFalse();
}
}| (bottomZ + ((currentPositionRotations - minPos) / (maxPos - minPos)) * (topZ - bottomZ)); | ||
| double wristPos = dblSub.get(); | ||
| //System.out.println("Wrist pose is:" + wristPos); | ||
| m_posePublisher.set(new Pose3d(0.2, 0.0, targetZ, new Rotation3d(-90, wristPos,-0))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rotation3d constructor expects angles in radians, but it appears you are passing degrees for the roll component (-90). This will result in an incorrect rotation in the simulation. You should convert the degree values to radians using Math.toRadians(). The wristPos value is also likely in degrees and should be converted as well.
| m_posePublisher.set(new Pose3d(0.2, 0.0, targetZ, new Rotation3d(-90, wristPos,-0))); | |
| m_posePublisher.set(new Pose3d(0.2, 0.0, targetZ, new Rotation3d(Math.toRadians(-90), Math.toRadians(wristPos), 0))); |
| this.setControl(new SwerveRequest.ApplyFieldSpeeds().withSpeeds(speeds) | ||
| .withSpeeds(speeds)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public PIDController pidX = AutoAlign.AutoAlignCommand.pidX; | ||
| public PIDController pidY = AutoAlign.AutoAlignCommand.pidX; | ||
| public PIDController pidRotate = AutoAlign.AutoAlignCommand.pidRotate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of issues with these PID controllers:
pidYis incorrectly initialized withAutoAlign.AutoAlignCommand.pidXinstead ofpidY. This is a bug.- These PID controllers are being shared from
AutoAlign.AutoAlignCommandwhere they arepublic static. This is a poor design practice as it creates shared mutable state between different components (AutoAlignCommandandCommandSwerveDrivetrain), which can lead to unexpected behavior. This class should have its own private, final PID controller instances for trajectory following. The corresponding fields inAutoAlign.AutoAlignCommandshould be reverted to be non-static instance fields.
| public PIDController pidX = AutoAlign.AutoAlignCommand.pidX; | |
| public PIDController pidY = AutoAlign.AutoAlignCommand.pidX; | |
| public PIDController pidRotate = AutoAlign.AutoAlignCommand.pidRotate; | |
| public PIDController pidX = new PIDController(4, 0, 0); | |
| public PIDController pidY = new PIDController(4, 0, 0); | |
| public PIDController pidRotate = new PIDController(8, 0, 0); |
| public static PIDController pidX = new PIDController(4, 0, 0); | ||
| public static PIDController pidY = new PIDController(4, 0, 0); | ||
| public static PIDController pidRotate = new PIDController(8, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These PID controllers should not be public static. Making them static creates a shared, mutable state between all instances of AutoAlignCommand and other classes that use them (like CommandSwerveDrivetrain). This is a dangerous pattern that can lead to unpredictable behavior and hard-to-debug issues. They should be instance fields (e.g., private final) to ensure each command has its own state.
| public static PIDController pidX = new PIDController(4, 0, 0); | |
| public static PIDController pidY = new PIDController(4, 0, 0); | |
| public static PIDController pidRotate = new PIDController(8, 0, 0); | |
| private final PIDController pidX = new PIDController(4, 0, 0); | |
| private final PIDController pidY = new PIDController(4, 0, 0); | |
| private final PIDController pidRotate = new PIDController(8, 0, 0); |
| new SwerveRequest.SysIdSwerveSteerGains(); | ||
| private final SwerveRequest.SysIdSwerveRotation m_rotationCharacterization = | ||
| new SwerveRequest.SysIdSwerveRotation(); | ||
| /*AutoAlign PID used in constructor for folloiwing choreo trajectories */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private final TalonFXSimState m_motorOneSimState; | ||
| private final TalonFXSimState m_motorTwoSimState; | ||
| private final StructPublisher<Pose3d> m_posePublisher; | ||
| public DoubleTopic arm_Pivot = NetworkTableInstance.getDefault().getDoubleTopic("Arm Pivot"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field name arm_Pivot does not follow standard Java naming conventions (camelCase). It should be renamed to armPivot for consistency.
| public DoubleTopic arm_Pivot = NetworkTableInstance.getDefault().getDoubleTopic("Arm Pivot"); | |
| public DoubleTopic armPivot = NetworkTableInstance.getDefault().getDoubleTopic("Arm Pivot"); |
| public PIDController pidX = AutoAlign.AutoAlignCommand.pidX; | ||
| public PIDController pidY = AutoAlign.AutoAlignCommand.pidX; | ||
| public PIDController pidRotate = AutoAlign.AutoAlignCommand.pidRotate; | ||
| public PIDController headingController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ; | ||
| ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import edu.wpi.first.networktables.NetworkTableInstance; | ||
| import edu.wpi.first.networktables.StructPublisher; | ||
| import edu.wpi.first.wpilibj.simulation.ElevatorSim; | ||
| import frc.robot.Robot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch added Choreo autos that the robot can run in simulation.