Copter, Sub, AP_Mount: Fix inconsistent handling of clearing ROIs #25277
Conversation
nexton-winjeel
force-pushed
the
upstream/fix-copter-do-set-roi-none
branch
from
6b66f97 to
1345eb4
Compare
|
Added an autotest to demonstrate the previous (and fixed) behaviour. |
nexton-winjeel
force-pushed
the
upstream/fix-copter-do-set-roi-none
branch
from
1345eb4 to
490d2bb
Compare
|
@peterbarker, @IamPete1: can you please look at this in Randy's absence? |
| @@ -5104,12 +5104,17 @@ MAV_RESULT GCS_MAVLINK::handle_command_int_packet(const mavlink_command_int_t &p | |||
| case MAV_CMD_DO_SET_MODE: | |||
| return handle_command_do_set_mode(packet); | |||
|
|
|||
| case MAV_CMD_DO_SET_ROI_NONE: { | |||
There was a problem hiding this comment.
I'm not really a fan of moving handling of this command up into the GCS layer. That seems like we're moving in the wrong direction.
I think the inconsistently comes from how Copter's special handling of ROI in that we want to be able to point the copter's nose at the ROI target in case the gimbal does not have yaw control.
There was a problem hiding this comment.
Agreed, but moving it here is consistent with how DO_SET_ROI and DO_SET_ROI_LOCATION are currently handled.
I'd rather move this here now, then refactor all three of them together.
There was a problem hiding this comment.
I think that @nexton-winjeel is probably correct that in the short-term this is probably the best fix. The very specific reason is that AP_Mount::handle_command() doesn't handle DO_SET_ROI or DO_SET_ROI_LOCATION so it makes sense that the DO_SET_ROI_NONE would also not be in AP_Mount::handle_command().
FYI @peterbarker the DO_SET_ROI_SYSID is also in AP_Mount::handle_command() which becomes even more of an outlier with this change. Of course it is such a rarely used command that it probably doesn't matter for now.
|
@IamPete1 @peterbarker feel free to merge this if you have no objections. |
|
Merged, thanks! |
Addresses #25276:
FYI: @rmackay9