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

Add test for geopose heading #28091

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Sep 13, 2024

Purpose

Add a test to prove the reported heading on the geopose interface is REP-103 compliant.

This matches the proposed clarifications here, and can be used as an example in the REP.
ros-infrastructure/rep#407

@@ -41,7 +47,7 @@
CMAC_HEADING = 353


def ros_quat_to_heading(quat):
def ros_quat_to_heading_deg(quat):
# By default, scipy follows scalar-last order – (x, y, z, w)
Copy link
Contributor

Choose a reason for hiding this comment

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

scipy is a large library to pull in just for quaternions and rotations. Have you thought about transforms3d (https://matthew-brett.github.io/transforms3d/)?

Copy link
Collaborator Author

@Ryanf55 Ryanf55 Sep 13, 2024

Choose a reason for hiding this comment

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

Yea, I just saw scipy was already used a bunch of places, including in the setup script, and thought the team would prefer a library they are familiar with.

PYTHON_PKGS+=" matplotlib scipy opencv-python pyyaml"

Your link looks like a much more focused library, but the apt version is out of date:
matthew-brett/transforms3d#65

Copy link
Contributor

@srmainwaring srmainwaring left a comment

Choose a reason for hiding this comment

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

LGTM. Added notes with suggestions, but no blockers.

@@ -57,10 +63,28 @@ def validate_position_cmac(position):
and math.isclose(position.altitude, CMAC_ABS_ALT, abs_tol=1.0)
)

def wrap_360(angle):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use the same calc as AP_Math here?

float wrap_360(const float angle)
{
    float res = fmodf(angle, 360.0f);
    if (res < 0) {
        res += 360.0f;
    }
    return res;
}

The calc does not wrap 360 deg to 0, not sure if that's an issue (means the function is not invertible).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pulled this from pymavlink but added recursion so you can handle multi-wrap. If the initial heading was directly north, the comparison would get tricky, but this util is only used in this hard-coded cmac test.


Per ROS REP-103, the quaternion should report 0 when the vehicle faces east,
and 90 degrees when facing north.
Because CMAC is NNW, we expect ~97 degees.
Copy link
Contributor

Choose a reason for hiding this comment

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

./Tools/autotest/locations.txt has this for CMAC. Heading is 353 => rotation = 97 as described.

CMAC=-35.363261,149.165230,584,353

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I just had to add a tolerance because the data is through the EAHRS with noise added. Peter mentioned to set the EKF type to match the SIM so it's exact, but we don't depend on mavproxy or support params yet, so I left that out of scope and used a tolerance instead.

@tridge tridge merged commit 0cc07ac into ArduPilot:master Sep 17, 2024
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants