Created odometry for Drivetrain.java#33
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds odometry functionality to the Drivetrain subsystem, enabling the robot to track its position on the field using wheel encoders and a gyroscope.
Key Changes:
- Added MecanumDriveOdometry with gyroscope integration to track robot pose
- Updated Drivetrain constructor to require a GyroEx parameter
- Added kinematics constants for mecanum drive wheel positions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| TeamCode/src/main/java/org/firstinspires/ftc/teamcode/subsystems/Drivetrain.java | Integrated odometry tracking with encoder readings, gyroscope input, and periodic pose updates |
| TeamCode/src/main/java/org/firstinspires/ftc/teamcode/OpModes/PrimaryOpMode.java | Instantiated RevIMU gyroscope and passed it to the Drivetrain constructor |
| TeamCode/src/main/java/org/firstinspires/ftc/teamcode/Constants.java | Added wheel location constants and MecanumDriveKinematics for odometry calculations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| @Override | ||
| public void periodic(){ |
There was a problem hiding this comment.
Missing space after method name - should be periodic() { for consistency with Java style conventions.
| public void periodic(){ | |
| public void periodic() { |
| drive.driveRobotCentric(strafe, forward, rotate); | ||
| } | ||
|
|
||
| public Pose2d getPose() {return pose;} |
There was a problem hiding this comment.
Missing space after getPose() and before {. Should be formatted as public Pose2d getPose() { return pose; } or split across multiple lines for consistency.
| public Pose2d getPose() {return pose;} | |
| public Pose2d getPose() { return pose; } |
| import com.arcrobotics.ftclib.geometry.Rotation2d; | ||
| import com.arcrobotics.ftclib.hardware.GyroEx; | ||
| import com.arcrobotics.ftclib.hardware.motors.Motor; | ||
| import com.arcrobotics.ftclib.kinematics.HolonomicOdometry; |
There was a problem hiding this comment.
This import is unused and should be removed.
| import com.arcrobotics.ftclib.kinematics.HolonomicOdometry; |
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; | ||
|
|
There was a problem hiding this comment.
These imports are unused and should be removed.
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; |
| private final MecanumDrive drive; | ||
| private final MecanumDriveOdometry odometry; | ||
| private final GyroEx gyroscope; | ||
| private Pose2d pose; |
There was a problem hiding this comment.
The pose field is not initialized and will be null until periodic() is called. This will cause a NullPointerException if getPose() is called before the first periodic() execution. Consider initializing it in the constructor: pose = new Pose2d(0.0, 0.0, new Rotation2d());
| new Translation2d(length/2, 0.381); | ||
| public static final Translation2d frontRightLocation = | ||
| new Translation2d(length/2, -0.381); | ||
| public static final Translation2d backLeftLocation = | ||
| new Translation2d(-length/2, 0.381); | ||
| public static final Translation2d backRightLocation = | ||
| new Translation2d(-length/2, -0.381); |
There was a problem hiding this comment.
The width variable is defined but never used. The wheel locations use hardcoded values (0.381) instead. Consider using width/2 consistently:
new Translation2d(length/2, width/2); // frontLeft
new Translation2d(length/2, -width/2); // frontRight
new Translation2d(-length/2, width/2); // backLeft
new Translation2d(-length/2, -width/2); // backRightThis ensures consistency and makes the code easier to maintain if the robot dimensions change.
| new Translation2d(length/2, 0.381); | |
| public static final Translation2d frontRightLocation = | |
| new Translation2d(length/2, -0.381); | |
| public static final Translation2d backLeftLocation = | |
| new Translation2d(-length/2, 0.381); | |
| public static final Translation2d backRightLocation = | |
| new Translation2d(-length/2, -0.381); | |
| new Translation2d(length/2, width/2); | |
| public static final Translation2d frontRightLocation = | |
| new Translation2d(length/2, -width/2); | |
| public static final Translation2d backLeftLocation = | |
| new Translation2d(-length/2, width/2); | |
| public static final Translation2d backRightLocation = | |
| new Translation2d(-length/2, -width/2); |
| import com.arcrobotics.ftclib.hardware.GyroEx; | ||
| import com.arcrobotics.ftclib.hardware.RevIMU; | ||
| import com.qualcomm.robotcore.eventloop.opmode.TeleOp; | ||
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; |
There was a problem hiding this comment.
This import is unused and should be removed.
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; |
| public static final String brMotorName = "backRightDrive"; | ||
|
|
||
| private static final double length = 0.2873375; | ||
| private static final double width = 0.3444875; |
There was a problem hiding this comment.
Extra space before double - should be a single space for consistency.
| private static final double width = 0.3444875; | |
| private static final double width = 0.3444875; |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private final MecanumDrive drive; | ||
| private final MecanumDriveOdometry odometry; | ||
| private final GyroEx gyroscope; | ||
| private Pose2d pose; |
There was a problem hiding this comment.
The pose field is not initialized in the constructor, which means it will be null until the first periodic() call. This could cause a NullPointerException if getPose() is called before periodic() runs. Initialize pose in the constructor with the initial pose value: this.pose = new Pose2d(0.0, 0.0, new Rotation2d());
| bLE.getRate(), bRE.getRate() | ||
| ); | ||
|
|
||
| pose = odometry.updateWithTime(System.currentTimeMillis()/1000.0, gyroscope.getRotation2d(), wheelSpeeds); |
There was a problem hiding this comment.
The time calculation System.currentTimeMillis()/1000.0 performs integer division before converting to double, potentially losing precision. Use System.currentTimeMillis()/1000.0 which should work, but consider using System.nanoTime() for better precision: System.nanoTime() / 1e9 or explicitly cast to double: (double)System.currentTimeMillis() / 1000.0
| import com.arcrobotics.ftclib.hardware.GyroEx; | ||
| import com.arcrobotics.ftclib.hardware.RevIMU; | ||
| import com.qualcomm.robotcore.eventloop.opmode.TeleOp; | ||
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; |
There was a problem hiding this comment.
The import com.qualcomm.robotcore.hardware.IntegratingGyroscope is unused and should be removed.
| import com.qualcomm.robotcore.hardware.IntegratingGyroscope; |
| public static final String brMotorName = "backRightDrive"; | ||
|
|
||
| private static final double length = 0.2873375; | ||
| private static final double width = 0.3444875; |
There was a problem hiding this comment.
Extra space between final and double should be removed for consistency with the line above.
Before issuing a pull request: