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

Fix Static Notches #28034

Merged
merged 3 commits into from
Sep 8, 2024
Merged

Fix Static Notches #28034

merged 3 commits into from
Sep 8, 2024

Conversation

andyp1per
Copy link
Collaborator

Bug introduced in #25442
I will note in passing that my review indicated this shouldn't be removed.
I have added a test to make sure this can't regress

@andyp1per andyp1per added the BUG label Sep 7, 2024
@IamPete1
Copy link
Member

IamPete1 commented Sep 7, 2024

I think we should still remove the check here:

if (params.tracking_mode() != HarmonicNotchDynamicMode::Fixed) {

It will allow runtime param changes to take affect for static notch as they do for the others.

@andyp1per
Copy link
Collaborator Author

It will allow runtime param changes to take affect for static notch as they do for the others

This was/is covered by the init() above. The reason not to do it here is the cost of updating the static notch all the time when it doesn't need it.

@IamPete1
Copy link
Member

IamPete1 commented Sep 7, 2024

This was/is covered by the init() above.

The init allows runtime change in frequency/bandwidth/attenuation but not runtime change in harmonics or double/triple notch.

The reason not to do it here is the cost of updating the static notch all the time when it doesn't need it.

I guess I don't really understand why we carefully check if we should re-init due to param change. It seems to be the update is the costly one we just do that all the time (except static). Why don't we check for a change in options or harmonic too and then call init and update at once?

@andyp1per
Copy link
Collaborator Author

The init allows runtime change in frequency/bandwidth/attenuation but not runtime change in harmonics or double/triple notch.

I mean this is true, but I don't think what you are suggesting fixes this does it? I agree that allowing runtime changes to these properties would be convenient.

I guess I don't really understand why we carefully check if we should re-init due to param change. It seems to be the update is the costly one we just do that all the time (except static). Why don't we check for a change in options or harmonic too and then call init and update at once?

Really just copying the pattern that was there for the LPF. It is true that calculating A&Q is expensive, so avoiding doing that all the time makes sense. However, we have made a lot of effort into making update cheap, so it may be that simply removing the check is not such a big deal anymore.

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

good find, thanks!

libraries/Filter/HarmonicNotchFilter.cpp Outdated Show resolved Hide resolved
libraries/Filter/HarmonicNotchFilter.cpp Outdated Show resolved Hide resolved
@andyp1per andyp1per requested a review from tridge September 8, 2024 09:04
@andyp1per
Copy link
Collaborator Author

Thanks @tridge I've pulled in the change

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

I think this is correct.

@peterbarker peterbarker merged commit 9726e8e into ArduPilot:master Sep 8, 2024
94 checks passed
@andyp1per andyp1per deleted the pr-static-notch branch September 10, 2024 12:40
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.

4 participants