Skip to content

Fix MSP2_INAV_WIND returning stale non-zero values when invalid - #11762

Merged
sensei-hacker merged 1 commit into
iNavFlight:release/9.1from
sensei-hacker:fix-msp2-inav-wind-zero-on-invalid
Aug 3, 2026
Merged

Fix MSP2_INAV_WIND returning stale non-zero values when invalid#11762
sensei-hacker merged 1 commit into
iNavFlight:release/9.1from
sensei-hacker:fix-msp2-inav-wind-zero-on-invalid

Conversation

@sensei-hacker

Copy link
Copy Markdown
Member

Summary

Flagged by Qodo's automated review on PR #11761 (a release/9.1maintenance-10.x merge PR): MSP2_INAV_WIND's handler in fc_msp.c called getEstimatedHorizontalWindSpeed() unconditionally and only used isEstimatedWindSpeedValid() to set the flags byte, not to gate the actual windSpeed/windAngle values. Confirmed real:

  • In wind_estimator.c, estimatedWind[] is written only inside the successful-estimate branch of updateWindEstimator() (the low-pass filter update) — it is never reset when hasValidWindEstimate clears (timeout at 15 min without altitude change, or validityScore decaying to 0 from repeated stale updates).
  • So once the estimate goes invalid, getEstimatedHorizontalWindSpeed() keeps returning the last computed value indefinitely, while the flags byte correctly reports invalid.
  • This contradicts MSP2_INAV_WIND's own documentation (docs/development/msp/msp_messages.json): "returns zeroes when wind estimation is not compiled in or not yet valid. Check bit 0 of flags before using speed/angle values."
  • Every other consumer of this API already follows the "check validity before reading" pattern instead of relying on the estimator to self-zero: io/gps.c, io/osd.c, flight/rth_estimator.c, navigation/navigation.c, flight/imu.c, mavlink/mavlink_streams.c, programming/logic_condition.c, sensors/pitotmeter.c. MSP2_INAV_WIND was the only caller that didn't.

Changes

src/main/fc/fc_msp.c: gate windSpeed/windAngle on isEstimatedWindSpeedValid(), matching the established pattern and the message's documented contract — send 0, 0, flags=0 when invalid, the real values with flags=1 when valid.

Testing

  • SITL build (USE_WIND_ESTIMATOR + USE_GPS both active via target/common.h) — compiles cleanly, zero warnings/errors in fc_msp.c or wind_estimator.c.
  • Verified by reading wind_estimator.c: estimatedWind[] has exactly one write site (the filter update inside the successful-estimate branch) and no reset path, confirming the staleness is real and not just a theoretical race.
  • Grepped every other call site of getEstimatedWindSpeed()/getEstimatedHorizontalWindSpeed()/isEstimatedWindSpeedValid() in src/main to confirm the fix matches the codebase-wide convention rather than introducing a new one.

Targets release/9.1 per this repo's bugfix base-branch convention; will flow forward to maintenance-10.x on the next sync.

getEstimatedHorizontalWindSpeed() was called unconditionally, so once
the wind estimate becomes invalid (e.g. the 15-minute stationary-altitude
timeout in wind_estimator.c), MSP2_INAV_WIND kept sending the last
computed windSpeed/windAngle instead of zero. estimatedWind[] is never
reset when hasValidWindEstimate clears, only the flags byte reflected
validity. This contradicted the message's own documentation ("returns
zeroes when wind estimation is not compiled in or not yet valid") and
diverged from every other consumer of this API (gps.c, osd.c,
rth_estimator.c, navigation.c, imu.c, mavlink_streams.c,
logic_condition.c, pitotmeter.c), all of which check
isEstimatedWindSpeedValid() before reading the value rather than relying
on the estimator to self-zero.

Reported by Qodo's automated review on PR iNavFlight#11761 (a release/9.1 ->
maintenance-10.x merge that carried this pre-existing bug forward,
unrelated to that merge itself). Fixed at the source (release/9.1) so it
flows forward on the next maintenance-10.x sync.
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

MSP2_INAV_WIND: zero wind speed/angle when estimate is invalid

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Gate MSP2_INAV_WIND speed/angle on wind-estimate validity.
• Return 0/0 with flags=0 when the estimator is invalid.
• Align MSP output with documented message contract and other call sites.
Diagram

graph TD
  A["MSP2_INAV_WIND handler"] --> B{"Wind valid?"}
  B -->|"yes"| C["Read wind estimate"] --> E["Write speed/angle/flags"] --> F["MSP response"]
  B -->|"no"| D["Use zeros"] --> E
  C --> G["Wind estimator"]
  subgraph Legend
    direction LR
    _proc["Process/step"] ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Reset estimator outputs when validity clears
  • ➕ Eliminates stale reads for any caller that forgets to gate on validity
  • ➕ Keeps estimator API safer by default
  • ➖ Touches estimator behavior and potentially affects filtering/telemetry semantics
  • ➖ Broader regression surface than fixing the single incorrect caller
2. Always compute but force-wire output to zero when invalid (current PR)
  • ➕ Matches the MSP message contract directly at the serialization boundary
  • ➕ Minimizes scope and aligns with existing codebase convention (check valid before read)
  • ➖ Relies on each consumer continuing to gate reads correctly outside MSP

Recommendation: Keep the PR’s approach: gate MSP2_INAV_WIND’s speed/angle on isEstimatedWindSpeedValid() and emit zeros when invalid. It is the smallest change that restores the documented contract and avoids expanding risk into the estimator’s internal lifecycle/state handling.

Files changed (1) +6 / -2

Bug fix (1) +6 / -2
fc_msp.cGate MSP2_INAV_WIND values on validity and zero when invalid +6/-2

Gate MSP2_INAV_WIND values on validity and zero when invalid

• Initializes windSpeed/windFlags to zero and only reads the wind estimator when isEstimatedWindSpeedValid() is true. This prevents MSP2_INAV_WIND from returning stale non-zero values while still indicating invalid via flags.

src/main/fc/fc_msp.c

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Test firmware build ready — commit c29dc54

Download firmware for PR #11762

244 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

@sensei-hacker
sensei-hacker merged commit 49082e8 into iNavFlight:release/9.1 Aug 3, 2026
23 checks passed
@sensei-hacker sensei-hacker added this to the 9.0.1 milestone Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant