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: add +- 45 roll rotations, use 32 digit constants, AP_Compass support auto orientation for +-45 roll #18098

Merged
merged 10 commits into from
Jul 28, 2021

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Jul 23, 2021

  • Adds +- 45 roll rotations.
  • Adds testing of quaternion rotations.
  • Makes duplicate rotations clear and adds a test to check for duplicates
  • all rotation constants to 32 digit
  • adds support for compass auto rotation for +- 45, bit of a re-work skipping the duplicates and pitch +7
  • Test for the rotation indexer used above, this was more to learn how to add a test than because its actually useful, possibly we don't even want it in tree.

Thanks to @Gone4Dirt for helping me with the quaternions.

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

Slight review only

libraries/AP_Math/vector3.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/CompassCalibrator.cpp Show resolved Hide resolved
@@ -21,6 +21,11 @@
#include "AP_Math.h"
#include <AP_InternalError/AP_InternalError.h>

#define HALF_SQRT_2_PlUS_SQRT_2 0.92387953251128673848313610506011 // sqrt(2 + sqrt(2)) / 2
#define HALF_SQRT_2_MINUS_SQTR_2 0.38268343236508972626808144923416 // sqrt(2 - sqrt(2)) / 2
#define HALF_SQRT_HALF_TIME_TWO_PLUS_SRT_TWO 0.65328148243818828788676000840496 // sqrt((2 + sqrt(2))/2) / 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Static constexpr ftype ?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are a define because the existing HALF_SQRT_2 was, I'm not really sure the benefit either way.

@IamPete1 IamPete1 force-pushed the rotataions branch 2 times, most recently from f135265 to d3d560c Compare July 24, 2021 12:10
@IamPete1
Copy link
Member Author

I have tested auto rotation at few orientations, including the two new ones. Compass mounted physically at no rotation, then changing AHRS_ORIENTATION and calibrating. The correct compass orientation then matches the AHRS orientation.

@tridge
Copy link
Contributor

tridge commented Jul 25, 2021

@IamPete1 the problem with having all the 45 degree rotations in mag cal is it makes it more likely that the auto-orientation code will pick the wrong orientation. That code doesn't have a lot of margin.
we need to ensure the 45s are not used for mag cal.

@IamPete1
Copy link
Member Author

IamPete1 commented Jul 25, 2021

@tridge Were already doing auto orientation for the 45 deg yaw rotations, I don't see why it would be worse for roll than it is for yaw? I agree that we should not do combined 45's like 45 yaw 45 roll.

@IamPete1
Copy link
Member Author

Were also currently auto rotating some of the more wacky ones, like ROTATION_ROLL_90_PITCH_68_YAW_293 and ROTATION_ROLL_90_PITCH_180_YAW_90

@tridge
Copy link
Contributor

tridge commented Jul 25, 2021

Were already doing auto orientation for the 45 deg yaw rotations, I

for some of them, yes, but I'd prefer to remove those from the auto-rotation list rather than adding more.

@@ -63,10 +63,12 @@ enum Rotation : uint8_t {
ROTATION_ROLL_270_PITCH_270 = 35,
ROTATION_ROLL_90_PITCH_180_YAW_90 = 36,
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is the same as ROLL_270_YAW_270, admittedly that is not one that we have, but is is much easier to understate as two numbers rather than 3.

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

i think COMPASS_AUTOROT should be extended to be "only 90s" or "90s and 45s"

@IamPete1 IamPete1 force-pushed the rotataions branch 3 times, most recently from 2cb9fb5 to 17cec01 Compare July 27, 2021 16:23
@tridge tridge merged commit e5d4620 into ArduPilot:master Jul 28, 2021
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