AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation #24772

khanasif786 · 2023-08-25T17:54:34Z

This adds support for MAV_CMD_IMAGE_STOP_CAPTURE. This is from the camera enhancements list #23151
Thanks to @trolledbypro for initial work on this
This has been tested latest in Mission and with simple mavlink command. (Single camera is used) Multiple should work.

khanasif786 · 2023-08-31T16:03:11Z

I see Mission doesn't support IMAGE START capture with multiple images. I will create a seperate PR for that if its okay.

rmackay9

Looks good to me. This has been tested of course?

khanasif786 · 2023-09-07T10:18:00Z

Tested with single camera in Mission and also with simple mavlink command. Could you please review.

libraries/AP_Camera/AP_Camera.cpp

khanasif786

Thanks @magicrub. I Have done suggested changes in the new patch

libraries/AP_Camera/AP_Camera.cpp

khanasif786

Done changes suggested by @rmackay9

libraries/AP_Camera/AP_Camera.cpp

libraries/AP_Camera/AP_Camera.h

libraries/AP_Camera/AP_Camera.cpp

libraries/AP_Mission/AP_Mission_Commands.cpp

khanasif786

@rmackay9 Thanks. i have done changes

libraries/AP_Camera/AP_Camera.cpp

libraries/AP_Camera/AP_Camera.h

libraries/AP_Mission/AP_Mission_Commands.cpp

libraries/AP_Camera/AP_Camera.cpp

rmackay9 · 2023-09-20T00:11:11Z

libraries/AP_Mission/AP_Mission.h

@@ -269,6 +269,7 @@ class AP_Mission

    // MAV_CMD_IMAGE_START_CAPTURE support
    struct PACKED image_start_capture_Command {
+        uint8_t instance;


I believe this will lead to corruption of the image-start-capture mission command for users that already have the command in a mission on a vehicle. I think this is OK though because Mission Planner at least never had the columns displayed to users so most users were probably leaving all columns blank (see MP PR here ArduPilot/MissionPlanner#3175)

@peterbarker what do you think?

is there any way we can add support without leading corruption ?

@khanasif786,

Yes, I think if the instance was placed at the end of the structure instead of the beginning.

libraries/AP_Mission/AP_Mission_Commands.cpp

khanasif786

@rmackay9 Addressed your comments. have a look

libraries/AP_Camera/AP_Camera.cpp

khanasif786 · 2023-09-20T07:42:02Z

libraries/AP_Mission/AP_Mission.h

@@ -269,6 +269,7 @@ class AP_Mission

    // MAV_CMD_IMAGE_START_CAPTURE support
    struct PACKED image_start_capture_Command {
+        uint8_t instance;


is there any way we can add support without leading corruption ?

libraries/AP_Mission/AP_Mission_Commands.cpp

rmackay9 · 2023-09-20T11:25:30Z

libraries/AP_Camera/AP_Camera.cpp

+    if (backend == nullptr) {
+        return false;
+    }
+    backend->take_picture();


I think this should return the result from backend->take_picture() instead of just returning true.

rmackay9 · 2023-09-20T11:33:02Z

@khanasif786,

Getting close now. Can you also rebase this on master?

rmackay9 · 2023-09-20T11:56:58Z

libraries/AP_Camera/AP_Camera.h


    // take multiple pictures, time_interval between two consecutive pictures is in miliseconds
    // total_num is number of pictures to be taken, -1 means capture forever
    void take_multiple_pictures(uint32_t time_interval_ms, int16_t total_num);
+    bool take_multiple_pictures(uint8_t instance, uint32_t time_interval_ms, int16_t total_num);


I think we should make these take-multiple-pictures methods fail if the total_num is non-zero (e.g. forever or some specified number) and the time_interval_ms is zero. I know you had this originally in the command parser and I asked you to remove it but after testing I see we really do need the check of a non-zero interval... it is better to have it down here in this method anyway though.

We could do this as a follow-up PR though (even I could do it)

Okay upto you then.

rmackay9 · 2023-09-20T12:01:25Z

libraries/AP_Mission/AP_Mission_Commands.cpp

+        if (cmd.content.image_start_capture.total_num_images == 1) {
+            if (cmd.content.image_start_capture.instance == 0) {
+                // take pictures for every backend
+                camera->take_picture();


Ideally I think this take_picture() should also return a boolean which is true if any backend succeeded in taking a picture. I'm happy to do this as a follow-up PR though.

okay upto you now.

Co-authored-by: Zachary Zalass <[email protected]>

khanasif786 · 2023-09-20T17:11:52Z

@rmackay9 . I have rebased it. Could you add the functionalities you told in another PR, Thus the code will also get re-tested on different setup.

issues addressed or will be handled in a follow-up PR

khanasif786 force-pushed the mav_cmd_stop_capture branch from b72dc9b to 147f44c Compare August 29, 2023 06:26

khanasif786 marked this pull request as ready for review August 31, 2023 16:04

rmackay9 approved these changes Aug 31, 2023

View reviewed changes

khanasif786 changed the title ~~AP_Mission/AP_Camera: add MAV_CMD_IMAGE_STOP_CAPTURE support~~ AP_Mission/AP_Camera: add MAV_CMD_IMAGE_STOP_CAPTURE/START_CAPTURE support and fixes to existing support Sep 7, 2023

khanasif786 force-pushed the mav_cmd_stop_capture branch 2 times, most recently from d2a8025 to cf94210 Compare September 7, 2023 10:13

magicrub reviewed Sep 7, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Show resolved Hide resolved

khanasif786 force-pushed the mav_cmd_stop_capture branch from cf94210 to 8d01a48 Compare September 7, 2023 17:54

khanasif786 commented Sep 7, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Show resolved Hide resolved

khanasif786 changed the title ~~AP_Mission/AP_Camera: add MAV_CMD_IMAGE_STOP_CAPTURE/START_CAPTURE support and fixes to existing support~~ AP_Mission/AP_Camera: add mavlinks STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation Sep 7, 2023

khanasif786 changed the title ~~AP_Mission/AP_Camera: add mavlinks STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation~~ AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation Sep 7, 2023

rmackay9 previously requested changes Sep 7, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

khanasif786 force-pushed the mav_cmd_stop_capture branch from 8d01a48 to dbb2e88 Compare September 18, 2023 16:28

khanasif786 commented Sep 18, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

khanasif786 force-pushed the mav_cmd_stop_capture branch from dbb2e88 to 4d9aa36 Compare September 18, 2023 16:29

rmackay9 reviewed Sep 19, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

rmackay9 reviewed Sep 19, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

rmackay9 reviewed Sep 19, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.h Outdated Show resolved Hide resolved

rmackay9 reviewed Sep 19, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

rmackay9 reviewed Sep 19, 2023

View reviewed changes

libraries/AP_Mission/AP_Mission_Commands.cpp Outdated Show resolved Hide resolved

khanasif786 force-pushed the mav_cmd_stop_capture branch from 4d9aa36 to 21a8fb0 Compare September 19, 2023 07:38

khanasif786 commented Sep 19, 2023

View reviewed changes

rmackay9 reviewed Sep 20, 2023

View reviewed changes

libraries/AP_Camera/AP_Camera.cpp Outdated Show resolved Hide resolved

rmackay9 reviewed Sep 20, 2023

View reviewed changes

libraries/AP_Mission/AP_Mission_Commands.cpp Outdated Show resolved Hide resolved

khanasif786 force-pushed the mav_cmd_stop_capture branch from 21a8fb0 to 616fb60 Compare September 20, 2023 07:43

khanasif786 commented Sep 20, 2023

View reviewed changes

rmackay9 reviewed Sep 20, 2023

View reviewed changes

rmackay9 mentioned this pull request Sep 20, 2023

mavcmd: add image-stop-capture command ArduPilot/MissionPlanner#3177

Merged

khanasif786 and others added 5 commits September 20, 2023 22:37

AP_Camera: fix MAV_CMD_START_CAPTURE handling

e10443f

AP_Mission: add MAV_CMD_START_CAPTURE support in mission

346e2cc

AP_Camera: add MAV_CMD_IMAGE_STOP_CAPTURE support

aa91d04

Co-authored-by: Zachary Zalass <[email protected]>

AP_Mission: add MAV_CMD_IMAGE_STOP_CAPTURE support

52128b8

Co-authored-by: Zachary Zalass <[email protected]>

GCS_MAVLink: add MAV_CMD_IMAGE_STOP_CAPTURE support

841d0cc

Co-authored-by: Zachary Zalass <[email protected]>

khanasif786 force-pushed the mav_cmd_stop_capture branch from 616fb60 to 841d0cc Compare September 20, 2023 17:07

rmackay9 added the MergeOnCIPass label Sep 20, 2023

peterbarker merged commit 65be0bd into ArduPilot:master Sep 21, 2023

This was referenced Sep 22, 2023

AP_Camera: image-start-capture with all zeros leads to infinite pics at zero interval #25073

Closed

Camera: image start/stop capture fixes #25075

Merged

common: Add camera instance field to MAV_CMD_IMAGE_START/STOP_CAPTURE mavlink/mavlink#2024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation #24772

AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation #24772

khanasif786 commented Aug 25, 2023 •

edited

Loading

khanasif786 commented Aug 31, 2023 •

edited

Loading

rmackay9 left a comment

khanasif786 commented Sep 7, 2023

khanasif786 left a comment •

edited

Loading

khanasif786 left a comment

khanasif786 left a comment

rmackay9 Sep 20, 2023 •

edited

Loading

khanasif786 Sep 20, 2023

rmackay9 Sep 20, 2023

khanasif786 left a comment

khanasif786 Sep 20, 2023

rmackay9 Sep 20, 2023

rmackay9 commented Sep 20, 2023

rmackay9 Sep 20, 2023 •

edited

Loading

khanasif786 Sep 20, 2023 •

edited

Loading

rmackay9 Sep 20, 2023

khanasif786 Sep 20, 2023

khanasif786 commented Sep 20, 2023

AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation #24772

AP_Mission/AP_Camera: add mavlink STOP_CAPTURE/START_CAPTURE command support and fixes to existing implementation #24772

Conversation

khanasif786 commented Aug 25, 2023 • edited Loading

khanasif786 commented Aug 31, 2023 • edited Loading

rmackay9 left a comment

Choose a reason for hiding this comment

khanasif786 commented Sep 7, 2023

khanasif786 left a comment • edited Loading

Choose a reason for hiding this comment

khanasif786 left a comment

Choose a reason for hiding this comment

khanasif786 left a comment

Choose a reason for hiding this comment

rmackay9 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

khanasif786 Sep 20, 2023

Choose a reason for hiding this comment

rmackay9 Sep 20, 2023

Choose a reason for hiding this comment

khanasif786 left a comment

Choose a reason for hiding this comment

khanasif786 Sep 20, 2023

Choose a reason for hiding this comment

rmackay9 Sep 20, 2023

Choose a reason for hiding this comment

rmackay9 commented Sep 20, 2023

rmackay9 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

khanasif786 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

rmackay9 Sep 20, 2023

Choose a reason for hiding this comment

khanasif786 Sep 20, 2023

Choose a reason for hiding this comment

khanasif786 commented Sep 20, 2023

khanasif786 commented Aug 25, 2023 •

edited

Loading

khanasif786 commented Aug 31, 2023 •

edited

Loading

khanasif786 left a comment •

edited

Loading

rmackay9 Sep 20, 2023 •

edited

Loading

rmackay9 Sep 20, 2023 •

edited

Loading

khanasif786 Sep 20, 2023 •

edited

Loading