implement 32 bit system IDs - #1229
Conversation
9e2ef3a to
0ede65f
Compare
| buf[ofs++] = (target_sysid >> 16) & 0xFF; | ||
| buf[ofs++] = (target_sysid >> 24) & 0xFF; | ||
| buf[ofs++] = target_compid; | ||
| } |
There was a problem hiding this comment.
Does MAVLINK_IFLAG_TARGETTED require MAVLINK_IFLAG_SYSID32?
| } | |
| if (incompat_flags & MAVLINK_IFLAG_TARGETTED) { | |
| buf[ofs++] = target_sysid & 0xFF; | |
| if (incompat_flags & MAVLINK_IFLAG_SYSID32) | |
| buf[ofs++] = (target_sysid >> 8) & 0xFF; | |
| buf[ofs++] = (target_sysid >> 16) & 0xFF; | |
| buf[ofs++] = (target_sysid >> 24) & 0xFF; | |
| } | |
| buf[ofs++] = target_compid; | |
| } |
There was a problem hiding this comment.
I'm not sure. It almost looks like the two flags are independent which has the benefit of not always burning the 3 bytes in the header.
| if (target_sysid > 255) { | ||
| msg->incompat_flags |= MAVLINK_IFLAG_TARGETTED; | ||
| msg->target_sysid = target_sysid; | ||
| msg->target_compid = target_compid; | ||
| } |
There was a problem hiding this comment.
| if (target_sysid > 255) { | |
| msg->incompat_flags |= MAVLINK_IFLAG_TARGETTED; | |
| msg->target_sysid = target_sysid; | |
| msg->target_compid = target_compid; | |
| } | |
| if (target_sysid > 0) { | |
| msg->incompat_flags |= MAVLINK_IFLAG_TARGETTED; | |
| msg->target_sysid = target_sysid; | |
| msg->target_compid = target_compid; | |
| if (target_sysid > 255) { | |
| msg->incompat_flags |= MAVLINK_IFLAG_SYSID32; | |
| } | |
| } |
There was a problem hiding this comment.
I think the confusion is in the name MAVLINK_IFLAG_TARGETTED. It should be called MAVLINK_IFLAG_TARGETTED_SYSID32. And then the original implementation is correct and your suggestion is wrong. A target_sysid > 0 is perfectly valid with the existing mavlink implementation and doesn't need a incompat flag.
| buf[0] = magic; | ||
| buf[1] = len; | ||
| if (magic == MAVLINK_STX_MAVLINK1) { | ||
| if (sysid > 255 || target_sysid > 255) { |
There was a problem hiding this comment.
Elsewhere, equivalent checks also check if msgid > 255.
There was a problem hiding this comment.
Agreed, it doesn't make sense to send a corrupt message over Mavlink1.
| /* | ||
| compat_flags bits | ||
| */ | ||
| #define MAVLINK_CFLAG_SYSID32 0x01 // system is capable of understanding 32 bit system ID |
There was a problem hiding this comment.
Would this be better as a new flag in MAV_PROTOCOL_CAPABILITY_MAVLINK2?
There was a problem hiding this comment.
MAV_PROTOCOL_CAPABILITY flags are a level higher. They document what Mavlink services/features are supported but they don't describe the protocol bytes changed here. And MAV_PROTOCOL_CAPABILITY flags are only received, once mavlink implementations are actually working/talking.
julianoes
left a comment
julianoes
left a comment
There was a problem hiding this comment.
- Should sysid32 be behind a #define, so that we can keep updating implementations that don't support sysid32, or should this be rolled out when updating universally and potentially break builds? Nevermind, seeing all the functions such as
_pack(...)I suppose it would add a lot of duplication and be very messy. - Is there any benefit of allowing 32 bit compids as well? I think I like the idea of the symmetry but I'm not sure it would be the right thing to do.
- Can we do a clean format PR before this. I see spaces and tabs added in this PR and I just find that a bit yucky.
I think I'm actually somewhat convinced that this could work.
| buf[0] = magic; | ||
| buf[1] = len; | ||
| if (magic == MAVLINK_STX_MAVLINK1) { | ||
| if (sysid > 255 || target_sysid > 255) { |
There was a problem hiding this comment.
Agreed, it doesn't make sense to send a corrupt message over Mavlink1.
| buf[5] = msgid & 0xFF; | ||
| return MAVLINK_CORE_HEADER_MAVLINK1_LEN+1; | ||
| } | ||
| uint8_t ofs = 2; |
There was a problem hiding this comment.
You could do uint8_t ofs = 0 at the very top and use it throughout.
| buf[ofs++] = (target_sysid >> 16) & 0xFF; | ||
| buf[ofs++] = (target_sysid >> 24) & 0xFF; | ||
| buf[ofs++] = target_compid; | ||
| } |
There was a problem hiding this comment.
I'm not sure. It almost looks like the two flags are independent which has the benefit of not always burning the 3 bytes in the header.
| /* | ||
| compat_flags bits | ||
| */ | ||
| #define MAVLINK_CFLAG_SYSID32 0x01 // system is capable of understanding 32 bit system ID |
There was a problem hiding this comment.
MAV_PROTOCOL_CAPABILITY flags are a level higher. They document what Mavlink services/features are supported but they don't describe the protocol bytes changed here. And MAV_PROTOCOL_CAPABILITY flags are only received, once mavlink implementations are actually working/talking.
| #define MAVLINK_IFLAG_MASK 0x01 // mask of all understood bits | ||
| #define MAVLINK_IFLAG_SIGNED 0x01 // this message has a MAVLink2 signature attached | ||
| #define MAVLINK_IFLAG_SYSID32 0x02 // this message uses a 32 bit system ID | ||
| #define MAVLINK_IFLAG_TARGETTED 0x04 // this message has target sysid/compid in an extended header |
There was a problem hiding this comment.
I'd make it clear that this is related to sysid 32. Just "targetted" could also mean compid, etc.
| #define MAVLINK_IFLAG_TARGETTED 0x04 // this message has target sysid/compid in an extended header | |
| #define MAVLINK_IFLAG_TARGETTED_SYSID32 0x04 // this message has target sysid/compid in an extended header |
| if (target_sysid > 255) { | ||
| msg->incompat_flags |= MAVLINK_IFLAG_TARGETTED; | ||
| msg->target_sysid = target_sysid; | ||
| msg->target_compid = target_compid; | ||
| } |
There was a problem hiding this comment.
I think the confusion is in the name MAVLINK_IFLAG_TARGETTED. It should be called MAVLINK_IFLAG_TARGETTED_SYSID32. And then the original implementation is correct and your suggestion is wrong. A target_sysid > 0 is perfectly valid with the existing mavlink implementation and doesn't need a incompat flag.
| MAVLINK_PARSE_STATE_SIGNATURE_WAIT, | ||
| MAVLINK_PARSE_STATE_SIGNATURE_WAIT_BAD_CRC | ||
| MAVLINK_PARSE_STATE_SIGNATURE_WAIT_BAD_CRC, | ||
| // new states appended to keep existing state numbering stable |
There was a problem hiding this comment.
Does that have an actual benefit? I suppose if implementations check against magic numbers?
|
I've spent some tokens on an Opus 5 review. See details below. @tridge let me know if you want to tackle these or whether you want to defer them to future PRs. Edit: and I'm not trying to slow you down or block you, just want to make sure we don't end up breaking too much. I'm eager to add this to PX4 and MAVSDK. DetailsReview:
|
Implements the MAVLink2.1 extended headers from mavlink/rfcs#20, amended to a 32 bit system ID so IPv4 addresses can be used directly as system IDs: - MAVLINK_IFLAG_SYSID32: set only when the sender sysid is over 255, the header sysid field widens in place from 1 to 4 bytes - MAVLINK_IFLAG_TARGETTED: set only when the target sysid is over 255, target_sysid[4] and target_compid[1] are appended to the header and the payload target byte is zeroed - MAVLINK_CFLAG_SYSID32 is always set in compat_flags to advertise capability. Old parsers ignore compat_flags, so frames with small IDs remain compatible with existing MAVLink2.0 parsers The wire header now varies in length (10 to 18 bytes), so header serialisation is centralised in mavlink_header_to_send_buffer() and used by finalize, mavlink_msg_to_send_buffer, _mavlink_resend_uart and signature check. The signature sha256 previously hashed the message struct as if it were the wire header; it now hashes the re-serialised header, which is also required by the wider struct sysid field. New finalize/send entry points carry an explicit target so the convenience send path (which never builds a mavlink_message_t) can emit the extended header. MAVLink1 output with a 32 bit ID is an error, never a truncation, as a masked sysid would alias another system and a zeroed target byte would become a broadcast. mavlink_msg_get_target_sysid()/mavlink_msg_get_target_compid() give routers a single call for the effective target of any message. On receive TARGETTED is honoured for any message, including ones with no payload target fields, for forward compatibility with full-RFC senders.
Tag target_system/target_component fields in mavparse (keyed on the routing semantics, so MANUAL_CONTROL.target is included) and use the tags in the C generator: - pack/send/encode system_id and target_system arguments widen to uint32_t. The payload struct stays wire-layout so its target byte is written as zero when the target is over 255 and the target is passed through to the _target finalize/send entry points - per-message get_target_system()/get_target_component() return the extended header target when MAVLINK_IFLAG_TARGETTED is set - decode() reads the raw payload byte for target fields so the wire struct is bit-exact in both the aligned and byte-swap builds; code needing the full 32 bit target must use the getters MAVLink1 generation is unchanged.
Round trips for all four sysid/target width combinations, golden legacy-compat check (frames with small IDs differ from MAVLink2.0 output only in compat_flags and CRC, and compat_flags 0 frames still parse), convenience-send and resend_uart byte-exact checks, signing over the variable-length header including corruption cases, MAVLink1 truncation guards and parser recovery from a truncated extended header.
Mirrors the C implementation: SYSID32 set only when srcSystem is over 255, TARGETTED only when a message's target_system field is over 255 (the payload byte is then zeroed), MAVLINK_CFLAG_SYSID32 always advertised, variable header length handled in framing and decode, and MAVLink1 packing raises MAVError for 32 bit IDs rather than truncating. On decode the extended header target is overlaid onto the decoded message fields, so existing code reading msg.target_system keeps working for targets over 255. New get_target_system() and get_target_component() return the effective target of any message. This also fixes get_payload() for MAVLink2: the payload slice previously started at offset 6 (the MAVLink1 header length) and so included 4 header bytes.
Round trips for all sysid/target width combinations, signing over extended headers including corruption cases, MAVLink1 guards, legacy compat framing, and golden frames produced by the C implementation to pin C/Python cross-language wire compatibility.
The CS and JavaScript runtimes read a fixed MAVLink2 header length and would consume the wrong byte count on MAVLink2.1 extended-header frames, mis-framing the stream instead of cleanly dropping them. Drop frames with incompatible flags the parser does not understand, and consume the extended header bytes so the stream stays in sync. The Java parser already rejects unknown incompat flags.
The Lua module parses the in-memory mavlink_message_t structure, not the wire format. The sysid field widened to 32 bits, moving the compid, msgid and payload offsets, and the CRC check must reconstruct the wire header (which depends on the SYSID32/TARGETTED flags) rather than assuming the structure matches the wire bytes. The extended header target is exposed as target_sysid/target_compid. Users often copy current scripts onto older firmware, so the layout is detected at runtime from the structure size: 291 bytes for the older 1 byte sysid layout, 299 bytes for the 32 bit sysid layout. decode_header additionally returns the payload offset as a second return value so callers do not need layout knowledge of their own.
Full MAVLink2.1 support for the NextGen JavaScript generator, mirroring the C and Python implementations: SYSID32 set only when the source system is over 255, TARGETTED only when a message's target_system field is over 255 (the payload byte is then zeroed), MAVLINK_CFLAG_SYSID32 always advertised, variable header length in framing and decode, extended header targets overlaid onto decoded fields, and MAVLink1 packing throws for 32 bit IDs. The static test fixtures gain the compat_flags advertisement and make_tests.py expects it for MAVLink2 frames, keeping the byte-level cross-check against the C implementation exact. Adds round trip tests for 32 bit source and target IDs.
…ages Negative array indexing returns undefined in JavaScript, so _link_id was never set from the signature block. Use slice instead, matching check_signature().
the MAVLink1 path skips the GOT_LENGTH state, which is where target_sysid and target_compid are cleared, so a re-used receive buffer kept the target from a previous MAVLink2 frame. Forwarding that MAVLink1 frame then silently failed, as _mav_header_pack() treats a target over 255 as unrepresentable in MAVLink1 and returns 0. This broke forwarding on a mixed-version link. The existing test could not catch this as it resets the parser buffer for every frame; the new test re-uses one parser across both frames.
3 participants
This is a WIP effort to implement 32 bit system IDs in MAVLink2. It is based on mavlink/rfcs#20 with a change to use 32 bit instead of 16 bit wide system ID.
Only C and python generation so far, plus addition of CS and JavaScript dropping of unknown incompatible flags
I am also planning on resolving the 32 bit uint32_t issue with ArduPilot parameters as part of this effort which will make using IPv4 addresses more practical