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

Reduce pre flight mag inconsistency test false positives #12327

Closed
wants to merge 3 commits into from

Conversation

priseborough
Copy link
Contributor

Work in progress for comment and review.

Raised in response to #12272.

@MaEtUgR are you able to test on your setup(s) and provide feedback? TXS

@dagar
Copy link
Member

dagar commented Jun 25, 2019

Under what circumstances would you advise relaxing the thresholds as an acceptable solution?

I guess what I'm really wondering is if we want to expose these tuning knobs versus identifying a mag that might as well be disabled entirely or a switch to skip the check entirely (what most users are actually doing when "tuning" the parameter).

* @decimal 2
* @increment 0.05
*/
PARAM_DEFINE_FLOAT(COM_ARM_MAG, 0.15f);
PARAM_DEFINE_FLOAT(COM_ARM_MAG_V, 0.7f);
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 there need to be a different angle for inclination and declination? As far as I understood the check is to find out if the mag is mounted the wrong way or the sensor orientation is set wrong. Not?


// calculate the horizontal angle between the two vectors to estimate the difference in magnetic heading thee sensors would produce
matrix::Quatf q_delta(mag_hvec_selected, mag_hvec_alternate);
matrix::Vector3f ang_hvec_delta = q_delta.to_axis_angle();
Copy link
Member

Choose a reason for hiding this comment

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

to_axis_angle() is "XXX DEPRECATED"

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

I'm sorry but this check seems too complicated to me.
Please allow me to make a suggestion and let you have a look if it's viable.

@@ -1249,10 +1279,12 @@ void VotedSensorsUpdate::calc_mag_inconsistency(sensor_preflight_s &preflt)

// skip check if less than 2 sensors
if (check_index < 1) {
preflt.mag_inconsistency_ga = 0.0f;
preflt.mag_inconsistency_h = 0.0f;
preflt.mag_inconsistency_v = 0.0f;

} else {
// get the vector length of the largest difference and write to the combined sensor struct
Copy link
Member

Choose a reason for hiding this comment

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

comment outdated

@MaEtUgR
Copy link
Member

MaEtUgR commented Jun 26, 2019

Here's my suggestion that I also successfully tested now: #12334

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

Successfully merging this pull request may close these issues.

3 participants