Skip to content

Draw the path a plane flies its mission along, and fix the arc/loiter drawing - #1753

Merged
peterbarker merged 22 commits into
ArduPilot:masterfrom
peterbarker:pr/plot-arcs-better
Oct 9, 2026
Merged

Draw the path a plane flies its mission along, and fix the arc/loiter drawing#1753
peterbarker merged 22 commits into
ArduPilot:masterfrom
peterbarker:pr/plot-arcs-better

Conversation

@peterbarker

@peterbarker peterbarker commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Follow-up to the arc and loiter mission drawing already on master ("map: plot
new arc waypoints better" through "map3d: show which way the mission is flown").
It started as fixes found by review and by flying test missions in SITL, and
grew into drawing the path a plane is actually going to fly, checked against
SITL flights of ArduPilot's own autotest missions.

The path a plane flies its mission along

MAVProxy/modules/lib/plane_track.py is new: a port of enough of ArduPlane's
navigation to work out where a plane or QuadPlane will go, rather than drawing
straight lines between items. It flies the mission with ArduPlane's L1
controller, at the aircraft's own speeds, roll limits and climb rates, and
models:

  • every loiter, with its direction, radius and exit (lined up on the next item
    as the mission stores it, or crosstracked from where it leaves the circle);
  • waypoints with acceptance and pass-by radii, and WP_MAX_RADIUS;
  • takeoffs: a fixed-wing one at the item's pitch, or faster where TECS has an
    airspeed sensor to fly by, on the course a log shows it was flown on; a VTOL
    one straight up, transitioning the way the aircraft points;
  • returns to launch, including the rally point one goes to, the circle a plane
    ends in, and each Q_RTL_MODE: QRTL's own altitude profile, and the
    fixed-wing spiral approach a QuadPlane lands from, flown into the wind;
  • VTOL landings: straight in, or on the approach param1 or Q_OPTIONS asks
    for;
  • jumps, jump tags, speed changes, altitude frames, and the return to launch
    ArduPlane flies when a mission runs out.

Where it cannot work a path out -- a vehicle that is not a plane, a command it
does not know, a mission that never finishes an item, or one the log holds only
part of -- it says so and the map falls back to drawing the mission's geometry.

Where it is drawn

  • map3d draws the flown path live, and has a Mission-path control to pick
    flown, geometry or plain, a Labels checkbox and a labelsize setting.
    The view's controls and the module's settings now agree whichever way a change
    crosses. The path is worked out on a thread, and redrawn when the mission,
    home, the wind, the way the vehicle points, or any parameter it is flown by
    changes.
  • MAVExplorer draws it for a log: map3d now works from telemetry logs as
    well as dataflash ones.
  • mavflightview reads missions out of telemetry logs again.

Reading a mission out of a log

LogMission moves from mavflightview to MAVProxy/modules/lib/log_mission.py
and is what both it and MAVExplorer read a mission with, so a log's mission is
read one way:

  • only the last mission to arrive whole replaces what was held, so a download
    the log ends part way through no longer replaces a complete mission;
  • MISSION_CLEAR_ALL, float MISSION_ITEM uploads and a CMD's logged frame
    are all honoured;
  • a mission downloaded again mid-flight is recognised as the one being flown --
    compared as AP_Mission gives it back, and ignoring home, which is the
    vehicle's own -- so where the flight had reached in it survives;
  • what a log holds only part of is drawn but not flown, for the mission and for
    the rally table alike: the items that arrived are where the vehicle was told
    to go, but what would have followed them is not known.

Drawing fixes

Commit What was wrong
map3d: resolve altitude frames before measuring a loiter climb An AMSL item before a home-relative LOITER_TO_ALT read as a huge dive and drew the wrong number of turns.
mp_util: only read param4 as a crosstrack choice where it is one ArduPilot reads param4 that way only for LOITER_TURNS, LOITER_TIME and LOITER_TO_ALT.
map3d: draw the turns LOITER_TURNS and DO_ORBIT ask for A three-turn climbing loiter was drawn as one turn.
mp_util: take loiter radii the way ArduPlane's update_loiter() does A LOITER_TIME radius of +-1 is a direction, and drew a 1 m circle instead of WP_LOITER_RAD.
MAVExplorer: draw only the last mission a log holds Every mission a log dumped was drawn, joined end to end.
MAVExplorer: draw a takeoff which has no position of its own Positionless takeoffs were dropped from logs.
map3d: take longitudes the short way round the antimeridian A mission across the seam was drawn the long way round.

Behaviour change

map: draw loiter circles and direction arrows by default. The 2D map's
loitercircle and showdirection are on, as is showdirection in map3d and
MAVExplorer, so every view of a mission looks the same.

Tests

The suite goes from 261 to 595 tests (5 skipped without wx or VTK); flake8 is
clean. tests/test_mission_geometry.py grows from 50 to 86 cases, and
tests/test_plane_track.py is new with 158.

Ten SITL flights are recorded under tests/missions/ and the drawn path is held
to each one, both across the ground and in height. The flights come from
autotests proposed in ArduPilot PR 34424 (QuadPlane.KalaupapaCanyonRun,
Plane.MissionItemTypes, QuadPlane.VTOLMissionItemTypes), which between them
fly every kind of item ArduPlane flies, every way a mission ends, and every
Q_RTL_MODE. Each recording says which ArduPlane flew it, since the drawing
follows a version of ArduPlane's behaviour, and tests/missions/record_flight.py
makes another from a log.

Flight Points Across: median / p90 / worst Height: median / p90 / worst
canyon run 1200 0.5 / 4.8 / 15.0 m 4.2 / 16.8 / 31.9 m
plane, every item type 954 2.2 / 12.3 / 27.6 m 1.0 / 4.3 / 14.7 m
plane, return to a rally point 184 6.1 / 27.5 / 34.5 m 0.6 / 5.5 / 11.3 m
plane, loiter for ever 93 1.5 / 10.1 / 17.8 m 0.5 / 5.7 / 11.1 m
QuadPlane, VTOL land straight in 141 0.3 / 12.2 / 24.6 m 0.5 / 3.8 / 7.2 m
QuadPlane, land on param1's approach 157 4.2 / 18.6 / 27.3 m 0.7 / 4.8 / 16.7 m
QuadPlane, land on Q_OPTIONS' approach 145 9.2 / 24.7 / 47.5 m 1.5 / 8.2 / 10.2 m
QuadPlane, RTL with Q_RTL_MODE 1 / 2 / 3 497 0.2-8.7 / 11.9-22.5 / 25.0-44.2 m 1.7-2.2 / 3.9-4.6 / 7.3-15.0 m

The worst figures are mostly a QuadPlane's transition and a plane's run down the
runway, neither of which is modelled; after the first 30 s each flight stays
closer. tests/missions/plane-mission-geometry.txt and
copter-mission-geometry.txt are SITL-flyable missions that exercise the
geometry drawing, described in tests/missions/README.md.

Every fix in the branch was checked by reverting it and watching a named test
fail, and the review rounds on this PR were answered the same way.

Known limits

  • The QRTL altitude ramp is ArduPilot master's. Plane 4.7 and earlier go
    straight to RTL_ALTITUDE, so a return entered high is flown lower than it is
    drawn there; the docstring says so.
  • A level circle is drawn as one lap however many it asks for, and LOITER_TIME's
    dwell is not turned into turns; extra laps would sit on top of the first.
  • Spiral turn counts come from TECS_CLMB_MAX / TECS_SINK_MIN, which SITL
    flies a little slower and steeper than.
  • Terrain that never resolves leaves the path at the aircraft's altitude
    (pre-existing).
  • The recordings cite commits from ArduPilot PR 34424, so regenerating them
    needs that branch until it lands.

@tridge

tridge commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-09-14)

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

Reviewed at head 282b7cddd9. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_14_AIReview/devcall_pr_reviews.html#prMAVProxy-1753

Verdict: COMMENT — no blockers, two issues worth fixing. The arc and loiter fixes match ArduPilot's AP_Mission, ArduPlane and logger behaviour, and reverting each fix makes tests fail.

Issues

  • MAVProxy/tools/mavflightview.py:523: with the latest released pymavlink, drawing a tlog mission now crashes.
    • pymavlink 2.4.49, the newest release, has no MAV_CMD_NAV_ARC_WAYPOINT or MAV_CMD_DO_ORBIT.
    • Those references are already on master (e.g. mp_slipmap_util.py:45) and setup.py still asks only for pymavlink>=2.4.38, but master never read tlog missions.
    • Tested with a tlog holding a 3-item mission: the base reads 0 items and draws; this head reads 3 and raises AttributeError. 32 of the new tests fail the same way under 2.4.49.
    • Fix: raise the requirement once a release has the constants, or look them up with getattr.
  • MAVProxy/modules/lib/mp_util.py:256: LOITER_TIME is still drawn at an explicit param3 radius that ArduPlane never flies.
    • Plane::verify_loiter_time() calls update_loiter(0), which uses WP_LOITER_RAD (or LOITER_RADIUS_DEFAULT if |WP_LOITER_RAD| <= 1). AP_Mission keeps only the sign of param3.
    • So a mission loaded from a file, or an upload read from a tlog, with param3=80 and WP_LOITER_RAD=60 is drawn at 80 while the plane flies 60. With |WP_LOITER_RAD| <= 1 the map draws a 1 m circle or nothing.
    • tests/test_mission_geometry.py:200-202 asserts the current behaviour.

Notes

  • Test gap: reverting the callers at mavproxy_map3d/__init__.py:344 or MAVExplorer.py:858 to reading param4 directly (the bug 930075b fixes) still passes every test.
  • Test order: pytest tests/test_mission_geometry.py tests/test_mavflightview.py fails test_a_mission_given_to_draw_is_left_alone with a TypeError. Loading MAVExplorer.py (tests/test_mission_geometry.py:279) sets MAVLINK20=1 after the MAVLink1 dialect is already loaded. Restoring the variable in the test, or setting it in a conftest, fixes it. Also, the mavflightview tests skip only when cv2 is missing, but the tool imports wx.
  • DO_JUMP past the last item: pymavlink's view_indexes() doesn't check that the target exists, so polygon_list() raises AttributeError. That was already reachable from a dataflash log or --mission file; it now also comes from a tlog, including a download that stopped early and is drawn by LogMission.fill() (mavflightview.py:479). Catching it around wp.polygon_list() in mission_objects() (mavflightview.py:643) would keep the plot going. Found by the Codex pass; it predates this PR.
  • mavflightview still doesn't clear the mission on a dataflash "New mission" followed by zero items (MISSION_TYPES excludes MSG, mavflightview.py:391). MAVExplorer now does. Pre-existing.

Checked and correct: clockwise-positive arcs, endpoints and radius at high latitude and across ±180°; param4 crosstrack semantics, including NaN; the LOITER_TIME ±1 direction; "New mission"/"Mission: %u" log handling; the FTP parameter merge; 9 LogMission edge cases; no new module-scope MAVLink constants.

CI: 2 passing (flake8 and the Windows build; no workflow runs pytest).

@tridge

tridge commented Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-09-15)

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

Re-reviewed at head 163018e2ed (previously 282b7cddd9); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_15_1123/devcall_pr_reviews.html#prMAVProxy-1753

Verdict: COMMENT, no blockers. All six findings from my previous comment are fixed. Both reviewers showed each fix is tested: undoing it makes a test fail. The new flown-path code (plane_track.py) matches ArduPlane closely, but it has a main-thread cost in the live 3D map, one hang, and draws some altitudes wrong.

Previous round

  • RESOLVED — tlog missions crashed with released pymavlink 2.4.49. mp_util.py:18-19 now names MAV_CMD_DO_ORBIT and MAV_CMD_NAV_ARC_WAYPOINT itself. All 35 MAVLink names the PR uses exist in 2.4.49. Going back to the dialect lookups gives 33 and 2 test failures under 2.4.49.
  • RESOLVED — LOITER_TIME drawn at its param3 radius. mp_util.py:269 uses WP_LOITER_RAD, falling back to 60 m (:283), as update_loiter() does. Undoing either part fails a test.
  • RESOLVED — no test on the param4 callers. Reverting mavproxy_map3d/__init__.py:375 or MAVExplorer.py:961 now fails a test.
  • RESOLVED — test order. tests/conftest.py restores MAVLINK20; the mavflightview tests also skip without wx.
  • RESOLVED — DO_JUMP past the last item. drawable_mission() (mavflightview.py:208) uses the same bound as AP_Mission::get_next_cmd.
  • RESOLVED — "New mission" followed by zero items. MSG is in MISSION_TYPES (mavflightview.py:413).

New

  • ISSUE — MAVProxy/modules/mavproxy_map3d/__init__.py:402: the live 3D map re-flies a plane mission on the main thread whenever home moves.
    • The cache key holds home's exact lat/lon. While disarmed, ArduPlane updates home every 5 s (Plane.cpp:396-400) and sends HOME_POSITION each time, so normal GPS wander misses the cache every time.
    • Measured: 0.05 s for the Kalaupapa mission, 0.2 s for a ~1.8 h survey, 5.4 s for a mission that reaches the 8 h cap. missionpath defaults to flown.
    • SITL GPS doesn't wander by default, so SITL testing won't show it.
    • Fix: ignore home moves under a few metres in the key, or compute off the main thread.
  • ISSUE — MAVProxy/modules/lib/plane_track.py:336: a mission crossing ±180° longitude runs for minutes. local() subtracts longitudes without wrapping. A 223 m leg from 179.998° to −179.999° took 99 s and returned 1.87 million points; the same leg without the crossing takes 0.00 s. The 8 h cap is only checked between items, so it doesn't stop this. ArduPilot wraps the difference (Location.cpp, diff_longitude).
  • ISSUE — the flown path is drawn at the wrong altitude in two common cases.
    • mavproxy_map3d/__init__.py:335: an item with no position, such as a LOITER_TO_ALT at 0,0, is passed with its altitude set to None. So it is flown at the current altitude. ArduPlane's Location::sanitize() replaces only the lat/lon and keeps a non-zero altitude.
    • plane_track.py:764-767: NAV_RETURN_TO_LAUNCH flies home at the current altitude. ArduPlane uses get_RTL_altitude_cm(), which is RTL_ALTITUDE above home (100 m by default) and only uses the current altitude when RTL_ALTITUDE < 0 (altitude.cpp:99-105). Changing RTL_ALTITUDE doesn't change the drawn path.
  • NOTE — smaller differences from ArduPlane:
    • plane_track.py:580: a LOITER_TURNS of 2.5 is drawn as 2.5 turns, but AP_Mission stores the integer part, so ArduPlane flies 2.
    • plane_track.py:451: ALT_SLOPE_MIN <= 0 disables the glide slope in ArduPlane; the port keeps it.
    • A mission over 8 h silently falls back to the geometry drawing, after up to ~5 s of work.
  • NOTE — test gap: changing plane_track.py:559-561 to use param3 as the LOITER_TIME radius (the bug fixed in mp_util) still passes all tests.

Checked and correct:

  • the L1 controller port: signs, units, bank limit, capture/circle switch;
  • loiter radius and direction, turn counting, and lining up to leave;
  • waypoint acceptance and pass-by;
  • DO_JUMP/DO_JUMP_TAG, and termination for LOITER_UNLIM and repeat −1 (peak memory 2.8 MB);
  • ArduPlane parameter defaults.

Tests: 169 passed in the three new files under both pymavlink 2.4.49 and master (Codex: 282 passed, 10 skipped for the whole suite). flake8 is clean. CI: 2 passing (flake8, Windows build); no workflow runs pytest.

@tridge

tridge commented Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head 918c407c8c (you were last told 163018e2ed); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_15_1710/devcall_pr_reviews.html#prMAVProxy-1753

All 8 findings from my previous comment are resolved, and 7 of the 8 fixes are shown tested by mutation. This push adds a MAVExplorer crash on older logs and a test that fails almost every run here, so the verdict is REQUEST CHANGES. Both fixes are one line.

Previous round

  • RESOLVED: map3d re-flying on the main thread as home wanders.
    • The flight now runs on a worker thread, and a home move under HOME_MOVE_CHANGE (10 m) reuses the cached path (mavproxy_map3d/__init__.py:414-419, :486-537).
    • Setting HOME_MOVE_CHANGE = 0 fails a test.
    • This holds for home-relative missions only; see the ISSUE below.
  • RESOLVED: antimeridian runaway. plane_track.py:351-354 and terrain.py both wrap the longitude difference. Removing either wrap fails a test, and a 300 m leg across 180° now takes 0.00 s.
  • RESOLVED: a positionless item flown at the current altitude. positionless_amsl() (plane_track.py:843-855) keeps its altitude, as Location::sanitize() does. Dropping it fails a test.
  • RESOLVED: RTL flown at the current altitude. rtl_location() (plane_track.py:539-562) follows get_RTL_altitude_cm() and the AP_Rally nearest-point/RALLY_LIMIT_KM/INCL_HOME logic, and matches the rally frame flags.
  • RESOLVED: LOITER_TURNS 2.5 drawn as 2.5 (plane_track.py:628-632). RESOLVED: ALT_SLOPE_MIN <= 0 (:470-472). Undoing either fails a test.
  • RESOLVED by design change: the silent fallback after ~5 s for an 8 h mission.
    • The cap is now 4 h, a mission is rejected up front by least_flight_time(), and the work is off the main thread.
    • The fallback to the geometry drawing is still silent while the style says "flown" (mavproxy_map3d/elements.py:552). It is cheap now.
  • RESOLVED: LOITER_TIME radius test gap (tests/test_plane_track.py:508). Using param3 as the radius now fails it.

New — please fix

  • BUG: MAVProxy/tools/MAVExplorer.py:734 crashes the 3D map on any pre-4.5 log that has rally points.
    • rally[m.Seq] = (m.Lat, m.Lng, m.Alt, m.Flags) needs Flags, which RALY only gained in 4.5: Copter-4.4.4 logs "QBBLLh" with TimeUS,Tot,Seq,Lat,Lng,Alt, Copter-4.5.0 logs "QBBLLhB" with ...,Flags.
    • pymavlink's DFMessage raises AttributeError for a missing field, and nothing around the call at :860 catches it. This affects every vehicle type.
    • Reproduced through mission_from_log() with a Flags-less RALY.
    • Fix: getattr(m, 'Flags', 0), which is what rally_amsl() already assumes for flags 0, plus a test with an old-format RALY.
  • ISSUE: tests/test_plane_track.py:932-936 (test_a_path_for_a_mission_since_changed_is_not_drawn) is racy.
    • The second send_mission() goes in before the worker has taken the first. fly_tracks() then correctly merges them and flies only the newer mission, which the assertion at :943 rejects.
    • It failed 9/10 in one pass and 10/10 when I re-ran it on Python 3.14. The cross-check reproduced it too.
    • Adding self.wait_for(lambda: calls) before changing the mission (the pattern flying_slowly() already uses) makes it pass 10/10.
    • No workflow runs pytest, so CI can't show this.
  • ISSUE: mavproxy_map3d/__init__.py:442-451: a mission with any AMSL-frame item still re-flies on every home update.
    • The cache key stores every altitude as amsl - home[2] (:445, and :450 for rally points). That is constant only for home-relative items, so an AMSL item's key changes whenever home's altitude wanders.
    • Measured with your live_module harness, 10 home moves of 1.5 m with ±0.3 m altitude: the relative mission flew once in 11 sends, and the same mission with one item switched to frame 0 flew 11 times.
    • With the flight slower than the home interval, most results are dropped as stale, so the path is rarely drawn.
    • Fix: key AMSL altitudes absolutely, and only relative ones against home.

Notes (non-blocking)

  • MAVExplorer.py:731-732 resets the rally table whenever a RALY with Seq == 0 arrives.
    • ArduPilot also logs a single RALY each time one point is set (AP_Rally.cpp:131), so editing only point 0 mid-log makes MAVExplorer forget points 1..n and pick RTL from the wrong set.
    • A full upload re-logs every point, so this is an edge case. Clearing on a change of Tot, or dropping entries with Seq >= Tot, avoids it.
  • The flown RTL path ends at the RTL point (plane_track.py:817-820, :834-837), whereas ArduPlane then loiters there at abs(RTL_RADIUS) in the direction its sign gives (ArduPlane/mode_rtl.cpp:98-103). This isn't new in this push, but a circle there would match what the aircraft does.
  • plane_track.py:937: least_flight_time() now runs outside the try at :941-944, so a NaN DO_JUMP param raises ValueError out of mission_track(). The map3d thread catches that; MAVExplorer's fly() doesn't. Moving it inside the try fixes it.
  • mavproxy_map3d/__init__.py:513: moved_home() also shifts legs to AMSL items by home's altitude change. It is bounded by the 10 m tolerance, so it's cosmetic.
  • Test gaps: removing the least_flight_time() pre-check still passes test_a_mission_too_long_to_fly_is_not (tests/test_plane_track.py:407); only the run time changes. The cross-check also found that allowing a DO_JUMP target equal to the item count passes the invalid-jump test.

What was checked

  • Released pymavlink: 22 of the 24 MAVLink names this PR's lines use exist in PyPI pymavlink 2.4.49. The other two are the literals at mp_util.py:18-19, so nothing can fail at import.
  • Degenerate-mission fuzz: all of these finished within 0.42 s with sensible output: jump loops, zero radius, repeated points, LOITER_UNLIM, NaN params, near the pole, and across the antimeridian.
  • Tests, from the checkout under released pymavlink:
    • Targeted: 146 passed, 1 failed (the racy test), 47 skipped for lack of wx.
    • Cross-check, full suite: 306 passed, 10 skipped, the same one failure.
  • Other checks:
    • flake8 (the CI command) is clean.
    • vermin finds nothing new in the added lines.
    • mp_util.py, mavflightview.py and conftest.py are unchanged since last round.
  • CI: flake8 passed. The Windows build was still running when checked.

REQUEST CHANGES: please fix the RALY Flags crash and the racy test. The AMSL cache key is worth doing in the same pass.

@AP-Review

AP-Review commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-16)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head fe8c38484e (you were last told 918c407c8c); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_16_1759/devcall_pr_reviews.html#prMAVProxy-1753

(My previous comment above is superseded by this one. It's left un-collapsed because these reviews now post from the AP-Review account, which can't edit the earlier comments posted under tridge.)

Every blocking finding from my previous comment is resolved, and all but one are locked in by a test that fails when the fix is reverted. Verdict REQUEST CHANGES → COMMENT. I am also withdrawing part of one of my own findings. The new items below are accuracy problems in the drawn path rather than blockers.

Previous round

  • RESOLVED: the RALY Flags crash on pre-4.5 logs. MAVExplorer.py:745 now reads getattr(m, 'Flags', 0), with a test at tests/test_plane_track.py:1571. Reverting to m.Flags fails that test, and a real DFMessage built from the Copter-4.4.4 QBBLLh format now goes through mission_from_log() cleanly.

  • RESOLVED: the racy path test. The wait_for is at tests/test_plane_track.py:1139. 25/25 passes at this head; removing the wait gives 6/25.

  • RESOLVED: the Seq == 0 rally reset. MAVExplorer.py:739-744 prunes on a change of Tot and ignores Seq >= Tot. Reverting fails test_mavexplorer_takes_rally_points_from_an_older_log.

  • RESOLVED: RTL ending at the point instead of loitering. plane_track.py:843-852 appends a NAV_LOITER_UNLIM with param3 = self.rtl_radius, so the sign gives the direction and 0 falls back to WP_LOITER_RAD. The test at :389 covers RTL_RADIUS 0 / +250 / −250 and fails when the appended loiter is removed. (See the new item below about the approach to that circle.)

  • RESOLVED: moved_home() shifting AMSL legs. The alt_moves argument at mavproxy_map3d/__init__.py:535, applied at :545. Always-shifting fails a test.

  • RESOLVED: both test gaps. tests/test_plane_track.py:445 catches the removal of the least_flight_time() pre-check, and :470 catches a DO_JUMP target equal to the item count. Both mutations now fail.

  • RESOLVED in code, still untested: least_flight_time() inside the try. It is at plane_track.py:980-984 and a NaN DO_JUMP now returns None instead of raising out of mission_track(). But moving the call back outside the try still passes the whole suite — there is no nan anywhere in the test file. One test would lock it in.

  • PARTIALLY RESOLVED, and I am withdrawing half of my own finding: the AMSL cache key. keyed_alt() at mavproxy_map3d/__init__.py:462-464 is the fix I asked for, and it works — an all-AMSL mission now flies once in 11 sends where it previously flew 11 times. The key += (round(home[2], 2),) at :479-482 deliberately re-adds home altitude for a mixed mission, and you are right that a mixed mission cannot be served from cache: moved_home() shifts the whole path by one dalt, which is wrong when only some legs move with home. Re-flying is the honest answer and both halves are tested.

    Withdrawn: I claimed last round that the re-flights were "mostly dropped as stale, so the path is rarely drawn". I could not reproduce that at a realistic home rate. Measured flight times at this head: a 40-item survey takes 0.06 s, 240 items 0.34 s, 400 items 0.45 s — against a 5 s home interval. The staleness only appears if the home interval is forced down to 0.05 s. Please disregard that part.

New — the drawn path can be silently wrong or silently absent

  • BUG: a LOITER_TURNS above ~40 turns silently produces no flown path at all. plane_track.py:653 sets extra = MAX_LAPS * seconds_per_lap — a flat 20-lap allowance that is not scaled by the requested param1. So a legitimately long loiter exhausts its own time limit and mission_track() returns None. Measured with the test fixture's parameters, a loiter followed by a waypoint:

    radius 80 m (WP_LOITER_RAD default): drawn up to 39 turns, NOT DRAWN from 40
radius 200 m:                        drawn up to 32 turns, NOT DRAWN from 33

    The comment at :69-76 describes MAX_LAPS as a stuck-detector — "the aircraft is taken to be stuck, circling a waypoint it cannot turn tightly enough to reach" — so I don't think a 40-turn loiter is an intended exclusion; it just falls under the same budget. Budgeting the requested turns separately from the stuck allowance would fix it. This lands badly in combination with the next item.

  • ISSUE: the fallback from "flown" to geometry is still silent, and the setting still reads flown. mavproxy_map3d/__init__.py:444-447 and elements.py:576-583. Measured on a copter with missionpath='flown': tracks handed to the viewer [None], setting still reads flown, nothing printed. Same for a mission declined for length, and now also for the turn-count case above. A user who explicitly types map3d set missionpath flown gets geometry with no indication that the thing they asked for could not be produced. One print() when the requested style cannot be honoured would cover all three.

  • ISSUE: the drawn RTL flies through the centre of the return circle before capturing it. plane_track.py:840 completes waypoint navigation to the RTL location and only then appends the loiter and calls fly_loiter(). ArduPlane calls update_loiter() during RTL navigation (ArduPlane/mode_rtl.cpp:98), so it captures the circle on approach. Measured at this head, a 3 km return with RTL_RADIUS = 250:

    closest approach to the RTL point, final half of the path: 0.53 m
the path then settles onto the circle at 267 m

    So the drawing shows an overflight of the centre that the aircraft will not fly. (These are model measurements, not SITL.) The 267 m against a requested 250 m is the L1 flown-radius adjustment and looks right.

  • ISSUE: a positionless item is classified from its raw frame, so an all-terrain mission is re-flown on every home update. fixed_alt.append(frame in (0, 5)) appears twice with the same expression but different data: at mavproxy_map3d/__init__.py:354 (positionless branch) frame is still the raw wploader frame, while at :404 (positioned branch) it has already been rewritten to 0 by the terrain resolution at :365-368. Both my passes found this independently. Measured over 11 sends:

    AMSL waypoints + positionless DO_CHANGE_SPEED at frame 3   -> flown 11 times
AMSL waypoints + positionless DO_CHANGE_SPEED at frame 0   -> flown  1 time
terrain waypoints + positionless DO_CHANGE_SPEED, frame 10 -> flown 11 times

    The frame-10 case is the clean defect: positionless_amsl(z, 10, home_amsl) returns None, so that item contributes None to the key and nothing about it moves with home — yet it is counted as moving and forces the whole mission to be re-flown. Resolving the frame before :354, or treating an item whose amsl is None as neither fixed nor moving, would fix it. An altitude-independent command like DO_CHANGE_SPEED arguably shouldn't enter this classification at all.

  • ISSUE: for a genuinely mixed mission the cache key pins home altitude to 1 cm, while same_home() tolerates 10 m. mavproxy_map3d/__init__.py:482. So even a correctly-classified mixed mission re-flies on essentially every home update. Measured over 10 home updates wobbling by a few cm: all-relative 1 re-fly, all-absolute 1 re-fly, mixed 10 re-flies. Replacing round(home[2], 2) with a quantisation at the granularity same_home() already tolerates gives 1 re-fly for the mixed case. Mixed is an ordinary shape here — send_mission() sets frame = 0 for a terrain item once terrain resolves (:366-369), so a terrain-following plane mission is mixed by construction, and ArduPlane re-sends HOME_POSITION every 5 s while disarmed (ArduPlane/Plane.cpp:397-404). Cost is ~32 ms per fly-through on your own tests/missions/plane-mission-geometry.txt, so it is minor, but reset_flown_track's own docstring notes a flight "can take a second or more".

  • ISSUE: the drawn rally marker disagrees with where the flown path returns to. send_rally() (mavproxy_map3d/__init__.py:677-686) sends the raw r.alt and elements.py:898-903 draws it at home_amsl + alt regardless of frame, while the new rally_points() at :504 resolves the frame properly. With a rally point at alt=100, home 584 m, origin 500 m, terrain 300 m:

    frame           drawn marker AMSL    RTL destination AMSL
absolute              684.0                 100.0
above home            684.0                 684.0
above origin          684.0                 600.0
above terrain         684.0                 400.0

    Relatedly, the two new self.send_rally() calls in the GPS_GLOBAL_ORIGIN/ORGN handlers at :816 and :821 cannot change anything — send_rally()'s output is identical after the origin changes — so they are no-ops as written.

  • ISSUE: MAVExplorer.py:987 re-arms mission_to_fly even when the flown path already came back None. m3d.mission_to_fly = (mission, fly) if track is None else None doesn't check that missionpath was already flown, so the next unrelated set re-runs the whole computation (measured: plane_mission_track calls go 1 → 2 after set showdirection true). Adding and mestate.settings.missionpath != 'flown' covers it.

Adjusted from the cross-check

  • My cross-check reported the unresolved-terrain case as a bug: plane_track.py:374 replaces an unknown AMSL with the aircraft's current altitude, and terrain-relative waypoints at 100 m AGL were measured drawing a path flat at 584 m AMSL. I have downgraded it, because item_amsl() at :321-324 documents None as "until then nobody knows" and :769-772 re-runs send_mission() once a deferred terrain lookup lands, so the wrong path is transient. The residual worth noting is the case where terrain never resolves (no tile, no network, terrain module off): the path then stays silently at the aircraft's altitude, because location() cannot distinguish "unknown" from "keep current altitude". A one-line note to the user, or suppressing the flown path while terrain is outstanding, would close it.

  • My cross-check also reported that test_a_loiter_is_left_lined_up_on_what_comes_next (tests/test_plane_track.py:152) passes with the exit-alignment check disabled. That part is true — I reverted lined_up() to return True and that test still passed. But it is not a coverage gap: the same mutation fails test_the_path_is_flown_in_the_order_flown and test_the_path_is_the_one_flown. So the behaviour is covered; it is only that the test named for it asserts eventual convergence on the next leg rather than heading at the loiter exit. Worth a tighter assertion, nothing more.

Notes (non-blocking)

  • mavproxy_map3d/__init__.py goes from 0 to 2 flake8 violations — :408 E128 and :461 E306 — and MAVExplorer.py gains one E126. CI cannot see any of them: scripts/run_flake8.py only checks files carrying AP_FLAKE8_CLEAN, and that file doesn't carry it (it was clean before this PR, so the marker would be worth adding).
  • 58 of the 156 new tests (37%) silently skip without wxPython, which is in the recommended extra, not install_requires — and CI runs no tests at all (.github/workflows/ is flake8 + a Windows build; grep -rn pytest .github/workflows/ is empty). So nothing anywhere verifies more than a third of the new suite.
  • :425 has a stale comment — track_wanted is a 5-tuple now, not (key, flown_from, home, items).
  • MAVExplorer.py:127-129 duplicates the style list and default as literals instead of using map3d.MISSION_STYLES, which the live module does use; they can drift.
  • mavproxy_map/__init__.py:166 binds self._label_suffix_for_wp_command to the shared mp_util.MISSION_LABEL_SUFFIXES dict. Nothing aliases it today, but a future per-instance edit would mutate the table the 3D map reads.

What was checked

  • The PR fixes 8 real failures that master has under released PyPI pymavlink 2.4.49 — the MAV_CMD_DO_ORBIT AttributeError at mavproxy_map/__init__.py:163 and the test_mission_geometry cases. That is exactly the class of bug CI cannot see, and it is worth calling out as a win rather than leaving it in the noise.
  • Released-pymavlink safety verified for real, not by eye: every module-scope mavutil.mavlink.* reference across all ten changed source files enumerated by AST and checked against 2.4.49 — 11 references, 0 missing. The two that 2.4.49 genuinely lacks (MAV_CMD_DO_ORBIT, MAV_CMD_NAV_ARC_WAYPOINT) are now numeric literals 34 and 36 in mp_util, which I checked against common.xml. All five touched modules import cleanly under 2.4.49.
  • Suite: 352 passed, 10 skipped on Python 3.10.21 / 3.11.16 / 3.12.14 / 3.13.15 / 3.14.4, under both released 2.4.49 and pymavlink git master — identical on all ten combinations. Five consecutive full runs identical. 13 thread- and timing-sensitive tests run 25× each in isolation: 325/325 passes, and 25/25 again under loadavg 47 on a 32-core box. No flake found; I could not reproduce last round's Python-3.14 failure at this head.
  • The new labelling work is genuinely covered: six mutations of test_map3d_mission_labels.py's targets, six failures, no survivors. Rendered offscreen to confirm the labels are actually visible (non-black pixels 1459 → 1847 → 1459 on toggle, image byte-identical after toggling back, no actor leak). Edge cases are clean — unknown MAV_CMD falls through .get() rather than raising, NaN lat/lon/alt and seq=None produce no exception, 700 labels refresh in 0.065 s.
  • Setting validation is correct on both sides: set missionpath bogus prints Must be one of ['flown', 'geometry', 'plain'] and leaves the value unchanged; PLAIN is accepted case-insensitively. (elements.set_mission_style() itself ignores an unknown style silently, but the setting layer never lets one through.)
  • Thread-safety of this push's changes: track_request/track_result/track_thread all touched under track_lock, track_wanted only on the main thread, all three assignments widened to 5-tuples consistently with the single wanted[4] read, and the stale-result guard wanted[0] != key exercised.
  • Nothing in the arc-drawing code is new here — this push's only change to mp_slipmap_util.py and mavproxy_map/__init__.py is sourcing the two command numbers from mp_util; the arc geometry is already on master.
  • CI snapshot: both build and build (3.10) passing.

Not reviewed: the ported rally_alt_frame/rally_amsl frame decoding against AP_Rally::rally_location_to_location, mission_from_log's new RALY/ORGN handling against a real log with rally points (I have none), and path_view()'s span maths on an antimeridian-crossing log.

COMMENT — nothing blocks merge. The turn-count cap and the silent fallback are the pair I'd most want fixed, since together they mean a legitimate mission loses its flown path with no explanation.

@AP-Review

AP-Review commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-16)

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

Re-reviewed at head 3e5a9b7062 (previously fe8c38484e); my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_17_0037/devcall_pr_reviews.html#prMAVProxy-1753

Verdict: COMMENT (was REQUEST CHANGES) — twelve of fifteen previous findings are fixed, and 24 of 26 mutations I tried are caught by the new tests, each with a named failing test. Nothing here blocks merge. Two new accuracy findings, one of which corrects something I got wrong last round.

Previous round

Resolved: the LOITER_TURNS time budget (radius 80 m now draws at least 295 turns, was not drawn from 40); the silent flown→geometry fallback; the RTL flying through the circle centre; positionless-item classification; the mixed-mission cache key; the rally marker and the no-op send_rally() calls; MAVExplorer re-arming mission_to_fly; least_flight_time() inside the try; the flake8 regression (map3d/__init__.py now carries AP_FLAKE8_CLEAN); the stale :425 comment; the duplicated style list; and the shared label dict.

Still open, all re-raised rather than new: CI runs no tests at all (repo-level, not yours — with wx blocked 88 tests skip against 13 normally); terrain that never resolves leaves the path silently at the aircraft's altitude (plane_track.py:374), which say_unflown does not cover because a path is produced; and test_a_loiter_is_left_lined_up_on_what_comes_next still asserts convergence rather than exit heading (cosmetic).

ISSUE — MAVProxy/modules/lib/plane_track.py:1052 — QRTL draws an ordinary glide slope, not ArduPilot's approach ramp

I need to correct my previous comment here. Last round I traced Q_RTL_MODE 1/2/3 and said the port was correct. It is not, and my reasoning was faulty: I inferred correctness from the fact that mutating "Q_RTL_MODE 3 returns at RTL_ALTITUDE" is caught by the suite. It is caught — by tests/test_plane_track.py:695, which asserts on track[-1], the final point only. That pins the destination altitude and says nothing about the 3 km of return in between.

:1034-1052 sets the destination to home_amsl + q_rtl_alt and then calls fly_waypoint(), which drives altitude only through MissionWalk.update_target_altitude (:504-523) — a port of fixed-wing Mode::update_target_altitude, i.e. a proportional glide slope. There is no port of ModeQRTL::update_target_altitude in the file.

ArduPilot does something else. Q_RTL_MODE=3 enters QRTL immediately (mode_rtl.cpp:21-25); in fixed-wing cruise the motors are not THROTTLE_UNLIMITED, so mode_qrtl.cpp:6-77 falls through to do_RTL(home.alt + qrtl_alt*100) and poscontrol_init_approach(). With Q_TRANS_DECEL=2, transition_threshold() is 181.5 m, so at 3 km the state is QPOS_APPROACH and mode_qrtl.cpp:186-215 overrides the fixed-wing slope, holding RTL_ALTITUDE until sink_dist from home.

Executed with RTL_ALTITUDE=100, Q_RTL_ALT=15, Q_RTL_MODE=3, mission [waypoint(3000,0), RTL]:

distance from home ArduPilot target model draws error
1200 m 100.00 m 50.23 m −49.8
990.9 m 100.00 m 44.73 m −55.3
807 m 100.00 m 39.79 m −60.2
501 m 61.50 m 31.54 m −30.0
195 m 19.77 m 23.31 m +3.5

(rtl_alt_delta=85, sink_dist=623.3 m, so the real ramp reaches RTL_ALTITUDE at 783.3 m.) Not version-sensitive — the ramp has been there since 3bb840f794 (2021-06-01) and is still on master. Only Q_RTL_MODE=3 is affected: mode 1 draws 100 m all the way, which matches RTL, and mode 2 goes through fly_vtol_approach. Accuracy only; the horizontal path is unaffected. The comment at :1035-1036 is right about the destination and silent about the approach.

Your own fixture cannot see it either: sitl-quadplane-rtl-q-rtl-mode-3.json was recorded with RTL_ALTITUDE=20, Q_RTL_ALT=15, so rtl_alt_delta=5 m and the ramp is flat there by construction.

ISSUE — tests/test_plane_track.py:1017 — the SITL comparison is horizontal only

distances() projects to (north, east) and drops the third element; arrivals() uses gps_distance. Making every drawn point 200 m too high leaves all 20 TestFlownMission assertions green (20 passed in 1.78s). The recordings already carry altitude (record_flight.py:141 writes round(m.Alt, 1)), so a vertical assertion is nearly free — and it is the highest-value thing you could add, since this PR moves a lot of altitude logic and none of it is checked against a real flight. On the mode-3 recording drawn is 4–7 m high throughout (at 990 m: flown 70.1, drawn 74.2) and nothing looks.

ISSUE — tests/missions/*.json — the fixtures cite commits and autotests that do not exist publicly

gh api repos/ArduPilot/ardupilot/commits/cd2319308b → 422 "No commit found"; same for f2c18f532f, and same against peterbarker/ardupilot. Plane.MissionItemTypes, QuadPlane.VTOLMissionItemTypes and QuadPlane.KalaupapaCanyonRun are in neither arduplane.py nor quadplane.py on master, GitHub code search returns 0 for all three, and no open ArduPilot PR mentions them. So the ground truth for the headline claim of 3e5a9b70 cannot be regenerated by anyone else, and "ArduPilot's autotest now has flights meant for this" is not true upstream yet. Either land the ArduPilot side first and cite resolvable SHAs, or say in the commit and in source that these came from a working branch. (Still better than the old canyon fixture, which cited no SHA at all.)

Notes

  • map3d/__init__.py:909 — a non-finite WIND.direction raises out of mavlink_packet: nan → ValueError, inf → OverflowError (driven through the real handler). ArduPilot sends wrap_360(degrees(atan2f(...))), NaN if the EKF wind estimate is NaN. mavproxy_link.py:1139 catches it, so at default moddebug 0 the message is silently dropped. One-line math.isfinite guard.
  • tests/test_map3d_mission_labels.py:204 — imports map3d_ui (→ wx) at line 204, before the fixture's pytest.importorskip("wx") at 205, so it fails rather than skips without wxPython. New in ce2179a5; it is the only one of your tests that fails in that configuration.
  • terrain.py:89 — the new whole-fetch deadline cannot interrupt a slow buffered read, because response.read(64*1024) blocks until its buffer or EOF and each recv resets the socket timeout. Measured: timeout=0.1 with a 4096-byte body dripped at 64 B/50 ms → 3.163 s; timeout=5.0 with 65536 bytes at 2048 B/400 ms → 12.410 s. This is still an improvement on master, which had no whole-fetch deadline at all — the guard is just partial. One caller, already wrapped in try/except, so it is a nit. (Also :94-95 uses %.0f, so timeout=0.1 prints "took longer than 0s".)
  • record_flight.py:117 — if value: drops a legitimately-zero heading/approach; a due-north approach would be recorded with no approach and replayed at STILL_AIR_APPROACH, a 180° error. if value is not None is the intent.
  • test_plane_track.py:1068 — near = abs(params[1]) * 1.4 is 0 for a LOITER_TO_ALT with param2 0 (meaning WP_LOITER_RAD). Latent; no current fixture has one.

Suite at this head: 414 passed, 13 skipped, 20 subtests on Python 3.14.4 / pymavlink 2.4.49; five full runs identical under randomised ordering; the six timing-sensitive TestDrawnTrack tests 25/25 in isolation. 11 module-scope mavutil.mavlink.* references enumerated by AST across the 13 changed files, 0 missing under released 2.4.49.

@AP-Review

AP-Review commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-18)

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

Re-reviewed at head b669fe9444 (previously 3e5a9b7062); my earlier AI comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_18_1127/devcall_pr_reviews.html#prMAVProxy-1753

Both accuracy ISSUEs from my previous comment are properly fixed, and I locked each one in with the exact mutation that exposed it last round. Four new commits, all of them responding to that comment. Verdict: COMMENT — everything new this round is a NOTE.

Resolved since last round

  • QRTL drew an ordinary glide slope, not ArduPilot's approach ramp. plane_track.py:532-564 is a new update_qrtl_target_altitude(), dispatched at :510-512 and armed at :1101. I diffed it line by line against ArduPlane/mode_qrtl.cpp on master — radius, rtl_alt_delta, sink_time, sink_dist, rad_min/rad_max, the approach-start latch, the ALT_SLOPE_MIN gate, the far-field interpolation and the don't-climb-back-up clamp all match, including the quirk that raw WP_LOITER_RAD is used with no LOITER_RADIUS_DEFAULT fallback; 10,000 numeric comparisons differed by < 0.00027 m. Mutation-verified three ways: removing the dispatch fails the new unit test and the mode-3 flight comparison; removing the clamp fails test_qrtl_does_not_climb_back_up_to_its_slope; neutering the ALT_SLOPE_MIN gate fails test_qrtl_comes_home_on_its_own_altitude_profile.
  • The SITL comparison was horizontal only. tests/test_plane_track.py:1122-1148 adds heights() (3D nearest-point-on-segment) and :1249-1256 adds test_the_path_is_flown_at_the_height_flown, with p90/worst limits per fixture. The same +200 m probe that left all 20 assertions green last round now fails 10 of 10 fixtures. Margins are ~1.4× actual — median absolute height error 0.5–4.2 m across the ten recordings, p90 3.8–16.8 m against limits of 6–25 m. Tight enough to be worth something, loose enough not to be brittle. This is the highest-value thing in the push, and re-recording the mode-3 fixture at RTL_ALTITUDE=60 (was 20) is what makes it actually discriminate the ramp — that was the specific weakness I raised.
  • The fixtures citing commits and autotests that did not exist publicly. Mostly closed, and I need to retract part of what I said: the source strings now name ArduPilot PR 34424, which exists, is open, and whose diff really does add def MissionItemTypes, def KalaupapaCanyonRun and def VTOLMissionItemTypes — so my claim that these autotests were nowhere public is out of date. cd2319308b, f2c18f532f and 6ea2776f35 all resolve. Residual below.
  • A non-finite WIND.direction raised out of mavlink_packet. map3d/__init__.py:906 now guards with math.isfinite; reverting reproduces the NaN ValueError and fails the new test_a_wind_with_no_estimate_changes_nothing (which covers nan, +inf, -inf).
  • test_map3d_mission_labels.py failed rather than skipped without wxPython. :205 now calls map3d_frame() before importing Map3DFrame; it skips with wx blocked.
  • The terrain whole-fetch deadline couldn't interrupt a slow buffered read, and %.0f printed "0s". terrain.py:89 uses read1 and %g. Re-measured against a real local dripping HTTP socket: 4096 B at 64 B/50 ms with timeout=0.1 went from 3.166 s to 0.108 s, and 65536 B at 2048 B/400 ms with timeout=5.0 from 12.406 s to 5.203 s — reproducing my previous numbers exactly. Residual: the deadline still isn't hard — slowly delivering a valid chunk header delays the timeout to 0.404 s with timeout=0.1, because read1() waits for that header. Still a large improvement; noting it so the guard isn't mistaken for absolute.
  • record_flight.py dropped a legitimately-zero heading/approach. New extras() at :51-59; restoring if value: returns {} and fails test_a_course_of_north_is_recorded.
  • near = abs(params[1]) * 1.4 was 0 for a default-radius LOITER_TO_ALT. Fixed at :1178-1182 with a named regression test — see N3 for the remaining hole one level up.

Still open and re-raised rather than new, all outside this PR: CI runs no tests at all (grep -l pytest .github/workflows/ returns nothing, so tests/ isn't even linted — and the 429 tests here are never executed); unresolved terrain still leaves the path silently at the aircraft's altitude (plane_track.py:406-407 — a live probe with unresolved 100 m AGL waypoints produced 291 points all at 584 m AMSL with the style still flown, so say_unflown doesn't cover it); and test_a_loiter_is_left_lined_up_on_what_comes_next still asserts convergence rather than exit heading.

New — all NOTEs

N1 — plane_track.py:551-558 — the far-field QRTL ramp is ArduPilot-master-only; no released firmware flies it.

The gradual descent from the approach-entry altitude down to RTL_ALTITUDE was added in bd9c0285fc (2026-08-20) and refined by 3dc0c3621b (2026-08-22). git tag --contains confirms neither is in any Plane-4.7.x tag — both are master-only. Plane-4.7.1 still has the single linear_interpolate(0, rtl_alt_delta, dist, rad_min, dist_rtl_alt_reached), which clamps beyond dist_rtl_alt_reached, i.e. holds RTL_ALTITUDE with no slope.

Measured with the new test's own parameters (entered at 300 m, RTL_ALTITUDE=100, Q_RTL_ALT=15, RTL_RADIUS=250, 3 km return):

dist from home drawn (master) ArduPlane 4.7.1 error
2500 m 248.8 148.7 +100.2
2000 m 199.4 100.7 +98.7
1500 m 147.8 100.0 +47.8
1123 m 110.7 100.0 +10.7
900 m 83.3 82.6 +0.7

Below reached (1123 m here) the two agree. Modelling the released behaviour against your mode-3 recording gives a p90 height error of 25.9 m against a 6.0 m limit, so the fixture genuinely discriminates them — the choice is real rather than academic.

I don't think this blocks anything: tracking master is a defensible call, and the previous state (an ordinary glide slope) was wrong for both. But MAVProxy is mostly pointed at released firmware and the divergence is ~100 m. One line in the update_qrtl_target_altitude() docstring saying the far-field slope is master (post-bd9c0285fc) and that 4.7 and earlier snap to RTL_ALTITUDE would save the next reader comparing against a 4.7 flight.

N2 — tests/missions/sitl-quadplane-*.json — the six QuadPlane recordings still cite a SHA that resolves nowhere. All six cite ardupilot 8d7ed31dfe; it returns 422 "No commit found" from both ArduPilot/ardupilot and peterbarker/ardupilot, and it isn't one of PR 34424's three commits. And these are exactly the six fixtures whose flight data was re-recorded this round — comparing parsed JSON, flown, home, items, heading and start all changed in all six, with mode-3 additionally moving RTL_ALTITUDE 20 → 60. The four Plane fixtures changed source only. Separately, grep RTL_ALTITUDE over PR 34424's Tools/autotest/quadplane.py patch returns nothing, so the published VTOLMissionItemTypes wouldn't reproduce the 60 m recording. Net: the six fixtures whose data is new are the six nobody else can regenerate. Much better than last round; to close it, push the RTL_ALTITUDE change into PR 34424 and re-cite a commit from it. (The commit message for 2aa370fa is already honest about RTL_ALTITUDE 60, so it's just the source strings catching up.)

N3 — tests/test_plane_track.py:1178-1181 — the new WP_LOITER_RAD fallback reintroduces the same hole one level up. It stops at the parameter itself and doesn't apply ArduPlane's |WP_LOITER_RAD| <= 1 → LOITER_RADIUS_DEFAULT rule, which the model does apply at plane_track.py:710. Executed: WP_LOITER_RAD=0 gives helper near=0.0 against model radius=60.0, so a fixture with WP_LOITER_RAD 0 or ±1 and a default-radius LOITER_TO_ALT would report the item as never reached — the failure just fixed, moved up a level. Latent; no current fixture has it. Factoring the rule out of :710 into a shared helper would stop the two drifting again.

N4 — tests/test_plane_track.py:1122-1148 — heights() measures to the 3D-nearest point, which is right for a spiral (as its docstring says) but means a uniformly-too-high path can partly match a different part of itself. Clearly not fully absorbed — the +200 m mutation fails all ten, and the medians are too good for it to be hiding much. Recording it only so it isn't mistaken for a strict vertical-offset check; no change needed.

N5 — plane_track.py:1205-1207: linear_interpolate's polarity swap is unreachable from both call sites and deleting it leaves the suite green (429 passed). It's a faithful port of AP_Math.cpp's, so keeping it is right — flagged only so it isn't mistaken for covered code. And tests/test_plane_track.py:1161 has a 117-character docstring line from a botched rewrap (passes flake8 at max-line-length=127).

Checked clean

The new linear_interpolate() is a faithful port of AP_Math.cpp's, polarity swap and endpoint behaviour included. plane_track.py:952 (NAV_VTOL_LAND flown in at the aircraft's altitude) is an exact port of QuadPlane::do_vtol_land's set_next_WP() followed by next_WP_loc.copy_alt_from(current_loc) — including ArduPilot leaving the already-computed offset_altitude stale; neutering it fails two fixtures. The QPOS window needs no finding: APPROACH < AIRBRAKE < POSITION1 < POSITION2, so the ramp applies at every entry distance and the model's unconditional application is right. self.qrtl_start needs no reset because fly_rtl() runs once and returns. Suite at this head: 429 passed, 13 skipped, 20 subtests on Python 3.14.4 / pymavlink 2.4.49, identical across three runs; 11 of 12 mutations tried are caught with a named failing test. With wx blocked the suite gives 89 skipped, 6 failures and 2 collection errors — my previous "88 skip" figure was incomplete, though the test_map3d_mission_labels fix itself does work.

Reviewed by: Claude + Codex (cross-checked) — a full read of the delta, the thread and the surrounding source, 12 mutations against the full suite, real-socket timing measurements and a line-by-line comparison against ArduPilot master and Plane-4.7.1, plus an independent codex exec pass given only this PR's number and the findings. Codex: 8 confirmed, 5 adjusted, 0 refuted, 0 new. The adjustments that change what you're being told are the terrain residual, the fixture-data correction in N2, and the wx-blocked counts.

(My previous AI comment could not be marked deprecated in place — it was posted under a different account and the edit is refused. It is superseded by this one.)

@AP-Review

AP-Review commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-18)

Automated review note — AI-generated (Claude), cross-checked by an independent Codex pass against the live diff. Please sanity-check before acting.
Verdict: ACCEPT

Re-reviewed at head 8a70115eb8 (previously b669fe9444); my earlier AI comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_18_1407/devcall_pr_reviews.html#prMAVProxy-1753

All three open notes from my last comment are closed, and the two that needed more than a docstring were verified by execution. Nothing new from either pass. Verdict: APPROVE.

Resolved since last round

  • N1 — the QRTL ramp is now labelled as master's (plane_track.py:540). The docstring gained exactly what was asked for. I re-checked the claim rather than the wording: git tag --contains returns no Plane-4.* tag for either bd9c0285fc (2026-08-20) or 3dc0c3621b (2026-08-22), so both really are master-only, and the description of 4.7's behaviour matches. That ~100 m divergence is now documented where the next reader will hit it.

  • N3 — default_loiter_radius() is shared, and the hole it left is closed (plane_track.py:1209). Used by both the model (:715) and the test helper (test_plane_track.py:1185), so ArduPlane's |WP_LOITER_RAD| <= 1 -> LOITER_RADIUS_DEFAULT rule now lives in one place. Mutation-verified: stripping the fallback fails test_a_loiter_to_altitude_with_no_radius_is_reached_at_the_default at :1086 — the new for none in (0, 1, -1) assertions — and nothing else, so the coverage is named and targeted (428 passed, 1 failed under the mutation).
    One thing I went looking for and did not find: the refactor inverts the comparison from abs(x) > 1 to abs(x) <= 1, and those differ for NaN — old returns 60.0, new returns NaN. It is unreachable, because parameter() at :129 filters NaN and falls back to the default, so self.loiter_radius cannot be NaN; I checked both forms over the reachable domain (0, ±1, ±0.5, ±60, 255, ±1e9 and either side of the boundary) with no mismatch. Mentioning it only because a later change could make it reachable.

  • N2 — the citations resolve and the autotest can now actually regenerate the recording. This is the one where re-citing a live SHA alone would not have closed it, so I checked both halves. All ten fixtures were re-cited (8d7ed31dfe→48c91a4c9e, f2c18f532f→bcb5e0c597, cd2319308b→3cf2e903d7), all three resolve in both ArduPilot/ardupilot and peterbarker/ardupilot, and all three are commits of PR 34424. Only the source strings changed — the flight data is byte-identical, which is what "as rebased" should mean. And the substantive half: 34424's head 48c91a4c9e is "VTOLMissionItemTypes: return as QRTL from above RTL_ALTITUDE" and does set RTL_ALTITUDE: 60 for Q_RTL_MODE 3 (quadplane.py:4137), so the published autotest now reproduces the 60 m mode-3 recording. That was the actual gap, not the citation.
    Residual, for the record and not a request: 34424 is still a draft, so these SHAs are the head of a branch that may be rebased again and the citations would go stale exactly as 8d7ed31dfe did. The source strings also name the PR and the test, which is the part that survives a rebase — about as good as a citation to unmerged work gets.

  • N5's overlong docstring line is fixed (test_plane_track.py:1165).

Closed, no change needed

N4 (heights() measures to the 3D-nearest point — right for a spiral, recorded as an accepted limitation) and N5 proper (linear_interpolate()'s polarity swap stays unreachable from both call sites, kept deliberately as a faithful port of AP_Math.cpp's).

Still open, all pre-existing and none introduced here

CI runs no tests at all — no workflow invokes pytest, so the 429 tests in this PR are never executed by CI. That remains the single highest-value thing that could change about this repository, and it is not this PR's job. Also plane_track.py:406 (unresolved terrain still leaves the path silently at the aircraft's altitude with the style still flown) and test_a_loiter_is_left_lined_up_on_what_comes_next (still asserts convergence rather than exit heading).

What I checked rather than assumed

The delta is exactly two commits, appended rather than force-pushed (ahead_by 2, behind_by 0), so the previous review's conclusions about everything else carry over intact. Suite at this head: 429 passed, 13 skipped, 20 subtests on Python 3.14.4 / pymavlink 2.4.49 — identical to the previous head, as a docstring-and-refactor round should be; the independent pass got the same three numbers separately. Fixture source fields were compared by parsing the JSON at both heads rather than by reading the diff, since these are single-line files where a diff shows nothing useful.

Reviewed by: Claude + Codex (cross-checked).

@AP-Review

AP-Review commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-28)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.
Verdict: REQUEST CHANGES

Re-reviewed at head 0c947c8f1f (you were last told 8a70115eb8); my earlier comment above is superseded. Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/followups/2026_09_28_1802/devcall_pr_reviews.html#prMAVProxy-1753

Everything from my previous comment still holds, and master fixed the biggest caveat: CI now runs pytest. That also turns this PR red, which is the one thing that has to change before merge (a one-line fix, item 1), so the verdict moves from ACCEPT to REQUEST CHANGES. Items 2 and 3 are real fidelity problems in the new code and worth fixing too. This round adds 4 commits: mission-end RTL, takeoff pitch/climb, reading tlogs in MAVExplorer, and a takeoff drawn towards the next waypoint. The rebase conflicts in 54b35bd6 and ae481512 resolve cleanly; your own + lines are unchanged.

Previous Status
QRTL ramp documented as master's, shared default_loiter_radius(), fixture citations (34424 is still a draft at 48c91a4c9e), docstring line Still resolved, untouched
heights() 3D-nearest, linear_interpolate() polarity Closed, unchanged
CI runs no tests Resolved by master (be773119), see 1 below
Unresolved terrain draws at the aircraft's altitude (plane_track.py:425); loiter-exit test asserts convergence (test_plane_track.py:416) Still open, pre-existing, not a request

New

  1. CI is red: tests/test_plane_track.py:2394. test_mavexplorer_waits_only_so_long_for_rally_terrain imports mavproxy_map3d.terrain, which imports vtk at module scope. CI doesn't install vtk, so both tests and tests-pymavlink-master fail with ModuleNotFoundError: No module named 'vtk' (1 failed, 507 passed). I reproduced it locally with vtk blocked. The fix is pytest.importorskip("vtk"), as the other 3D tests do.

  2. MAVExplorer now throws away the takeoff course it measured: plane_track.py:917. Drawing towards the next waypoint makes sense for the live map, where heading is only the way the aircraft points before launch. But MAVExplorer's heading comes from takeoff_course() (MAVExplorer.py:892). That is the ground course over the first 30 m flown, a good estimate of the course Plane::verify_takeoff holds (firmware locks hold_course_cd from GPS course corrected for accumulated yaw error, so it is an estimate, not the exact value). Measured:

    • On your three fixed-wing SITL fixtures, the worst launch point is 20 m off the drawn path now, against 3–4 m with the logged course honoured. That gap is why the test needed the 45° LAUNCH_SPREAD.
    • In a synthetic log with the runway east, WP1 3 km north and a 4° default pitch, the 50 m climb-out is now drawn ending 714 m north. The previous head drew it 227 m east, and the log reached 50 m at 290 m east.

    The docstrings at MAVExplorer.py:950 ("holds the course the log shows it flown on") and on takeoff_course() now describe behaviour that no longer happens. Could the measured course still be honoured? For example, keep towards-next-waypoint as the default, but have MAVExplorer pass its course separately, or stop map3d passing the pre-launch yaw for fixed-wing.

  3. An interrupted tlog transfer replaces a complete mission: MAVExplorer.py:813. Every MISSION_COUNT wipes items at once:

    • A re-download that stops after items 0–1, following a complete 3-item upload, leaves {0,1}. That is contiguous, so it gets flown as takeoff then the new mission-end RTL: the path reaches about 340 m north instead of 2994 m.
    • MISSION_CLEAR_ALL isn't read, so a cleared mission is still drawn.
    • The same branch (:817-818) also resets started and flown_from, so even a complete re-download of the unchanged mission after launch erases the launch history: started goes from {1: 1} to {} and the drawn launch moves from 600 m east of home back to home. GCSs re-download missions mid-flight routinely, so this one is likely to be hit on real tlogs. Keeping the complete mission items is not enough; the flight-start state has to survive a download that confirms the same mission.

    LogMission in mavflightview.py:414 (in this PR) already handles the first two, and float MISSION_ITEM as well. Reusing it would close this.

  4. Minor:

    • ARSPD_USE=2 counts as a sensor in use (plane_track.py:355). AP_Airspeed::use() returns false for it whenever throttle is non-zero, so during a powered takeoff the aircraft climbs at its pitch. == 1 would match.
    • These mutations survive the tests:
      • dropping the MISSION_COUNT reset (the test's old mission is shorter than the new one, so seq overwrite hides it)
      • rally_flags_for_frame() returning 0 (frame 3 and "no flags" are both above-home)
      • HOME_POSITION mapped to the origin
      • the DO_CHANGE_SPEED applied before the mission-end RTL

Checked and correct

  • The takeoff and climb modelling matches ArduPlane:
    • 4° default pitch (do_takeoff).
    • Pitch-only climb without an airspeed sensor, and "no lower than pitch" with one (takeoff_calc_pitch).
    • RTL climbs at once and slopes only down; AUTO does the same only with FLIGHT_OPTIONS bit 14 (setup_alt_slope).
    • Mission end changes to RTL (exit_mission_callback), and a mission RTL item also switches mode.
  • Each of those is pinned by a new test: reverting it fails a named test.
  • The tlog vehicle/sysid filtering and the ORGN/rally-flag mappings match ArduPilot (AP_Rally.h, MissionItemProtocol_Rally.cpp).
  • Every MAVLink constant used, including the module-scope RALLY_FRAME_FLAGS, exists in released pymavlink 2.4.49 under both MAVLink 1 and 2.
  • Full suite locally: 554 passed, 5 skipped. Each new commit passes on its own, and the commit split is fine.

@AP-Review

AP-Review commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-28)

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

Re-reviewed at head 2c78da32de (you were last told 0c947c8f1f); my earlier comment above is superseded. Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/followups/2026_09_28_2206/devcall_pr_reviews.html#prMAVProxy-1753

4 of the 5 items from my previous comment are resolved. The 5th, the tlog mission handling, is mostly resolved: one part still doesn't work on real logs (below). The CI blocker is fixed and tests / tests-pymavlink-master are green. The verdict moves from REQUEST CHANGES to COMMENT. Each fix is pinned: I reverted each one and a named test failed.

Previous Status
1. CI red on the vtk import Resolved. importorskip at tests/test_plane_track.py:2617. It passes with pymavlink 2.4.49 and vtk blocked, as in CI
2. MAVExplorer threw away the measured takeoff course Resolved. takeoff_course is a separate argument (plane_track.py:923), MAVExplorer passes it (MAVExplorer.py:1014), and the live map3d still draws towards the next WP. The fixed-wing fixtures now hold the launch to 10 m
3. tlog missions: interrupted download, MISSION_CLEAR_ALL, float MISSION_ITEM Resolved by moving LogMission to log_mission.py
3. …launch history lost on a mid-flight re-download Still open for real logs, see below
4. ARSPD_USE=2 Resolved (plane_track.py:358)
4. Surviving mutations (MISSION_COUNT, rally flags, HOME_POSITION, speed change before RTL) Resolved. Each now fails a named test
Unresolved terrain at the aircraft's altitude; loiter-exit convergence test Pre-existing, untouched, not a request

The move itself is clean. log_mission.py is the old mavflightview code except for the optional type argument, the whole()/fill() split, and CMD now keeping its logged Frame. mavflightview is wired to it correctly. The 11 fixture JSON changes only add the firmware name to source and takeoff_course (equal to the existing heading). items and flown are unchanged.

Still open

A re-downloaded mission only counts as "the same" if it is bit-identical: MAVExplorer.py:849-854. The reset compares the whole mission, including seq 0 and raw float lat/lon. With ArduPilot, a download almost never matches an earlier upload at seq 0:

  • An uploaded seq 0 is never stored (AP_Mission::replace_cmd returns early for index 0).
  • A downloaded seq 0 is always the live AHRS home.
  • Plane re-sets home at arming (AP_Arming_Plane.cpp:290) and keeps updating it while disarmed.

test_mavexplorer_keeps_where_a_mission_downloaded_again_was_flown (tests/test_plane_track.py:2489) passes only because its download reuses the uploaded seq 0 exactly. With your telemetry_mission() and only the downloaded seq 0 changed:

  • Home altitude +0.4 m: started goes from {1: 1} to {}, and the launch is drawn at home instead of 600 m east.
  • Home latitude +1e-7°: same result.
  • A float MISSION_ITEM upload followed by a MISSION_ITEM_INT download of the same mission also resets.

Leaving seq 0 out of the comparison, and comparing positions with a small tolerance, fixes all of these. I tried a quick version and all 168 tests still pass.

Minor

  • log_mission.py:76 reads m.Prm1..Prm4 directly. The told head's MAVExplorer used getattr(..., 0.0), so CMD from logs before March 2014 (the COpt,Prm1 format) now raises. mavflightview on master already did this, so it is very low priority.

@AP-Review

AP-Review commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-28)

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

Re-reviewed at head 901e73b67a (you were last told 2c78da32de); my earlier comment above is superseded. Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/followups/2026_09_28_2319/devcall_pr_reviews.html#prMAVProxy-1753

All but one item from my previous comment is resolved, and each fix is pinned: reverting it makes a named test fail. The mid-flight re-download is fixed for the cases I listed, but it still resets on ordinary missions. My suggestion last round didn't go far enough there. You did what I asked. CI is green (tests, tests-pymavlink-master), and 173 tests pass locally.

Previous Status
CI red on the vtk import Resolved, still green
MAVExplorer threw away the measured takeoff course Resolved. 336ed4a passes it as heading for a VTOL takeoff and as takeoff_course for a fixed-wing one (MAVProxy/tools/MAVExplorer.py:1057-1064), pinned by tests/test_plane_track.py:2639
tlog missions: interrupted download, MISSION_CLEAR_ALL, float MISSION_ITEM Resolved. 587e22f gives the rally table the same handling. Shorter and partial re-downloads, count 0, a mission-only CLEAR_ALL, and out-of-order or duplicate items all behave. One mixed-protocol exception is below
Re-downloaded mission must be bit-identical Narrowed, still open. Details below
ARSPD_USE=2 Resolved
Surviving mutations Resolved
log_mission.py required Prm1..4 Resolved (MAVProxy/modules/lib/log_mission.py:87, pinned by tests/test_plane_track.py:2579)
Terrain at the aircraft's altitude; loiter-exit test Pre-existing, not a request

The 9 fixture JSON edits only drop the course the takeoff doesn't use. items and flown are unchanged, and drawn() gives an identical track from the old and new files.

Still open: same_mission() is still too strict (MAVProxy/tools/MAVExplorer.py:717)

Leaving out seq 0 fixed the home cases. But frame and params are still compared exactly, and ArduPilot doesn't send either back as uploaded:

  • AP_Mission::mission_cmd_to_mavlink_int zeroes the packet, so a command with no location (every DO_*) comes back with frame 0.
  • Location commands come back with only frame 0, 3 or 10, never the _INT variants.
  • On Plane, NAV_WAYPOINT param1 and param4 come back as 0.

Each of these resets started from {1: 1} to {}, so the launch is drawn at home again. I tested them with your telemetry_mission() pattern, with the vehicle's download following the upload:

  • a DO_CHANGE_SPEED uploaded with frame 3 (or 2) and downloaded with frame 0;
  • waypoints uploaded as GLOBAL_RELATIVE_ALT_INT and downloaded as GLOBAL_RELATIVE_ALT;
  • a Plane waypoint uploaded with param1=5 and downloaded with 0.

The position tolerance is also too tight for a float upload. This corrects my own claim from this round. AP_Mission.cpp:1653-1654 converts a float MISSION_ITEM with packet.x * 1.0e7f in float32, so the stored point can be up to 6.4e-6° off. At longitude 149.1652374 that is 2.2e-6°, i.e. item 1491652352 rather than ...374. Both errors are over SAME_POSITION (1e-6°). A MAVLink1 GCS downloading as float MISSION_ITEM after an INT upload is off by up to half a float32 ulp, 7.6e-6° at longitude 149.

Suggested fix:

  • Normalise the frame (5→0, 6→3, 11→10, and 2→0 for items with no position), and compare it only for items that have a position.
  • Widen SAME_POSITION to about 1e-5° (~1 m).
  • Compare params only where ArduPilot round-trips them, rather than dropping them outright. A re-upload that really changes a speed or a jump count is a different mission.

New (minor)

  • A legacy rally point survives an empty modern rally download, and this is a regression from 2c78da32de. Legacy RALLY_POINT messages update rally directly (MAVExplorer.py:907), but the modern path only replaces it when rally_log's own table changes (:886-890). So a legacy point followed by a vehicle MISSION_COUNT(count=0, MAV_MISSION_TYPE_RALLY) now still draws the legacy point. At 2c78da32de it gave []. Only logs that mix the two rally protocols are affected.
  • The tolerance isn't pinned. Setting SAME_POSITION or SAME_ALTITUDE (MAVExplorer.py:697-698) to 0 leaves all 173 tests passing, because tests/test_plane_track.py:2561 only moves seq 0, which is skipped. A float MISSION_ITEM upload followed by the INT download the vehicle would really send would pin it.

@AP-Review

AP-Review commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-29)

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

Reviewed at head 683f777313.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/1.html#prMAVProxy-1753

Re-reviewed at 683f777313, the head after the rebase plus e1b3dac and 683f777. Everything from my last comment is resolved and pinned by tests: the same_mission() cases, the float tolerance, and the legacy rally point after an empty download. Thanks.

A few smaller things, none blocking:

  • MAVProxy/tools/MAVExplorer.py:730 (diff): RETURNED_FRAME needs MAV_FRAME_MISSION: MAV_FRAME_GLOBAL. AP_Mission accepts a location command in frame 2 and returns it in frame 0 (AP_Mission.cpp mavlink_int_to_mission_cmd/mission_cmd_to_mavlink_int). A DO_LAND_START uploaded that way therefore makes a re-download look like a new mission and resets started. The one-line fix keeps all tests passing. I don't know which ground stations do this.
  • MAVProxy/modules/lib/plane_track.py:1205 (diff): uses_vtol_approach() only sees an explicit RTL item. It misses the RTL MissionFlight.run() appends at mission end, so with Q_RTL_MODE=2 the map3d view (mavproxy_map3d/__init__.py:536) never redraws a plain waypoint mission for wind. Tested: final course -177 deg drawn, 91 deg with the wind.
  • MAVProxy/modules/lib/log_mission.py:132 (diff): if a log's only transfer stops early (count 4, items 0-2), plane_mission_track() flies the prefix and adds an RTL the vehicle may never fly. It checks for gaps (MAVExplorer.py:1158) but not for a missing tail. Keeping the count, or skipping the flown track for an incomplete transfer, would match the "no guessing" rule there.
  • Optional: SAME_ALTITUDE = 100 still passes every test. An altitude-only +5 m move of a waypoint would pin it.

@AP-Review

AP-Review commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-09-30)

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

Reviewed at head abe76f786b.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/2.html#prMAVProxy-1753

Re-reviewed at abe76f786b, which adds fd1e9f1 and abe76f7 on top of 683f777313. All four items from my last comment are fixed and pinned by tests. Thanks.

A few smaller things, none blocking:

  • MAVProxy/modules/mavproxy_map3d/__init__.py:438 (diff): the live map flies a download that is still in progress. wploader.add() bumps last_change, so each received prefix goes to flown_track() and gets an RTL appended. With 3 of 4 items, it drew a return home for a mission that ends in an unlimited loiter. It clears when the download completes, but not if it stalls. Skipping the flown track while count() < expected_count would fix it.
  • MAVProxy/modules/lib/plane_track.py:1217 (diff): a NAV_LAND that a DO_JUMP skips still clears runs_out. So with Q_RTL_MODE=2, WP, DO_JUMP(4,1), NAV_LAND, WP is drawn without the wind approach, even though the mission runs out and RTLs. It is a contrived case.
  • MAVProxy/tools/MAVExplorer.py:1410 (diff): say_unflown() could add "or the log does not hold all of it". That is now the reason for a cut-short transfer.
  • Optional: removing not self.items from LogMission.partial() (diff) still passes every test. A test with a whole mission followed by a cut-short re-download would pin it.

@AP-Review

AP-Review commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-01)

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

Reviewed at head 9a3df5b140.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/3.html#prMAVProxy-1753

Re-reviewed at 9a3df5b140, which adds 9a3df5b on top of abe76f786b. All four items from my last comment are fixed and pinned by tests. Thanks.

Three smaller things, none blocking:

  • MAVProxy/modules/mavproxy_map3d/__init__.py:497 (diff): wp ftp, wp show and wp ftpload replace the mission without updating expected_count. After a longer MAVLink download, the new gate then hides the flown path of the shorter mission until something resets the count. Setting expected_count = count() in those paths would fix it, as load_waypoints() already does.
  • MAVProxy/modules/lib/plane_track.py:1213 (diff): a DO_JUMP with a NaN target now raises out of uses_vtol_approach() on the main thread. idle_task() swallows it, so the mission is not redrawn at all, and an old pending track can be drawn over the new markers. mission_track() returns None for the same items, so the same guard here would do. Malformed input only.
  • MAVProxy/modules/mavproxy_map3d/__init__.py:537 (diff): a parameter change does not redraw the flown path. Changing RTL_ALTITUDE from 100 to 300 left the return 200 m low until some other redraw. Watching for parameter changes, including the initial fetch, would cover it.

@AP-Review

AP-Review commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-01)

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

Reviewed at head f9c35de075.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/5.html#prMAVProxy-1753

Re-reviewed at f9c35de075, which adds f9c35de on top of 9a3df5b140. All three items from my last comment are fixed, and each is pinned by a test that fails when its fix is reverted. Thanks.

Two smaller things, neither blocking:

  • MAVProxy/modules/mavproxy_map3d/__init__.py:948 (diff): a HOME_POSITION that changes only the altitude does not redraw the flown path, because set_home_position() returns early when lat/lon are unchanged. Moving home from 584 m to 684 m left the path starting at 584 m. Calling send_mission() when home_amsl changes, as the ORGN branch does, would fix it.
  • MAVProxy/tools/MAVExplorer.py:1211 (diff): first_navigation_item() runs outside the guard mission_track() has. A log with a NaN DO_JUMP target before the first nav item, plus a logged mission start, makes map3d raise before opening the viewer, where it should fall back to geometry. Catching the same exceptions here would do. Malformed logs only.

@AP-Review

AP-Review commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-01)

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

Reviewed at head 8f0fdbbd74.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/6.html#prMAVProxy-1753

Re-reviewed at 8f0fdbbd74, which adds 8f0fdbb on top of f9c35de075. Both items from my last comment are fixed. The altitude-only HOME_POSITION change now redraws the path, and the NaN DO_JUMP case in first_navigation_item() now falls back to geometry. Each is pinned by a test that fails when its fix is reverted. Thanks.

One small thing, not blocking:

  • MAVProxy/tools/MAVExplorer.py:1031 (diff): in a tlog that has a complete legacy RALLY_POINT table followed by a modern rally download that stops part way, the legacy table is replaced by the partial one. Before any modern table has completed, LogMission.whole() returns only the items that arrived (log_mission.py:132). With two legacy points, then MISSION_COUNT(2, rally) and only item 0, one rally point is left and the predicted RTL goes somewhere else. Keeping the previous rally until the modern table completes or is cleared would fix it. This needs mixed-protocol logs and only affects the drawing.

Not exercised: SITL, real logs opened in the GUI, and pymavlink other than 2.4.49.

@peterbarker peterbarker changed the title Tests and fixes for plotting of arcs on maps Draw the path a plane flies its mission along, and fix the arc/loiter drawing Oct 1, 2026
@AP-Review

AP-Review commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-02)

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

Reviewed at head fe11a40e5b.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/7.html#prMAVProxy-1753

Re-reviewed at fe11a40e5b, which adds fe11a40 on top of 8f0fdbbd74. The mixed-protocol rally point from my last comment is fixed, though not the way I suggested. A partial modern download is now drawn but marked not whole, and the mission is left unflown, so no wrong RTL is predicted. That makes sense given both protocols write the same AP_Rally table, and the new test pins it. Thanks.

A few small things, none blocking:

  • MAVProxy/tools/MAVExplorer.py:1090 (diff): the 'New rally' branch resets rally but not rally_whole. A DataFlash log with a short RALY table (e.g. Tot=2, Seq=0), then a cleared table, ends with no rally points marked not whole, and the flown path is not drawn. Setting rally_whole = True there fixes it.
  • MAVProxy/tools/MAVExplorer.py:1040 (diff): rally_whole is only updated when the points or arrivals change. A rally MISSION_COUNT(2) with no items after it, and no earlier table, leaves it True. The mission is then flown with no rally points although the vehicle has two. The 0 < dropped from LogMission.partial() (log_mission.py:151) looks meant for this case, but the guard never sees it. No test pins that partial() change either. With 0 < restored, the focused test suite still passes (335 passed). A count-only test, with and without a partial table before it, would cover both.
  • MAVProxy/modules/mavproxy_map3d/__init__.py:533 (diff): the flown-track key uses keyed_alt(), which gives 100 for both a relative 100 m waypoint and an absolute 100 m one. Changing only the frame reuses the cached or pending path, drawn about 684 m up instead of about 110 m. Adding the fixed-altitude flag to the key entries fixes it.
  • MAVProxy/modules/mavproxy_map3d/__init__.py:583 (diff): rally_points() reads module('rally').rallyloader, which the default RallyModule doesn't have. The exception returns [], so the live flown path's RTL ignores known rally points. The rally markers had the same problem before this PR. rally_count()/rally_point(i) exist on both rally modules, though RallyModule.rally_point() drops the altitude frame, so that also needs handling.
  • MAVProxy/tools/MAVExplorer.py:952 (diff): the docstring and the comment at :959 still say only a transfer that arrives whole replaces the table, which is no longer true for rally points.

Not exercised: SITL, real logs in the GUI, the VTK view, and pymavlink other than 2.4.49.

@AP-Review

AP-Review commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-03)

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

Reviewed at head c0fa3f1490.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/8.html#prMAVProxy-1753

Re-reviewed at c0fa3f14. This is a rewrite of fe11a40e5b into 18 commits with an identical tree, so nothing has changed since my last comment and those points still stand. A closer look also found five new ones. None is blocking. All of them only affect the drawn prediction.

Still open from last time:

  • MAVExplorer.py:1090: 'New rally' clears rally but not rally_whole. A short RALY table followed by a cleared one leaves the flown path undrawn. Setting rally_whole = True there fixes it.
  • MAVExplorer.py:1040: a rally MISSION_COUNT with no items after it, and no earlier table, leaves the table marked whole. The mission is then flown home-only. partial() (log_mission.py:151) is True but never asked. No test covers a count-only download.
  • map3d init.py:533: the flown-track key has the altitude but not its frame. A waypoint changed from relative 100 m to absolute 100 m reuses the cached or pending path, 684 m instead of 111 m.
  • map3d init.py:583: rally_points() reads .rallyloader, which the default RallyModule does not have, so the live RTL ignores loaded rally points. send_rally() had this gap already; the flown path is the new user.
  • MAVExplorer.py:952: the docstring, and the comment at :958, still say only a whole transfer replaces the table, which is no longer true for rally points.

New:

  • plane_track.py:481: when advancing, next_nav_index() gives up the second time it meets a DO_JUMP. AP_Mission only does that during lookahead (AP_Mission.cpp:2328). When advancing it runs the repeats and moves on. So a finite loop with no nav command in it (a self-jump, or WP, DO_SET_SERVO, DO_JUMP x3, WP B) ends the drawn mission early: 2907 m short of B, against 24 m.
  • plane_track.py:462: any negative repeat count is treated as for ever, but AP_Mission only repeats for ever on -1, and never follows -2. A -2 jump draws the same truncated path as -1 (2001 m short of the next item, against 24 m).
  • plane_track.py:1251: a NAV_WAYPOINT at lat/lon 0 with absolute altitude 0 is aimed at 0 m AMSL. ArduPlane's set_next_WP() holds the current altitude for a zero altitude in any frame, so the drawn leg sags to 608 m where it should stay at 700 m. NAV_LAND is not affected.
  • MAVExplorer.py:1411: only cmd_set() pushes settings to open 3D views. Changing missionpath, showlabels or labelsize in MAVExplorer's Settings dialog never reaches the view, so choosing 'flown' there draws nothing until some later set command.
  • map3d init.py:431: a terrain-relative waypoint whose terrain is not cached yet is passed to the predictor with no altitude. It is then flown at the aircraft's current height: 584 m where the real height is 974 m. The path is redrawn once terrain arrives, and the wrong one stays if it never does. Rally points with unknown height are deliberately left unflown, so these could be treated the same way.

Not exercised: SITL or a real vehicle, real logs opened in MAVExplorer, the wx/VTK GUI (three display tests skipped), real terrain fetching, and pymavlink other than 2.4.49. CI is 'passing' at c0fa3f14 in the pinned metadata and was not re-queried.

@AP-Review

AP-Review commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-07)

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

Reviewed at head c20f211dda.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/9.html#prMAVProxy-1753

Re-reviewed at c20f211d. Since my last comment at c0fa3f14, the branch has been rebased onto current master and has four new fix commits. Those commits fix most of what I raised:

  • 'New rally' now resets the whole flag.
  • Count-only rally downloads are not flown.
  • The flown-track key includes the altitude frame.
  • Settings-dialog changes reach the 3D views.
  • Jump repeat counts below -1 are ignored.
  • Finite nav-free loops are flown past.
  • A positionless absolute-0 waypoint holds height.
  • Unfetched terrain stands at home+alt until it arrives.

Each fix has a passing test (343 passed headless). None of the points below is blocking. All of them affect only the drawn prediction.

  • map3d init.py:590: rally_points() now goes through RallyModule.rally_point(). That returns flags=0, so the frame is lost, and it multiplies a stored MISSION_ITEM altitude by 100. After a normal download, a rally point 100 m above a 584 m home is drawn, and used as the RTL target, at 10584 m AMSL. The tests replace rally_point() with ready-made points, so they miss it. Reading frame and z from the module's wploader items would fix both.
  • plane_track.py:486: a jump met a second time is now stepped past before loops_forever() is checked. A -1 self-jump, or a -1 loop holding only DO commands, is drawn going on to the next waypoint. AP_Mission's get_next_cmd() gives up after 64 jumps and the mission completes there. A self-jump repeated 64 or more times ends the same way. The x3 case is right.
  • MAVExplorer.py:1047: rally_whole is recomputed on every MISSION_COUNT or MISSION_CLEAR_ALL, including the mission's and the fence's. A legacy rally table only partly in the log is then flown as whole once the mission is listed again. Limiting this to rally or ALL types fixes it.
  • map3d init.py:511: the live path waits for a complete mission download but not for a complete rally download. A partial or stalled rally table gives an RTL target based on only some of the points.
  • plane_track.py:1261: zero altitude now means "hold height" for every positionless command. Loiters and the VTOL approach sanitize() first, which only fills in a relative zero, so a loiter at absolute 0 still goes to 0 m AMSL. Very minor.
  • MAVExplorer.py:961: the docstring is fixed, but this comment still says a table that never arrives whole leaves the old one in place. For rally points that is no longer so.

Not exercised: SITL or a real vehicle, real logs in the GUI, real terrain fetching, and pymavlink other than 2.4.49. CI is pending at c20f211d.

@AP-Review

AP-Review commented Oct 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-08)

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

Reviewed at head 2281c8d970.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/10.html#prMAVProxy-1753

Re-reviewed at 2281c8d9. Since my last comment at c20f211d, the branch has been rebased onto current master. That rebase only merges master's new get_mission_for_map() into commits 3, 15 and 20, and the code behind every point below is unchanged. The PR's tests still pass (343 passed headless). None of the points is blocking. All of them affect only the drawn prediction, and all six from last time are still open:

  • map3d init.py:593: rally_points() goes through RallyModule.rally_point(). That returns flags=0, so the frame is lost, and it multiplies a stored MISSION_ITEM altitude by 100. After a normal download, a rally point 100 m above a 584 m home is drawn, and used as the RTL target, at 10584 m AMSL. Reading frame and z from the module's wploader items would fix both.
  • plane_track.py:486: a jump met a second time is stepped past before loops_forever() is checked. A self-jump repeated 64 or more times, or -1, is drawn going on to the next waypoint. AP_Mission gives up after 64 jumps, completes the mission and the plane goes to RTL, so the drawing should end there too, rather than just being marked as looping. A -1 loop of only DO commands doesn't reach the next waypoint either, though it isn't the 64-jump cap that stops it.
  • MAVExplorer.py:1047: rally_whole is recomputed on every MISSION_COUNT or MISSION_CLEAR_ALL, including the mission's and the fence's. A legacy rally table only partly in the log is then flown as whole once the mission is listed again. Limiting this to rally or ALL types fixes it.
  • map3d init.py:514: the live path waits for a complete mission download but not for a complete rally download. A partial or stalled rally table gives an RTL target based on only some of the points.
  • plane_track.py:1261: zero altitude means "hold height" for every positionless command. Loiters and the VTOL approach sanitize() first, which only fills in a relative zero, so a loiter at absolute 0 still goes to 0 m AMSL. Very minor.
  • MAVExplorer.py:963: this comment still says a table that never arrives whole leaves the old one in place. For rally points that is no longer so.

Not exercised: SITL or a real vehicle, real logs in the GUI, real terrain fetching, and pymavlink other than 2.4.49. CI is pending at 2281c8d9.

peterbarker and others added 22 commits October 9, 2026 11:27
@peterbarker
mp_util: name the commands pymavlink 2.4.49 does not have
503aba1 
pymavlink 2.4.49, the latest release, has no MAV_CMD_NAV_ARC_WAYPOINT
or MAV_CMD_DO_ORBIT in any of its dialects, while setup.py asks only
for pymavlink>=2.4.38.  The 2D map module, its mission arcs, the 3D
map and mission_circle_radius() all look them up in the dialect, so
with a released pymavlink they raise AttributeError: drawing a
mission with a loiter in it fails, and 21 of the tests do too.

Name the two commands in mp_util, and use those.  The arc waypoint
was given a fallback of its own in each of the three modules which
draw it; there is one name for it now, and DO_ORBIT, which is still
looked up in the dialect, has one too.
@peterbarker
map3d: take longitudes the short way round the antimeridian
cac8ac8 
Everything the 3D map draws -- terrain, the flight path, the mission,
fences, rally points and KML -- goes through enu() into a local frame
about the view's origin, which subtracted longitudes as they are.
Either side of the antimeridian was then drawn the world apart: with
the origin at 179.998, a point at -179.999 a few hundred metres east
landed nearly 40,000km west.

Wrap the difference, as ArduPilot's diff_longitude() does.  A point
given past 180 instead of wrapped, as ring_points() gives them, still
lands where it did.

The rest of the map made the same mistake.  The camera's focal point
went past 180 as it moved east over the antimeridian, picking terrain
tiles from the western edge of the world rather than the ones beside
it, and tiles were fetched by how far they were from the camera in
unwrapped longitude, so those just across were fetched last.  Terrain
heights under a point past 180 were looked for in no tile at all,
terrain samples fetched the wrong tile, looking at a point across the
antimeridian turned the camera the world away, and a fence edge across
it was sampled for terrain the long way round.  MAVExplorer centred
its view on the average longitude of the flight path, which for a
flight across the antimeridian is on the far side of the world.
@peterbarker
map3d: resolve altitude frames before measuring a loiter climb
0200ea4 
"map: plot the loiter path a vehicle actually flies" works out how many
turns a NAV_LOITER_TO_ALT is drawn with from how much climb is left when
the vehicle arrives.  The live module measured that by subtracting the
raw altitude of the item before from the loiter's own, whatever frames
the two were in, so an AMSL item before a home-relative loiter read as a
dive of the whole height of the field, and the spiral was drawn with the
turns of that dive.

Resolve both to AMSL before subtracting.  An altitude that cannot be
resolved yet is unknown rather than guessed: a relative item before home
is known, or a terrain-frame item before the terrain under it has been
fetched, has nothing to measure against, and its loiter is drawn as a
single turn.  MAVExplorer already resolved its items before measuring,
so it was not affected.

The test goes through send_mission() itself rather than only the helper,
and fails against the raw subtraction.
@peterbarker
mp_util: only read param4 as a crosstrack choice where it is one
97aaec6 
"map3d: join and leave loiter circles on a tangent" draws the vehicle
pulling back onto a track out of a loiter's centre whenever a circling
item's param4 was not positive.  That is what param4 means for three
of the circling commands and nothing like it for the other two:
LOITER_UNLIM's param4 is a yaw angle, and it is never left anyway,
and DO_ORBIT counts its turns there.

ArduPilot agrees: AP_Mission turns param4 into loiter_xtrack for
LOITER_TURNS, LOITER_TIME and LOITER_TO_ALT and for no other command,
and ArduPlane only restarts the next leg from the exit point when
that flag is set.  mission_crosstracks_from_centre() says the same,
and both the live map and MAVExplorer ask it rather than testing
param4 themselves; each has a test which fails if it goes back to
reading param4 directly.
@peterbarker
map3d: draw the turns LOITER_TURNS and DO_ORBIT ask for
2608e97 
"map: plot loiter and orbit commands using their radius" added
mission_circle_turns() to read the turn count a LOITER_TURNS or a finite
DO_ORBIT carries, but nothing called it: both producers only ever filled
circle_turns in for NAV_LOITER_TO_ALT, which works its own count out
from the climb.  A three-turn loiter climbing while it circled was drawn
as a single turn.

Hand the count through from the live module and from MAVExplorer, with
LOITER_TO_ALT still working its own out.  A circling item at a level
altitude is still drawn as one turn whatever it asks for, since the rest
would be drawn exactly on top of the first.

The drawing tests build their MissionItems by hand, which is how this
went unnoticed, so the new tests go through the producers instead: the
live module's send_mission() and MAVExplorer's resolve_mission_amsl().
@peterbarker
mp_util: take loiter radii the way ArduPlane's update_loiter() does
e6a78bc 
The MAVLink spec gives NAV_LOITER_TIME a radius in param3, but
ArduPilot has nowhere to keep one: the loiter time takes all
sixteen bits of the only spare field a location-carrying command
has, so AP_Mission keeps param3 as the loiter_ccw bit alone and
hands it back as -1 when the item asked for counter-clockwise and +1
otherwise.  ArduPlane's verify_loiter_time() calls update_loiter(0)
whatever the item was uploaded with, which flies WP_LOITER_RAD:
counter-clockwise if the item asked for it, and otherwise whichever
way WP_LOITER_RAD's own sign says.

mission_circle_radius() read the +-1 as a radius, so a LOITER_TIME
from a live vehicle or a log was drawn as a circle a metre across --
nothing anyone can see -- and one from a mission file, or read from
an upload in a tlog, at a radius the vehicle never flies.  Take the
size from the vehicle's own radius for every LOITER_TIME, and follow
its direction too: a negative param3 turns the circle
counter-clockwise, and anything else leaves it going the vehicle's
way.  With no vehicle radius to take, nothing is drawn.

update_loiter() treats a radius of a metre or less on the other
loiter items the same way, and flies LOITER_RADIUS_DEFAULT, 60m, when
WP_LOITER_RAD is itself a metre or less.  Do both of those as well,
rather than drawing a circle a metre across or none at all.

Found flying a plane mission in SITL: the vehicle handed back p3=1.0
for a LOITER_TIME uploaded with none.
@peterbarker
MAVExplorer: draw only the last mission a log holds
f5e8dd3 
cmd_map3d appended every CMD message it found to the mission it drew.  A
log carries the whole mission once at its start and again every time
one is uploaded, so a flight which had its mission changed drew every
mission it had ever held, joined into one line by the legs between the
end of one and the start of the next.

Keep the items by sequence number instead, and start again at the
logger's "New mission", which it writes before every mission it logs --
including a cleared one, which has no items after it at all.  A shorter
mission replacing a longer one therefore leaves nothing of the longer
behind, and a cleared mission draws nothing.  Logs from before the
logger wrote the message still start again at a mission's first item.

The reading moves out of cmd_map3d into mission_from_log(), so the tests
can hand it a log.

Found in a SITL plane log where the vehicle booted holding one mission
and was then sent another: MAVExplorer drew 31 items, both missions end
to end, for a flight of 16.
@peterbarker
MAVExplorer: draw a takeoff which has no position of its own
4675a6b 
A takeoff item normally carries an altitude and no position: the vehicle
climbs from wherever it is when the item runs.  The live 3D map draws
one as the climb from home, but cmd_map3d dropped every CMD without a
position, so from a log a mission started at its first waypoint and the
line ran diagonally up to it instead of showing the climb.

A log can say where the climb really began.  The autopilot announces
"Mission: <seq> <command>" as it starts each item, so the takeoff is
drawn from where the vehicle was at that moment.  A mission never flown
far enough to get there falls back on home, the mission's own first
item, which ArduPilot sets where it arms; and a mission uploaded before
the vehicle had a home leaves that item empty too -- as happens in SITL
-- so where the flight itself began stands in for it then.  Anything
else without a position still has nowhere to be drawn and is left out.

TAKEOFF_COMMANDS moves to mp_util, so the module and MAVExplorer read it
from the one place.

Found flying a copter mission with a positionless takeoff in SITL.
@peterbarker
map: draw loiter circles and direction arrows by default
c7454c2 
The 2D map's loitercircle and showdirection settings were both off,
so a mission with loiters in it was drawn as a line through their
centres with nothing to say the vehicle circles there, and nothing to
say which way round the mission runs.  mavflightview and MAVExplorer
draw the circles whatever the settings say, so the same mission looked
different depending on which one it was opened in.

Now that the circles are the ones the vehicle flies, turn both on,
and turn on the 3D map's direction arrows in the map3d module and
MAVExplorer with them, so the live maps and MAVExplorer's 3D map
show a mission's circles and its direction alike.  mavflightview has
no direction setting and still draws its mission without arrows.
"map set loitercircle 0", "map set showdirection 0" and their map3d
and MAVExplorer equivalents put them back.
@peterbarker @claude
tests: add missions which exercise the loiter drawing
a5544b6 
plane-mission-geometry.txt and copter-mission-geometry.txt are
SITL-flyable missions which exercise the drawing this fixes: climbing and
descending spirals, crossing and non-crossing tangents between loiters,
both param4 settings, mixed altitude frames, the arc waypoints only a
multirotor flies, and the loiters it holds position for.  README.md
describes each one and what to look for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
mavflightview: read the mission out of telemetry logs again
7825b04 
A telemetry log carries the missions downloaded from the vehicle or
uploaded to it, each a MISSION_COUNT and then its items, in whatever
order they were asked for and perhaps never all of them; a dataflash log
carries a CMD for each item whenever the vehicle writes the whole
mission out.  LogMission, in the lib so that any of the tools can read a
log's mission with it, reads either and keeps only the last mission to
arrive whole: the map draws the mission the vehicle was flying rather
than every one the log holds joined end to end, a transfer the log ends
part way through leaves the one before it as it was, and a mission
cleared away is cleared.  MISSION_CLEAR_ALL, the float items a ground
station may upload instead of MISSION_ITEM_INT, and the frame a
dataflash CMD logs are taken with it.

WP_LOITER_RAD, which the loiter circles are drawn at, is looked up once
the log has been read, since a telemetry log carries the parameters as
it goes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker
mp_util: take the parameters MAVProxy fetched over FTP from a log
8c9933a 
When MAVProxy fetches a vehicle's parameters over FTP it logs them to
the tlog itself, as PARAM_VALUEs from MAV_COMP_ID_MISSIONPLANNER so
they can be told apart from ones the autopilot sent.  A log's params
only offer the autopilot's own, so a log from a vehicle whose
parameters were fetched that way has just the few it sent as they
changed -- 35 of 1479 in the log this was found in, and not
WP_LOITER_RAD, so mavflightview still drew no loiter circles.

Add log_params(), which takes both, the autopilot's own values over
the fetched ones, and use it where mavflightview and MAVExplorer's 3D
map read parameters to draw a mission.

A log picks the vehicle whose parameters it offers from its first
heartbeat, and one with no heartbeat in it -- cut out of a longer
log, say -- offers none at all.  Take the parameters of the one
vehicle they came from there, and none where several vehicles sent
some, rather than guess between them.
@peterbarker
MAVExplorer: take home from the log where the mission has none
c5e2740 
A mission uploaded before the vehicle has a home logs its home item
as nothing at all, and resolve_mission_amsl() then measures every
home-relative altitude from the lowest position in the log instead.
The first positions are logged before the EKF has an origin, at
nothing too, so a QuadPlane log from SITL drew its relative-altitude
items 165m low.

The vehicle logs its home in ORGN once it has one.  Where the
mission's home item is empty, fill it from the last one logged.
@peterbarker @claude
plane_track: fly a mission through ArduPlane's navigation
79c1770 
Work out the path a plane or QuadPlane flies a mission along, rather
than the lines between its items: a port of enough of ArduPlane's
navigation to fly the mission with its L1 controller, at the aircraft's
own airspeeds, roll limits and climb rates.

It flies every loiter, with the radius and direction the item and
WP_LOITER_RAD give it and the exit ArduPlane takes -- lined up on the
next item as the mission stores it, or crosstracking from where the
circle is left; waypoints with their acceptance and pass-by radii, and
WP_MAX_RADIUS; a fixed-wing takeoff at the item's pitch, or faster where
TECS has an airspeed sensor to fly by, and a VTOL one straight up and
away the way the aircraft points; the return to launch a RETURN_TO_LAUNCH
item or a mission running out makes, to home or the rally point nearest
it, with the circle a plane ends in and each Q_RTL_MODE a QuadPlane
lands by; a VTOL landing straight in or on the fixed-wing spiral
approach param1 or Q_OPTIONS asks for, into the wind; and jumps, jump
tags, speed changes and altitude frames along the way.

Where the path cannot be worked out -- a command it does not know, an
item the aircraft never finishes, or a flight longer than it is worth
following -- it says so, and the caller draws the mission's geometry
instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
map3d: draw the path a plane flies its mission along
9781abc 
The 3D map draws the path the vehicle is going to fly its mission along,
worked out by plane_track, rather than the lines between its items.  It
is flown on a thread of its own, since flying a long mission takes a
moment, and drawn as soon as it is worked out; a mission which cannot be
flown through is drawn as its geometry, and the map says why once.

The mission is flown again when anything it is flown by changes: the
mission itself, home moving along or up and down, the wind a VTOL
landing approaches into, the way the vehicle points before a takeoff, or
any of the parameters.  A download still coming in is drawn but not
flown, since the items so far would be flown as a mission which runs
out.  A rally point a return would go to is drawn where the return goes
to it, at the altitude its own frame gives it.

The view has controls of its own for what to draw: the mission as the
path flown, as its geometry, or as plain lines between items, and
whether to label each item with its number and kind, at a size the
labelsize setting gives.  Those and the module's settings end up
agreeing whichever way a change crosses between them, and a view opened
later is told what they are.  The labels are the 2D map's, which both now
take from mp_util.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
MAVExplorer: draw the path a log's mission was flown along
3b7dc24 
MAVExplorer reads the mission a log holds with LogMission, so a
dataflash log and a telemetry log are read the same way, and draws the
path the vehicle flies it along in the 3D map, with the rally points a
return may go to and home as the log gives them.

What the log says of the flight is used where the mission does not say:
where the mission's first item began, which is where the flight starts,
and the course measured over the start of the takeoff, which is the way
a QuadPlane pointed as it transitioned and the course a fixed-wing
takeoff held -- nothing before a launch says either.  The course a VTOL
landing approach was flown on is the one the vehicle announced.

A mission downloaded again mid-flight is recognised as the one being
flown, compared as AP_Mission gives it back and leaving out home, which
is the vehicle's own: so where the flight had reached in it survives a
ground station asking the vehicle what it holds.  What the log holds only
part of -- a mission or a rally table a transfer ends part way through --
is drawn, since those items are where the vehicle was told to go, but
not flown, since what would have followed them is not known.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
wp: say how many items to expect when a mission is loaded
feef7c6 
wp ftp, wp ftpload and wp show fill the loader without saying how many
items to expect, so what they load looks like part of a download which
stopped: the wp module asks the vehicle for items it already has, and
anything else watching the loader waits for the rest of a mission which
is all there.  Say the count, as loading from a file already does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
tests: check the drawing against SITL flights of every mission item
a868206 
Ten SITL flights of ArduPilot's autotest missions are recorded here, and
the path drawn for each mission is held to the one flown: across the
ground, and in height.  The flights are of the missions ArduPilot PR
34424 adds -- QuadPlane.KalaupapaCanyonRun, Plane.MissionItemTypes and
QuadPlane.VTOLMissionItemTypes -- which between them fly every kind of
item ArduPlane flies, every way a mission ends, every Q_RTL_MODE and
each way a QuadPlane lands.

Each flight is checked three ways: that every item is reached in the
order the mission takes them, with about as far flown between each pair
as the aircraft flew; that each point flown is near the path drawn,
within limits of its own; and that it is at the height drawn, measured
to the nearest point of the path so that a spiral is measured against
the turn of it the aircraft was on.  Half of each flight is drawn within
0.2 to 9m of where it went, and nine tenths within 4 to 28m.

A recording says which firmware flew it, since the drawing follows what
a version of ArduPlane does, and record_flight.py makes another from a
log: what MAVExplorer hands the drawing, and the path flown once a
second from the end of a QuadPlane's transition or a plane's takeoff to
the start of its landing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
plane_track: follow the jumps, and the altitudes, ArduPilot does
a1f99c3 
Two places the mission was read differently from AP_Mission:

- Meeting a DO_JUMP twice over ended the drawn mission.  AP_Mission gives
  up that way only while it is looking ahead (AP_Mission::get_next_cmd);
  advancing, it runs the jump's repeats out and carries on past it, so a
  loop with nothing to navigate to in it -- a jump to itself, or one over
  commands which are not flown to -- is flown past rather than ending the
  mission 3km short of the item after it.
- Any negative repeat count was taken as repeating for ever, where
  AP_MISSION_JUMP_REPEAT_FOREVER is -1 alone and a jump asking for fewer
  is never followed at all.

An item with no position of its own and no altitude either is flown at
the altitude the aircraft is at, which Plane::set_next_WP() gives it in
whatever frame the zero is in; an absolute zero was drawn as 0m AMSL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
map3d: take the rally points from the module which has them
80978e8 
The 3D map read them from a rallyloader on the rally module, which has no
such thing: it is a MissionItemProtocolModule, and answers for its points
itself.  So nothing was ever drawn for them, and a return to launch in
the live map went to home however near a rally point was.  The tests had
a double with the attribute the code reached for, which is why they did
not notice; they now stand in for what the module really offers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
map3d: draw a mission while its terrain is still coming, and by its f…
5b2ba03 
…rames

Two things about the path the live map draws:

- An item above terrain which has not been fetched yet was flown at
  whatever height the aircraft happened to be at, 390m out where the real
  ground was.  Stand it above home until the terrain arrives, which is
  near enough to be worth drawing, and the mission is drawn again as soon
  as the terrain is known.
- The path already drawn is kept against what the mission is, which folded
  an item's altitude frame away: 100m above a home at 584m and 100m AMSL
  were the same item, so changing one to the other drew the path worked
  out for the other.  Keep the frame with the altitude.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker @claude
MAVExplorer: know the whole of a rally table, and keep the views current
6b784a1 
A rally table is only a table to work a return out from when the whole of
it is known, and two ways of knowing it were missed: the logger writing
out a table with nothing in it -- 'New rally' and no points -- which is
the whole of it, and a download which says how many points to expect and
ends before any of them arrive, which is none of it.  The first left a
mission unflown for want of points which had been cleared away, and the
second flew it home as though the vehicle held none.

The settings an open 3D view is drawn by are also taken each time round
the loop rather than only when a set command gives them, so changing the
mission path, the labels or their size in the settings dialog reaches the
view: choosing 'flown' there drew nothing until some later set command.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AP-Review

AP-Review commented Oct 9, 2026

Copy link
Copy Markdown

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

Reviewed at head 6b784a1a4c.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1753/11.html#prMAVProxy-1753

Re-reviewed at 6b784a1a. Since my last comment at 2281c8d9, the branch has been rebased onto master b6fd023b. The only change to the PR's commits is master's zero-argument super() in one context line, so the code behind every point below is unchanged. The PR's tests still pass (343 passed headless). None of the points is blocking. All of them affect only the drawing, never what is sent to the vehicle.

The six points from last time are all still open:

  • map3d init.py:593: rally_points() goes through RallyModule.rally_point(). That returns flags=0 and multiplies a stored MISSION_ITEM altitude by 100. After a normal download, a rally point 100 m above a 584 m home is drawn, and used as the RTL target, at 10584 m AMSL. Reading frame and z from the module's wploader items would fix both.
  • plane_track.py:486: a self-jump repeated 64 or more times, or -1, is drawn going on to the next waypoint. AP_Mission gives up after 64 jumps, completes the mission and the plane goes to RTL, so the drawing should end there.
  • MAVExplorer.py:1047: rally_whole is recomputed on the mission's and the fence's MISSION_COUNT too, so a legacy rally table only partly in the log is flown as whole once the mission is listed. Limit it to rally counts and rally/ALL clears.
  • map3d init.py:514: the live path waits for a complete mission download but not for a complete rally download.
  • plane_track.py:1261: a positionless loiter at absolute 0 is drawn holding height, but sanitize() only fills in a relative zero, so ArduPlane flies it to 0 m AMSL. Very minor.
  • MAVExplorer.py:963: this comment says a table that never arrives whole leaves the old one in place. For legacy rally points that is no longer so.

Two new minor points:

  • mavflightview.py:480: the tlog mission reader takes MISSION_COUNT, clears and items from any vehicle. In a multi-vehicle tlog, a second vehicle's empty count, or a clear sent to it, wipes the first vehicle's drawn mission. MAVExplorer filters these by vehicle.
  • plane_track.py:1341: least_flight_time() takes airspeed as ground speed, while the flight model flies at true airspeed. High up, a mission near the 4-hour cap is rejected and falls back to geometry. A 330 km leg at 3000 m is rejected at 15000 s, though the model flies it in 12922 s.

Not exercised: SITL or a real vehicle, real logs in the GUI, real terrain fetching, and pymavlink other than 2.4.49. CI is pending at 6b784a1a.

@peterbarker
peterbarker merged commit 4ed8940 into ArduPilot:master Oct 9, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

AIReview

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@peterbarker @tridge @AP-Review