create_rotors_subsystem - #3
Conversation
James Justin
|
Be sure to make the pull request name more specific. "I did..." is not a good naming convention since it has extra words that don't add anything" |
There was a problem hiding this comment.
Pull request overview
Adds a new Rotors subsystem intended to manage multiple TalonFX-driven rollers/flywheels, along with the CAN IDs and placeholder TalonFX configurations needed to configure those motors.
Changes:
- Introduces
Rotorssubsystem with TalonFX instances, config application, getters, and setters. - Adds rotor-related CAN IDs to
DeviceIDs.rotorIDs. - Adds
ConstRotorswith TalonFXConfiguration placeholders for each rotor motor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/main/java/frc/robot/subsystems/Rotors.java | Implements the new rotor subsystem (motor objects, configuration, getters/setters). |
| src/main/java/frc/robot/DeviceIDs.java | Defines CAN IDs for the rotor subsystem motors. |
| src/main/java/frc/robot/constants/ConstRotors.java | Adds configuration constants intended to be applied to rotor TalonFX devices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final TalonFX flywheelRollersEast = new TalonFX((rotorIDs.FLYWHEEL_EAST_FOLLOWER)); | ||
| final TalonFX flywheelWestFollower = new TalonFX((rotorIDs.TRANSFER_ROLLERS_WEST_CAN)); |
| intakeRollersWest.getConfigurator().apply(ConstRotors.FLYWHEEL_ROLLERS_WEST_CONFIGURATION); | ||
| transferRollersEast.getConfigurator().apply(ConstRotors.TRANSFER_ROLLERS_EAST_CONFIGURATION); | ||
| transferRollersWest.getConfigurator().apply(ConstRotors.TRANSFER_ROLLERS_WEST_CONFIGURATION); | ||
| flywheelRollersEast.getConfigurator().apply(ConstRotors.FLYWHEEL_ROLLERS_EAST_CONFIGURATION); | ||
| flywheelRollersEast.getConfigurator().apply(ConstRotors.FLYWHEEL_ROLLERS_EAST_CONFIGURATION); |
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getIntakeRollersWest() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getTransferRollersEast() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getTransferRollersWest() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelRollersEast() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelRollersWest() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelEastFollower() { | ||
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelWestFollower() { | ||
| return serializerRollers.getVelocity().getValue(); |
| intakeRollersWest.set(speed); | ||
|
|
||
| } | ||
|
|
||
| public void intakeRollersWest(double speed) { | ||
| intakeRollersEast.set(speed); |
| public void flywheelRollersEast(double speed) { | ||
| flywheelEastFollower.set(speed); | ||
|
|
||
| } | ||
|
|
||
| public void flywheelRollersWest(double speed) { | ||
| flywheelRollersWest.set(speed); | ||
|
|
||
| } | ||
|
|
||
| public void flywheelWestFollower(double speed) { | ||
| flywheelRollersWest.set(speed); | ||
|
|
| public static final int FLYWHEEL_WEST_FOLLOWER = 18; | ||
| public static final int FLYWHEEL_EAST_FOLLOWER = 19; | ||
|
|
| import edu.wpi.first.epilogue.Logged; | ||
| import edu.wpi.first.units.measure.AngularVelocity; | ||
| import edu.wpi.first.wpilibj2.command.SubsystemBase; | ||
| import frc.robot.DeviceIDs; |
TaylerUva
left a comment
There was a problem hiding this comment.
good so far, check requested changes
| final TalonFX flywheelRollersWest = new TalonFX((rotorIDs.FLYWHEEL_ROLLERS_WEST_CAN)); | ||
| final TalonFX flywheelRollersEast = new TalonFX((rotorIDs.FLYWHEEL_EAST_FOLLOWER)); |
|
|
||
| } | ||
|
|
||
| public AngularVelocity getSerializerRollers() { |
There was a problem hiding this comment.
what about the rollert? what are we getting from the roller? this applies to all your get* functions
|
|
||
| } | ||
|
|
||
| public void intakeRollersEast(double speed) { |
There was a problem hiding this comment.
what are we doing to intakeRollersEast? look at your other functions too
TaylerUva
left a comment
There was a problem hiding this comment.
Remember to pay attention to key details, like making sure names align, a lot of mismatches were caught and these could physically break the robot
| return serializerRollers.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getIntakeRollersEastVelocity() { |
There was a problem hiding this comment.
Don't need a accessor method for every motor, like getIntakeRollersEast/WestVelocity should be replaced by a single getIntakeRollersVelocity where only gets the master motor. this same issue happened multiple times in this same file.
| public void intakeRollersEastSpeed(double speed) { | ||
| intakeRollersEast.set(speed); | ||
| } | ||
|
|
||
| public void intakeRollersWestSpeed(double speed) { | ||
| intakeRollersWest.set(speed); | ||
| } | ||
|
|
||
| public void transferRollersEastSpeed(double speed) { | ||
| transferRollersEast.set(speed); | ||
| } | ||
|
|
||
| public void transferRollersWestSpeed(double speed) { | ||
| transferRollersWest.set(speed); | ||
| } | ||
|
|
||
| public void flywheelEastSpeed(double speed) { | ||
| flywheelEast.set(speed); | ||
| } | ||
|
|
||
| public void flywheelWestSpeed(double speed) { | ||
| flywheelWest.set(speed); | ||
| } | ||
|
|
||
| public void flywheelWestFollowerSpeed(double speed) { | ||
| flywheelWestFollower.set(speed); | ||
| } | ||
|
|
||
| public void flywheelEastFollowerSpeed(double speed) { | ||
| flywheelEastFollower.set(speed); | ||
| } |
There was a problem hiding this comment.
- add word "set" to all setter methods
- you set all motors in the same function. like setIntakeRollersSpeed should set both east and west motors, the follower should be using: setControl([theFollowerInstance])
| final TalonFX flywheelWest = new TalonFX((rotorIDs.FLYWHEEL_WEST_CAN)); | ||
| final TalonFX flywheelEast = new TalonFX((rotorIDs.FLYWHEEL_EAST_CAN)); | ||
| final TalonFX flywheelWestFollower = new TalonFX((rotorIDs.FLYWHEEL_WEST_FOLLOWER_CAN)); | ||
| final TalonFX flywheelEastFollower = new TalonFX(rotorIDs.FLYWHEEL_EAST_FOLLOWER_CAN); |
There was a problem hiding this comment.
need to add followers; examples: Follower flywheelEastFollower = new Follower(flywheelTopEast.getDeviceID(), MotorAlignmentValue.Aligned);
| public static final TalonFXConfiguration INTAKE_ROLLERS_WEST_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration TRANSFER_ROLLERS_EAST_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration TRANSFER_ROLLERS_WEST_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration FLYWHEEL_EAST_FOLLOWER_CONFIGURATION = new TalonFXConfiguration(); |
| public static final TalonFXConfiguration FLYWHEEL_EAST_FOLLOWER_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration FLYWHEEL_EAST_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration FLYWHEEL_WEST_CONFIGURATION = new TalonFXConfiguration(); | ||
| public static final TalonFXConfiguration FLYWHEEL_WEST_FOLLOWER_CONFIGURATION = new TalonFXConfiguration(); |
| public static final TalonFXConfiguration FLYWHEEL_WEST_FOLLOWER_CONFIGURATION = new TalonFXConfiguration(); | ||
|
|
||
| public static final double STOP = 0; | ||
|
|
There was a problem hiding this comment.
all of your configurations are empty, you need to add:
CONFIG.MotorOutput.NeutralMode = NeutralModeValue.Brake;
CONFIG.MotorOutput.Inverted = InvertedValue.Clockwise_Positive;
CONFIG.CurrentLimits.SupplyCurrentLimitEnable = true;
CONFIG.CurrentLimits.SupplyCurrentLowerLimit = 35;
for all configurators
i added followers
added foollowers ansd can check speed
added configurationhs
| AngularVelocity lastDesiredFlyWheelSpeed = Units.RPM.of(0); | ||
| AngularVelocity lastDesiredTransferRollersSpeed = Units.RPM.of(0); | ||
| Follower flywheelEastFollower = new Follower(flywheelTopEast.getDeviceID(), false); | ||
| Follower flywheelWestFollower = new Follower(flywheelTopWest.getDeviceID(), false); |
There was a problem hiding this comment.
both west flywheel motors should be following top east, and flywheelWestFollower should be:
Follower flywheelWestFollower = new Follower(flywheelTopEast.getDeviceID(), MotorAlignmentValue.Opposed);
| public AngularVelocity getFlywheelEastVelocity() { | ||
| return flywheelTopEast.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelWestVelocity() { | ||
| return flywheelTopWest.getVelocity().getValue(); | ||
| } |
There was a problem hiding this comment.
should combine both and only get east
|
|
||
| public void setFlywheelSpeeds(AngularVelocity speed) { | ||
| flywheelTopEast.setControl(flyWheelVelocityRequest.withVelocity(speed)); | ||
| flywheelTopWest.setControl(flyWheelVelocityRequest.withVelocity(speed)); |
fixxed comments
| return transferRollersEast.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public AngularVelocity getFlywheelEastVelocity() { |
There was a problem hiding this comment.
Doesn't getFlyWheelSpeeds() already cover this?
Fixed Angela's comment Co-Authored-By: Justinpham17386769 <244249272+Justinpham17386769@users.noreply.github.com> Co-Authored-By: jayden mendoza <244560677+jaymendo670-png@users.noreply.github.com>
James
Justin