Use MAV_CMD_DO_CHANGE_ALTITUDE and MAV_CMD_DO_REPOSITION for moving vehicle

[peterbarker]

MissionPlanner currently uses some archaic code which relies on magic "current"
values to move the vehicle in GUIDED mode using mission_item and
mission_item_int commands. Use MAV_CMD_DO_REPOSITION and
MAV_CMD_DO_CHANGE_ALTITUDE instead, so we can retire the ArduPilot code.

Both are sent as COMMAND_INT, so lat/lng go on the wire as exact degE7 rather
than through a float.

Falling back

The new commands are not as old as the fleet: Copter and Rover only handle
DO_REPOSITION from 4.1, and DO_CHANGE_ALTITUDE is Plane-only, from 4.6.3.
Everything else NAKs with UNSUPPORTED, so the previous mission-item path is
kept and used whenever the vehicle says it does not know the command, or does
not answer at all. An UNSUPPORTED answer is remembered per vehicle, so an
older vehicle pays one round trip per connection rather than one per command,
and the memory is cleared on reconnect.

A vehicle which refuses the command (a fence breach, say) is a different
thing: that is reported to the operator and the legacy path is not tried, so
Mission Planner no longer talks its way around a refusal the firmware
deliberately made. This is what needs the new doCommandResult/doCommandIntResult
variants: the existing bool helpers collapse every non-ACCEPTED result to false,
which cannot tell "does not support it" from "said no".

Those helpers hold giveComport while they wait for the ack, and they leaked it
if the wait threw - a truncated packet on a lossy link was enough to leave the
port held until some unrelated operation cleared it. It is now released in a
finally, and only where that call took it, which also fixes getHomePosition
releasing its own hold when it retried. That part touches every command
Mission Planner sends, so it is a separate commit.

The ack wait is short (1s, one resend) because these are sent from the UI
thread and from the follow/swarm loops.

The guided target shown on the map is now also recorded from COMMAND_INT on the
receive side, so tlog playback and a second GCS on the same link still draw it.

[peterbarker]

Tested basic do-repos and changealt using github build products.

[meee1]

this shouldnt be changed

[peterbarker]

Are you concerned with external callers to this method breaking?

I believe I changed all in-tree callers to pass in the required altitude rather than the Locationwp object.

The new method of changing the altitude - MAV_CMD_DO_CHANGE_ALTITUDE doesn't deal with waypoints at all - it's just an altitude. The old method did require the Locationwp as it's calling setWP (leaving lat/lng at and setting the magic value "3" to get the weird behaviour we are trying to eliminate in ArduPilot).

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-03)

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

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_03/devcall_pr_reviews.html#prMissionPlanner-3724

Reviewed at head a0db1d7229. COMMENT — moving off the legacy mission-item hack is right; one parameter is wrong.

DO_REPOSITION passes yaw 0, which commands a heading rather than leaving it alone

MAVLinkInterface.cs:4437 sends 0 as param4. From the spec in common.xml, MAV_CMD_DO_REPOSITION param 4: "Yaw heading… NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.). For planes indicates loiter direction (0: clockwise, 1: counter clockwise)".

So 0 commands yaw to north on a copter, and selects clockwise loiter on a plane — where the legacy path it replaces left yaw untouched. That's a silent behaviour change on every "fly to here". The fix is float.NaN. Found independently by both passes of this review.

(param3 is fine: the spec says a radius of zero or NaN is ignored.)

Renaming the public setNewWPAlt overloads breaks external callers

Already raised inline and it stands: both public overloads are replaced by setNewAlt with a different signature. Updating the one in-tree caller doesn't help plugins or scripts — source callers stop compiling and precompiled ones can throw MissingMethodException at runtime. Thin [Obsolete] wrappers forwarding gotohere.alt to setNewAlt cost almost nothing.

Minor

37 of the added lines are tab-indented in a space-indented file, which shows up in the diff as the new blocks sitting at a different depth from the catch clauses they belong to.

Checked and clear

The rest of the packing is correct: DO_REPOSITION speed -1 (use default), the CHANGE_MODE flag, lat/lon scaled to int degE7 for COMMAND_INT, and the frame passed through; DO_CHANGE_ALTITUDE sends metres with MAV_FRAME.GLOBAL_RELATIVE_ALT in param2 as the spec requires. Unsupported commands are NACKed and fall through to the legacy path as intended — the right shape for talking to older firmware.

CI green (6 passing).

[AP-Review]

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 6b558b12c4 (previously reviewed at a0db1d7229); my earlier AI comment above is superseded.

All three findings from my previous comment are resolved. One new regression turned up in their place, so: REQUEST CHANGES on that one item.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_16_2037/devcall_pr_reviews.html#prMissionPlanner-3724

Previous round

• DO_REPOSITION yaw param4 was 0 instead of NaN — RESOLVED. MAVLinkInterface.cs:4437 now reads float.NaN, // param4 - yaw (NaN: keep current yaw behaviour), which is exactly the documented "leave it alone" value in common.xml. Confirmed on the firmware side too, so this is not spec-lawyering: ArduPlane/GCS_MAVLink_Plane.cpp:567 treats NaN as the default clockwise and never calls set_radius_and_direction() because param3 is 0, and ArduCopter/GCS_MAVLink_Copter.cpp:431 never reads param4 at all, so yaw is genuinely untouched.

• Renaming the public setNewWPAlt overloads broke external callers — RESOLVED. Both original signatures are back as thin forwarders at MAVLinkInterface.cs:4489-4499, byte-for-byte compatible, each marked [Obsolete("Use setNewAlt")] — the right deprecation path, so source callers still compile and precompiled plugins will not throw MissingMethodException. Dropping everything but .alt from the forwarded Locationwp is correct, and the author's May reasoning holds: the legacy path sent the item with current == 3, which the firmware handles as handle_change_alt_request() — altitude only, lat/lng discarded.

• 37 added lines were tab-indented in a space-indented file — RESOLVED. 62 added lines against the merge-base, 0 tab-indented, 0 containing a tab anywhere.

New this round

• BUG ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4445 (found by the independent Codex pass, then verified against the source) — A successful reposition returns without updating MAVlist[sysid, compid].GuidedMode, which both legacy paths did — so "Fly to Here Alt" and the destination marker break after a "Fly to Here". Raised by the independent cold pass and then verified against the source at this head. The new doCommandInt(DO_REPOSITION) success path returns at :4445 with no assignment to GuidedMode; the paths it replaces do set it (the setWP handler at :5715/:5743 and the setPositionTargetGlobalInt handler at :5770). The consumers are real: GCSViews/FlightData.cs:2918-2922 ("Fly to Here Alt") rebuilds the target as lat = MainV2.comPort.MAV.GuidedMode.x / 1e7, lng = ... .y / 1e7 — it sets only .z and .frame itself — so on a fresh connection those read 0 and it commands a flight to lat 0, lng 0; and FlightData.cs:3072,3076 gate on GuidedMode.z == 0f. This is a regression introduced by this PR, in the exact feature it changes, and it is the one thing left standing now that the previous round's findings are fixed. Setting GuidedMode on the success path before returning would close it.

• ISSUE ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4444 — The legacy fallback fires on a deliberate firmware refusal, not just on "unsupported", and the fallback path is not fence-checked. doCommandInt collapses every non-ACCEPTED MAV_RESULT to false, so UNSUPPORTED (old firmware — the case you want) and FAILED/DENIED (the firmware deliberately said no) are indistinguishable. Concretely on Plane: a "Fly To Here" outside an enabled fence returns MAV_RESULT_FAILED from GCS_MAVLink_Plane.cpp:581 before the mode change; MP sees false, falls into the legacy block, and setWP(current = 2) reaches ModeGuided::handle_guided_request → set_guided_WP() with no fence check at all — so the plane flies to the point the firmware just refused. In fairness this is not a regression (before this PR MP only ever used the unchecked path, so the end result is the same), but the new code makes the firmware's explicit refusal ineffective, which cuts against the point of the PR. Gating the fallback on MAV_RESULT_UNSUPPORTED would fix it, which needs a doCommandInt variant returning MAV_RESULT rather than bool. Copter is unaffected — its fallback is fence-checked.

• ISSUE ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4433 — Repeated-update callers go from fire-and-forget to a blocking, comport-exclusive ACK round trip. For non-Plane vehicles the old tail was setPositionTargetGlobalInt — no ACK, no blocking, no giveComport. The new first attempt is doCommandInt(..., requireack: true), which sets giveComport = true and blocks for up to 3 retries × 2000 ms = 8 s before throwing. Affected repeat callers: Swarm/FollowPath.cs:50 loops over every non-leader MAV serially at 5 Hz, so N vehicles now cost N serial round trips inside a 200 ms budget; ExtGuidedPlugin.cs:61 is called from Loop() with no giveComport guard. Controls/FollowMe.cs:196 degrades gracefully (0.5 Hz and it does guard). Worth considering requireack: false for the swarm/follow callers.

2 further minor notes are in the report rather than here.

Checked and clear

• Every command parameter re-checked against the live common.xml: speed −1 (ignored by Copter and Plane; Rover guards with is_positive()), conditional CHANGE_MODE, radius 0, yaw NaN, integer degE7 coordinates, metre altitude.
• param5/param6 are declared int in doCommandInt, so (int)(gotohere.lat * 1e7) is carried as an exact int32 into command_int.x/.y — no float-mantissa quantisation.
• MAV_DO_REPOSITION_FLAGS.CHANGE_MODE == 1 matches the enum, and FollowMe's setguidedmode: false correctly sends 0, honouring the enum's own "should not be set for follow me applications".
• Old-firmware fallback works: unsupported commands are always ACKed MAV_RESULT_UNSUPPORTED, so doCommandInt returns false promptly and the legacy current == 3 path runs. DO_CHANGE_ALTITUDE is Plane-only and 4.7+.
• CI snapshot: Build Debug and Build Release pass; four mobile builds pending; no failures.

---

Independent cold pass: REQUEST CHANGES. It confirmed every parameter against the fetched common.xml and confirmed the two resolved findings — and it found the GuidedMode regression, which the primary read missed entirely. That finding was then verified against the source before being included here.

Reviewed by: Claude (full read of the diff, thread and surrounding source) + an independent Codex cold pass that was not shown these findings.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (head `15bb12a069`)

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

Re-reviewed at head 15bb12a069 (previously 6b558b12c4); my earlier comment above is superseded.

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

---

Verdict: COMMENT (was REQUEST CHANGES) — the BUG and the refusal-fallback ISSUE are both properly fixed, and fixed the right way: adding the MAV_RESULT-returning command variants rather than papering over it. CI is 6/6 green at this head.

Previous round

Finding	Status
DO_REPOSITION success path returns without updating MAVlist[...].GuidedMode	RESOLVED — MAVLinkInterface.cs:4478-4482. I checked the scaling, which is the part that could still have been wrong: gotohere.id is set to MAV_CMD.WAYPOINT at :4454 before the assignment, WAYPOINT=16 carries [hasLocation()], so the implicit Locationwp → mavlink_mission_item_int_t operator takes the isLocationCommand branch (locationwp.cs:157-158) and scales lat/lng by 1e7 — which is exactly what FlightData.cs:2921-2922 and :4200-4201 divide by
Legacy fallback fires on a deliberate firmware refusal, and is not fence-checked	RESOLVED — the doCommandResult/doCommandIntResult/*Async family (:2693-2705, :2866-2886); setGuidedModeWP no longer falls back on anything but UNSUPPORTED (:4485-4490). Re-confirmed Plane's fence rejection is MAV_RESULT_FAILED (GCS_MAVLink_Plane.cpp:579-583), which now short-circuits
Repeat callers now pay a blocking, comport-exclusive ACK round trip	STILL OPEN — re-raised, see below
DO_CHANGE_ALTITUDE as COMMAND_LONG while DO_REPOSITION uses COMMAND_INT	STILL OPEN, and it now has teeth — see N2
The recommended replacement is itself [Obsolete]	STILL OPEN (cosmetic) — :4535/:4541 point at setNewAlt, which carries a bare [Obsolete] at :4547

On the blocking-ACK point: no requireack: false anywhere, :4474 still passes true. Re-derived independently — retrys = 3, timeout = 2000 at :2971-2974, and the loop resends three times before throwing, so 4 × 2000 ms = 8 s with giveComport = true held throughout, and AwaitSync is GetAwaiter().GetResult() so it blocks the calling thread. Swarm/FollowPathControl.cs:69-83 runs on a Thread.Sleep(200) loop and Swarm/FollowPath.cs:50 iterates every non-leader MAV serially inside it, with no giveComport guard; ExtGuidedPlugin.cs:61 likewise. New this round: goHereToolStripMenuItem_Click runs on the UI thread, so on Copter a lost ACK now freezes Mission Planner for 8 s where the old setPositionTargetGlobalInt tail returned instantly.

Two corrections I owe you on my own previous comment. Controls/FollowMe.cs defaults to 0.5 Hz but the dropdown offers 2 Hz, so its budget is 500 ms, not 2 s — and it does guard on giveComport. And I said a build with AP_MAVLINK_COMMAND_LONG_ENABLED = 0 "would time out 8 s before falling back rather than NACK"; that is wrong — GCS_Common.cpp:5457-5474 NACKs with MAV_RESULT_COMMAND_INT_ONLY.

ISSUE — MAVLinkInterface.cs:4487 — a refused reposition tells the operator nothing

Operator has a fence enabled and clicks Fly To Here just outside it on a Plane. Plane returns MAV_RESULT_FAILED; MP takes the :4485 branch, logs DO_REPOSITION refused: FAILED, and returns. FlightData.cs:3094-3099 wraps the call in try/catch and shows Strings.CommandFailed — but nothing is thrown, so no dialog appears, the marker does not move, and the vehicle carries on. The only signal is that nothing happened, and the natural response is to click again.

The sibling function added in the same PR does the opposite: setNewAlt at :4580-4583 throws "Alt Change Failed: " + result, surfaced by FlightData.cs:4387-4390, and the file's own convention is to throw ("Guided Mode Failed" at :4517). Throwing the MAV_RESULT here would make the two new paths consistent and tell the operator why.

ISSUE — MAVLinkInterface.cs:4580 — COMMAND_INT_ONLY is not a refusal, but is treated as one

MAV_RESULT_COMMAND_INT_ONLY is a statement about the message form, not the command — and DO_CHANGE_ALTITUDE is the one command here still sent as COMMAND_LONG, so that result blocks the working fallback and throws. On a build with AP_MAVLINK_COMMAND_LONG_ENABLED = 0, result == 8 is neither ACCEPTED nor UNSUPPORTED nor null, so :4583 throws, the UI shows "Error Communicating", and the setWP(current = 3) fallback at :4598 — which that firmware handles fine — never runs. Before this PR the bool return meant it fell back and worked.

Scope, honestly: no in-tree ArduPilot board or vehicle sets that define to 0 today (GCS_config.h:130-131 defaults it to 1), so this is latent, not live. It is worth fixing because the fix removes the class for free: MAV_CMD_DO_CHANGE_ALTITUDE is already in Plane's COMMAND_INT switch (GCS_MAVLink_Plane.cpp:744-745), so switching :4560 to doCommandIntResult makes both new commands COMMAND_INT and makes COMMAND_INT_ONLY unreachable. COMMAND_LONG_ONLY and TEMPORARILY_REJECTED are the same shape.

Notes

• :4494 — the UnsupportedCommands cache is deliberately not populated on the timeout path, so an autopilot that never ACKs pays the full 8 s on every call for the life of the connection. Probably the right call — one dropped ACK must not permanently disable the feature — but it compounds the blocking-ACK item above for the 5 Hz swarm loop. The cache lifetime itself is fine (MAVlist.Clear() on Open()).
• :2966 — pre-existing: doCommandIntResultAsync has no IN_PROGRESS branch where doCommandResultAsync does. Not reachable for DO_REPOSITION on ArduPilot, but the new refusal gating raises the cost if it ever is.

Re-checked and correct, so not raised: the parameter packing field by field (speed -1, conditional CHANGE_MODE, radius 0, yaw NaN, degE7, metre alt) against Copter/Plane/Rover; DO_CHANGE_ALTITUDE param1/param2 against GCS_MAVLink_Plane.cpp:616-631, including that it is not in command_long_stores_location() so the LONG→INT conversion passes them through untouched; frame pass-through matching the old setPositionTargetGlobalInt tail exactly (the swarm callers' unset frame = 0 is a pre-existing Swarm/FollowPath.cs bug, not yours); and that the doCommand/doCommandInt refactor is behaviour-preserving for all existing callers.

[Copilot]

🟡 Changes recommended

Unresolved acknowledgement handling, blocking follow updates, and obsolete API usage remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates guided-flight movement to use standard MAVLink command APIs with legacy fallbacks.

Changes:

• Uses DO_REPOSITION and DO_CHANGE_ALTITUDE.
• Adds result-aware command helpers.
• Tracks unsupported commands per vehicle.
• Updates altitude handling in the flight UI.

File summaries

File	Summary
GCSViews/FlightData.cs	Uses the new altitude API.
ExtLibs/ArduPilot/Mavlink/MAVState.cs	Tracks unsupported commands per vehicle.
ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs	Implements command-based movement, result handling, and fallbacks.

Review details

Suppressed comments (2)

ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4550

• The updated FlightData call resolves to the one-argument setNewAlt(float) overload, but this overload is marked [Obsolete] here. That makes the new call emit an obsolete warning and routes the new UI code through an API declared not to be used; either remove the attribute from the intended convenience overload or call the explicit (sysid, compid, altitude) overload.

        [Obsolete]
        public void setNewAlt(float new_relhome_alt_m)
        {
            setNewAlt((byte) sysidcurrent, (byte) compidcurrent, new_relhome_alt_m);

GCSViews/FlightData.cs:4386

• The changed call resolves to the new one-argument setNewAlt(float) overload, but that overload is marked [Obsolete] just below. This introduces a deprecation warning and keeps the UI on the implicit sysidcurrent/compidcurrent target API; call the explicit-target overload here instead, or remove the obsolete attribute if the one-argument overload is intended to be the supported UI API.

                MainV2.comPort.setNewAlt(newalt / CurrentState.multiplieralt);

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-17)

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

Full report, including everything that was checked and found clean: https://uav.tridgell.net/DevCallReviews/2026_09_17_AIReview/devcall_pr_reviews.html#prMissionPlanner-3724

---

COMMENT

Re-reviewed at head f7fa9e8abb (previously 15bb12a069); my earlier comment above is superseded.

Both blocking items from my previous comment are properly fixed, and fixed the right way: setNewAlt now sends DO_CHANGE_ALTITUDE as COMMAND_INT (so MAV_RESULT_COMMAND_INT_ONLY is unreachable and the COMMAND_LONG_ONLY guard points the right way), and a refused DO_REPOSITION now throws so the operator sees it. doCommandIntResultAsync also gained the missing IN_PROGRESS branch. All 6 CI legs are green.

Still open

• ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:2932 — the shortened retry budget was applied to
  DO_REPOSITION only, so the "Change Alt" button can still freeze the UI for ~8 s on a lossy link.
  The new override tests actionid == MAV_CMD.DO_REPOSITION; DO_CHANGE_ALTITUDE (sent from :4577) keeps
  retrys = 3 / timeout = 2000, i.e. send, resend at ~2 s, ~4 s, ~6 s, throw at ~8 s. doCommandIntResult uses
  .AwaitSync(), which blocks the calling thread, and the caller GCSViews/FlightData.cs:4388 modifyandSetAlt_Click
  is a Click handler on the UI thread with uicallback passed null, so nothing pumps the message loop. After the
  throw it still runs the legacy setWP path, and since UnsupportedCommands is deliberately not populated on timeout,
  the full cost is paid again on every press. Before this PR the button went straight to setWP.
  Giving DO_CHANGE_ALTITUDE the same 1x1000 ms treatment, or making the budget a parameter, closes it.

• MAVLinkInterface.cs:4481-4489 — repeat callers now pay a blocking, comport-exclusive ACK round trip per
  vehicle per cycle (re-raised, but much reduced). Swarm/FollowPathControl.cs:79-82 runs a Thread.Sleep(200) loop
  and Swarm/FollowPath.cs:52 iterates every non-leader MAV serially inside it; each iteration is now a synchronous
  doCommandIntResult(..., requireack: true) holding giveComport, where the old tail was a fire-and-forget
  setPositionTargetGlobalInt. Controls/FollowMe.cs:196 is the same shape.
  The per-MAV catch you added means one stall no longer skips the rest, so this is a design cost rather than a defect.
  Two corrections to my own last comment: the right reference is :4481-4489, not :4474; and the ~2 s figure is
  nominal, not a worst-case bound, since packet reads can overrun the timer and IN_PROGRESS resets it.
  requireack: false for the high-rate callers would remove it — the capability probe only needs to succeed once.

Smaller points

• GCSViews/FlightData.cs:4393 — the PR's own new call site targets an overload this PR marks [Obsolete],
  so new UI code compiles with a CS0618 warning. Same point Copilot raised. In fairness, the overload it replaced was
  already obsolete, so this continues an existing warning rather than introducing one — but either drop the
  attribute from the convenience overload or call the three-argument form here.
• MAVLinkInterface.cs:2932 — command-specific timing policy is hard-coded inside a generic helper that every
  COMMAND_INT goes through. It works; an optional parameter would keep the policy at the call site, and would let
  setNewAlt opt in.
• MAVLinkInterface.cs:4510/:4600 — the COMMAND_LONG_ONLY guard is defensive-only against ArduPilot
  (MAV_RESULT_COMMAND_LONG_ONLY is emitted nowhere in master) and, unlike the UNSUPPORTED branch, does not populate
  UnsupportedCommands, so such a vehicle pays the probe round trip on every call. No objection to keeping it.

Withdrawn from my previous round: I was going to flag :4502 assigning the whole Locationwp as newly overwriting
GuidedMode.frame on Copter/Rover. The second pass pushed back and is right — copying both altitude and frame
keeps the cached target internally consistent, and preserving the old frame could reinterpret the newly cached altitude
incorrectly. The underlying issue (four callers never set Locationwp.frame at all, leaving it MAV_FRAME_GLOBAL) is
pre-existing and not yours.

Verified rather than assumed: parameter packing field by field — speed -1, CHANGE_MODE only when
setguidedmode (matching MAV_DO_REPOSITION_FLAGS_CHANGE_MODE == 1 in all three vehicles' handlers), radius 0, yaw
float.NaN, lat/lng as exact int degE7, altitude in metres. DO_CHANGE_ALTITUDE as COMMAND_INT is what
ArduPlane/GCS_MAVLink_Plane.cpp:744 expects (param1 metres, param2 frame), it is Plane-only and only since 4.6, and
everything else correctly NAKs UNSUPPORTED and is cached once. DO_REPOSITION is supported on every vehicle MP talks
to. MAVlist.Clear() on Open() means the cache cannot survive a reconnect or firmware swap. Units convert exactly
once. And I checked every one of the 13 call sites that could now see the new throw: all are already inside
try/catch, and giveComport is cleared before the throw in both helpers.

On meee1's "this shouldnt be changed" for the setNewWPAlt signature: both original overloads are preserved byte-for-byte
as thin forwarders at :4552-4562, so external and precompiled callers are unaffected, and peterbarker's reply on the
merits is confirmed by GCS_MAVLink_Plane.cpp:477-491, which reads only the altitude out of the Location.

[AP-Review]

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.

Full report, with what was checked and found clean: https://uav.tridgell.net/DevCallReviews/2026_09_18/devcall_pr_reviews.html#prMissionPlanner-3724

Re-reviewed at head b69a149d40 (previously f7fa9e8abb): both findings from my previous comment are resolved, and resolved the right way. Nothing new is blocking — COMMENT.

The branch was rebased, so GitHub's three-dot compare returns the whole PR; I recovered the real delta by diffing the two heads file by file, and it is small and entirely responsive.

Previous round

• RESOLVED — the shortened retry budget was applied to DO_REPOSITION only, so "Change Alt" could freeze the UI for ~8 s. The command-specific if inside the generic helper is gone; retries/timeoutms are parameters (:2875, :2885) consumed at :2929/:2931, and setNewAlt passes retries: 1, timeoutms: 1000 at :4582. Re-derived from the loop: send, one resend at ~1000 ms, TimeoutException at ~2000 ms — nominal worst case down from 8 s to 2 s. Defaults of 3/2000 preserve behaviour for every pre-existing caller. This is exactly the "optional parameter, policy at the call site" shape, which also resolves the second half of that finding.
• RESOLVED — GCSViews/FlightData.cs:4393 targeted an overload this PR marks [Obsolete] (CS0618, also Copilot's point). It now calls the non-obsolete three-argument overload, and no in-tree caller of setNewWPAlt or setNewAlt(float) remains outside the forwarders, so the PR introduces no new CS0618.
• Still open, unchanged in status — the COMMAND_LONG_ONLY guard is defensive-only against ArduPilot and does not populate UnsupportedCommands (now :4507/:4599). Cosmetic, and I said last time I had no objection to keeping it. Re-confirmed against master that MAV_RESULT_COMMAND_LONG_ONLY is emitted nowhere in the tree.

Still open, much reduced — MAVLinkInterface.cs:4483

setGuidedModeWP still sends DO_REPOSITION with requireack: true, so giveComport is held at :2916 and AwaitSync blocks the calling thread until an ACK or the budget expires. Affected: Swarm/FollowPath.cs:52 inside Swarm/FollowPathControl.cs:69-83 — a Thread.Sleep(200) loop iterating every non-leader MAV serially, no giveComport guard — and ExtLibs/ExtGuided/ExtGuidedPlugin.cs:61 from Loop(). Controls/FollowMe.cs:196 does guard and degrades to skipping sends instead.

With the per-MAV catch and the 4× shorter budget I would now call this a design cost rather than a defect, and three qualifications from the Codex pass are worth recording: 2 s is nominal rather than an upper bound (reads can overrun and IN_PROGRESS resets the timer); the swarm sleeps after its serial commands, so the cycle is command time plus the sleep; and requireack: false is not a free fix — it would discard refusal handling and the capability discovery that populates UnsupportedCommands, so it needs a separate probe strategy rather than just flipping the flag.

NOTE, pre-existing but newly reachable — MAVLinkInterface.cs:2957

doCommandIntResultAsync sets giveComport = true at :2916 and clears it on every explicit exit, but the await readPacketAsync() at :2957 is not inside a try/finally. The Codex pass corrected my trigger and the correction stands: the plain no-bytes 1200 ms timeout is swallowed internally at :4900 and the helper then clears the flag on its own timeout — the path that actually escapes is a truncated MAVLink header, reaching the uncaught throw at :4968, where the reader releases only readlock.

This PR does not cause it, but it changes the exposure: the old Copter/Rover tail was the fire-and-forget setPositionTargetGlobalInt, which never touched giveComport; every Fly-To-Here now takes this path. Controls/FollowMe.cs:192 gates on giveComport == false and would stop sending. Not permanent — besides connect time, other operations reset it (e.g. GCSViews/ConfigurationView/ConfigFrameType.cs:38) — but recovery depends on something else happening. A try { … } finally { giveComport = false; } around the wait loop in both helpers would close it for everyone and is cheap.

Minor — GCSViews/FlightData.cs:4396-4398

setNewAlt now throws "Alt Change Failed: " + result carrying the actual MAV_RESULT, but modifyandSetAlt_Click catches it bare and shows Strings.ErrorCommunicating, so a DENIED and a dropped link look identical to the operator. The sibling paths this PR touched all do the opposite — :2930, :3107, :5967, :5992 show Strings.CommandFailed + ex.Message. One line, for consistency.

Also: still marked draft, with all six CI legs green.

---

What the clearance covers, since most of this review was checking rather than finding. Every COMMAND_INT parameter slot was verified field by field against ArduPilot master: param1 -1 correctly ignored by Copter and Plane and guarded by Rover's is_positive(); param2 CHANGE_MODE == 1 matching all three vehicles and correctly not set for FollowMe's setguidedmode: false; param3 0 skipped by Plane's param3 > 0; param4 NaN treated as 0 by Plane and unread by Copter/Rover; param5/6 as degE7 int32 with no float in the path; param7 in metres, which is what location_from_command_t reads. On the COMMAND_LONG precision question I computed rather than asserted: at a degE7 latitude of 473977420 the float32 ulp is 32 counts, so 0.13 m of error, ~0.36 m worst case in that decade — and this code avoids it entirely by using COMMAND_INT. No AMSL/relative mismatch is introduced; the pre-PR tails used the identical frame byte, and the callers that leave Locationwp.frame at 0 are byte-for-byte pre-existing. DO_CHANGE_ALTITUDE is Plane-only, so Copter/Rover NACK UNSUPPORTED once per connection and then take the legacy path — the right shape, and the cache is cleared by MAVlist.Clear() on Open() so it cannot survive a firmware swap. All 13 setGuidedModeWP call sites are inside a try/catch, so the new throw reaches no unguarded caller. And MAVlist[].GuidedMode = gotohere scales correctly, because gotohere.id is set to WAYPOINT first and WAYPOINT carries [hasLocation], so Locationwp.Convert multiplies lat/lng by 1e7 to match the / 1e7 readers.

[AP-Review]

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.

Full report, including everything that was checked and found clean: https://uav.tridgell.net/DevCallReviews/2026_09_18/devcall_pr_reviews.html#prMissionPlanner-3724

---

COMMENT

Re-reviewed at head 08c1891283 (previously b69a149d40); my earlier comment is superseded.

Both actionable findings from the last round are resolved, and resolved the right way — a real try/finally rather than extra assignments, and an error string that now carries the MAV_RESULT. Two items stay open, both already known and one already discussed by you in this thread. Three low-severity notes below, two of which the Codex pass corrected against me. No blockers on a draft.

Previous round

• RESOLVED — the giveComport leak on a thrown wait. Both helpers now wrap the whole wait loop: doCommandResultAsync try at MAVLinkInterface.cs:2793 with finally { giveComport = false; } at :2851-2854, and doCommandIntResultAsync try at :2941 with its finally at :2999-3002. Any exception out of readPacketAsync(), out of uicallback?.Invoke(), or down the TimeoutException path now releases the port — and it fixes it for every pre-existing caller of doCommand/doCommandInt, not just the new ones.
• RESOLVED — DENIED and a dropped link no longer look identical. GCSViews/FlightData.cs:4395-4398 is now catch (Exception ex) { CustomMessageBox.Show(Strings.CommandFailed + ex.Message, Strings.ERROR); }, matching the convention at :2929, :3106, :5966, :5991.
• STILL OPEN, and acknowledged — the synchronous ACK holds the port. :4499 still passes requireack: true with retries: 1, timeoutms: 1000 at :4504, and Extensions.cs:110-118 confirms AwaitSync is Task.Run(...).GetAwaiter().GetResult(), i.e. it blocks the calling thread — reaching Swarm/FollowPath.cs:52 inside the Thread.Sleep(200) loop in Swarm/FollowPathControl.cs:69-83, and ExtLibs/ExtGuided/ExtGuidedPlugin.cs:61 from Loop(). I agree with the previous round's downgrade to a design cost rather than a defect, and you have since discussed the trade-off here. Re-raised only to name one more call site — below.
• STILL OPEN (cosmetic) — the COMMAND_LONG_ONLY guard (:4523, :4615) is defensive-only against ArduPilot and does not populate UnsupportedCommands. Unchanged, no objection. Likewise unchanged and deliberate: UnsupportedCommands is not populated on the timeout path, so a silent autopilot pays the probe every call.

Nothing from the last round is disputed — you acted on both actionable items.

NOTE — MAVLinkInterface.cs:2734 and :2924 — the new try starts one statement too late

The leak this commit closes is still reachable. In both helpers the order is if (requireack) giveComport = true; (:2734 / :2924), then generatePacket(...) (:2736 / :2926), and only then try { (:2793 / :2941) — I read the file at this head to confirm that rather than inferring it from the diff. generatePacket is not exception-safe: its BaseStream.Write(packet, 0, i) at :1441, inside the if (BaseStream.IsOpen) at :1439, is not wrapped — the try/catch pairs in that method are the SaveToTlog and logfile blocks that follow.

A link that drops between the IsOpen test and the write throws IOException out of generatePacket with giveComport already true and outside the new finally; it is captured into the returned Task, rethrown by AwaitSync, caught by setGuidedModeWP's catch at :4506, and the port stays held. Codex adds that on the non-Plane reposition path neither the catch nor the legacy fallback clears it, so normal telemetry reads stay gated off.

Narrow and pre-existing in shape, but it is exactly the class this commit set out to close. Fix: move giveComport = true; and the initial generatePacket inside the try, or open the try immediately after the requireack short-circuit return.

NOTE — Controls/OpenGLtest2.cs:123 — one more blocking-ACK caller, on the UI thread

MouseDown += OnMouseDown (:108) fires on mouse presses in the 3D view, and OnMouseDown calls setGuidedModeWP(..., false). That call is now a synchronous doCommandIntResult(..., requireack: true) which holds giveComport and blocks the calling thread; before this PR the non-Plane tail was the fire-and-forget setPositionTargetGlobalInt, which returned instantly.

Codex confirmed the call path and the pre-PR behaviour, and corrected three things in my first write-up — all of which make this milder than I had it, so taking them in order:

• it is not every mouse press — the command is attempted only past the non-zero altitude/coordinate guard, and only when support has not already been cached as absent;
• the budget is one retry at 1000 ms then timeout 1000 ms later, i.e. ~2 s nominal, reaching ~2.4 s under a deterministic model with silent reads — derived, not measured live;
• the MAV_RESULT_DENIED outcome on a Copter not already in GUIDED (ArduCopter/GCS_MAVLink_Copter.cpp:435-437, since setguidedmode false means flags 0) is not a new failure — the old position-target handler at :1085 also ignored requests outside guided operation. And a promptly received denial costs no timeout at all.

My claim that this fires on the press that begins a view drag is UNCONFIRMED — Codex could not establish that such a gesture is implemented, and neither could I. So what is left is narrow: this specific caller would be better with requireack: false or a fire-and-forget async, and the empty catch at :127-130 means a stalled call shows the operator nothing. The general design cost you have already acknowledged; this just adds the call site.

NOTE — MAVLinkInterface.cs:2711 and :2896 — "no link" reads as "the vehicle refused", in the log

Both async helpers return MAV_RESULT.FAILED when BaseStream == null || !IsOpen. In setGuidedModeWP that is neither UNSUPPORTED nor COMMAND_LONG_ONLY, so :4523-4527 logs "DO_REPOSITION refused: FAILED" and throws.

Narrowing this after Codex's check, which was fair: the FAILED-for-closed-link return is explicitly documented behaviour, and the operator-visible strings ("Guided Mode Failed: FAILED", "Alt Change Failed: FAILED") do not actually attribute refusal to the vehicle — only the log line says "refused". So it is a diagnostic-wording point, not a refusal-handling bug. Still worth a distinct sentinel, since the PR's whole new error path keys off MAV_RESULT.

Checked clean

Every command parameter slot was re-verified against ArduPilot master rather than carried over, by both reviewers independently. DO_REPOSITION (:4491-4504) sends (-1, flags, 0, NaN, latE7, lonE7, altMetres): param1 -1 — Copter and Plane never read it, Rover guards with is_positive() (Rover/GCS_MAVLink_Rover.cpp:530); param2 CHANGE_MODE only when setguidedmode, matching the flag tests in Copter :434, Plane :587, Rover :511; param3 0 — Plane's packet.param3 > 0 guard at GCS_MAVLink_Plane.cpp:605 skips set_radius_and_direction; param4 NaN — Plane :568 reads it as loiter_ccw = 0; param5/6 declared int so (int)(lat * 1e7) reaches command_int.x/.y as exact int32 with no float anywhere in the path; param7 metres, which is what location_from_command_t expects. DO_CHANGE_ALTITUDE (:4590-4598) goes out as COMMAND_INT with param1 metres and param2 GLOBAL_RELATIVE_ALT, exactly what GCS_MAVLINK_Plane::handle_command_int_DO_CHANGE_ALTITUDE (ArduPlane/GCS_MAVLink_Plane.cpp:616-631) reads, so MAV_RESULT_COMMAND_INT_ONLY is unreachable.

Frame handling adds no conversion. Every frame MP can put in Locationwp.frame is accepted by mavlink_coordinate_frame_to_location_alt_frame (GCS_Common.cpp:7574-7590), and the frame byte passed at :4501 is byte-for-byte the one the pre-PR setPositionTargetGlobalInt/setWP tails used — so relative-vs-AMSL conversion happens exactly once, as before. setNewAlt hardcoding GLOBAL_RELATIVE_ALT matches the legacy setWP(..., GLOBAL_RELATIVE_ALT, 3) it replaces. Not updating MAV.GuidedMode.z is correct rather than an oversight: the legacy path sent current = 3, and the cache is only written for current == 2 (:5809-5813, :5837-5841).

The try/finally rewrite is otherwise behaviour-preserving: diffed against b69a149d40, the only semantic change is the added finally; every pre-existing early return still returns before entering the try, and the retry generatePacket calls are inside it. All 14 call sites of setGuidedModeWP/setNewAlt outside MAVLinkInterface.cs are inside a try/catch, so the new throw reaches no unguarded caller. API compatibility holds: both original setNewWPAlt overloads survive as [Obsolete] forwarders at :4565-4575, and doCommandAsync/doCommandIntAsync delegate to the *Result variants with == MAV_RESULT.ACCEPTED, reproducing the old bool exactly, with retries/timeoutms defaulting to 3/2000 so no pre-existing caller changes timing. CI is green (6/6).

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-20)

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

Re-reviewed at head e70a86a7ad (previously 08c1891283) — my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_20/devcall_pr_reviews.html#prMissionPlanner-3724

Verdict: COMMENT. Still a draft. The one actionable item from last round is fixed, and fixed the right way — nothing below blocks. I also have a correction to my own previous compatibility claim, which turned out to be wrong in a way that matters.

Previous round — every finding accounted for

• RESOLVED — the try started one statement too late. At this head MAVLinkInterface.cs:2739 opens the try, :2743 takes giveComport, and :2747 is the first generatePacket, with the finally at :2857-2858 guarded by if (heldComport). Same shape in the INT helper (try :2930, acquisition :2934, send :2938, finally :3010-3011). So an IOException out of BaseStream.Write (:1444) can no longer strand the port. All four early return MAV_RESULT.ACCEPTED paths (:2765, :2772, :2787, :2796) dropped their manual giveComport = false and rely on the finally — I checked that's behaviour-identical for requireack: true.
• RESOLVED — "no link" read as "the vehicle refused" in the log. :4535 now reads "DO_REPOSITION failed". The distinct-sentinel suggestion wasn't taken (a closed link still surfaces as MAV_RESULT.FAILED), but the misleading attribution is gone and the user-visible strings never claimed refusal. Closing it.
• STILL OPEN, and I agree it's a design cost rather than a defect — the synchronous ACK holds giveComport on high-rate callers. :4500-4513 with requireack:true, retries:1, timeoutms:1000, reached from Swarm/FollowPath.cs:52, ExtGuidedPlugin.cs:61, FollowMe.cs:196, OpenGLtest2.cs:123. One qualification I verified this round that makes it milder than my earlier rounds implied: readPacketAsync() parses and dispatches inline (:5530/:5532 → CurrentState.cs:2278), so telemetry keeps flowing and cs keeps updating while the call waits; what stalls is the calling thread plus anything gated on giveComport. The actual sleep is in Swarm/FollowPathControl.cs:82, after SendCommand(), so command time extends the cycle.
• DISPUTED — by me, against myself. I withdraw the 3D-view drag sub-claim. Last round I flagged as unconfirmed the worry that OpenGLtest2.OnMouseDown fires on the press that begins a camera drag. It doesn't — OnMouseMove (:133-159) only stores coordinates; there's no camera pan/rotate driven from mouse movement anywhere in the handler. The gesture doesn't exist, so the concern is void.
• STILL OPEN (cosmetic) — the COMMAND_LONG_ONLY guard at :4532/:4624 remains unreachable against ArduPilot (0 matches for MAV_RESULT_COMMAND_LONG_ONLY in master), and UnsupportedCommands is still deliberately not populated on the timeout path.

Backward compatibility — and a correction to what I told you last round

This is the angle that matters most for a GCS, and I got it wrong before.

DO_CHANGE_ALTITUDE is Plane-only and lands later than I said: first in Plane-4.6.3, not 4.6.0. Fetching each tag's ArduPlane GCS sources directly: 4.5.7 → 0 matches, 4.6.0 → 0, 4.6.1 → 0, 4.6.2 → 0, 4.6.3 → present, 4.7.0 → present. My 2026-09-18 comment said "only since 4.6", which was wrong — it landed inside the 4.6 point-release line, after 4.6.2. So every Plane up to and including 4.6.2, plus every Copter and Rover ever, NAKs this command.

DO_REPOSITION on Copter and Rover is also younger than "years": Copter-4.0.7 has 0 matches; 4.1.0 has it. So 4.0.x needs the fallback too.

The upshot: the legacy fallback carries the majority of the fleet, not a corner case — which makes it the most important thing in this PR, and I'm glad to say it works. On Copter-4.0.7, COMMAND_INT is dispatched (GCS_Common.cpp:3158) and handle_command_int_packet ends in return MAV_RESULT_UNSUPPORTED;, so an old vehicle ACKs promptly, :4530 caches it in UnsupportedCommands, and every later call skips straight to setPositionTargetGlobalInt with no probe cost and no timeout. MAVState.cs:329 uses a ConcurrentDictionary, so that cache is safe against the UI thread, the FollowMe thread and the swarm loop touching the same MAV. The residual cost is only an autopilot that never ACKs: retries:1, timeoutms:1000 gives ~2 s before the legacy path, repeated per call because the timeout deliberately doesn't populate the cache.

New this round — all NOTE level

• :2736 and :2927 (comments) — heldComport records "we assigned it", not "it wasn't already held", so "only ever release a hold we took ourselves" overstates the guarantee. With requireack:true the finally still clears an outer caller's hold: Utilities/Firmware.cs:808/812 and 861/865 set giveComport = true, then reach the default-ACK reboot call (:2610-2613) and the early return at :2787. Not a regression — the pre-PR code cleared it on that same return — and heldComport = !giveComport would change those reboot paths, so I'm not asking for a change. Only the comment.
• In your favour: dropping giveComport = false from the !requireack path fixes a pre-existing bug in getHomePositionAsync. :3423 takes the hold and the retry at :3437 uses requireack:false, which under the old code cleared it mid-operation; it now survives. One correction to my own reasoning: I shouldn't have said the main reader would "steal and lose" the reply — the result arrives through the subscription installed at :3406-3417, which a main-reader delivery can also satisfy. The fix prevents competing reads; it doesn't follow that the reply was being lost. Still worth a line in the commit message as a second bug closed.
• Controls/OpenGLtest2.cs:127-130 and Swarm/FollowPath.cs:60-63 are bare catch blocks and neither file has an ILog field. Two corrections to my earlier framing: "no trace anywhere" is wrong — timeouts are logged at :4518, refusals at :4535, legacy-path exceptions at :4570, so there is a log trace, just no caller-level feedback. And these two catches are added by this PR, not pre-existing. The missing e.Button filter in OnMouseDown (any button attempts a reposition) is pre-existing, subject to setGuidedModeWP's own coordinate/altitude guard.
• Out of scope, on the record: the guided HTTP endpoints (Utilities/httpserver.cs:914-924, :963-973, and the Xamarin copies at :776-786, :825-835) catch and then unconditionally write 200 OK / "Sent Guide Mode Wp". Those catches are pre-existing, but this PR's new refusal throw at :4536 escapes the retained catch at :4568-4571, so it's the first time a vehicle refusal can reach them and be reported as success.
• Low value: the happy path loses the setGuidedModeWP/setNewWPAlt summary log lines (:4549/:4639 sit inside the legacy fallback blocks, below the returns at :4525/:4617). Information isn't lost — :2923 logs the command and parameters — just reformatted. Mentioning it only because MP support work leans on these logs.

What was checked and found clean

The command packing was re-verified field by field against current ArduPilot master rather than carried over. DO_REPOSITION sends (-1, flags, 0, NaN, latE7, lonE7, altMetres): param1 -1 is unread by Plane/Copter and guarded by is_positive(packet.param1) in Rover; param2 sets CHANGE_MODE only when setguidedmode, matching all three vehicles' tests; param3 0 is skipped by Plane's !isnan && > 0 and unread by Copter/Rover; param4 NaN is NaN-guarded by Plane and unread by Copter and Rover — the one I most wanted to re-check, since NaN into an unguarded consumer would be a real bug, and it's clean; param5/6 are declared int all the way down and reach command_int.x/.y as exact int32 degE7, no float32 anywhere; param7 is metres with the frame byte handled by location_from_command_t, which rejects isnan(in.z). DO_CHANGE_ALTITUDE sends param1 metres and param2 GLOBAL_RELATIVE_ALT, which is exactly what handle_command_int_DO_CHANGE_ALTITUDE reads before its int32_t(alt * 100) — matching the legacy setWP(..., GLOBAL_RELATIVE_ALT, 3) it replaces and the UI's CurrentState.multiplieralt division at GCSViews/FlightData.cs:4412. No new unit or AMSL/relative conversion is introduced.

No mechanism went away: comparing declaration lists against efb080190d, 0 methods removed and 5 added; both legacy tails (setWP with current 2/3, setPositionTargetGlobalInt) retained and reachable; both original setNewWPAlt overloads kept as [Obsolete] forwarders at :4574-4584 with no in-tree caller, so no new CS0618. All 12 external setGuidedModeWP call sites are inside try/catch, so the new throw reaches no unguarded caller. MAVlist[...].GuidedMode = gotohere at :4524 scales correctly via Locationwp.Convert, and setNewAlt correctly does not touch MAV.GuidedMode (the cache is written only for current == 2, :4168-4174). MAV_RESULT.IN_PROGRESS is handled in both helpers. Copter's MAV_RESULT_DENIED outside guided is a new report of an old no-op, not a new failure, and a prompt denial costs no timeout.

Computing the delta needed the merge-base method — the three-dot compare returns ~85 files including CLAUDE.md, .fr.resx translations and CI churn, because the branch was rebased. The real delta is confined to MAVLinkInterface.cs.

CI is 6 of 6 green but compile-only — Build Debug/Release/AAB/APK/OSX/IOS. No unit test, no SITL round trip, nothing that exercises either command against a vehicle. The only behavioural evidence on this PR is your 2026-05-17 note ("Tested basic do-repos and changealt using github build products"), which predates this head by four months and several rewrites of the helpers. Since the heldComport/try rework touches every doCommand/doCommandInt caller in Mission Planner, one manual smoke test of an unrelated ack-requiring command (an arm, a reboot, a calibration) before undrafting would be cheap insurance.

_{Both a primary read and an independent second pass were run; disagreements are recorded in the report rather than averaged away. Exact per-release match counts differed between passes (we counted different file sets), so only the release boundaries above are stated as fact. Anything not reproduced is labelled as such — please push back on it.}

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-24)

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

Re-reviewed at head ef6df54ba4; my earlier comment above is superseded. COMMENT — nothing blocking on a draft. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_24_AIReview/devcall_pr_reviews.html#prMissionPlanner-3724

The delta since e70a86a7ad is comment text only (6 insertions / 2 deletions, both the reworded explanatory comment above bool heldComport). The PR is in good shape — every previously blocking item is fixed. One issue is worth your attention before this leaves draft, because its consequence is an altitude-frame flip, and the Codex cross-check reached the same conclusion independently and rated it blocking.

The one substantive item

ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4528 — the accepted-DO_REPOSITION path clobbers the cached GuidedMode.frame.

MAVlist[sysid, compid].GuidedMode = gotohere; assigns the whole struct, so GuidedMode.frame takes the caller's Locationwp.frame. Four callers never set it — Controls/FollowMe.cs:178-186, Swarm/FollowPath.cs:52, ExtLibs/ExtGuided/ExtGuidedPlugin.cs:62, and both httpserver.cs handlers — so it becomes 0 = MAV_FRAME_GLOBAL (AMSL). The pre-PR Copter/Rover tail was setPositionTargetGlobalInt, which touches only GuidedMode.x/.y/.z and deliberately leaves .frame alone, so this clobber is new for Copter and Rover. (Plane already had it, via setWP(current=2).)

Failure sequence: right-click -> "Fly To Here Alt", choose frame "Relative" (FlightData.cs:2931-2932 stores .frame = 3). Run FollowMe, the swarm FollowPath, or an httpserver guided request — GuidedMode.frame becomes 0. Then use "Fly To Coords": FlightData.cs:5952-5954 takes the frame from the cache (now 0) while the altitude comes from what the user just typed at :5974. A "50" meant as 50 m above home goes out as 50 m AMSL. At a 500 m ASL site that is 450 m below the launch point.

Worth noting the history, since I withdrew a broader version of this last round: the Codex argument that copying altitude and frame together keeps the cached target internally consistent is correct, and it does hold for FlightData.cs:3114-3117 ("Fly To Here", which reads both altitude and frame from the cache). It does not cover flyToCoordsToolStripMenuItem_Click, which mixes a fresh altitude with a cached frame. That is the distinction that makes this round's version stand.

Cheapest fix that preserves the consistency argument: set .x/.y/.z (and .frame only when the caller actually supplied one) rather than assigning the whole struct — or give the four frameless callers an explicit frame = (byte)MAV_FRAME.GLOBAL_RELATIVE_ALT, which would also fix the wrong frame they already send.

Notes

• Controls/OpenGLtest2.cs:108 subscribes MouseDown += OnMouseDown with no button filter and no connected/guided/mode guard, and the blocking call is at :123, so every mouse-down of any button calls setGuidedModeWP(..., false). doCommandIntResult is AwaitSync() with a null uicallback, so nothing pumps the message loop. Re-deriving the budget: readPacketAsync forces ReadTimeout = 1200 and swallows its own TimeoutException, giving about 2.0 s nominal and roughly 2.4 s on a silent link — a typical figure, not a strict upper bound. Pre-PR this already sent a guided target on every press; what is new is the blocking wait. requireack: false or a fire-and-forget async would suit this caller. Correction to last round: the "fires on the press that begins a drag" framing was wrong — there is no drag handling in OnMouseMove. "Any mouse press" is the confirmed claim.
• MAVLinkInterface.cs:2697 — the new XML doc says "Commands not requiring an ack return ACCEPTED; a closed link returns FAILED", but omits that PREFLIGHT_CALIBRATION, PREFLIGHT_REBOOT_SHUTDOWN and GET_HOME_POSITION also return ACCEPTED without any ack even when requireack is true (:2767, :2774, :2789, :2798). Callers switching on the result would read those as vehicle confirmation. One extra sentence. (Calibration bypasses the ACK wait when p5==1 or p6==1, not only when both are 1; and the behaviour predates this PR.)

Checked and clean

Every DO_REPOSITION (192) param slot was checked against MP's generated Mavlink.cs:987-990 and each ArduPilot handler: p1 -1 groundspeed (Copter and Plane never read param1, Rover guards with is_positive); p2 flags, where CHANGE_MODE == 1 matches Copter, Rover and Plane and is correctly not set for the FollowMe/OpenGL setguidedmode:false callers; p3 radius 0 (Plane gates on > 0); p4 float.NaN (Plane maps NaN to clockwise, the old default); p5/p6 as int32 degE7 straight into COMMAND_INT x/y with no float in the path; p7 metres. DO_CHANGE_ALTITUDE (186): p1 metres, p2 (float)MAV_FRAME.GLOBAL_RELATIVE_ALT — exactly what ArduPlane/GCS_MAVLink_Plane.cpp:613-629 reads; sent as COMMAND_INT so MAV_RESULT_COMMAND_INT_ONLY is unreachable.

On compatibility: DO_CHANGE_ALTITUDE is handled only by Plane, and the base handle_change_alt_request is an empty virtual, so Copter/Rover fall through to MAV_RESULT_UNSUPPORTED, populate the negative cache once per connection and use the legacy path thereafter — and that legacy path was also a no-op on Copter/Rover while still answering MAV_MISSION_ACCEPTED, so nothing regresses. The cache cannot outlive a reconnect or firmware swap, since MAVlist.Clear() runs in Open(), and MAVList's indexer auto-creates rather than throwing, so the new pre-try UnsupportedCommands access cannot throw for an unknown MAV. All 13 setGuidedModeWP call sites and the single setNewAlt call site are inside a try/catch at this head, so the new throw reaches no unguarded caller. All 6 CI legs green.

The previous round's try-placement finding is resolved — in both helpers bool heldComport = false; is declared at :2739/:2932 with the try opening immediately after, and the finally releases only a hold this call took. That also fixes a real pre-existing bug at getHomePositionAsync (:3427/:3441), where the old unconditional giveComport = false released the hold getHomePosition had taken around its own retry loop.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-24)

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

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_24_AIReview/devcall_pr_reviews.html#prMissionPlanner-3724

Re-reviewed at head f001c557c1 (previously ef6df54ba4). COMMENT, still a draft. The delta since my last comment is 10 insertions / 2 deletions in one file, and it is exactly the two items I raised.

Previous round:

• The whole-struct GuidedMode assignment at :4528 is resolved — and the new shape is the right one rather than merely a different one: setPositionTargetGlobalInt at :4719-4722 does precisely the same three .x/.y/.z assignments and deliberately leaves .frame alone, so your accepted-DO_REPOSITION path now matches the tail it replaces, and Plane stops inheriting the old setWP clobber.
• The XML doc omission at :2697 is resolved.
• Controls/OpenGLtest2.cs:108 is still open — re-raising rather than presenting as new. It is still a bare MouseDown += OnMouseDown, OnMouseDown at :116 still has no e.Button test, and the blocking setGuidedModeWP(..., false) is still at :123. The try/catch you added handles the new throw but not the "any button, always" trigger.

The per-vehicle support matrix is the governing risk here, so I established it rather than assuming it — from the ardupilot tree via git log -S and git tag --contains:

	DO_REPOSITION	DO_CHANGE_ALTITUDE
Plane	3.6.0	4.7.0
Copter	4.1.0	not handled
Rover	4.1.0	not handled
Sub	4.7.0	not handled

Sub is the outlier — ArduSub-stable is a 4.5.x, so every Sub in the field on 4.5 or earlier has no DO_REPOSITION handler. This is not a regression, and I verified the mechanism rather than assuming it: every release I checked returns MAV_RESULT_UNSUPPORTED from the default of GCS_MAVLINK::handle_command_int_packet (confirmed on master and at ArduSub-4.1.0, Copter-4.0.7 and Rover-4.0.0 via git show <tag>:...), so MP NAKs once, caches it in UnsupportedCommands and uses the legacy tail for the rest of the connection. DO_CHANGE_ALTITUDE behaves the same way, and the legacy path it falls back to was also a no-op on those vehicles (handle_change_alt_request is an empty virtual at GCS.h:1021, overridden only by Plane), so nothing is lost. Firmware old enough to ignore COMMAND_INT entirely never ACKs, the TimeoutException is caught at :4526, and the legacy path runs.

ISSUE — the guided-target marker disappears on tlog playback and for a second GCS

ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:5884

processInfoFromStream is the receive-side snooper that rebuilds MAV.GuidedMode from traffic on the wire, and it handles exactly the three messages this PR stops sending: MISSION_ITEM with current == 2 (:5834), MISSION_ITEM_INT with current == 2 (:5862), and SET_POSITION_TARGET_GLOBAL_INT (:5889). There is no COMMAND_INT case — the full list is MISSION_COUNT, MISSION_ITEM, MISSION_ITEM_INT, HOME_POSITION, SET_POSITION_TARGET_GLOBAL_INT, RALLY_POINT, CAMERA_FEEDBACK, FENCE_POINT, PARAM_VALUE, TIMESYNC.

Two paths are affected. generatePacket writes every sent packet to the tlog (:1450), and processInfoFromStream is called at :5296 before the "if its a gcs packet - dont process further" guard at :5299, with its own comment saying "including gcs packets on playback" — so replaying a guided flight no longer reconstructs the target. The same gap applies live to a second Mission Planner watching a shared link.

Consequence: MAV.GuidedMode.x stays 0, and FlightData.cs:4222-4226, FlightPlanner.cs:6925-6929 and the Xamarin FlightData.xaml.cs:1280-1283 all gate the blue "Guided Mode" marker on GuidedMode.x != 0, so it is absent or stale. The sending instance is fine, which is why this survived the earlier rounds. A COMMAND_INT branch decoding DO_REPOSITION into GuidedMode would close it, and would be the natural home for keeping DO_CHANGE_ALTITUDE in sync.

Severity is a feature regression in log review and multi-GCS awareness, not flight safety — but worth fixing before this leaves draft, since it rots silently: nobody notices a missing marker until they are reviewing an incident.

Checked and clear

Lat/lon scaling is correct — the thing most likely to be wrong in a change like this. Locationwp.lat/.lng are double, (int)(gotohere.lat * 1e7) is computed in double and binds to the int p5/p6 parameters of doCommandIntResult, landing in command_int.x/.y as exact int32 degE7. No float anywhere in the path. The altitude frame is a faithful pass-through of what the old code sent on both tails, MP's UI offers only GLOBAL / GLOBAL_RELATIVE_ALT / GLOBAL_TERRAIN_ALT, and all three are accepted by the same mavlink_coordinate_frame_to_location_alt_frame helper the old path used — so no terrain-frame regression. The legacy fallback at :4654 passes the matching frame, which I checked specifically because it builds a fresh Locationwp whose default frame is 0.

Every ACK outcome walked: ACCEPTED caches and returns; UNSUPPORTED falls through to legacy; COMMAND_LONG_ONLY falls through; no ACK at all raises TimeoutException, caught, leaving result null so the guard short-circuits and legacy runs; FAILED/DENIED throws and deliberately skips legacy, which is your stated point. Parameter semantics check out field by field, including param2's CHANGE_MODE flag being set only for setguidedmode and correctly not for the two follow-style callers. param4 NaN survives the wire (not a trailing field, so MAVLink2 truncation cannot touch it). All 13 setGuidedModeWP call sites and the 1 setNewAlt site are inside try/catch. The helper refactor is behaviour-preserving and fixes two real pre-existing giveComport leaks. CI 6/6 green.

Incidentally an improvement worth noting: SET_POSITION_TARGET_GLOBAL_INT silently hold_position()s on a frame it cannot convert, whereas DO_REPOSITION returns DENIED, which MP now surfaces to the operator.

Two concerns from my second pass were dropped after checking them, so you are not left chasing them: MAV_RESULT_TEMPORARILY_REJECTED being treated as a hard refusal does not stand (it means retry later, it is not cached as unsupported, and the no-fallback-after-refusal behaviour is deliberate per your comment); and MAVList.Clear() emptying only masterlist is unreachable, since heartbeat handling promotes vehicle states through MAVlist.Create at :5494.

[AP-Review]

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

Re-reviewed at head 8072efa979 (previously told f001c557c1); my earlier comment above is superseded. Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/2026_09_28_AIReview/devcall_pr_reviews.html#prMissionPlanner-3724

The new COMMAND_INT snooper fixes last round's playback issue and decodes correctly. The verdict moves because of something we got wrong last round: we called the field-wise GuidedMode update on the accepted DO_REPOSITION path "the right shape". Both independent Codex passes then showed it leaves a stale altitude frame on Plane. Apologies for steering you there.

Issue

• ExtLibs/ArduPilot/Mavlink/MAVLinkInterface.cs:4534-4536: on Plane, a relative altitude is later re-sent as AMSL.
  • GuidedMode starts zeroed, so its frame is 0 = MAV_FRAME_GLOBAL.
  • On a fresh connection "Fly To Coords" picks GLOBAL_RELATIVE_ALT (GCSViews/FlightData.cs:5951-5954). The accepted DO_REPOSITION caches x/y/z but leaves the frame at 0.
  • The next "Fly to Here" builds its target from GuidedMode.z and GuidedMode.frame (FlightData.cs:3114-3117), so a 100 m relative target is re-sent as 100 m AMSL, which is below ground at most sites. A later "Fly To Coords" also inherits GLOBAL.
  • On master, Plane went through setWP, which stored the whole target including its frame (:4177), so this is a Plane regression. Copter/Rover's setPositionTargetGlobalInt path already leaves the frame stale on master.
  • Your comment is right that caching a caller's default frame 0 would be just as bad. So record the frame where the operator chose it: set MainV2.comPort.MAV.GuidedMode.frame = frame; in "Fly To Coords", as "Fly to Here Alt" already does (FlightData.cs:2932), and/or store gotohere.frame on the accepted path when the caller supplied one.

Previous round

• RESOLVED: guided marker missing on tlog playback. MAVLinkInterface.cs:5893-5909 now rebuilds GuidedMode from DO_REPOSITION / DO_CHANGE_ALTITUDE. It still runs before the playback guard (:5296 vs :5299).
• WITHDRAWN: Controls/OpenGLtest2.cs:108 any-button mouse-down. master already sends a guided target on every MouseDown there, and your change only adds the try/catch. Our error.
• RESOLVED, but see the Issue above: the whole-struct GuidedMode assignment. The XML doc item is resolved too.

Notes

• MAVLinkInterface.cs:5900: the snooper also drops the packet's frame (cmd.frame, or param2 for DO_CHANGE_ALTITUDE).
  • A received packet always carries one, and the snoopers this replaces kept it (locationwp.cs:71, :90, :115).
  • A terrain-frame DO_REPOSITION leaves z=100.5 with frame 0.
  • This mostly affects playback, but processInfoFromStream also sees another GCS's commands live.
• MAVLinkInterface.cs:5907: the snooper records the request, not the outcome.
  • A DENIED DO_REPOSITION still moves the cached target. So does DO_CHANGE_ALTITUDE on Copter/Rover/Sub, which answer UNSUPPORTED.
  • That cached target feeds "Fly to Here" / "Fly to Here Alt". Meanwhile the live setNewAlt ACCEPTED path (:4627) never updates GuidedMode.z.

Checked: degE7 x/y and float z scaling match every other GuidedMode writer and reader. Mono and .NET harnesses built against ExtLibs/Mavlink round-trip lat/lng/z and NaN param4 on both wire versions. Unknown or broadcast targets create a hidden MAVlist entry and never throw; it is promoted intact on the first heartbeat. MP's own sent packets never reach the snooper live. UNSUPPORTED is cached and falls back, COMMAND_LONG_ONLY falls back uncached, DENIED/FAILED throw, and a timeout falls back and releases the port. CI 6/6. A full MissionPlanner build wasn't feasible here.