Skip to content
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

Commander: Remove dynamic position velocity probation period #14224

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Feb 24, 2020

Describe problem solved by this pull request
If a multicopter looses GPS position for 10 seconds it needs to have GPS position passing all accuracy checks for over 100 seconds such that the estimate gets accepted as usable again by Commander even though the EKF is already fusing it for a long time. This results in perfectly accurate and used position information with the user not being able to switch to any position controlled mode or position controlled failsafe to be possible.

Describe your solution
I'm investigating on this test branch what impact it has if we omit this to me confusing and unexpected logic and have a fixed configurable probation period for accepting valid position information.

Describe possible alternatives
I discussed with @dagar today that it would be best to capture the original intent of the author @priseborough and make sure it's still intact while hopefully solving unexpected cases. I'm happy to learn more details.

Test data / coverage
Tested this a bit in SITL droping GPS information randomly and flying the drone into the ground to have inconsistent estimation that is falged invalid for navigation.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Apr 1, 2020

I brought this up in call with @priseborough a while ago and he explained that we need to avoid having position validity going off and on too quickly because we're at the border of certain thresholds or realize after takeoff that navigation is not possible but he's open to other solutions than the current one in Commander like e.g. a hysteresis. One way of reproducing a problem is to fly without magnetometer in SITL.

@stale
Copy link

stale bot commented Jul 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jul 2, 2020
@dagar dagar closed this Jan 10, 2021
@LorenzMeier LorenzMeier deleted the remove-dynamic-pos-vel-probation branch January 18, 2021 14:45
@MaEtUgR MaEtUgR restored the remove-dynamic-pos-vel-probation branch March 22, 2022 09:37
@MaEtUgR MaEtUgR reopened this Mar 22, 2022
@stale stale bot removed the stale label Mar 22, 2022
@MaEtUgR MaEtUgR force-pushed the remove-dynamic-pos-vel-probation branch from 27d04c1 to e550cfe Compare March 22, 2022 20:36
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Mar 22, 2022

I'm resurrecting this pr since the issue now as before persists. We just had another instance of the dynamic probation period prohibiting a perfectly accurate position estimate because of a brief GPS drop out until the vehicle reached the ground luckily this time without crashing. I see a pattern where on every single multirotor product I worked on that gets enough testing hours this issue happens at some point. The solution was always me disabling the probation period by adding

param set-default COM_POS_FS_GAIN 0
param set-default COM_POS_FS_PROB 1

to the airframe and then the issue never occurs anymore. That's also why I originally opened this pr and since we have enough cases that speak for this change and enough testing without the probation period I'm bringing it up again.

I talked to @priseborough this morning and he agreed that with the yaw estimator such intermittent navigation failures so far did not happen anymore so he agrees to remove the probation period because it's at the moment more likely to harm than to help. In case any yaw-related navigation failures occur we can solve them in another way.

@MaEtUgR MaEtUgR marked this pull request as ready for review March 22, 2022 20:48
@MaEtUgR MaEtUgR requested a review from bresch March 23, 2022 10:33
@MaEtUgR MaEtUgR force-pushed the remove-dynamic-pos-vel-probation branch from e550cfe to 77f8713 Compare March 23, 2022 10:36
bresch
bresch previously approved these changes Mar 23, 2022
@@ -132,7 +132,7 @@ class Commander : public ModuleBase<Commander>, public ModuleParams
void battery_status_check();

bool check_posvel_validity(const bool data_valid, const float data_accuracy, const float required_accuracy,
const hrt_abstime &data_timestamp_us, hrt_abstime *last_fail_time_us, hrt_abstime *probation_time_us,
const hrt_abstime &data_timestamp_us, hrt_abstime *last_fail_time_us,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const hrt_abstime &data_timestamp_us, hrt_abstime *last_fail_time_us,
const hrt_abstime &data_timestamp_us, hrt_abstime &last_fail_time_us,

Since you're changing that line already, could you switch last_fail_time_us to a reference instead of pointer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll follow up. 👍

@MaEtUgR MaEtUgR changed the title Commander: Investigation without dynamic position velocity probation period Commander: Remove dynamic position velocity probation period Mar 23, 2022
@MaEtUgR MaEtUgR force-pushed the remove-dynamic-pos-vel-probation branch 3 times, most recently from da1f96c to 17265e4 Compare March 23, 2022 16:18
@MaEtUgR MaEtUgR force-pushed the remove-dynamic-pos-vel-probation branch from 17265e4 to 9e9ce3c Compare March 23, 2022 16:30
@dagar dagar merged commit e6ed595 into master Mar 23, 2022
@dagar dagar deleted the remove-dynamic-pos-vel-probation branch March 23, 2022 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants