Support MAVLink 32 bit sysid - #3041
Draft
julianoes wants to merge 6 commits into
Draft
Conversation
Groundwork for MAVLink 2's 32 bit system IDs as proposed in ArduPilot/pymavlink#1229, so that e.g. an IPv4 address can be used directly as a system ID. System IDs widen from uint8_t to uint32_t throughout: MavlinkAddress, Mavsdk::Configuration, System, and every plugin's target_system_id. Component IDs stay 8 bit. This breaks the C++, C and Python APIs, which is why it lands before the next major. Mavsdk::MavlinkMessage and the MavlinkDirect proto were already uint32 and are unchanged. Two places had to stop truncating: - Routing. A target above 255 travels in an extended header and the payload's target_system field then reads as 0, which get_target_system_id() would have reported as a broadcast. It now checks IFLAG_TARGETTED first. While widening its return type it also turned out that _MAV_PAYLOAD yields a char*, so a target above 127 sign extended; hence the explicit cast. - Command acks. mavlink_command_ack_t's target_system is 8 bit, so acks are now packed with the originating system ID passed alongside rather than encoded from the struct. Depends on two upstream changes: - libmavlike is bumped to fbcb13f, which makes the MAVLink 2 header variable length (10 to 18 bytes) and adds golden frame tests against the C implementation's output. - The MAVLink C library cannot carry a wide system ID until mavlink/mavlink bumps its pymavlink submodule to include the PR above. MAVLINK_HASH therefore stays at upstream d6a7eeaf and the 32 bit paths sit behind #ifdef MAVLINK_IFLAG_TARGETTED, including four of the five new system tests. They were verified by generating the C headers from the PR branch locally: all 103 system tests and 413 unit tests pass with them, and 99 + 413 pass without.
mavlink/mavlink's pr-sysid-32 moves the pymavlink submodule to tridge's pr-sysid-32, so the generated C headers can carry a system ID wider than 8 bits. With this the four #ifdef MAVLINK_IFLAG_TARGETTED system tests build and pass, taking the system suite from 99 to 103. The pin is temporary and has to go back to a mavlink/mavlink hash on master before release. The guard stays so the tests degrade to the 8 bit compatibility case if it does.
Collaborator
|
Gosh, I think you'll need someone smarter than me to review this. |
Collaborator
Author
|
@hamishwillee sorry did I tag you to review? I must have meant it more as a heads-up. |
Four separate things, all surfaced by widening system IDs:
- jni: mavsdk_configuration_create_manual() and set_system_id() still
cast to uint8_t, so a wide system ID was truncated on the way through
the Kotlin bindings. The getters return jint, which is signed, so an
ID above 2^31-1 reads back negative on the Kotlin side; all 32 bits
survive and toUInt() recovers it.
- gimbal: get_own_system_id() is uint32_t now, and MSVC warns C4244 on
the implicit conversion to the float command parameter. With WERROR
that failed every Windows job. Made the conversion explicit.
- build-with-system-deps.sh pins mavlink and libmav itself, so the
SUPERBUILD=OFF job kept building the old libmav. Bumped both to match
third_party. They were also only checked out on first clone, meaning
an existing deps directory silently ignored a version bump; they are
now checked out on every run.
- kotlin.yml cached third_party keyed on hashFiles('./third_party/**'),
but third_party lives under cpp/. The pattern matched nothing, so the
key was constant and every jni job restored a stale dependency
install no matter what changed. This was already broken, it just
needed a dependency bump to show.
No Kotlin regeneration was needed: only the plugin bindings are
generated, from the protos, and no proto changed. The JNI contract
validation still passes with 40 contracts and 712 native symbols.
macOS was an artifact upload failure (ENOTFOUND), not a build or test
failure, so nothing to do there.
Two more spots found by sweeping for conversions of the now wider system IDs, both of which MSVC flags as C4244 and so fail the Windows jobs under WERROR: - mavlink_command_ack_t::target_system is 8 bit. A wider origin now stores 0 there, which reads as a broadcast, the same thing the wire does for an extended target. send_command_ack() and the auto ack path pass the full value separately, so the ack is still addressed correctly. - MavlinkDirectServer still truncated the target into the payload, where MavlinkDirect no longer does. It now takes the same extended header path as the client.
MavlinkRequestMessageHandler::Callback passes a 32 bit system ID, but these three lambdas still took it as uint8_t. On 64 bit that narrowing is C4244, which the Windows jobs don't enable, so it went unnoticed. On 32 bit Windows size_t is unsigned int and therefore the same type as uint32_t, which makes MSVC report it as C4267 instead, and that one is on and fatal under WERROR.
libevents held system IDs in 8 bit fields, so passing the now wider ones in truncated them, which MSVC flags as C4267 on 32 bit Windows where size_t and uint32_t are the same type. Point libevents at mavlink/libevents#pr-sysid32, which widens them, and follow the one API change that came with it: REQUEST_EVENT's target does not fit the struct handed to the send callback, so the callback now gets it separately. Pack the fields rather than encoding the struct so that a target above 255 ends up in the extended header. Note that an event still cannot address a system above 255, since EVENT's destination_system is not a routing target and so stays 8 bit. Such a system only sees broadcast events until the message itself grows a wider field. The libevents tag needs to move to a merged commit before this lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is WIP, done by LLM to support: ArduPilot/pymavlink#1229
Based on julianoes/libmavlike#4 and mavlink/mavlink#2567.