Filter: HarmonicNotch: fix Quintuple Notch

[IamPete1]

Summary

As far as I can tell Quintuple Notch has never worked, this constrain resulted in them being silently converted to a triple notch. Found when writing ArduPilot/WebTools#332

They were added in #30994 and supposedly were tested and have a autotest.

Classification & Testing (check all that apply and add your own)

• Checked by a human programmer
• Non-functional change
• No-binary change
• Infrastructure change (e.g. unit tests, helper scripts)
• Automated test(s) verify changes (e.g. unit test, autotest)
• Tested manually, description below (e.g. SITL)
• Tested on hardware
• Logs attached
• Logs available on request

Description

[IamPete1]

There might be some argument that we should increment the filter version flag again since updating it to 3 did not really add support for quintuple notch.

[IamPete1]

Some more proof that they never worked, they are never hit in the coverage test https://firmware.ardupilot.org/coverage/Filter/HarmonicNotchFilter.cpp.gcov.html.

[andyp1per]

When I tested it it would have been on a 4.6 derivative rather than master so its conceivable that I did not get bitten by some of the other rework of this area of code.

[IamPete1]

When I tested it it would have been on a 4.6 derivative rather than master so its conceivable that I did not get bitten by some of the other rework of this area of code.

The constrain was added in #21026, it is in 4.6

[IamPete1]

I have added a fix for the notch count allocation. Because the notch count is not evaluated for the composite filters it could end up off the end of the array. This is a problem if the max number of notches is not a multiple of the number of composite filters. I think this bug exists in 4.6 and 4.7 with F7 as it has a limit of 39 notches. 39 is not divisible by 2, so with a double notch it would be possible to hit the bug. The other limits are all multiples of 1,2, and 3.

[tridge]

Automated review note - AI-generated (Claude), independently cross-checked by a second model and re-verified against the diff. Please sanity-check before acting.

Reviewed at head caad765164.

Verdict: COMMENT

• ISSUE - libraries/Filter/Filter.h:20 - The AP_FILTER_VERSION 3→4 bump is warranted, but the consumer is not ready for it. The bump is right — identical QuintupleNotch parameters now produce a materially different transfer function, and firmware needs no special case since FV is just a uint8_t in VER. But WebTools Filter Review recognises only versions 1 and 2, and models only single/double/triple — it ignores the quintuple option bit. So FV=4 logs currently fall back to a default model and cannot be analysed correctly, which is precisely what the version field exists to prevent. Worth a matching WebTools PR alongside this.

• NOTE - libraries/Filter/tests/test_notchfilter.cpp:366 - The new HarmonicNotchMaxFilters test has no assertion and unit-test CI is not ASan-enabled, so it can run clean despite an overrun. Verified under ASan: at the PR head it passes; with the clamp removed but the old non-rounded MIN(..., 78) restored it is a deterministic heap-buffer-overflow; and against true pre-PR code it passes because the test silently degrades to a triple notch. So it does reproduce the bug in the intermediate state, but it is not a base-vs-PR regression test and will not fail in normal CI.

• NOTE - libraries/Filter/HarmonicNotchFilter.cpp:207 - notch_count() assumes any value above 3 means exactly 5. Current callers only ever return 1/2/3/5 so this is safe today, but the invariant is unenforced and expand_filter_count() is public and accepts arbitrary sizes — composite_notches=4 would allocate four and write five.

• NOTE - libraries/Filter/HarmonicNotchFilter.cpp:497 - log_notch_centers() can report wrong harmonic frequencies when the allocation is capped or there are more than six sources: it derives num_sources from a possibly partial allocation, caps it at six, then uses the capped value as the second-harmonic offset, which does not match update's harmonic-major layout. Bounds-safe, but the FCN values can mislead.

Full report, including what was checked and found clean: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html