MAVLink tunnel: resume MSP replies that do not fit the TX buffer

[b14ckyy]

Bug

Found while writing a GCS client for the MSP-over-MAVLink tunnel (#11718) and reproduced on a Matek F765-WING with 10.0.0-RC1 over a physical UART at 115200: MSP_BOXNAMES never completes, only two of the expected chunks arrive, every retry fails the same way. Small replies work, and so does a 640-byte all-zero reply (MAVLink2 trims trailing zeros).

mavlinkSendTunnelMspReply() writes all TUNNEL chunks of a reply back to back, and mavlinkSendMessage() drops any frame that does not fit the port's free TX space. Hardware UART TX rings are 256 bytes and a full chunk is 145 bytes on the wire, so the second chunk of every reply above roughly 221 framed bytes is dropped deterministically. USB VCP (4 KB CDC buffer) and SITL (65535 free) never show it, and the unit-test stub reported 1024 free bytes, so the existing fragmentation tests passed.

Fix

Resumable, non-blocking send:

• The encoded reply and a resume offset are kept as one pending state together with the ingress port and target ids. The reply payload buffer was already shared, so one reply at a time is the natural model.
• A new single-port send checks serialTxBytesFree() against the encoded length before writing, and neither drops nor consumes a sequence number when the frame does not fit. mavlinkSendMessage() itself is unchanged in behaviour (the encode step is shared).
• Remaining chunks are flushed at the start of each telemetry cycle for that port, before RX and before the periodic stream, so the reply gets TX space first and finishes over the following cycles. The flush respects the half-duplex MAVLink RX backoff like the stream does.
• A request that arrives while a reply is still pending is dropped without touching the shared buffer; clients wait for the complete reply before the next request, so this only affects pipelining clients or a second port. A reply that makes no progress for one second is abandoned, so a port whose TX never drains cannot lock the tunnel. The pending state is cleared on port re-init.
• MSP_REBOOT: a reply left pending by a full ring is flushed before the reboot (it was lost before as well).

No settings, no PG change. docs/Mavlink.md gains one sentence describing the observable behaviour.

Verification

• Hardware: same board and UART as above, 560-byte reply now arrives in 5 chunks in 106 ms, 0 timeouts over 113 requests.
• Unit tests: the serial stub gets a settable TX budget that drains per write, so a 255-byte ring can be simulated. Seven tunnel tests cover the resume across cycles, a held-back chunk, busy drops on the same and on a second half-duplex port, the stall timeout, port re-init and the reboot flush. Against the old code five of them fail. Full suite: 586/586.
• Sizes vs 3d2c8fdcd: MATEKF765 flash +512 B, RAM +48 B; MATEKH743 flash +508 B; MATEKF405SE flash +576 B, RAM +48 B. 512 KB targets have no MAVLink and are unaffected.

Known limits, unchanged from before: the periodic stream can still use TX space a chunk did not fit into, which delays but never corrupts a reply; radio flow control (txbuffFree) is not considered by the tunnel.

🤖 Generated with Claude Code

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Resume MSP tunnel replies across constrained MAVLink TX cycles

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Resumes fragmented MSP-over-MAVLink replies across telemetry cycles when UART TX space is limited.
• Preserves half-duplex behavior and clears stalled, reinitialized, or reboot-bound pending replies.
• Adds constrained-buffer, multi-port, timeout, lifecycle, and reboot coverage plus client guidance.

Diagram

graph TD
  Client["GCS Client"] --> Handler["MSP Handler"] --> State["Pending Reply"] --> Room{"Frame fits?"} -->|Yes| Tx["Ingress TX"]
  Cycle["Telemetry Cycle"] --> State
  Room -->|No| State
  Tx -->|More chunks| State
  State -->|One second stalled| Clear["Clear State"]

Loading

High-Level Assessment

The resumable single-reply state is the best fit for the existing shared payload buffer and non-blocking embedded telemetry loop. Blocking until TX drains would disrupt flight-controller scheduling, while enlarging UART rings would consume target-specific RAM without addressing arbitrary congestion; both were appropriately avoided.

Files changed (8) +454 / -64

Bug fix (6) +150 / -48

fc_mavlink.cPersist and resume fragmented MSP tunnel replies +76/-24

Persist and resume fragmented MSP tunnel replies

• Replaces immediate back-to-back chunk transmission with shared pending-reply state and capacity-aware flushing. Drops overlapping requests, expires stalled replies, and flushes pending reboot acknowledgements before post-processing.

src/main/fc/fc_mavlink.c

fc_mavlink.hExpose pending tunnel reply flushing +3/-0

Expose pending tunnel reply flushing

• Declares the tunnel reply flush entry point for telemetry runtime integration when MSP tunneling is enabled.

src/main/fc/fc_mavlink.h

mavlink_internal.hAdd shared pending tunnel reply state +11/-0

Add shared pending tunnel reply state

• Adds state for the encoded MSP frame, resume offset, ingress port, destination identities, and last-progress timestamp. Stores this state in the global MAVLink context.

src/main/mavlink/mavlink_internal.h

mavlink_ports.cClear pending replies during port reset +3/-0

Clear pending replies during port reset

• Discards pending tunnel state when its originating MAVLink port is reinitialized, preventing stale frames from being resumed on a new port runtime.

src/main/mavlink/mavlink_ports.c

mavlink_runtime.cAdd capacity-aware single-port sending and cycle retries +56/-24

Add capacity-aware single-port sending and cycle retries

• Extracts per-port message encoding and adds a send path that checks TX capacity before writing or consuming a sequence number. Flushes pending tunnel chunks before RX and periodic telemetry while respecting half-duplex backoff.

src/main/mavlink/mavlink_runtime.c

mavlink_runtime.hDeclare capacity-aware MAVLink send API +1/-0

Declare capacity-aware MAVLink send API

• Exposes the single-port conditional send function used by resumable tunnel reply transmission.

src/main/mavlink/mavlink_runtime.h

Tests (1) +303 / -16

mavlink_unittest.ccCover constrained-buffer tunnel reply lifecycle +303/-16

Cover constrained-buffer tunnel reply lifecycle

• Extends serial stubs with configurable TX budgets and a second MAVLink port. Adds tests for cross-cycle resumption, insufficient space, busy requests, timeout abandonment, port reinitialization, half-duplex multi-port isolation, sequence continuity, and reboot flushing.

src/test/unit/mavlink_unittest.cc

Documentation (1) +1 / -0

Mavlink.mdDocument resumable tunnel replies and serialization rules +1/-0

Document resumable tunnel replies and serialization rules

• Explains that oversized replies continue across telemetry cycles, only one request is served at a time, and stalled replies expire after one second.

docs/Mavlink.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Half-duplex replies can collide with requests ✓ Resolved 🐞 Bug ☼ Reliability

Description

mavlinkTunnelMspReplyIsBusy() directly calls mavlinkFlushTunnelMspReply(ingressPortIndex)
instead of only reporting pending state, bypassing the half-duplex backoff enforced by the telemetry
loop. When a second complete tunnel request arrives on the same port while a prior reply is pending,
receive processing has just updated lastRxFrameUs, yet dispatch can immediately transmit an old
reply chunk during the protected guard interval.

Code

src/main/fc/fc_mavlink.c[R94-95]

+    // Flushes only the ingress port; writing another port here would bypass its half-duplex backoff.
+    mavlinkFlushTunnelMspReply(ingressPortIndex);

Evidence

Every received byte updates lastRxFrameUs, and isMAVLinkTelemetryHalfDuplexBackoff() uses the
elapsed time since that timestamp to suppress tunnel and periodic transmission during
TELEMETRY_MAVLINK_DELAY. The telemetry runtime calls the reply flush only when this predicate is
false, but a completed tunnel command reaches mavlinkTunnelMspReplyIsBusy() during dispatch, whose
unconditional direct flush bypasses the same guard on the ingress port.

src/main/fc/fc_mavlink.c[92-96]
src/main/mavlink/mavlink_runtime.c[229-237]
src/main/mavlink/mavlink_runtime.c[295-305]
src/main/mavlink/mavlink_runtime.c[322-326]
src/main/mavlink/mavlink_runtime.c[231-234]
src/main/mavlink/mavlink_runtime.c[295-304]
src/main/mavlink/mavlink_runtime.c[322-337]
src/main/fc/fc_mavlink.c[136-139]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`mavlinkTunnelMspReplyIsBusy()` flushes a pending reply while processing an incoming request, bypassing the half-duplex RX guard applied by the telemetry runtime. A pipelined request received on the pending reply's ingress port can therefore trigger transmission during the protected interval instead of being safely dropped.
## Fix Focus Areas
- src/main/fc/fc_mavlink.c[92-96]
- src/main/mavlink/mavlink_runtime.c[295-305]
- src/main/mavlink/mavlink_runtime.c[322-337]
## Recommended Fix
Make `mavlinkTunnelMspReplyIsBusy()` non-transmitting so it only reports whether a reply remains pending, and leave transmission to `mavlinkRuntimeHandle()`, whose telemetry-cycle flush already enforces half-duplex backoff. The incoming pipelined request should then be dropped as documented. Add a same-port half-duplex test proving that a pipelined request neither writes pending reply chunks nor executes until the backoff expires.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[github-actions]

RAM / Flash usage vs. base commit 3d2c8fd — commit 8005a05

Target	Flash Δ	RAM Δ
MATEKF405	+464 B (+0.06%)	CCM: ±0 B (±0.00%) RAM: +48 B (+0.04%)
MATEKF722	±0 B (±0.00%)	ITCM_RAM: ±0 B (±0.00%) RAM: ±0 B (±0.00%) TCM: ±0 B (±0.00%)
MATEKF765	+496 B (+0.07%)	DTCM_RAM: ±0 B (±0.00%) SRAM1: +48 B (+0.04%)
MATEKH743	+476 B (+0.06%)	D2_RAM: ±0 B (±0.00%) DTCM_RAM: ±0 B (±0.00%) ITCM_RAM: -72 B (-0.44%) RAM: +36 B (+0.02%)

See RAM/flash optimization guide for techniques to reduce usage.

[github-actions]

Test firmware build ready — commit 8005a05

Download firmware for PR #12036

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

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

[b14ckyy]

@xznhj8129 If you have some time, I would appreciate if you have a look and tell me if you have concerns before we merge. Fix is verified on my end and 560B message goes through no problem. very nice feature.