Added camera plugin with basic functionality #338

julianoes · 2018-03-23T20:27:50Z

Todos:

camera definition and settings inspired by https://github.com/mavlink/qgroundcontrol/blob/master/src/Camera/QGCCameraControl.cc.

Fixes #154.

shakthi-prashanth-m

Nice! Does the plugin deal only with first camera ?

We need to implement MAVLink Camera protocol isn't it?
I mean that, we should first request for camera information and on response we receive available cameras, etc. I can probably contribute that part if you are ok with that.

We don't seem to handle heartbeat of camera component ? Did you disable autopilot heartbeats while testing ?

shakthi-prashanth-m · 2018-03-24T01:22:57Z

plugins/camera/camera_impl.cpp

+    return camera_result_from_command_result(
+               _parent.send_command_with_ack(
+                   MAV_CMD_IMAGE_START_CAPTURE,
+                   MavlinkCommands::Params {0.0f, // all camera IDs


What is the difference between Camera ID and Camera Component ID ?

Camera ID should actually be a thing of the past. I removed the comments.

shakthi-prashanth-m · 2018-03-24T01:24:35Z

plugins/camera/camera_impl.cpp

+                                            1.0f, // take only one picture
+                                            float(_capture_sequence++),
+                                            NAN, NAN, NAN},
+                   MAV_COMP_ID_CAMERA));


As we discussed, there'll be one instance per camera; so, camera component ID would be a member. Isn't it ?

That's right. For now I'm just going to add the current single device plugin. Later we can extend it.

hamishwillee · 2018-03-25T23:39:26Z

plugins/camera/camera.h

+class Device;
+
+/**
+ * @brief The Camera class allows to control generic mavlink cameras.


Perhaps to

The Camera class can be used to manage cameras that implement the MAVLink Camera Protocol: https://mavlink.io/en/protocol/camera.html

This plugin is different in that multiple instances can be used with one device. We should capture that here too.

hamishwillee · 2018-03-25T23:40:10Z

plugins/camera/camera.h

+{
+public:
+    /**
+     * @brief Constructor. Creates the plugin for a specific Device.


FYI, if that rename for System goes in then this will have to change.

hamishwillee · 2018-03-25T23:43:32Z

This looks like it supports just a single camera per device (if not, then need to explain the "how"here). There is also no example.

shakthi-prashanth-m

Looks good :) I just added few suggestions and questions.

shakthi-prashanth-m · 2018-03-26T05:17:28Z

integration_tests/camera_test_helpers.cpp

+    auto prom = std::make_shared<std::promise<void>>();
+    auto ret = prom->get_future();
+
+    // Let's get the mode first


We're setting mode, not get isn't it ?

I suspect this comment is here by mistake, possibly due to the copy paste error: https://github.com/dronecore/DroneCore/pull/338/files#diff-ac9feaa7a785b954819058e5ec940b4fR18

Thanks, fixed in 3bba620.

shakthi-prashanth-m · 2018-03-26T05:20:41Z

plugins/camera/camera.h

+     * @param result The enum value for which a human readable string is required.
+     * @return Human readable string for the Camera::Result.
+     */
+    static const char *result_str(Result result);


I think we should make

static std::string result_str(Result result);

in all the plugins.

I agree. Will change it.

shakthi-prashanth-m · 2018-03-26T05:21:28Z

plugins/camera/camera.h

+    /**
+     * @brief Stop video capture (synchronous).
+     *
+     * This stops a video recording again.


See comments for stopping image capture.

shakthi-prashanth-m · 2018-03-26T05:30:11Z

plugins/camera/camera_definition.h

+    ~CameraDefinition();
+
+
+    void load_file(const char *filename);


Should be

void load_file(const std::string &filepath);

That's nicer, I'll change it.

shakthi-prashanth-m · 2018-03-26T05:32:33Z

plugins/camera/camera_definition.h

+    void load_string(const std::string &content);
+
+    const char *get_vendor() const;
+    const char *get_model() const;


const std::string &get_vendor() const; const std::string &get_model() const;

Done but not with a reference but a copy.

shakthi-prashanth-m · 2018-03-26T05:34:16Z

plugins/camera/camera_definition_test.cpp

+
+TEST(CameraDefinition, LoadE90InfoFile)
+{
+    CameraDefinition cd;


Nice name cd :)

shakthi-prashanth-m · 2018-03-26T05:39:50Z

plugins/camera/camera_impl.cpp

+        MAV_COMP_ID_CAMERA);
+
+    _parent.send_command_with_ack_async(
+        MAV_CMD_REQUEST_STORAGE_INFORMATION,


http://mavlink.org/messages/common/#MAV_CMD_REQUEST_STORAGE_INFORMATION,
http://mavlink.org/messages/common/#STORAGE_INFORMATION
are WIP. So, we are just adding hooks in the plugin right now, isn't it ?

Yer we should probably remove the WIP soon. It has been there for a long time.

shakthi-prashanth-m · 2018-03-26T06:21:12Z

plugins/camera/camera_impl.cpp

+    mavlink_camera_information_t camera_information;
+    mavlink_msg_camera_information_decode(&message, &camera_information);
+
+    load_definition_file(camera_information.cam_definition_uri);


Don't we store camera definition version ?

Not yet, good point.

So as long as we don't cache the file there is no value in having the definition version.

shakthi-prashanth-m · 2018-03-26T06:29:34Z

integration_tests/camera_mode.cpp

+    std::this_thread::sleep_for(std::chrono::seconds(2));
+
+    set_mode(camera, Camera::Mode::VIDEO);
+    std::this_thread::sleep_for(std::chrono::seconds(2));


I assume this is just like pausing and not mandatory for set mode activity ? Because, I see you're already waiting in set_mode().

Good catch, double is probably not needed.

I think I removed most of these.

shakthi-prashanth-m · 2018-03-26T06:33:10Z

integration_tests/camera_settings.cpp

+
+    LogDebug() << "Possible settings in photo mode: ";
+    for (auto setting : settings) {
+        LogDebug() << "- " << setting.second;


It looks good if both name and value are printed. Isn't it ?

I think so, I need to work on this, it's far from finished.

shakthi-prashanth-m · 2018-03-26T08:44:42Z

plugins/camera/camera.h

+    /**
+     * @brief Information about a picture just captured.
+     */
+    struct CaptureInfo {


We can add a flag that tells whether its a standalone camera.

struct CaptureInfo { bool is_standalone; struct Position { ... }; struct Quaternion { ... }; ... };

So, if is_standalone is true, then application need not look into position and quaternion details.

I would say if it is standalone you actually need to look into it because you can't look at the autopilot's telemetry.

shakthi-prashanth-m · 2018-03-26T08:46:06Z

plugins/camera/camera.h

+    bool get_possible_settings(std::map<std::string, std::string> &settings);
+    bool get_possible_options(const std::string &setting_name, std::vector<std::string> &options);
+
+    typedef std::function<void(Result, const std::string &)> get_option_callback_t;


Just a suggestion: How about grouping all typedefs together to ease reading ?

I tried to group together with methods that use the typedefs, so that you don't need to scroll around when looking at a method signature.

shakthi-prashanth-m · 2018-03-26T11:01:22Z

plugins/camera/camera.h

+     * @return Result of request.
+     */
+    Result stop_photo_interval();
+


How about providing a static method that tells the number of cameras on the drone.

static size_t Camera::how_many_cameras() const;

OR

static size_t Camera::total_cameras() const;

whatever is more intuitive. We get to know this info in CAMERA_INFORMATION. We get one response for each camera found on the drone.

As said, I'll work on multi camera support when single camera support is merged and properly working. I can't do all at once 😄.

Lazy! So hard to find good help now days :-)

@hamishwillee in what context ? :-)

I have a gazillion things I could do every day and I'm always trying to figure out what's most relevant 😕.

Just teasing (I thought obviously?). You do plenty of good.

I understood that actually 😃 and it's probably how it always goes.

shakthi-prashanth-m · 2018-04-04T11:24:51Z

plugins/camera/camera.h

+    bool get_possible_settings(std::map<std::string, std::string> &settings);
+    bool get_possible_options(const std::string &setting_name, std::vector<std::string> &options);
+
+    typedef std::function<void(Result, const std::string &)> get_option_callback_t;


shakthi-prashanth-m · 2018-04-04T11:27:21Z

plugins/camera/camera_definition.cpp

+    std::cout << content;
+}
+
+const char *CameraDefinition::get_model() const


Same as here.

shakthi-prashanth-m · 2018-04-04T11:27:52Z

plugins/camera/camera_definition.cpp

+           FirstChildElement("model")->GetText();
+}
+
+const char *CameraDefinition::get_vendor() const


Same as here.

shakthi-prashanth-m · 2018-04-04T11:29:26Z

plugins/camera/camera_definition.cpp

+{
+}
+
+void CameraDefinition::load_file(const char *filename)


Same as here.

shakthi-prashanth-m · 2018-04-04T11:30:11Z

plugins/camera/camera_definition.cpp

+{
+    std::vector<std::string> exclusion_parameter_names {};
+
+    parameters.clear();


shakthi-prashanth-m · 2018-04-04T12:16:36Z

plugins/camera/camera_impl.cpp

+        LogWarn() << "Error: no camera defnition available yet.";
+        if (callback) {
+            callback(Camera::Result::ERROR, "");
+        }


I am not sure. Don't we need to return from here ? It seems falling through.

Very good catch, thank you!

shakthi-prashanth-m · 2018-04-04T12:19:13Z

plugins/camera/camera_definition.h

+    void update_setting(const std::string &name, const ParameterValue &value);
+
+    typedef std::map<std::string, std::shared_ptr<Parameter>> parameter_map_t;
+    bool get_parameters(parameter_map_t &parameters, bool filter_possible);


Its a selector, isn't it ? If so, it should be

bool get_parameters(parameter_map_t &parameters, bool filter_possible) const;

I tried to make a few of these methods const but could not because when the map[key] is used it can insert an entry in the map if not available and thus change the map. Presumably map[key] is the wrong way and map.at(key) should be used instead.

shakthi-prashanth-m · 2018-04-04T12:19:52Z

plugins/camera/camera_definition_test.cpp

+
+using namespace dronecore;
+
+static const char *e90_unit_test_file = "extensions/plugins/camera/e90_unit_test.xml";


std::string is better. :)

shakthi-prashanth-m · 2018-04-04T12:23:14Z

plugins/camera/camera_impl.cpp

+}
+
+Camera::Result CameraImpl::start_video()
+{


Don't we need to check whether video capture in progress ? Also, we need to make sure we're in Video mode. Same applies to async versions.

I made a TODO.

shakthi-prashanth-m · 2018-04-04T12:25:40Z

plugins/camera/camera_impl.cpp

+{
+    std::lock_guard<std::mutex> lock(_capture.mutex);
+
+    return camera_result_from_command_result(


Don't we need to make sure we're in Photo mode? Same applies to async versions.

shakthi-prashanth-m · 2018-04-05T09:30:04Z

plugins/camera/camera_impl.cpp

+}
+
+Camera::Result CameraImpl::start_video()
+{


shakthi-prashanth-m · 2018-04-05T09:31:03Z

plugins/camera/camera_impl.h

+    void receive_int_param(const std::string &name, bool success, int value);
+    void receive_float_param(const std::string &name, bool success, float value);
+
+    CameraDefinition *_camera_definition = nullptr;


It could be a unique pointer, I guess.

JonasVautherin · 2018-04-09T09:25:04Z

cmake/compiler_flags.cmake

 if(APPLE)
+    # We need a define if on APPLE


Same thing here: if(APPLE) add_definitions("-DAPPLE") tells me that "if APPLE is defined, we add the definition called 'APPLE'". And I assume it is needed, otherwise we would not do it.

The comment represents 25% of this piece of code and just says exactly the same :P. Even worse: maybe at some point, the comment will get disconnected from the add_definitions call, and it will be misguiding.

I guess I wrote these comments because I'm a cmake noob so for me it wasn't trivial that the variable APPLE would not result in a define APPLE while compiling. Now I know 😄 .

Removed it :)

JonasVautherin · 2018-04-09T09:27:45Z

cmake/compiler_flags.cmake

@@ -40,3 +40,6 @@ set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -DDEBUG")
 set(CMAKE_CXX_FLAGS_COVERAGE "${CMAKE_CXX_FLAGS_COVERAGE} --coverage")
 set(CMAKE_EXE_LINKER_FLAGS_COVERAGE "${CMAKE_EXE_LINKER_FLAGS_COVERAGE} --coverage")
 set(CMAKE_LINKER_FLAGS_COVERAGE "${CMAKE_LINKER_FLAGS_COVERAGE} --coverage")
+
+# Otherwise tinyxml2 complains.


And I believe this one is useful, because that's a bit peculiar.

Right, anyway I appreciate the discussion 👍

shakthi-prashanth-m · 2018-04-12T11:53:36Z

plugins/camera/camera_impl.h

+        std::mutex mutex {};
+        Camera::mode_callback_t callback {nullptr};
+        void *timeout_cookie {nullptr};
+    } _get_mode;


I think we should store camera mode here. We can use this mode to validate whether we're in Photo or Video mode in APIs like start_video(), take_photo(), etc. Isn't it ?

We actually store the camera mode in the camera_definition, so I'm not sure if we need it here as well.

shakthi-prashanth-m · 2018-04-12T11:56:44Z

@julianoes please rebase to develop. Thanks.

shakthi-prashanth-m · 2018-04-12T14:21:06Z

plugins/camera/camera_impl.cpp

+                   MAV_CMD_VIDEO_START_CAPTURE,
+                   MAVLinkCommands::Params {0.0f, // Reserved, set to 0
+                                            NAN, // fps not set yet
+                                            NAN, // resolution not set yet


There is no resolution parameter in Video capture command.

2500 MAV_CMD_VIDEO_START_CAPTURE Starts video capture (recording). Use NAN for reserved values.

Mission Param #1 Reserved (Set to 0)

Mission Param #2 Frequency CAMERA_CAPTURE_STATUS messages should be sent while recording (0 for no messages, otherwise frequency in Hz)

Mission Param #3 Reserved (all remaining params)

oh you're right, that changed.

This adds more camera features and some integration tests for it.

This is a mavlink spec leftover that has been removed.

The cURL headers leave a define for ERROR which causes compile errors on Windows.

With a recent E90 update, this is not needed anymore.

julianoes · 2018-04-20T18:46:28Z

Ok, integration tests work against E90, E50, ET, and I've tried to address all comments so this should be ok to merge, finally.

shakthi-prashanth-m

Good work @julianoes !

hamishwillee · 2018-04-23T00:36:35Z

plugins/camera/camera.h

+ * MAVLink Camera Protocol: https://mavlink.io/en/protocol/camera.html.
+ *
+ * Currently only a single camera is supported.
+ * When multiple cameras are supported the plugin will need to instantiated


hamishwillee · 2018-04-23T00:38:42Z

plugins/camera/camera.h

+
+
+    /**
+     * @brief Get camera status.


Get camera status (asynchronous).

(just for consistency with all other methdos)

hamishwillee · 2018-04-23T01:13:09Z

plugins/camera/camera.h

+        uint16_t rotation_deg = 0u; /**< @brief Video image rotation clockwise (0-359 degrees). */
+        std::string uri {}; /**< @brief Video stream URI. */
+
+        void set_highest()


Missing documentation

hamishwillee · 2018-04-23T01:13:35Z

plugins/camera/camera.h

+            bit_rate_b_s = BIT_RATE_AUTO;
+        }
+
+        constexpr static const float FRAME_RATE_HIGHEST = -1.0f;


Missing documentation for these ones.

hamishwillee · 2018-04-23T01:14:21Z

Looks good. I added some minor docs fixes. There is still a method and a few constants to document (marked up).

shakthi-prashanth-m · 2018-04-23T04:24:50Z

MAVLink Video stream spec needs to be changed as raised here: mavlink/mavlink#883. So, we've a dependency on mavlink/mavlink#885.

julianoes · 2018-04-23T13:43:55Z

@hamishwillee thanks for making the doc fixes, appreciated!

Added camera plugin with basic functionality

julianoes self-assigned this Mar 23, 2018

julianoes requested a review from shakthi-prashanth-m March 23, 2018 20:27

julianoes force-pushed the add-camera branch 2 times, most recently from c14cc29 to ce6cb7a Compare March 23, 2018 21:00

shakthi-prashanth-m reviewed Mar 24, 2018

View reviewed changes

julianoes force-pushed the add-camera branch from bc435ce to 2ccc4ae Compare March 24, 2018 14:55

hamishwillee reviewed Mar 25, 2018

View reviewed changes

shakthi-prashanth-m reviewed Mar 26, 2018

View reviewed changes

shakthi-prashanth-m reviewed Apr 4, 2018

View reviewed changes

julianoes force-pushed the add-camera branch from 32f7b53 to 7536ddc Compare April 5, 2018 09:15

shakthi-prashanth-m reviewed Apr 5, 2018

View reviewed changes

JonasVautherin reviewed Apr 9, 2018

View reviewed changes

shakthi-prashanth-m mentioned this pull request Apr 10, 2018

Camera plugin to implement MAVLink Camera Protocol client interface #292

Closed

10 tasks

shakthi-prashanth-m reviewed Apr 12, 2018

View reviewed changes

julianoes added 9 commits April 13, 2018 15:48

Added camera plugin with basic functionality

452ba9c

camera: added API docstrings

780c1e1

camera: added setter/getter for camera mode

4916523

camera: status, mode, capture, status, tests

f50af93

This adds more camera features and some integration tests for it.

camera: add missing include

eba1144

camera: clean up camera ID comments

81f33bd

This is a mavlink spec leftover that has been removed.

camera: actually use using namespace

8f99d44

camera: don't ignore get_mode command ack

e8c7091

camera: clean up different timeouts and mutices

0a95d07

julianoes added 11 commits April 20, 2018 10:25

core: remove ERROR define

1367076

The cURL headers leave a define for ERROR which causes compile errors on Windows.

camera: remove some wrong or unnecessary comments

3bba620

build_system: remove unnecesary comments

efdfa43

camera: improve and add comments

1ff5fdd

camera: add getter for setting/option names

6f40598

camera: use std::string instead of const char*

ff2580d

appveyor: copy dronecore_camera.dll for unit tests

35a7e83

camera: the lastest E90 version has more options

a94ff6d

camera: don't do E90 specific tests by default

993bcf5

camera: add tests for Yuneec E50 and ET as well

6f63912

camera: get mode before set mode not needed

8c54323

With a recent E90 update, this is not needed anymore.

appveyor: copy tinyxml2.dll for unit test

7594450

julianoes force-pushed the add-camera branch from ad83c44 to 7594450 Compare April 20, 2018 20:05

shakthi-prashanth-m approved these changes Apr 21, 2018

View reviewed changes

hamishwillee reviewed Apr 23, 2018

View reviewed changes

Minor docs fixes

c8a5c17

hamishwillee reviewed Apr 23, 2018

View reviewed changes

video stream: Add missing docs

bcb5009

julianoes merged commit 15f6d91 into develop Apr 23, 2018

julianoes deleted the add-camera branch April 23, 2018 13:44

This was referenced Apr 24, 2018

Camera plugin Reference mavlink/MAVSDK-docs#111

Merged

Camera/Gimbal guide and example docs mavlink/MAVSDK-docs#112

Open

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

rt-2pm2 pushed a commit to rt-2pm2/DronecodeSDK that referenced this pull request Nov 27, 2018

Merge pull request mavlink#338 from dronecore/add-camera

5c7f8b5

Added camera plugin with basic functionality


		using namespace dronecore;

		static const char *e90_unit_test_file = "extensions/plugins/camera/e90_unit_test.xml";

2500	MAV_CMD_VIDEO_START_CAPTURE	Starts video capture (recording). Use NAN for reserved values.
	Mission Param #1	Reserved (Set to 0)
	Mission Param #2	Frequency CAMERA_CAPTURE_STATUS messages should be sent while recording (0 for no messages, otherwise frequency in Hz)
	Mission Param #3	Reserved (all remaining params)

Added camera plugin with basic functionality #338

Added camera plugin with basic functionality #338

Conversation

julianoes commented Mar 23, 2018 • edited Loading

shakthi-prashanth-m left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee commented Mar 25, 2018

shakthi-prashanth-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakthi-prashanth-m Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakthi-prashanth-m Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamishwillee Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes commented Mar 23, 2018 •

edited

Loading

shakthi-prashanth-m left a comment •

edited

Loading

shakthi-prashanth-m Mar 26, 2018 •

edited

Loading

shakthi-prashanth-m Mar 26, 2018 •

edited

Loading

hamishwillee Apr 19, 2018 •

edited

Loading