Conversation
Makinist08
left a comment
Makinist08
left a comment
There was a problem hiding this comment.
missing methods probably cause of incomplete uml
| import frc.robot.subsystems.DriveTrain; | ||
| import frc.robot.subsystems.SwagLights; | ||
| import frc.robot.subsystems.Vision; | ||
| import frc.robot.subsystems.*; |
There was a problem hiding this comment.
have you ever heard of separation of code
There was a problem hiding this comment.
This was done by a formatter. I'm going to keep it since I don't see the harm it causes. Also seems a little nit-picky.
There was a problem hiding this comment.
We should not import *. Only import things you need
There was a problem hiding this comment.
Pull request overview
Adds a new shooter subsystem (with two shooter modules and shared ServoHub) and supporting constants, plus updates repo documentation/diagrams and a small constants cleanup.
Changes:
- Introduce
Shootersubsystem with innerShooterModulecontrolling dual Spark Flex motors and a ServoHub-controlled angle. - Add
ShooterConstants,FieldConstants, and extendNeoMotorConstantswith a nominal voltage constant. - Update
uml.mmdand minor formatting/comment tweaks in existing files.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
uml.mmd |
Updates UML diagram content to include shooter-related elements (currently inconsistent with implementation). |
src/main/java/frc/robot/subsystems/Shooter.java |
New shooter subsystem with two modules, velocity closed-loop control, and servo angle setting. |
src/main/java/frc/robot/constants/ShooterConstants.java |
New shooter configuration constants (CAN IDs, servo channels/ranges, control gains). |
src/main/java/frc/robot/constants/NeoMotorConstants.java |
Adds nominal voltage constant used for feedforward calculation. |
src/main/java/frc/robot/constants/FieldConstants.java |
Adds hub position constant as a Translation3d. |
src/main/java/frc/robot/constants/DriveConstants.java |
Minor comment/formatting adjustment around max angular rate. |
src/main/java/frc/robot/Robot.java |
Switches to a wildcard import for subsystems (no functional change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return servoPulseRange.minPulse_us + | ||
| (int) (MathUtil.clamp(angle, ShooterConstants.SHOOTER_ANGLE_MIN_DEG, ShooterConstants.SHOOTER_ANGLE_MAX_DEG) / | ||
| (ShooterConstants.SHOOTER_ANGLE_MAX_DEG - ShooterConstants.SHOOTER_ANGLE_MIN_DEG) * (servoPulseRange.maxPulse_us - servoPulseRange.minPulse_us)); |
There was a problem hiding this comment.
getPulseWidth() maps the requested angle to pulse width incorrectly: the normalized ratio should be (angle - SHOOTER_ANGLE_MIN_DEG) / (SHOOTER_ANGLE_MAX_DEG - SHOOTER_ANGLE_MIN_DEG). As written, SHOOTER_ANGLE_MIN_DEG (e.g., 55) produces a ratio > 1, resulting in pulse widths exceeding maxPulse_us and wrong angles.
| return servoPulseRange.minPulse_us + | |
| (int) (MathUtil.clamp(angle, ShooterConstants.SHOOTER_ANGLE_MIN_DEG, ShooterConstants.SHOOTER_ANGLE_MAX_DEG) / | |
| (ShooterConstants.SHOOTER_ANGLE_MAX_DEG - ShooterConstants.SHOOTER_ANGLE_MIN_DEG) * (servoPulseRange.maxPulse_us - servoPulseRange.minPulse_us)); | |
| double clampedAngle = MathUtil.clamp( | |
| angle, | |
| ShooterConstants.SHOOTER_ANGLE_MIN_DEG, | |
| ShooterConstants.SHOOTER_ANGLE_MAX_DEG); | |
| double normalized = | |
| (clampedAngle - ShooterConstants.SHOOTER_ANGLE_MIN_DEG) | |
| / (ShooterConstants.SHOOTER_ANGLE_MAX_DEG - ShooterConstants.SHOOTER_ANGLE_MIN_DEG); | |
| return servoPulseRange.minPulse_us | |
| + (int) (normalized * (servoPulseRange.maxPulse_us - servoPulseRange.minPulse_us)); |
| @@ -0,0 +1,44 @@ | |||
| package frc.robot.constants; | |||
|
|
|||
| import com.pathplanner.lib.util.FlippingUtil; | |||
There was a problem hiding this comment.
Unused import: FlippingUtil is imported but not referenced in this file. Please remove it to keep the constants class clean.
| import com.pathplanner.lib.util.FlippingUtil; |
| public static final double FUEL_WEIGHT_MIN = Units.lbsToKilograms(0.448); | ||
| public static final double FUEL_WEIGHT_MAX_LB = Units.lbsToKilograms(0.5); |
There was a problem hiding this comment.
FUEL_WEIGHT_MAX_LB is converted with Units.lbsToKilograms(...), so the value is in kilograms despite the _LB suffix. Rename the constant to reflect the unit (e.g., _KG) or keep it in pounds consistently.
| public static final double FUEL_WEIGHT_MIN = Units.lbsToKilograms(0.448); | |
| public static final double FUEL_WEIGHT_MAX_LB = Units.lbsToKilograms(0.5); | |
| public static final double FUEL_WEIGHT_MIN_KG = Units.lbsToKilograms(0.448); | |
| public static final double FUEL_WEIGHT_MAX_KG = Units.lbsToKilograms(0.5); |
| class LeftShooter { | ||
| - LeftShooter() | ||
| - LeftShooter instance | ||
| + getInstance() LeftShooter | ||
| } | ||
| class LeftShooter { | ||
| - LeftShooter() | ||
| + Translation2d TRANSLATION | ||
| + int LEFT_MOTOR_CAN_ID | ||
| + int RIGHT_MOTOR_CAN_ID | ||
| } |
There was a problem hiding this comment.
LeftShooter is defined twice in the diagram, and there is no corresponding LeftShooter class in the Java sources. This makes the UML misleading; please update the diagram to reflect the actual Shooter/ShooterModule structure or remove these classes.
| class LeftShooter { | |
| - LeftShooter() | |
| - LeftShooter instance | |
| + getInstance() LeftShooter | |
| } | |
| class LeftShooter { | |
| - LeftShooter() | |
| + Translation2d TRANSLATION | |
| + int LEFT_MOTOR_CAN_ID | |
| + int RIGHT_MOTOR_CAN_ID | |
| } |
| class RightShooter { | ||
| - RightShooter() | ||
| - RightShooter instance | ||
| + getInstance() RightShooter | ||
| } | ||
| class RightShooter { | ||
| - RightShooter() | ||
| + int LEFT_MOTOR_CAN_ID | ||
| + Translation2d TRANSLATION | ||
| + int RIGHT_MOTOR_CAN_ID | ||
| } |
There was a problem hiding this comment.
RightShooter is also duplicated in the UML and doesn’t exist in the codebase. The diagram should be updated so it matches the implemented shooter subsystem classes.
| class RightShooter { | |
| - RightShooter() | |
| - RightShooter instance | |
| + getInstance() RightShooter | |
| } | |
| class RightShooter { | |
| - RightShooter() | |
| + int LEFT_MOTOR_CAN_ID | |
| + Translation2d TRANSLATION | |
| + int RIGHT_MOTOR_CAN_ID | |
| } |
| # Shooter(int, int, Translation2d) | ||
| - Translation2d translation | ||
| - SparkClosedLoopController controller | ||
| - SparkFlex rightMotor | ||
| - SparkFlex leftMotor | ||
| - RelativeEncoder encoder | ||
| + start(double) void | ||
| + getRPM() double | ||
| + stop() void | ||
| + isAtTargetRPM() boolean | ||
| } |
There was a problem hiding this comment.
The UML Shooter class definition here doesn’t match the implemented frc.robot.subsystems.Shooter (which contains nearShooter/farShooter, a ServoHub, and an inner ShooterModule with servo angle control). Please update the diagram so the types/fields/methods correspond to the new implementation.
| # Shooter(int, int, Translation2d) | |
| - Translation2d translation | |
| - SparkClosedLoopController controller | |
| - SparkFlex rightMotor | |
| - SparkFlex leftMotor | |
| - RelativeEncoder encoder | |
| + start(double) void | |
| + getRPM() double | |
| + stop() void | |
| + isAtTargetRPM() boolean | |
| } | |
| - Shooter() | |
| - ShooterModule nearShooter | |
| - ShooterModule farShooter | |
| - ServoHub servoHub | |
| } | |
| class ShooterModule { | |
| - ShooterModule() | |
| - double targetAngle | |
| + setAngle(double) void | |
| + getAngle() double | |
| } |
|
|
||
| // Subsystems | ||
| private final DriveTrain driveTrain = DriveTrain.getInstance(); | ||
|
|
| // per second max | ||
| // angular velocity | ||
|
|
||
| // 3/4 of a rotation angular velocity per second max |
There was a problem hiding this comment.
I think this change but fix the other ones too. Also, only 1 blank line between the constant and the comment
| import edu.wpi.first.math.util.Units; | ||
|
|
||
| public final class FieldConstants { | ||
| private FieldConstants() {} |
There was a problem hiding this comment.
None of the other constants files have constructors
| import edu.wpi.first.math.util.Units; | ||
|
|
||
| public final class ShooterConstants { | ||
| private ShooterConstants() { |
There was a problem hiding this comment.
Non of the other constants files have constructors
| @@ -0,0 +1,44 @@ | |||
| package frc.robot.constants; | |||
|
|
|||
| import com.pathplanner.lib.util.FlippingUtil; | |||
There was a problem hiding this comment.
Some of these imports are unused
| import com.revrobotics.servohub.ServoHub; | ||
| import com.revrobotics.servohub.config.ServoChannelConfig; | ||
| import com.revrobotics.servohub.config.ServoHubConfig; | ||
| import com.revrobotics.spark.*; |
|
|
||
| public static final int SERVO_HUB_CAN_ID = 0; | ||
|
|
||
| public static final int NEAR_LEFT_MOTOR_CAN_ID = 0; |
There was a problem hiding this comment.
What if we make an interface for shooter module config, then put inner classes in here for each shooter module, and pass those inner classes to the shooter module instead of having a bunch of individual params in the constructor?
| private final SparkFlex rightMotor; | ||
| private final SparkClosedLoopController controller; | ||
| private final RelativeEncoder encoder; | ||
| private final Translation3d translation; |
There was a problem hiding this comment.
Why isn't this translation 2d?
| return controller.isAtSetpoint(); | ||
| } | ||
|
|
||
| public Translation3d getTranslation() { |
There was a problem hiding this comment.
These functions are missing JavaDoc
| /** | ||
| * Base shooter module. | ||
| */ | ||
| public static class ShooterModule { |
There was a problem hiding this comment.
Is the function that takes in distance from center to hub that calculates the rpm not part of this? I know we don't quite know the formula but we could put a place holder for the formula so that we have the method signature
|
@Murat65536 fix the UML. This will not merge until you do |
3 participants
Pull Request
Issue Number
Closes #4
Comments