Improve mavlink parameters after split

[Consti10]

This builds up on the work already done by @julianoes in https://github.com/mavlink/MAVSDK/pull/1772

And replaces this pull request: https://github.com/mavlink/MAVSDK/pull/1807

Goals: Refactor mavlink parameters to reduce duplicated code, eliminate bugs(relates to https://github.com/mavlink/MAVSDK/issues/1805) and document the code without changing the public api's.

In general, the work flow for refactoring is:

1. Implement a generic way (for example a templated retrieve_server_param ) method
2. refactor the fixed type methods (for example retrieve_server_param_float, retrieve_server_param_int, ...

Not all of the refactorings are that simple, but in general the workflow is the same.

[julianoes]

Note that I changed the base branch to my PR, so only your diff is visible here.

[Consti10]

So right now my goal is to make the server / provider exactly adhere to all functionalities of the mavlink protocoll (as already said earlier, the get_by_index instead of get_by_string_id part was/ is missing. While I am not sure if we will ever get the client then that far (there it really really gets complicated) that'd still mean though that other libraries who perhaps use features (like get param by index) can work with it.

I also found out that after all the refactoring, on the providing side the WorkItem has basically become a re-implementation of the mavlink Value / Ack messages.

We can also just as well create the mavlink responses as mavlink messages, then push the message onto the work queue. This makes the code less error prone, since we create the response right at the place where we have all the data available, instead of basically creating a copy of the data that is then needed for the mavlink message, and creating the mavlink message on the work queue.

[Consti10]

So after running around in circles multiple times, I think the issue with the param_count/param_index for extended vs non-extended is actually not really solvable with how the current public api is written.

In short, if you look at the server / param provider - The public api does not protect against the following issues:

1. The parameter set might change after initial operation, and there is no way to use the server in a safe way to avoid this => I think the only way to solve this issue is to change the public api such that either provide_server_param() is removed and the parameter set is provided in the constructor, or we add a public method in the likes of ready_for_communication() - note that both these solutions are breaking downwards compability, since by default ready_for_communication must be off

2. It is impossible to avoid changing the indices of parameters when talking both extended and non-extended protocoll - I don't think it is a good idea to mix these 2 up. It would be possible to just provide a immutable flag when creating the server and/or client (use_extended) and then the client / server either speaks extended or non-extended protocol, but not both.

[julianoes]

ready_for_communication()

I think that would be a valid approach. It's indeed a bit of a problem with params, and in PX4 this is done similar, in fact.

[Consti10]

As you might have noticed from the commits, I am getting to a point where the Server / parameter_receiver is almost ready and kept the following feature:

1. Talking to clients using either the extended or non-extended protocoll (in case of a client using the non-extended protocoll, if the user added any string parameters, they are hidden

while adding: 2) support for requests using param_index instead of param_id (this allows clients safe & quaranteed parameter synchronization over lossy links, what the previous implementation was lacking) and also 3) adding more safety features, following the mavlink specification(s) (for example ignoring messages with the wrong target system / component usw) and more.

Unfortunately, these changes make it incompatible with previous mavsdk releases - most notably the target component id.

This is because the previous implementation did not follow the mavlink quideline(s) on target component id's. I only noticed that basically there is a brain twist assigned with mavlink_parameter_sender - in the form it is used in mavsdk, it is one parameter sender supposed to talk to one or more components on a system. However, it is impossible to achieve parameter synchronization this way, as well as there is the risk of param_id clashes from multiple components on the same system.

I was tricked in this regard by the get_all_params() in the public api of mavlink_parameter_sender - I assumed that would return the full parameter set from a specific server (that is a server with his specific system and component id ) while in reality, it would do something completely else (basically undefined unless used in a testing environment with no packet loss and no more than 1 param component per system).

So the pickle is the following: What I need is both a parameter server and a parameter client that - when used on air and ground - supports:

1. parameter synchronization over lossy links
2. changing parameters.
3. a single param server / client per component.

And that's why I was working on both param receiver / sender.

However, without changing the public api's of mavsdk parameter_server / client (and making them basically incompatible with previous releases) 1) and 3) cannot be achieved.

What do you say ? For us, unfortunatley - since 1) and 2) are hard requirements, we can only use mavsdk if the public api is changed such that 1) and 2) are fulfilled.

[Consti10]

Here is a quick list of the changes that need to be done to the public api (so to say):

1. For the server: None other than what I already mentioned in regards to the invariant parameter set

2. for the client: a) Each client needs to know the target component id ( aka which component he talks to) as well as weather to use the extended protocoll or not on construction / before he can be used. b) if either publically or mavsdk-internally you need to talk to more than one parameter server component, you need multiple instances of mavlink_parameter_sender

[Consti10]

2 more simple questions:

1. How do I create a simple unit test for the mavlink core libary (I want to test mavlink_paramater_xxx directly without going through the public api route).

2. How do I use the mavlink_passthrough plugin in the "system_tests" to emulate packet loss ? It doesn't compile.

[Consti10]

2 more simple questions:

1. How do I create a simple unit test for the mavlink core libary (I want to test mavlink_paramater_xxx directly without going through the public api route).
2. How do I use the mavlink_passthrough plugin in the "system_tests" to emulate packet loss ? It doesn't compile.

Found the unit test(s)

[julianoes]

Just to make sure, is the ID numbering now like this?

Normal params:

WIDGET_SO  0
WIDGET_BLA 1

Extended params:

GADGET_FOO 0
GADGET_PI  1
GADGET_PA  2
GADGET_PO  3

I believe that's how it should be, right?

[Consti10]

There are quite a few changes here by now which makes it hard to review...

I think it would be good if we can get CI passing and then merge it into my PR, and then merge mine (both) into main and start to use and test it.

Don't forget to fix style:

tools/fix_style.sh .
# or
tools/run-docker.sh tools/fix_style.sh .

So the issue I have right now is the following: MASDK (neither in the public api or in the core implementation) does not differentiate between components on a system (at least in the system_impl, aka not the server side).

Which makes it unusable for me in its current form (I cannot change parameter X of component Y on system Z, I can only change parameter X of (any component) on system Z.

Like already said, this also makes all parameters synchronization impossible - at least as soon as a system has more than 1 component that speaks the param protocoll. And it also makes it unsafe to use on systems with more than 1 (param) component per system - since who quarantees that param_id 's are unique per system (they only need to be unique per component).

The only fix (in my eyes) woul be to create comonent_imp.cpp and have all things (like parameter sender, but also command sender for examle) in there. But that is a bit out of scope for me.

Since I already invested so much time into a mavsdk compatible impl. of param server / client I am probably going to "hack one peram sender per component" into the system_impl.cpp and use that for now. But I cannot say if I have the resources to then clean that up & merge it into main mavsdk.

[julianoes]

@Consti10 thanks. I will have to think about what you wrote. I hope we can find away to enable your use case.

[Consti10]

@Consti10 thanks. I will have to think about what you wrote. I hope we can find away to enable your use case.

Thanks, that's nice to hear. I think it would be great for MAVSDk to fully support a seperation by components, this is nothing special of OpenHD, but rather a "feature" of mavlink itself OpenHD needs to use (and MAVSDK - probably for simlicity - so far doesn't really differentiate there).

Btw, the behaviour how a param server / component should behave regarding sys and comp id is well documented by mavlink, and here are my current implementation(s) for param sender / receiver: https://github.com/OpenHD/MAVSDK/blob/pr-improve-mavlink-parameters-after-split-exp-custom-flavour/src/mavsdk/core/mavlink_parameter_receiver.h#L174 and https://github.com/OpenHD/MAVSDK/blob/pr-improve-mavlink-parameters-after-split-exp-custom-flavour/src/mavsdk/core/mavlink_parameter_sender.h#L271

Once could even propagate this all the way up to the MavlinkMessageHandler , see https://mavlink.io/en/guide/routing.html#c-library-mavgen excerpt: Specifically, the mavlink_msg_entry_t structure contains flags to tell you if the message contains target system/component information (FLAG_HAVE_TARGET_SYSTEM, FLAG_HAVE_TARGET_COMPONENT)

[julianoes]

@Consti10 I finally spent some time today to continue here. Sorry for the delay. First step was to rebase my PR and yours on top of it. I had a hard time with a few conflicts, so I decided to squash your (many!) commits, I hope that's ok.

Since I don't have permission to this PR, I'm closing this one and manually merging it into https://github.com/mavlink/MAVSDK/pull/1772.

Thanks again for all your great work here! I have some different opinions on some of the pieces but I think most of your work is very useful, also some of the comments.

And lastly, I think I found a way to support your use case with multiple components that need to be addressed. :)