AP_NavEKF3: anchor yaw while ground-learning in MAG_CAL=7 - #34057
AP_NavEKF3: anchor yaw while ground-learning in MAG_CAL=7#34057andyp1per wants to merge 2 commits into
Conversation
Previous review (2026-08-17)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Verdict: REQUEST CHANGES. CI is green and no maintainer has weighed in yet, so treat this as input rather than a verdict that outranks priseborough's. What's right, and worth saying first: the guard is exactly the existing mode-7 ground-learning gate, so the anchor is live precisely when the learning it references is live — that's the right coupling. States and covariance are handled consistently (full Jacobian update, Two things I think need a response before merge. Issues
Notes
Checked and clean: lane switching is unaffected — |
|
@andyp1per even though I approved the original mode 7 PR, I unfortunately don't remember it, and looking at PR 32200 I don't understand how it works. How is yaw observable on the ground? Where does it get the yaw from? |
|
Short answer: on the ground, in mode 7, it isn't observable - and that's the bug this PR fixes. Mode 7 turns on 3-axis fusion (earth + body field states) while the vehicle is stationary. 3-axis fusion has no independent yaw reference: the only heading information is in the magnetometer, and the filter is simultaneously free to explain any measurement with the body field states. On a stationary vehicle a yaw error and a body field offset are indistinguishable - the filter can rotate yaw and shift MX/MY/MZ to match, with zero innovation. So yaw isn't held by anything. It sits wherever setYawFromMag() left it at alignment and then free-integrates the z gyro bias. Once the origin is set and EK3_MAG_EF_LIM pins the earth field to the WMM tables, the residual has to go somewhere, and it goes into yaw. Each core walks in its own direction, which is what trips the yaw consistency pre-arm check. I measured 0.35 deg/s and 150 deg of walk on the bench with mag test ratio never above 0.34 - the filter is perfectly happy, it just isn't measuring heading. That's the difference from the other ground modes. MAG_CAL 0/2/3 use heading fusion on the ground - the FUSE_YAW branch of SelectMagFusion(), which takes the tilt-compensated compass heading and fuses it as a direct yaw observation - that is where their yaw comes from. Mode 7 skipped that branch entirely because it's in FUSE_MAG, so it got the field learning but lost the heading reference. This PR gives mode 7 the same heading observation the other ground modes already have, alongside the 3-axis fusion rather than instead of it, gated on onGroundNotMoving - motion is what makes yaw observable to the 3-axis fusion, so the anchor is only needed while stationary. That is also the gate already used for the STATIC yaw fusion in the no-yaw-sensor fallback of SelectMagFusion(), which exists to stop ground yaw drift for the same reason. The heading comes from the raw compass, not from the learned field states, so it isn't circular. Two consequences that matter:
On whether that learning survives to be useful: it does. On the in-flight reset, resetMagFieldStates() rewrites earth_magfield and re-inflates P[19..21], but zeroStatesVarCov() only zeroes covariance rows and columns, not the states - so the ground-learned body_magfield carries through. Known limitation, same one ordinary heading fusion has always had: a body field component perpendicular to the horizontal earth field is not separable from yaw by a heading measurement, so it shows up as a heading offset of asin(b_perp/H) rather than being learned - about 5.7 deg for H = 0.2 G and b_perp = 0.02 G. Subtracting body_magfield from the heading measurement would fix that on paper but reintroduces exactly the circularity above, so I left it. The one thing I'd flag for you rather than defend: while anchored this fuses the same compass sample twice per cycle, once as a scalar heading and again as three components, and the second fusion doesn't account for the correlation. With EK3_YAW_M_NSE at 0.5 rad the heading update is weak - at a converged ground yaw variance of ~2 deg the Kalman gain is 0.005, so it perturbs the subsequent 3-axis innovations by about 1% - but it is statistically double-counting. Gating on onGroundNotMoving confines it to a disarmed, stationary vehicle. Decimating it further would cut the double-count, but it would also weaken the z gyro bias observation, which is the part that actually stops the walk rather than just opposing it. I'd rather do it this way and say so than hide it. |
dc0c982 to
5c02dd7
Compare
Previous review (2026-08-20)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_20_1016/devcall_pr_reviews.html Re-reviewed at head Both concerns from my previous comment are now honestly disclosed, and where I could compute your numbers they're right — including one where my severity claim was too strong, which I'm withdrawing. Verdict: downgraded from REQUEST CHANGES to COMMENT. Three asks remain, none requiring a change to the algorithm. The guard changed materially ( Resolved" Withdrawn — my previous comment was wrong or overstated
Still open
NewThe new guard leaves an unbounded window of exactly the condition the commit message diagnoses. Since the anchor is disarmed-only, trace the armed-on-ground state in mode 7: This is not a regression against master, but it is a narrowing against your previous revision, which covered that window — and my own "too broad" note is what prompted the change. The trade is real in both directions, since anchoring while armed means anchoring under motor interference. Worth confirming it's deliberate and saying so in the commit message. Magnitude unconfirmed — the 0.35 deg/s bench figure was measured with no anchor and an unlearned bias, so it's an upper bound. Doc wording"While the vehicle is stationary on the ground" describes an armed stationary copter — a state that is provably not anchored. "While disarmed and stationary" is accurate and one word longer. The second clause implies motion restores observability, but I independently confirmed several of your claims: CI was 1 pass / 82 pending when I looked. This should go to @priseborough before merge regardless — he hasn't commented yet. |
5c02dd7 to
43fa2ba
Compare
|
Thanks - the three open asks are addressed. The perpendicular-body-field limitation is now in the commit message and, in one sentence, in the EK3_MAG_CAL doc, so it survives into the tree rather than living only in this thread. The double-fusion disclosure is in the commit message too, including your point that the gain starts at 0.5 after a yaw reset and only settles to ~1% after a few seconds - that refinement is right and worth having on the record. Param doc now reads "While disarmed and stationary", not "stationary on the ground". XKFS.MAG_FUSION gets a fourth value, 3 = fuse mag with yaw anchored, so a log can tell the two apart. On the armed-on-ground window: deliberate, and now stated in the commit message. onGround is literally !motorsArmed, so anchoring there means anchoring to a heading the running motors are disturbing. It also isn't the case that motivated the PR - the walk trips the pre-arm yaw consistency check, so the failure I set out to fix happens entirely while disarmed. Whatever walk is left in the armed window should be smaller than the 0.35 deg/s bench figure, since the disarmed anchor has converged the body field that was driving it by then, but I haven't measured that. On the innovation gate: agreed it's inoperative here and I'm leaving it. It's the same ungated-on-ground policy every other MAG_CAL value already gets through the FUSE_YAW branch, and gating it only for mode 7 would reject exactly the large-initial-innovation case the anchor exists to correct. On your translation question: updateMovementCheck() keys off gyro magnitude, gyro jerk, accel magnitude deviation from g, and accel jerk. A constant-velocity translation doesn't clear it - accel magnitude stays at g. What clears it is rotation or acceleration, and rotation is what makes the body field separable from yaw. Acceleration on the ground while disarmed is someone handling the vehicle, which isn't a heading you want anchored to anyway. |
Previous review (2026-08-22)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_22_1100/devcall_pr_reviews.html Re-reviewed at head All three previously-open asks are resolved. Verdict stays COMMENT, nothing blocking. CI fully green (99/99), including the three jobs that would catch a malformed Resolved
A correction I owe youI previously questioned whether New, all minor
The innovation gate is unchanged and still can't reject here (needs @priseborough still hasn't commented; |
43fa2ba to
03d30cb
Compare
When EK3_MAG_CAL=7 (GroundAndInflight) learns the field states on the ground, a stationary vehicle gives the 3-axis fusion no yaw observability: a yaw error is absorbed by the body field states with zero innovation and each core then free-integrates its own gyro bias. On a vehicle whose local field disagrees with the WMM tables the yaw walks away from the compass heading as soon as the origin is set and the earth field states reset to the tables - each core in its own direction, which trips the pre-arm yaw consistency check and blocks arming. Observed on the bench at 0.35 deg/s and 150 deg of walk while DCM held the true heading throughout, with the mag test ratio never exceeding 0.34. Fuse the magnetic heading alongside the 3-axis fusion while disarmed and stationary. That pins yaw to the compass, and with the earth field held to the tables the body field states become observable at a fixed heading - which is the battery-signature learning the mode exists for. The heading measurement comes from the raw compass, not the learned field states, so the anchor is not circular. Holding the earth field to the tables requires EK3_MAG_EF_LIM non-zero and a table field, which is the default. With EK3_MAG_EF_LIM=0 the earth field is initialised from the measurement instead, so the table mismatch that drives the walk does not arise; the anchor still pins yaw there, but three mag components cannot separate the earth and body field states at a fixed attitude. Gate on onGroundNotMoving rather than !inFlight. Rotation is what makes yaw observable to the 3-axis fusion, so the anchor is only needed while stationary, and onGroundNotMoving already gates the equivalent STATIC yaw fusion used to stop ground yaw drift without a yaw sensor. It also avoids !inFlight, which stays true well past takeoff on planes and through the first 1.5 m of climb on copters. That gate also requires the motors to be disarmed, so an armed vehicle idling on the pad or taxiing runs 3-axis fusion with no anchor, as it does today. That is deliberate: anchoring there would pin yaw to a heading the running motors are disturbing. Mode 4 has the same on-ground observability gap, but its behaviour is long-standing and is left alone. The anchor inherits the limitation heading fusion has always had. A body field component perpendicular to the horizontal earth field is not separable from yaw by a heading measurement, so it lands in yaw as an offset of asin(b_perp/H) - 5.7 deg for H = 0.2 G and b_perp = 0.02 G - rather than being learned. Subtracting body_magfield from the heading measurement would remove that on paper, at the cost of making the anchor circular. Leaving the yaw test ratio live while anchored is deliberate. It feeds the pre-arm compass variance check, the GPS yaw check and the 5 second ground heading reset, none of which mode 7 could reach while the ratio was forced to zero. Modes 0, 2 and 3 already fuse the same heading and leave the same ratio live on the ground, so this reports a yaw disagreement mode 7 used to hide rather than adding a failure mode of its own - a sustained 40 deg disagreement blocks arming on copter in either mode. While anchored the same compass sample is fused twice, once as a heading and again as three components, with no allowance for the correlation. At the converged ground yaw variance the heading gain is 0.005, so it moves the subsequent 3-axis innovations by around 1%; the gain starts at 0.5 on the first sample after a yaw reset and decays over the following few seconds. Decimating the anchor would cut the double count but also weakens the z gyro bias observation, which is the part that stops the walk rather than just opposing it. Log the anchored case as its own XKFS.MAG_FUSION value so a log reader can tell it from unanchored 3-axis fusion. Replay reproduces the unfixed walk on the two bench logs that hit this, 54 to 207 deg over 105 s.
03d30cb to
fc7735c
Compare
Previous review (2026-08-25)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Re-reviewed at head (The raw delta looks big, but master moved since my last review — several of this PR's earlier commits plus unrelated AP_RTC/terrain work are now merged. The PR itself adds two commits: the anchoring change and its ardusub log mirror.)
CI was 99/99 green at the prior head; a fresh run is pending after the rebase. |
Mirror the anchored XKFS.MAG_FUSION value and add a Sub leg asserting it while disarmed and stationary, that arming releases the anchor, and that disarming restores it. Without the anchoring change the leg fails with mag fusion selection 2 where 3 is expected.
fc7735c to
f2d42a1
Compare
|
Test added as a fourth leg of the Sub FuseMag test, alongside the existing MAG_CAL 0/3/4 legs. It fails without the fix: reverting AP_NavEKF3_MagFusion.cpp to master gives "Expected mag fusion selection 3, found 2". One correction to the shape you suggested. Arming is exactly when the anchor cannot be active - Sub takes the non-fly-forward branch of detectFlight(), where onGround is !motorsArmed, so an armed sub reads FUSE_MAG and a test that arms and asserts 3 would fail. The leg asserts both sides instead: anchored while disarmed and stationary, released on arm, restored on disarm. That pins down the disarmed-only gate rather than just the log value. Being straight about the other half of what you asked for: the leg also checks that the heading holds within 5 deg over 30 s while anchored, but that part is a guard rather than a regression test. SITL's field comes from the same WMM tables the EKF resets to, so there is no mismatch to drive the walk, and the unfixed build fails at the selection assert before it reaches the heading check. Reproducing the real failure needs an induced table/field disagreement, which I have not attempted. |
|
Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Re-reviewed at head What I verified at this head: the anchor is structurally impossible while armed ( Two non-blocking notes:
|
Summary
EK3_MAG_CAL=7 (6774c1b) learns the mag field states while on the ground, but a stationary vehicle gives the 3-axis fusion no yaw observability, so the yaw estimate can walk away from the compass heading and block arming. This anchors the yaw with a magnetic heading fusion while the vehicle is stationary on the ground.
Classification & Testing (check all that apply and add your own)
Replay A/B on the two bench logs that hit the failure, same tree with only this change different. Unfixed reproduces the walk, 54 to 207 deg over 105 s.
The fixed-side runs are being re-measured against the current onGroundNotMoving gate. The earlier figures (both cores within 1 deg of the compass heading for the full log, and a second vehicle tracking DCM within 1.6 deg across 47 s stationary plus a ~200 deg hand rotation) were taken with an earlier !inFlight gate, which anchored through the rotation that the current gate deliberately releases.
Description
With mag field learning active on a stationary vehicle, a yaw error is absorbed by the body field states with zero innovation, so nothing corrects it and each core free-integrates its own gyro bias. We hit this on an octaquad at a site whose local field disagrees with the WMM tables: the walk starts the moment the origin is set and the earth field states reset to the tables, each core reconciling the mismatch by rotating yaw in its own direction at ~0.35 deg/s. The pre-arm reported "EKF3 Yaw inconsistent" growing 86 to 169 deg (so "Wait" could never clear it) while DCM held the true heading on the same magnetometer and the mag test ratio never exceeded 0.34 - the filter was self-consistent and wrong.
The fix fuses the magnetic heading alongside the 3-axis fusion while the vehicle is stationary on the ground in mode 7. The heading measurement comes from the raw compass, not the learned field states, so the anchor is not circular. With yaw pinned and the earth field held to the tables, the body field states become fully observable at a fixed heading, which is the battery-signature learning the mode exists for. Falling back to plain heading fusion on the ground (as mode 3 does) would also stop the walk, but it inhibits the ground learning that is the point of mode 7.
The gate is onGroundNotMoving rather than a simple in-flight test. Motion is what gives the 3-axis fusion yaw observability in the first place, so the anchor is only needed while stationary and should release as soon as the vehicle moves. onGroundNotMoving already gates the equivalent job at AP_NavEKF3_MagFusion.cpp:271, the STATIC yaw fusion that stops ground yaw drift when there is no yaw sensor. An !inFlight test looks equivalent but is not: it stays true well past takeoff on planes, and through the first 1.5 m of climb or 5 s of flight on copters, which would leave the heading anchored into exactly the motor interference window mode 7 exists to avoid.
While anchored, the same compass sample feeds both the heading fusion and the 3-axis fusion in one cycle, and the second fusion does not account for the correlation. With EK3_YAW_M_NSE at 0.5 rad the heading update is weak - at a converged ground yaw variance the gain is of order 0.005 - and the onGroundNotMoving gate confines it to a disarmed, stationary vehicle. Decimating the anchor would reduce it further but would also weaken the yaw gyro bias observation, which is what actually stops the walk rather than merely opposing it.
Replay of the failing log, unfixed vs fixed, against the compass-only DCM reference (the step at ~42 s is a real field disturbance that DCM also tracks). To be regenerated with the current gate:
Mode 4 (ALWAYS) shares the same on-ground observability gap but its behaviour is long-standing, so this change is scoped to mode 7 only.