MSP-adjCenter_adjScale - #14920
Merged
Merged
MSP-adjCenter_adjScale#14920
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 optional adjustmentCenter and adjustmentScale fields to MSP adjustment range serialization and deserialization in src/main/msp/msp.c, with backwards compatibility guarded by remaining byte checks. Updates src/config submodule reference to a different commit. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
Do you want to test this code? You can flash it directly from the Betaflight App:
WARNING: It may be unstable. Use only for testing! |
Contributor
Author
|
@coderabbitai, can you write a description for this PR. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1106
🏁 Script executed: Length of output: 67
What ChangedThis PR extends the MSP (MultiWii Serial Protocol) to include two additional fields for adjustment ranges:
Modified File:
Changes:
Why This ChangeThis enhancement allows the MSP protocol to transmit center point and scale values for adjustment ranges, enabling configurators and ground control stations to read and write these parameters. This provides more granular control over adjustment configurations, allowing users to define custom center points and scaling factors for their adjustment ranges. Testing
You can edit the PR description and paste this content there! 🧠 Learnings used |
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 14, 2026 00:40
83521cb to
e1c0328
Compare
spatzengr
marked this pull request as ready for review
February 14, 2026 01:12
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/msp/msp.c`:
- Around line 2789-2790: The two unconditional sbufReadU8 calls that assign
adjRange->adjustmentCenter and adjRange->adjustmentScale can overread when older
configurators omit those bytes; update the MSP_SET_ADJUSTMENT_RANGE handling to
check sbufBytesRemaining(src) before reading each new byte (or at least ensure
>=2 bytes remain) and only call sbufReadU8 to populate
adjRange->adjustmentCenter and adjRange->adjustmentScale when the guard passes,
leaving existing values unchanged if the bytes are absent.
- Around line 1457-1458: The serialization uses sbufWriteU8/sbufReadU8 for
adjustmentRange_t's uint16_t fields (adjustmentCenter and adjustmentScale),
causing silent truncation; update the writes in msp.c (where sbufWriteU8(dst,
adjRange->adjustmentCenter) and sbufWriteU8(dst, adjRange->adjustmentScale) are
called) to sbufWriteU16, and update the corresponding reads (where sbufReadU8 is
used to populate adjustmentCenter/adjustmentScale) to sbufReadU16 so the code
matches the uint16_t types in adjustmentRange_t.
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 14, 2026 02:21
e1c0328 to
e1daf5b
Compare
Contributor
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/main/msp/msp.c (1)
1448-1459:⚠️ Potential issue | 🔴 CriticalAPI version not bumped and backward compatibility guard missing for new MSP fields.
This PR modifies the
MSP_ADJUSTMENT_RANGESandMSP_SET_ADJUSTMENT_RANGEmessage formats by adding two new fields (adjustmentCenterandadjustmentScale), but has two issues:
API_VERSION_MINOR not incremented: Per Betaflight convention,
API_VERSION_MINORinmsp_protocol.hshould be bumped immediately when MSP message layouts change. The struct field types (uint8_t) are correct and match thesbufWriteU8/sbufReadU8calls, but the API version was not incremented in this commit. Configurators rely on this to detect protocol changes.Missing backward compatibility guard: In the
MSP_SET_ADJUSTMENT_RANGEhandler (line 2796-2797), the two new fields are read unconditionally:adjRange->adjustmentCenter = sbufReadU8(src); adjRange->adjustmentScale = sbufReadU8(src);Older configurators will not send these bytes. Add a
sbufBytesRemaining(src) >= 2guard before reading them to maintain compatibility with older clients.
Contributor
Author
|
@coderabbitai, can you suggest a patch for to address these issues > #14920 (review) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 5964 🏁 Script executed: Length of output: 1862
Here's a patch to address both issues: diff --git a/src/main/msp/msp.c b/src/main/msp/msp.c
index 1234567..abcdefg 100644
--- a/src/main/msp/msp.c
+++ b/src/main/msp/msp.c
@@ -2786,8 +2786,10 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, int16_t cmdMSP,
adjRange->adjustmentConfig = sbufReadU8(src);
adjRange->auxSwitchChannelIndex = sbufReadU8(src);
- adjRange->adjustmentCenter = sbufReadU8(src);
- adjRange->adjustmentScale = sbufReadU8(src);
+ if (sbufBytesRemaining(src) >= 2) {
+ adjRange->adjustmentCenter = sbufReadU8(src);
+ adjRange->adjustmentScale = sbufReadU8(src);
+ }
activeAdjustmentRangeReset();
}
diff --git a/src/main/msp/msp_protocol.h b/src/main/msp/msp_protocol.h
index 2345678..bcdefgh 100644
--- a/src/main/msp/msp_protocol.h
+++ b/src/main/msp/msp_protocol.h
@@ -59,7 +59,7 @@
*/
`#define` API_VERSION_MAJOR 1
-#define API_VERSION_MINOR 48
+#define API_VERSION_MINOR 49
`#define` API_VERSION_LENGTH 2
`#define` MULTIWII_IDENTIFIER "MWII";Changes explained:
Note: The review mentioned a type mismatch issue, but looking at the current code, the struct correctly uses 🧠 Learnings used |
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 14, 2026 02:32
e1daf5b to
c09c5ca
Compare
Contributor
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/main/msp/msp.c (1)
1448-1459:⚠️ Potential issue | 🟠 MajorIncrement
API_VERSION_MINORfrom 48 to 49 insrc/main/msp/msp_protocol.hto reflect the protocol changes toMSP_ADJUSTMENT_RANGESandMSP_SET_ADJUSTMENT_RANGE. Adding new fields (adjustmentCenterandadjustmentScale) changes the wire protocol, and clients need the bumped version to differentiate between implementations.
Contributor
Author
|
@coderabbitai, the current version of the configurator |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Here's the updated patch with just the backward compatibility guard: diff --git a/src/main/msp/msp.c b/src/main/msp/msp.c
index 1234567..abcdefg 100644
--- a/src/main/msp/msp.c
+++ b/src/main/msp/msp.c
@@ -2786,8 +2786,10 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, int16_t cmdMSP,
adjRange->adjustmentConfig = sbufReadU8(src);
adjRange->auxSwitchChannelIndex = sbufReadU8(src);
- adjRange->adjustmentCenter = sbufReadU8(src);
- adjRange->adjustmentScale = sbufReadU8(src);
+ if (sbufBytesRemaining(src) >= 2) {
+ adjRange->adjustmentCenter = sbufReadU8(src);
+ adjRange->adjustmentScale = sbufReadU8(src);
+ }
activeAdjustmentRangeReset();
}This ensures backward compatibility with the current configurator (2025.12 on API 47) while adding the new fields for the next release. 🧠 Learnings used |
Contributor
Author
|
@coderabbitai, that suggested patch is already implemented in the PR. Please review. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1586
✅ Type consistency: The struct fields are ✅ Backward compatibility: The guard ✅ Protocol asymmetry (by design):
✅ API version: Already at 48 for the next release, so no bump needed. The implementation is solid and ready. The PR correctly extends the MSP protocol for adjustment ranges while maintaining backward compatibility with the current configurator (2025.12 on API 47). 🚀 🧠 Learnings used |
Contributor
Author
|
@coderabbitai, if you are ok with the code, as indicated in your review, can you change your status to approved? |
Contributor
|
✅ Type alignment between struct fields (uint8_t) and MSP operations (sbufReadU8/sbufWriteU8) I'll approve the PR now. 🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 14, 2026 22:52
c09c5ca to
5a4ab07
Compare
Contributor
|
I'd need to look deeper, but I'm not sure that 8 bits will be enough data. If we expect a channel to have 1024 steps, then 8 bit only having 256 wouldn't be able to cover that to have it represent the center. Again, I don't know this code well enough to know if we do need 16 bits or if 8 is plenty, but thats the only possibly concerning thing that I can see. |
KarateBrot
approved these changes
Feb 20, 2026
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 20, 2026 21:51
5a4ab07 to
b50b31c
Compare
haslinghuis
reviewed
Feb 20, 2026
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 20, 2026 22:25
5694d16 to
4ad7b62
Compare
Member
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
haslinghuis
approved these changes
Feb 20, 2026
Quick-Flash
approved these changes
Feb 20, 2026
KarateBrot
approved these changes
Feb 20, 2026
Member
|
Wait, something went wrong, no? The PR looks very different now. |
haslinghuis
requested changes
Feb 20, 2026
Member
There was a problem hiding this comment.
@spatzengr need to update submodules - also please avoid push with force - keeping separate commits - makes previews easier (does not remove comments) - and is a kind of backup
Contributor
Author
@haslinghuis |
Contributor
Author
|
@coderabbitai, can you suggest a patch to fix the build failure at https://github.com/betaflight/betaflight/actions/runs/22248374401/job/64367005221?pr=14920 |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 88273
🏁 Script executed: Length of output: 989 🏁 Script executed: Length of output: 970 🏁 Script executed: Length of output: 1950 🏁 Script executed: Length of output: 2774 🏁 Script executed: Length of output: 1107 🏁 Script executed: Length of output: 699 🏁 Script executed: Length of output: 2776 🏁 Script executed: Length of output: 437 🏁 Script executed: Length of output: 1131 🏁 Script executed: Length of output: 1089 🏁 Script executed: Length of output: 1976 🏁 Script executed: Length of output: 2033
Root Cause:
Solution: #!/bin/bash
# Reset the pico-sdk submodule to match master
cd lib/main/pico-sdk
git checkout ee68c78d0afae2b69c03ae1a72bf5cc267a2d94c
cd ../..
# Stage the change
git add lib/main/pico-sdk
# Commit the fix
git commit -m "Fix: Revert pico-sdk submodule to match master"
# Push the fix
git pushAlternatively, here's a patch you can apply: diff --git a/lib/main/pico-sdk b/lib/main/pico-sdk
index a1438dff1..ee68c78d0 160000
--- a/lib/main/pico-sdk
+++ b/lib/main/pico-sdk
@@ -1 +1 @@
-Subproject commit a1438dff1d38bd9c65dbd693f0e5db4b9ae91779
+Subproject commit ee68c78d0afae2b69c03ae1a72bf5cc267a2d94cThis change was unrelated to your MSP adjustments work and was likely made accidentally when you ran 🧠 Learnings used |
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 21, 2026 02:23
16a5168 to
1f87bca
Compare
Contributor
Author
|
@haslinghuis , sorry for the last Force Push, I needed to clean up the |
Member
|
@spatzengr config submodule is still part of the PR. Try https://www.betaflight.com/docs/development/Git#update-submodules-config 
|
Member
|
@coderabbitai verify #14920 (comment) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1608
diff --git a/src/config b/src/config
index cbc4b7ed33..e378347625 160000
--- a/src/config
+++ b/src/config
@@ -1 +1 @@
-Subproject commit cbc4b7ed331786a448cd5bc0671dbecd3439059e
+Subproject commit e37834762542f9b7087e917431a0141cbb04a4da
# Reset the config submodule to match master
git checkout origin/master -- src/config
# Stage the change
git add src/config
# Commit the fix
git commit -m "Remove unintended config submodule change"
# Push
git pushThis will revert the config submodule back to master's version (cbc4b7ed33) while keeping your MSP changes in 🧠 Learnings used |
Contributor
Author
|
@coderabbitai, verify #14920 (comment) one more time. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 333
Current files changed:
To remove the config submodule change, run: # Reset the config submodule to match master
git checkout origin/master -- src/config
# Stage the change
git add src/config
# Commit the fix
git commit -m "Remove unintended config submodule change"
# Push (don't use --force)
git pushAfter pushing, only 🧠 Learnings used |
spatzengr
force-pushed
the
MSP-adjCenter_adjScale
branch
from
February 21, 2026 11:20
d2ddc75 to
7003417
Compare
Contributor
Author
|
@coderabbitai, verify #14920 (comment) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 273
Current state of PR
The PR is now clean and ready! Great work resolving the submodule issue. 🧠 Learnings used |
haslinghuis
approved these changes
Feb 21, 2026
x4FF3
pushed a commit
to openwch/betaflight
that referenced
this pull request
Apr 7, 2026
Add adjustRange & adjustScale to MSP
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
4 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.
adjustmentCenterandadjustmentRangeto MSP to complement Add adjCenter and adjScale to Adjustments Tab betaflight-configurator#4863 for adding both variables to the Adjustments Tab.Summary by CodeRabbit
Release Notes
Improvements
Chores