Pre-storm surge fixes/changes #193
Conversation
TEST AT NEPF 10/21
| private static final Transform2d leftOfTag = | ||
| new Transform2d( | ||
| Units.inchesToMeters(36.5 / 2), Units.inchesToMeters(-12.97 / 2), Rotation2d.k180deg); | ||
| Units.inchesToMeters(34 / 2), Units.inchesToMeters(-12.97 / 2), Rotation2d.k180deg); |
There was a problem hiding this comment.
Why are we doing in-line math? Why not just say 17 instead of 34/2?
There was a problem hiding this comment.
Same comment for all of the places we're doing the /2 through all of this...
| redBranchJ, | ||
| redBranchK, | ||
| redBranchL); | ||
| private static final Pose2d lBlueReefFaceAB = |
There was a problem hiding this comment.
Do we have any team style guidelines around global constants? Usually global constants are in all caps, but it depends on our team style standards.
| new SwerveRequest.FieldCentric() // Add a 10% deadband | ||
| .withDriveRequestType(DriveRequestType.OpenLoopVoltage) | ||
| .withDriveRequestType(DriveRequestType.Velocity) | ||
| .withForwardPerspective(ForwardPerspectiveValue.BlueAlliance); |
There was a problem hiding this comment.
Should BlueAlliance be hard-coded here? (I fully realize you didn't edit this line.)
| public static Pose2d getNearestLeftBranch(Pose2d p, boolean isBlue) { | ||
| List<Pose2d> branchPose2ds = isBlue ? blueLeftBranchPoses : redLeftBranchPoses; | ||
| return p.nearest(branchPose2ds); | ||
| public static Pose2d getTargetPose(Pose2d pose, AlignType type) { |
There was a problem hiding this comment.
Maybe rename the type variable here. It's odd that we have a class variable named 'type' and a routine parameter named 'type'. The parameter variable will win in this case, but it can be confusing for maintainers.
| this.drive = drive; | ||
| pidRotate.enableContinuousInput(-Math.PI, Math.PI); | ||
| this.controls = controls; | ||
| this.type = type; |
There was a problem hiding this comment.
type should not be a class global if the only place you use it is in initialize(). Instead, it should be a parameter passed into initialize().
| } else { | ||
| return Commands.parallel( | ||
| Commands.sequence( | ||
| spinnyClaw.coralGripIntakePower(), Commands.waitSeconds(0.05), spinnyClaw.stop()), |
There was a problem hiding this comment.
Is spinnyClaw guaranteed to be non-null?
| // manual control still works | ||
| // only used on solo controls | ||
|
|
||
| public Command coralLevelThree(BooleanSupplier score, BooleanSupplier ipScore) { // same as L4 |
There was a problem hiding this comment.
If this is the same as L4, can we combine into a single routine and pass a parameter for the smaller difference in height?
| } | ||
|
|
||
| public Command coralLevelTwo( | ||
| BooleanSupplier score, BooleanSupplier ipScore) { // same as L4 and L3 |
There was a problem hiding this comment.
Combine to a single routine with a parameter for any target differences?
|
|
||
| public final class AllianceUtils { | ||
| public static boolean isBlue() { | ||
| if (!DriverStation.getAlliance().isEmpty()) { |
There was a problem hiding this comment.
Should we throw or return error if .isEmpty() rather than assuming Red?
| } | ||
|
|
||
| public static boolean isRed() { | ||
| if (!DriverStation.getAlliance().isEmpty()) { |
…/Reefscape2025 into pre-WASS-fixes/changes
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a wide range of fixes and changes, primarily focused on refactoring the auto-alignment logic, updating control schemes, and reorganizing the Shuffleboard layout. The refactoring efforts, such as consolidating alignment commands and centralizing alliance logic, are commendable and improve code maintainability. However, there are several areas that could be improved. I've identified significant code duplication in the SuperStructure subsystem that should be addressed. There are also opportunities to enhance type safety in Controls.java by using enums instead of strings. The Shuffleboard layout has some inconsistencies, including a duplicated tab name that could cause confusion. Additionally, some logic in BargeAlign.java has been commented out, and a haptic feedback feature for the driver appears to have been removed, both of which might be unintentional. Finally, there are minor performance and style improvements that can be made in AutoAlign.java and the new AllianceUtils.java.
running off of drivebt2 which is running off of girls gen changes FYI
fixes #184