-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pr ground effect zone #11592
Pr ground effect zone #11592
Conversation
@EliaTarasov Yes that looks good. What do you think about using the parameter EKF2_GND_EFF_DZ to set the ground effect dead zone and at the same time control enabling / disabling? E.g. if EKF2_GND_EFF_DZ = 0 -> disabled, otherwise enabled. |
Yes it's done. |
@EliaTarasov Awesome. I will test this on our side, feel free to report your test results. |
@EliaTarasov Is that a real flight? From the plots it looks like it worked fine, did it not? |
@EliaTarasov Ah, actually I realized there is one thing I wanted to change in the EKF. Currently it will only do the compensation if the vehicle is within 1 m from local_pos.z = 0. This is not very useful in practice as you might not land at the same elevation as you took off and also baro altitude drifts over time. |
That's the SITL. I tried to trigger the
Could you then point out what's need to be done to improve it? |
@EliaTarasov I can push the fix tomorrow. |
@RomanBapst Great, thanks! |
@EliaTarasov See PX4/PX4-ECL#585
I think this pretty much covers all the cases, what do you think? |
@RomanBapst I agree on the items you want to add. Do you have any test results which are include pr? Logs: |
17df26f
to
b840fea
Compare
@MaEtUgR thanks, fixed. |
1dfb26f
to
a5c9a27
Compare
@RomanBapst i added the first item from your list:
Is it a correct way? |
src/modules/ekf2/ekf2_main.cpp
Outdated
@@ -1174,7 +1177,13 @@ void Ekf2::run() | |||
} | |||
} | |||
|
|||
if (range_finder_updated) { _ekf.setRangeData(range_finder.timestamp, range_finder.current_distance); } | |||
if (range_finder_updated) { | |||
if (range_finder.current_distance <= _gnd_effect_deadzone.get()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EliaTarasov We should not directly use the range finder data but instead the distance to ground estimate from the estimator. You can just move the activation of the ground effect further down where we publish local position. The code could then look like:
if (lpos.dist_bottom_valid && lpos.dist_bottom < _gnd_effect_deadzone.get() < 1.0f)
src/modules/ekf2/ekf2_main.cpp
Outdated
@@ -1250,6 +1259,10 @@ void Ekf2::run() | |||
if (vehicle_land_detected_updated) { | |||
if (orb_copy(ORB_ID(vehicle_land_detected), _vehicle_land_detected_sub, &vehicle_land_detected) == PX4_OK) { | |||
_ekf.set_in_air_status(!vehicle_land_detected.landed); | |||
|
|||
if (_gnd_effect_deadzone.get() > 0.0f) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EliaTarasov We need to combine this with the distance to ground check. Basically if we have distance to ground we use that otherwise we use the flag from the landing detector.
Signed-off-by: Roman <bapstroman@gmail.com>
Co-Authored-By: EliaTarasov <elias.tarasov@gmail.com>
cd7a053
to
8431841
Compare
@RomanBapst i've combined you suggestions but confused of result. Is it correct? Logs: They are almost identical, but i expect the second log should display |
8431841
to
4f902d2
Compare
src/modules/ekf2/ekf2_main.cpp
Outdated
@@ -1390,6 +1393,16 @@ void Ekf2::run() | |||
lpos.dist_bottom = _rng_gnd_clearance.get(); | |||
} | |||
|
|||
// update ground effect flag based on terrain estimation | |||
if (lpos.dist_bottom_valid && lpos.dist_bottom < _gnd_effect_deadzone.get()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EliaTarasov I suggested to use _gnd_effect_deadzone.get() here but I don't think that makes sense. Should we use a fixed value like 1m or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomanBapst yes we may but i think it's better to add another parameter for this purpose. So _gnd_effect_deadzone
would be both for flag and for barometer innovations,
and say _gnd_effect_distance
would be for distance above ground where we may expect such innovations.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomanBapst any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EliaTarasov Apologies for the delay! Yes, that sounds like a good idea. Let's do those changes and then get this PR in. After this I want to continue taking into account takeoff as well (for the case where we don't have a terrain estimate). I already talked to @priseborough and he is ok with removing the local position dependency in the estimator. I think it's anyway a good idea to not put this kind of logic into the estimator itself but have it reside here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomanBapst Great, thanks! I've added the distance
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomanBapst Good to go?
@EliaTarasov Looks good, asking the test team to try it.
Thanks! |
Tested on Pixhawk v2 Cube; Parameter EKF2_GND_EFF_DZ was changed to 4. https://review.px4.io/plot_app?log=853b5e1b-9c70-4f87-8af7-eea984ec08ad Tested on Pixhawk v2 Cube; Parameter EKF2_GND_EFF_DZ was changed to 4. https://review.px4.io/plot_app?log=307107cf-b532-45d0-a84c-ec58b2fbdb84 |
Tested on Pixhawk 4 v5;
Pitch and yaw movements were slower than usual besides that it was a good flight. Tested on Pixhawk 4 v5;
No issues were noted, good flight in general. Tested on Pixhawk 4mini v5;
Good flight in general. Tested on Pixhawk 4mini v5;
No issues were noted, good flight in general. |
@EliaTarasov @hamishwillee We need to document this :-) |
@hamishwillee I assume we can put it in the user guide, maybe here? |
Continuation of work on adding ground effect zone.