AP_Compass: report specific failure reason when fit rejected#32757
AP_Compass: report specific failure reason when fit rejected#32757christianpetri wants to merge 1 commit into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
I think 950 is too low, that is within the range of offsets that we might see in normal operation. I think we might be better off to try and improve the reporting at the end of a calibration attempt rather than guessing at the beginning. |
f4e053b to
a428227
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @christianpetri, Thanks for this. One small thing is we don't accept merge commits so could you rebase on master to remove it? |
a6d793b to
b0f7dc6
Compare
peterbarker
left a comment
There was a problem hiding this comment.
Create an autotest
- want other compasses to keep calibrating if one of them gets a long field
- we have lots of autotests already doing calibration, steal code from there
- does this make calibration faster with a badly-affected compass?
Generally in favour of this at DevCallEU
What's the flash cost?
b0f7dc6 to
4b438b1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4b438b1 to
13e6f2d
Compare
13e6f2d to
93c2395
Compare
93c2395 to
560b8e1
Compare
|
Not keen on the short circuit stuff, but I like the improved reporting at the end of the calibration. To get the most out of this we might need some GCS changes, in mission planner you can't see messages on the compass calibration page, you would have to go and look manualy. |
560b8e1 to
09774b4
Compare
|
Dropped the early-abort in pull_sample() as requested. Kept and improved the fit_acceptable() diagnostics: each failure condition (fitness NaN, radius out of range, offsets too large, diagonal/offdiagonal out of range, fitness over tolerance) now emits its own GCS message with the actual values, so all failures are visible in a single calibration attempt rather than just the first hit. Removed const from fit_acceptable() since GCS_SEND_TEXT is a side effect. This is consistent with the pre-existing messages already in calculate_orientation() and fix_radius(). |
SITL Test PlanManual SITL verification for the
Related PRs
Setup sim_vehicle.py -v ArduCopter -f calibration --console --mapConnect Mission Planner via TCP Reset between tests Tests
Pass criteria
|
09774b4 to
4fbb08d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4fbb08d to
774fc4c
Compare
774fc4c to
88799cf
Compare
IamPete1
left a comment
There was a problem hiding this comment.
I like this! And Thanks for doing the Mission Planner fix too!
…AL_STATUS Add three new failure reason enum values (8/9/10) to MAG_CAL_STATUS, and add descriptions to existing entries 6 and 7 for consistency. Implemented in ArduPilot firmware: ArduPilot/ardupilot#32757
|
Merged in mavlink/mavlink. |
|
Hi @hamishwillee and @peterbarker, quick question about the MAVLink to ArduPilot flow. Is opening that PR sufficient, or are there additional steps we should complete first before it can be brought into ArduPilot? Thank you very much for your guidance. |
|
Hi @hamishwillee and @peterbarker, quick question about the MAVLink to ArduPilot flow. The mavlink enums from ArduPilot/mavlink#506 are now merged. The ardupilot submodule pointer still needs to be bumped to |
|
Yes. Looks like Peter merged it now. |
15ec2b8 to
bb3f8ea
Compare
bb3f8ea to
a065fb7
Compare
bda23d2 to
70dd6df
Compare
70dd6df to
94b0ca3
Compare
…l dialogs ArduPilot/ardupilot#32757 adds three new MAG_CAL_STATUS values sent in MAG_CAL_REPORT.cal_status when calibration fit is rejected: 8 = BAD_OFFSETS - offset component >= COMPASS_OFFS_MAX 9 = BAD_DIAG - diagonal/off-diagonal scaling out of range 10 = BAD_FITNESS - RMS residual exceeds tolerance Previously these fell through silently, leaving the cal dialog stuck. Changes: - Use calStatus > MAG_CAL_SUCCESS guard (future-proof for any further codes) - Track failure status per compass in lastFailureStatus dictionary - Show failure code in result label; set progress/picture to red on failure - Add unit tests covering the guard logic and wire values Note: Mavlink.cs is NOT changed - it is auto-generated. Named enum members MAG_CAL_BAD_OFFSETS/DIAG/FITNESS arrive when mavlink/mavlink#2478 merges and Mavlink.cs is regenerated. Tests use raw byte casts until then.
Physical Hardware Test ResultsTested on MatekF405-Wing with Mission Planner (using ArduPilot/MissionPlanner#3510 - MAVLink bump that includes new compass cal status values). Firmware: commit 94b0ca3 (compass-cal-early-abort branch) The PR now correctly reports specific failure reasons instead of generic "Compass calibration failed": Baseline Test: Success
Test 1: Radius Failure (Attempt 2, 74% complete)
Message: "Compass calibration failed: the radius of the fitted sphere is unrealistically small or large" Test 2: Offset Magnitude Failure (Attempt 2, 12% complete)
Message: "Compass calibration failed: offset magnitude too large. [x:408.8 y:1988.0 z:-1747.3 limit ±1800]" Notes
The specific failure reporting works as designed! Users can now understand why calibration failed and take appropriate corrective action. |
Additional Notes: Progress Stall DetectionDuring testing, I observed a scenario where calibration progress can stall indefinitely: Scenario: Magnetic Field Too StrongWhen the magnetic field is extremely strong (magnet very close to compass), the calibration can get stuck at approximately 33% progress (during step two sample collection). The field distortion is so severe that the algorithm cannot collect valid samples across the required orientations, causing the progress bar to freeze. Current Behavior
Attempted SolutionI explored adding a timeout/stall detection mechanism to abort calibration when progress stops advancing. The challenge is:
Future WorkA stall detection mechanism would be a valuable addition:
I decided to defer this work to keep the current PR focused on the core improvement (specific failure reporting when fit validation fails). The stall detection feature can be addressed in a separate PR with proper SITL testing infrastructure. Note: Users can manually cancel stalled calibrations via Mission Planner/GCS. The stall scenario requires unusually strong magnetic interference (magnet within a few cm / mm of compass). |





Summary
Extend the
CompassCalibrator::Statusenum with three new terminal failure values (FAILED_OFFSETS=8,FAILED_DIAG_SCALING=9,FAILED_RESIDUALS_HIGH=10) and refactorfit_acceptable()into a pure-queryfit_acceptable_status() constso thatMAG_CAL_REPORT.cal_statusreports the specific reason calibration was rejected instead of always returning the genericFAILED.Classification & Testing (check all that apply and add your own)
Description
Previously
fit_acceptable()returned a silentfalse— the GCS and user had no way to know why calibration was rejected. This PR maps each rejection condition to a specificStatusvalue that is transmitted in theMAG_CAL_REPORT.cal_statusfield.New
Statusenum values (extendMAG_CAL_STATUSMAVLink enum):FAILED_OFFSETSCOMPASS_OFFS_MAXFAILED_DIAG_SCALINGFAILED_RESIDUALS_HIGH_fitness > sq(_tolerance)or NaNFAILED_RADIUS(7) was already present. All four conditions were previously collapsed intoFAILED(5).Behaviour changes:
fit_acceptable()replaced byfit_acceptable_status() const— pure query, no side effects; returns the specificStatusdirectly.set_status()with the returned value.set_status(FAILED)is guarded against overwriting any of the five specificFAILED_*statuses (previously onlyFAILED_ORIENTATIONandFAILED_RADIUSwere protected).COMPASS_CAL_RETRY_REPORT_HOLD_MS = 500 ms) ensures a GCS pollingMAG_CAL_REPORTat a few Hz reliably observes the specific failure reason before the next retry overwrites it.update_cal_status(),failed(),is_calibrating(), andsend_mag_cal_report()all handle the three new cases identically toFAILED_RADIUS.Note: The new integer values (8/9/10) are transmitted immediately. GCS will display the raw number until a matching
MAG_CAL_STATUSentry is added in the upstream mavlink/mavlink XML and a pymavlink bump lands in ArduPilot. The autotest usesgetattr(mavutil.mavlink, 'MAG_CAL_FAILED_RESIDUALS_HIGH', 10)fallbacks pending that bump.Related
MAG_CAL_FAILED_OFFSETS/FAILED_DIAG_SCALING/FAILED_RESIDUALS_HIGHto common.xmlTesting
Builds clean on SITL and MatekF405-Wing (
./waf configure --board MatekF405-Wing && ./waf plane). Flash delta vs master: +64 bytes on MatekF405-Wing plane.SITL autotest
SITLCompassCalibrationpasses:Two subtests verify the new codes:
COMPASS_CAL_FIT=0.001) — all compasses reportMAG_CAL_FAILED_RESIDUALS_HIGH (10)instead ofMAG_CAL_FAILED (5).SIM_MAG1_OFS_X/Y/Z=2000) — one compass reportsMAG_CAL_FAILED_OFFSETS (8); remaining compasses still succeed.Hardware test on MatekF405-Wing V2 with ArduPlane: successful calibration completes normally. With a strong nearby magnet (field ~1500 mGauss),
FAILED_RADIUSis reported in Mission Planner messages, confirming the specific status is visible instead of the previous silent generic failure.