multicopter land detector ground contact fixes #15083
Conversation
|
Thanks for applying this issue!
|
I think acting on user intent like this is interesting. We've actually already discussed this idea in the context of a unified navigator + flight_tasks. How do you handle lower level manual modes or a crash?
@MaEtUgR thoughts?
Yes this has come up before, especially with the cheap vl53l1x sensors that have a very low minimum range. We'd mainly need to get the architecture sorted so we can automatically use the appropriate sensor. |
I currently don't handle lower level manual modes or a crash, but it can be added to conditions that "allows" ground contacts. |
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
2 times, most recently
from
40e6224 to
7dc9ca8
Compare
dagar
changed the title
|
Flew in Stabilized, Altitude hold, position hold, loiter without issues, to finish the flight I activated RTL, the vehicle returned and landed, but it didn't disarm, even when going minimum throttle, minimum yaw. I kept minimum throttle and minimum yaw and eventually it disarmed. Vehicle: S500 with Pixhawk 4. Log: https://review.px4.io/plot_app?log=a19cdf1a-c113-4cd0-a5e1-15161446cdbb |
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
2 times, most recently
from
fad8673 to
6a61932
Compare
|
The changes I made should address the problem @nrogelio encountered (MPC published a local position setpoint with NAN). I've also reviewed some of the older problematic logs. Needs more testing + careful log review. |
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
from
6a61932 to
10d5c42
Compare
@julianoes yes it's a bit weird, but I think of it as "handing off" from ground contact -> maybe_landed -> landed where we can relax the ground contact requirements as soon as maybe_landed (or if already landed). Structurally I really don't like the interplay between these states, but that line has already been crossed with things like the |
Thanks @BazookaJoe1900, and I do agree about the throttle going to 0 immediately, that's just a slightly more intrusive change that needs to be introduced in the multicopter position controller. |
There was a problem hiding this comment.
I had a closer look and although the code makes sense and the cleanup and logic improvements seem to make sense I cannot predict for all cases what it changes. Since we at some point need to overcome this uncertainty I suggest we progress when we have tested it.
| @@ -113,6 +113,14 @@ void MulticopterLandDetector::_update_params() | |||
| param_get(_paramHandle.minManThrottle, &_params.minManThrottle); | |||
| param_get(_paramHandle.landSpeed, &_params.landSpeed); | |||
|
|
|||
| if (_param_lndmc_z_vel_max.get() > _params.landSpeed) { | |||
There was a problem hiding this comment.
Minor thought: A buffer between the two parameters might be desired.
Might not be necessary because of const float land_speed_threshold = 0.9f * math::max(_params.landSpeed, 0.1f);.
There was a problem hiding this comment.
LNDMC_Z_VEL_MAX is actually another parameter I suspect we could live without.
Typically it's the land speed threshold constraining the max climb rate. max_climb_rate = math::min(land_speed_threshold * 0.5f, _param_lndmc_z_vel_max.get());
The other case is for 2 seconds (LAND_DETECTOR_LAND_PHASE_TIME_US) after landed the max_climb_rate is set to 2.5 x LNDMC_Z_VEL_MAX. Wouldn't the land speed threshold work here as well?
There was a problem hiding this comment.
Wouldn't the land speed threshold work here as well?
Probably, honestly that was there since before I first did any changes and I only preserved it.
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
from
286bf3c to
35466aa
Compare
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
2 times, most recently
from
f707ab3 to
321037f
Compare
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
3 times, most recently
from
cb69a60 to
2cde99f
Compare
- if "landed" and "maybe_landed" states are false then both the "hit_ground" and the "low_thrust" condition need to be true in order to detect landing - ground contact MC NAN setpoint workaround - ground contact additionally check acceleration setpoint - schedule with vehicle_local_position updates (most updates require valid local position) - don't allow LNDMC_Z_VEL_MAX to exceed MPC_LAND_SPEED - ground contact horizontal and vertical movement checks default to failed if estimates aren't available
dagar
force-pushed
the
pr-land_detector_mc_ground_contact
branch
from
2cde99f to
0c8a759
Compare
There was a problem hiding this comment.
@dagar Very nice cleanup and improvements! Thanks for taking the time to go through this! 👍
Sorry that I put so many comments. Since I invested time into reviewing I wanted to capture my thoughts such that as many potential surprises as possible can be catched early.
I think there was no major issue. Let's do final testing.
| matrix::Vector3f _acceleration{}; | ||
|
|
||
| bool _armed{false}; | ||
| bool _previous_armed_state{false}; ///< stores the previous actuator_armed.armed state |
There was a problem hiding this comment.
Tabs to indent, spaces to align right?
| bool _previous_armed_state{false}; ///< stores the previous actuator_armed.armed state | |
| bool _previous_armed_state{false}; ///< stores the previous actuator_armed.armed state |
There was a problem hiding this comment.
Yes, but also trying to maintain consistency by default. In this case the entire line was moved from below.
Something like clang-format could probably help get us there overall.
|
|
||
| // TODO: we need an accelerometer based check for vertical movement for flying without GPS | ||
| return (_has_low_thrust() || hit_ground) && (!_horizontal_movement || !_has_position_lock()) | ||
| && (!vertical_movement || !_has_altitude_lock()); | ||
| return ground_contact && !_horizontal_movement && !vertical_movement; |
There was a problem hiding this comment.
Summary of the big changes:
- The switch to the acceleration setpoint is handled correctly
- No vertical movement via velocity estimate is a requirement (even in case it's not available, see comment above) while before low thrust was enough
|
|
||
| // Determine the system min throttle based on flight mode | ||
| if (!_flag_control_climb_rate_enabled) { | ||
| sys_min_throttle = (_params.minManThrottle + 0.01f); |
There was a problem hiding this comment.
Needs to be tested if that's reachable with crappy stick input. Also isn't there a failsafe case where there's no link and no vertical velocity so you have to emergency descend with constant acceleration and hence no climb rate control?
There was a problem hiding this comment.
That entire case really needs to be tested at some point. At the moment I wouldn't bet on it functioning properly at multiple levels. It's rare because we virtually always have vertical velocity and position with baro.
There was a problem hiding this comment.
*inproperly
I agree. So the only thing to test for this change is if you get land detection in stabilized, that's it.
| // Ground contact, no thrust and no movement -> landed | ||
| return _ground_contact_hysteresis.get_state() && _has_minimal_thrust() && !rotating; | ||
| // Otherwise, landed if the system has minimum thrust (manual or in failsafe) and no rotation for at least 8 seconds | ||
| return (_min_thrust_start > 0) && (hrt_elapsed_time(&_min_thrust_start) > 8_s); |
There was a problem hiding this comment.
Summary of the big changes:
- low thrust threshold is mode/
control_climb_rate_enableddependent - only rotation around body roll and pitch are considered, rotation around yaw gets ignored now
- freefall detection is considered
Co-authored-by: Matthias Grob <[email protected]>
Co-authored-by: Matthias Grob <[email protected]>
Co-authored-by: Matthias Grob <[email protected]>
|
Overall it looks good, although a bit delayed in triggering After the minor hover thrust estimate changes and validity flag (#15370) merge I'll take another pass in the MC land detector to raise the |
|
Thanks for thorough testing @nrogelio. Let's create a separate issue to followup with the failure to transition from |
|
IT'S DONE |
|
I noticed the documentation wasn't up to date, so I started a pull request. To further improve the documentation, I wonder:
|
I am still surprised that the land detection doesn't take into account such high level modes, and yes false positive landings have led to drones disarming mid-air. We will def have to address this. Edit: Ah no, actually the |
Opening this for discussion/testing of possible fixes for #13124, #11683, #13507.
https://logs.px4.io/plot_app?log=4b65cf44-80cb-4910-bc2a-151028afe9da