-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Add RC smoothing throttle configuration to UI #4644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces throttle-specific RC smoothing fields and UI: adds rcSmoothingThrottleCutoff and rcSmoothingAutoFactorThrottle to RX config, makes MSP parsing/serialization API-version aware (>=1.47 uses throttle fields, older uses legacy feedforward), updates receiver UI/HTML and i18n keys for throttle-specific controls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Receiver UI
participant MSP as MSPHelper
participant FW as Firmware (MSP API)
participant FC as FC.RX_CONFIG
rect rgba(240,248,255,0.6)
note over UI,FW: Load RX config
UI->>MSP: Request MSP_RX_CONFIG
MSP->>FW: Read MSP_RX_CONFIG
FW-->>MSP: Payload
alt API >= 1.47
MSP->>FC: rcSmoothingThrottleCutoff, rcSmoothingAutoFactorThrottle ← payload
else API < 1.47
MSP->>FC: rcSmoothingFeedforwardCutoff, rcSmoothingInputType ← payload
end
MSP-->>UI: Parsed config (includes throttle or legacy fields)
end
rect rgba(240,255,240,0.6)
note over UI,FW: Save RX config
UI->>MSP: Build MSP_SET_RX_CONFIG (from UI/FC)
alt API >= 1.47
MSP->>FW: Include rcSmoothingThrottleCutoff, rcSmoothingAutoFactorThrottle
else API < 1.47
MSP->>FW: Include rcSmoothingFeedforwardCutoff, rcSmoothingInputType
end
FW-->>UI: Ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/js/tabs/receiver.js (3)
678-699
: Hide throttle controls on legacy APIs.For API < 1.47 the throttle smoothing fields are unused, yet this branch only hides the cutoff input and leaves the manual row visible, exposing a dead control. Please hide the entire throttle block for older firmware to avoid confusing users.
- } else { - $(".tab-receiver .rcSmoothing-throttle-cutoff").hide(); + } else { + $(".tab-receiver .rcSmoothing-throttle-manual").hide(); + $(".tab-receiver .rcSmoothing-throttle-cutoff").hide();
702-714
: Fix auto-factor toggle logic for throttle mode.
rcSmoothing-feedforward-select
is hidden for API ≥ 1.47, so its default value of “0” keeps the auto factor row permanently visible even when both setpoint and throttle are manual. Please gate the check on the appropriate control for each API version and include the throttle select in the change handler.- $('select[name="rcSmoothing-setpoint-manual-select"], select[name="rcSmoothing-feedforward-select"]').change( - function () { - if ( - $('select[name="rcSmoothing-setpoint-manual-select"]').val() === "0" || - $('select[name="rcSmoothing-feedforward-select"]').val() === "0" - ) { - $(".tab-receiver .rcSmoothing-auto-factor").show(); - } else { - $(".tab-receiver .rcSmoothing-auto-factor").hide(); - } - }, - ); - $('select[name="rcSmoothing-setpoint-manual-select"]').change(); + const rcSmoothingFeedforwardSelect = $('select[name="rcSmoothing-feedforward-select"]'); + const rcSmoothingThrottleSelect = $('select[name="rcSmoothing-throttle-manual-select"]'); + $( + 'select[name="rcSmoothing-setpoint-manual-select"], select[name="rcSmoothing-feedforward-select"], select[name="rcSmoothing-throttle-manual-select"]', + ).on("change", function () { + const setpointAuto = $('select[name="rcSmoothing-setpoint-manual-select"]').val() === "0"; + const secondaryAuto = semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47) + ? rcSmoothingThrottleSelect.val() === "0" + : rcSmoothingFeedforwardSelect.val() === "0"; + $(".tab-receiver .rcSmoothing-auto-factor").toggle(setpointAuto || secondaryAuto); + }); + $('select[name="rcSmoothing-setpoint-manual-select"]').trigger("change");
990-1009
: ExtendupdateInterpolationView
for throttle smoothing.When RC smoothing is disabled (or the cutoff is zero) the setpoint/feedforward rows are hidden, but the new throttle rows remain visible. Synchronize the visibility logic so throttle controls hide alongside the others under the same conditions.
- if (semver.lt(FC.CONFIG.apiVersion, API_VERSION_1_47)) { - $(".tab-receiver .rcSmoothing-feedforward-manual").show(); - } - - $(".tab-receiver .rcSmoothing-setpoint-manual").show(); + if (semver.lt(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + $(".tab-receiver .rcSmoothing-feedforward-manual").show(); + } else { + $(".tab-receiver .rcSmoothing-throttle-manual").show(); + } + + $(".tab-receiver .rcSmoothing-setpoint-manual").show(); @@ - $(".tab-receiver .rcSmoothing-feedforward-cutoff").hide(); - $(".tab-receiver .rcSmoothing-setpoint-cutoff").hide(); - $(".tab-receiver .rcSmoothing-feedforward-manual").hide(); - $(".tab-receiver .rcSmoothing-setpoint-manual").hide(); - $(".tab-receiver .rcSmoothing-auto-factor").hide(); + $(".tab-receiver .rcSmoothing-feedforward-cutoff").hide(); + $(".tab-receiver .rcSmoothing-setpoint-cutoff").hide(); + $(".tab-receiver .rcSmoothing-feedforward-manual").hide(); + $(".tab-receiver .rcSmoothing-setpoint-manual").hide(); + $(".tab-receiver .rcSmoothing-auto-factor").hide(); + if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + $(".tab-receiver .rcSmoothing-throttle-manual").hide(); + $(".tab-receiver .rcSmoothing-throttle-cutoff").hide(); + } @@ - if (FC.RX_CONFIG.rcSmoothingSetpointCutoff === 0) { - $(".tab-receiver .rcSmoothing-setpoint-cutoff").hide(); - } + if (FC.RX_CONFIG.rcSmoothingSetpointCutoff === 0) { + $(".tab-receiver .rcSmoothing-setpoint-cutoff").hide(); + } + if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47) && FC.RX_CONFIG.rcSmoothingThrottleCutoff === 0) { + $(".tab-receiver .rcSmoothing-throttle-cutoff").hide(); + }
🧹 Nitpick comments (1)
locales/en/messages.json (1)
2101-2103
: Clarify throttle manual help textPlease mirror the phrasing used by the setpoint/feedforward helpers so users understand this toggles the throttle cutoff frequency selection. e.g. “Selects whether the throttle filter cutoff frequency is automatically calculated …”.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json
(2 hunks)src/js/fc.js
(1 hunks)src/js/msp/MSPHelper.js
(2 hunks)src/js/tabs/receiver.js
(2 hunks)src/tabs/receiver.html
(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:957-960
Timestamp: 2025-08-13T15:13:10.214Z
Learning: In src/js/tabs/receiver.js, the updateInterpolationView() function already contains logic to hide ".rcSmoothing-feedforward-cutoff" when FC.RX_CONFIG.rcSmoothingFeedforwardCutoff === 0, which handles the API ≥ 1.47 case since feedforward cutoff isn't initialized from DOM for newer APIs.
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is contained within table rows that have the class rcSmoothing-feedforward-manual, so hiding elements with class rcSmoothing-feedforward-manual automatically hides both the dropdown and the cutoff input field.
Applied to files:
src/tabs/receiver.html
src/js/tabs/receiver.js
📚 Learning: 2025-08-13T15:13:10.214Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:957-960
Timestamp: 2025-08-13T15:13:10.214Z
Learning: In src/js/tabs/receiver.js, the updateInterpolationView() function already contains logic to hide ".rcSmoothing-feedforward-cutoff" when FC.RX_CONFIG.rcSmoothingFeedforwardCutoff === 0, which handles the API ≥ 1.47 case since feedforward cutoff isn't initialized from DOM for newer APIs.
Applied to files:
src/js/msp/MSPHelper.js
src/js/tabs/receiver.js
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is a child of rcSmoothing-feedforward-manual in the DOM structure, so hiding the parent automatically hides the child.
Applied to files:
src/js/tabs/receiver.js
🧬 Code graph analysis (2)
src/js/msp/MSPHelper.js (2)
src/js/fc.js (1)
FC
(131-997)src/js/data_storage.js (2)
API_VERSION_1_47
(5-5)API_VERSION_1_47
(5-5)
src/js/tabs/receiver.js (1)
src/js/data_storage.js (2)
API_VERSION_1_47
(5-5)API_VERSION_1_47
(5-5)
Graphically this looks GREAT! Only issue is that the auto and manual values need to be associated with the channel to which they apply. At present if both setpoint and throttle are auto, there is only one auto field. should be separate auto fields, associated with the relevant channel. Maybe group all the setpoint related switches and value fields togeter, then a separator, then all the throttle related elements? Testing just now also shows that if I choose 'manual mode' for a channel, but then enter a cutoff of zero, and save, the UI does not update to show auto mode now being active for that channel. . if I disconnect/reconnect it does show correctly. |
I must say... I appreciate the complexity in coding this separately for throttle vs setpoint/ff. Life would be even easier if firmware usedsame cutoffs on throttle as for setpoint and ff. i don't personally think that we need throttle to be handled at different frequencies. i see no value in that. So now before a LOT of work, should we change the firmware to share just one auto/manual smoothing configuration, one that applies to setpoint, ff and throttle ? maybe we discuss that in Discord? the firmware coding should not be difficult now we know what to do there. @KarateBrot @blckmn @limonspb - your thoughts about same smoothing for all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/receiver.js
(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:957-960
Timestamp: 2025-08-13T15:13:10.214Z
Learning: In src/js/tabs/receiver.js, the updateInterpolationView() function already contains logic to hide ".rcSmoothing-feedforward-cutoff" when FC.RX_CONFIG.rcSmoothingFeedforwardCutoff === 0, which handles the API ≥ 1.47 case since feedforward cutoff isn't initialized from DOM for newer APIs.
📚 Learning: 2025-08-13T15:13:10.214Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:957-960
Timestamp: 2025-08-13T15:13:10.214Z
Learning: In src/js/tabs/receiver.js, the updateInterpolationView() function already contains logic to hide ".rcSmoothing-feedforward-cutoff" when FC.RX_CONFIG.rcSmoothingFeedforwardCutoff === 0, which handles the API ≥ 1.47 case since feedforward cutoff isn't initialized from DOM for newer APIs.
Applied to files:
src/js/tabs/receiver.js
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is contained within table rows that have the class rcSmoothing-feedforward-manual, so hiding elements with class rcSmoothing-feedforward-manual automatically hides both the dropdown and the cutoff input field.
Applied to files:
src/js/tabs/receiver.js
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is a child of rcSmoothing-feedforward-manual in the DOM structure, so hiding the parent automatically hides the child.
Applied to files:
src/js/tabs/receiver.js
🧬 Code graph analysis (1)
src/js/tabs/receiver.js (1)
src/js/data_storage.js (2)
API_VERSION_1_47
(5-5)API_VERSION_1_47
(5-5)
🔇 Additional comments (2)
src/js/tabs/receiver.js (2)
488-496
: LGTM: API-versioned save logic is correct.The conditional correctly routes throttle cutoff to newer APIs (≥ 1.47) and preserves feedforward cutoff for legacy versions.
988-988
: LGTM: Throttle cutoff visibility logic is consistent.The show/hide logic for throttle cutoff mirrors the existing patterns for setpoint and feedforward cutoffs, correctly hiding when RC smoothing is disabled or when the cutoff value is 0.
Based on learnings.
Also applies to: 1000-1000, 1014-1016
@ctzsnooze I have the feeling we should keep it separate. Most of us are familiar with 5 inch drones but what if this separation of throttle and setpoint is helpful for bigger rigs? I wouldn't remove that only because I personally cannot see a use case. Actually I think it could be quite nice to have a separate smoothing control for throttle in case you want to get rid of nervous jitter in your fingers for cinematic flights etc. Maybe it's also helpful for very light and powerful toothpicks which have super "touchy" throttle response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- pre-approving for potential firmware merge.
|
true, we have firmware and cli support for separate auto and manual values for setpoint and throttle, and in fact we always have. The more gerneric So it's legal to set this in CLI, where both setpoint and throttle are in auto mode, but auto smoothing is set to 30 for setpoint, but only 12 for throttle:
|
|
Preview URL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/receiver.js (1)
688-688
: Consider improving visibility logic clarity.Line 688 hides the throttle cutoff UI for older APIs, but this is already handled by the conditional block starting at line 664. While not incorrect, it creates redundancy that could be clarified with a comment.
Consider adding a clarifying comment:
} else { + // For API < 1.47, hide throttle controls and use legacy feedforward controls $(".tab-receiver .rcSmoothing-throttle-cutoff").hide();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json
(2 hunks)src/js/fc.js
(1 hunks)src/js/msp/MSPHelper.js
(2 hunks)src/js/tabs/receiver.js
(6 hunks)src/tabs/receiver.html
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/fc.js
- locales/en/messages.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-13T15:13:10.214Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:957-960
Timestamp: 2025-08-13T15:13:10.214Z
Learning: In src/js/tabs/receiver.js, the updateInterpolationView() function already contains logic to hide ".rcSmoothing-feedforward-cutoff" when FC.RX_CONFIG.rcSmoothingFeedforwardCutoff === 0, which handles the API ≥ 1.47 case since feedforward cutoff isn't initialized from DOM for newer APIs.
Applied to files:
src/js/tabs/receiver.js
src/tabs/receiver.html
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is contained within table rows that have the class rcSmoothing-feedforward-manual, so hiding elements with class rcSmoothing-feedforward-manual automatically hides both the dropdown and the cutoff input field.
Applied to files:
src/js/tabs/receiver.js
src/tabs/receiver.html
📚 Learning: 2025-08-13T15:12:48.509Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4569
File: src/js/tabs/receiver.js:650-674
Timestamp: 2025-08-13T15:12:48.509Z
Learning: In src/js/tabs/receiver.js, the rcSmoothing-feedforward-cutoff element is a child of rcSmoothing-feedforward-manual in the DOM structure, so hiding the parent automatically hides the child.
Applied to files:
src/js/tabs/receiver.js
🧬 Code graph analysis (2)
src/js/tabs/receiver.js (1)
src/js/data_storage.js (2)
API_VERSION_1_47
(5-5)API_VERSION_1_47
(5-5)
src/js/msp/MSPHelper.js (2)
src/js/fc.js (1)
FC
(131-998)src/js/data_storage.js (2)
API_VERSION_1_47
(5-5)API_VERSION_1_47
(5-5)
🔇 Additional comments (11)
src/js/msp/MSPHelper.js (2)
1051-1057
: LGTM! API-versioned parsing correctly handles throttle fields.The parsing logic properly reads
rcSmoothingThrottleCutoff
andrcSmoothingAutoFactorThrottle
for API >= 1.47, while maintaining backward compatibility by reading the deprecatedrcSmoothingFeedforwardCutoff
for older firmware versions.
2003-2011
: LGTM! API-versioned emission correctly handles throttle fields.The emission logic properly writes
rcSmoothingThrottleCutoff
andrcSmoothingAutoFactorThrottle
for API >= 1.47, while maintaining backward compatibility by writingrcSmoothingFeedforwardCutoff
andrcSmoothingInputType
for older firmware versions. The symmetry with the parsing logic ensures correct round-trip behavior.src/js/tabs/receiver.js (5)
488-506
: LGTM! Save logic correctly handles throttle cutoff.The save configuration properly:
- Reads
rcSmoothingThrottleCutoff
for API >= 1.47 (line 493-495)- Syncs the dropdown to auto mode when cutoff is 0 (lines 496-498)
- Reads
rcSmoothingAutoFactorThrottle
(lines 499-501)- Falls back to legacy
rcSmoothingFeedforwardCutoff
for older APIs (lines 503-505)This addresses the UI sync bug mentioned in the PR comments by updating the dropdown state when saving a manual cutoff of 0.
664-686
: LGTM! Throttle control initialization is correct.The initialization logic properly:
- Hides legacy feedforward controls for API >= 1.47 (line 665)
- Sets up throttle cutoff input with current value (lines 667-668)
- Initializes dropdown state based on cutoff value (lines 670-674)
- Configures change handler to show/hide cutoff input (lines 675-686)
- Triggers initial change event (line 686)
The visibility logic correctly handles the manual=0 → auto mode transition.
731-735
: LGTM! Auto factor throttle initialization is correct.The code properly initializes the throttle auto factor input for API >= 1.47 and hides it for older firmware versions, maintaining version-specific UI behavior.
999-1025
: Verify visibility logic matches UI requirements from PR comments.The PR comments from ctzsnooze indicate that separate auto/manual controls should be shown for setpoint and throttle channels. The current implementation shows/hides controls based on cutoff values, but the comment suggests the UI should display separate auto fields even when both are in auto mode.
Current behavior:
- Lines 1007-1012: Shows auto factor controls only when either feedforward or setpoint cutoff is 0
- Lines 1017-1026: Hides controls when RC smoothing is disabled
Expected behavior (per PR comments):
- Show separate auto factor fields for setpoint and throttle
- Each channel should display its own auto/manual state
Please verify if the current visibility logic aligns with the intended UX described in the PR comments. The comments suggest showing "separate auto fields associated with each channel" rather than a single shared auto factor control.
Based on PR comments by ctzsnooze:
when both channels are in auto there is only a single auto field; the UI should show separate auto fields associated with each channel
1036-1038
: LGTM! Final throttle cutoff visibility guard is correct.The code properly hides the throttle cutoff UI when the value is 0, consistent with the auto mode behavior.
src/tabs/receiver.html (4)
230-244
: LGTM! Auto factor row structure is correct.The row properly defines the setpoint auto factor input with appropriate i18n keys and help icon. The structure is consistent with other configuration rows.
246-274
: LGTM! Throttle manual controls are correctly structured.The throttle-specific rows properly:
- Provide a manual/auto select dropdown (lines 246-263)
- Include a throttle cutoff input field (lines 264-274)
- Use appropriate i18n keys (
receiverThrottleTypeSelect
,receiverRcSmoothingThrottleCutoffHz
)- Include help icons with throttle-specific help keys
This structure enables separate control of throttle smoothing as requested in the PR comments.
276-290
: LGTM! Auto factor throttle row is correctly structured.The row properly defines the throttle auto factor input with appropriate i18n keys (
receiverRcSmoothingAutoFactorThrottle
) and help icon. This provides the separate throttle auto factor field mentioned in the PR comments.
292-320
: LGTM! Feedforward controls are correctly restored for backward compatibility.The feedforward manual/auto select and cutoff rows (lines 292-320) are properly structured to support older firmware versions (API < 1.47). The structure mirrors the throttle controls and uses appropriate i18n keys (
receiverRcFeedforwardTypeSelect
,receiverRcSmoothingFeedforwardCutoff
).This maintains backward compatibility while supporting the new throttle-specific controls for newer firmware.
Summary by CodeRabbit
New Features
Localization