Skip to content

MAVLink: support mavlink 2.1 32 bit system IDs - #33734

Open
tridge wants to merge 21 commits into
ArduPilot:masterfrom
tridge:pr-mavlink32bitsysid
Open

MAVLink: support mavlink 2.1 32 bit system IDs#33734
tridge wants to merge 21 commits into
ArduPilot:masterfrom
tridge:pr-mavlink32bitsysid

Conversation

@tridge

@tridge tridge commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements 32 bit system IDs

See also:

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Description

This implements mavlink/rfcs#20 but with 32 bit system IDs to allow for IPv4 addresses to be used for system IDs. This should make large drone light shows easier.
I also plan on making full 32 bit integers work with mavlink parameters to make this more practical for real IPv4 addresses. That will be a separate effort. For now this PR is really 24 bit sysid support, but once we get the mavlink param change in it will work with 32 bit

Some open questions:

  • should we put target_system&0xff in the uint8_t target_system field in messages or should we put in 0? This PR uses 0
  • the lua script changes are tricky as the mavlink receiver gets the raw C structure which is now 3 bytes larger. Should we add compatibility code so the new lua script handles both forms? UPDATE: I've added compat code

Testing done both in SITL and on a fixed wing (bixler) at Spring Valley

Note on MAVLink peripherals

Setting MAV_SYSID above 255 means all packets are incompatible with older mavlink2 implementations. That means all packets to older peripherals will be dropped. We could add pre-arm checks in the future if this turns out to be an issue.

Comment thread libraries/AP_Follow/AP_Follow.cpp Outdated
@tridge
tridge force-pushed the pr-mavlink32bitsysid branch 2 times, most recently from 8c631c4 to b45972c Compare July 18, 2026 07:57
@tridge tridge removed the DevCallEU label Jul 22, 2026
tridge added 12 commits July 28, 2026 07:29
the component was compared against last_src_system, so chunked
statustexts from different components on the same system were joined
together.
Widen system ID handling throughout for MAVLink2.1 32 bit system IDs
(mavlink/rfcs#20):

 - MAV_SYSID, MAV_GCS_SYSID and MAV_GCS_SYSID_HI become AP_Int32 with
   parameter width conversion. Values must stay below 2^24 until the
   parameter transport moves away from float32
 - routing table entries, get_targets() and find_by_mavtype use the
   mavlink_msg_get_target_sysid/compid helpers so targets in the
   extended header are honoured
 - packet_overhead_chan() conservatively reserves the extended header
 - TSYN and MAVC log the full 32 bit source and target system IDs
Widen gimbal and target sysid storage and pass 32 bit targets via the
extended header aware send path. MAV_CMD_DO_GIMBAL_MANAGER_CONFIGURE
accepts a controller sysid up to 2^24-1, the largest a float command
parameter can carry exactly.
Widen source/target system IDs in the MAVC log message and the MAVLink
log transfer target. Write_Command() takes the target from the caller,
as the packet's target_system byte is zero when the target travelled in
the extended header.
FOLL_SYSID becomes AP_Int32 with parameter width conversion. Values
must stay below 2^24 until the parameter transport moves away from
float32. The conversion runs from the vehicle's load_parameters() as
mode entry at startup can read the parameter.
Widen the CAN forwarding requester system ID so replies to a 32 bit
GCS are not aliased.
tridge added 9 commits July 28, 2026 07:29
Widen the recorded Loweheiser source sysid so the source-match guard
and the commands we send back are not truncated. The LOEC log SI field
widens to uint32 to match.
The basic ID packet is sent as a raw struct, so there is no extended
header available for a target sysid over 255; clamp to zero as we do
elsewhere rather than silently truncating.
The ADSB, SoloGimbal, MAVLink gimbal v2 and camera v2 simulators
truncated the vehicle sysid they learn from its heartbeat, matched
commands against the payload target byte (which is zero when the
target travels in the extended header), and addressed replies through
payload structs. The simulated heartbeats are also serialised properly
rather than writing the mavlink_message_t memory as wire bytes, which
is no longer the wire layout now that sysid is 32 bit.
Writing len bytes from &msg.magic assumed the mavlink_message_t header
matched the wire format. With a 32 bit sysid the struct header is 3
bytes longer than a small-sysid wire header, so the frame was shifted
and lost its last 3 bytes, including the CRC.
Reboots with MAV_SYSID 100000, checks the SYSID32 extended header is
used, fetches parameters from a source system over 255, and runs
mission upload and arming checks at the new sysid.
Accept 32 bit sysids in MAV_CMD_DO_FOLLOW and convert the Follow
parameters at load time.
Convert the Follow parameters at load time, before mode entry can read
FOLL_SYSID.
@tridge

tridge commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

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

Review verdict: COMMENT

A large but carefully staged change (one commit per subsystem, submodule bump isolated, parameter conversions complete). The blocking item is external: the modules/mavlink pointer must land upstream first. The 24-bit parameter range and the Lua size-sniffing shim are the two things worth tightening before merge.

Findings

  • ISSUEBlocked on an unmerged upstream mavlink commit — this is what the red check actually says. The job is named check-merge-commits but runs check_branch_conventions.py, and every check passes except one: 8f8c578ed394 references modules/mavlink@5ad0e9105606: exists in ArduPilot/mavlink but is not yet merged to master (it is ahead of master). Notably "No merge commits" and "All submodule updates are isolated in their own commits" both pass. So this is the expected coordination gate on ArduPilot/mavlink#515, not a defect in the branch — but it does mean the PR cannot merge until that lands.
  • ISSUEThe settable range is 24-bit, but the stated motivation needs 32. The widened parameters are capped at 16777215 (224−1) — three get @Range: 1 16777215 and two @Range: 0 16777215 — presumably because PARAM_VALUE carries parameters as a float and only 24 bits of mantissa survive the round trip. The PR body already flags full 32-bit parameter transport as separate future work — but since the headline use case is putting an IPv4 address in the system ID, and IPv4 needs all 32 bits, the parameter documentation should say outright that the settable range is currently 24-bit. As it stands a user reading "32 bit system IDs" will try to set an address above 16.7M and have it rejected with no explanation. GCS.cpp:41
  • ISSUEMount's _SYSID_DFLT is widened with no @Range at all. The other five widened parameters all get an explicit 16777215 ceiling to stay inside float-exact territory, but AP_Mount_Params::sysid_default goes AP_Int8AP_Int32 while its metadata block is left untouched — and it has no @Range on master either. Widening it therefore removes even the implicit 0–255 bound the AP_Int8 type gave it, leaving a 32-bit parameter with no documented limit that a GCS or parameter-checking tool could enforce. It should get the same @Range: 0 16777215 as its siblings. AP_Mount_Params.h:60
  • ISSUEThe Lua compatibility shim sniffs firmware layout by struct size, with hardcoded offsets. mavlink_msgs.sysid32_layout() decides between the old and new mavlink_message_t layouts with #message >= 299, then reads the extended target at fixed offsets 295 and 299. That works today, but it silently couples user-facing Lua to the exact byte size of a C struct: any future change to mavlink_message_t flips the heuristic with no error, and the failure mode is misparsed messages rather than a clean break. At minimum the three constants deserve a comment naming the struct fields they correspond to so a future mavlink bump has a chance of catching them. mavlink_msgs.lua:30
  • NOTE — Parameter widening is handled completely — verified 1:1. This was the main thing I expected to find broken, since AP_Param matches stored entries on type == phdr.type (AP_Param.cpp:495,528), so a bare AP_Int16AP_Int32 change would silently reset every user's value to the default. All six widened parameters have a matching convert_parameter_width() call: GCS::sysid, mav_gcs_sysid, mav_gcs_sysid_high (in GCS::init), AntennaTracker g.sysid_target (in AntennaTracker/system.cpp) and AP_Follow::_sysid (in AP_Follow::convert_params) — all from AP_PARAM_INT16 — plus AP_Mount sysid_default from AP_PARAM_INT8 (in AP_Mount::init). Nothing is missed. GCS.cpp:1193
  • NOTE — send_message_target() documents an unenforced caller contract. Its comment requires the caller to have already written the payload's target_system byte as (target_sysid > 255 ? 0 : target_sysid). Nothing checks this, and getting it wrong produces a packet that is subtly mis-addressed rather than rejected. Since the helper already has target_sysid in hand, consider having it write that byte itself. GCS.h:520
  • NOTE — Routing correctly handles the targetted-without-msg-entry case. get_targets() now proceeds when MAVLINK_IFLAG_TARGETTED is set even if msg_entry == nullptr, which is necessary for forwarding messages this build does not know — the old code bailed out. Widening the sentinel-carrying locals to int64_t (so a full uint32_t plus the -1 "no target" sentinel both fit) is the right call, and the %lld/(long long) debug cast matches. MAVLink_routing.cpp:60
  • NOTE — 82 checks still pending. Only 2 had reported at review time, so the build/autotest surface of a 68-file change touching every vehicle is essentially unverified so far. Worth a re-check once the run completes.

Generated from an automated review of all PRs labelled DevCallTopic on 2026-07-28. Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants