Tuning logic audit: 20 correctness/safety fixes, docs sync, 72 new tests - #433
Merged
Conversation
Comprehensive audit of the tuning pipeline (5 audit agents + web research against Betaflight firmware sources and official firmware-presets): State machine & apply flow: - Allow *_analysis → completed transition (zero-change skip-verification regression from c51890e — completing a no-change session threw) - BF_SETTING_RANGES: cross-size PID envelope (6"/7" D 81-100 no longer aborts the whole apply), firmware-exact ranges (dyn_notch_min 20-250, d_min_gain 0-100, tpa_rate 0-100, simplified_dmax_gain 0-200), new FF gates - Informational recs never auto-applied (renderer filter + handler guard) — PIDSUM 400→1000 yaw jump was silently applied via the FF path - Feedforward changes now verified post-apply (MSP-readable subset) Analysis math: - Notch group-delay formula corrected (spurious numerator term caused ~3-9× underestimate); LPF2 modeled as PT1 per BF 4.3+ defaults (was biquad, ~2× over) - TF DC gain + bandwidth reference from 1-5 Hz band average (bin 0 is regularization-dominated with zero-mean setpoints) - TF-4 threshold style-aware (20·log10(1−sse/100)) with −0.4 dB noise floor - Effective log-rate warning uses looptime×pInterval×pDenom (was 8× off) Recommendation quality: - DTE < 0.3 emits informational "improve filters first" rec instead of silently deleting the D change - PIDSUM recommendations informational-only (weight gate is a house rule) - Yaw ringing threshold relaxed ×1.5 (consistent with overshoot/rise) - Size-aware mechanical health threshold (healthy whoops no longer flagged as damaged hardware) - dyn_notch_count=0 no longer masks resonance peaks as "covered" - Dynamic lowpass disable hysteresis (4 dB vs 6 dB enable) stops flip-flop - RC smoothing advisory skipped for aggressive style (racing runs 20-30) Values re-anchored to primary sources (BF pid.h, firmware-presets): - 1" whoop bounds P≤90/D≤80, pTypical 72; micro D/P ceiling 1.0 - iterm_relax aggressive 20-40 typ 30 (BF wiki: 30-40 racing) - anti-gravity gate 400g→700g (5" presets keep default 80) - TPA small breakpoint 1500→1250; RPM Q 3-4" 700-1000 (SupaflyFPV) BBL as primary source: - Static LPF cutoffs + dyn_notch range always enriched from BBL headers (disconnected-FC analysis ran against defaults instead of flight values) Dead code removed: DYNAMIC_LOWPASS_BY_SIZE, ADVANCED_CONFIG.DEBUG_MODE; DTERM_LPF2_DISABLE_THRESHOLD_DB wired into its rule. Tests: 3111+6 skipped → 3189 passing, 0 skipped (72 new incl. analytic group-delay anchors, informational-apply guards, size-aware health, BBL-primary enrichment; 6 toast tests unskipped with proper useToast mock). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qqWVVTHHDDSa73kHRGS12
- PID_TUNING_KNOWLEDGE.md: corrections verified against primary sources — racing iterm_relax 30-40 (BF wiki), d_min 4.5 defaults 30/34 (rename is 2025.12 not 4.5), anti-gravity 110/120 = 7-9" heavy only, whoop P 60-90, PT1 lowpass defaults + delay anchors, notch 100 Hz rule reattribution, S_xy formula label, F-DLPF rules + hysteresis, house-rule labeling for size-extrapolated tables, current SupaflyFPV preset reality - README: decision tables (F-DN-Q confidence, DTE-block, PW-IRELAX floor 7, missing MOD row, TF-4 style-aware), safety bounds table 1"/2.5" split, 26 analysis modules, prop wash band phrasing - ARCHITECTURE/SPEC/QUICK_START: E2E count 32, broken links → docs/complete/ - src/main/CLAUDE.md + src/main/analysis/CLAUDE.md: apply gate, FF verify, zero-change transition, corrected estimator formulas, hysteresis - infrastructure: /admin/beta auth, v2/v3 tags, diagnostic-resolve usage, deploy triggers + D1 migrations, worker file listings - tuning-advisor skill: updated checklist values (informational PIDSUM, 700g anti-gravity gate, micro damping ceiling, dead-constant removal) - Test counts 3111 → 3189 (README, ARCHITECTURE, SPEC, TESTING) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qqWVVTHHDDSa73kHRGS12
There was a problem hiding this comment.
Pull request overview
This PR performs a broad correctness/safety audit of FPVPIDlab’s tuning pipeline (analysis → recommendations → apply → verification) and synchronizes documentation + tests to match the corrected behavior and firmware-accurate bounds.
Changes:
- Fixes multiple apply/verification safety issues (informational recommendations never auto-apply; PID gate uses cross-size bounds; feedforward MSP read-back verification added; zero-change sessions can complete directly).
- Corrects several analysis math/data-source issues (TF DC gain reference band, notch group-delay formula, LPF2 modeled as PT1 by default, BBL headers treated as primary source for key filter settings).
- Updates docs and expands/revives test coverage to anchor the new behaviors.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TESTING.md | Updates test inventory totals and expands descriptions for newly covered behaviors. |
| src/renderer/hooks/useTuningWizard.ts | Filters out informational recommendations so they never count as “changes” / never apply. |
| src/renderer/hooks/useConnection.test.ts | Revives toast tests with a hoisted useToast mock and more realistic event sequencing. |
| src/main/utils/verifyAppliedConfig.ts | Adds optional MSP feedforward read-back verification and FF setting classification. |
| src/main/utils/verifyAppliedConfig.test.ts | Adds feedforward verification test coverage (match/mismatch/unchecked/legacy client). |
| src/main/storage/TuningSessionManager.ts | Allows *_analysis → completed transitions for zero-change sessions. |
| src/main/storage/TuningSessionManager.test.ts | Adds coverage for zero-change completion + cross-mode transition rejection. |
| src/main/msp/mspLayouts.ts | Removes debug_mode mapping and documents BBL-header source-of-truth rationale. |
| src/main/ipc/handlers/tuningHandlers.ts | Uses cross-size PID envelope for gating, firmware-accurate setting ranges, and defense-in-depth filtering of informational recs; passes FF changes into verification. |
| src/main/ipc/handlers.test.ts | Adds regression tests for cross-size D=100, informational drop, and new firmware-exact bounds. |
| src/main/CLAUDE.md | Updates apply/verification documentation to reflect new bounds + FF verification + zero-change completion. |
| src/main/analysis/TransferFunctionEstimator.ts | Uses a 1–5 Hz band DC-gain reference and reuses it for bandwidth computation. |
| src/main/analysis/TransferFunctionEstimator.test.ts | Adds unit tests for the new DC-gain computation behavior and fallbacks. |
| src/main/analysis/PIDRecommender.ts | Updates yaw ringing threshold relaxation, micro damping ratio ceiling, style-aware TF-4 thresholding, DTE block informational rec behavior, and marks PID-sum limits informational. |
| src/main/analysis/PIDRecommender.test.ts | Adds/updates extensive tests for new thresholds, micro bounds, TF-4 logic, and informational behaviors. |
| src/main/analysis/PIDAnalyzer.ts | Threads flight style into RC-link FF baseline recommendations. |
| src/main/analysis/MechanicalHealthChecker.ts | Makes extreme-noise threshold size-aware; exports resolver helper. |
| src/main/analysis/MechanicalHealthChecker.test.ts | Adds tests for size-aware extreme-noise thresholding and fallback behavior. |
| src/main/analysis/headerValidation.ts | Fixes log-rate warning to use effective blackbox log rate; ensures BBL headers override key static cutoff/notch-range fields. |
| src/main/analysis/headerValidation.test.ts | Adds tests for effective log rate + BBL-primary override behavior. |
| src/main/analysis/GroupDelayEstimator.ts | Fixes notch group-delay math and models LPF2 as PT1 by default. |
| src/main/analysis/GroupDelayEstimator.test.ts | Adds analytic anchor tests and PT1 LPF2 modeling tests. |
| src/main/analysis/FilterRecommender.ts | Corrects dyn-notch “coverage” when dyn_notch_count=0 and adds D-term LPF2 disable threshold constant usage. |
| src/main/analysis/FilterRecommender.test.ts | Adds tests for disabled dyn notch coverage, D-term LPF2 boundary behavior, and updated RPM Q midpoint expectations. |
| src/main/analysis/FilterAnalyzer.ts | Passes drone size into mechanical health check for size-aware thresholds. |
| src/main/analysis/FeedforwardAnalyzer.ts | Makes RC smoothing advisory style-aware (skips aggressive/racing). |
| src/main/analysis/FeedforwardAnalyzer.test.ts | Adds tests ensuring the RC smoothing advisory is skipped for aggressive style. |
| src/main/analysis/DynamicLowpassRecommender.ts | Adds disable hysteresis (4 dB) to prevent flip-flop across sessions. |
| src/main/analysis/DynamicLowpassRecommender.test.ts | Adds tests for disable hysteresis boundaries and removes dead-code multiplier tests. |
| src/main/analysis/constants.ts | Updates micro bounds, micro damping ratio ceiling, racing iterm_relax ranges, anti-gravity weight threshold, RPM Q ranges, and TPA small breakpoint. |
| src/main/analysis/CLAUDE.md | Syncs analysis module documentation to corrected math/models/thresholds. |
| SPEC.md | Fixes doc links and refreshes test counts + E2E run composition description. |
| README.md | Syncs module count, bounds/tables/decision-table language, and tuning-rule descriptions to updated implementation. |
| QUICK_START.md | Fixes offline UX testing doc link path. |
| infrastructure/SCRIPTS.md | Updates diagnostic resolve script usage to include required resolution parameter. |
| infrastructure/README.md | Updates infrastructure module tree to reflect new beta/email/migration components and scripts. |
| infrastructure/ENDPOINTS.md | Corrects endpoint auth/version notes and documents public /admin/beta shell behavior. |
| infrastructure/DEPLOYMENT.md | Aligns pipeline trigger paths and documents D1 migration steps for license worker. |
| docs/UX_IMPROVEMENT_IDEAS.md | Adds explicit “Active” status marker. |
| docs/README.md | Adds a new design doc entry and clarifies FC_STATE_CACHE status/location. |
| docs/PID_TUNING_KNOWLEDGE.md | Updates tuning knowledge content to match corrected rules, bounds, and sourcing. |
| ARCHITECTURE.md | Updates global test totals and analysis module count/paths and E2E run description. |
| .claude/skills/tuning-advisor/SKILL.md | Updates tuning-advisor checklist items to reflect corrected logic and removed dead code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…FF verify, act() wrap - verifyAppliedConfig: optional MSP FF fields returning undefined go to `unchecked` instead of a false mismatch (older firmware/layouts) - index.ts reconnect fallback verification now passes appliedFeedforwardChanges - useConnection.test: wrap connection-change callback in act() for consistency Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qqWVVTHHDDSa73kHRGS12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full audit of the FPV tuning pipeline (analysis math, recommendation rules, apply flow, data sources) cross-checked against primary sources: Betaflight firmware (
pid.h,settings.c), the officialbetaflight/firmware-presetsrepo, betaflight.com docs, and the real BBL fixtures intest-fixtures/bbl/.Critical fixes
VALID_TRANSITIONS(introduced in c51890e) omitted*_analysis → completed, so the PR fix: skip verification when no recommendations applied #241 "skip verification when nothing applied" flow threwInvalid phase transition.dMax=80while the recommender legally produces 81–100 for large quads. Gate now uses the cross-size envelope + firmware-exact ranges for every setting (several were wider than the FC accepts:d_min_gain,tpa_rate,dyn_notch_min_hz,simplified_dmax_gain).pidsum_limit_yaw 400→1000jump went straight to the FC. Now filtered in the renderer and the handler.DEFAULT_FILTER_SETTINGS(250/150 Hz) instead of the flight-time values (e.g. 500/75 in the fixtures).Robustness & convergence
dyn_notch_count = 0no longer masks resonance peaks as "covered"Values re-anchored to community presets
1" whoop bounds (P≤90/D≤80, typical 72, D/P ceiling 1.0), racing iterm_relax 20–40 typ 30 (BF wiki), anti-gravity gate 400→700 g (5" presets keep 80), TPA micro breakpoint 1250, RPM Q 3-4" 700–1000 (SupaflyFPV).
Docs & KB
PID_TUNING_KNOWLEDGE.mdcorrections with citations (d_min defaults 30/34, the d_min→d_max rename is 2025.12 not 4.5, PT1 delay anchors, 100 Hz rule reattributed to notches, …), README decision tables, safety-bounds table, module count 26, broken links, infra endpoint auth/version fixes, tuning-advisor skill checklist sync.Tests
3111 (+6 skipped) → 3189 passing, 0 skipped: 72 new tests (analytic group-delay anchors, informational-apply guards, BBL-primary enrichment, size-aware health, hysteresis boundaries, FF verification) and the 6 long-skipped toast tests revived with a proper
useToastmock.Full audit trail: 5 parallel audit agents (filter rules, PID rules, TF math, MSP/BBL data sources, apply-flow safety) + doc-sync + 2 web-research agents; final
/tuning-advisor reviewpassed with the one blocking finding (informational apply) fixed in this PR.Test plan
npx tsc --noEmitcleannpm run lint:checkcleannpm run test:run— 3189/3189, 0 skippednpm run test:e2e— 32/32 (all three tuning-cycle flows + diagnostic + smoke)/tuning-advisor review— blocking issue fixed, math independently derived🤖 Generated with Claude Code
https://claude.ai/code/session_016qqWVVTHHDDSa73kHRGS12