Skip to content

Analytic Tune: Overhaul of Analytic tune to include plane - #337

Open
bnsgeyer wants to merge 24 commits into
ArduPilot:mainfrom
bnsgeyer:pr-poscon-tune
Open

bnsgeyer wants to merge 24 commits into
ArduPilot:mainfrom
bnsgeyer:pr-poscon-tune

Conversation

@bnsgeyer

@bnsgeyer bnsgeyer commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

This PR provides a pretty extensive update to analytic tune.
Adds dynamic page build to accommodate both attitude and position controller tuning
Adds SID Axes for Plane
Update params to include changed params in 4.7

@bnsgeyer bnsgeyer self-assigned this Oct 6, 2026
@AP-Review

AP-Review commented Oct 7, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-07)

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

Reviewed at head 8b1a0ced78.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/337/1.html#prWebTools-337

Thanks for the big overhaul. I reviewed head 8b1a0ce and reproduced the problems below with synthetic SysID logs.

The main one: the notch parameters from the log no longer reach the form. load_log sets INS_HNTCH_/INS_HNTC2_ before the form exists, and setup_FFT_data doesn't reload them (https://github.com/ardupilot/webtools/pull/337/files#diff-3d5b6156863de28ac11e95577f9808c1e7f02f0cf9fb8865666d25dc7892880cR1213). Roll/Pitch/Yaw predictions therefore always use the default 150 Hz / 75 Hz enabled notch. Moving the notch loop, including set_bitmask_size, into setup_FFT_data should fix it.

Other things to look at:

Which ArduPilot PR adds SID axis 24 and the Plane position axes? I couldn't find them in master.

Not covered: accuracy against a real flight log, and real-browser checks of the fixed-wing and quadplane paths.

@AP-Review

AP-Review commented Oct 8, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-08)

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

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

Thanks for the notch fix. I re-checked at 807a548, and the logged INS_HNTCH_/INS_HNTC2_ values now reach the form on the first Calculate, so that earlier problem is resolved.

The main remaining issue: Calculate rebuilds the whole parameter form and rewrites it from the log (https://github.com/ardupilot/webtools/pull/337/files#diff-3d5b6156863de28ac11e95577f9808c1e7f02f0cf9fb8865666d25dc7892880cR1142). Gains that were edited or loaded with Load Parameters are silently reset when Calculate is pressed again, for example for another SID run or time window. Load Parameters before the first Calculate throws. Base kept the edits. Building the form only when the vehicle or axis layout changes, or carrying values across the rebuild, would fix it.

Still open from last time, unchanged at this head:

New this round: the Lateral/Longitudinal loader reads DVN/VN and DVE/VE into the position variables (https://github.com/ardupilot/webtools/pull/337/files#diff-0dad09add812fbabafe507751f37abfbed999d85b065b6ded70ea1c01a0bc027R1497), so the position response is really a velocity response. Should those be TPN/PN and TPE/PE? If velocity was intended, a comment would help.

Minor: the new console.log("PARM", PARM) (https://github.com/ardupilot/webtools/pull/337/files#diff-0dad09add812fbabafe507751f37abfbed999d85b065b6ded70ea1c01a0bc027R704) and other debug logs; the set_bitmask_size loop left in load_log (https://github.com/ardupilot/webtools/pull/337/files#diff-0dad09add812fbabafe507751f37abfbed999d85b065b6ded70ea1c01a0bc027R714) never takes effect; a double Calculate click can decorate the form twice.

The question about which ArduPilot PR adds SID axis 24 and the Plane position axes is still open.

Not covered: accuracy against a real flight log, and real-browser checks of the fixed-wing and quadplane paths.

@AP-Review

Copy link
Copy Markdown

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

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

Re-reviewed at 05c47d4: one blocking regression remains, plus several should-fix problems in the new position and vertical pages. Since 807a548, the DRB velocity integration has been fixed and now scales correctly.

Blocking

  1. AnalyticTune/index.html:1142 Calculate resets edits (still open): every click rebuilds the form and reloads log values, so typed or imported gains are lost, and Load Parameters before the first Calculate throws. Keep the form when the layout is unchanged, or carry the values across the rebuild.

Should fix

  • AnalyticTune/AnalyticTune.js:1497 Position reads velocity (still open): PSCN/PSCE position arrays read DVN/VN and DVE/VE; use TPN/PN and TPE/PE.
  • AnalyticTune/AnalyticTune.js:1426 Heading wrap: the arithmetic mean of DesYaw (logged 0..360) gives about 180 for a north-facing vehicle and flips the body-frame signals; use atan2(mean sin, mean cos).
  • AnalyticTune/index.html:1122 SELECT changes ignored (still open): notch enable and mode changes do nothing.
  • AnalyticTune/AnalyticTune.js:1818 Vertical export incomplete (still open): Save Parameters drops the other PSC_VELZ_/PSC_ACCZ_ group and the NEF/NTF FILT notch.
  • ESLint on current main: main now runs ESLint on PRs, and main plus this PR gives 150 errors, mostly undeclared variables. Rebase, declare them, then refresh the suppressions.
Minor (5)

Not fully reviewed: accuracy against a real SysID flight log, real-browser checks of the fixed-wing and quadplane paths, and the live CI result.

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.

2 participants