Skip to content

Conversation

@stephenjust
Copy link
Contributor

Why are we doing this?

We're seeing some weird jumpiness in the pose estimators. I've tried to reason through the Pose Subsystem to make sure that the pose we care about is always updated properly.

Whats changing?

Questions/notes for reviewers

How this was tested

  • tested on robot
  • tested in simulator
  • unit tests added

Video/screenshots (from simulator or live robot)


PR feedback legend

Symbol Meaning
⭐ ⭐ ⭐ must be addressed
⭐ ⭐ should be addressed
something to consider, a good idea

Copilot AI review requested due to automatic review settings April 4, 2025 19:37
@stephenjust stephenjust requested a review from a team as a code owner April 4, 2025 19:37
@stephenjust stephenjust requested review from rokadias and removed request for Copilot April 4, 2025 19:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/main/java/competition/subsystems/pose/PoseSubsystem.java:133

  • [nitpick] Consider renaming 'useDeadwheelsForVisionPose' to more clearly indicate that it determines whether the full deadwheel or full swerve odometry is used, as the current naming might be slightly ambiguous.
return (PoseEstimator<T>) (this.useDeadwheelsForVisionPose.get() ? this.fullDeadwheelOdometry : this.fullSwerveOdometry);

src/main/java/competition/subsystems/pose/PoseSubsystem.java:147

  • [nitpick] Adding inline documentation to clarify the role and intended usage of the secondary estimator would help maintain clarity, given the close resemblance of its logic to the primary estimator.
private <T> PoseEstimator<T> getSecondaryPoseEstimator() {

@Team488 Team488 deleted a comment from Copilot AI Apr 4, 2025
Copy link
Contributor

@rokadias rokadias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only questionable thing is the update via wheelpositions as its untested persay.
LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants