Add suport for MAV_CMD_DO_SET_ROI_LOCATION in AP_Mission

[bytesByHarsh]

Feature request

Currently only in GCS_Common.cpp file MAV_CMD_DO_SET_ROI_LOCATION support is added, the command sent via Command Long Packet is parsed properly but when MAV_CMD_DO_SET_ROI_LOCATION is sent as a mission item it returns MAV_RESULT_UNSUPPORTED to GCS.

Describe the solution you'd like
A possible solution can be adding a switch statement in the AP_mission.cpp file for MAV_CMD_DO_SET_ROI_LOCATION.

Platform
[ X ] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

If required I can create a MR for adding these changes in the code.

[bytesByHarsh]

Also if possible can we add the support for MAV_CMD_DO_SET_ROI_NONE as well.

[DonLakeFlyer]

Related to QGC problems discussed here: mavlink/qgroundcontrol#11963

QGC also uses MAV_CMD_DO_SET_ROI_WPNEXT_OFFSET for structure scan missions

[entone]

Would love to see support for this as well.

Does Mission Planner use different commands that are supported to provide the same functionality?

[rmackay9]

Hi @entone, AP supports the very similar DO_SET_ROI which can be seen in MP's Plan screen's command field's drop-down. It's also documented here on the wiki

[DonLakeFlyer]

Right but DO_SET_ROI was deprecated 7 years ago. QGC only supports the current ROI command set. These are used not only in missions, but in setting an ROI directly while flying as well as the ROI_WPNEXT_OFFSET variant when building structure scan missions. As it stands now because of this a decent sized pile of good user functionality in QGC is not available to Ardupilot users. I realize the majority of Ardupilot users might use Mission Planner. But there are still a huge pile that use QGC. As far as I see the new ROI commands are basically just a restructuring of the old ones. I don't think there is new functionality, just a rearranging of the deck chars. I don't think it should not be super hard to support the new set while still keeping support for the old ones.

[DonLakeFlyer]

Something along these lines as a start:

don@Donalds-Mac-Pro ardupilot % git diff
diff --git a/ArduCopter/mode_auto.cpp b/ArduCopter/mode_auto.cpp
index c5be03c8b1..860424440a 100644
--- a/ArduCopter/mode_auto.cpp
+++ b/ArduCopter/mode_auto.cpp
@@ -767,10 +767,14 @@ bool ModeAuto::start_command(const AP_Mission::Mission_Command& cmd)
         break;
 
     case MAV_CMD_DO_SET_ROI:                // 201
+    case MAV_CMD_DO_SET_ROI_LOCATION:       // 195
+    case MAV_CMD_DO_SET_ROI_NONE:           // 197
         // point the copter and camera at a region of interest (ROI)
         do_roi(cmd);
         break;
 
+    
+
 #if HAL_MOUNT_ENABLED
     case MAV_CMD_DO_MOUNT_CONTROL:          // 205
         // point the camera to a specified angle
@@ -993,6 +997,8 @@ bool ModeAuto::verify_command(const AP_Mission::Mission_Command& cmd)
     case MAV_CMD_DO_CHANGE_SPEED:
     case MAV_CMD_DO_SET_HOME:
     case MAV_CMD_DO_SET_ROI:
+    case MAV_CMD_DO_SET_ROI_LOCATION:
+    case MAV_CMD_DO_SET_ROI_NONE:
 #if HAL_MOUNT_ENABLED
     case MAV_CMD_DO_MOUNT_CONTROL:
 #endif
@@ -1967,7 +1973,7 @@ void ModeAuto::do_set_home(const AP_Mission::Mission_Command& cmd)
     }
 }
 
-// do_roi - starts actions required by MAV_CMD_DO_SET_ROI
+// do_roi - starts actions required by MAV_CMD_DO_SET_ROI, MAV_CMD_DO_SET_ROI_LOCATION, MAV_CMD_DO_SET_ROI_NONE
 //          this involves either moving the camera to point at the ROI (region of interest)
 //          and possibly rotating the copter to point at the ROI if our mount type does not support a yaw feature
 // TO-DO: add support for other features of MAV_CMD_DO_SET_ROI including pointing at a given waypoint
diff --git a/libraries/AP_Mission/AP_Mission.cpp b/libraries/AP_Mission/AP_Mission.cpp
index 6f8492bbbe..16ef3d5520 100644
--- a/libraries/AP_Mission/AP_Mission.cpp
+++ b/libraries/AP_Mission/AP_Mission.cpp
@@ -912,6 +912,7 @@ bool AP_Mission::stored_in_location(uint16_t id)
     case MAV_CMD_DO_LAND_START:
     case MAV_CMD_DO_GO_AROUND:
     case MAV_CMD_DO_SET_ROI:
+    case MAV_CMD_DO_SET_ROI_LOCATION:
     case MAV_CMD_NAV_VTOL_TAKEOFF:
     case MAV_CMD_NAV_VTOL_LAND:
     case MAV_CMD_NAV_PAYLOAD_PLACE:
@@ -1243,6 +1244,15 @@ MAV_MISSION_RESULT AP_Mission::mavlink_int_to_mission_cmd(const mavlink_mission_
     case MAV_CMD_DO_GO_AROUND:                          // MAV ID: 191
         break;
 
+    case MAV_CMD_DO_SET_ROI_LOCATION:                   // MAV ID: 195
+        cmd.p1 = MAV_ROI_LOCATION;
+        break;
+
+    case MAV_CMD_DO_SET_ROI_NONE:                       // MAV ID: 196
+        cmd.p1 = MAV_ROI_NONE;
+        cmd.content.location = Location(); // zero location
+        break;
+
     case MAV_CMD_DO_SET_ROI:                            // MAV ID: 201
         cmd.p1 = packet.param1;                         // 0 = no roi, 1 = next waypoint, 2 = waypoint number, 3 = fixed location, 4 = given target (not supported)
         break;

_LOCATION and _NONE are pretty easy I think. WPNEXT_OFFSET is only slightly trickier since it needs to handle the offsets.

[entone]

@rmackay9 thanks! I'm on Linux so using QGC. I was just wondering how MP managed to support it, looks like we got an answer from @DonLakeFlyer.

[DonLakeFlyer]

I also put a word out on the QGC discord channel to see if anyone had the time to help out with the coding on this in ardupilot.

[rmackay9]

Thanks @DonLakeFlyer and @entone for the reminder on this. Surely we can fix this. Sorry about the delay

[brad112358]

For some reason, recent versions of QGC have started using MAV_CMD_DO_SET_ROI_LOCATION instead of MAV_CMD_DO_SET_ROI for Ardupilot vehicles. I suspect this is unintended since APMFirmwarePlugin.cc still does not include MAV_CMD_DO_SET_ROI_LOCATION in supportedCommands{}. I will report a QGC issue, but until QGC is "fixed", this Ardupilot deficiency has become rather more critical I think.

[DonLakeFlyer]

For some reason, recent versions of QGC have started using MAV_CMD_DO_SET_ROI_LOCATION instead of MAV_CMD_DO_SET_ROI for Ardupilot vehicles

That was reverted a while ago. Are you running latest daily? It was a mistake caused by the confusion around ArduPilot supporting ROI_LOCATION as a command but not as a mission item.

[brad112358]

No, I'm not running the latest daily and it looks like the PR that reverted it was never merged to the Stable_V4.3 branch, so the latest released versions still have this issue even though they were released after the troublesome PR was reverted.

I'm running the latest release.

[rmackay9]

I've created AP flight code PR #29567 to add support for these two commands. If anybody would like to test I'm happy to create a firmware for you, just tell which which autopilot you're using