Skip to content

FilterReview: support up to four harmonic notches - #362

Open
j-w9 wants to merge 5 commits into
ArduPilot:mainfrom
j-w9:filter-review-4-notches
Open

j-w9 wants to merge 5 commits into
ArduPilot:mainfrom
j-w9:filter-review-4-notches

Conversation

@j-w9

@j-w9 j-w9 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #252.

ArduPilot now supports a third harmonic notch, and a fourth on some boards (ArduPilot/ardupilot#30246). This adds INS_HNTC3_* and INS_HNTC4_* to FilterReview: their filters in the estimated post-filter spectrum, the tracking lines on the FFT plot and spectrogram, and the logged notch frequencies.

The third and fourth notch inputs and show options only appear if the log, or a loaded parameter file, has those parameters, so the page is unchanged for two-notch firmware.

Testing:

  • On five logs, all FFT, Bode and spectrogram traces are identical to main.
  • A log with its notch 2 parameters renamed to notch 3 (and to notch 4) gives identical results to the original on main.

The Filter Tool link still only carries notches 1 and 2, as the Filter Tool supports two.

ArduPilot now supports a third harmonic notch, and a fourth on some
boards (ArduPilot/ardupilot#30246). Add INS_HNTC3_* and INS_HNTC4_* to
the notch parameters, their filters and plot tracking lines, and the
logged notch frequencies for notches 3 and 4.

The third and fourth notch inputs and show options are only shown if the
log, or a loaded parameter file, has those parameters.

Fixes ArduPilot#252
@tridge tridge added the AIReview label Nov 9, 2026
@AP-Review

AP-Review commented Nov 9, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-11-09)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 9b02f5790f.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/362/1.html#prWebTools-362

Notch 3/4 support in FilterReview itself checks out: plot indexing, show/hide, reset and logged notch tracking all behaved correctly. Two nonblocking issues remain.

Should fix

  • FilterReview/FilterReview.js:1841: "Open in FilterTool" sends INS_HNTC3/4_*, but FilterTool has no third or fourth notch, so it silently shows a response without them. Add notches 3/4 to FilterTool, or warn when an enabled notch 3/4 can't be carried over.
Minor (1)
  • FilterReview/FilterReview.js:2052: save_parameters() always writes the 20 HNTC3/4 inputs, even when the firmware lacks them and the panels are hidden. Reloading that file then shows the notch 3/4 panels with ENABLE=0. Skip a notch group in save_parameters() when its NotchNParams fieldset is hidden.

… them

Reloading a saved file would otherwise show the notch 3 and 4 panels
with them disabled.
@j-w9

j-w9 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Save now skips notch 3 and 4 when the firmware doesn't have them.

On the Filter Tool link: Filter Tool only has two notches, which the description mentions. I'd rather add notches 3 and 4 to Filter Tool in its own PR than grow this one. Happy to do that next if you want it.

@AP-Review

AP-Review commented Nov 9, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-11-09)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head fddc708bee.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/362/2.html#prWebTools-362

FilterReview's four-notch support looks good at fddc708. One minor issue remains.

Both earlier points are fixed. Saving now skips notches 3/4 when the firmware lacks them, and Open in filter tool warns about notches it can't pass on. Doing Filter Tool notch 3/4 support in its own PR is fine.

Minor (1)
  • FilterReview/FilterReview.js:1953: the warning is a modal alert() shown before window.open(). In Chromium, if the user leaves it open more than about 5 s, the popup blocker stops the Filter Tool tab from opening. Call window.open() first and then alert(), or show the note inline beside the button.

A popup blocker can stop the window opening if the alert is left open.
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head c27ac689d1.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/362/3.html#prWebTools-362

FilterReview's four-notch support looks good at c27ac68, with one minor issue.

Since the last review, Open in filter tool now opens the Filter Tool tab before showing the notch 3/4 warning, so a slow-to-dismiss alert can no longer trip the popup blocker.

Minor (1)
  • FilterReview/FilterReview.js:1841: on firmware without notches 3/4, those hidden groups are still built with the reset default MODE=1. For a log with no RATE or Copter CTUN, HarmonicNotchFilter then shows an "Unsupported notch mode 1" alert for each one, on load and on every Calculate filters. Only alert when the notch is enabled, or skip hidden notches in load_filters().

…s enabled

Notches 3 and 4 are reset to MODE 1 when the firmware lacks them, and
logs with no throttle source alerted for each on every calculate.
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.

FilterReview: add support for upto 4 harmonic notches

3 participants