Flight task auto params #9287
Flight task auto params #9287
Conversation
| param_t MPC_TILTMAX_AIR_h { PARAM_INVALID }; | ||
| param_t MPC_MAN_TILT_MAX_h { PARAM_INVALID }; | ||
| DEFINE_PARAMETERS( | ||
| (ParamFloat<px4::params::MPC_THR_MAX>) MPC_THR_MAX, |
There was a problem hiding this comment.
I strongly suggest you stick to the usual naming pattern for members: something like _maximum_thrust. Otherwise we have another inconsistency in the code-base.
However it's not part of this PR.
There was a problem hiding this comment.
I discussed that with @MaEtUgR, and we came to the conclusion that it is more confusing to redefine the parameter names. For instance, the parameter in one module can have a complete different name in another module. In addition, from the name it is also not clear who is the owner of that parameter.
Reusing the name of the metadata removes the consistent redefinition of parameter names...
There was a problem hiding this comment.
That needs a bigger upstream discussion then, you cannot simply make that decision in an unrelated PR. And if decided someone needs to go through the codebase and update all other places.
In addition, there are also reasons against this, for example:
- allows to use more descriptive variable names, since params are restricted to 16 chars.
- naming consistency with other other class members: after all, a parameter is just a class member like any other
- naming conflicts/inconsistencies with macros/globals: typically macros or globals use all uppercase. This is also confusing for newcomers
There was a problem hiding this comment.
That needs a bigger upstream discussion then, you cannot simply make that decision in an unrelated PR
I don't expect that this PR (= base of this PR) gets merged soon, but you are right, probably these param name changes should not be added to this PR. In the meantime I will initiate a discussion about parameter naming.
after all, a parameter is just a class member like any other
Not really. There should be a distinction between class members that change regularly (like setpoints) and class members that are parameters which do not change frequently.
naming conflicts/inconsistencies with macros/globals: typically macros or globals use all uppercase. This is also confusing for newcomers
I am sure that can be easily solved by adding a px4 convention page in the px4 devguide. I think the fact that there is no formal px4 convention page is the root cause of the confusion because a beginner has no chance to know what is legacy code vs expected px4 implementation.
There was a problem hiding this comment.
I am sure that can be easily solved by adding a px4 convention page in the px4 devguide. I think the fact that there is no formal px4 convention page is the root cause of the confusion because a beginner has no chance to know what is legacy code vs expected px4 implementation.
That sounds like a good idea. Probably as a sub-page of Contribution. All we have at the moment is that you should use astyle for formatting.
Can you (or @bkueng ) create an issue outlining the main conventions you are interested in - in bullet point form and ideally with examples. We can then drag in others and iterate.
| * This method should only be called if parameters | ||
| * have been updated. | ||
| */ | ||
| void overwriteParams(); |
There was a problem hiding this comment.
Can you make ModuleParams::updateParams virtual, and override it here instead?
There was a problem hiding this comment.
sure, that could work as well
| MPC_MAN_TILT_MAX = math::radians(MPC_MAN_TILT_MAX); | ||
|
|
||
| // Tilt needs to be in radians | ||
| MPC_TILTMAX_AIR.set(math::radians(MPC_TILTMAX_AIR.get())); |
There was a problem hiding this comment.
This is prone to errors: if for some reason overwriteParams is not called, or several times, or you use MPC_TILTMAX_AIR.commit(), you will have completely wrong values. I suggest using a separate class member instead (I used the _deg suffix to make clear which one is in degrees). Not super nice, but much less error prone.
Stifael
force-pushed
the
FlightTask-Auto
branch
from
aaf1648
to
b705ef5
Compare
…h ModuleParams type
…ethod that overwrites parameter values
Stifael
force-pushed
the
FlightTask-Auto-params
branch
from
0a6d0fb
to
932104a
Compare
@bkueng can you have a look?