MSP-over-MAVLink tunnel - #11718
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
This #11472 PR has been split into a sequence of PR's by inav-claude for easier review and handling, but i'll include a whole laydown of what it entails as final product here.
Upstream PRs can't base on fork branches, so all 7 target What this isA ground-up rework of INAV's MAVLink implementation: from a single-port, single-file telemetry bolt-on into a modular MAVLink subsystem with:
Routing design follows ArduPilot's Developed on ArchitectureThe monolithic
Headers: built against the checked-in STorM32 dialect (337-message STorM32/ArduPilot/common superset) because it carries OlliW's mLRS-specific messages. The regen moved the merged Feature laydownMulti-port
Routing
Streams & protocol services
Telemetry detail
Missions
Command parity (MAVLink + MSPv2 + Programming Framework)
mLRS integration
High latency
MSP-over-MAVLink tunnel
SITL/simulator fixes
Design decisions worth remembering
Verification
Known limits / non-goals
|
d9a51d0 to
aa0b251
Compare
- MAV_CMD_COMPONENT_ARM_DISARM and MSP2_INAV_ARM_DISARM through the normal arming path, succeeding only when the requested state is reached - RTH via a temporary BOXNAVRTH source on the RC mode selector (activateRTHMode) instead of the failsafe/geozone forced-RTH latch; wired to MAV_CMD_NAV_RETURN_TO_LAUNCH, ArduPilot DO_SET_MODE RTL, MSP2_INAV_ACTIVATE_RTH, and Programming Framework operation 61. Cleared by a pilot flight-mode change or disarm - QGC/ArduPilot pause: DO_SET_MODE Loiter/PosHold/Brake enters normal PosHold at the current position via a temporary BOXNAVPOSHOLD source - Normal current-position LAND (transient waypoint, uploaded mission untouched) via MAV_CMD_NAV_LAND, MSP2_INAV_ACTIVATE_LANDING, PF op 62 - MAV_CMD_DO_SET_HOME through the native waypoint-0 backend - MSP2_INAV_TIMESYNC returning the MAVLink TIMESYNC boot clock - Temporary fixed-wing loiter-radius override: DO_REPOSITION.param3 (meters) or int32 loiterRadius appended to MSP2_INAV_SET_GLOBAL_TARGET (cm); volatile, cleared on disarm/reboot, only active in PosHold - SET_POSITION_TARGET_GLOBAL_INT / _LOCAL_NED guided handling - MAV_CMD_CONDITION_YAW; explicit unsupported MAV_CMD_NAV_TAKEOFF stub - GCSN OSD flight-mode element while GCS navigation is active Unit slice: 81/81 passing.
A command-triggered landing (MAV_CMD_NAV_LAND / MSP direct land) borrows NAV_STATE_WAYPOINT_RTH_LAND and the FW autoland FSM with a transient waypoint, while activeWaypointIndex still points at whatever mission item was last active. The unconditional reached-marking added for mission LAND items would emit MISSION_ITEM_REACHED for that stale index and could mark a loaded mission complete. Capture forcedLandingActivated before the existing clears and skip the marking for commanded landings at both finish sites.
MSP transport over MAVLink TUNNEL (private payload type 0x8001, MAVLink 2 only) so the Configurator can talk MSP over an existing MAVLink telemetry link. Reuses the MSP parser/encoder through narrow msp_serial seams; replies are fragmented and returned on the ingress port only; MSPv1/v2 framing symmetry is preserved end-to-end. Reboot post-processing is allowed; serial passthrough and ESC 4-way are rejected before dispatch. Malformed payload lengths are dropped and stale partial frames time out. Reply/frame buffers are file-scope, not task stack. Companion configurator branch adds the MAVLink Tunnel wireless option. Full mavlink_unittest suite: 86/86 passing. Final PR of the mavlink_multiport2 stack: tree now matches the feature branch (remaining deltas are upstream maintenance-10.x drift only).
docs/Mavlink.md documents the full MAVLink stack: multiport + routing,
datastream groups and CLI settings, identity/capabilities, mLRS
integration, supported outgoing/incoming messages and commands, mode
mappings, mission behavior and MSP parity gaps, MSP-over-MAVLink
tunnel, and high-latency mode. Updated from the development branch to
cover flight-mode-change STATUSTEXT notices and reworded for mainline.
docs/Settings.md regenerated from settings.yaml (picks up the per-port
mavlink_port{1-4}_* settings).
End-to-end live harness against a running SITL/FC MAVLink endpoint: API version, FC variant/version, build info, EEPROM write, reboot over the tunnel, and reconnect recovery after reboot.
- Document per-peer reconnect STATUSTEXT/arming re-announcement - Fix STATUSTEXT severity: arming-disable and mode notices are NOTICE only - Correct MSP-parity LAND/RTH rows: leg speed and RTH land flag are captured on upload, dropped on download - Restrict stream-rate CLI settings to port 1 (ports 2-4 have none) - Complete upload-translator list (RTH/SET_POI/SET_HEAD) and DO_REPOSITION param4 heading - Note downloads always reply MISSION_ITEM_INT - Fix dead MSP-Navigation-Messages.md link -> Navigation.md wp section
|
Went through this part's isolated diff (the MSP-over-MAVLink tunnel) — a couple of things I noticed, flagging as questions since I may be missing context:
Minor, non-blocking:
On the positive side, the passthrough/4-way and reboot gating logic checked out against |
|
One more thought on the The header (≤16 bytes) and CRC (1-2 bytes) that Something like: static void mavlinkSendTunnelMspReply(...)
{
uint8_t hdrBuf[16];
uint8_t crcBuf[2];
int hdrLen, crcLen;
// build header+crc only, no data copy - data stays where it already is
int dataLen = mspSerialBuildFrameHeader(reply, mspVersion, hdrBuf, &hdrLen, crcBuf, &crcLen);
const uint8_t *dataBuf = replyPayloadHead; // already in tunnelReplyPayloadBuf
const int totalLen = hdrLen + dataLen + crcLen;
uint8_t chunk[MAVLINK_MSG_TUNNEL_FIELD_PAYLOAD_LEN];
for (int offset = 0; offset < totalLen; offset += sizeof(chunk)) {
int chunkLen = MIN((int)sizeof(chunk), totalLen - offset);
int written = 0;
// each call copies whatever part of [offset, offset+chunkLen) falls
// within that segment, returns bytes written, 0 if no overlap -
// a chunk boundary can straddle header/data or data/crc
written += copyFromSegment(chunk + written, hdrBuf, hdrLen, offset, chunkLen - written);
written += copyFromSegment(chunk + written, dataBuf, dataLen, offset - hdrLen, chunkLen - written);
written += copyFromSegment(chunk + written, crcBuf, crcLen, offset - hdrLen - dataLen, chunkLen - written);
mavlinkSendTunnelReply(targetSystem, targetComponent, chunk, chunkLen);
}
}That would drop |
|
Implemented this along the lines you suggested. The reply payload still lives in
While doing that, I also:
The changes are now on the actual This removes roughly 4.1 KB of redundant static RAM by eliminating The remaining issue is I tried making that buffer local to command processing, but a 4 KB stack allocation is not safe on these targets and caused widespread build/link failures, so that attempt was reverted. I also removed the temporary target-specific disable because silently disabling the feature on one board is not a proper solution. At this point the redundant frame allocation is solved, but I do not have a sound answer for the remaining reply workspace. It appears to require either more selective feature gating, reuse of an existing large buffer with safe ownership, or a more substantial change to how large MSP replies are produced. Guidance on which direction is acceptable would be appreciated. |
|
I have a theory and wonder if you agree: MSP_PORT_OUTBUF_SIZE is 4,112 on FLASHFS targets only so MSP_DATAFLASH_READ can read a full flash page at a time, which is faster than reading perhaps 256 or 512 bytes at a time. So making the cost of making the buffer smaller, say 512 bytes, would be slower reads of the blackbox. But that 4096-byte data is then broken into 128-byte chunks and sent over a slow radio link anyway. So it's not going to be fast anyway. Therefore there would be effectively no cost to using a smaller buffer? It's entirely possible that I am missing something here. Please let me know if you see something I am not seeing. Looking at the other largest messages that I found: MSP2_INAV_SERVO_MIXER = 432 bytes. mode ranges 160B Non flashfs targets use 512 bytes, right? So 512 should work? |
|
I made a draft PR against your branch. It's largely AI-generated and only lightly tested, but it should be a good start. |
tunnelReplyPayloadBuf was sized off MSP_PORT_OUTBUF_SIZE (4112 bytes under USE_FLASHFS), which exists to let MSP_DATAFLASH_READ return a full 4096-byte flash page in one shot for fast blackbox downloads over USB/serial. The tunnel doesn't need that: serializeDataflashReadReply() already clamps its read to whatever destination buffer space is available, so a smaller buffer just costs more round-trips, never an overflow. Every tunnel reply is also fragmented into 128-byte TUNNEL payload chunks regardless of source buffer size, so the large contiguous staging buffer bought the tunnel nothing. Add a dedicated MAVLINK_TUNNEL_MSP_REPLY_BUF_SIZE (512 bytes) instead of reusing MSP_PORT_OUTBUF_SIZE. This matches the non-FLASHFS default already in production on every such target, and comfortably covers the largest ordinary MSP2 reply found (MSP2_INAV_SERVO_MIXER, 432 bytes). Frees ~3.6KB and resolves the RAM overflow reported on BROTHERHOBBYF405V3 (FLASHFS + tunnel): build now links with 3,364 bytes of RAM free instead of overflowing by ~236 bytes.
MSP2_INAV_SERVO_MIXER (432 bytes) was the largest reply found when the audit was scoped to MSP2 handlers only. Widening to MSP1 handlers found MSP_LED_STRIP_CONFIG at exactly 512 bytes (128 LEDs x 4 bytes, unconditional loop) -- the true largest reply, with no margin to spare. Not a new risk (every non-FLASHFS board already ships this exact buffer size), but the comment should say so accurately.
bytesRemainingInBuf was captured before the 4-byte address write, then used to clamp readLen against that same figure -- so a request with size >= bytesRemainingInBuf writes address(4) + readLen(up to bytesRemainingInBuf) = 4 bytes past the end of dst. sbufWriteU32() and flashfsReadAbs() write straight through dst->ptr with no bounds checking, so this silently corrupts whatever follows the destination buffer in memory. On the plain serial MSP path this was effectively unreachable: a client requesting the standard full 4096-byte flash page never hits the clamp, since MSP_PORT_OUTBUF_SIZE (4112 under USE_FLASHFS) has 16 bytes of incidental headroom. The MSP-over-MAVLink tunnel's smaller, dedicated reply buffer (512 bytes, no such headroom) makes it trivially reachable: any tunneled MSP_DATAFLASH_READ request for >= 509 bytes -- a normal way to page through a blackbox log -- now overflows. Fix: subtract the address size from the available-space figure before clamping, so readLen + address size never exceeds what's actually in the buffer.
MAVLINK_TUNNEL_MSP_REPLY_BUF_SIZE was set to 512 on the reasoning that it matches the non-FLASHFS MSP_PORT_OUTBUF_SIZE already shipping on every such board, with MSP_LED_STRIP_CONFIG (128 LEDs x 4 bytes) as the largest reply that fits. MSP2_INAV_LED_STRIP_CONFIG_EX is larger. It writes LED_MAX_STRIP_LENGTH (128) x sizeof(ledConfig_t), and ledConfig_t is a packed 40-bit bitfield, so that is 5 bytes per LED, not 4: 640 bytes. It writes via sbufWriteDataSafe(), so a 512-byte buffer does not overflow, but it truncates the reply to 510 bytes and reports no error. The Configurator derives the LED count from the payload length (ledCount = data.byteLength / 5), so its LED Strip tab would load 102 of 128 LEDs silently. BROTHERHOBBYF405V3 -- the FLASHFS target that motivated the smaller buffer -- has USE_LED_STRIP, and the tunnel is enabled on every MAVLink target by default, so this reply is reachable there. Use 768 instead. Every other reply measured against SITL is 432 bytes or below (MSP2_INAV_SERVO_MIXER 432, MSP_OSD_CONFIG 356, MSP2_COMMON_PG_LIST 294, MSP_SERVO_MIX_RULES 288), so 768 leaves 128 bytes of headroom over the largest one. Cost on BROTHERHOBBYF405V3 is 256 bytes against the 512-byte version: RAM 127708 -> 127964 of 131072, still 3108 bytes free. Against the original MSP_PORT_OUTBUF_SIZE sizing this still frees ~3.3KB.
|
Test firmware build ready — commit Download firmware for PR #11718 244 targets built. Find your board's
|
Stub only -- captures the buffer-decoupling and shrink-audit technique from the MSP tunnel reply buffer fix before it's lost, ahead of the tracked document-ram-flash-optimization-practices project writing the full guide and linking it from Development.md.
Surfaces RAM/flash regressions and creep on every PR instead of only via a hard CI failure (motivated by PR #11718's terrain-cache RAM overflow, which was only caught because CI happened to fail on that specific board). Extracts flash/RAM usage from each build's .elf right after compiling (no second build of the base branch), persists a baseline per branch as a release asset in the pr-test-builds companion repo, and diffs 4 representative targets (one per MCU family) against it on PR builds.
2 participants
Part 7/7 of the mavlink_multiport2 stack — final part; the tree now carries the complete feature set.
MSP-over-MAVLink tunnel
MSP transport over the MAVLink Tunnel service (private payload type
0x8001, MAVLink 2 only), so the INAV Configurator can talk MSP over an existing MAVLink telemetry link — typically a radio that exposes no separate MSP transport. The companion configurator PR adds a "MAVLink Tunnel" option under Wireless mode with SYSID selection for multi-vehicle networks.msp_serialseams (byte parser, encode-to-buffer, command processing) — no duplicate MSP framing logic.TUNNELmessages and returned on the ingress port only;target_component = 0is accepted for local delivery but never fanned out to other ports.MSP_REBOOTpost-processing is supported over the tunnel; serial passthrough and ESC 4-way passthrough are rejected before dispatch.Documentation
This PR also lands the complete user-facing
docs/Mavlink.md(multiport/routing, datastream groups and CLI settings, mLRS integration, supported messages/commands, mode mappings, mission behavior and MSP parity gaps, tunnel, high-latency mode) and regeneratesdocs/Settings.mdfor the per-portmavlink_port{1-4}_*settings.Testing
mavlink_unittestsuite: 86/86 passing (tunnel cases: malformed-length rejection, pre-dispatch passthrough rejection, reboot ingress-port handling, stale-frame reset, multi-message reply fragmentation).src/test/mavlink/tunnel/) covering that same sequence against a running SITL/FC.Companion PRs:
inav-configurator:mavlink_multiport2(tunnel UI + wrapping),mspapi2:mavlink_multiport2.