Conversation
mduffor
requested review from
AnshulBompada,
Manyata-Patel,
barbute and
mduffor
| // ex: this.robotDrive = robotDrive; | ||
| this.drive = drive; | ||
|
|
||
| autoChooser = new SendableChooser<>(); |
There was a problem hiding this comment.
@barbute @AnshulBompada @Manyata-Patel, everyone needs to get on the same page on how the choosers will be created.
| default: | ||
| // Instantiate subsystems that are driven by playback of recorded sessions. (IO modules) | ||
| drive = new Drive( new Module[] { | ||
| new Module("FL", new ModuleIO(){}), |
There was a problem hiding this comment.
You can't create concrete instances of interfaces. Does this compile?
There was a problem hiding this comment.
Needs to be tested with replay
|
|
||
| driverController.povLeft().onTrue(drive.setDriveStateCommandContinued(DriveState.SNIPER_LEFT)).onFalse(drive.setDriveStateCommand(DriveState.TELEOP)); | ||
|
|
||
| driverController.b().onTrue(drive.setDriveStateCommandContinued(DriveState.DRIFT_TEST)).onFalse(drive.setDriveStateCommand(DriveState.TELEOP)); |
There was a problem hiding this comment.
Can you please define "Drift"?
| private void configureButtonBindings() { | ||
| driverController.x().onTrue(Commands.runOnce(() -> {drive.resetGyro();})); | ||
|
|
||
| driverController.povUp().onTrue(drive.setDriveStateCommandContinued(DriveState.SNIPER_UP)).onFalse(drive.setDriveStateCommand(DriveState.TELEOP)); |
There was a problem hiding this comment.
What is "UP" with the drivebase?
| azimuthVoltageControl = new VoltageOut(0); | ||
| driveVoltageControl = new VoltageOut(0); | ||
|
|
||
| // Closed - Loop control // |
There was a problem hiding this comment.
Need comments here on how you are creating these controllers, and what parameters you are passing in.
| azimuth.setPosition( | ||
| Rotation2d.fromRotations( | ||
| absolutePosistionSignal.getValueAsDouble()).minus(angleOffset).getRotations(), | ||
| 2.5); |
There was a problem hiding this comment.
The 2.5, assuming it is "timeout in seconds" needs to be assigned to a named variable/constant so the code can be better understood.
| /** | ||
| * Stop swerve module | ||
| */ | ||
| public void stop(){ |
There was a problem hiding this comment.
Do you also need to set your velocities and accelerations to null?
| var desiredDeltaPose = new Pose2d( | ||
| speeds.vxMetersPerSecond * dt, | ||
| speeds.vyMetersPerSecond * dt, | ||
| new Rotation2d(speeds.omegaRadiansPerSecond * dt * kDriftRate.get())); |
There was a problem hiding this comment.
Please explain the purpose of drift rate. Its default value is 2, so you are actually setting your Chassis speed to twice what is being asked for, even after it has been discretized.
| } | ||
|
|
||
| public boolean isSpeedOptimized(SwerveModuleState state, SwerveModuleState optimizedState) { | ||
| return state.speedMetersPerSecond != optimizedState.speedMetersPerSecond; |
There was a problem hiding this comment.
Don't directly compare floats. Use an epsilon comparison within a reasonable epsilon.
| private double invert; | ||
|
|
||
| public HeadingController() { | ||
| invert = (RobotBase.isReal()) ? -1.0 : 1.0; |
There was a problem hiding this comment.
Why is this inverting based on robot mode?
…kign through duffys commnets
…/template-talon_standard into drive-level-2-remake
Fixed Heading controller to work better and is a little bit easier to read, teleop controlelr added controller, fixed most naming issues, updated alliance flip util, adding comments to explain code
…/template-talon_standard into drive-level-2-remake
| public ChassisSpeeds computeChassisSpeeds(Rotation2d robotAngle, ChassisSpeeds currentRelativeSpeeds){ | ||
|
|
||
| // Scales the filtered value if the person driving wanted a slower or faster robot // | ||
| double xChangedDemand = linearScalar.get() * applyDeadband(xSupplier.getAsDouble(), linearDeadband.get()); |
There was a problem hiding this comment.
Why is the linear scalar applied before exponent?
| double omegaChangedDemand = omegaScalar.get() * applyDeadband(omegaSupplier.getAsDouble(), omegaDeadband.get()); | ||
|
|
||
| // Makes sure that the exponenet is a whole number // | ||
| int linearExp = (int) Math.round(linearInputsExponent.get()); |
There was a problem hiding this comment.
Why are exponents rounded to whole numbers?
| int rotationExp = (int) Math.round(rotationInputsExponent.get()); | ||
|
|
||
| // Converts the value to m/s for the chassis speeds // | ||
| double xVelocityMPS = DriveConstants.kMaxLinearSpeed * Math.pow(xChangedDemand, linearExp); |
There was a problem hiding this comment.
Why are the linear and exponent calculations not clamped between 0 and 1 before applying to velocity? If the values are set incorrectly, you could request a faster-than-max speed from the drive system. I would suggest doing all of the control adjustments and storing them into variables, and only then after everything has been adjusted do you multiply by kMaxLinearSpeed.
…/template-talon_standard into drive-level-2-remake
Merge 9105 swerve code back to templates
4 participants
No description provided.