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

Mag cal: automatically disable internal mags if external ones are available #24316

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

bresch
Copy link
Member

@bresch bresch commented Feb 11, 2025

Solved Problem

Magnetometers inside the autopilot are almost always highly affected by varying magnetic fields generated by the drone due to their location inside the drone.
The internal mag is useful for initial autopilot bench testing, before it gets integrated inside a drone, and when performing the calibration of the external one to automatically detect the rotation parameter. However, it then only causes issues as it often triggers false warnings (mag inconsistency) and in case of an in-air failure of the external mag, a fallback to the internal mag can lead to worse performance than not using a mag at all.

Solution

Automatically disable all internal mags just after calibration if an external one is available.

Changelog Entry

For release notes:

New parameter: -
Documentation: todo

Test coverage

Bench tested

@bresch bresch requested a review from dagar February 11, 2025 11:06
@bresch bresch self-assigned this Feb 11, 2025
Copy link

github-actions bot commented Feb 11, 2025

🔎 FLASH Analysis

px4_fmu-v5x [Total VM Diff: 152 byte (0.01 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%    +152  +0.0%    +152    .text
  +1.0%     +64  +1.0%     +64    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.8%     +60  +0.8%     +60    ../../src/modules/commander/mag_calibration.cpp
  +2.2%     +28  +2.2%     +28    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +0.0%      +5  +0.0%      +5    [section .text]
  +0.2%      +3  +0.2%      +3    ../../src/systemcmds/ver/ver.cpp
  -0.0%      -4  -0.0%      -4    ../../src/modules/control_allocator/ControlAllocator.cpp
  -0.1%      -4  -0.1%      -4    ../../src/modules/ekf2/EKF/yaw_estimator/EKFGSF_yaw.cpp
+0.0%     +56  [ = ]       0    .debug_abbrev
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
-0.0%      -8  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
+0.0%     +16  [ = ]       0    .debug_frame
+0.0%    +442  [ = ]       0    .debug_info
  +0.1%     +27  [ = ]       0    ../../src/lib/sensor_calibration/Magnetometer.cpp
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
  +0.1%     +50  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +0.2%    +150  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +0.0%     +27  [ = ]       0    ../../src/modules/mag_bias_estimator/MagBiasEstimator.cpp
  +0.0%     +27  [ = ]       0    ../../src/modules/sensors/sensors.cpp
  +0.1%    +138  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.0%     +27  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
+0.0%    +334  [ = ]       0    .debug_line
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
  +0.6%     +30  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +1.1%    +165  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +0.9%    +168  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  -0.0%      -2  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
  -0.2%      -2  [ = ]       0    task/task_cancelpt.c
+0.0%    +183  [ = ]       0    .debug_loc
  -0.0%     -15  [ = ]       0    ../../src/modules/commander/Commander.cpp
  -1.3%     -86  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +0.4%    +113  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  -0.1%     -15  [ = ]       0    ../../src/modules/mag_bias_estimator/MagBiasEstimator.cpp
  +0.1%     +15  [ = ]       0    ../../src/modules/sensors/sensors.cpp
  +0.5%    +141  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.0%     +30  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
+0.0%    +233  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  +3.3%     +64  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +1.2%     +80  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +1.3%     +96  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +1.6%      +1  [ = ]       0    task/task_cancelpt.c
+0.0%     +64  [ = ]       0    .debug_str
  +1.4%     +64  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
-1.5%    -152  [ = ]       0    [Unmapped]
+0.0% +1.29Ki  +0.0%    +152    TOTAL

px4_fmu-v6x [Total VM Diff: 144 byte (0.01 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%    +144  +0.0%    +144    .text
  +1.0%     +64  +1.0%     +64    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.8%     +60  +0.8%     +60    ../../src/modules/commander/mag_calibration.cpp
  +2.2%     +28  +2.2%     +28    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  -0.0%      -4  -0.0%      -4    ../../src/modules/control_allocator/ControlAllocator.cpp
  -0.1%      -4  -0.1%      -4    ../../src/modules/ekf2/EKF/yaw_estimator/EKFGSF_yaw.cpp
+0.0%     +56  [ = ]       0    .debug_abbrev
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
-0.0%      -8  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
+0.0%     +16  [ = ]       0    .debug_frame
+0.0%    +442  [ = ]       0    .debug_info
  +0.1%     +27  [ = ]       0    ../../src/lib/sensor_calibration/Magnetometer.cpp
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
  +0.1%     +50  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +0.2%    +150  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +0.0%     +27  [ = ]       0    ../../src/modules/mag_bias_estimator/MagBiasEstimator.cpp
  +0.0%     +27  [ = ]       0    ../../src/modules/sensors/sensors.cpp
  +0.1%    +138  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.0%     +27  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
+0.0%    +342  [ = ]       0    .debug_line
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
  +0.6%     +30  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +1.1%    +165  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +0.9%    +168  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  -0.0%      -2  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
  +0.5%      +6  [ = ]       0    task/task_cancelpt.c
+0.0%    +198  [ = ]       0    .debug_loc
  -0.0%     -15  [ = ]       0    ../../src/modules/commander/Commander.cpp
  -1.3%     -86  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +0.4%    +113  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +0.1%     +15  [ = ]       0    ../../src/modules/sensors/sensors.cpp
  +0.5%    +141  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  +0.0%     +30  [ = ]       0    src/modules/mavlink/modules__mavlink_unity.cpp
+0.0%    +230  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  +3.3%     +64  [ = ]       0    ../../src/modules/commander/HealthAndArmingChecks/checks/magnetometerCheck.cpp
  +1.2%     +80  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
  +1.3%     +96  [ = ]       0    ../../src/modules/sensors/vehicle_magnetometer/VehicleMagnetometer.cpp
  -3.0%      -2  [ = ]       0    task/task_cancelpt.c
+0.0%     +64  [ = ]       0    .debug_str
  +1.4%     +64  [ = ]       0    ../../src/modules/commander/mag_calibration.cpp
-0.2%    -144  [ = ]       0    [Unmapped]
+0.0% +1.31Ki  +0.0%    +144    TOTAL

Updated: 2025-02-14T14:54:14

@bresch bresch requested a review from dagar February 11, 2025 19:24
@dagar
Copy link
Member

dagar commented Feb 11, 2025

Nice idea, I just wonder if we should do something basic to validate the external is alive and "good"?

@hamishwillee
Copy link
Contributor

@bresch This will require docs update. The whole calibration doc will need some minor work, but in particular we will need to update https://docs.px4.io/main/en/config/compass.html#additional-calibration-configuration and the sub topic on disabling a compass

My assumption is that if the internal compass is disabled after calibration it is still calibrated. So the section would change from "Disable a Compass" to "Re-enable a compass" and include explanation that often this is worse than nothing so explain when/why you would do it.

@dakejahl
Copy link
Contributor

This makes sense, I think many users already disable the internal mag after calibration anyway. The cherry on top would be a sensor status tab in QGC Vehicle page. Right now it's not obvious to users what sensors are enabled, functional, and which calibration parameters corresponds to which device.

@bresch bresch force-pushed the pr-disable_internal_mag branch 2 times, most recently from 5458fda to ae31c14 Compare February 13, 2025 16:00
@bresch
Copy link
Member Author

bresch commented Feb 13, 2025

The cherry on top would be a sensor status tab in QGC Vehicle page

oh yes, that could be nice to have

which calibration parameters corresponds to which device.

Yes, if QGC could show you something like "Mag 1 (external): IST8310" and the calibration values.

@bresch bresch requested a review from dagar February 14, 2025 09:38
sys_has_mag defines the required number of mags; if one mag is needed,
it can be any instance, not necessarily mag 0
@bresch bresch force-pushed the pr-disable_internal_mag branch from fadeba9 to d7e3a81 Compare February 14, 2025 14:49
Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

Code looks clean and I bench tested it a bit.

@bresch bresch merged commit fe5c887 into main Feb 14, 2025
62 of 63 checks passed
@bresch bresch deleted the pr-disable_internal_mag branch February 14, 2025 15:12
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.

5 participants