FlightTasks: Stick library for auto land #15325
Conversation
There was a problem hiding this comment.
LGTM.
Just from a general design perspective, it worries me a little that we aren't evaluating explicitly whether the sticks are valid when we are using them. Instead we implicitly expect them to internally timeout, and give a 0 response. I made a suggestion which goes towards this pattern, while I agree it doesn't change anything right now, it's the kind of defensive programming which prevents future bugs. But this is more of a general pattern, and this isn't necessarily the best example.
| if (speed < land_speed * 0.5f) { | ||
| speed = land_speed * 0.5f; | ||
| // user input assisted land speed | ||
| if (_param_mpc_land_rc_help.get()) { |
There was a problem hiding this comment.
Don't modify if the sticks aren't good
| if (_param_mpc_land_rc_help.get()) { | |
| if (_param_mpc_land_rc_help.get() && _sticks.evaluateSticks(_time_stamp_current)) { |
There was a problem hiding this comment.
I actually want to do exactly this. I must have postponed it because of the implementation order. We should definitely add it, thanks for pointing out!
There was a problem hiding this comment.
I added this to the last commit (the rest is unchanged).
MaEtUgR
force-pushed
the
stick-library-for-auto-land
branch
from
9d90c9a to
0dbffac
Compare
GitHub apparently changed the base to master because the original base-branch was merged but it didn't understand that rebasing without conflict is possible so I did exactly that manually. ✔️ |
| // user input assisted land speed | ||
| if (_param_mpc_land_rc_help.get()) { | ||
| _sticks.evaluateSticks(_time_stamp_current); | ||
| speed = land_speed + _sticks.getPositionExpo()(2) * land_speed; |
There was a problem hiding this comment.
If the behavioral change is intended, don't forget to update the description of the parameter MPC_LAND_RC_HELP
There was a problem hiding this comment.
I just realized that the change makes is potentially quite a bit slower depending on the configuration. Let's preserve the possibilities.
There was a problem hiding this comment.
I'm not sure if we really want to enable such a high downwards speed close to ground. I refactored the implementation to be more clear and adjusted the parameter description. It's the last commit (the rest is unchanged).
| @@ -53,7 +53,7 @@ class Sticks : public ModuleParams | |||
| Sticks(ModuleParams *parent); | |||
| ~Sticks() = default; | |||
|
|
|||
| void evaluateSticks(hrt_abstime now); ///< checks and sets stick inputs | |||
| bool evaluateSticks(hrt_abstime now); ///< checks and sets stick inputs | |||
There was a problem hiding this comment.
We could also rename it checkAndSetStickInputs and get rid of the comment.... :D
There was a problem hiding this comment.
I did that also to the other function, see last commit. Also rebased on master since CI is succeeding again. Sadly I lost the review and am not allowed anymore to override and merge 🤷
Before the autohority was only enough to slow down the descend but not stop to zero vertical velocity.
MaEtUgR
force-pushed
the
stick-library-for-auto-land
branch
from
7b9376d to
30855c4
Compare
Describe problem solved by this pull request
This is a follow-up to #15324 but I didn't want to overload the original pr.
The auto task uses the sticks since #12220 to adjust the land speed during final descend. It did so in a
FlightTaskManualindependent way duplicating the functionality.Describe your solution
Stickslibrary from FlightTasks: Move stick handling into library, remove FlightTaskManual #15324Stickslibrary I had to untangle the landing gear logic since you don't want to override the landing gear automatically set by the land mode using your RC.Describe possible alternatives
A clear and concise description of alternative solutions or features you've considered.
Test data / coverage
I tested it in SITL but I did not have a landing gear in my tests so please review that logic thoroughly.