FW turn predictor: gate the arc turn coordinator off <=512KB flash targets - #11998
sensei-hacker wants to merge 4 commits into
Conversation
…2KB flash PR iNavFlight#11812's coordinated-turn arc coordinator (updateFwTurnArc, ~440 lines) and its direct helpers (getFwCoordinatedTurnRadius, getFwTurnFeedForward) account for ~8KB of the feature's flash footprint, concentrated almost entirely in one LTO-merged function. This alone is what pushed ZEEZF7V3 (STM32F722, 512KB flash) over its FLASH1 budget on later maintenance-10.x commits (PR iNavFlight#11924's CI run reported a 171-byte overflow there). wp_turn_mode's DIRECT setting (legacy heading-PID turn) already has a no-op path through the arc coordinator, so gating this code out and falling back to DIRECT behavior on <=512KB targets requires reviving nothing. USE_FW_TURN_PREDICTOR is added to common.h's existing "MCU_FLASH_SIZE > 512" gate list, alongside USE_AUTO_TRANSITION etc. Deliberately NOT gated: updateFwEnergyBankGuard, updateFwLoiterArc/ getFwStableLoiterRadius, and applyFwRollInSmoothing. These are general fixed-wing improvements independent of wp_turn_mode and apply to every flash size. Verified: ZEEZF7V3 FLASH1 490,743 B -> 484,703 B (-6,040 B). MATEKH743 (2048KB) keeps the feature compiled in (confirmed via preprocessor replay, since LTO inlines the gated symbols away either way). No conflict markers, no accidental deletions - a pure #ifdef wrap.
…DICTOR nav_fw_turn_ff_gain and nav_fw_wp_turn_max_lead_time are only read by code gated behind USE_FW_TURN_PREDICTOR (getFwTurnFeedForward and the FLY_BY corner-cut block), so on <=512KB flash targets they were settable but silently inert. condition: hides them there, matching the existing USE_AUTO_TRANSITION-gated settings in this file. Also updated nav_fw_wp_turn_mode's description to note that its COORD_* values (COORD_FLYBY is the default) fall back to a plain DIRECT turn on those same targets, since the setting itself stays selectable - flagged by code review as worth surfacing since it affects the shipped default. docs/Settings.md regenerated via src/utils/update_cli_docs.py.
…E_FW_TURN_PREDICTOR The previous commit added condition: USE_FW_TURN_PREDICTOR to these two settings.yaml entries, so their SETTING_*_DEFAULT macros are no longer generated when the flag is undefined - breaking every <=512KB target with "undeclared" errors in navConfig_t's PG_RESET_TEMPLATE. Neither field is read anywhere outside the gated arc-coordinator code, so leaving them zero-initialized when gated is a safe substitute. Verified: ZEEZF7V3 links again (FLASH1 484,687 B, 98.61%), MATEKH743 unaffected (FLASH1 795,579 B, 43.36%, both settings still present).
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoDisable fixed-wing arc turn prediction on ≤512 KB targets
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Aircraft keep banking after loiter
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| #ifdef USE_FW_TURN_PREDICTOR | ||
| updateFwTurnArc(deltaMicros); | ||
| #endif |
There was a problem hiding this comment.
1. Aircraft keep banking after loiter 🐞 Bug ≡ Correctness
Gating updateFwTurnArc() also removes the only per-cycle assignment that clears fwArcActive, while the ungated updateFwLoiterArc() can still set that flag. On targets with 512 KB flash or less, leaving an established loiter or exceeding its release band makes the loiter function return without clearing the flag, so cross-track correction remains suppressed and the stale arc-bank command continues overriding the navigation controller.
Agent Prompt
## Issue description
Compiling out `updateFwTurnArc()` removes the per-cycle reset of `fwArcActive`, although the retained loiter controller can set it. After an established loiter stops driving, stale arc state continues overriding normal fixed-wing navigation.
## Fix Focus Areas
- src/main/navigation/navigation_fixedwing.c[715-744]
- src/main/navigation/navigation_fixedwing.c[1163-1207]
- src/main/navigation/navigation_fixedwing.c[1290-1294]
## Recommended Fix
Move the per-cycle `fwArcActive = false` reset out of `updateFwTurnArc()` and place it immediately before the optional waypoint-arc update and unconditional loiter-arc update. This lets either controller assert activity during the current cycle while ensuring gated builds clear stale loiter activity when neither controller drives.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11998 249 targets built. Find your board's
|
|
Compare and contrast #11996 |
Flash comparison: this gating PR vs the behaviour-identical refactor (#11996)Built all four states with the CI compiler (arm-none-eabi-gcc 13.2.1, RelWithDebInfo, LTO) on the fullest 512 KB targets from the #11924 CI ranking. B and C both sit directly on top of A, so the deltas are clean; D is 75 commits older and only serves as a "no feature at all" reference.
FLASH1, budget 491,520 B:
Per target: B−A = −716…−780 B, C−A = −6,032…−6,096 B (RAM −100 B), C−B = −5,252…−5,316 B. MATEKH743 is byte-identical under A and C, so the gate is off above 512 KB as intended. B+C combined was not built; arithmetically ZEEZF7V3 would land near 490.8 KB. What the numbers say:
Refs and exact commands are recorded; happy to re-run on any target you want added. |
|
Follow-up: closed the testing gap noted above. A SITL WP-mission test (arm → fly DIRECT → fly COORD_FLYBY, comparing the FC's own (The prior arming block was a SITL test-harness sequencing bug — HITL was enabled before gyro calibration finished — not a firmware issue; fixed in the test script only.) |
|
I am running a 3 stage flash check as before on all F722 targets right now and report back. removing the old code means removing the "Direct" option entirely? >That would mean no fallback and every turn will be an arc. Would need to force the FLYBY as fallback for the other two. I am almost out of claude budget. Tomorrow when its reset, I will do a more intensive optimization check of INAV, starting with that Idea. Maybe We can find other so far undiscovered levers. I have the big 20x plan now and can let my worker agents run for hours to check possibilities. |
Yeah, that was my thought. Instead of having three algorithms, have two. |
|
On it right now. Just a pure POC analysis. Option 1: remove direct and remaining turn smoothing code and rely only on the 3 new methods (with 2 algorithms and one is just a reverse). and FlyOver would be the fallback as its closest to the "Direct" method. Option 2: gate it for F722 only. so other boards keep the Direct option. |
I stand corrected -- we're looking at going from four algorithms to choose from, to three algorithms to choose from. Just writing that sentence, gut feeling is is that four different choices on how to make a turn in a mission might be too many. :) In a race, maybe you fly the turns for the gates a little differently in different courses. On a mission -- it kinda feels like three options might be enough? Without saying which three options there should be, but three choices seems like enough? |
|
analysis just finished and there is no code that only DIRECT uses. its a simple PID carrot method that inav uses in general to hold course. So there is nothing to gain back. But you are actually right. Even if it does not help anything. The Direct method as a selectable option doesn't do anything better except harder and more wind dependent turns. FLyOver is the smooth and controlled equivalent. Fallback will still be "DIRECT" if a prediction cannot get a reasonable turn path but having it selectable makes not much sense. My suggestion:
I will look into flash optimization possibilities tomorrow. maybe we find old wasteful code somewhere. If we can find some flash somewhere, we can ungate F722 for RC2. If not we discuss what we can do or if we just exclude them entirely. |
|
OK this is important: After the refactor I did there is literally just ONE FC in the F722 family that overflows. Every other FC still has room. Full report of the 86 Builds: Flash report: all 86 STM32F722 (512 KB) targets, refactor #11996 vs gating #11998Every target built from
Both branch from the same The ten tightest targets
Whole family
"Remove DIRECT / the old turn smoothing" (measured, prototype only)No 9.x turn-smoothing code survives; DIRECT is two OR-clauses that share the coordinator's "no tracking" early-return. Removing it entirely: MATEKF722 −16 B, MATEKH743 +240 B ( ReadingThe refactor keeps the feature on 85 of 86 boards and unbreaks none; the gating unbreaks ZEEZF7V3 by 13 bytes and drops the coordinated WP turn on all 86. A ZEEZF7V3-only Full 86-row table (B/C bytes, %, RAM, deltas) available on request; raw build logs kept. |
Why ZEEZF7V3 is 11.6 KB heavier than MATEKF722 and 12.8 KB heavier than its own sibling ZEEZF7V2
So the overflow is not the turn feature's doing and not fixable durably by gating it: it is one board carrying every driver either of its revisions or storage options could need. SuggestionSplit ZEEZF7V3 into two target variants (by IMU revision, or by blackbox backend, whichever matches the actual hardware) instead of gating the coordinated turns for the whole 512 KB class. That gives the board 4–12 KB of headroom permanently and leaves the other 85 F722 targets with the feature they still fit. A per-board Full 86-row table and the per-driver |
|
I see there is a ZEEZF7V3 20x20 and a ZEEZF7V3 30x30. |
|
so lets split the target then? I can make a PR for that. ZEEZF7V3_20MM and ZEEZF7V3_30MM? |
2 participants
Summary
PR #11812 added a fixed-wing coordinated waypoint-turn system (the "arc turn coordinator"). Its geometry/state-machine code, concentrated almost entirely in one function (
updateFwTurnArc(), ~440 lines), costs roughly 8KB of flash. On the STM32F722 target ZEEZF7V3 (512KB flash), that pushed FLASH1 over budget — CI on PR #11924 failed withregion 'FLASH1' overflowed by 171 byteson that target, unrelated to PR #11924's own change (which measured at 0 bytes delta on the same target).This PR gates the arc coordinator behind a new
USE_FW_TURN_PREDICTORflag, added to the existingMCU_FLASH_SIZE > 512feature-gate list incommon.h(alongsideUSE_AUTO_TRANSITIONetc.), so it's compiled out entirely on <=512KB flash targets.Changes
src/main/target/common.h: addUSE_FW_TURN_PREDICTORto theMCU_FLASH_SIZE > 512gate list.src/main/navigation/navigation_fixedwing.c: wrapupdateFwTurnArc(),getFwCoordinatedTurnRadius(),getFwTurnFeedForward(), their exclusive static state, and their call sites in#ifdef USE_FW_TURN_PREDICTOR.nav_fw_wp_turn_mode'sDIRECTsetting already has a no-op path through the arc coordinator, so gating it out and falling back to DIRECT behavior on affected targets revives nothing — it's the same code path DIRECT mode already took.updateFwEnergyBankGuard,updateFwLoiterArc/getFwStableLoiterRadius,applyFwRollInSmoothing. These are general fixed-wing improvements from the same PR, independent ofwp_turn_mode, and stay compiled in on every flash size.src/main/fc/settings.yaml: marknav_fw_turn_ff_gainandnav_fw_wp_turn_max_lead_timecondition: USE_FW_TURN_PREDICTOR(they're only read by the now-gated code, so they were previously settable-but-inert on affected targets — matches the existingUSE_AUTO_TRANSITIONconvention in this file). Updatednav_fw_wp_turn_mode's description to note that itsCOORD_*values (the default isCOORD_FLYBY) silently behave asDIRECTon <=512KB flash targets.src/main/navigation/navigation.c: guard the two now-conditional settings' default-value initializers with the same flag.docs/Settings.mdregenerated viasrc/utils/update_cli_docs.py.Testing
nm/objdumpon the compilednavigation_fixedwing.c.othat whenUSE_FW_TURN_PREDICTORis undefined, none ofupdateFwTurnArc,getFwTurnFeedForward, or their exclusive state symbols exist in the binary at all — sonav_fw_wp_turn_mode = DIRECTand= COORD_FLYBYare provably identical in control flow on gated builds, by construction, not just by runtime observation.ARMING_DISABLED_SENSORS_CALIBRATING | ARMING_DISABLED_NAVIGATION_UNSAFE/ "WAITING FOR GPS FIX" once a nav-capable mode range is configured, despite feeding a valid GPS fix via MSP). I confirmed this reproduces identically on the unmodified base commit (before any change in this PR), so it's a pre-existing SITL test-infrastructure gap, not a regression from this change — but it means the actual in-flight turn behavior on a gated build hasn't been flight-tested by me, only proven equivalent at the code level. Requesting a maintainer or someone with working SITL/hardware HITL flight-test this before merge if that gap matters for confidence here.Code Review
Reviewed with the inav-code-review agent. Findings addressed: added
condition:to the two settings that become inert when gated (previously missing), and updatednav_fw_wp_turn_mode's description to surface the <=512KB fallback behavior rather than leaving it silent. No CRITICAL issues found; ifdef boundaries traced symbol-by-symbol and confirmed self-consistent (every gated symbol's only usages are themselves inside the same gate).Related
Relates to #11812 (the feature this gates) and #11924 (where the ZEEZF7V3 overflow was first observed in CI, though unrelated to that PR's own change).