Skip to content

AP_Compass: fixed zero compass diagonals #22666

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

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Jan 16, 2023

this fixes a regression from 4.2 to 4.3.

previously we automatically set the diagnoals to 1,1,1 if they were 0,0,0. We don't do that any more. I was helping a user who had copied an old config with 0,0,0 for diagonals and did not understand two things:

  • why the compass did not work in 4.3
  • why large vehicle mag cal didn't work to fix it

tested on CubeOrange

this fixes a regression from 4.2 to 4.3.

previously we automatically set the diagnoals to 1,1,1 if they were
0,0,0. We don't do that any more. I was helping a user who had copied
an old config with 0,0,0 for diagonals and did not understand two
things:

- the compass did not work in 4.3
- large vehicle mag cal didn't work
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

LGTM

.. not scope-creeping. bit the declaration of diagonals and offdiagonals should be moved down into this code block being modified.

);

mag = mat * mag;
if (!diagonals.is_zero()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any element being zero would be bad in here. I wonder if it would be possible to get into that situation with loaded parameters.

Since this equivalent to what we had pre-regression, it really doesn't matter.

@rmackay9
Copy link
Contributor

rmackay9 commented Jan 16, 2023

For accountability it would be good to understand how the mistake was made and if there is a way this kind of error could be avoided in the future. To err is human of course but we should also try and improve.

@IamPete1
Copy link
Member

IamPete1 commented Jan 16, 2023

The issues was added in PR to move to doing Vector3f defaults in the table. We used to always set to 1 manually AP_Param didn't handle defaulting of vector parameters. Since we were so aggressive in setting to 1 if 0 prior to that PR i'm not sure how the user would have managed to save a param set where they were zero.

I did call this out as a possibility in the PR: #21290 (comment)

@tridge tridge merged commit 863b4bf into ArduPilot:master Jan 17, 2023
@rmackay9
Copy link
Contributor

rmackay9 commented Feb 9, 2023

This is included in Copter-4.3.4-rc1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants