Skip to content

Add wind estimation for ExternalAHRS - #34410

Draft
peterbarker wants to merge 9 commits into
ArduPilot:masterfrom
peterbarker:pr-claude/externalahrs-wind-estimation
Draft

peterbarker wants to merge 9 commits into
ArduPilot:masterfrom
peterbarker:pr-claude/externalahrs-wind-estimation

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

ExternalAHRS backends now estimate wind, feeding the shared wind-triangle estimator from the external device's velocity and attitude. Builds on the estimator lift merged in #33946.

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Two new autotests. ExternalAHRSWindEstimate flies a VectorNav SITL in LOITER with a simulated wind and requires the WIND estimate to converge on it and to be reported valid — validity is not visible in the WIND message (Plane sends the vector regardless), so a small test script reports ahrs:get_wind()'s validity over a NAMED_VALUE_FLOAT.

ExternalAHRSWindEstimateVelocityStall stalls just the velocity messages of a simulated InertialLabs (velocity and position are separate messages on that device) while turning in LOITER, and requires the wind estimate to match the pre-stall estimate the moment the External backend is re-selected. This test was proven to discriminate: with wind feeding keyed to the position timestamp instead of the velocity timestamp, the stalled velocity is re-fused every position message and the corrupted estimate fails the check (12.7° off in an A/B run); with this series it survives bit-identical.

Every commit builds individually; WindEstimates, Deadreckoning and DeadreckoningNoAirSpeed also pass at the tip (no change to DCM behaviour).

Description

ExternalAHRS provides a velocity and attitude solution but no wind estimate, so a vehicle flying on an external AHRS reports no wind. This series feeds the external velocity and attitude into the same wind-triangle estimator DCM uses (lifted to AP_AHRS_Backend in #33946) and publishes the result.

Commit by commit:

  • track whether a wind estimate has been producedestimate_wind's first calls merely seed its filter history; until it has actually blended a measurement the zero _wind is not an estimate at all. A new flag lets backends publish an honest wind_valid. DCM deliberately does not consult it: DCM has always reported its initial zero wind as valid, and this series does not change DCM behaviour.
  • estimate wind for ExternalAHRS from the wind triangle — driven from get_results(), reusing the velocity and attitude just captured for publication, once per new external sample (estimate_wind additionally rate-limits to 10Hz internally). wind_valid requires wind estimation enabled (i.e. Plane — other vehicles are unaffected), an estimate actually produced, and the source healthy: an unhealthy source's estimate may be arbitrarily stale, while a merely-old estimate from a healthy source stays valid — a stale wind is still useful when dead-reckoning.
  • record when velocity was last updated (AP_ExternalAHRS) — every driver publishing a velocity stamps state.last_velocity_update_us, and a get_velocity_NED() overload returns the velocity and its timestamp captured under a single semaphore acquisition.
  • key wind estimation on new velocity samples — feeding was initially keyed to last_location_update_us, but on devices delivering velocity and position in separate messages a stalled velocity stream would be re-fused on every new position message, corrupting the persistent filter state while wind_valid remained true. The feed now keys on the velocity's own timestamp, so a stalled velocity stream stops wind estimation entirely.
  • SITL InertialLabs velocity stallSIM_ILABS_NO_VEL splices the VELOCITIES message out of the simulated packet while attitude and position continue, enabling the stall autotest above.
  • require a valid attitude to feed the estimator — no behaviour change (an early return already guarantees it), but estimate_wind requires a unit fuselage direction, so the requirement is made local to the feed rather than a consequence of distant control flow.

Accepted properties, for the record:

  • On split-stream devices the attitude paired with a velocity sample may come from a neighbouring packet — while both streams are flowing, a skew of at most one packet interval, which the slow filter tolerates (DCM's pairing of the last GPS velocity with the current attitude is far looser). A stalled attitude stream drives the source unhealthy.
  • Samples received while the source is unhealthy still update the filter state, matching DCM's long-standing shape; health gates wind_valid instead.
  • A stalled velocity stream also feeds stale velocity to navigation consumers on master today (with, for example, InertialLabs' health check only covering attitude freshness); a generic ExternalAHRS velocity-freshness check would be a worthwhile follow-up but is out of scope here.

Follow-up enabled but deliberately not included: routing ExternalAHRS synthetic airspeed to its own backend (needs both the frontend dispatch switch and an External producer for _last_airspeed_TAS; synthetic airspeed still comes from DCM in exactly the cases it does today).

This PR was AI-assisted

estimate_wind's first calls merely seed its filter history; until it
has actually blended a measurement, the zero _wind is not an
estimate at all.  Record when _wind has been updated so backends can
publish an honest wind_valid.

DCM deliberately does not consult this flag: it has always reported
its initial zero wind as a valid estimate, and this refactor does
not change existing behaviour.
ExternalAHRS provides a velocity and attitude solution but no wind
estimate, so previously a vehicle running an external AHRS reported
no wind.  Feed the external velocity and attitude into the same
wind-triangle estimator DCM uses and publish the result.

The estimator is driven from get_results, reusing the velocity and
attitude just captured for publication, and is fed once per new
external sample (estimate_wind additionally rate-limits internally).

The estimate is only produced, and only reported valid, while wind
estimation is enabled (i.e. on Plane); other vehicles are
unaffected.  It is only valid once the estimator has actually
produced an estimate, and is invalidated while the external source
is unhealthy, since it may then be arbitrarily stale; a merely-old
estimate from a healthy source stays valid, as a stale wind is
still useful when dead-reckoning.
Reports the validity of ahrs:get_wind() as a NAMED_VALUE_FLOAT for
autotest consumption; validity is not otherwise visible in telemetry.
Flies a VectorNav external-AHRS SITL in LOITER with a simulated wind
and asserts the WIND estimate converges, exercising the wind triangle
driven from ExternalAHRS velocity and attitude.

The WIND message is sent whether or not the estimate is valid, so a
small script reports the validity of ahrs:get_wind(); the test also
requires the estimate be reported valid.
Stamp state.last_velocity_update_us in every driver which publishes a
velocity, and provide an accessor returning the velocity and its
timestamp captured under a single semaphore acquisition.

Velocity freshness cannot be inferred from last_location_update_us:
several devices (e.g. InertialLabs) deliver velocity and position in
separate messages, so the position timestamp can keep advancing while
the velocity stream has stalled.
When SIM_ILABS_NO_VEL is set, splice the VELOCITIES message out of
the transmitted packet - the message-type list entry and the payload
bytes - while attitude and position messages continue to flow.
Allows testing of how consumers handle a stalled velocity stream on
a device which delivers velocity and position in separate messages.
The wind feed was keyed to last_location_update_us, but some devices
(e.g. InertialLabs) deliver velocity and position in separate
messages, so a stalled velocity stream would be re-fused on every new
position message, corrupting the persistent wind-filter state while
wind_valid remained true.

Key the feed to the velocity's own timestamp instead, captured
atomically with the velocity under a single semaphore acquisition; a
stalled velocity stream now stops wind estimation entirely.
Stall the simulated InertialLabs velocity stream while turning in
LOITER.  The stale velocity flags in the filter status demote the
AHRS to DCM, but the External backend continues to run, so the wind
estimate must match the pre-stall estimate the moment the external
backend is re-selected.  With wind feeding keyed to
last_location_update_us the estimate is corrupted by the stalled
velocity and fails that check.
…ator

The early return on invalid attitude already guarantees this, so no
behaviour changes; estimate_wind requires a unit fuselage direction,
and a freshly-zeroed dcm_matrix must never reach it, so make the
requirement local to the feed rather than a consequence of control
flow far above it.
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Reviewed at head 38afa668e9.

Full report, with everything that was checked: https://uav.tridgell.net/DevCallReviews/2026_09_16_AIReview/devcall_pr_reviews.html#pr34410

Verdict: COMMENT — 1 bug, 1 issue below.

The design is sound and the frame/sign/unit work is right — no new conversion is introduced, estimate_wind() is the shared DCM estimator and _wind stays the NED vector DCM has always produced. Empirically the estimate converged to 5 m/s @ 40-45° for SIM_WIND_SPD=5 / SIM_WIND_DIR=45. The one thing worth fixing before it leaves draft is the _have_wind_estimate gate, which is the PR's own central claim.

Bugs

  • libraries/AP_AHRS/AP_AHRS_External.cpp:136_have_wind_estimate does not mean what its comment says — wind_valid goes true on the ground with a meaningless ~zero wind. The header documents the flag as meaning the filter has been seeded, but the seeding branch at AP_AHRS_DCM.cpp:1162 is if (now - _last_wind_time > 10000) and _last_wind_time is zero-initialised, so on any boot where the first external attitude+velocity sample arrives less than 10 s after power-on the branch does not fire. _last_fuse is still {0,0,0}, so the first call runs the turning branch against _last_vel = {0,0,0}, blends a degenerate result and sets the flag. Measured with a probe after wait_ready_to_arm(), before takeoff, SIM_WIND_SPD=5 SIM_WIND_DIR=45: as-is gives WINDVALID==1 ON THE GROUND with {direction: -6.00, speed: 0.725}; with ARSPD_USE=0 it gives speed: 3.5e-11 — an exactly-zero wind published as a valid estimate, precisely the "reads as calm to every consumer" failure the flag was added to prevent. Mutating the seed condition to if (_last_wind_time == 0 || now - _last_wind_time > 10000) gives WINDVALID stayed 0 on the ground and the full test still passes. Practical impact is low (the value is bounded near zero and the only validity-gated consumer is used in flight after convergence), but the honest-wind_valid property the PR is built around is not delivered.

Issues

  • libraries/AP_AHRS/AP_AHRS_External.cpp:135 — A latched _have_wind_estimate plus no staleness bound means an arbitrarily old wind is published as valid. The flag is never cleared, and the >10 s "scrap our data" path resets _last_fuse/_last_vel but neither _wind nor the flag. A device whose velocity stream stalls while still reporting healthy will report a frozen wind as valid indefinitely; the same applies across an unhealthy→healthy transition. The PR deliberately accepts "a merely-old estimate from a healthy source stays valid", but there is no upper bound at all. Worth an explicit age limit.

4 further minor notes (style, coverage, pre-existing items) are in the report rather than here, so they don't bury the above.


Independent cold pass: no verified defects, APPROVE. The primary read's BUG was proven by a SITL probe and a source mutation, so it is retained despite the disagreement.

Reviewed by: Claude (full read of the diff, thread and surrounding source) + an independent Codex cold pass that was not shown these findings. Findings marked both reviewers were reproduced independently twice.

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants