feat: Migrate PID tuning tab to Vue 3 / Pinia#4848
Open
haslinghuis wants to merge 72 commits intobetaflight:masterfrom
Open
feat: Migrate PID tuning tab to Vue 3 / Pinia#4848haslinghuis wants to merge 72 commits intobetaflight:masterfrom
haslinghuis wants to merge 72 commits intobetaflight:masterfrom
Conversation
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 complete Vue 3 + Pinia implementation of the PID Tuning tab: new PidTuningTab with PID/Rates/Filter sub-tabs, Pinia pidTuning store for snapshot/change tracking, profile selection dialog + composable, tab mounting/adapter wiring, i18n key, and a detailed MIGRATION.md plan. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PidTuningTab
participant PiniaStore
participant FC
participant MSP
User->>PidTuningTab: Mount component
PidTuningTab->>FC: Read CONFIG (profiles)
PidTuningTab->>MSP: Request controller/profile names (API gated)
PidTuningTab->>MSP: Request PID, advanced, RC tuning, filter config
MSP-->>PidTuningTab: Return data
PidTuningTab->>PiniaStore: storeOriginals(...)
User->>PidTuningTab: Modify sliders
PidTuningTab->>PiniaStore: checkForChanges(...)
PiniaStore-->>PidTuningTab: hasChanges = true
User->>PidTuningTab: Click Save
PidTuningTab->>MSP: MSP2_SET_TEXT (profile names) (API gated)
PidTuningTab->>MSP: Persist (EEPROM/write)
PidTuningTab->>FC: Update CONFIG
PidTuningTab->>PiniaStore: storeOriginals(...) (reset)
PidTuningTab->>PidTuningTab: Reinitialize sliders
User->>PidTuningTab: Click Revert
PidTuningTab->>PiniaStore: revertToOriginals()
PiniaStore->>FC: Restore FC data
PidTuningTab->>PidTuningTab: Reinitialize sliders
sequenceDiagram
participant User
participant PidTuningTab
participant useDialog
participant ProfileSelectionDialog
participant MSP
participant FC
User->>PidTuningTab: Click "Copy Profile"
PidTuningTab->>useDialog: openProfileSelection(...)
useDialog->>ProfileSelectionDialog: show()
ProfileSelectionDialog-->>User: display options
User->>ProfileSelectionDialog: Select & confirm
ProfileSelectionDialog-->>useDialog: emit confirm(selected)
useDialog-->>PidTuningTab: onConfirm(selected)
PidTuningTab->>FC: Prepare COPY_PROFILE payload
PidTuningTab->>MSP: MSP_COPY_PROFILE
MSP-->>FC: Profile copied
PidTuningTab->>PidTuningTab: Reload MSP/FC data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 2
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/PidSubTab.vue`:
- Around line 521-545: Remove the temporary debug console.log calls inside
initializeSliders (the logs that reference TuningSliders.sliderDGain,
sliderDGain.value, and sliderDGainDisplay.value) and any similar console.log
statements around lines 608–611; keep the assignment logic (sliderPidsMode,
sliderDGain, sliderPIGain, etc.), nextTick() call, and profileName handling
intact so functionality is unchanged, only stripping out the debug output.
In `@src/js/main.js`:
- Around line 516-522: Remove the verbose debug console.log call before merging:
delete the console.log(...) statement that logs GUI.active_tab and TABS around
the expertModeChanged invocation (the block referencing GUI.active_tab and
TABS[GUI.active_tab]), leaving only the call to
TABS[GUI.active_tab]?.expertModeChanged?.(checked); or alternatively gate the
log behind a debug flag if runtime diagnostics are required.
🧹 Nitpick comments (2)
MIGRATION.md (1)
487-487: Optional: Add language specifiers to fenced code blocks.Static analysis flagged several fenced code blocks without language specifiers (lines 487, 671, 737, 874, etc.). Adding language identifiers (e.g.,
```textor```plaintext) would improve markdown compliance.src/components/tabs/PidTuningTab.vue (1)
360-364: Consider removing unused cleanup function.The
cleanupfunction is defined but never exposed or called. ThetabAdapterinvue_tab_mounter.jsprovides its own cleanup implementation that simply invokes the callback immediately.🧹 Proposed removal
-// Cleanup callback - called from gui.js tab_switch_cleanup when switching away from this tab -function cleanup(callback) { - // Any cleanup needed before unmounting - // Call the callback to signal cleanup is complete - if (callback) callback(); -} - // No need to expose methods - using global reactive state via tabState // defineExpose is not needed anymore
37 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/FilterSubTab.vue`:
- Around line 501-508: The number input bound to dterm_lowpass_dyn_max_hz is
unusable because both min and max are set to 1000; update the input attributes
in the component (the <input> controlled by dterm_lowpass_dyn_max_hz shown next
to dtermLowpassEnabled and dtermLowpassMode) so the max is larger than the min
(or remove the max constraint) and choose a sensible upper bound (e.g., 10000 or
the system’s allowed maximum) and adjust step/min as needed to allow user
adjustments.
In `@src/components/tabs/pid-tuning/PidSubTab.vue`:
- Around line 1294-1315: The onSliderChange handler is scheduling multiple
setTimeouts which can flip isUserInteracting.value too early; introduce a
component-scoped timer id (e.g., userInteractionTimeout or
userInteractionTimeoutId) and call clearTimeout(userInteractionTimeout) at the
start of onSliderChange before creating the new timeout, then assign the result
of setTimeout to that variable; ensure the timer variable is declared outside
onSliderChange (and cleared in the component unmount/cleanup) so rapid slider
events cancel previous timers and only the latest timeout resets
isUserInteracting.value.
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 1527-1531: The throttle curve watcher only tracks throttleMid,
throttleHover and throttleExpo so drawThrottleCurve() can miss updates when
throttleLimitType or throttleLimitPercent change; update the watcher to include
throttleLimitType and throttleLimitPercent (alongside
throttleMid/throttleHover/throttleExpo) so that nextTick -> drawThrottleCurve()
runs whenever any of those reactive values change, referencing the existing
watcher and drawThrottleCurve() function to make the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/PidSubTab.vue`:
- Around line 1063-1088: The profile name edits stored in the profileName ref
are never written back to the legacy config array; update the save/submit
handler that persists PID settings to set
FC.CONFIG.pidProfileNames[FC.CONFIG.profile] = profileName.value.trim() (and
then trigger whatever existing persistence/refresh logic runs after saving).
Locate the profileName ref and the save method used for PID profiles and ensure
you write the trimmed profileName.value into FC.CONFIG.pidProfileNames at index
FC.CONFIG.profile before completing the save.
- Around line 876-915: The TPA inputs are bound to the wrong RC_TUNING fields:
change the tpaBreakpoint input to bind to rcTuning.dynamic_THR_breakpoint
(replace v-model.number="rcTuning.TPA_BREAKPOINT"), remove or stop binding the
tpaRate input to dynamic_THR_breakpoint
(v-model.number="rcTuning.dynamic_THR_breakpoint") and instead either
remove/conditionally render the rate input when the rate field doesn't exist
(API < 1.45) or bind it only when a proper rate property exists; update
references to rcTuning.dynamic_THR_PID, rcTuning.dynamic_THR_breakpoint, and the
tpaRate/tpaBreakpoint inputs in PidSubTab.vue accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 391-397: The recursive initModel retry timer can outlive the
component and call getBoundingClientRect on null refs; assign the setTimeout id
to rcUpdateInterval whenever scheduling retries inside initModel, add guards at
the top of initModel (and any DOM access sites) to return early if the related
refs are null before calling getBoundingClientRect, and clear the timer on
unmount (and also cancel animationFrameId and stop rendering by setting
keepRendering = false) in the component's beforeUnmount/unmounted hook so
rcUpdateInterval is cleared and no further retries run after refs are nulled.
In `@src/components/tabs/PidTuningTab.vue`:
- Around line 253-263: The three no-op functions copyProfile, copyRateProfile,
and resetProfile are hooked to header buttons but do nothing; restore the legacy
behaviors by porting the dialog and action logic from the previous tab
implementation: implement copyProfile to open the profile-copy dialog and
dispatch the same store action or emit the same event used in the legacy tab,
implement copyRateProfile to open the rate-profile-copy dialog and perform the
same copy routine, and implement resetProfile to show the reset confirmation
dialog and call the reset action on confirm; if you cannot port the dialogs now,
disable or hide the header buttons instead so users cannot trigger these TODO
functions. Ensure you reference and reuse the legacy dialog component names and
store action names when wiring these functions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 391-396: The code reuses rcUpdateInterval for both init retry
timeouts and the recurring RC update interval, which can orphan a pending
timeout; fix by introducing a separate variable (e.g., initModelTimeoutId) used
exclusively by initModel() for setTimeout retries and leave rcUpdateInterval for
the setInterval RC updates (references: rcUpdateInterval, initModel,
animationFrameId, keepRendering); ensure initModel stores timeouts in
initModelTimeoutId, clear/initModelTimeoutId on successful init and in the
component unmount/cleanup path, and only clear rcUpdateInterval where you stop
the regular RC updates.
- Around line 979-996: The code currently calls balloon.draw() twice by
iterating over balloons before and after drawAngleModeLabels; remove the first
loop (the for (const balloon of balloons) { balloon.draw(); } that appears
before the drawAngleModeLabels call) so each balloon is rendered only once,
keeping the single draw loop that follows drawAngleModeLabels; this touches the
balloons array iteration and uses symbols balloon.draw(), drawAngleModeLabels,
isBetaflightRates, ratesType, balloonsDirty, ctx, canvas, and rates.
In `@src/components/tabs/PidTuningTab.vue`:
- Around line 253-285: The copyProfile function is using the undefined variable
profile.value; replace all uses of profile.value with currentProfile.value
(e.g., in the exclusion check when building options, and when setting
FC.COPY_PROFILE.srcProfile and the console log). Ensure copyProfile still sets
FC.COPY_PROFILE.type = 0 and FC.COPY_PROFILE.dstProfile = targetProfile, and
that the MSP call (MSP.promise with MSPCodes.MSP_COPY_PROFILE and
mspHelper.crunch) remains unchanged.
- Around line 287-319: The copyRateProfile function incorrectly references
rateProfile.value (undefined) instead of the actual reactive/current value
currentRateProfile; update all occurrences of rateProfile.value in
copyRateProfile to use currentRateProfile (or currentRateProfile.value if it is
a ref), e.g., when building options exclusion, setting
FC.COPY_PROFILE.srcProfile, logging the copy operation, and anywhere else in
that function; ensure FC.COPY_PROFILE assignment still uses the numeric profile
index and keep the existing MSP.promise(MSPCodes.MSP_COPY_PROFILE,
mspHelper.crunch(...)) call and error handling unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 1584-1591: Track and clear the nested 100ms setTimeout so its
callback won't run after unmount: store the timeout id returned by setTimeout in
a variable (e.g., modelInitTimeout) when scheduling the callback that adds
window.addEventListener("resize", handleModelResize) and starts the animation
loop (sets lastTimestamp and animationFrameId via
requestAnimationFrame(renderModel)); then in the component teardown
(onUnmounted) clearTimeout(modelInitTimeout), remove the resize listener
(handleModelResize), and cancelAnimationFrame(animationFrameId) to fully clean
up.
🧹 Nitpick comments (3)
src/components/tabs/pid-tuning/RatesSubTab.vue (2)
1438-1443: Empty function can be removed or implemented.
draw3DRatesPreview()is defined but only contains a comment. If it's not needed, consider removing it to avoid confusion.
1520-1521: Remove duplicate comment.Lines 1520-1521 have the same comment "// Watch for changes and redraw" repeated.
🧹 Suggested fix
-// Watch for changes and redraw // Watch for changes and redraw watch([ratesType, rcRate, rcRatePitch, rcRateYaw, rollRate, pitchRate, yawRate, rcExpo, rcPitchExpo, rcYawExpo], () => {src/components/tabs/PidTuningTab.vue (1)
447-456: Unusedcleanupfunction is dead code.The
cleanupfunction is defined but never exposed viadefineExposeor called. The comment at line 454-455 confirmsdefineExposeisn't used. Either remove this function or expose it if it's needed bygui.jstab_switch_cleanup.🧹 Suggested fix - remove unused function
-// Cleanup callback - called from gui.js tab_switch_cleanup when switching away from this tab -function cleanup(callback) { - // Any cleanup needed before unmounting - // Call the callback to signal cleanup is complete - if (callback) callback(); -} - -// No need to expose methods - using global reactive state via tabState -// defineExpose is not needed anymore
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 436-440: The computed showMaxRateWarning currently returns false;
change it to return true when any of the maxAngularVelRoll, maxAngularVelPitch,
or maxAngularVelYaw values exceed 1800 (matching the legacy check in
src/js/tabs/pid_tuning.js), e.g., compute and return (maxAngularVelRoll > 1800
|| maxAngularVelPitch > 1800 || maxAngularVelYaw > 1800) so the template warning
displays when an axis angular velocity exceeds 1800°/s.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 436-443: showMaxRateWarning uses parseInt on
maxAngularVelRoll/maxAngularVelPitch/maxAngularVelYaw which return "" for
Betaflight rates, producing NaN and silencing the warning; update
showMaxRateWarning to use the numeric max-angular-velocity values instead
(either compute numeric velocities directly inside the computed or reference
existing numeric computed/methods that produce degrees/sec regardless of
isBetaflightRates), e.g., derive numericMaxAngularVelRoll/Pitch/Yaw or expose
the numeric computed for use here and then compare those against
MAX_RATE_WARNING; apply the same change to the other occurrences that check max
angular velocities (the similar logic around the later block noted in the
review).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/tabs/pid-tuning/RatesSubTab.vue`:
- Around line 1627-1643: The untracked setTimeout call used to delay the initial
draw should be assigned to a variable (e.g., initTimeout) so it can be cleared
on component teardown; wrap the existing setTimeout(...) invocation (which calls
nextTick -> drawRateCurves, drawThrottleCurve, updateRatesLabels) into a tracked
timer and add an onUnmounted handler that calls clearTimeout(initTimeout) to
prevent the callback from running after unmount; keep the existing immediate
nextTick call as-is and rely on the existing null guards in
drawRateCurves/drawThrottleCurve/updateRatesLabels.
🧹 Nitpick comments (2)
src/components/tabs/pid-tuning/RatesSubTab.vue (2)
1456-1461: Unused functiondraw3DRatesPreviewcan be removed.This function is defined but never called anywhere in the component. The 3D preview rendering is handled by
renderModel()instead. Consider removing this dead code to reduce confusion.🧹 Suggested removal
-function draw3DRatesPreview() { - if (!ratesPreviewCanvas.value || !model) return; - - // This function is called continuously via requestAnimationFrame - // It rotates the 3D model based on current RC input -} -
1538-1539: Duplicate comment can be removed.Line 1538 duplicates the comment on line 1539.
🧹 Suggested fix
-// Watch for changes and redraw // Watch for changes and redraw watch([ratesType, rcRate, rcRatePitch, rcRateYaw, rollRate, pitchRate, yawRate, rcExpo, rcPitchExpo, rcYawExpo], () => {
- Replace hard-coded UI strings in Gyro Lowpass blocks with (...) for translation support - Ensures all user-facing labels use i18n keys for consistent localization
haslinghuis
force-pushed
the
vue-pid-tuning
branch
from
74dddcc to
6fa264e
Compare
…idSubTab.vue (partial)
|
Contributor
|
🎉 Do you want to test this code? 🎉 |
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.
1. Architectural Shift: From Monolith to Sub-Tabs
The legacy pid_tuning.js was a "mega-file" that handled everything from rate calculations to filter graphs. PR #4848 breaks this down into a more maintainable Vue structure:
Component-Based Sub-tabs: It separates the PID Settings, Filter Settings, and Rate Settings into distinct Vue components. This mirrors the pattern seen in the Receiver Tab migration but at a much larger scale.
The Store Pattern: Like other recent migrations (e.g., the Setup Tab), it heavily utilizes a centralized store (likely Pinia) to sync the MSP_PID, MSP_FILTER_CONFIG, and MSP_RC_TUNING data. This ensures that if you change a value in the "Simplified Tuning" slider, the raw PID values in the adjacent sub-tab stay in sync without manual DOM refreshes.
2. The "Simplified Tuning" Logic
One of the biggest challenges in this PR is the Simplified Tuning Sliders.
Legacy: In the jQuery version, moving a slider triggered a chain of imperative math functions that recalculated hidden fields before sending the MSP command.
Vue Migration: PR #4848 implements this using Computed Properties. This is a significant upgrade in reliability; the relationship between the "Master Multiplier" and the individual Kp,Ki,Kd values is now declarative. It’s much harder for the UI to show a value that doesn't match what the Flight Controller (FC) is actually receiving.
3. Rate Preview & Graphing
The Rates sub-tab migration is the "heavy lifter" of this PR.
Current implementation: Uses a custom canvas-based drawing routine.
Migration: The PR refactors this into a reactive Vue component. It has to handle multiple "Rate Types" (Betaflight, Actual, RaceFlight, etc.).
Consistency: It follows the visual style of the Blackbox Explorer Vue components, aiming for a unified "look and feel" for all graph data across the Betaflight ecosystem.
4. Safety & Validation
The PID tab is where users are most likely to "brick" their flight performance with a typo.
Input Validation: PR #4848 leverages Vue’s reactive form validation. Unlike the legacy version, which often relied on onBlur events to catch errors, the Vue version can provide real-time feedback (red borders/tooltips) as the user types, preventing invalid MSP packets from being sent.
Consistency with other Migrations
This PR follows the "Gold Standard" set by the Receiver Tab (#4830) migration by:
Summary by CodeRabbit
New Features
Localization
Documentation