Punk 2.0 #18
Punk 2.0 #18
Conversation
| /** Creates a new TurnToDegree. */ | ||
| SwerveSubsystem swerve = SwerveSubsystem.getInstance(); | ||
| FeederSubsystem feeder = FeederSubsystem.getInstance(); | ||
| TakeFeedSubsystem feeder = TakeFeedSubsystem.getInstance(); |
There was a problem hiding this comment.
very good overall.
There are mostly documentation changes which I want you to implement.
grape🍇 - https://youtu.be/xOKBM16UFE4
| public static final int driveCurrentThreshold = 60; | ||
| public static final double driveCurrentThresholdTime = 0.1; | ||
| public static final boolean driveEnableCurrentLimit = true; | ||
| public static final String CAN_BUS_NAME = "canBus"; |
There was a problem hiding this comment.
I think it might be Important to change this to "canivor can-bus" or something similar.
|
|
||
| /** Center of the speaker opening (blue alliance) */ | ||
| public static Pose2d centerSpeakerOpening = AllianceFlipUtil.apply( | ||
| new Pose2d(0.0, fieldWidth - Units.inchesToMeters(104.0), new Rotation2d())); | ||
| public static Pose2d centerSpeakerOpening = | ||
| new Pose2d(0.0, fieldWidth - Units.inchesToMeters(104.0), new Rotation2d()); |
There was a problem hiding this comment.
hmm, maybe you can add a function called applyAllianceTranslation which will apply the AllianceFlipUtils::apply logic
| public ClimbSetSpeed(DoubleSupplier speed) { | ||
| addRequirements(climb); | ||
| this.speed = speed; | ||
| } | ||
|
|
||
| @Override | ||
| public void execute() { | ||
| climb.setSpeed(speed.getAsDouble()); | ||
| } | ||
|
|
||
| @Override | ||
| public void end(boolean interrupted) { | ||
| climb.setSpeed(0); | ||
| } |
There was a problem hiding this comment.
where is finished?
is isFinished initialized to false by default?
| @@ -29,7 +29,7 @@ public void initialize() { | |||
| // Called every time the scheduler runs while the command is scheduled. | |||
| @Override | |||
| public void execute() { | |||
| swerve.drive(new Translation2d(),0,true,true,true); | |||
| swerve.drive(new Translation2d(),0,true,false); | |||
There was a problem hiding this comment.
what the fuck does 0,true,false mean?
Is this command incomplete?
| private final ShooterArmSubsystem shooterArm = ShooterArmSubsystem.getInstance(); | ||
| double angle; | ||
|
|
||
| public ShooterArmMoveToAngle(double angle) { |
There was a problem hiding this comment.
Document what this function expects to get
And don't just say
@param angle is the expected angle of the subsystem
say something more descriptive like what are the expected units of this angle (degrees, radians or encoder ticks?)
this should be at moveArmTo function also.
These documentation changes will help the next rashatz in your position understand how to write code and your line of thought. so adding them is worth it.
| public double getUpShooterSpeed() { | ||
| return m_upShooterMotor.getVelocity().getValue(); | ||
| } | ||
|
|
||
| public double getDownShooterSpeed() { | ||
| return m_downShooterMotor.getVelocity().getValue(); | ||
| } |
There was a problem hiding this comment.
Again, think about it you are getting the velocity (angular, measured in ticks, radians or degrees...)
This is a small but meaningful change.
| public boolean checkIfShooterAtSpeed() { | ||
| return ((Math.abs(getUpShooterSpeed() - targetSpeed) < shooterConstants.MAX_ERROR) | ||
| && ((Math.abs(getDownShooterSpeed() - targetSpeed) < shooterConstants.MAX_ERROR))); | ||
| } |
There was a problem hiding this comment.
maybe you should create a more generic form of this function
public boolean checkIfShooterAtVelocity(double targetVelocity, double acceptableError) {
return (Math.abs(getUpShooterVelocity() - targetVelocity) < acceptableError)
&& (Math.abs(getDownShooterVelocity() - targetVelocity) < acceptableError);
}and then use function overloading to create a simpler version:
public boolean checkIfShooterAtVelocity() {
return checkIfShooterAtVelocity(targetVelocity, shooterConstants.MAX_ERROR);
}| && ((Math.abs(getDownShooterSpeed() - targetSpeed) < shooterConstants.MAX_ERROR))); | ||
| } | ||
|
|
||
| public double speakerInterpolate() { |
There was a problem hiding this comment.
Looks good, just document that this function returns the expected velocity of the shooter.
| TalonFXConfiguration upConfigs = new TalonFXConfiguration(); | ||
| TalonFXConfiguration downConfigs = new TalonFXConfiguration(); |
There was a problem hiding this comment.
Fuck CTRE for not giving you a better way to statically create a configuration in the constants file.
| private TalonFX m_shooterArmMotor; | ||
| DigitalInput limitSwitch = new DigitalInput(SwitchID); | ||
| DigitalInput limitSwitch = new DigitalInput(shooterArmConstants.SWITCH_ID); |
There was a problem hiding this comment.
Can't stress enough how much this micro switch could not be trusted.
Make sure that if this switch breaks you still have some sort of fall back which makes sure the subsystem does not break.
added changing LED function if wont work we won't use it
@ori-coval - next time you ask for a CR at least create a PR for me to go through