-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
[WIP] Improve robustness to bad and lost airspeed data #11846
Conversation
cd16a54
to
1b44483
Compare
I'm not sure that I like commander taking on a stall speed and load factor calculation, but I don't have an immediate alternative to suggest architecturally. We already have this accelerated stall logic in the FW position controller, although it was done with the minimum airspeed (to avoid parameterizing a stall speed). Ideally this will all be handled in the same place. |
// Do actions based on value of COM_ASPD_FS_ACT parameter | ||
status.aspd_fault_declared = false; | ||
status.aspd_use_inhibit = false; | ||
status.aspd_fail_rtl = false; |
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.
If these are expected to be set on every iteration what's preventing spamming mavlink messages every iteration?
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.
If any of the fault conditions are triggered, then a fault tie is recorded here: https://github.com/PX4/Firmware/blob/a76726845a2299d7ea4da5c3b77a09f998fffe55/src/modules/commander/Commander.cpp#L4181
All faults have to clear for a minimum time here: https://github.com/PX4/Firmware/blob/a76726845a2299d7ea4da5c3b77a09f998fffe55/src/modules/commander/Commander.cpp#L4212
before fault_cleared can be set to true and _tas_use_inhibit is set to false.
Until this occurs, fault_declared cannot be set to true again which means that once a fault has been declared there is a minimum time of _tas_use_start_delay seconds before it can be cleared and re-declared.
|
||
/** | ||
* RTL delay after bad airspeed measurements are detected if COM_ASPD_FS_ACT is set to 4. Ensure the COM_ASPD_STALL parameter is set correctly before use. |
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.
Missing short description.
@RomanBapst Help with review and testing would be appreciated. |
1b44483
to
a767268
Compare
* @group Commander | ||
*/ | ||
PARAM_DEFINE_INT32(COM_ASPD_FS_ACT, 0); |
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.
TAS seems to be the more commonly used Param abbreviation for "airspeed". Should we rename this COM_TAS_FS_ACT?
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.
Done.
Looking through the logs supplied by @Antiheavy in #10733 I think there are changes to significantly reduce false positives associated with the use of the innovation check, eg:
However the load factor test on its own is already providing significant improvement in protection against a FOD or water pitot blockage so if EKF checks were be turned off by default, then the change could be rolled in now and the estimator related work rolled in with a second PR. |
This seems like a good idea for getting a basic and already tested safety feature implemented for v1.9 |
@dagar Could you fix the compile error so we can get this in with checks being off by default? Bringing stuff in but disabling it and collecting log data is the secret behind successful software development in a lot of industries, in particular in apps and web services. |
I've added some changes that reduces the likelihood of false positives for users that want to try the innovation checks, but has them disabled by default so that if the user activates protection they will get the stall ratio test only unless they specifically set an innovation test ratio level. By default protection and logging is off (COM_ASPD_FS_ACT = 0) I will now add a patch that logs the required diagnostic data even when COM_ASPD_FS_ACT = 0 |
Yes I'll take another pass tomorrow. |
I've added "(Experimental)" to all the parameter names and moved them to the "Developer" category. |
2d45562
to
fc30f09
Compare
Use the time integral of the exceedance to improve rejection of gust induced transients. Previous implementation could trigger above 0.7 * COM_TAS_FS_INNOV so fix scaling so that test ratios below COM_TAS_FS_INNOV cannot trigger. Change to an integer representation for the the threshold parameter and enable innovation test to be disabled by setting the threshold parameter to 0. Improve parameter documentation. The airspeed innovation test is disabled by default.
Always log the consistency checked variable, even when disabled. Revert the innovation threshold parameter back to a float to make its meaning more intuitive vs the estimator_status.tas_test_ratio it is compared against.
fc30f09
to
2ad6d55
Compare
d4cf777
to
b8302e7
Compare
Merging this as discussed to facilitate more real world testing. Fair warning for any early adopters, I anticipate a bit of parameter naming churn. |
This is now in master, thanks everyone! |
Continuation of #10733