Skip to content
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

[DRAFT] Use 3d Geometry for Odometry classes / Pose Estimation #4943

Closed

Conversation

jlmcmchl
Copy link
Contributor

WIP. Java odometry tests pass, but pose estimation tests do not pass. Some data collected suggests that the theta error doesn't appear to be converging, which would certainly cause bad poses to never converge. When working on #4668 it seemed clear that in order for the position to converge, the rotation has to converge as well. It's clear that the position is attempting to converge, but from the data I have so far it doesn't look like any significant adjustment is being made to the pose rotation.

Posted for visibility and feedback on naming choices. Three functions needed new names for 3d geometry:

  • getPoseMeters()
  • getEstimatedPose()
  • setVisionMeasurementStdDevs()
    • This one is also because it takes a different size Vector (6 entries vs 3), but java generics don't provide any additional type hints to the compiler. From an ABI standpoint they look the same.

Plan is to return to this later this weekend or early next week.

* numbers to trust global measurements from vision less. This matrix is in the form [x, y,
* theta]ᵀ, with units in meters and radians.
*/
public void setVisionMeasurementStdDevs3d(Matrix<N4, N1> visionMeasurementStdDevs) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have four elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use the Rotation times(double scalar) function to scale the rotation component of the twist, but I could scale each component of the rvec individually I suppose. Would that be preferred over one scalar used for the rotation component?

Copy link
Member

@calcmogul calcmogul Jan 15, 2023

Choose a reason for hiding this comment

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

What you did makes sense. The docs for each element needs to be updated though, because it still says there's three elements: x, y, and heading; instead of x, y, z, and angle (used for all three rotation axes).

Copy link
Contributor Author

@jlmcmchl jlmcmchl Jan 16, 2023

Choose a reason for hiding this comment

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

Did some additional reading and came across this - https://en.wikipedia.org/wiki/Rotation_formalisms_in_three_dimensions#Angle-Angle-Angle

This would seem to suggest we should go ahead with 3 orthogonal std dev inputs for rotation, because they're measured independently on orthogonal axes. This makes some sense; gyroscopes typically measure 3 independent axes so the input signals themselves are not coupled tightly, and could behave differently.

I'll update docs for the new functions to recognize std dev inputs as x, y, z, roll, pitch, yaw. Think I should change heading to yaw in the 2d function docs as well?

Copy link
Member

Choose a reason for hiding this comment

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

Does scaling the twist elements individually work as expected? They aren't actually angles afterall, so they don't match the stddev units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've decided to revert the change from earlier back to [x, y, z, heading] for stdev parameters + kalman gain after speaking with some people who know more about 3d rotation than I do. This hasn't yet led to the new 3d test passing, unfortunately.

As per discussion on discord, it appears that pose.exp(pose.log(other)) == other doesn't appear to be held true within a reasonable margin of error - the translation component of the
computed pose is always correct, but the heading differs significantly between the computed pose and the expected result. Data demonstrating this as attached.

robot_vision_data.csv

columns are presented for the following:

  • time of measurement within test
  • robot_pose (sampled pose from pose buffer)
  • vision_pose (input pose to addVisionMeasurement function)
  • twist (robot_pose.log(vision_pose))
  • exp_pose ('robot_pose.exp(twist)`)
  • pose_error (Translation3d between exp_pose and vision_pose)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's the Transform3d applying things in the wrong order?

  // Get left Jacobian of SO3. See first line in right column of
  // http://asrl.utias.utoronto.ca/~tdb/bib/barfoot_ser17_identities.pdf
  Matrixd<3, 3> J;
  if (thetaSq < 1E-9 * 1E-9) {
    // J = I + 0.5ω
    J = Matrixd<3, 3>::Identity() + 0.5 * Omega;
  } else {
    double theta = std::sqrt(thetaSq);
    // J = I + (1 − std::cos(θ))/θ² ω + (θ − std::sin(θ))/θ³ ω²
    J = Matrixd<3, 3>::Identity() + (1.0 - std::cos(theta)) / thetaSq * Omega +
        (theta - std::sin(theta)) / (thetaSq * theta) * OmegaSq;
  }

  // Get translation component
  Vectord<3> translation =
      J * Vectord<3>{twist.dx.value(), twist.dy.value(), twist.dz.value()};

  const Transform3d transform{Translation3d{units::meter_t{translation(0)},
                                            units::meter_t{translation(1)},
                                            units::meter_t{translation(2)}},
                              Rotation3d{twist.rx, twist.ry, twist.rz}};

  return *this + transform;

@Starlight220 Starlight220 linked an issue Jan 15, 2023 that may be closed by this pull request
@jlmcmchl jlmcmchl marked this pull request as ready for review January 19, 2023 01:18
@jlmcmchl jlmcmchl requested a review from a team as a code owner January 19, 2023 01:18
@calcmogul
Copy link
Member

The command tests don't seem to include wpimath JNI, so Matrix.pow(double) isn't available (unless we add it), and that's what fails the build. It was originally added for fractional matrix powers in LinearQuadraticRegulator.latencyCompensate(). We could replace the pow(2) in Pose3d.exp() with just multiplying the matrix by itself to avoid the JNI dependency.

@Starlight220
Copy link
Member

We should add the JNI for the tests.

@calcmogul
Copy link
Member

Sure, but we can do that in a separate PR since this one no longer requires it.

@jlmcmchl jlmcmchl force-pushed the jmcmichael/3d-pose-estimation branch from 94f45bd to 6d3328f Compare February 7, 2023 00:18
@calcmogul calcmogul requested a review from a team as a code owner March 24, 2023 15:49
@calcmogul
Copy link
Member

@jlmcmchl Is this PR going to require a complete rewrite? I recall you mentioning wanting to do an alternative design.

@jlmcmchl
Copy link
Contributor Author

jlmcmchl commented Sep 1, 2023

Yes - the bulk of the rewrite work is begun on #5473. The rewrite I previously referred to has already been applied in #5355

@calcmogul
Copy link
Member

calcmogul commented Nov 22, 2023

OBE by kinematics rewrite in #5355. Needs a rewrite.

@calcmogul calcmogul closed this Nov 22, 2023
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.

Generalize the odometry and pose estimator classes to 3D
3 participants