GCS_MAVLink: GCS_Common: Add support to MAV_CMD_DO_SET_SYS_CMP_ID #26424
Conversation
patrickelectric
force-pushed
the
add_set_cmp_sys_id
branch
from
13342a0
to
c6c7fa5
Compare
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| return MAV_RESULT_FAILED; | ||
| } | ||
|
|
||
| if (!AP_Param::set_and_save_by_name("SYSID_THISMAV", new_system_id)) { |
There was a problem hiding this comment.
Should check we're not setting the same value into the parameter that it already has here.
Unless it is not configured in storage, in which case we may or may not want to save it....
There was a problem hiding this comment.
Changed to: set_and_save_by_name_ifchanged
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| } | ||
|
|
||
| if (do_reboot) { | ||
| hal.scheduler->reboot(false); |
There was a problem hiding this comment.
If this returns then there's been a failure, so we should not return ACCEPTED.
There was a problem hiding this comment.
What should we return ?
There was a problem hiding this comment.
MAV_RESULT_FAILED looks appropriate.
There was a problem hiding this comment.
I'm not sure if I follow you logic here. I just moved the reboot action to a late_reboot. The idea is to allow the vehicle to ACK the command before rebooting.
| { | ||
| const uint8_t new_system_id = packet.param1; | ||
| const uint8_t new_component_id = packet.param2; | ||
| const uint8_t do_reboot = packet.param3; |
There was a problem hiding this comment.
Should this be a "reboot" or "this change should take effect immediately" flag?
There was a problem hiding this comment.
This is a reboot flag. https://mavlink.io/en/messages/development.html#MAV_CMD_DO_SET_SYS_CMP_ID
There was a problem hiding this comment.
Yep, we read the message description.
The intent is for the thing to take effect immediately, 'though, isn't it? We can probably do that without a reboot my modifying the mavlink state variables.
I think this field should probably be "take effect immediately", rather than "reboot". We can discuss at DevCallEU tonight if you like....
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
patrickelectric
force-pushed
the
add_set_cmp_sys_id
branch
from
c6c7fa5
to
1798513
Compare
| const uint8_t new_component_id = packet.param2; | ||
| const uint8_t do_reboot = packet.param3; | ||
|
|
||
| if (new_system_id == 0 || hal.util->get_soft_armed()) { |
There was a problem hiding this comment.
I wonder why we need to reject the command if new_system_id is zero (apart from "that's what the message says")
This means you can't tell a system to just change its component ID.
... but you can tell it to just change its system ID. This is weird.
There was a problem hiding this comment.
The entire explanation of the message logic is here: mavlink/mavlink#2082 (comment)
In a system with multiple components per vehicle, accepting a broadcast will change all components to a single component_id, and that would be terrible. The only way to change the component ID is to communicate with it directly. You can change all components to the same system_id though.
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
patrickelectric
force-pushed
the
add_set_cmp_sys_id
branch
from
1798513
to
924dff8
Compare
Needs: ArduPilot/mavlink#355