Wheel radius characterization #196
Conversation
|
bro I forgot to commit during the meeting what the heck |
|
/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 wheel radius characterization routine, which is a valuable addition for improving odometry accuracy. The implementation is well-structured, using a dedicated command and state class. My feedback focuses on improving robustness, maintainability, and safety. Key suggestions include avoiding hardcoded values for module counts, using a proper logger instead of System.out.println, making the characterization auto opt-in instead of enabled by default, and cleaning up some temporary/debug code.
| public static Robot r = Robot.getInstance(); | ||
| public static final Subsystems s = r.subsystems; | ||
| public static final Controls controls = r.controls; | ||
| public static final boolean RUN_MEASUREMENT_AUTO = true; |
There was a problem hiding this comment.
The RUN_MEASUREMENT_AUTO flag is hardcoded to true. This is risky as it will cause the characterization routine to run instead of the selected autonomous mode every time the robot is in auto. This could lead to unexpected behavior during a match. It's highly recommended to set this to false by default and use a Shuffleboard toggle to enable it only when needed for testing.
| public static final boolean RUN_MEASUREMENT_AUTO = true; | |
| public static final boolean RUN_MEASUREMENT_AUTO = false; |
| // soloController | ||
| // .povUp() | ||
| // .whileTrue( | ||
| // s.groundArm | ||
| // .moveToPosition(GroundArm.GROUND_POSITION) | ||
| // .andThen(Commands.idle()) | ||
| // .withName("ground up position")); |
There was a problem hiding this comment.
This block of code is commented out. If it's no longer needed, it should be removed to improve code clarity. If it's being temporarily replaced for testing, it's better to use version control to switch between versions rather than leaving commented-out code.
src/main/java/frc/robot/subsystems/drivebase/CommandSwerveDrivetrain.java
Show resolved
Hide resolved
| public static final double DRIVE_BASE_RADIUS = | ||
| Math.max( | ||
| Math.max( | ||
| Math.hypot( | ||
| CompTunerConstants.FrontLeft.LocationX, CompTunerConstants.FrontLeft.LocationY), | ||
| Math.hypot( | ||
| CompTunerConstants.FrontRight.LocationX, | ||
| CompTunerConstants.FrontRight.LocationY)), | ||
| Math.max( | ||
| Math.hypot( | ||
| CompTunerConstants.BackLeft.LocationX, CompTunerConstants.BackLeft.LocationY), | ||
| Math.hypot( | ||
| CompTunerConstants.BackRight.LocationX, CompTunerConstants.BackRight.LocationY))); |
There was a problem hiding this comment.
The calculation for DRIVE_BASE_RADIUS is correct, but the deeply nested Math.max calls make it difficult to read. This can be refactored using a Java Stream for improved readability and maintainability.
public static final double DRIVE_BASE_RADIUS =
java.util.stream.Stream.of(
CompTunerConstants.FrontLeft,
CompTunerConstants.FrontRight,
CompTunerConstants.BackLeft,
CompTunerConstants.BackRight)
.mapToDouble(c -> Math.hypot(c.LocationX, c.LocationY))
.max()
.orElse(0.0);There was a problem hiding this comment.
What is a java Stream? and what does .max() do?
| () -> { | ||
| double[] positions = getWheelRadiusCharacterizationPositions(drive); | ||
| double wheelDelta = 0.0; | ||
| for (int i = 0; i < 4; i++) { |
There was a problem hiding this comment.
The loop hardcodes the number of modules to 4. It's more robust to use positions.length to dynamically determine the number of modules. This makes the code more reusable for different swerve configurations.
for (int i = 0; i < positions.length; i++) {| System.out.println( | ||
| "********** Wheel Radius Characterization Results **********"); | ||
| System.out.println( | ||
| "\tWheel Delta: " + formatter.format(wheelDelta) + " radians"); | ||
| System.out.println( | ||
| "\tGyro Delta: " + formatter.format(state.gyroDelta) + " radians"); | ||
| System.out.println( | ||
| "\tWheel Radius: " | ||
| + formatter.format(wheelRadius) | ||
| + " meters, " | ||
| + formatter.format(Units.metersToInches(wheelRadius)) | ||
| + " inches"); |
There was a problem hiding this comment.
The use of System.out.println (here and on line 53) is not ideal for production code. It's better to use a proper logging framework (e.g., WPILib's Logger or DataLogManager). This allows for configurable log levels and makes it easier to manage log output, especially during competitions.
Inspo: https://github.com/Mechanical-Advantage/AdvantageKit/blob/main/template_projects/sources/talonfx_swerve/src/main/java/frc/robot/commands/DriveCommands.java
see guys I contribute too
This was brought up to me on one of the programming discords as a way to measure your robots wheel radius.
This is critical for us, because this year we struggled with auto tuning, and being able to diagnose and find wheel radius instantly is incredibly important.
Want to test on monday 11/17 @Jetblackdragon