param: changes for making param set/get work with ArduPilot #1703

ykhedar · 2022-02-21T12:30:27Z

This pull request contains the minimal necessary changes needed in the MAVSDK to make param set/get work with Ardupilot. Following the discussion from #1683, I decided to try the suggestion B from @julianoes

B) Always download all params on startup to then have the type cached when the function is used. This would take longer but make the API easier. What do you think?

I had to, however create a different function to deal with the param set for APM

float MAVLinkParameters::ParamValue::get_4_float_bytes() const
float MAVLinkParameters::ParamValue::get_4_float_bytes_apm() const

As mentioned before, Ardupilot likes to have direct cast to float instead of the re-interpret cast used in MAVSDK for PX4 param set. More details can be found in https://mavlink.io/en/services/parameter.html#ardupilot

Awaiting valued feedback on this.

src/mavsdk/core/mavlink_parameters.cpp

src/mavsdk/core/system_impl.cpp

JonasVautherin · 2022-02-21T13:31:56Z

src/mavsdk/core/system_impl.cpp

@@ -767,15 +791,29 @@ std::pair<MAVLinkParameters::Result, int> SystemImpl::get_param_int(const std::s
    auto res = prom.get_future();

    MAVLinkParameters::ParamValue value_type;
-    value_type.set<int32_t>(0);
-
+    //value_type.set<int32_t>(0);


Suggested change

//value_type.set<int32_t>(0);

src/mavsdk/core/system_impl.cpp

JonasVautherin · 2022-02-21T21:19:44Z

The CI is not happy, and you should fix the style with something like ./tools/fix_style.sh . 👍 😊

julianoes · 2022-03-10T04:31:47Z

@ykhedar I've rebased your PR, fixed style issues, and force pushed it to your branch.

I also added a test to check get_all_params:

cmake --build build/default/ -j6 && build/default/src/integration_tests/integration_tests_runner --gtest_filter="SitlTest.GetAllParams"

(For ArduPilot, don't forget to change add_udp_connection() to add_udp_connection(14550).

The test fails for me for me for ArduPilot,

[05:29:00|Info ] Int params: 0 (param.cpp:149)
[05:29:00|Info ] Float params: 0 (param.cpp:155)
[05:29:00|Info ] Combined params: 0 (param.cpp:161)

and PX4:

[05:30:13|Error] Error: unknown mavlink param type:  (mavlink_parameters.cpp:965)
[05:30:13|Error] Error: unknown mavlink param type:  (mavlink_parameters.cpp:965)
[05:30:14|Info ] Int params: 0 (param.cpp:149)
[05:30:14|Info ] Float params: 0 (param.cpp:155)
[05:30:14|Info ] Combined params: 0 (param.cpp:161)

so something is not quite right yet.
It works for PX4 before that.

One thing that I think of changing is to only get the param that you want to set. That way we don't have to get all params which is not necessarily something that everyone wants to do, always.

julianoes · 2022-03-11T07:12:14Z

FYI @ykhedar I'm currently reworking this.

ykhedar · 2022-03-11T08:32:39Z

@ykhedar I've rebased your PR, fixed style issues, and force pushed it to your branch.

I also added a test to check get_all_params:
cmake --build build/default/ -j6 && build/default/src/integration_tests/integration_tests_runner --gtest_filter="SitlTest.GetAllParams"
(For ArduPilot, don't forget to change add_udp_connection() to add_udp_connection(14550).

The test fails for me for me for ArduPilot,
[05:29:00|Info ] Int params: 0 (param.cpp:149)
[05:29:00|Info ] Float params: 0 (param.cpp:155)
[05:29:00|Info ] Combined params: 0 (param.cpp:161)
and PX4:
[05:30:13|Error] Error: unknown mavlink param type:  (mavlink_parameters.cpp:965)
[05:30:13|Error] Error: unknown mavlink param type:  (mavlink_parameters.cpp:965)
[05:30:14|Info ] Int params: 0 (param.cpp:149)
[05:30:14|Info ] Float params: 0 (param.cpp:155)
[05:30:14|Info ] Combined params: 0 (param.cpp:161)
so something is not quite right yet. It works for PX4 before that.

One thing that I think of changing is to only get the param that you want to set. That way we don't have to get all params which is not necessarily something that everyone wants to do, always.

Hi @julianoes Thanks for taking a look at the PR. I am not able to work on it for at least a week more due to the other project so please go ahead and rework if you have the time.

Last time I checked the tests not passing for PX4 set param was most probably because the params were not being cached at the right time. I was looking into it but then dragged into the other project.

I believe caching all params on first connection is not a bad idea. I have seen this behaviour in Mavproxy and really like that, as soon as it is connected to the vehicle it saves all params to a a file. Since MAVSDK is usually connected over a wire to the Autopilot, should not be a huge problem speed or lost packet wise. What do you think?

I don't think we have to translate it to a string to then convert it back to an int. We can just assume the bytes sent are float, and then cast them to int.

This changes the param class quite a bit. This is a step towards better compatibility with the MAVLink spec, specifically support of int types other than int32_t, e.g. uint8_t. Since the MAVSDK API only offers int and float params, we need to handle these other int types internally. This is done by looking up the param in the cache if available, and if not a param_get is done before doing a param set. With these changes we should now also correctly translate float to int and int to float for the two implementations, so bytewise copying for PX4 and casting to float for ArduPilot.

ykhedar · 2022-04-09T22:57:41Z

@julianoes There was a small bug(wrong function name) which was causing issues for CI. I fixed it in my last commit and CI seems to be happy now. Are you still working on something for this pull request or we can rebase and merge it into the main branch?

julianoes · 2022-04-13T03:54:44Z

@ykhedar I'm having a look now, sorry for the delay.

julianoes · 2022-04-13T03:59:52Z

@ykhedar thanks for fixing that, I was wondering what was going on!

julianoes

Reviewed again, let's get this in and start using it.

julianoes · 2022-03-10T02:58:44Z

src/mavsdk/plugins/param/param_impl.cpp

@@ -19,7 +19,7 @@ ParamImpl::~ParamImpl()
    _parent->unregister_plugin(this);
 }

-void ParamImpl::init() {}
+void ParamImpl::init() { _parent->get_all_params(); }


This is actually a fundamental change. This will now always get all params even though we might not need them all. While I'm not opposed to this in the long term, I would want to introduce it carefully and do some testing first.

ykhedar requested review from bazfp, julianoes, JonasVautherin and TSC21 February 21, 2022 12:30

JonasVautherin requested changes Feb 21, 2022

View reviewed changes

JonasVautherin reviewed Feb 21, 2022

View reviewed changes

src/mavsdk/core/system_impl.cpp Outdated Show resolved Hide resolved

julianoes force-pushed the apm-devel branch from 7079099 to b1e14be Compare March 10, 2022 04:26

julianoes force-pushed the apm-devel branch 2 times, most recently from 4daa8d2 to 4abfa7e Compare March 15, 2022 03:50

ykhedar and others added 12 commits March 23, 2022 17:42

param: changes for making param set/get work with ArduPilot

0fc2d49

modified all param caching and implemented suggestions of reviewers.

d04235f

core/plugins: fix style

0ecfda8

core: remove commented out code

4a4cdaf

integration_tests: add test to get all params

ed34893

core: small improvement to avoid uninit

d86b486

core: simplify int param conversion

6c6bf58

I don't think we have to translate it to a string to then convert it back to an int. We can just assume the bytes sent are float, and then cast them to int.

core: rename methods to explain what they do

054399b

core: support all mavlink types that fit in 32bit

c89f644

plugins: parampair -> param_pair (drive-by)

2ef6447

core: add missing include

0265865

julianoes force-pushed the apm-devel branch from 4abfa7e to 0265865 Compare March 23, 2022 04:42

param: corrected async func for get_param_float()

6be007a

ykhedar requested a review from JonasVautherin April 9, 2022 22:45

julianoes approved these changes Apr 13, 2022

View reviewed changes

julianoes added the enhancement label Apr 13, 2022

julianoes merged commit 0c13d0f into mavlink:main Apr 13, 2022

ykhedar mentioned this pull request Apr 14, 2022

plugin: corrected support for basic actions for ardupilot #1738

Merged

julianoes mentioned this pull request Aug 30, 2022

Parameters - setParamFloat works but setParamInt does not mavlink/MAVSDK-Swift#215

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

param: changes for making param set/get work with ArduPilot #1703

param: changes for making param set/get work with ArduPilot #1703

ykhedar commented Feb 21, 2022

JonasVautherin Feb 21, 2022

ykhedar Feb 21, 2022

JonasVautherin commented Feb 21, 2022

julianoes commented Mar 10, 2022

julianoes commented Mar 11, 2022

ykhedar commented Mar 11, 2022

ykhedar commented Apr 9, 2022

julianoes commented Apr 13, 2022

julianoes commented Apr 13, 2022

julianoes left a comment

julianoes Mar 10, 2022

param: changes for making param set/get work with ArduPilot #1703

param: changes for making param set/get work with ArduPilot #1703

Conversation

ykhedar commented Feb 21, 2022

JonasVautherin Feb 21, 2022

Choose a reason for hiding this comment

ykhedar Feb 21, 2022

Choose a reason for hiding this comment

JonasVautherin commented Feb 21, 2022

julianoes commented Mar 10, 2022

julianoes commented Mar 11, 2022

ykhedar commented Mar 11, 2022

ykhedar commented Apr 9, 2022

julianoes commented Apr 13, 2022

julianoes commented Apr 13, 2022

julianoes left a comment

Choose a reason for hiding this comment

julianoes Mar 10, 2022

Choose a reason for hiding this comment