param_server: implemented change_param_? functions

[dayjaby]

The implementation for mavlink/MAVSDK-Proto#317

Tested in the autopilot_server example, after adding a change_param_int for MY_PARAM from 1 to 2:

Found Param MY_PARAM: 2

Not tested whether other connected mavlink instances receive a PARAM_VALUE after a parameter change.

[dayjaby]

Rebasing against main. @julianoes But tools/generate_from_protos.sh && tools/fix_style.sh wants to change back all the grpc.pb.cc/grpc.pb.h files. How did you generate with your most recent change?

Edit: Nevermind! Just removed the build folder and rebuild MAVSDK completely. Now it works.

[JonasVautherin]

I am sorry if that is a stupid question, but I need to ask it again 🙈:

Given that the change_server_param* functions have exactly the same signature as the provide_server_param* ones, and given the fact that change_server_param anyway has to check if the param already exists or not...

... why can't we keep the current API and just "update" the param if provide is called a second time?

So one could call:

provide_param_int("SYS_HITL", 0); // This initializes the value the first time
[...]
provide_param_int("SYS_HITL", 1); // This changes the value (which was previously set to 0)

Why is that not possible? 🤔

To me it's super weird to say "if you call provide_param twice for the same param, you'll get an error that you need to handle, and if you call change_param on a param that was not previously "provided", then you'll get another error that you need to handle".

[dayjaby]

1. To make the intent more clear: "Provide" does something different than "change".
2. To not allow any late accidental parameter "providing", for example if you try to change a parameter that did not exist before (so you rather enjoy having to deal with a ParamNotFound result)

[JonasVautherin]

1. I would debate that, but I'm known for not being a good reference for what is "clear", so I'll let @julianoes decide. My opinion is that it does the same thing: it assigns a value to the parameter. When programming, I don't usually think about the difference between "initialization" and "assignment", I just use = both times:

int i = 3;
i = 5;

It would not be clearer to me to do int i = provideInt(3); and then changeInt(i, 5); 😇.

2. Yeah maybe that's a technical reason to do it like this. For my understanding: can't provide* detect that and throw a ParamNotFound error in that case? What currently happens if provide* is called late? Does it silently fail?

BTW is that a MAVLink limitation? It is a bit sad to not be able to add a param at runtime 😕.

[dayjaby]

BTW is that a MAVLink limitation? It is a bit sad to not be able to add a param at runtime

Quote from https://mavlink.io/en/services/parameter.html#parameters_invariant: "The protocol requires that the parameter set does not change during normal operation/after parameters have been read."

That page can explain it better than me, so definitely worth a read 😄

[JonasVautherin]

That page can explain it better than me

Yeah I don't think that there is a reasonable technical reason for that, it's just the way it is designed, I suppose 🤷.

Anyway if there is no other way, I am fine for the change_param*, but if technically it's feasible with provide_param* (e.g. because provide_param* can detect late init somehow), then IMO it would make the API nicer to only use provide_param.

But again, let's @julianoes be the judge of that 🙈

[julianoes]

I need to have a closer look at provide_param, it should be possible. The question is what is clearer, more intuitive as an API. I could go either way on that...

[julianoes]

How did you generate with your most recent change?
Edit: Nevermind! Just removed the build folder and rebuild MAVSDK completely. Now it works.

I had updated all the third party dependencies in the meantime: #2069.

[julianoes]

Yeah I don't think that there is a reasonable technical reason for that,

@JonasVautherin the technical reason, from what I understand, is that you query all parameters using a "num_params" variable. So, once you start a transfer, you can't have the indices change from under you.

I suppose you're saying that this "num_params" variable should only be valid for the time of a transfer. That way you could change it later.

[JonasVautherin]

I suppose you're saying that this "num_params" variable should only be valid for the time of a transfer. That way you could change it later.

Yes, I was just saying that the specs say that the number of params cannot change, but I can imagine a protocol that allows for transferring an arbitrary number of variables reliably 🙈. That's why it was not obvious to me at first that MAVLink would have this limitation.

[dayjaby]

I vote against turning MAVSDK into a "non-compliant component" and just add the change function to the param_server 😉

[JonasVautherin]

Sure, I'm all for honoring the specs, that's not what I meant 🤣. Those are still two independent problems, though, and the MAVLink specs don't imply the MAVSDK API in this case.

Fine if you guys think that splitting provide and change is clearer. But if you think that it is necessary because of the MAVLink specs, then I disagree. That's all I'm saying 🙈.

And to me it seems like the intent in the current implementation was to reuse provide: https://github.com/mavlink/MAVSDK/blob/main/src/mavsdk/core/mavlink_parameter_server.cpp#L97

[julianoes]

🤷‍♂️

[julianoes]

I think we need to catch the case where a provide is misused and actually changes the param, right? And we should make it really clear in the proto docstring what the difference between provide and change is!