Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a Shooter subsystem for a robot, adding velocity control capabilities with configurable modes and PID control.
Changes:
- Added shooter velocity control with PID configuration and soft limits
- Introduced ShooterModes enum for managing different shooting states (SHOOTER, REVERSE, IDLE)
- Created a command factory method for running the shooter with specified modes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/main/java/frc/robot/subsystems/shooter/Shooter.java | Implemented Shooter subsystem with state management, motor configuration, and command creation |
| src/main/java/frc/robot/Constants.java | Added PID constants, speed constants, and ShooterModes enum for shooter configuration |
| .vscode/settings.json | Changed Java formatter from Spotless to RedHat extension |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configForwardSoftLimit(ShooterConstants.maxForwardOutput, true); | ||
| configReverseSoftLimit(ShooterConstants.maxReverseOutput, true); | ||
| config.applyTalonConfig(motor); | ||
| this.setConfig(); |
There was a problem hiding this comment.
The setConfig() method is called twice: once by the parent constructor and again explicitly here. This creates a new ShooterConfig instance that overwrites the config after it has been applied to the motor. The explicit this.setConfig() call should be removed since the parent constructor already handles configuration initialization.
| this.setConfig(); |
| public static class ShooterConfig extends Config { | ||
| public ShooterConfig() { | ||
| super("Shooter", ShooterConstants.id, Constants.canbus); | ||
| @Getter |
There was a problem hiding this comment.
The shooterState field has a getter but no setter, yet it is initialized to IDLE and never updated. Consider either adding logic to update this state based on the current shooter mode, or adding a setter if external updates are needed.
| @Getter | |
| @Getter | |
| @Setter |
| SHOOTER(shooterSpeed, 1.0), | ||
| REVERSE(reverseSpeed, -0.5), | ||
| IDLE(idleSpeed, 0.0); | ||
|
|
||
| public AngularVelocity speed; | ||
| public double output; | ||
|
|
||
| ShooterModes(AngularVelocity speed, double output) { | ||
| this.speed = speed; | ||
| this.output = output; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Inconsistent indentation in the ShooterModes enum. The enum constants and their members use deeper indentation (12 spaces) compared to the standard 4-space indentation used elsewhere in the codebase.
| SHOOTER(shooterSpeed, 1.0), | |
| REVERSE(reverseSpeed, -0.5), | |
| IDLE(idleSpeed, 0.0); | |
| public AngularVelocity speed; | |
| public double output; | |
| ShooterModes(AngularVelocity speed, double output) { | |
| this.speed = speed; | |
| this.output = output; | |
| } | |
| } | |
| SHOOTER(shooterSpeed, 1.0), | |
| REVERSE(reverseSpeed, -0.5), | |
| IDLE(idleSpeed, 0.0); | |
| public AngularVelocity speed; | |
| public double output; | |
| ShooterModes(AngularVelocity speed, double output) { | |
| this.speed = speed; | |
| this.output = output; | |
| } | |
| } |
| import frc.robot.Constants.ShooterConstants; | ||
| import frc.robot.Constants.ShooterConstants.ShooterModes; | ||
| import frc.robot.Constants.ShooterConstants.ShooterState; | ||
| import frc.team5431.titan.core.misc.Logger; |
There was a problem hiding this comment.
The Logger import from frc.team5431.titan.core.misc.Logger is added but never used in this file. Consider removing this unused import.
| import frc.team5431.titan.core.misc.Logger; |
oops Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
micw1417
left a comment
micw1417
left a comment
There was a problem hiding this comment.
ur so cool make this the IO paradigm they want later
2 participants
No description provided.