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

Remove ICM Gyro Pre-filter #14891

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Jul 23, 2020

This PR is on top of #14423 and removes the gyro pre-filter that is applied in the ICM drivers.
There are a few reasons for doing this:

  • You only want as much filtering as you need and what you need will vary. A fixed 188Hz filter removes that configurability.
  • With the introduction of INS_GYRO_RATE we have remove the fixed backend rate of 1000Hz, a fixed 188Hz is likely to affect the gyro values differently as we modify the backend rate
  • It introduces delay (and CPU cost)
  • With notch filtering its now likely less important
  • It makes it harder for the FFT to track noise.

So although removing the filter will inevitably add some noise. The question is - does it matter?

I ran tests with a gyro rate of 2kHz on an MPU6000 with/without and with double-notches. I picked 2kHz here because this was the frequency at which the differences were most evident. These were the results:

Standard:
image

No pre-filter:
image

No pre-filter, double notches:
image

A few things to note:

  • There is more noise without the filter
  • The noise peaks are much clearer
  • The noise levels are perfectly acceptable
  • Double-notches squash the extra noise very nicely
  • There is no discernable aliasing noise

Note to reviewers - this is actually a two-line change, most of the changes are in #14423

@lthall
Copy link
Contributor

lthall commented Jul 30, 2020

This is one of those PR's where we need a good reason not to do it.

I suspect that this was supposed to be removed a long time ago but was overlooked.

@proficnc
Copy link
Contributor

I agree, unless a valid reason for keeping this filter is found, it should be removed.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Please tweak the commit message; you're not just "disabling" it, you're nuking it completely :-) The PR title is correct...

Any idea why there were different filter constants for the v2 sensor?

@tridge tridge merged commit e53a434 into ArduPilot:master Aug 3, 2020
@andyp1per andyp1per deleted the pr-gyro-prefilter branch August 4, 2020 08:10
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.

None yet

7 participants