mavgen_c: Helper functions to get MAV_CMD metadata and range check params#1216
Open
hamishwillee wants to merge 15 commits into
Open
mavgen_c: Helper functions to get MAV_CMD metadata and range check params#1216hamishwillee wants to merge 15 commits into
hamishwillee wants to merge 15 commits into
Conversation
peterbarker
reviewed
Jun 9, 2026
Dialects such as standard.xml and minimal.xml define no MAV_CMD enum, so generating mav_cmd_helpers.h raised RuntimeError and aborted the whole mavgen run when more than one dialect was processed. Make mavgen_c_cmd_helpers.generate() return False and skip writing the header in that case instead of raising, and only print "Generated" when a header was actually emitted.
The bool is_int parameter on param_invalid/lat_in_range/lon_in_range was easy to pass wrong and forced lossy (float)int32 casts at call sites. Replace it with explicitly typed variants: param_invalid(float) NaN / +-0.0 coord_invalid_int(int32_t) INT32_MAX 'use current' sentinel lat_in_range_int(int32_t) lat_in_range_float(float) lon_in_range_int(int32_t) lon_in_range_float(float) The int variants take int32_t degE7 directly so COMMAND_INT / MISSION_ITEM_INT callers no longer cast. Update check_range and the docs examples accordingly.
__builtin_memcpy is a GCC/Clang extension and is not available on all toolchains (e.g. MSVC), which contradicts the header's self-contained, portable promise. The mavlink fixed headers (protocol.h, mavlink_helpers.h) already use <string.h> memcpy; follow that convention.
mav_cmd_helpers.h is a generated dialect header, like mavlink.h and the per-message headers, which guard with '#ifdef __cplusplus / extern "C"'. The previous unconditional 'namespace mav_cmd_helpers' diverged from that convention and broke the documented unqualified API under C++ (callers would have needed mav_cmd_helpers:: qualification). Use extern "C" so the same unqualified calls work in both C and C++.
check_range previously skipped any param that was NaN or +-0.0, so a value of 0 bypassed bounds even for params whose XML minimum is greater than 0 (e.g. DO_MOTOR_TEST motor instance >= 1, fence vertex count >= 3). Only NaN/Inf are now treated as 'not set' and skipped; a literal 0 is checked like any other value. This prefers a false positive, which can be relaxed by updating the XML ranges, over letting an out-of-range 0 through. Rename the internal _bound_is_set helper to _is_finite since it is now also used to test param values, not just bounds.
The generated header named the wrong generator file (mavgen_cmd_helpers.py) and referenced a 'build system / mavlink submodule' regeneration flow that does not necessarily exist. Name the actual generator and the real source XML basename, and describe regeneration generically via mavgen.
MAV_CMD enums contain non-command marker entries: the synthetic MAV_CMD_*_ENUM_END the parser appends, and the *_LAST NOP entries that only mark the upper bound of a command group (hasLocation=false, described as NOPs). None are sendable commands, so drop them from the param_bounds and cmd_flags tables.
check_range indexes params[param-1] using the 1-based param number from the bounds table. Valid MAVLink XML keeps this in 1..7, but a malformed or extended entry with param==0 (unsigned wrap) or param>7 would read past the params[7] stack array. Skip any such entry defensively.
check_range previously skipped any non-finite param, so +/-Inf was silently accepted as 'not set' alongside NaN. Treat only NaN as the unset sentinel; +/-Inf is now range-checked and fails against any bound on the violated side, consistent with preferring a false positive over passing a bad value. Replace the _is_finite helper with a NaN-specific _is_nan: 'finite' was the wrong concept since it also rejects Inf. Bounds use NaN as their 'unbounded' sentinel and are never Inf, so the same predicate answers 'is this bound set?' correctly.
…ditions MAV_CMD check additions
Contributor
Author
|
Note, fixes by Julian look good to me - they are individually explained in each commit. |
Contributor
Author
|
This is the header built for common - note, others don't have mav_cmd, so no header built. |
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
3 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.
MAVLink command parameters (p1–p7) carry numeric values whose valid ranges are defined in the MAVLink XML — for example, a heading param may only accept 0–360°, or a speed param may require a non-negative value.
Validating these ranges at the MAVLink layer lets a vehicle or GCS reject malformed commands an mission items early, before they reach the flight stack, reducing the risk of undefined behaviour caused by out-of-range inputs.
This PR provides efficient range checking in a generated mavgen_c header
mav_cmd_helpers.h.These can be used by any flight stack to check inputs.
In addition to explicit range checking, it also provides methods for checking that lat/lon values are in valid ranges.
Note that what is actually range-checked depends on the library you build. Docs in https://github.com/ArduPilot/pymavlink/pull/1216/changes#diff-0009f780fff194d866e51fab6ff08d0765c0182b1a2ffb0b85651ff34bc9dde3 and in-source.