Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG` #351

shakthi-prashanth-m · 2018-04-06T12:00:06Z

Adds new types MAVLinkCommands::CmdInt and MAVLinkCommands::CmdLong types that abstract to those of MAVLink protocol:

Also includes the changes in core and plugins, wherever these commands are used. COMMAND_INT is used only in Gimbal plugin as of now for MAV_CMD_DO_SET_ROI_LOCATION.

Adds new types `MAVLinkCommands::CmdInt` and `MAVLinkCommands::CmdLong` types that abstract to those of MAVLink protocol: http://mavlink.org/messages/common/#COMMAND_INT http://mavlink.org/messages/common/#COMMAND_LONG Also includes the changes in core and plugins, wherever these commands are used. `COMMAND_INT` is used only in Gimbal plugin as of now for `MAV_CMD_DO_SET_ROI_LOCATION`.

julianoes · 2018-04-06T12:09:59Z

core/mavlink_commands.h

+            float param4 = NAN;
+            int32_t lat_deg = 0;
+            int32_t lon_deg = 0;
+            float alt_m = NAN;


This would be called param7 I'd say.

I just realized, we better name last 3 members as x, y and z., x could be in meters also in case of Local position. So, _deg would mislead. Right ?

x int32_t PARAM5 / local: x position in meters * 1e4, global: latitude in degrees * 10^7

y int32_t PARAM6 / local: y position in meters * 1e4, global: longitude in degrees * 10^7

z float PARAM7 / z position: global: altitude in meters (relative or absolute, depending on frame.

Ok, makes sense then

julianoes · 2018-04-06T12:18:25Z

core/mavlink_system.cpp

+    cmd.params.param1 = float(mode);
+    cmd.params.param2 = float(custom_mode);
+    cmd.params.param3 = float(custom_sub_mode);
+    cmd.target_component_id = get_autopilot_id();


I think that's actually nicer!

julianoes · 2018-04-06T12:19:06Z

core/mavlink_commands.h

@@ -30,20 +31,65 @@ class MAVLinkCommands

    typedef std::function<void(Result, float)> command_result_callback_t;

-    struct Params {
-        float v[7];
+    struct CmdInt {


I would call it CommandInt because we use command everywhere, and not cmd, right?

Right. I'll change.

cmd in argument should be fine, I think. Right ?

MAVLinkCommands::Result MAVLinkSystem::send_command(MAVLinkCommands::CommandInt &cmd) {...}

julianoes · 2018-04-06T12:19:39Z

core/mavlink_commands.h

@@ -6,6 +6,7 @@
 #include <string>
 #include <functional>
 #include <mutex>
+#include <memory>


What's this include for? Shouldn't it be in mavlink_commands.cpp?

Yeah, forgot to remove. I wanted to use unique_ptr but realized its an overkill. :)

julianoes · 2018-04-06T12:22:21Z

core/mavlink_system.h

-                                     command_result_callback_t callback,
-                                     uint8_t component_id = 0);
+    // FIXME: I tried to use templates for these;
+    // but I get undefined reference. Need to dig it later.


I think like this is fine. I'd even tend to:

send_command_int(...); send_command_long(...);

OK. I am encouraging compile-time polymorphism :-)

julianoes · 2018-04-06T12:23:23Z

plugins/action/action_impl.cpp

+    MAVLinkCommands::CmdLong cmd {};
+
+    cmd.command = MAV_CMD_NAV_LAND;
+    cmd.target_component_id = _parent->get_autopilot_id();


Note that the behavior here could potentially change because we switch from having all params as NAN to 0.f. I would vote to set all reserved as NAN.

I would vote to set all reserved as NAN.

OK.
So what should they be initialized by default ?

MAVLinkCommands:CmdInt -> MAVLinkCommands::CommandInt MAVLinkCommands:CmdLong -> MAVLinkCommands::CommandLong

In MAVLink spec for `COMMAND_INT` last 3 params are named as x, y and z. Units of these would vary from meteres (Local pos) to degrees (Global pos). Hence renamed as x, y and z.

Most of the "Reserved" values in MAVLink spec are NAN. However, in some cases "Reserved" value could be "0". This utility method supplied can be used to set any value to mark as "Reserved". For.eg: For commands `MAV_CMD_LOGGING_START`, `MAV_CMD_LOGGING_STOP` etc, "Reserved" value is 0.

Correct param name "reserved_value" in set_as_reserved().

julianoes

Looks good to me now.

…MAND_INT Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG`

shakthi-prashanth-m added core plugins feature request labels Apr 6, 2018

shakthi-prashanth-m self-assigned this Apr 6, 2018

shakthi-prashanth-m requested a review from julianoes April 6, 2018 12:00

shakthi-prashanth-m added the in progress label Apr 6, 2018

shakthi-prashanth-m changed the title ~~core, plugins: Add support for COMMAND_INT #326~~ Add support for COMMAND_INT and abstraction for the same including COMMAND_LONG Apr 6, 2018

julianoes reviewed Apr 6, 2018

View reviewed changes

Shakthi Prashanth M added 3 commits April 6, 2018 18:39

core, plugins: Rename CmdInt -> CommandInt

4e79d5d

MAVLinkCommands:CmdInt -> MAVLinkCommands::CommandInt MAVLinkCommands:CmdLong -> MAVLinkCommands::CommandLong

core, plugins: Correct param names of COMMAND_INT

c67db7a

In MAVLink spec for `COMMAND_INT` last 3 params are named as x, y and z. Units of these would vary from meteres (Local pos) to degrees (Global pos). Hence renamed as x, y and z.

shakthi-prashanth-m force-pushed the 326-add-support-for-COMMAND_INT branch from 7538d52 to e09ac5a Compare April 6, 2018 13:32

core: plugins: Rename cmd -> command for consistency

eeece31

Correct param name "reserved_value" in set_as_reserved().

shakthi-prashanth-m requested a review from JonasVautherin April 6, 2018 14:40

julianoes approved these changes Apr 9, 2018

View reviewed changes

julianoes merged commit 849c3ec into develop Apr 9, 2018

julianoes deleted the 326-add-support-for-COMMAND_INT branch April 9, 2018 07:59

julianoes removed the in progress label Apr 9, 2018

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

Support for COMMAND_INT #326

Closed

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#351 from dronecore/326-add-support-for-COM…

cb41f94

…MAND_INT Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG`

julianoes added feature and removed feature request labels Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG` #351

Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG` #351

shakthi-prashanth-m commented Apr 6, 2018

julianoes Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018

julianoes Apr 9, 2018

julianoes Apr 6, 2018

julianoes Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018

julianoes Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018

julianoes Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018 •

edited

Loading

julianoes Apr 9, 2018

julianoes Apr 6, 2018

shakthi-prashanth-m Apr 6, 2018 •

edited

Loading

julianoes left a comment

x	int32_t	PARAM5 / local: x position in meters * 1e4, global: latitude in degrees * 10^7
y	int32_t	PARAM6 / local: y position in meters * 1e4, global: longitude in degrees * 10^7
z	float	PARAM7 / z position: global: altitude in meters (relative or absolute, depending on frame.

Add support for COMMAND_INT and abstraction for the same including COMMAND_LONG #351

Add support for COMMAND_INT and abstraction for the same including COMMAND_LONG #351

Conversation

shakthi-prashanth-m commented Apr 6, 2018

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 Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakthi-prashanth-m Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

julianoes left a comment

Choose a reason for hiding this comment

Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG` #351

Add support for COMMAND_INT and abstraction for the same including `COMMAND_LONG` #351

shakthi-prashanth-m Apr 6, 2018 •

edited

Loading

shakthi-prashanth-m Apr 6, 2018 •

edited

Loading