-
Notifications
You must be signed in to change notification settings - Fork 9
Mechanism2D in AdvantageScope testing #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| LiveWindow.disableAllTelemetry(); | ||
| LiveWindow.enableTelemetry(PDH); | ||
| mech = new Mechanism2d(1, 2); | ||
| root = mech.getRoot("climber", 0.5 + Units.inchesToMeters(5.5), Units.inchesToMeters(19.5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? Not sure what the "climber" is, as this year's robot isn't supposed to climb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnel pivot?
| root = mech.getRoot("climber", 0.5 + Units.inchesToMeters(5.5), Units.inchesToMeters(19.5)); | ||
| SmartDashboard.putData("Mechanism", mech); | ||
| m_elevator = | ||
| root.append(new MechanismLigament2d("elevator", 1, 90, 2, new Color8Bit(Color.kRed))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a comment that this is a single-stage elevator even though the robot actually uses a multi-stage elevator.
| public static final double CORAL_QUICK_INTAKE = 1.6; | ||
| public static final double MIN_EMPTY_GROUND_INTAKE = 4.5; | ||
| public static final double MIN_FULL_GROUND_INTAKE = 8.0; | ||
| private static final double MOTOR_ROTATIONS_PER_METER = 40; // Inaccurate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to make this accurate by measuring how far the elevator extends (in meters) at CORAL_LEVEL_FOUR_PRE_POS and then simply change this to 37.5 / <actual relative elevator extension>, where <actual relative elevator extension> is what's measured with a tape measure.
| Mechanism2d mech; | ||
| MechanismRoot2d root; | ||
| MechanismLigament2d m_elevator; | ||
| MechanismLigament2d m_wrist; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nicer to encapsulate the Mechanism (and similar "report physical position") code in a different class instead of adding it to the main Robot class. These are generally called 'digital twins', so could use something like DigitalTwin.java and have robotPeriodic() simply call an update() function on it.
| m_motorOneSimState.setRawRotorPosition(targetPos); | ||
| m_motorTwoSimState.setRawRotorPosition(targetPos); | ||
|
|
||
| elevatorPose3d.set(new Pose3d(new Pose2d(0.0, getHeightMeters(), new Rotation2d(0.0,0.0)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running this in the simulator, it's continually logging this error here:
Error at java.base/java.lang.Thread.getStackTrace(Thread.java:1619): x and y components of Rotation2d are zero
at java.base/java.lang.Thread.getStackTrace(Thread.java:1619)
at edu.wpi.first.math.geometry.Rotation2d.<init>(Rotation2d.java:126)
at frc.robot.subsystems.ElevatorSubsystem.periodic(ElevatorSubsystem.java:388)
at edu.wpi.first.wpilibj2.command.CommandScheduler.run(CommandScheduler.java:269)
at frc.robot.Robot.robotPeriodic(Robot.java:137)
at edu.wpi.first.wpilibj.IterativeRobotBase.loopFunc(IterativeRobotBase.java:400)
at edu.wpi.first.wpilibj.TimedRobot.startCompetition(TimedRobot.java:148)
at edu.wpi.first.wpilibj.RobotBase.runRobot(RobotBase.java:419)
at edu.wpi.first.wpilibj.RobotBase.lambda$startRobot$1(RobotBase.java:490)
at java.base/java.lang.Thread.run(Thread.java:840)
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a Mechanism2d visualization for the robot's arm and elevator, and refactors the AdvantageScope/Elastic layout for better organization. The changes also include updates to the auto-alignment and scoring logic, particularly for the solo controller.
My review focuses on improving code maintainability and fixing a few issues in the new simulation code. I've suggested refactoring some duplicated methods in Controls.java and replacing magic numbers with constants. There are also several minor cleanup items in ElevatorSubsystem.java, along with a critical bug in the Rotation3d usage for the simulation visualization that should be addressed.
| TESTpose.set(new Pose3d( | ||
| 0.2, 0.0, targetZ, | ||
| new Rotation3d(0.0, 0.0, -135))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rotation3d constructor takes radians, but is being passed -135. This is likely intended to be degrees and should be converted using Units.degreesToRadians(-135). Using -135 directly results in a very large and incorrect rotation in radians.
| TESTpose.set(new Pose3d( | |
| 0.2, 0.0, targetZ, | |
| new Rotation3d(0.0, 0.0, -135))); | |
| TESTpose.set(new Pose3d( | |
| 0.2, 0.0, targetZ, | |
| new Rotation3d(0.0, 0.0, edu.wpi.first.math.util.Units.degreesToRadians(-135)))); |
| private Command getSoloCoralBranchHeightCommandL() { | ||
| return switch (branchHeight) { | ||
| case CORAL_LEVEL_FOUR -> superStructure | ||
| .coralLevelFour(soloController.rightBumper()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_THREE -> superStructure | ||
| .coralLevelThree(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_TWO -> superStructure | ||
| .coralLevelTwo(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_ONE -> superStructure | ||
| .coralLevelOne(soloController.rightBumper(), () -> AutoAlign.poseInPlaceL1L()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| }; | ||
| } | ||
|
|
||
| private Command getSoloCoralBranchHeightCommandR() { | ||
| return switch (branchHeight) { | ||
| case CORAL_LEVEL_FOUR -> superStructure | ||
| .coralLevelFour(soloController.rightBumper()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_THREE -> superStructure | ||
| .coralLevelThree(soloController.rightBumper()) | ||
| .coralLevelThree(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_TWO -> superStructure | ||
| .coralLevelTwo(soloController.rightBumper()) | ||
| .coralLevelTwo(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| case CORAL_LEVEL_ONE -> superStructure | ||
| .coralLevelOne(soloController.rightBumper()) | ||
| .coralLevelOne(soloController.rightBumper(), () -> AutoAlign.poseInPlaceL1R()) | ||
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods getSoloCoralBranchHeightCommandL and getSoloCoralBranchHeightCommandR are nearly identical, with the only difference being the BooleanSupplier passed to superStructure.coralLevelOne. You can refactor them into a single method that accepts the BooleanSupplier as a parameter. This reduces code duplication and improves maintainability.
You can then call this new method from configureSoloControllerBindings like getSoloCoralBranchHeightCommand(() -> AutoAlign.poseInPlaceL1L()) and getSoloCoralBranchHeightCommand(() -> AutoAlign.poseInPlaceL1R()).
| private Command getSoloCoralBranchHeightCommandL() { | |
| return switch (branchHeight) { | |
| case CORAL_LEVEL_FOUR -> superStructure | |
| .coralLevelFour(soloController.rightBumper()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_THREE -> superStructure | |
| .coralLevelThree(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_TWO -> superStructure | |
| .coralLevelTwo(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_ONE -> superStructure | |
| .coralLevelOne(soloController.rightBumper(), () -> AutoAlign.poseInPlaceL1L()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| }; | |
| } | |
| private Command getSoloCoralBranchHeightCommandR() { | |
| return switch (branchHeight) { | |
| case CORAL_LEVEL_FOUR -> superStructure | |
| .coralLevelFour(soloController.rightBumper()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_THREE -> superStructure | |
| .coralLevelThree(soloController.rightBumper()) | |
| .coralLevelThree(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_TWO -> superStructure | |
| .coralLevelTwo(soloController.rightBumper()) | |
| .coralLevelTwo(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_ONE -> superStructure | |
| .coralLevelOne(soloController.rightBumper()) | |
| .coralLevelOne(soloController.rightBumper(), () -> AutoAlign.poseInPlaceL1R()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| }; | |
| } | |
| private Command getSoloCoralBranchHeightCommand(BooleanSupplier l1PoseSupplier) { | |
| return switch (branchHeight) { | |
| case CORAL_LEVEL_FOUR -> superStructure | |
| .coralLevelFour(soloController.rightBumper()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_THREE -> superStructure | |
| .coralLevelThree(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_TWO -> superStructure | |
| .coralLevelTwo(soloController.rightBumper(), () -> AutoAlign.poseInPlace()) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| case CORAL_LEVEL_ONE -> superStructure | |
| .coralLevelOne(soloController.rightBumper(), l1PoseSupplier) | |
| .andThen(() -> soloScoringMode = soloScoringMode.NO_GAME_PIECE); | |
| }; | |
| } |
| /* Trigger readyToScore = new Trigger(() -> AutoAlign.poseInPlace()); | ||
| readyToScore.onTrue( | ||
| Commands.runOnce(() -> soloController.setRumble(RumbleType.kBothRumble, 1.0))); | ||
| readyToScore.onFalse( | ||
| Commands.runOnce(() -> soloController.setRumble(RumbleType.kBothRumble, 0.0))); | ||
| Commands.runOnce(() -> soloController.setRumble(RumbleType.kBothRumble, 0.0))); */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| root = mech.getRoot("climber", 0.5 + Units.inchesToMeters(5.5), Units.inchesToMeters(19.5)); | ||
| SmartDashboard.putData("Mechanism", mech); | ||
| m_elevator = | ||
| root.append(new MechanismLigament2d("elevator", 1, 90, 2, new Color8Bit(Color.kRed))); | ||
| var pivot = | ||
| m_elevator.append( | ||
| new MechanismLigament2d( | ||
| "pivot offset", Units.inchesToMeters(4), -90, 2, new Color8Bit(Color.kDarkRed))); | ||
| m_wrist = | ||
| pivot.append( | ||
| new MechanismLigament2d( | ||
| "wrist", Units.inchesToMeters(14.5), 270, 6, new Color8Bit(Color.kFirstRed))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initialization of the Mechanism2d uses several magic numbers for dimensions (e.g., 5.5, 19.5, 4, 14.5) and visualization properties (e.g., 1, 2, 90, -90, 270, 6). It would be better to define these as named constants in a relevant constants class (e.g., SubsystemConstants or a new RobotDimensions class). This improves readability and makes it easier to update these values.
| import frc.robot.Robot; | ||
| import frc.robot.sensors.ArmSensor; | ||
| import frc.robot.subsystems.auto.AutoLogic; | ||
| import jdk.jfr.Timestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private StructPublisher<Pose3d> elevatorPose3d = NetworkTableInstance.getDefault().getStructTopic("elevator/heightPose", Pose3d.struct).publish(); | ||
| public StructPublisher<Pose3d> TESTpose = NetworkTableInstance.getDefault().getStructTopic("debug/TEST", Pose3d.struct).publish(); | ||
| //public StructPublisher<Pose3d> TESTpose2 = NetworkTableInstance.getDefault().getStructTopic("debug/TEST2", Pose3d.struct).publish(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section contains commented-out code and inconsistent formatting. The commented-out publisher for TESTpose2 should be removed if it's not needed. Also, the extra spaces before StructPublisher can be removed for consistent formatting.
| private StructPublisher<Pose3d> elevatorPose3d = NetworkTableInstance.getDefault().getStructTopic("elevator/heightPose", Pose3d.struct).publish(); | |
| public StructPublisher<Pose3d> TESTpose = NetworkTableInstance.getDefault().getStructTopic("debug/TEST", Pose3d.struct).publish(); | |
| //public StructPublisher<Pose3d> TESTpose2 = NetworkTableInstance.getDefault().getStructTopic("debug/TEST2", Pose3d.struct).publish(); | |
| private StructPublisher<Pose3d> elevatorPose3d = NetworkTableInstance.getDefault().getStructTopic("elevator/heightPose", Pose3d.struct).publish(); | |
| public StructPublisher<Pose3d> TESTpose = NetworkTableInstance.getDefault().getStructTopic("debug/TEST", Pose3d.struct).publish(); |
| return runOnce( | ||
| () -> { | ||
| if (hasBeenZeroed) { | ||
| System.out.println("Setting elevator level to: " + pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| double smoothedAngleZ = 0.4; | ||
| double smoothingFactor = 0.1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!Robot.getInstance().sensors.armSensor.booleanInClaw()) { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| double smoothingFactor = 0.5;// Percentage Scaler | ||
| double bottomZ = 0.2; | ||
| double topZ = 1.55; | ||
| double minPos = 0.0; | ||
| double maxPos = 37.5; | ||
| double targetZ = (bottomZ + ((curPos - minPos) / (maxPos - minPos)) * (topZ - bottomZ)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.