Land Detector Properties instead of one State #13170
Conversation
MaEtUgR
force-pushed
the
land-detector-properties
branch
from
3d3a57f to
f6dfe90
Compare
MaEtUgR
force-pushed
the
land-detector-properties
branch
from
f6dfe90 to
383bd15
Compare
|
I rebased, now it's the real diff ready for review. |
| bool freefall # true if vehicle is currently in free-fall | ||
| bool ground_contact # true if vehicle has ground contact but is not landed (1. stage) | ||
| bool maybe_landed # true if the vehicle might have landed (2. stage) | ||
| bool landed # true if vehicle is currently landed on the ground (3. stage) |
There was a problem hiding this comment.
I like the logic ordering you did here.
| const bool freefallDetected = _freefall_hysteresis.get_state(); | ||
| const bool ground_contactDetected = _ground_contact_hysteresis.get_state(); | ||
| const bool maybe_landedDetected = _maybe_landed_hysteresis.get_state(); | ||
| const bool landDetected = _landed_hysteresis.get_state(); |
There was a problem hiding this comment.
This is a great consolidation of logic here from _update_state(). Nice work!
| .ground_contact = true, | ||
| .maybe_landed = true, | ||
| .landed = true, | ||
| }; |
There was a problem hiding this comment.
I think this was intentional and it is correct to initialize to the new values you have, but just in case it was not intended I wanted to point out the master branch current constructor initialized values:
_land_detected.ground_contact = false;
_land_detected.maybe_landed = false;
There was a problem hiding this comment.
We could leave this initializer like it was, the only place it get published is in the Run() method and there it also overwrites all of the struct content.
There was a problem hiding this comment.
I think it's good the way you have it. Just wanted to point it out in case. :)
| .maybe_landed = false, | ||
| .ground_contact = true, | ||
| .maybe_landed = true, | ||
| .landed = true, |
There was a problem hiding this comment.
Same here as previous comment above. I think these new values are the correct states to be initialized to. This looks good to me!
There was a problem hiding this comment.
Here the initialization is useful since if there was no land detected published the position controller will assume the vehicle is landed and before only one flag was able to be true so it was landed, now it makes sense that landed implies maybe_landed and ground_contact.
| .ground_contact = true, | ||
| .maybe_landed = true, | ||
| .landed = true, | ||
| }; |
There was a problem hiding this comment.
missing setting of in_ground_effect.
Not used for multicopter, but seems better to set it too.
There was a problem hiding this comment.
It's used for multicopter but in the estimator to stop fusing affected baro data into the altitude.
Only look at the last commit: 3d3a57f✔️I based this pr on a rebased version of #11874 since I don't want to generate any conflicts. It's mergable after #11874.
Describe problem solved by the proposed pull request
Internally the land detector determines certain properties of the vehicle and some depend on each other e.g. if the vehicle is landed it is also maybe landed but others don't e.g. there can be ground contact but there's still ground effect distorting the barometer. Until now there was one single state determined from all these properties using prioritization and then all the original flags were published but because there the state was in between only one could be true at once. As a result:
land_detected.ground_contact || land_detected.maybe_landed || land_detected.landedeven though the first property always holds when the others holdDescribe your preferred solution
All properties are published like they are determined. Multiple properties can hold at the same time. Interdependencies of properties are covered by their respective detection functions.
Test data / coverage
I SITL tested and multicopter land detection still works fine:
You can see well that the flags turn on one after the other instead of switching on and off again one after the other.