land detector and hysteresis cleanup

[dagar]

A large portion of the intermittent CI failures seem to be related to the land detector.

Debugging...

[dagar]

The VTOL land detector airspeed filtered is occasionally getting stuck infinite.

http://ci.px4.io:8080/blue/organizations/jenkins/Firmware/detail/PR-9157/38/pipeline

INFO  [land_detector] _airspeed_filtered: inf

@lamping7 FYI

[Stifael]

These if statements are probably not what you want.
You will get an PX4_ERR message every single time the get_..._state() is true but the corresponding hysteresis not.

[dagar]

@lamping7 20/20 full CI passes.

[dagar]

These if statements are probably not what you want.

Yes, it was crappy debugging code to try and catch something that was only failing in the CI system intermittently.

[dagar]

@Stifael what do you think about publishing each land detector state separately like this? I did it for debugging a failure, but I'm wondering if it's worth keeping.

[dagar]

Airspeed finite checks removed from this PR and merged in #9163.

[dagar]

For the land detector I'd still like to discuss the need to hold and publish a single state at a time.

https://github.com/PX4/Firmware/blob/25300a6b11ae5d363b2eafe24718d44c454a859e/src/modules/land_detector/LandDetector.cpp#L195-L209

Internally the land detector is a number of different booleans (landed, maybe_landed, ground_contact, freefalling), and it publishes the same booleans, but only one active at a time.

The vehicle_land_detected message should either publish all of these booleans or a single a state.

debugging code removed

[lamping7]

It makes sense to have it as you have done here because for MC the only thing stopping multiple states from being active at once is the loosely coupled check for the previous state at the end of each respective state getter, excluding freefall which is seperate. It could help future debugging if something changes for the worse.

Changing the message to send one value for the state could be done with refactoring to other modules that use the message (Commander and mc pos ctl), but the state machine that is the land detector should probably be modified to ensure only one can be active, with tighter transition control. On the other hand much of this makes no sense for fixed wing, or even rover which serves no purpose at all.

[Stifael]

The vehicle_land_detected message should either publish all of these booleans or a single a state.

I vote for all. If the vehicle is landed, then for sure it is also maybe landed and also has ground contact and therefore it makes sense to capture that in the code implementation as well.
In addition, I remember that at some point the landdetector was planned to become a vehicle-model specific module. If that is still the case, then I think it makes even more sense to allow multiple boolean states to be true at the same time.
I think for MC it would require a few changes outside of the landdetector.

[lamping7]

A downside to doing this, showing ground contact and maybe landed, is that they need to be checked in the correct order in the mc pos controller, altering thrust, which is not clear unless you know how this works.

[Stifael]

That should not be a problem. During ground contact the thrust in xy is turned off, and during maybe landed the entire thrust vector is set to 0. This means it does not even require an if-elseif statement

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lamping7]

Has this been superseded?