Feature/aerotune gps filter autotune#4929
Open
simonjardine wants to merge 52 commits intobetaflight:masterfrom
Open
Feature/aerotune gps filter autotune#4929simonjardine wants to merge 52 commits intobetaflight:masterfrom
simonjardine wants to merge 52 commits intobetaflight:masterfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Switch from virtual:pwa-register to workbox-window Workbox class directly - Wire both 'waiting' and 'externalwaiting' events to catch all update paths - Use wb.messageSkipWaiting() + 'controlling' event to trigger reload cleanly - Add setInterval hourly wb.update() poll — forces unconditional network fetch of sw.js, bypassing GitHub Pages cache header limitations - Explicitly set skipWaiting: false and clientsClaim: false in workbox config - Guard against duplicate dialogs with updatePromptShown flag - Reset flag on No so prompt reappears on next detected update Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new AeroTune Vue tab (calculator, analyzer, autotune CLI flow), Blackbox BBL/BFL parsing and frame decoding, a D3 filter frequency visualiser, GPS accuracy scatter plot, PID external-change tracking with lifecycle resets, Workbox-managed SW update flow, gh-pages deploy script, and documentation/changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AeroTune UI (Browser)
participant Store as pidTuningStore
participant Serial as Serial Backend
participant FC as Flight Controller
UI->>Store: validate chirp params & intensity
UI->>Serial: request enter CLI mode
Serial->>FC: send CLI enter (e.g., '#')
Serial->>FC: send "set ..." commands (staggered)
Serial->>FC: send "save"
FC-->>Serial: ack / save responses
Serial-->>UI: confirmation / dispatched command list
UI->>Serial: request chirp assignment / start
FC-->>Serial: telemetry / chirp log stream
Serial-->>UI: forward telemetry for analyzer
UI->>Store: markExternalChange()
Store-->>UI: hasChanges = true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vite.config.js (1)
144-153:⚠️ Potential issue | 🟠 MajorSet
injectRegister: falseto disable auto-registration when using manualWorkboxflow.This config lacks an explicit
injectRegistersetting, so it defaults to"auto", which auto-injects a registration script since no virtual modules are imported. Meanwhile,src/js/browserMain.jsmanually callswb.register()on line 98, creating two concurrent registration paths. This race condition risks missing or duplicatingwaiting/externalwaitingevents. Disable the injected registration to ensure only the manual Workbox flow controls service worker lifecycle.Config fix
VitePWA({ + injectRegister: false, registerType: "prompt", workbox: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.js` around lines 144 - 153, The VitePWA plugin configuration currently relies on default injectRegister which can auto-inject a registration script and race with your manual Workbox flow; explicitly set injectRegister: false in the VitePWA(...) config so only your manual registration (wb.register() called in src/js/browserMain.js) runs and you avoid duplicate/missing waiting/externalwaiting events.
🧹 Nitpick comments (2)
src/js/browserMain.js (1)
38-38: Declareworkbox-windowdirectly inpackage.json.The app bundle now imports this package itself, so it should be declared explicitly in
dependenciesinstead of relying onvite-plugin-pwato hoist it transitively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/browserMain.js` at line 38, The import line "import { Workbox } from 'workbox-window'" shows the bundle directly depends on the workbox-window package, so add "workbox-window" to package.json dependencies (not devDependencies) to declare it explicitly; update package.json dependencies to include the package (matching the version used by your project or the one Vite/PWA previously provided) and run your package manager install so the import in browserMain.js (Workbox) resolves without relying on vite-plugin-pwa to hoist it.package.json (1)
26-26: Pingh-pagesinstead of fetching it ad hoc.
npx gh-pagesmakes deployments depend on whatever version npm resolves at runtime. Addinggh-pagestodevDependenciesand invoking the local binary keeps the release path reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 26, Update package.json to pin gh-pages in devDependencies and use the local package binary in the deploy script: add a specific gh-pages version (not fetched ad hoc via npx) to devDependencies (e.g., "gh-pages": "<fixed-version>") and change the "deploy" script that currently references npx gh-pages to invoke the package binary (use the package script resolution so the script runs gh-pages from node_modules, e.g., keep the existing "deploy" key but replace "npx gh-pages -d src/dist" with a direct gh-pages invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-53: Update the release headings in CHANGELOG.md to use the
project's CalVer scheme instead of the current SemVer-like headings: replace the
three headers "## [1.4.0] - 2026-03-24", "## [1.2.0] - 2026-03-24", and "##
[1.1.0] - 2026-03-24" with their proper CalVer release names (e.g., "##
[2026.03.24] - 2026-03-24" or the repository's canonical CalVer strings) so they
reflect calendar versioning used by Betaflight Configurator; ensure any internal
API SemVer mentions remain unchanged and update any cross-references in the file
that mention those old headings.
In `@src/components/tabs/AeroTuneTab.vue`:
- Around line 136-204: The table columns for D Max, Derivative and D Min are
misaligned: the template displays {{ pids.*_d }} under "D Max" and {{
pids.d_min_* }} under "Derivative", while applyToFC() writes d_min_* into
ADVANCED_TUNING.dMax* and calculatePIDs() computes d_min_* = 0.887 * *_d,
causing the ceiling to be below the floor. Fix by reordering the table cells so
the Derivative column shows {{ pids.*_d }}, the D Min column shows {{
pids.d_min_* }}, and the D Max column shows the correct max value (matching
whatever value applyToFC() expects), and verify applyToFC(), calculatePIDs(),
and ADVANCED_TUNING.dMax* use the same mapping of *_d vs d_min_* consistently.
- Around line 1690-1708: The code currently always uses sessions[0] from
BBLHeaderParser.parseFile and a single headerEnd, so multi-session .bbl files
are only analyzed for the first session; update the logic to pick and decode the
intended session instead of hard-coding sessions[0]. Use the sessions array
returned by BBLHeaderParser.parseFile to either (a) iterate over sessions and
call findBBLBinaryStart and FrameDecoder.decodeFrames for each session, or (b)
select the correct session (e.g., by timestamp/index) and compute that session's
headerEnd (use a session-specific buffer slice or the session metadata if it
exposes an offset/header end), then construct FrameDecoder(configForSession) and
call decoder.decodeFrames(buffer, headerEndForSession, 0) for that session;
adjust any downstream use of frames/analysisResult to handle multiple session
results or the selected session. Ensure references to BBLHeaderParser,
parseFile, sessions, findBBLBinaryStart, FrameDecoder, and decodeFrames are
updated accordingly.
- Around line 1592-1630: Call MSP_PID to load the current PID table into FC.PIDS
before mutating and sending MSP_SET_PID: perform await
MSP.promise(MSPCodes.MSP_PID) (handle errors like the MSP_PID_ADVANCED read),
then patch only the roll/pitch/yaw rows on FC.PIDS (indices 0,1,2) rather than
assuming a fully populated array, and only after that call
MSP.promise(MSPCodes.MSP_SET_PID) and MSP_SET_PID_ADVANCED; also ensure the
FC.PIDS and FC.PIDS_ACTIVE initializations and DEFAULT_PIDS use the 5-slot
layout (5×3 = 15 values) so the code uses indices 0..4 (ROLL, PITCH, YAW, LEVEL,
MAG) rather than a 10-slot array.
In `@src/components/tabs/GpsTab.vue`:
- Around line 283-285: The panel title "GPS Accuracy (Stationary)" is hardcoded
in GpsTab.vue; replace it with a translatable string using the app's i18n helper
(e.g. $t('gps.accuracy_stationary') or an appropriate key) in the span/div with
class "spacer_box_title" so the title is routed through i18n like the rest of
the tab and add the new key to your locale files.
In `@src/components/tabs/pid-tuning/FilterResponseChart.vue`:
- Around line 281-285: The Nyquist marker uses a hard-coded 8 kHz base rate;
change the calculation to use the actual gyro sample clock so the marker scales
for 32 kHz targets. Read the gyro clock flag
(FC.PID_ADVANCED_CONFIG.gyroUse32kHz or equivalent reactive state) and compute
baseRate = gyroUse32kHz ? 32000 : 8000, then set gyroRate = baseRate /
state.gyroSyncDenom (keeping pidDenom = Math.max(state.pidProcessDenom, 1) and
nyquist = gyroRate / (pidDenom * 2)); update the same logic wherever similar
calculations appear (e.g., the block covering lines 455-483).
In `@src/components/tabs/pid-tuning/PidSubTab.vue`:
- Around line 961-983: The template uses hasApi145 but it isn't defined; add a
computed getter in the <script setup> similar to showProfileName/usesAdvancedTpa
that returns whether FC.CONFIG.apiVersion >= API_VERSION_1_45 (use semver.gte
against FC.CONFIG.apiVersion and API_VERSION_1_45). Declare const hasApi145 =
computed(() => semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) (or
equivalent) so the template's :max binding and any conditional rendering work
correctly.
In `@src/index.html`:
- Around line 207-209: Replace the hardcoded visible label and title on the
AeroTune tab by using the same i18n/i18n_title attributes pattern as sibling
tabs: locate the <li class="tab_aerotune"> and its <a class="tabicon
ic_aerotune"> element and swap the literal "AeroTune" text and title="AeroTune"
for the i18n key attributes (e.g., i18n="..." and i18n_title="...") used by
other tabs so the label and tooltip are localized.
In `@src/js/aerotune/bbl-header-parser.js`:
- Around line 20-22: parseFile currently decodes the entire fileBuffer into an
ASCII string up front which doubles memory for large logs; instead, treat
fileBuffer as binary (Uint8Array or ArrayBuffer) and scan for the header
terminator bytes to find the end of the header, then decode only the header
slice with TextDecoder("ascii"). In practice: when parseFile receives a
non-string fileBuffer, create a Uint8Array view, locate the header terminator
sequence (the existing header terminator used by .bbl/.bfl files) with indexOf
on that Uint8Array, slice up to that index + terminator length, and call new
TextDecoder("ascii").decode(headerSlice); fall back to decoding the whole buffer
only if the terminator isn't found; preserve the string-path behavior when
fileBuffer is already a string.
In `@src/js/aerotune/data-stream.js`:
- Around line 335-342: readTag2_3SVariable() incorrectly delegates to
readTag2_3S32() even though selectors 1 and 2 use different per-value bit
widths; update readTag2_3SVariable to read the top-2-bit selector first, then
handle cases: for selector 0 and 3 reuse the existing read logic (or call
readTag2_3S32), for selector 1 decode three signed fields with widths 5,5,4 bits
respectively, and for selector 2 decode three signed fields with widths 8,7,7
bits respectively; ensure you perform the same sign-extension/ordering and
return structure as readTag2_3S32 so downstream code receives correctly decoded
values.
In `@src/js/aerotune/frame-decoder.js`:
- Around line 1-10: Remove the proprietary header block containing "© 2026
aerobot2.com — All Rights Reserved" from the AeroTune modules and replace it
with the project-compatible license header used by the repository (copy the
existing SPDX/license header used in other source files); apply this change to
the modules named frame-decoder.js, bbl-header-parser.js, and data-stream.js so
each file starts with the repository’s standard license header instead of the
proprietary one.
- Around line 478-499: The skip methods currently resync which can cut into
valid payloads; update _skipEventFrame, _skipSlowFrame and _skipGPSFrame to
consume frames according to their declared header/field descriptions instead of
scanning for markers: use this.frameFields (e.g. this.frameFields.E, .S, .G, .H)
to obtain the field list and encodings for the frame type being skipped, then
loop the declared fields and call the same stream-read primitives the decoder
uses for those encodings (e.g. stream.readByte(), stream.readSignedVB(),
stream.readUnsignedVB(), stream.readFloat()/appropriate read method) to advance
exactly over the frame payload; for _skipEventFrame also read the event-specific
header (eventType and any length/field count from this.frameFields.E) and skip
its declared fields rather than resyncing. Ensure _skipGPSFrame uses
this.frameFields.G (or H) encodings similarly so no marker-scanning is used.
In `@src/js/browserMain.js`:
- Around line 64-67: The service-worker callbacks in browserMain.js call
i18n.getMessage(...) but i18n is not imported; add the missing import for the
i18n binding (e.g. import { i18n } from "./localization") at the top of
browserMain.js so the references in the service-worker event handlers (the calls
to i18n.getMessage used for title/text/buttonYesText/buttonNoText) resolve and
do not throw ReferenceError.
In `@src/stores/pidTuning.js`:
- Around line 20-24: The externalChangeFlag (ref false) is only cleared by
save() so it persists across disconnects or switching flight controllers,
causing storeOriginals() / checkForChanges() to incorrectly keep
hasChanges=true; update the pidTuning store to reset externalChangeFlag when a
disconnect or new FC session is detected (e.g., add and call a
clearExternalChangeOnDisconnect/handleSessionChange helper from the connection
lifecycle or expose a resetExternalChange() method and invoke it from the
connection/disconnect handlers), and ensure clearExternalChange() is called from
those handlers as well as from save(); touch the externalChangeFlag reference
and the functions storeOriginals(), checkForChanges(), save(), and
clearExternalChange() in your change so the flag is reliably cleared on
disconnect or new-session events.
---
Outside diff comments:
In `@vite.config.js`:
- Around line 144-153: The VitePWA plugin configuration currently relies on
default injectRegister which can auto-inject a registration script and race with
your manual Workbox flow; explicitly set injectRegister: false in the
VitePWA(...) config so only your manual registration (wb.register() called in
src/js/browserMain.js) runs and you avoid duplicate/missing
waiting/externalwaiting events.
---
Nitpick comments:
In `@package.json`:
- Line 26: Update package.json to pin gh-pages in devDependencies and use the
local package binary in the deploy script: add a specific gh-pages version (not
fetched ad hoc via npx) to devDependencies (e.g., "gh-pages": "<fixed-version>")
and change the "deploy" script that currently references npx gh-pages to invoke
the package binary (use the package script resolution so the script runs
gh-pages from node_modules, e.g., keep the existing "deploy" key but replace
"npx gh-pages -d src/dist" with a direct gh-pages invocation).
In `@src/js/browserMain.js`:
- Line 38: The import line "import { Workbox } from 'workbox-window'" shows the
bundle directly depends on the workbox-window package, so add "workbox-window"
to package.json dependencies (not devDependencies) to declare it explicitly;
update package.json dependencies to include the package (matching the version
used by your project or the one Vite/PWA previously provided) and run your
package manager install so the import in browserMain.js (Workbox) resolves
without relying on vite-plugin-pwa to hoist it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31ef307d-0535-4a97-8c43-8c44d42dabc7
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/images/icons/cf_icon_aerotune_orange.svgis excluded by!**/*.svgsrc/images/icons/cf_icon_aerotune_white.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
CHANGELOG.mdpackage.jsonsrc/components/elements/GpsAccuracyPlot.vuesrc/components/tabs/AeroTuneTab.vuesrc/components/tabs/GpsTab.vuesrc/components/tabs/PidTuningTab.vuesrc/components/tabs/pid-tuning/FilterResponseChart.vuesrc/components/tabs/pid-tuning/FilterSubTab.vuesrc/components/tabs/pid-tuning/PidSubTab.vuesrc/css/main.lesssrc/css/tabs/aerotune.lesssrc/css/tabs/gps.lesssrc/index.htmlsrc/js/aerotune/bbl-header-parser.jssrc/js/aerotune/data-stream.jssrc/js/aerotune/frame-decoder.jssrc/js/browserMain.jssrc/js/gui.jssrc/js/main.jssrc/js/vue_components.jssrc/stores/pidTuning.jsvite.config.js
Comment on lines
+3
to
+53
| ## [1.4.0] - 2026-03-24 | ||
|
|
||
| ### The big one — AUTO TUNE is here | ||
|
|
||
| After years of watching ArduCopter pilots have all the fun with one-click AutoTune, | ||
| Betaflight now has its own. Flip one switch in the air and the chirp system sweeps | ||
| through every frequency your drone can produce — Pitch, Roll then Yaw — automatically. | ||
| Land, drop the log in Step 2, done. | ||
|
|
||
| The AUTO TUNE tab keeps it simple. Pick your shake intensity — Easy, Medium or Hard — | ||
| hit Configure, assign your Chirp switch in Modes, and fly. Advanced pilots can dig into | ||
| sweep frequency and duration under the Advanced Settings panel. | ||
|
|
||
| Confirmed working on Betaflight 25.12. | ||
|
|
||
| ### Also shipped today | ||
|
|
||
| - **Native BFL file support** — drop your raw Betaflight blackbox file straight into | ||
| Step 2. No more exporting CSV from Blackbox Explorer first. Just fly, land, analyse. | ||
|
|
||
| - **Filter frequency visualiser** — now shows the crossfade sweep zone between your | ||
| dynamic filter min and max frequencies as a shaded region. See exactly where your | ||
| filters are working. | ||
|
|
||
| - **Rock solid update notifications** — the "new version available" prompt now fires | ||
| reliably on every deploy, even when GitHub Pages caches the service worker. | ||
|
|
||
| - **GPS sky plot and accuracy scatter** — see your satellites by position and lock | ||
| status, plus live 1σ and 2σ accuracy calculated as fixes come in. Know your GPS is | ||
| solid before you fly. | ||
|
|
||
| ## [1.2.0] - 2026-03-24 | ||
|
|
||
| ### Added | ||
| - Filter visualiser now shows crossfade sweep zones — dynamic notch and D-Term LPF1 dynamic range shown as semi-transparent shaded regions between min and max frequencies | ||
| - Filter frequency response visualiser at the bottom of the Filter Settings tab — full-width D3 chart showing signal attenuation (0 dB to −40 dB) across 0–1000 Hz for all active filters: Gyro LPF1 (blue, static or dynamic min/max pair), Gyro LPF2 (cyan), D-Term LPF1 (orange, dynamic-aware), D-Term LPF2 (yellow), dynamic notch filters (red, Q-scaled notches evenly spaced across min/max range), and RPM filter harmonics (green, shown only when DShot telemetry is enabled). A dashed Nyquist marker is drawn at half the computed PID loop rate. All curves update live as filter values are changed on the tab. Filter math uses standard biquad transfer functions: PT1 (1st-order RC), BIQUAD (Butterworth 2nd order), PT2 (cascaded PT1²), PT3 (cascaded PT1³), and notch via |fn²−f²|/√((fn²−f²)²+(fn·f/Q)²). | ||
|
|
||
| ## [1.1.0] - 2026-03-24 | ||
|
|
||
| ### Added | ||
| - GPS satellite sky plot — polar scatter showing satellite positions by azimuth/elevation, coloured by lock status (green=locked, blue=found, red=searching), labelled with constellation and satellite ID | ||
| - GPS stationary accuracy scatter plot — live position fix scatter with 1σ and 2σ accuracy displayed, CEP rings at 50% and 95%, cumulative accuracy calculation | ||
| - AeroTune PID output table redesigned to match Betaflight PID tuning tab layout — Roll/Pitch/Yaw rows with Proportional/Integral/D Max/Derivative/Feedforward columns | ||
| - AeroTune voltage scaling — PIDs now correctly scale down for 6S (−13%) and 8S (−23%) builds | ||
| - AeroTune prop size scaling — corrected scaling curve from 2-inch to 8-inch with real-world validated anchor points | ||
| - AeroTune tooltip (i) icons on all PID column headers with plain-English seesaw analogy explanations | ||
| - Bando flying style added | ||
|
|
||
| ### Changed | ||
| - Apply PIDs button resized and recoloured to match Betaflight orange theme | ||
| - Freestyle renamed to Bando |
Contributor
There was a problem hiding this comment.
Align these headings with the project's CalVer scheme.
1.4.0 / 1.2.0 / 1.1.0 don't match the repo's calendar versioning and read like three separate releases shipped on the same day. This should use the actual Betaflight Configurator release naming.
Based on learnings: The Betaflight Configurator project uses CalVer (calendar versioning) format for versioning (e.g., "2025.12.0") rather than traditional SemVer, while still using SemVer for internal API versioning.
🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: Use a hyphen to join words.
Context: ...re your filters are working. - Rock solid update notifications — the "new ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 3 - 53, Update the release headings in
CHANGELOG.md to use the project's CalVer scheme instead of the current
SemVer-like headings: replace the three headers "## [1.4.0] - 2026-03-24", "##
[1.2.0] - 2026-03-24", and "## [1.1.0] - 2026-03-24" with their proper CalVer
release names (e.g., "## [2026.03.24] - 2026-03-24" or the repository's
canonical CalVer strings) so they reflect calendar versioning used by Betaflight
Configurator; ensure any internal API SemVer mentions remain unchanged and
update any cross-references in the file that mention those old headings.
Comment on lines
+1
to
+10
| /** | ||
| * AeroTune 7 — BBL Frame Decoder | ||
| * | ||
| * Decodes binary I-frames and P-frames from Betaflight Blackbox logs. | ||
| * Extracts gyro, setpoint, PID terms, and motor output data. | ||
| * | ||
| * Written from scratch by aerobot2.com | ||
| * Reference: BBL format specification (public documentation) | ||
| * | ||
| * © 2026 aerobot2.com — All Rights Reserved |
Contributor
There was a problem hiding this comment.
Remove the proprietary license header from the new AeroTune modules.
All Rights Reserved blocks redistribution of this source under the configurator’s repository license. Please replace it here — and in the sibling src/js/aerotune/bbl-header-parser.js / src/js/aerotune/data-stream.js — with project-compatible licensing before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/aerotune/frame-decoder.js` around lines 1 - 10, Remove the proprietary
header block containing "© 2026 aerobot2.com — All Rights Reserved" from the
AeroTune modules and replace it with the project-compatible license header used
by the repository (copy the existing SPDX/license header used in other source
files); apply this change to the modules named frame-decoder.js,
bbl-header-parser.js, and data-stream.js so each file starts with the
repository’s standard license header instead of the proprietary one.
Comment on lines
+20
to
+24
| // Set by external tools (e.g. AeroTune) that write PIDs to FC hardware | ||
| // before navigating to this tab. While true, storeOriginals() keeps | ||
| // hasChanges = true so the Save button stays enabled across Refresh calls. | ||
| // Only cleared when the user explicitly saves (clearExternalChange()). | ||
| const externalChangeFlag = ref(false); |
Contributor
There was a problem hiding this comment.
Reset the external-change flag on disconnect or new FC sessions.
Right now the flag is only cleared by save(). If AeroTune marks it and the user refreshes, disconnects, or connects to a different FC before saving, storeOriginals() / checkForChanges() will keep forcing hasChanges = true because this Pinia store survives across tabs and connections.
Also applies to: 55-59, 61-77, 90-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/pidTuning.js` around lines 20 - 24, The externalChangeFlag (ref
false) is only cleared by save() so it persists across disconnects or switching
flight controllers, causing storeOriginals() / checkForChanges() to incorrectly
keep hasChanges=true; update the pidTuning store to reset externalChangeFlag
when a disconnect or new FC session is detected (e.g., add and call a
clearExternalChangeOnDisconnect/handleSessionChange helper from the connection
lifecycle or expose a resetExternalChange() method and invoke it from the
connection/disconnect handlers), and ensure clearExternalChange() is called from
those handlers as well as from save(); touch the externalChangeFlag reference
and the functions storeOriginals(), checkForChanges(), save(), and
clearExternalChange() in your change so the flag is reliably cleared on
disconnect or new-session events.
…se headers, MSP read before write
- AeroTuneTab.vue: swap Derivative and D Max table column order so
Derivative (d_min) appears before D Max, matching Betaflight PID tab
convention; fix applyToFC() so FC.PIDS[][2] receives d_min values
and FC.ADVANCED_TUNING.dMaxRoll/Pitch receive the actual D Max values
(was inverted — d_min going to dMax register and vice-versa)
- AeroTuneTab.vue: add await MSP.promise(MSPCodes.MSP_PID) before
patching FC.PIDS so we write against current FC values, not stale defaults
- browserMain.js: add missing import { i18n } from './localization' used
by service worker callbacks (pwaOnNeedRefreshTitle, pwaOnOffilenReadyTitle)
- aerotune/bbl-header-parser.js, frame-decoder.js, data-stream.js: replace
proprietary '© 2026 aerobot2.com — All Rights Reserved' with
SPDX-License-Identifier: GPL-3.0-or-later to match repo license
- stores/pidTuning.js: add resetForConnection() that clears externalChangeFlag
and resets hasChanges/originalsReady on disconnect or new connection
- serial_backend.js: call usePidTuningStore().resetForConnection() in both
onOpen() and onClosed() so stale AeroTune flags never persist across sessions
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Read FC.PID_ADVANCED_CONFIG.gyroUse32kHz to determine the actual gyro base clock (32000 Hz vs 8000 Hz) before computing gyroRate and Nyquist frequency. Previously the marker was hardcoded to 8 kHz, placing it 4x too low on 32 kHz targets and making filter guidance misleading. gyroUse32kHz is included in the reactive state snapshot so the marker updates automatically if the value changes while the tab is open. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/js/aerotune/bbl-header-parser.js (1)
15-18: Consider minor static analysis suggestions.SonarCloud flagged two stylistic improvements:
- Line 17: Use a class field declaration instead of
thisassignment in constructor- Lines 64-68: Use
for-ofloop instead of indexedforloopThese are optional improvements that don't affect correctness.
♻️ Optional refactor
class BBLHeaderParser { + LOG_START_MARKER = "H Product:Blackbox flight data recorder by Nicholas Sherlock"; + constructor() { - this.LOG_START_MARKER = "H Product:Blackbox flight data recorder by Nicholas Sherlock"; }_parseHeaderLines(lines) { let raw = {}; - for (let i = 0; i < lines.length; i++) { - let match = lines[i].match(/^H\s+(.+?):(.*)$/); + for (const line of lines) { + let match = line.match(/^H\s+(.+?):(.*)$/); if (!match) continue; raw[match[1].trim()] = match[2].trim(); }Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/aerotune/bbl-header-parser.js` around lines 15 - 18, Move the LOG_START_MARKER out of the constructor into a class field on BBLHeaderParser (declare LOG_START_MARKER = "H Product:Blackbox flight data recorder by Nicholas Sherlock" directly on the class) instead of assigning it with this in the constructor, and replace the indexed for loop currently iterating over the lines (the loop around the code referenced by lines 64-68) with a for-of loop to iterate directly over each line item; update any uses of the loop index to use the loop variable instead.src/js/aerotune/data-stream.js (1)
33-43: Add braces to single-line if statements for consistency.SonarCloud flagged several single-line
ifstatements without braces. While functional, adding braces improves maintainability.♻️ Optional refactor for consistency
readByte() { - if (this.pos >= this.end) return -1; // EOF + if (this.pos >= this.end) { + return -1; // EOF + } return this.buffer[this.pos++]; } peekByte() { - if (this.pos >= this.end) return -1; + if (this.pos >= this.end) { + return -1; + } return this.buffer[this.pos]; } readS16() { let val = this.readU16(); - if (val > 32767) val -= 65536; + if (val > 32767) { + val -= 65536; + } return val; } readTag2_3S32() { let leadByte = this.readByte(); - if (leadByte === -1) return [0, 0, 0]; + if (leadByte === -1) { + return [0, 0, 0]; + }Also applies to: 59-64, 197-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/aerotune/data-stream.js` around lines 33 - 43, Add braces to all single-line if statements for consistency and readability: update readByte and peekByte (and other similar methods around the other occurrences flagged) so their if checks that return -1 or perform any single-statement action are wrapped with curly braces. Locate the single-line ifs in functions readByte, peekByte and the other flagged blocks (the other occurrences near the same style) and change them from "if (cond) return ..." or similar to a braced form "if (cond) { return ...; }".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/tabs/AeroTuneTab.vue`:
- Around line 1769-1815: The configureFc() method currently uses raw
serial.send() and manual buffers; replace that with the project's CLI flow:
ensure CONFIGURATOR.connectionValid && CONFIGURATOR.cliValid &&
CONFIGURATOR.cliActive before sending, enter CLI using GUI.timeout_add(...) to
send 0x23 with the 250ms delay pattern used in useCli.js, wait for
CONFIGURATOR.cliValid/CLI prompt confirmation, then dispatch each command via
the send() wrapper function from useCli.js (not serial.send) with the same
staggered timing, and finally set chirpConfirmText/chirpConfigured after
confirming CLI commands completed; update references inside configureFc to call
send() and rely on CONFIGURATOR flags instead of direct serial buffers.
In `@src/js/aerotune/frame-decoder.js`:
- Around line 133-139: The single-line if/else blocks in the P-frame decode path
are missing braces which SonarCloud flagged; update the conditional at the block
using this.history[0] and this._decodePFrame(stream) so both the true branch
(stats.pFrames++) and the false branch (checking this.history[0] to increment
stats.skipped or stats.errors, then calling this._resync(stream)) are wrapped in
braces, and apply the same change to the other reported spot near the similar
logic (around the lines flagged) so all single-statement branches use { } to
prevent future errors.
- Around line 286-303: The switch cases that declare const variables (e.g., in
the ENC_TAG8_8SVB branch where const count =
this._countConsecutiveEncoding(fieldDefs, fieldIndex, ENC_TAG8_8SVB) is defined,
and the other case around lines with ENC_TAG2_3SVAR) must be wrapped in their
own block scope to prevent cross-case access; change each affected case to use a
block (case XXX: { /* compute const count or other consts */ return
stream.readTag8_8SVB(count); } ) so const declarations are scoped to that case
and don't leak into following cases, referencing the
_countConsecutiveEncoding(fieldDefs, fieldIndex, ENC_TAG8_8SVB) call and
stream.readTag* methods when applying the fix.
In `@src/js/browserMain.js`:
- Around line 60-77: The guard updatePromptShown is only reset in
buttonNoCallback so dialog dismissals via Esc/cancel leave it true; modify
showUpdatePrompt to attach a generic close/cancel handler when calling
GUI.showYesNoDialog (in addition to existing buttonYesCallback and
buttonNoCallback) that resets updatePromptShown = false so any dismissal clears
the guard, and ensure the handler is removed or not double-attached when the Yes
path (wb.messageSkipWaiting / controlling -> reload) runs; reference
showUpdatePrompt, updatePromptShown, and GUI.showYesNoDialog to locate where to
add the close/cancel cleanup.
- Around line 51-55: The app is registering the service worker twice (VitePWA's
auto-injection plus your manual Workbox registration in browserMain.js using the
Workbox instance `wb`), causing duplicate lifecycle events; to fix, disable
VitePWA's runtime injection by setting the plugin option injectRegister to false
in your Vite PWA config (vite.config.js) so only the manual `const wb = new
Workbox(...)` / `wb.register()` path in browserMain.js runs, then rebuild —
alternatively, remove the manual `wb.register()` call and rely on VitePWA's
`registerType: "prompt"` if you prefer automatic registration.
---
Nitpick comments:
In `@src/js/aerotune/bbl-header-parser.js`:
- Around line 15-18: Move the LOG_START_MARKER out of the constructor into a
class field on BBLHeaderParser (declare LOG_START_MARKER = "H Product:Blackbox
flight data recorder by Nicholas Sherlock" directly on the class) instead of
assigning it with this in the constructor, and replace the indexed for loop
currently iterating over the lines (the loop around the code referenced by lines
64-68) with a for-of loop to iterate directly over each line item; update any
uses of the loop index to use the loop variable instead.
In `@src/js/aerotune/data-stream.js`:
- Around line 33-43: Add braces to all single-line if statements for consistency
and readability: update readByte and peekByte (and other similar methods around
the other occurrences flagged) so their if checks that return -1 or perform any
single-statement action are wrapped with curly braces. Locate the single-line
ifs in functions readByte, peekByte and the other flagged blocks (the other
occurrences near the same style) and change them from "if (cond) return ..." or
similar to a braced form "if (cond) { return ...; }".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 757a259d-c1ba-4191-bcf8-f2a2bc6e651b
📒 Files selected for processing (7)
src/components/tabs/AeroTuneTab.vuesrc/js/aerotune/bbl-header-parser.jssrc/js/aerotune/data-stream.jssrc/js/aerotune/frame-decoder.jssrc/js/browserMain.jssrc/js/serial_backend.jssrc/stores/pidTuning.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stores/pidTuning.js
Comment on lines
+60
to
+77
| const showUpdatePrompt = () => { | ||
| if (updatePromptShown) return; | ||
| updatePromptShown = true; | ||
| console.log("PWA: new service worker waiting — prompting user to update"); | ||
| GUI.showYesNoDialog({ | ||
| title: i18n.getMessage("pwaOnNeedRefreshTitle"), | ||
| text: i18n.getMessage("pwaOnNeedRefreshText"), | ||
| buttonYesText: i18n.getMessage("yes"), | ||
| buttonNoText: i18n.getMessage("no"), | ||
| buttonYesCallback: () => { | ||
| // Tell the waiting SW to skip waiting, then reload once it takes control. | ||
| wb.addEventListener("controlling", () => window.location.reload()); | ||
| wb.messageSkipWaiting(); | ||
| }, | ||
| buttonNoCallback: () => { | ||
| // Allow the prompt to reappear next time a new SW is detected. | ||
| updatePromptShown = false; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where GUI.showYesNoDialog is defined
fd -t f '\.js' | xargs rg -l 'showYesNoDialog\s*[=:]' | head -20Repository: betaflight/betaflight-configurator
Length of output: 60
🏁 Script executed:
# Look at the full context of the showUpdatePrompt function in browserMain.js
cat -n src/js/browserMain.js | sed -n '50,90p'Repository: betaflight/betaflight-configurator
Length of output: 2460
🏁 Script executed:
# Search for GUI object definition and showYesNoDialog implementation
rg 'showYesNoDialog' -A 15 -B 2Repository: betaflight/betaflight-configurator
Length of output: 9114
🏁 Script executed:
# Get the complete showYesNoDialog implementation
rg 'showYesNoDialog' -A 30 src/js/gui.jsRepository: betaflight/betaflight-configurator
Length of output: 1185
🏁 Script executed:
# Check the full context around the showUpdatePrompt function and beyond
cat -n src/js/browserMain.js | sed -n '50,110p'Repository: betaflight/betaflight-configurator
Length of output: 3181
Reset the updatePromptShown guard when the dialog is dismissed, not just in buttonNoCallback.
GUI.showYesNoDialog() only runs callbacks when Yes/No buttons are clicked. With showModal(), pressing Esc fires a cancel event and closes the dialog without triggering either callback. If this happens, updatePromptShown remains true, blocking subsequent waiting/externalwaiting events from showing the prompt again until reload. Add a close event handler to reset the guard on any dialog dismissal, not only when the user clicks "No".
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 71-71: Prefer globalThis over window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/browserMain.js` around lines 60 - 77, The guard updatePromptShown is
only reset in buttonNoCallback so dialog dismissals via Esc/cancel leave it
true; modify showUpdatePrompt to attach a generic close/cancel handler when
calling GUI.showYesNoDialog (in addition to existing buttonYesCallback and
buttonNoCallback) that resets updatePromptShown = false so any dismissal clears
the guard, and ensure the handler is removed or not double-attached when the Yes
path (wb.messageSkipWaiting / controlling -> reload) runs; reference
showUpdatePrompt, updatePromptShown, and GUI.showYesNoDialog to locate where to
add the close/cancel cleanup.
…g, frame sync, i18n, security hotspots
Multi-session BBL (AeroTuneTab.vue):
- Refactor findBBLBinaryStart() to accept sessionIndex parameter using
a two-phase scan (locate Nth marker, then collect header lines) so
each session gets its own correct binary-data start offset
- Extract BBL decode/analyze logic into _decodeBBLSession() method
- On load store buffer + all sessions; auto-analyze session 0
- Show session selector <select> dropdown when >1 session exists;
runBBLSession() re-decodes the chosen session on change
Tag2_3SVARIABLE encoding (data-stream.js):
- Implement readTag2_3SVariable() properly instead of delegating to
readTag2_3S32(); selector 1 uses 5,5,4-bit fields and selector 2
uses 8,7,7-bit fields packed across two and three bytes respectively;
selectors 0 and 3 are identical to Tag2_3S32
Frame sync (frame-decoder.js):
- Add _skipFrameFields() helper that consumes each field using its
declared encoding via _readFieldGroup(), keeping the stream aligned
- Fix _skipSlowFrame() to use _skipFrameFields(frameFields.S)
- Fix _skipGPSFrame() to use _skipFrameFields(frameFields.G/H)
- Fix _skipEventFrame() with a payload-size lookup table for all known
Betaflight event types instead of falling back to resync
GPS i18n (GpsTab.vue + locales/en/messages.json):
- Replace hardcoded "GPS Accuracy (Stationary)" with $t('gpsPanelAccuracyTitle')
- Add gpsPanelAccuracyTitle key to English locale
Security hotspot (AeroTuneTab.vue):
- Replace popup.document.write() in openInstructionsPopup() with a
Blob + URL.createObjectURL() approach; eliminates document.write and
the blank-window pattern flagged by static analysis
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/tabs/pid-tuning/FilterResponseChart.vue (1)
457-481: Extract the FC snapshot into one helper.The watch effect and the resize callback rebuild the same state object twice. Keeping that mapping in a single
getChartState()helper will prevent the two redraw paths from drifting the next time a field is added or renamed.Also applies to: 499-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/pid-tuning/FilterResponseChart.vue` around lines 457 - 481, The state mapping from FC (FILTER_CONFIG, PID_ADVANCED_CONFIG, MOTOR_CONFIG) is duplicated in two places (the watch effect and the resize callback); extract that mapping into a single helper function (e.g., getChartState()) that returns the object currently assigned to state (fields like gyroLpf1Hz, gyroLpf1Type, dynNotchCount, dshotTelemetry, etc.), then replace the duplicated inline object constructions in the watch effect and the resize handler to call getChartState() so both redraw paths use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/tabs/pid-tuning/FilterResponseChart.vue`:
- Around line 4-5: The chart UI currently hardcodes English strings (e.g., the
<span class="chart-title"> and <span class="chart-subtitle"> in
FilterResponseChart.vue plus axis labels, Nyquist label and legend text defined
in the chart options/series) so the visualizer ignores the app locale; replace
these literals with i18n locale keys and interpolate any dynamic values (like
the frequency range or Nyquist frequency) via the app's translation method
(e.g., $t or i18n.t) used elsewhere in the project, update the chart
options/series label fields and axis.title/text entries to use those keys, and
add corresponding translation entries for each key so the title, axis labels,
Nyquist label, and legend text render per the active locale.
- Around line 281-286: The MSPHelper reader is consuming the gyroUse32Khz byte
via data.readU8() but never storing it into the flight controller state, so
components reading state.gyroUse32kHz (e.g., in FilterResponseChart.vue) always
see false; update the MSP read path in MSPHelper (where data.readU8() is called
for gyroUse32Khz) to persist that value into FC.PID_ADVANCED_CONFIG (or the same
state object consumers expect) as a boolean (true when non-zero), and replicate
the same fix for the other occurrences noted (the other read sites around the
referenced offsets) so both state snapshots reflect the 32 kHz flag. Ensure the
symbol names match consumers: set FC.PID_ADVANCED_CONFIG.gyroUse32kHz (or the
exact property used by state.gyroUse32kHz) from the read byte.
---
Nitpick comments:
In `@src/components/tabs/pid-tuning/FilterResponseChart.vue`:
- Around line 457-481: The state mapping from FC (FILTER_CONFIG,
PID_ADVANCED_CONFIG, MOTOR_CONFIG) is duplicated in two places (the watch effect
and the resize callback); extract that mapping into a single helper function
(e.g., getChartState()) that returns the object currently assigned to state
(fields like gyroLpf1Hz, gyroLpf1Type, dynNotchCount, dshotTelemetry, etc.),
then replace the duplicated inline object constructions in the watch effect and
the resize handler to call getChartState() so both redraw paths use the same
source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b4ce082-1dad-48e2-a80b-ccb26ffe6ad9
📒 Files selected for processing (1)
src/components/tabs/pid-tuning/FilterResponseChart.vue
Comment on lines
+4
to
+5
| <span class="chart-title">Filter Frequency Response</span> | ||
| <span class="chart-subtitle">0 – 1000 Hz</span> |
Contributor
There was a problem hiding this comment.
Localize the new chart copy.
The title, axis labels, Nyquist label, and legend text are all hard-coded in English here, so the new visualizer ignores the configurator's active locale. Please move these strings to locale keys and interpolate the dynamic values there.
Also applies to: 121-185, 204-224, 295-295, 401-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tabs/pid-tuning/FilterResponseChart.vue` around lines 4 - 5,
The chart UI currently hardcodes English strings (e.g., the <span
class="chart-title"> and <span class="chart-subtitle"> in
FilterResponseChart.vue plus axis labels, Nyquist label and legend text defined
in the chart options/series) so the visualizer ignores the app locale; replace
these literals with i18n locale keys and interpolate any dynamic values (like
the frequency range or Nyquist frequency) via the app's translation method
(e.g., $t or i18n.t) used elsewhere in the project, update the chart
options/series label fields and axis.title/text entries to use those keys, and
add corresponding translation entries for each key so the title, axis labels,
Nyquist label, and legend text render per the active locale.
Comment on lines
+281
to
+286
| // ── Nyquist / loop-rate marker ── | ||
| const gyroSyncDenom = Math.max(state.gyroSyncDenom, 1); | ||
| const pidDenom = Math.max(state.pidProcessDenom, 1); | ||
| const gyroBaseClock = state.gyroUse32kHz ? 32000 : 8000; | ||
| const gyroRate = gyroBaseClock / gyroSyncDenom; | ||
| const nyquist = gyroRate / (pidDenom * 2); |
Contributor
There was a problem hiding this comment.
The 32 kHz Nyquist fix still isn't wired to MSP-backed state.
src/js/msp/MSPHelper.js:1101-1120 still consumes the gyroUse32Khz byte with data.readU8() and never stores it on FC.PID_ADVANCED_CONFIG. Both state snapshots here therefore default to false, so 32 kHz targets will still draw the marker from an 8 kHz base clock on load.
Also applies to: 482-485, 522-524
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tabs/pid-tuning/FilterResponseChart.vue` around lines 281 -
286, The MSPHelper reader is consuming the gyroUse32Khz byte via data.readU8()
but never storing it into the flight controller state, so components reading
state.gyroUse32kHz (e.g., in FilterResponseChart.vue) always see false; update
the MSP read path in MSPHelper (where data.readU8() is called for gyroUse32Khz)
to persist that value into FC.PID_ADVANCED_CONFIG (or the same state object
consumers expect) as a boolean (true when non-zero), and replicate the same fix
for the other occurrences noted (the other read sites around the referenced
offsets) so both state snapshots reflect the 32 kHz flag. Ensure the symbol
names match consumers: set FC.PID_ADVANCED_CONFIG.gyroUse32kHz (or the exact
property used by state.gyroUse32kHz) from the read byte.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/tabs/AeroTuneTab.vue (2)
181-204:⚠️ Potential issue | 🔴 Critical
d_min_*is still wired in as the main D value.
copyValues()treatsroll_d/pitch_das the D-term andd_min_*as the smaller floor, but Lines 1626-1634 pushd_min_*intoFC.PIDS[][2], and the table renders those same floor values under “Derivative”. The FC will end up with a lower D than the user saw/generated. Write*_dtoMSP_PID, patch the dedicated D-min advanced fields separately, and keep the table columns aligned with that same mapping.Also applies to: 1624-1657, 1686-1691
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/AeroTuneTab.vue` around lines 181 - 204, The PID D-term floor values (d_min_roll/d_min_pitch) are being written into the main D slot sent to the FC (MSP_PID / FC.PIDS[][2]) and displayed in the Derivative column, but copyValues() and the UI should use roll_d/pitch_d/yaw_d as the actual D-term and treat d_min_* only as the separate D-min floor; update the code paths that build and send MSP_PID (where FC.PIDS[][2] is populated) to use roll_d/pitch_d/yaw_d, and ensure the D-min advanced fields are sent/updated separately (preserving their own mapping), then align the table columns so the Derivative column shows *_d values while the D-min floor column shows d_min_* values (check functions/methods: copyValues(), the MSP_PID send/serialize logic, and the template bindings for the table rows for roll/pitch/yaw).
1813-1858:⚠️ Potential issue | 🟠 Major
configureFc()still bypasses the existing CLI handshake.This path sends
#and then rawserial.send()writes on timers without waiting for the CLI prompt. If the port is still in MSP mode, commands can be dropped or interleaved with MSP traffic, andsavecan reboot before the sequence is actually accepted. Please reuse the established CLI entry/send flow and gate command dispatch on the configurator’s confirmed CLI-valid state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/AeroTuneTab.vue` around lines 1813 - 1858, configureFc() currently writes '#' and raw serial.send() timers directly, bypassing the existing CLI handshake and risking dropped/interleaved commands; change it to reuse the configurator's CLI entry/send flow (e.g., call the existing CONFIGURATOR method that enters CLI and a sendCli/sendCommand helper) instead of direct serial.send, gate dispatch on the configurator's confirmed CLI-ready state (check CONFIGURATOR.connectionValid and the CLI-ready flag), send commands sequentially through that CLI API (ensuring the final 'save' runs last and wait for its acknowledgement or prompt before marking chirpConfigured true), and replace direct use of serial.send and the manual enterBuf/sendRaw logic while still updating chirpConfirmText when the configurator confirms the commands were accepted.
🧹 Nitpick comments (1)
src/components/tabs/GpsTab.vue (1)
650-658: Consider usingshift()instead ofsplice(0, 1)for clarity.Both are functionally equivalent, but
shift()is the idiomatic JavaScript method for removing the first element from an array.♻️ Suggested change
if (accuracyFixes.value.length > 500) { - accuracyFixes.value.splice(0, 1); + accuracyFixes.value.shift(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/GpsTab.vue` around lines 650 - 658, Replace the splice used to drop the oldest element with the idiomatic array method: in the block that pushes into accuracyFixes.value (check the gpsInfo.fix and latitude/longitude checks and the rolling cap logic around accuracyFixes.value.length > 500), call accuracyFixes.value.shift() instead of accuracyFixes.value.splice(0, 1) so the first element is removed clearly and equivalently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/tabs/AeroTuneTab.vue`:
- Around line 1702-1711: The decoder is reading from the selected session's
binary start to the end of the whole file, which lets non-final sessions run
into the next session's ASCII header and corrupt later sessions; in
_decodeBBLSession compute the current session's binary end by locating the next
session's binary start (e.g., call findBBLBinaryStart(buffer, sessionIdx + 1))
and if that returns 0 use buffer.length, then either slice the buffer for the
session (buffer.slice(headerEnd, sessionEnd)) or pass the sessionEnd as the
decode end to FrameDecoder.decodeFrames instead of decoding to buffer.length;
apply the same fix where decodeFrames is called around the other occurrence
noted (near line ~1770) so every session is bounded by its own byte range.
In `@src/js/aerotune/data-stream.js`:
- Around line 35-36: The read helper that returns -1 on EOF (the method that
currently does "if (this.pos >= this.end) return -1; return
this.buffer[this.pos++];") must not be silently converted into zeros by callers;
change EOF to be propagated as a decode failure (return null or throw) and
update all callers to treat null as a fatal decode error rather than filling
with zeros. Specifically, modify the byte/array-reading helpers so they return
null on EOF instead of -1, update _signExtend and any tag-decoding helpers that
accept -1 to handle null and propagate it up, and ensure
FrameDecoder._validateFrameEnd no longer treats stream.eof() as a valid frame
boundary but treats a null/EOF from readers as a frame validation failure so
truncated frames are rejected. Ensure all places mentioned (the read helper,
_signExtend, the tag branches, and FrameDecoder._validateFrameEnd) propagate
null/throw rather than converting EOF into valid values.
In `@src/js/aerotune/frame-decoder.js`:
- Around line 300-302: The default branch in the switch inside the frame
decoding logic (the block that currently returns [stream.readSignedVB()])
silently treats unknown header encodings as signedVB which desynchronizes
subsequent parsing; change the default behavior in the decoder (the switch
handling header encoding in frame-decoder.js) to fail fast instead of consuming
bytes — e.g. throw a descriptive Error (or return a clearly handled failure
value) indicating "Unsupported encoding" and include the offending encoding
value so callers of the decode function can stop processing rather than calling
stream.readSignedVB().
---
Duplicate comments:
In `@src/components/tabs/AeroTuneTab.vue`:
- Around line 181-204: The PID D-term floor values (d_min_roll/d_min_pitch) are
being written into the main D slot sent to the FC (MSP_PID / FC.PIDS[][2]) and
displayed in the Derivative column, but copyValues() and the UI should use
roll_d/pitch_d/yaw_d as the actual D-term and treat d_min_* only as the separate
D-min floor; update the code paths that build and send MSP_PID (where
FC.PIDS[][2] is populated) to use roll_d/pitch_d/yaw_d, and ensure the D-min
advanced fields are sent/updated separately (preserving their own mapping), then
align the table columns so the Derivative column shows *_d values while the
D-min floor column shows d_min_* values (check functions/methods: copyValues(),
the MSP_PID send/serialize logic, and the template bindings for the table rows
for roll/pitch/yaw).
- Around line 1813-1858: configureFc() currently writes '#' and raw
serial.send() timers directly, bypassing the existing CLI handshake and risking
dropped/interleaved commands; change it to reuse the configurator's CLI
entry/send flow (e.g., call the existing CONFIGURATOR method that enters CLI and
a sendCli/sendCommand helper) instead of direct serial.send, gate dispatch on
the configurator's confirmed CLI-ready state (check CONFIGURATOR.connectionValid
and the CLI-ready flag), send commands sequentially through that CLI API
(ensuring the final 'save' runs last and wait for its acknowledgement or prompt
before marking chirpConfigured true), and replace direct use of serial.send and
the manual enterBuf/sendRaw logic while still updating chirpConfirmText when the
configurator confirms the commands were accepted.
---
Nitpick comments:
In `@src/components/tabs/GpsTab.vue`:
- Around line 650-658: Replace the splice used to drop the oldest element with
the idiomatic array method: in the block that pushes into accuracyFixes.value
(check the gpsInfo.fix and latitude/longitude checks and the rolling cap logic
around accuracyFixes.value.length > 500), call accuracyFixes.value.shift()
instead of accuracyFixes.value.splice(0, 1) so the first element is removed
clearly and equivalently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bff97033-2bea-4a2e-a3ef-9c2630160d64
📒 Files selected for processing (5)
locales/en/messages.jsonsrc/components/tabs/AeroTuneTab.vuesrc/components/tabs/GpsTab.vuesrc/js/aerotune/data-stream.jssrc/js/aerotune/frame-decoder.js
✅ Files skipped from review due to trivial changes (1)
- locales/en/messages.json
Comment on lines
+35
to
+36
| if (this.pos >= this.end) return -1; // EOF | ||
| return this.buffer[this.pos++]; |
Contributor
There was a problem hiding this comment.
Partial EOFs are being decoded as real values.
Lines 124, 175, 199, 279, and 350 convert EOF into 0/zero-filled arrays, and the tag branches below can also feed -1 into _signExtend(). Because FrameDecoder._validateFrameEnd() accepts stream.eof() as a valid frame boundary, a truncated tail frame can be committed with synthetic values instead of being rejected. Please propagate EOF as a decode failure (null/throw) from these helpers so corrupt logs fail cleanly instead of skewing the analyzer.
Also applies to: 117-129, 165-185, 197-264, 277-333, 348-416
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 35-35: Expected { after 'if' condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/aerotune/data-stream.js` around lines 35 - 36, The read helper that
returns -1 on EOF (the method that currently does "if (this.pos >= this.end)
return -1; return this.buffer[this.pos++];") must not be silently converted into
zeros by callers; change EOF to be propagated as a decode failure (return null
or throw) and update all callers to treat null as a fatal decode error rather
than filling with zeros. Specifically, modify the byte/array-reading helpers so
they return null on EOF instead of -1, update _signExtend and any tag-decoding
helpers that accept -1 to handle null and propagate it up, and ensure
FrameDecoder._validateFrameEnd no longer treats stream.eof() as a valid frame
boundary but treats a null/EOF from readers as a frame validation failure so
truncated frames are rejected. Ensure all places mentioned (the read helper,
_signExtend, the tag branches, and FrameDecoder._validateFrameEnd) propagate
null/throw rather than converting EOF into valid values.
Renames the calculated D Max fields throughout AeroTuneTab.vue so the variable name unambiguously identifies the value: - calculatePIDs(): roll_d → dMax_roll, pitch_d → dMax_pitch - Table bindings: pids.roll_d → pids.dMax_roll, etc. - applyToFC(): p.roll_d → p.dMax_roll, p.pitch_d → p.dMax_pitch - copyValues() clip text: label updated to D_Max= to match Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/css/tabs/aerotune.less (1)
714-718: Consider increasing contrast on the yaw chirp header.The yaw header uses
#4b9ffftext on a low-opacity blue background (rgba(75, 159, 255, 0.18)), which may have similar contrast issues to those previously fixed in the pitch and roll headers. For consistency with those fixes, consider using#fffffffor the text color.🎨 Suggested adjustment for consistency
.at-chirp-card-header--yaw { background: rgba(75, 159, 255, 0.18); - color: `#4b9fff`; + color: `#ffffff`; border-left: 3px solid `#4b9fff`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/css/tabs/aerotune.less` around lines 714 - 718, Update the .at-chirp-card-header--yaw rule to increase text contrast by changing the text color from `#4b9fff` to `#ffffff` (matching the pitch/roll header fixes); keep the existing background rgba(75, 159, 255, 0.18) and border-left color `#4b9fff` unless you want to further adjust contrast for the border as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 57-62: Update the GitHub Actions step that uses
peaceiris/actions-gh-pages@v3 to use peaceiris/actions-gh-pages@v4; locate the
workflow step named "Deploy to gh-pages" (the uses entry
peaceiris/actions-gh-pages@v3) and change the version tag to `@v4` while keeping
the existing inputs (github_token, publish_dir, publish_branch) unchanged.
---
Nitpick comments:
In `@src/css/tabs/aerotune.less`:
- Around line 714-718: Update the .at-chirp-card-header--yaw rule to increase
text contrast by changing the text color from `#4b9fff` to `#ffffff` (matching the
pitch/roll header fixes); keep the existing background rgba(75, 159, 255, 0.18)
and border-left color `#4b9fff` unless you want to further adjust contrast for the
border as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b72a8669-7acb-4cf3-8445-65399041a614
📒 Files selected for processing (19)
.github/workflows/build.yml.github/workflows/deploy.ymllocales/en/messages.jsonpackage.jsonsrc/components/tabs/AeroTuneTab.vuesrc/components/tabs/pid-tuning/PidSubTab.vuesrc/components/tabs/presets/PresetCard.vuesrc/components/tabs/presets/PresetDetailsDialog.vuesrc/composables/useFirmwareFlashing.jssrc/css/main.lesssrc/css/tabs/aerotune.lesssrc/index.htmlsrc/js/browserMain.jssrc/js/gui.jssrc/js/main.jssrc/js/serial_backend.jssrc/js/vue_components.jssrc/stores/presets.jsvite.config.js
💤 Files with no reviewable changes (1)
- .github/workflows/build.yml
✅ Files skipped from review due to trivial changes (5)
- src/stores/presets.js
- src/components/tabs/presets/PresetDetailsDialog.vue
- src/index.html
- locales/en/messages.json
- src/js/browserMain.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/js/vue_components.js
- src/js/serial_backend.js
- vite.config.js
Comment on lines
+57
to
+62
| - name: Deploy to gh-pages | ||
| uses: peaceiris/actions-gh-pages@v3 | ||
| with: | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| publish_dir: ./dist-files | ||
| publish_branch: gh-pages |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
peaceiris/actions-gh-pages latest version 2024
💡 Result:
The latest version of peaceiris/actions-gh-pages is v4.0.0, released on April 8, 2024.
Citations:
- 1: https://github.com/peaceiris/actions-gh-pages
- 2: https://github.com/peaceiris/actions-gh-pages?search=1
- 3: https://git.durrantlab.pitt.edu/peaceiris/actions-gh-pages
- 4: https://github.com/marketplace/actions/github-pages-action
- 5: https://github.com/peaceiris/actions-gh-pages/releases
🏁 Script executed:
# Check if the file exists and show the relevant lines
if [ -f .github/workflows/deploy.yml ]; then
echo "=== File found, showing lines 55-65 ==="
sed -n '55,65p' .github/workflows/deploy.yml
else
echo "File not found"
fiRepository: betaflight/betaflight-configurator
Length of output: 395
🌐 Web query:
peaceiris/actions-gh-pages v3 vs v4 breaking changes migration
💡 Result:
peaceiris/actions-gh-pages v4.0.0 (released April 8, 2024) is the latest version. No explicit breaking changes or detailed migration guide from v3 to v4 are documented in the README or release notes across sources. Repositories recommend updating workflows from v3 to v4 with minimal changes, primarily changing the version tag in the 'uses' line (e.g., from @v3 to @v4). Key notes for migration: - Use specific tags like @v4.0.0 for stability, as major tags (v3, v4) guarantee no breaking changes within the major version per the project's release strategy. - The README examples use v4 directly, showing identical input syntax to v3 (e.g., github_token, publish_dir, keep_files: true). - keep_files now supports force_orphan (noted as v3 limitation fixed in v4). - Updated to Node 20.11.1 and other deps (from CHANGELOG snippets in Dependabot PRs). - Consider GitHub's official Pages actions (actions/upload-pages-artifact@v3, actions/deploy-pages@v4) as recommended alternative for new workflows. Example v3 to v4 update: v3: uses: peaceiris/actions-gh-pages@v3 with: github_token: ${{ secrets.GITHUB_TOKEN }} publish_dir: ./public v4: uses: peaceiris/actions-gh-pages@v4 with: github_token: ${{ secrets.GITHUB_TOKEN }} publish_dir: ./public No reported widespread breaking issues; updates via Dependabot succeed seamlessly. For full details, check CHANGELOG.md in repo (linked in releases).
Citations:
- 1: https://github.com/peaceiris/actions-gh-pages
- 2: Bump peaceiris/actions-gh-pages from 3 to 4 thomaspoignant/go-feature-flag#1756
- 3: actions-github-pages v4.0.0 peaceiris/actions-gh-pages#1075
- 4: https://github.com/peaceiris/actions-gh-pages/releases
- 5: https://git.durrantlab.pitt.edu/peaceiris/actions-gh-pages
- 6: support: Deployment doesnt take the latest status i.e. pages cannot be updated peaceiris/actions-gh-pages#1124
Update peaceiris/actions-gh-pages to v4.
Version 4 is the latest stable release (April 2024) and is a straightforward update with no breaking changes. The input parameters remain identical.
🔧 Suggested fix
- name: Deploy to gh-pages
- uses: peaceiris/actions-gh-pages@v3
+ uses: peaceiris/actions-gh-pages@v4
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
publish_dir: ./dist-files
publish_branch: gh-pages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Deploy to gh-pages | |
| uses: peaceiris/actions-gh-pages@v3 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_dir: ./dist-files | |
| publish_branch: gh-pages | |
| - name: Deploy to gh-pages | |
| uses: peaceiris/actions-gh-pages@v4 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_dir: ./dist-files | |
| publish_branch: gh-pages |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 58-58: the runner of "peaceiris/actions-gh-pages@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml around lines 57 - 62, Update the GitHub Actions
step that uses peaceiris/actions-gh-pages@v3 to use
peaceiris/actions-gh-pages@v4; locate the workflow step named "Deploy to
gh-pages" (the uses entry peaceiris/actions-gh-pages@v3) and change the version
tag to `@v4` while keeping the existing inputs (github_token, publish_dir,
publish_branch) unchanged.
…tion, correct D baseline - Add _getCrossoverSearchWindow(propInches) mapping 3"–10" to expected GC frequency bands - Pass search window into _computeStabilityMargins; search HIGH→LOW within window for descending 0dB crossing; phase crossover also restricted to same window; null returned when no crossing found in range - runSysID now accepts propInches; passes searchWindow and midpoint baselineHz to _synthesizePID - _synthesizePID uses prop-size midpoint as D-gain adjustment baseline instead of hardcoded 80 Hz - Template: show "Search window: min–max Hz (based on X" prop)" note above stability table; show descriptive "No crossover found in expected range" message instead of N/A when phaseMargin is null - Add .at-sysid-search-window-note and .at-sysid-no-crossover CSS rules in aerotune.less Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rossover, hover-baseline UI labelling Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…PID suggestion for bad data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… — high PM means increase P Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rt per axis above Bode plots
…le with apply/copy
…-size baseline not current FC PIDs
|
Contributor
|
🎉 Do you want to test this code? 🎉 |
Member
|
This is a massive amount of changes, there should be discussion with the development team before creating a PR of this extent. We appreciate the work, but right now this is completely out of the scope for the upcoming release |
Author
Thanks mate, I didn't think it would make the new release. The scatter points work well in GPS tab, this is a nice feature before doing a long range flight ;-) The Aerotune is still in development, I've been using it daily... |
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
2 participants
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.
G'day Betaflight team,
I'm Simon Jardine , one of the first commercial
multirotor operators in Australia, and contributor to early CASA
regulatory frameworks.
I've been flying and tuning multirotors for over 20 years and I
built these features because pilots needed them. Here's what's in
this PR:
AeroTune Tab
PID calculator built into the configurator.
Enter your motor KV, prop size, cell count, weight and flying style
and get flyable PIDs instantly. Applies directly to the FC with one
click.
AUTO TUNE
Betaflight's first one-click auto tune using the
built-in chirp/SysID system. Configure amplitudes, flip one switch
in the air, firmware sweeps Pitch → Roll → Yaw automatically.
Confirmed working on 25.12.
GPS Sky Plot
satellite positions by azimuth/elevation with
lock status colours. Plus a stationary accuracy scatter with live
1σ/2σ accuracy. Know your GPS is solid before you fly.
Filter Frequency Visualizer
Live frequency response curve
showing all your filters — gyro LPFs, D-term filters, dynamic
notches and RPM harmonics — on one graph. Updates as you change
values.
Native BFL file supporting the log analyzer — no more exporting
CSV from Blackbox Explorer first.
It's all live and working at:
https://simonjardine.github.io/betaflight-configurator/
Happy to discuss any of it.
Simon Jardine
aerobot2.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores