fix(mavlink): reject unsupported params in commands and missions#27541
Conversation
github-actions
Bot
added
kind:bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
a1cf04f to
4554e4e
Compare
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: 984 byte (0.05 %)]px4_fmu-v6x [Total VM Diff: 984 byte (0.05 %)]Updated: 2026-06-27T01:22:19 |
dakejahl
left a comment
dakejahl
left a comment
There was a problem hiding this comment.
Thanks for this, can you adjust the function return type and variable names?
17cc85d to
452976b
Compare
| * @return true if all unsupported params are unset, false if any | ||
| * unsupported param has a non-zero, non-NaN value | ||
| */ | ||
| static bool check_params(uint16_t cmd, bool for_mission, |
There was a problem hiding this comment.
[error] clang-diagnostic-unused-function [error]
unused function check_params
452976b to
8268e30
Compare
There was a problem hiding this comment.
Wow, thank you.
The overall concept really cool. I've added a few specific comments inline. Here are some questions/confirmations/suggestiosn
-
What happens if a command or mission item not supported at all?
It always validates as true if the command/mission is not supported, because the command won't be there. So that should fall through and be caught in the final catch on MAV_MISSION_UNSUPPORTED.
-
What about cases where handling is deferred. For example, camera commands and mission items can be handled by either MAVLink or a connected camera.
I think that whatever we put in here will then restrict both MAVLink and connected cameras.
Propably OK, but it means that we probably have to allow params on connected cameras we don't support "just in case". -
It is possible for a param to be supported differently on FW and on MC, and for VTOL to support that command in each mode.
For example MAV_CMD_NAV_TAKEOFF has yaw and pitch. Pitch makes sense for FW takeoff but not much for MC, and yaw is the reverse (spec should says that).
It is theoretical in this case since I don't think we support either, but we might one day, so we should allow an approach for specifying per-vehicle support.
This should ideally be something that comes in addition as a special case. -
I like that when you add a new command but forget to update the table the command will still pass.
But it would be good if there was some kind of hint to developers they need to come and fill it in.
@julianoes, would it make sense to set a status log or something if an unsupported command is received? -
This doesn't check the param5, param6, param7 for the reason:
Params 5–7 in COMMAND_LONG/COMMAND_INT are not validated (params 5–6 carry int32 lat/lon in COMMAND_INT, making unified float validation unsound).
In MISSION_ITEM_INT you're always dealling with int32 so you can definitely check in that case.
In commands, by the time you get to this point the values are already converted to NaN and we're in the shared handler
So you can check.Upshot I think we can support all params.
-
As noted inline, the current implementation returns only the error for param1. Probably should return a precise error (or accepted) instead of a bool, then in handling code return the error, and ignore accepted.
In future, we would also like to be able to check valid ranges. For example a yaw might be set to +/-180.
NOthing to do now, but if we return different errors we can also check ranges@julianoes BTW I was thinking we'd add helpers in the C library to return min, max values if set.
Job for another day. -
When we're sure this is about right, can you do a basic sanity check against MAVSDK and QGC - maybe with Claude, but definitely try upload/download and see that errors are reported as expected.
They should be checking errors etc, but we need to let them know that perhaps they should be popping them up to users or whatever.
87d9551 to
7185dc6
Compare
|
Here's some more claude feedback
Note HamishW comment. So the examples here are all cases where we might support them via FC-connected hardware or via MAVLink gimbal. In the case of orbit, is only supported if the appropriate module is installed. mavlink_command_params.h:0 / mavlink_mission.cpp:1469 / mavlink_receiver.cpp:688 When check_params_for_vehicle() returns -1 (command not in table), both call sites emit PX4_WARN("MAV_CMD %u not in param validation table…") and then continue. But a large number of commands that are handled are deliberately not in the table: In mavlink_mission.cpp (MAV_FRAME_MISSION switch, lines ~1665–1695):
In handle_message_command_both the validation fires for everything before the per-command switch, so any unregistered receiver command (e.g. DO_ORBIT=34, DO_REPOSITION=192, DO_FIGURE_EIGHT=35, A drone actively using guided mode will flood the log with this warning. The PR description says "commands with all 4 params valid have no table entry; pass-through is equivalent to a 0x0F mask" — but that logic never reaches the logger. The fix is one of: add them to the table with correct masks (right answer for mission commands that have defined params), or change PX4_WARN → PX4_DEBUG for the < 0 case (acceptable for truly pass-through commands).
mavlink_command_params.h:89–90 Masks 0x78 for both NAV_VTOL_TAKEOFF and NAV_VTOL_LAND allow only p4 (yaw) + p5-7 (coordinates). The MAVLink spec defines:
The Navigator may not consume these today, but GCS tools (QGC) may send them non-zero. If any of these are set, the mission is rejected with MAV_MISSION_INVALID_PARAM1/2/3 — which is a silent regression. Worth
mavlink_command_params.h:217–258 and ~269–318 check_params_for_vehicle and check_params_int_for_vehicle share the binary search, mask lookup, and VehicleOverrides loop verbatim. The only difference is p5/p6 checked as float vs int32. Factor out a
test/mavsdk_tests/test_mavlink_param_validation.py The test is a standalone pymavlink script, not hooked into the existing SITL test framework (Tools/simulation/, test/mavsdk_tests/ via run_tests.py). CI won't catch regressions. Should be wrapped in pytest and |
…pam, fix int_mode cast Signed-off-by: Himaghna <pen314paper@gmail.com>
… int_mode For MAV_FRAME_MISSION items, x/y are generic p5/p6 params (not lat/lon), so GCS tools send 0 for unused params — not INT32_MAX. Using check_params_int_for_vehicle here caused int_param_is_unset(0)=false, which rejected valid DO_LAND_START and similar items with x=y=0. Fix by normalizing INT32_MAX to 0.0f before calling check_params_for_vehicle, so both the MISSION_ITEM_INT sentinel and the conventional float zero are treated as unset. This restores acceptance of zero-param items like DO_LAND_START while still correctly rejecting items that carry non-zero, non-sentinel values in unsupported param slots. Signed-off-by: Himaghna <pen314paper@gmail.com>
@hamishwillee thanks for the review. For the Test CI integration, I've kept it standalone for now, since converting to mavsdk SITL gate is a bit out of scope for this fix PR. Happy to do it as a follow-up if you'd like. |
| { 84, 0x78, 0x78 }, // NAV_VTOL_TAKEOFF: p4:yaw; p5-7:lat/lon/alt | ||
| { 85, 0x78, 0x78 }, // NAV_VTOL_LAND: p4:yaw; p5-7:lat/lon/alt | ||
| { 84, 0x7C, 0x7C }, // NAV_VTOL_TAKEOFF: p3:approach_hdg,p4:yaw; p5-7:lat/lon/alt | ||
| { 85, 0x7F, 0x7F }, // NAV_VTOL_LAND: p1:options,p2:approach_hdg,p3:loiter_r,p4:yaw; p5-7:lat/lon/alt | ||
| { 93, 0x0F, 0x0F }, // NAV_DELAY: p1:delay,p2:hour,p3:min,p4:sec | ||
| { 112, 0x01, 0x01 }, // CONDITION_DELAY: p1:seconds | ||
| { 114, 0x01, 0x01 }, // CONDITION_DISTANCE: p1:distance |
There was a problem hiding this comment.
This was always unsupported in missions, but wasn't rejected. That has been fixed (today) so can be removed from mission list.
| VEHICLE_MC = (1u << 1), | ||
| VEHICLE_VTOL = (1u << 2), | ||
| VEHICLE_ROVER = (1u << 3), | ||
| VEHICLE_ANY = 0xFF, |
There was a problem hiding this comment.
I'm not SURE if we need this. The "any" case automatically should fall through to the default table, which should fall though to accepting all params.
hamishwillee
left a comment
hamishwillee
left a comment
There was a problem hiding this comment.
@bvrtoverfitprimes Thanks very much for this. I've added a couple of minor comments, but Claude seems happy and I'm at the limit of what I am capable as a reviewer.
What I would like you to do is confirm that you have done a real world test with a mission as well?
- try some real word tests using this to download a few simple missions and confirm that QGC does indeed do what we expect.
- Perhaps try a few VTOL cases since they don't seem as well tested.
- Perhaps see if anything changes if we don't allow 0 as an option for the check in the real world.
@julianoes I'm at the end of my reviewing skills here, even with Claude's help. This appears to meet my broad expectation:
- Allows specifying of vehicle specific checks of support for params in MAV_CMD in missions and commands
- Fall back to generic checks of same
- If MAV_CMD is not present in tables falls back to accepting all params (so no worse off than we are now).
- Handles INT32MAX and NaN as the case to indicate we're setting this as explicitly not supported. Also allows 0 for this right now
- To your knowledge, does anything expect PX4 to use the 0 case as a sentinel? If not, we should comment that out and make the tests on MAVSDK and QGC. It would be better to disallow those as soon as we can.
I think it's good, but would be nice to have some professional developer eyes on it.
…ad enum, and RTL test frame
…ams-27483' into fix/mavlink-reject-unsupported-params-27483
| zeroed or NaN. | ||
|
|
||
| Usage: | ||
| python3 test_mavlink_param_validation.py [--url udp://:14540] [--timeout 5] |
There was a problem hiding this comment.
Funny to add this to the mavsdk_tests. It is using pymavlink not mavsdk - which is fine, but it clearly needs its own place somewhere. Maybe one directory up.
And then best case would be to have this run in CI somewhere, which is where the mavsdk_tests harnesses would come in handy. In that case, you'd have to rewrite this and add it to the mavsdk tests.
| * 1–7 1-based index of the first offending param | ||
| * -1 command not in table (no validation applied) | ||
| */ | ||
| [[maybe_unused]] static int check_params_for_vehicle(uint16_t cmd, bool for_mission, uint8_t vehicle_type, |
There was a problem hiding this comment.
What was the reason to implement this all in the header? I don't think it matters in this case because mavlink gets a unified build to prevent multiple translation units, but in general, I probably wouldn't have it all in h.
Oh, and should be hpp actually (I know that's not consistent everywhere yet).
|
@bvrtoverfitprimes Had a chat to @julianoes about this. He's added a few comments above, but we think this is nearly ready to go in. Thanks so much for this. |
|
The |
hamishwillee
left a comment
hamishwillee
left a comment
There was a problem hiding this comment.
@mrpollo Would be good to get this in. UP to you whether you want to take it in v1.18.
Thanks @bvrtoverfitprimes (and @julianoes )
|
Thanks @bvrtoverfitprimes! |
|
Thanks from me too @bvrtoverfitprimes |
Due to the stricter checking introduced by #27541, we cannot just put the position params into any item, it will be rejected if the params are not in fact position entries.
Due to the stricter checking introduced by #27541, we cannot just put the position params into any item, it will be rejected if the params are not in fact position entries.
Strict command-parameter validation added in PX4#27541 rejects MAV_CMD_NAV_TAKEOFF when an unsupported param is set. QGroundControl sends param1 (minimum pitch) = -1 on multicopter takeoff, which is non-zero/non-NaN, so commands were silently denied (MAV_RESULT_DENIED with no log) on MC airframes and QGC guided takeoff stopped working. Extend the NAV_TAKEOFF override to include VEHICLE_MC so param1 is accepted and ignored on multicopters, matching FW/VTOL handling.
|
Hey I appreciate the eagerness for compliance and order but please do always verify latest before merging that you are not suddenly rejecting commonly used commands and hence break basic compatibility. The takeoff command from stable QGC not working anymore on main is a recurring pattern. Here's my suggestion to fix it again without reverting the entire command filtering: #27899 |
Strict command-parameter validation added in #27541 rejects MAV_CMD_NAV_TAKEOFF when an unsupported param is set. QGroundControl sends param1 (minimum pitch) = -1 on multicopter takeoff, which is non-zero/non-NaN, so commands were silently denied (MAV_RESULT_DENIED with no log) on MC airframes and QGC guided takeoff stopped working. Extend the NAV_TAKEOFF override to include VEHICLE_MC so param1 is accepted and ignored on multicopters, matching FW/VTOL handling.
5 participants
Problem
PX4 silently accepted any non-NaN value in params it does not use for a given MAV_CMD. A GCS had no way to detect a malformed mission item or command, and params reserved today could cause unexpected behaviour if assigned meaning in a future spec update. Fixes #27483
What changed
Validation sits entirely at the MAVLink ingress boundary; Commander and Navigator are unchanged.
A param is considered unset when it is NaN (MAVLink standard) or 0.0 (conventional GCS default for unused float fields). Any other value in an unsupported slot is rejected.
Testing
Verified on PX4 SITL (SIH, px4_sitl_default). All 27 tests pass:
Limitations