Code review pull request #51
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lycaon7
requested changes
Jun 18, 2021
src/main/java/frc/robot/utilities/TrigonPlaystationController.java
Outdated
Show resolved
Hide resolved
Mboesn
commented
Jun 18, 2021
Comment on lines
58
to
63
| private TrigonSwerveControllerCMDGP motionTest; | ||
| private IntakeOpenerCMD intakeCMD; | ||
|
|
||
| private ShootCMDGP shootCMDGP; | ||
| private ShootWithPitcherCMDGP ShootWithPitcherCMDGP; | ||
| private CollectCMDGP collectCMDGP; |
There was a problem hiding this comment.
IntakeOpenerCMD Should be OpenIntake
CollectCMDGP should just be Collect
Subsystems are nouns.
Command are verbs of the nouns, all of these names should reflect the actions of the command instead of saying they are commands.
There was a problem hiding this comment.
this comment was brought to you by Ohad not me
Comment on lines
86
to
91
| SmartDashboard.putData("Shooter Command", shooterCMD); | ||
| SmartDashboard.putData("CalibrateShooterKfCMD", calibrateShooterKfCMD); | ||
| SmartDashboard.putData("CalibrateLoaderKfCMD", calibrateLoaderKfCMD); | ||
| SmartDashboard.putData("TurnToTargetCMD", turnToTargetCMD); | ||
| SmartDashboard.putData("TurnAndPositionToTargetCMD", turnAndPositionToTargetCMD); | ||
| SmartDashboard.putData("TrigonSwerveControllerCMDGP", motionTest); |
There was a problem hiding this comment.
should be under "Commands/
| /** | ||
| * @return the vector between the middle of the robot and the target. | ||
| */ | ||
| private Vector2d calculateVector() { |
There was a problem hiding this comment.
this isnt used, does it need to exist?
Comment on lines
+61
to
+64
| return 4.15*Math.pow(Math.E, -0.0604*y) + 0.8; | ||
| else | ||
| return 4.15*Math.pow(Math.E, -0.0604*y) + 0.8; | ||
| } |
| /** | ||
| * @param tableKey the key of the limelight - if it was changed. | ||
| */ | ||
| public VanillaLimelight(String tableKey, LimelightConstants constants) { |
There was a problem hiding this comment.
change name to limelight
|
|
||
| @Override | ||
| public void end(boolean interrupted) { | ||
| if(ledSS != null) |
|
|
||
| import java.util.HashMap; | ||
|
|
||
| public class SensorTestCMD extends CommandBase { |
There was a problem hiding this comment.
test this class improtant
laviRZ
requested changes
Jun 20, 2021
Comment on lines
61
to
63
| private ShootCMDGP shootCMDGP; | ||
| private ShootWithPitcherCMDGP ShootWithPitcherCMDGP; | ||
| private CollectCMDGP collectCMDGP; |
| */ | ||
| public RobotContainer() { | ||
|
|
||
| Logger.configureLoggingAndConfig(this, true); |
Comment on lines
86
to
91
| SmartDashboard.putData("Shooter Command", shooterCMD); | ||
| SmartDashboard.putData("CalibrateShooterKfCMD", calibrateShooterKfCMD); | ||
| SmartDashboard.putData("CalibrateLoaderKfCMD", calibrateLoaderKfCMD); | ||
| SmartDashboard.putData("TurnToTargetCMD", turnToTargetCMD); | ||
| SmartDashboard.putData("TurnAndPositionToTargetCMD", turnAndPositionToTargetCMD); | ||
| SmartDashboard.putData("TrigonSwerveControllerCMDGP", motionTest); |
Comment on lines
103
to
105
| shooterCMD = new ShooterCMD(subsystemContainer.SHOOTER_SS, null, robotConstants.shooterConstants, | ||
| () -> SmartDashboard.getNumber("Shooter/Desired Velocity", 0)); | ||
| calibrateShooterKfCMD = new CalibrateShooterKfCMD(subsystemContainer.SHOOTER_SS, |
| * Initializes all commands. | ||
| */ | ||
| public void initializeCommands() { | ||
| SmartDashboard.putNumber("Shooter/Desired Velocity", 0); |
There was a problem hiding this comment.
It's not a command that you're initializing, so don't put it here
There was a problem hiding this comment.
this was required because that value is given to the command in the constructor and if you attempt to get a value without sending it first weird and annoying things happen
Comment on lines
113
to
118
| double rand = 0.1 * this.rand.nextInt(10); | ||
| int odd = randomOddNumber(); | ||
| while (odd < 5 && rand == 0.0) { | ||
| odd = randomOddNumber(); | ||
| } | ||
| return -(rand + odd * 0.01); |
| */ | ||
| private int randomOddNumber() { | ||
| int rand = this.rand.nextInt(10); | ||
| return rand % 2 == 0 ? rand + 1 : rand; |
Comment on lines
+64
to
+81
| hasRecalculatedF = false; | ||
| hasSetpoint = false; | ||
| ballsShotCount = 0; | ||
| sampleCount = 0; | ||
| outputSum = 0; | ||
| lastTimeAtSetpoint = Timer.getFPGATimestamp(); | ||
| currentState = ShooterState.AfterShot; | ||
| TBHController.reset(); | ||
| PIDController.reset(); | ||
| if (isUsingLimelight) | ||
| limelight.startVision(Target.PowerPort); | ||
| if(desiredVelocitySupplier.getAsDouble() < 2000){ | ||
| desiredVelocity = 3200; | ||
| DriverStationLogger.logToDS("ShooterCMD/tooLowDesiredVelocity: " + desiredVelocitySupplier.getAsDouble()); | ||
| } | ||
| else | ||
| desiredVelocity = desiredVelocitySupplier.getAsDouble(); | ||
| f = constants.KF_COEF_A * desiredVelocity + constants.KF_COEF_B; |
Comment on lines
+16
to
+17
| // private final TrigonDoubleSolenoid solenoid; | ||
| // private final ColorSensorV3 colorSensor; |
Comment on lines
+9
to
+18
| private final JoystickButton buttonA; | ||
| private final JoystickButton buttonB; | ||
| private final JoystickButton buttonX; | ||
| private final JoystickButton buttonY; | ||
| private final JoystickButton leftBumper; | ||
| private final JoystickButton rightBumper; | ||
| private final JoystickButton leftStickButton; | ||
| private final JoystickButton rightStickButton; | ||
| private final JoystickButton backButton; | ||
| private final JoystickButton startButton; |
There was a problem hiding this comment.
whywhywhywhywhywhwyhwhyw
so ugly
switched ledcontroller and blink time to constants switched isopen place in constructor
…-TRIGON/RobotCode2021 into BranchForCodeReview1
Based on Ohad code review done by me not Ohad
based on ohads code review fixed by me not him
Co-Authored-By: lycaon7 <[email protected]> Co-authored-by: lycaon7 <[email protected]>
this took way too long but it works hopefully
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request has all the code being merged into an empty branch.
This is done in order to have an easy way to review the entire code and write comments about everything.