AP_InertialSensor: fixed Q calculation for notch filters #28765
+13
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
when doing the init() we must use the reference frequency, not the current frequency. Using the current frequency leaves us with an incorrect value for Q. If the current frequency is below the reference frequency (which shouldn't happen in 4.5, but has been seen in 4.3) then the Q can be much too low and massive phase lag can happen. The vulnerability in 4.5.x is that the current frequency could be well above the reference frequency. For example, the user may be doing a motor test at 30s after boot, which is when we stop the sensors_converging() test, and thus is the last time we call init(). In that case we can end up with a Q which is much larger than the one that should come from INS_HNTCH_FREQ and INS_HNTCH_BW, and thus end up with a filter that produces a lot less attenuation than is desired, potentially leading to instability due to high noise. There are other scenarios that can cause this - for example a motor test of a fwd motor at 30s after boot, or a spurious FFT peak due to someone knocking the aircraft, or the vibration of a IC engine.
tridge
force-pushed
the
pr-notch-init-wrong-Q
branch
from
edc6d1a
to
fa828a7
Compare
|
Still an issue - needs to pass CI |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
when doing the notch init() we must use the reference frequency, not the current frequency. Using the current frequency leaves us with an incorrect value for Q. If the current frequency is below the reference frequency (which shouldn't happen in 4.5, but has been seen in 4.3) then the Q can be much too low and massive phase lag can happen. I am also not 100% certain a low freq in init() can't happen in 4.5.x, the route it came in for the 4.3.x crash logs is quite complex.
The way the init() works is we call it continuously up until 30s after boot, then we call it only if the user changes the notch parameters. This means the precise state of the aircraft at the 30s after boot mark is critical. Depending on what is happening to the aircraft at 30s after boot the Q value could be a very long way off.
The confirmed vulnerability in 4.5.x is that the current frequency could be well above the reference frequency. For example, the user may be doing a motor test at 30s after boot, which is when we stop the sensors_converging() test, and thus is the last time we call init(). In that case we can end up with a Q which is much larger than the one that should come from INS_HNTCH_FREQ and INS_HNTCH_BW, and thus end up with a filter that produces a lot less attenuation than is desired, potentially leading to instability due to high noise.
There are other scenarios that can cause this - for example a motor test of a fwd motor at 30s after boot, or a spurious FFT peak due to someone knocking the aircraft, or the vibration of a IC engine.
I've checked, and it is already fixed in 4.6.x