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

AP_Math: rename earth_to_body to body_to_earth #28178

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

robertlong13
Copy link
Collaborator

@robertlong13 robertlong13 commented Sep 21, 2024

This function actually converts a vector represented in the body frame to the earth frame. The previous name was wrong. This function matches the behavior of AHRS::body_to_earth. Here is a lua script and an example log of it running in SITL to demonstrate that fact:
compare_ahrs_and_quat.zip

The confusion stems from the fact that the rotation that you apply to rotate frame A such that it becomes frame B is the opposite of the rotation you apply to convert a vector represented in frame A to be represented in frame B. Or, in other words, the rotation that you apply to rotate frame A such that it becomes frame B is the rotation you apply to convert a vector represented in frame B to be represented in frame A.

Interestingly, the docs for the lua binding correctly described what the function was doing (applying the rotation described by the quaternion to a vector), but the original function description described the opposite (converting a body-frame vector to earth-frame).

Let's do an example in 2D. XE/YE is the Earth frame coordinate system. Our vehicle is pointing at a heading of 45 degrees. If we rotate our Earth reference frame XE/YE 45 degrees to the right, we get the body reference frame XB/YB.

image

One might call this rotation of 45 degrees to the right "a rotation from E to B". Someone might call a rotation matrix that converts a representation of a vector in frame E to one in frame B "a rotation from E to B". Those are exact opposites of each other. I'd argue the former is more correct, but because it's so easy to get confused, we need to be extremely clear what we mean. I have cleaned up some comments in the code that were ambiguous or flat-out wrong.

Unfortunately, there are many other cases remaining (stuff like get_rotation_body_to_ned, which I think should be renamed to get_body_to_ned or maybe get_body_to_ned_matrix and get a comment explaining very clearly what it does and how it's used). If people are generally on-board with cleaning those up, I'd be happy to in another PR if this one gets approved.

@robertlong13
Copy link
Collaborator Author

@robertlong13 robertlong13 force-pushed the pr/quat_earth_to_body_fix branch from 6d9c60a to ac5de3b Compare September 21, 2024 20:52
@@ -136,7 +136,7 @@ void ModeFlowHold::flowhold_flow_to_angle(Vector2f &bf_angles, bool stick_input)
// compensate for height, this converts to (approx) m/s
sensor_flow *= constrain_float(height_estimate, height_min, height_max);

// rotate controller input to earth frame
Copy link
Contributor

Choose a reason for hiding this comment

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

Txs for the renaming.

Is this comment an improvement though? I think it's rotating the flow sensor's input from body-frame into earth-frame. I guess it's just a difference between "rotate" and "convert".

Anyway, I'm actually not a fan of this mode in any case, it's quite sensor and frame specific and so it leads to a fair number of support calls on the forums.

Copy link
Collaborator Author

@robertlong13 robertlong13 Sep 22, 2024

Choose a reason for hiding this comment

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

I do believe this is an improvement because "rotate to earth frame" can mean two opposite things depending on the context. The rewording is unambiguous and clear.

Now, for this particular comment in flowhold, it's pretty obvious what's going on, so this one comment arguably didn't need the rewrite.

But let's take an example of one of the other comments which I think is dangerously ambiguous:

// rotate a 2D vector from earth frame to body frame
Vector2f earth_to_body2D(const Vector2f &ef_vector) const;

If the body frame is rotated 90 degrees to the right, does this function rotate the input vector 90 degrees to the right? Or does it rotate it 90 degrees to the left? It's ambiguous. It depends on if the author means active rotation transformation or passive. In this case, if you carefully inspect what the function is doing, they mean passive, so rotating 90 degrees to the left. It's way clearer to just say that you are converting an earth-frame representation to a body-frame representation; yes you apply a rotation transformation to achieve that conversion, but I think "rotate" is a very tricky word in this context.

@rmackay9
Copy link
Contributor

Thanks very much for this, we have very few users of this call so I think this should be a relatively safe change. If there are any uses of it (plane acrobatics script?) we probably have to test they still work OK.

@robertlong13
Copy link
Collaborator Author

Thanks very much for this, we have very few users of this call so I think this should be a relatively safe change. If there are any uses of it (plane acrobatics script?) we probably have to test they still work OK.

Yup, had to change the plane aerobatics script to reflect the rename. The autotest still works, here is the log of the airshow.
SUPERAIRSHOW.zip

@IamPete1
Copy link
Member

IamPete1 commented Sep 23, 2024

You could add your example into the math test that we run as part of CI. https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Scripting/tests/math.lua

We could add keep the old function in the docs and mark it as deprecated and add a comment to point people to the new one.

That would be something like this:

-- Applies rotation to vector argument
---@deprecated -- function name incorrect for function use body_to_earth for the same behavior 
---@param vec Vector3f_ud
function Quaternion_ud:earth_to_body(vec) end

That gets us this in VSCode:
image

The language server checks pick it up:

ardupilot/libraries/AP_Scripting/examples/simple_loop.lua:16:1
        Code: deprecated
        Deprecated.(function name incorrect for function use body_to_earth for the same behavior)

However we might need some changes so that the docs don't generate a error for a unexpected definition in CI.

The binding generator also has the ability to print a one time error when a binding is first used. That would allow a "softer" deprecation where the code will still work.

@Georacer
Copy link
Contributor

The quaternion class isn't defined specifically in any particular direction: Earth to body or body to Earth.

But it does contain a .from_euler constructor and Euler angles are pretty well defined. As per your link, the Euler angles are an "passive" transformation from the Earth frame into the body frame, in the yaw, pitch, roll order.

Thus, from this starting point and the method definition, (similar to Wikipedia's), we can deduce that the quaternion used in ArduPilot is in-line with the textbook definition.

A point of attention: The operation that uses a quaternion to transform an Earth-frame vector into a body-frame vector is (Stevens, B. L., Lewis, F. L., & E.N. Johnson. (2016). Aircraft Control and Simulation, Wiley, p49):

$v_{body} = q^{-1} * v_{Earth} * q$

We typically write this quaternion as $q_{b/e}$ (To body from Earth).
Whereas

$v_{Earth} = q_{b/e} * v_{body} * q_{b/e}^{-1}$

Pay attention to the position of the $^{-1}$ exponent.

Now, such a multiplication between a quaternion and a vector isn't defined in C++, so we had rolled out the earth_to_body method, which uses a rotation matrix as an intermediate step.

That rotation matrix conforms to the constructions of a body to Earth rotation matrix $m$, as seen in Wikipedia, or as the transpose/inverse of the Earth to body matrix seen in (Stevens2016, p53).

Thus, the following will be true:

$v_{e}= m \cdot v_b$

Concluding, I agree with the rename.

Copy link
Contributor

@priseborough priseborough left a comment

Choose a reason for hiding this comment

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

We could quibble whether it is necessary to eliminate use of the 'rotate' word in the context of a function that clearly converts a vector from one frame to another, however these changes do make a much clearer distinction between the rotation of an axis frame vs transformation that the rotation performs on a vector and will make it less likely for a developer to misuse one of the functions. I like the consistent terminology used.

@robertlong13 robertlong13 force-pushed the pr/quat_earth_to_body_fix branch from ac5de3b to 437fe03 Compare October 15, 2024 07:10
@robertlong13 robertlong13 force-pushed the pr/quat_earth_to_body_fix branch from 437fe03 to 6ddefa3 Compare October 16, 2024 06:30
@robertlong13 robertlong13 force-pushed the pr/quat_earth_to_body_fix branch from 6ddefa3 to 9273b60 Compare October 16, 2024 12:42
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.

6 participants