MAVLink mission transfer state machine

[xznhj8129]

⚠ Part 5/7 of the mavlink_multiport2 split — depends on #11715.
GitHub can't base an upstream PR on a fork branch, so this targets maintenance-10.x and its diff also contains the earlier parts of the stack. Review only this part's own commits:

• `98a301e71` Add MAVLink mission transfer state machine
• `6857a38d6` Add MAVLink SITL sanity harness

This part in isolation: xznhj8129/inav@mav/04-streams-protocol...mav/05-mission
Merge order: parts 1 → 7 in sequence.

Part 5/7 of the mavlink_multiport2 stack.

Rewritten MAVLink mission protocol, replacing the old ad-hoc counter-based implementation (absent since part 3/7) with an owned transfer state machine. The transfer design is adapted from Betaflight's GPLv3 mavlink_mission.c (commit 87d4bd63) and reshaped for INAV's routed multi-port runtime.

Transfer robustness

• Transfers track the initiating system, component, and ingress port; only that partner may continue a transfer.
• Uploads retry the outstanding request every 1.5 s and abort after five retries. Downloads expire after 5 s of inactivity and accept one-item retransmission.
• Out-of-sequence upload items re-request the expected item; valid out-of-order download requests are served; explicit MAV_MISSION_OPERATION_CANCELLED returns the transfer to idle.
• Legacy MISSION_REQUEST downloads are answered with MISSION_ITEM_INT, per the current MAVLink mission service.
• Protocol-specific failure ACKs throughout.

Translation (MAVLink ↔ INAV waypoints)

• Uploads are staged: the full MAVLink mission is received and translated into a temporary INAV waypoint list, validated, and committed only after the transfer succeeds — a failed upload never leaves a half-written mission.
• QGC planned-home item 0 is skipped (INAV stores home separately); MAVLink sequence 1 becomes INAV waypoint 1. A real current first waypoint is preserved, not mistaken for planned home.
• QGC-style NaN/unused waypoint params are tolerated.
• Modifier items fold onto the correct INAV waypoint instead of consuming slots: DO_CHANGE_SPEED → pending leg speed; CONDITION_DELAY → previous waypoint becomes/updates POSHOLD_TIME; altitude modifiers update the previous geographic waypoint while preserving INAV's p3 bitfield semantics (bit 0 = AMSL reference, bits 1–4 = user actions).
• NAV_WAYPOINT → WAYPOINT or POSHOLD_TIME (hold time set), NAV_LOITER_TIME → POSHOLD_TIME, NAV_LAND → LAND (retaining supplied coordinates), DO_JUMP → JUMP with remapped target.

Persistence and reporting

• Successful uploads and mission clears update nonvolatile waypoint storage; on persistence failure the previous mission is restored in RAM instead of leaving it cleared or half-written.
• 1 Hz MISSION_CURRENT (count, current item, execution mode, state); active-item flags on downloads.
• MISSION_ITEM_REACHED broadcast to all active MAVLink ports from a navigation-side reached latch (hold-time waypoints, altitude-enforced holds, and waypoint-next transitions).

Caveats

Mission translation to INAV's MSP waypoint model is lossy (leg speed, LAND elevation, RTH land flag, p3 user-action bits). MAVLink mission downloads are best-effort reconstruction from the stored MSP waypoint list — not a canonical mission backup. Documented in docs/Mavlink.md (part 7/7).

Testing

• Unit slice: 58/58 passing (mavlink_unittest).
• Includes the live SITL mission rig (src/test/mavlink/missions/, 8 scenario cases incl. planned-home skip, modifier folding, JUMP remap, failed-upload preservation, legacy download) and the routing compliance harness (src/test/mavlink/routing/).
• Also includes the standalone SITL sanity harness (src/test/mavlink/sanity/): 8-check pass/fail rig covering MSP, MAVLink telemetry, RC override, GCS heartbeat, mission upload/readback, and stream/message-rate control.
• Full SITL build, warnings-as-errors, clean.
• X-Plane and QGC SITL testing successful; proprely made missions (without takeoff) load and fly accurately

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[sensei-hacker]

This is an AI-generated draft of things to check on — flagged for the author's judgment, not a formal review verdict.

Reviewed this part in isolation (pr-11715-head..pr-11716-head, commits 98a301e718/6857a38d6f), and independently verified the SITL/hardware builds and the mavlink_unittest 58/58 result — all check out cleanly. Architecturally this is solid: the staged-upload → validate → commit-with-rollback flow genuinely protects against a half-written mission on failure, and the DO_JUMP target remapping correctly rejects jumps into folded modifier items. A few things worth a look before/around merge:

1. LAND-terminated missions may never report completion. This PR wires the new "waypoint reached" latch (navMarkWaypointReached/navigationConsumeWaypointReached) into three places: the HOLD_TIME case in NAV_STATE_WAYPOINT_REACHED, the altitude-reached branch in NAV_STATE_WAYPOINT_HOLD_TIME, and the catch-all in NAV_STATE_WAYPOINT_NEXT's entry. But NAV_STATE_WAYPOINT_RTH_LAND's entry (unchanged by this PR) routes success straight to RTH_FINISHING/RTH_FINISHED → WAYPOINT_FINISHED, bypassing WAYPOINT_NEXT entirely. So a mission ending in NAV_WP_ACTION_LAND — the normal terminal action for fixed-wing/rover — looks like it would never fire MISSION_ITEM_REACHED for that item, and MISSION_CURRENT would report MISSION_STATE_NOT_STARTED indefinitely after landing rather than MISSION_STATE_COMPLETE. Did your SITL/QGC testing happen to cover a LAND-ending mission specifically, or mostly HOLD_TIME/WAYPOINT-ending ones? If this is real it'd be worth a fix and a test that drives the actual FSM rather than stubbing navigationConsumeWaypointReached() directly.

2. MISSION_CLEAR_ALL doesn't check transfer ownership. MISSION_COUNT and MISSION_REQUEST_LIST both deny a non-owning partner while a transfer is active (mavMissionTransfer.state != IDLE && !mavlinkMissionSenderOwnsTransfer()). mavlinkHandleIncomingMissionClearAll doesn't have that check — any local-target sender can cancel another partner's in-progress upload/download. Intentional (CLEAR_ALL as a "global" op), or worth the same guard for consistency with "only that partner may continue a transfer"?

3. Legacy MISSION_REQUEST (non-INT) download path looks unreachable. Both mavlinkHandleIncomingMissionRequest and mavlinkHandleIncomingMissionRequestInt call mavlinkSendMissionItemResponse(seq, true) — hardcoded true, so the useIntMessages == false branch (the mavlink_msg_mission_item_pack float encoder) can never execute. If INT-encoding is intentionally used unconditionally per the mission microservice spec, a one-line comment at both call sites would save a future reader from "fixing" what looks like a bug.

4. mavlinkHandleMissionItemCommon is ~356 lines, and the mission_type != MAV_MISSION_TYPE_MISSION rejection boilerplate repeats near-verbatim across five handlers. Not blocking, but the duplication is plausibly related to #3 above (a decision applied in one copy, missed in a sibling) — might be worth splitting per-command validate/build helpers at some point given parts 6/7 build on top of this file.

Minor, bundled:

• Lines ~609-617: eight UNUSED(...) calls on parameters that are used elsewhere in the same function — safe to delete.
• mavlinkHandleArmedGuidedMissionItem's current == 2/current == 3 are magic numbers for guided fly-to vs. guided altitude-change — a short comment would help.
• The upload-commit snapshot (mavlinkMissionSnapshot_t previousMission) is stack-allocated in two places while the staging buffer is a static global elsewhere in the file — not a safety issue given the module's non-reentrancy, just a style inconsistency.

Aside, spanning the stack rather than specific to this PR: by #11717 there are now four independent static bool mavlinkIsLocalTarget(...)-shaped helpers doing the same "is this addressed to my system/component" check — one each in mavlink_command.c, mavlink_guided.c, mavlink_streams.c (all literally named mavlinkIsLocalTarget), plus mavlinkMissionTargetIsLocal here in mavlink_mission.c. None are declared in a shared header. Not urgent, but flagging now in case it's easier to consolidate before the stack finalizes than after.

[xznhj8129]

Thanks for the review — all four were looked at, and we also ran an independent second-opinion audit over the diff, which turned up a few more issues in the same areas. Everything below is pushed (this PR's unit suite grew to 62 tests, all green; SITL builds warnings-as-errors at every stack level; and the shipped src/test/mavlink rigs pass against the 7/7 tip: missions 8/8, routing 11/11, tunnel OK).

1. LAND completion — confirmed, and it went deeper than the finding. Fixed in 02d5b36. Marking in the WAYPOINT_RTH_LAND success branch alone would still have missed the fixed-wing autoland path: with USE_FW_AUTOLAND and a configured approach, landing exits into the autoland FSM and terminates in FW_LANDING_FINISHED without ever returning through that branch. Both sites now mark (the autoland site keyed off the existing fwLandState.landWp, with landState guarding the self-looping re-entry). The audit also caught that unconditional marking would regress against the command-parity work: a MAV_CMD_NAV_LAND outside a mission borrows the same states with a transient waypoint and would have credited a stale mission index — guarded in 0cc2e89 (on #11717, since that's where the command path lives). Two adjacent completion bugs surfaced while testing this and are fixed in 1262f51: the reached-latch was consumed before checking for an active port (a shared port closing on disarm right after landing destroyed the final notification), and MISSION_CURRENT reported ACTIVE after touchdown because WAYPOINT_FINISHED still maps to NAV_WP_MODE — completion now outranks activity and clears on a fresh WP-mode engagement so re-flights report correctly. Your instinct to distrust FSM-stubbed tests was right; the new unit tests drive the reporting path off the real latch fields, and a live X-Plane flight with a LAND-terminated mission is still planned — with the disclosed gap that it exercises the simple landing controller, not an approach-configured autoland.

2. CLEAR_ALL ownership — confirmed, guarded in 603288a with the same shape as the COUNT / REQUEST_LIST denials, plus unit tests for both directions (non-owner denied with the owner's transfer left usable; owner can still cancel its own). One deliberate call worth flagging: ownership includes the ingress port, matching the existing guards, so a GCS that starts a transfer on port A and fails over to port B mid-transfer is denied until the transfer times out (~9 s upload worst case, 5 s download). Kept that for consistency; happy to relax all three guards together if you'd rather.

3. Legacy MISSION_REQUEST — dead branch deleted rather than commented (603288a): downloads always answer MISSION_ITEM_INT, which is what the MISSION_REQUEST deprecation guidance calls for, so the unreachable float encoder and the two redundant useIntMessages = true reassignments are gone, with a tombstone comment explaining why legacy requests get INT replies. Upload-side float support untouched.

4. Agreed on the refactor, deferred. Restructuring mavlinkHandleMissionItemCommon mid-stack churns 6/7–7/7 for zero behaviour change, so it's a post-merge cleanup. Your causal read holds, for what it's worth — the CLEAR_ALL guard miss was exactly a sibling-copy escape.

Separately: since 4/7 merged while this was in flight, one audit finding against the reconnect commit became a small follow-up instead — #11731. It moves heartbeat presence tracking from per-port to per-peer (a steady peer no longer masks a newly joining one; port failover registers), adds a per-port rate limit on arming snapshots (a slow-heartbeat peer can't resend on every beat), and drops the enable-transition snapshot in favour of the heartbeat signal alone.