Add adjCenter and adjScale to Adjustments Tab#4863
Add adjCenter and adjScale to Adjustments Tab#4863spatzengr wants to merge 1 commit intobetaflight:masterfrom
Conversation
|
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 two new adjustment fields—adjustmentCenter and adjustmentScale—across localization, the Adjustments UI, VirtualFC defaults, and MSP parsing/encoding (conditionally enabled for API_VERSION_1_48+), increasing ADJUSTMENT_RANGES item size from 6→8 bytes when supported. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "AdjustmentsTab (UI)"
participant Model as "VirtualFC"
participant MSP as "MSPHelper"
participant FC as "Flight Controller"
rect rgba(200,200,255,0.5)
UI->>Model: request adjustments
Model-->>UI: return adjustments (include center, scale)
end
rect rgba(200,255,200,0.5)
UI->>UI: user edits center/scale
UI->>Model: update adjustment item (center, scale)
end
rect rgba(255,200,200,0.5)
UI->>MSP: saveAdjustments (encode ADJUSTMENT_RANGES as 8 bytes/item if supported)
MSP->>FC: send MSP_SET_ADJUSTMENT_RANGE payload
FC-->>MSP: ACK/response
MSP-->>UI: confirmation
end
rect rgba(255,255,200,0.5)
FC->>MSP: MSP_ADJUSTMENT_RANGES (8 bytes/item if API>=1.48)
MSP->>Model: parse and populate center/scale
Model-->>UI: refresh adjustments
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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, please write an overview, description and review this PR. |
🧩 Analysis chain🏁 Script executed: Length of output: 5878
PR OverviewThis PR enhances the Adjustments Tab by adding support for two new configuration parameters: Adjustment Center and Adjustment Scale. These fields allow users to fine-tune the behavior of adjustment functions by configuring center point and scaling values directly from the configurator interface. PR DescriptionChanges MadeThis PR introduces UI controls and data handling for two new adjustment parameters:
Technical ImplementationUI Enhancements:
Localization:
Data Handling:
User ExperienceUsers can now configure adjustment center and scale values directly in the Adjustments Tab, providing more granular control over adjustment functions without needing to use the CLI. Now let me review the implementation: Code Review✅ Strengths
💡 Suggestions
<input
type="number"
step="1"
v-model.number="adjustment.adjustmentCenter"
...
/>
📋 Overall AssessmentThe implementation is solid and production-ready. The code is clean, follows Vue best practices, maintains consistency with the existing codebase, and properly handles all edge cases (disabled state, default values, backward compatibility). The grid layout changes are correctly applied to both the header and adjustment rows. Recommendation: ✅ Approve pending minor optional enhancements ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
8dc14bd to
0ac5132
Compare
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
0ac5132 to
7bae6fb
Compare
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
7bae6fb to
795ec80
Compare
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
795ec80 to
7173bfe
Compare
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
7173bfe to
e0624cf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/msp/MSPHelper.js (1)
1014-1028:⚠️ Potential issue | 🔴 CriticalCritical: byte count divisor must be
/8, not/9.The loop reads exactly 8 bytes per item: the original 6 fields (slotIndex, auxChannelIndex, start, end, adjustmentFunction, auxSwitchChannelIndex) plus the two new fields (adjustmentCenter, adjustmentScale). Dividing by 9 will cause incorrect item counts and frame misalignment during parsing.
Update line 1014:
- const adjustmentRangeCount = data.byteLength / 9; // 9 bytes per item. + const adjustmentRangeCount = data.byteLength / 8; // 8 bytes per item.(Note: the SET encoding at line 2632 correctly sends 9 bytes because it includes adjustmentRangeIndex as a leading byte; the GET response has no per-item index.)
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
e0624cf to
09000f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/msp/MSPHelper.js (1)
1014-1028:⚠️ Potential issue | 🔴 CriticalAdd API version guard for MSP_ADJUSTMENT_RANGES structure size change.
The code unconditionally reads 8 bytes per adjustment range, but lacks version gating for the two newer fields (
adjustmentCenter,adjustmentScale). If connected to firmware versions that send only 6 bytes per item,data.byteLength / 8will compute the wrong item count, causing the two extrareadU8()calls to read into subsequent items' data and corrupt all parsed entries.This pattern is inconsistent with how the rest of MSPHelper.js handles protocol changes—20+ similar features are guarded with
semver.gte(FC.CONFIG.apiVersion, API_VERSION_X)checks. The sender code at lines 2639–2640 also uses|| 0fallback, suggesting these fields were added after the base structure.Determine which firmware API version introduced these two fields (check betaflight firmware's msp.c struct), then guard the additional reads and byte-count calculation accordingly:
Example approach
case MSPCodes.MSP_ADJUSTMENT_RANGES: FC.ADJUSTMENT_RANGES = []; - const adjustmentRangeCount = data.byteLength / 8; + const bytesPerItem = semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_4X) ? 8 : 6; + const adjustmentRangeCount = data.byteLength / bytesPerItem; for (let i = 0; i < adjustmentRangeCount; i++) { const adjustmentRange = { slotIndex: data.readU8(), auxChannelIndex: data.readU8(), range: { start: 900 + data.readU8() * 25, end: 900 + data.readU8() * 25, }, adjustmentFunction: data.readU8(), auxSwitchChannelIndex: data.readU8(), - adjustmentCenter: data.readU8(), - adjustmentScale: data.readU8(), + adjustmentCenter: semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_4X) ? data.readU8() : 0, + adjustmentScale: semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_4X) ? data.readU8() : 0, };Replace
API_VERSION_1_4Xwith the actual API version from betaflight firmware that introduced these fields.
🤖 Fix all issues with AI agents
In `@src/js/msp/MSPHelper.js`:
- Around line 2638-2640: The encoder for MSP_SET_ADJUSTMENT_RANGE is adding two
bytes with .push8(adjustmentRange.auxSwitchChannelIndex) and
.push8(adjustmentRange.adjustmentCenter || 0) without checking firmware API
version; mirror the parsing-side safeguard by wrapping those .push8() calls with
the same API version check used when parsing MSP_SET_ADJUSTMENT_RANGE so older
firmware won't receive the extra bytes—locate the build/encode code in
MSPHelper.js where MSP_SET_ADJUSTMENT_RANGE is composed and gate the two .push8
calls with the existing API version conditional used elsewhere in this file.
09000f3 to
58d4d6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/js/msp/MSPHelper.js`:
- Line 1014: The inline comment on the bytesPerItem const is inverted relative
to the ternary; update the comment next to "const bytesPerItem =
semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48) ? 8 : 6" so it correctly
states that the code returns 8 bytes per item when apiVersion is >=
API_VERSION_1_48 and 6 bytes otherwise (note that adjustmentCenter and
adjustmentScale were added in 1.48).
- Around line 2639-2644: Add the missing semicolon after the chained call to
buffer.push8(adjustmentRange.auxSwitchChannelIndex) and correct the indentation
of the subsequent if block: ensure the if (semver.gte(FC.CONFIG.apiVersion,
API_VERSION_1_48)) { ... } block is indented to align with the previous
statement (8 spaces like other chains) and that the chain before it ends with a
semicolon; adjust surrounding lines in the MSPHelper.js code where
buffer.push8(...) and the semver.gte check occur to match the project's
indentation style.
src/js/msp/MSPHelper.js
Outdated
| .push8(adjustmentRange.auxSwitchChannelIndex) | ||
| if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) { | ||
| buffer | ||
| .push8(adjustmentRange.adjustmentCenter || 0) | ||
| .push8(adjustmentRange.adjustmentScale || 0); | ||
| } |
There was a problem hiding this comment.
Fix indentation and add missing semicolon after the chain.
ESLint reports incorrect indentation (12 spaces instead of 8 for the if block). Also, the .push8(adjustmentRange.auxSwitchChannelIndex) expression on line 2639 is missing its terminating semicolon — it works, but every other chain in this function ends with one.
Suggested fix
- .push8(adjustmentRange.auxSwitchChannelIndex)
- if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) {
- buffer
- .push8(adjustmentRange.adjustmentCenter || 0)
- .push8(adjustmentRange.adjustmentScale || 0);
- }
+ .push8(adjustmentRange.auxSwitchChannelIndex);
+ if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) {
+ buffer
+ .push8(adjustmentRange.adjustmentCenter || 0)
+ .push8(adjustmentRange.adjustmentScale || 0);
+ }📝 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.
| .push8(adjustmentRange.auxSwitchChannelIndex) | |
| if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) { | |
| buffer | |
| .push8(adjustmentRange.adjustmentCenter || 0) | |
| .push8(adjustmentRange.adjustmentScale || 0); | |
| } | |
| .push8(adjustmentRange.auxSwitchChannelIndex); | |
| if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) { | |
| buffer | |
| .push8(adjustmentRange.adjustmentCenter || 0) | |
| .push8(adjustmentRange.adjustmentScale || 0); | |
| } |
🧰 Tools
🪛 ESLint
[error] 2640-2640: Expected indentation of 8 spaces but found 12.
(indent)
[error] 2641-2641: Expected indentation of 12 spaces but found 16.
(indent)
[error] 2642-2642: Expected indentation of 16 spaces but found 20.
(indent)
[error] 2643-2643: Expected indentation of 16 spaces but found 20.
(indent)
[error] 2644-2644: Expected indentation of 8 spaces but found 12.
(indent)
🤖 Prompt for AI Agents
In `@src/js/msp/MSPHelper.js` around lines 2639 - 2644, Add the missing semicolon
after the chained call to buffer.push8(adjustmentRange.auxSwitchChannelIndex)
and correct the indentation of the subsequent if block: ensure the if
(semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_48)) { ... } block is indented
to align with the previous statement (8 spaces like other chains) and that the
chain before it ends with a semicolon; adjust surrounding lines in the
MSPHelper.js code where buffer.push8(...) and the semver.gte check occur to
match the project's indentation style.
Update MSPHelper.js Update AdjustmentsTab.vue Update messages.json
spatzengr
force-pushed
the
AdjTab-Center_Scale
branch
from
58d4d6e to
1dceb93
Compare
|
|
🎉 Do you want to test this code? 🎉 |
1 participant
Summary by CodeRabbit
New Features
Documentation (Localization)