Improve msp #4632
Improve msp #4632
Conversation
|
Warning Rate limit exceeded@haslinghuis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughSwitched charCodeAt -> codePointAt for Unicode-safe byte extraction; consolidated timeouts into a single public MSP.TIMEOUT (1000) and removed MIN_TIMEOUT/MAX_TIMEOUT/timeout; added CRC-based duplicate-request detection using Array.some; standardized const for buffer views; replaced clearInterval with clearTimeout in callback cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant MSP
participant Serial
participant Timer
Caller->>MSP: send_message(request, callbacks)
MSP->>MSP: build buffer (encoders use codePointAt)
MSP->>MSP: compute CRC, check existing requests with Array.some
alt duplicate request found
MSP-->>Caller: return / reuse existing request handling
else new request
MSP->>Serial: write(buffer)
alt not serial.connected
MSP-->>Caller: error/return
else connected
MSP->>Timer: start TIMEOUT (MSP.TIMEOUT)
Serial-->>MSP: bytesSent events
MSP-->>Caller: callback_sent when bytesSent == buffer.length
alt response received before TIMEOUT
Serial-->>MSP: response
MSP->>Timer: clearTimeout()
MSP-->>Caller: success callbacks
else TIMEOUT fires
Timer-->>MSP: timeout
MSP->>Timer: clearTimeout()
MSP-->>Caller: timeout handling
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
haslinghuis
force-pushed
the
update-msp
branch
from
674994c to
e3ece6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/js/msp/MSPHelper.js (1)
93-101: Guard non-ASCII/code-unit handling when pushing text bytescodePointAt may return values > 255 and config may be null/undefined. push8 will truncate silently; also surrogate pairs are stepped by 1 here. At minimum, guard the input and mask to 8‑bit to make intent explicit and avoid TypeErrors.
Apply this diff:
@@ - const size = Math.min(length, config.length); + const text = typeof config === "string" ? config : ""; + const size = Math.min(length, text.length); @@ - for (let i = 0; i < size; i++) { - buffer.push8(config.codePointAt(i)); + for (let i = 0; i < size; i++) { + const cp = text.codePointAt(i) ?? 0x20; + buffer.push8(cp & 0xff); // MSP text fields are 8-bit }And similarly mask to 8‑bit in other call sites:
- buffer.push8(FC.CONFIG.name.codePointAt(i)); + buffer.push8(FC.CONFIG.name.codePointAt(i) & 0xff); @@ - buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i)); + buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i) & 0xff); @@ - buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i)); + buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i) & 0xff); @@ - buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0)); + buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0) & 0xff); @@ - buffer.push8(" ".codePointAt(0)); + buffer.push8(" ".codePointAt(0) & 0xff);Optionally, if MSP fields are strictly ASCII, consider replacing non-ASCII with '?' to avoid ambiguous truncation.
Also applies to: 2252-2260, 2358-2364, 2370-2378
src/js/msp.js (3)
359-371: Iterate code points safely for CLI payloadArray.from on a string iterates UTF‑16 code units, not code points. For non‑BMP chars this will emit two entries. If CLI is 8‑bit/ASCII, mask to 0xff and/or iterate with for...of to avoid splitting surrogate pairs.
Apply this diff:
- const data = Array.from(str, (c) => c.codePointAt(0)); + const data = []; + for (const ch of str) { + data.push((ch.codePointAt(0) ?? 0x20) & 0xff); + }
381-386: Minor: remove unused variable and consider early return shapeconnected is assigned but unused. Also, when short‑circuiting, calling callback_msp with a structured result (including an error indicator) could be more consistent, but optional.
Apply this diff:
- send_message(code, data, callback_sent, callback_msp, doCallbackOnError) { - const connected = serial.connected; + send_message(code, data, callback_sent, callback_msp, doCallbackOnError) {
406-417: Timeout handling can leak callbacks; avoid resend-on-timeout or add cleanupWhen the timer fires, the code resends once but doesn’t remove the callback or schedule a follow-up; clearTimeout inside the timeout handler has no effect. This can leave stale entries in MSP.callbacks if no response ever comes.
Prefer one of:
- Do not resend on timeout; instead, remove the callback and optionally notify via callbackOnError.
- Or, resend and also schedule a second timeout to hard-clean the callback if still unresolved.
Minimal change (no resend, cleanup on timeout):
- obj.timer = setTimeout(() => { - console.warn( - `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`, - ); - serial.send(bufferOut, (_sendInfo) => { - obj.stop = performance.now(); - const executionTime = Math.round(obj.stop - obj.start); - clearTimeout(obj.timer); // prevent leaks - }); - }, this.TIMEOUT); + obj.timer = setTimeout(() => { + console.warn( + `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`, + ); + const idx = this.callbacks.indexOf(obj); + if (idx !== -1) this.callbacks.splice(idx, 1); + if (doCallbackOnError && typeof callback_msp === "function") { + callback_msp({ command: code, data: new DataView(new ArrayBuffer(0)), length: 0, crcError: true, timeout: true }); + } + }, this.TIMEOUT);This aligns with the PR goal of reducing serial stress (no immediate retry) and fixes the leak.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(7 hunks)src/js/msp/MSPHelper.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
📚 Learning: 2025-09-19T20:41:42.451Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Applied to files:
src/js/msp.js
🧬 Code graph analysis (2)
src/js/msp/MSPHelper.js (1)
src/js/fc.js (2)
i(264-264)FC(131-990)
src/js/msp.js (3)
src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/data_storage.js (1)
CONFIGURATOR(7-38)src/js/gui.js (1)
GUI(465-465)
🔇 Additional comments (7)
src/js/msp/MSPHelper.js (1)
1768-1769: Good fix: clear the right timer APISwitching to clearTimeout matches the timer created with setTimeout.
src/js/msp.js (6)
7-12: Symbols via codePointAt are fineASCII constants resolve to the same byte values; change is safe.
61-61: Public TIMEOUT constant is a good simplificationCentralizing timeout to MSP.TIMEOUT improves consistency.
Please confirm no remaining references to deprecated timeout controls (MIN_TIMEOUT/MAX_TIMEOUT/timeout) exist elsewhere.
290-292: Listener registration check is fineStrict check with indexOf avoids duplicates.
320-321: Const views in encoder improve clarityImmutability here is appropriate.
424-426: Good: only acknowledge callback_sent on full writeChecking bytesSent matches buffer length avoids false positives.
389-397: Duplicate detection: preserve multi-callback for MSP_MULTIPLE_MSP
OnlyMSP_MULTIPLE_MSP(src/js/msp/MSPHelper.js:1736) passes an array of callbacks—confirm CRC+length+code matching still registers all queued callbacks; fall back to code+payloadSize-only if it suppresses new callbacks.
There was a problem hiding this comment.
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 (1)
src/js/msp/MSPHelper.js (1)
93-100: Text encoding length/count mismatch and potential truncation; prefer code‑point aware loop and explicit 8‑bit clampUsing codePointAt(i) with i++ counts UTF‑16 code units, not code points. For surrogate pairs this can miscount size and push truncated bytes (>0xFF truncated). Since getText() reads a length byte then 8‑bit chars, encode with a code‑point aware iteration and clamp to 0xFF.
Apply:
- const size = Math.min(length, config.length); - // length byte followed by the actual characters - buffer.push8(size); - - for (let i = 0; i < size; i++) { - buffer.push8(config.codePointAt(i)); - } + const codepoints = Array.from(config); // code-point aware + const size = Math.min(length, codepoints.length); + buffer.push8(size); + for (let i = 0; i < size; i++) { + buffer.push8(codepoints[i].codePointAt(0) & 0xff); + }
🧹 Nitpick comments (7)
src/js/msp/MSPHelper.js (3)
2252-2256: Clamp name bytes to 8‑bit; current code may truncate high code points inconsistentlyMSP_SET_NAME payload is byte-based. Explicitly clamp to 0xFF (consistent with read path) to avoid relying on implicit TypedArray coercion.
- for (let i = 0; i < FC.CONFIG.name.length && i < MSP_BUFFER_SIZE; i++) { - buffer.push8(FC.CONFIG.name.codePointAt(i)); - } + for (let i = 0; i < FC.CONFIG.name.length && i < MSP_BUFFER_SIZE; i++) { + buffer.push8(FC.CONFIG.name.codePointAt(i) & 0xff); + }
2358-2362: Clamp label bytes to 8‑bit for VTX powerlevelEnsure 8‑bit payload; avoid implicit truncation.
- for (let i = 0; i < FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.length; i++) { - buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i)); - } + for (let i = 0; i < FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.length; i++) { + buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i) & 0xff); + }
2369-2378: Clamp band name/letter bytes to 8‑bit; consider code‑point aware iteration for names
- Use 0xFF clamp for all single‑byte pushes.
- For band_name iteration, consider Array.from for surrogate safety (optional if strictly ASCII).
- for (let i = 0; i < FC.VTXTABLE_BAND.vtxtable_band_name.length; i++) { - buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i)); - } + for (let i = 0; i < FC.VTXTABLE_BAND.vtxtable_band_name.length; i++) { + buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i) & 0xff); + } - if (FC.VTXTABLE_BAND.vtxtable_band_letter != "") { - buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0)); + if (FC.VTXTABLE_BAND.vtxtable_band_letter != "") { + buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0) & 0xff); } else { - buffer.push8(" ".codePointAt(0)); + buffer.push8(" ".codePointAt(0) & 0xff); }Please confirm MSP text fields are defined as single‑byte (ASCII/Latin‑1) on the FC side. If UTF‑8 is desired, both write and read paths (getText) must be updated to agree on multi‑byte encoding.
src/js/msp.js (4)
61-61: Fixed, public TIMEOUT (1000ms): verify adequacy for slow links/commandsA flat 1s timeout may be tight for BLE or larger requests (e.g., dataflash). It triggers a single resend, then waits for callback cleanup. Consider per‑backend or per‑command overrides later if needed.
Would you like a follow‑up patch to allow an optional per‑call timeout override (e.g., send_message(code, data, cbSent, cbMsp, onErr, timeoutMs)) with default MSP.TIMEOUT?
358-360: CLI encoding: clamp bytes to 0xFF to avoid implicit truncationMake truncation explicit and consistent.
- const data = Array.from(str, (c) => c.codePointAt(0)); + const data = Array.from(str, (c) => c.codePointAt(0) & 0xff);
388-395: Duplicate request detection via CRC over encoded buffer: acceptable; monitor for collisionsCRC8 over the full encoded request (including header/length/payload) is a pragmatic key and should be low‑collision for our sizes. Keep an eye on any false positives.
Cache the computed CRC alongside the callback object to avoid recomputing for subsequent checks:
- Save obj.requestCrc = keyCrc
- Compare using i.requestCrc when available
Based on learnings
404-415: Timeout resend flow: one‑shot retry is fine; minor nit on clearing expired timerThe timer is cleared inside its own callback after resend, which is harmless but redundant post‑fire. No functional issue.
If desired, set a separate retry‑attempt flag instead of clearing the already‑fired timer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(7 hunks)src/js/msp/MSPHelper.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
🧬 Code graph analysis (2)
src/js/msp/MSPHelper.js (1)
src/js/fc.js (2)
i(264-264)FC(131-990)
src/js/msp.js (3)
src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/data_storage.js (1)
CONFIGURATOR(7-38)src/js/gui.js (1)
GUI(465-465)
🔇 Additional comments (6)
src/js/msp/MSPHelper.js (1)
1767-1769: Correct timer cleanupSwitching to clearTimeout for a timeout handle is correct.
src/js/msp.js (5)
7-13: LGTM: symbol derivation via codePointAt(0)Functionally identical to charCodeAt for single‑byte symbols; clearer intent.
289-292: LGTM: stricter listener de‑duplicationUsing === prevents accidental truthy matches.
319-322: LGTM: const buffer/viewMinor immutability improvement.
379-385: Connected/virtual guards: looks correctShort‑circuiting when not connected or in virtual mode prevents queue growth.
Please confirm serial.connected is a stable boolean on all backends (USB, WebBLE). If some backends expose a getter, we might need to call a method instead.
419-424: Only invoke callback_sent on full write: goodPrevents spurious sent callbacks on partial writes.
haslinghuis
force-pushed
the
update-msp
branch
from
e3ece6c to
d3e4dda
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/msp/MSPHelper.js (1)
1759-1781: Fire only one pending callback per response to avoid double-invocation across duplicatesCurrently every callback with the same code is invoked for a single response. This misroutes acks if multiple requests with the same code are in-flight with different payloads. Process a single matching callback per response.
- for (let i = dataHandler.callbacks.length - 1; i >= 0; i--) { + for (let i = dataHandler.callbacks.length - 1; i >= 0; i--) { // iterating in reverse because we use .splice which modifies array length if (dataHandler.callbacks[i]?.code === code) { // save callback reference const callback = dataHandler.callbacks[i].callback; const callbackOnError = dataHandler.callbacks[i].callbackOnError; // remove timeout clearTimeout(dataHandler.callbacks[i].timer); // remove object from array dataHandler.callbacks.splice(i, 1); if (!crcError || callbackOnError) { // fire callback if (callback) { callback({ command: code, data: data, length: data.byteLength, crcError: crcError }); } } else { console.warn(`code: ${code} - crc failed. No callback`); } + // Process only one matching callback per response + break; } }src/js/msp.js (1)
359-371: CLI message encoding isn’t Unicode-safe; bytes are truncated to 8-bitArray.from with codePointAt produces values >255 for non‑ASCII, but the buffer is Uint8, so high bits are dropped. If Unicode input is expected, UTF‑8 encode before writing and size by bytes.
- const data = Array.from(str, (c) => c.codePointAt(0)); - const dataLength = data ? data.length : 0; - const bufferSize = dataLength + 3; // 3 bytes for protocol overhead + const encoded = new TextEncoder().encode(str || ""); + const dataLength = encoded.length; + const bufferSize = dataLength + 3; // 3 bytes for protocol overhead const bufferOut = new ArrayBuffer(bufferSize); const bufView = new Uint8Array(bufferOut); bufView[0] = this.symbols.START_OF_TEXT; // STX - for (let ii = 0; ii < dataLength; ii++) { - bufView[1 + ii] = data[ii]; - } + for (let ii = 0; ii < dataLength; ii++) { + bufView[1 + ii] = encoded[ii]; + } bufView[bufferSize - 2] = this.symbols.LINE_FEED; // LF bufView[bufferSize - 1] = this.symbols.END_OF_TEXT; // ETX
🧹 Nitpick comments (4)
src/js/msp.js (4)
61-61: Fixed global TIMEOUT: verify adequacy across links (BLE, large payloads)A hardcoded 1000 ms may be short on slow/latent links (BLE, WebUSB + hubs) or jumbo responses. Consider making it configurable (e.g., per transport) or at least environment-tunable.
Would you like a small patch to read TIMEOUT from a user setting or scale by payload size/link type?
387-395: Dedup using CRC on the whole buffer: add a cached key to avoid recomputationComputing CRC8 for every queued callback on every send is avoidable. Cache the request CRC when creating the entry and compare against that.
- const keyCrc = this.crc8_dvb_s2_data(view, 0, view.length); - const requestExists = this.callbacks.some( - (i) => - i.code === code && - i.requestBuffer?.byteLength === bufferOut.byteLength && - this.crc8_dvb_s2_data(new Uint8Array(i.requestBuffer), 0, i.requestBuffer.byteLength) === keyCrc, - ); + const keyCrc = this.crc8_dvb_s2_data(view, 0, view.length); + const requestExists = this.callbacks.some((i) => i.code === code && i.requestCrc === keyCrc);And when creating the object (Line 396 block):
const obj = { code: code, requestBuffer: bufferOut, + requestCrc: keyCrc, callback: callback_msp, callbackOnError: doCallbackOnError, start: performance.now(), };
404-415: Avoid calling callback_sent twice (initial send and timeout resend)callback_sent is invoked once on the initial send and again on the timeout resend. Double-calling can confuse callers.
- obj.timer = setTimeout(() => { + obj.timer = setTimeout(() => { console.warn( `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`, ); - serial.send(bufferOut, (sendInfo) => { - if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) { - callback_sent(); - } - }); + // Re-send without invoking callback_sent again + serial.send(bufferOut); }, this.TIMEOUT);Please confirm no callers rely on callback_sent being invoked on retries.
Also applies to: 421-425
315-336: Optional: use symbol constants instead of magic numbers in V1 headerFor readability and consistency with symbols, consider:
- bufView[0] = 36; // $ - bufView[1] = 77; // M - bufView[2] = 60; // < + bufView[0] = this.symbols.BEGIN; + bufView[1] = this.symbols.PROTO_V1; + bufView[2] = this.symbols.TO_MWC;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(7 hunks)src/js/msp/MSPHelper.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T20:41:42.451Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Applied to files:
src/js/msp.js
🧬 Code graph analysis (2)
src/js/msp/MSPHelper.js (1)
src/js/fc.js (2)
i(264-264)FC(131-990)
src/js/msp.js (2)
src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/data_storage.js (1)
CONFIGURATOR(7-38)
🔇 Additional comments (4)
src/js/msp/MSPHelper.js (1)
1767-1770: Correct timer cleanupSwitching to clearTimeout() matches the setTimeout-based timers in MSP.send_message. Good catch.
src/js/msp.js (3)
7-12: Symbols via codePointAt: OKUsing codePointAt for single-char literals is fine and keeps intent clear.
290-293: Strict check when adding listeners: OKindexOf(...) === -1 prevents duplicates; good.
319-321: const bufferOut/bufView: OKMinor readability/immutability improvement.
| for (let i = 0; i < size; i++) { | ||
| buffer.push8(config.charCodeAt(i)); | ||
| buffer.push8(config.codePointAt(i)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
codePointAt + push8 truncates >255; not Unicode-safe and iterates by code units
Using codePointAt(i) and pushing a single byte will silently truncate non-ASCII code points and also steps through surrogate pairs incorrectly. If Unicode support is intended, encode to bytes (e.g., UTF‑8) and count bytes, not JS string length.
Proposed fix for setText:
- const size = Math.min(length, config.length);
- // length byte followed by the actual characters
- buffer.push8(size);
-
- for (let i = 0; i < size; i++) {
- buffer.push8(config.codePointAt(i));
- }
+ const encoded = new TextEncoder().encode(config || "");
+ const size = Math.min(length, encoded.length);
+ buffer.push8(size);
+ for (let i = 0; i < size; i++) {
+ buffer.push8(encoded[i]);
+ }📝 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.
| for (let i = 0; i < size; i++) { | |
| buffer.push8(config.charCodeAt(i)); | |
| buffer.push8(config.codePointAt(i)); | |
| } | |
| }; | |
| const encoded = new TextEncoder().encode(config || ""); | |
| const size = Math.min(length, encoded.length); | |
| buffer.push8(size); | |
| for (let i = 0; i < size; i++) { | |
| buffer.push8(encoded[i]); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 97 to 100, the loop uses
config.codePointAt(i) and push8 which truncates values >255 and mis-iterates
surrogate pairs; instead encode the string to UTF-8 (e.g., via TextEncoder), set
size to the encoded byte length, and iterate the resulting Uint8Array pushing
each byte with push8 so multi-byte Unicode characters are preserved and bytes
counted correctly.
| case MSPCodes.MSP_SET_NAME: | ||
| const MSP_BUFFER_SIZE = 64; | ||
| for (let i = 0; i < FC.CONFIG.name.length && i < MSP_BUFFER_SIZE; i++) { | ||
| buffer.push8(FC.CONFIG.name.charCodeAt(i)); | ||
| buffer.push8(FC.CONFIG.name.codePointAt(i)); | ||
| } | ||
| break; |
There was a problem hiding this comment.
SET_NAME suffers from the same truncation issue
Same concern as setText: codePointAt + push8 truncates non‑ASCII and walks code units. Prefer UTF‑8 encoding and byte-length limiting.
- const MSP_BUFFER_SIZE = 64;
- for (let i = 0; i < FC.CONFIG.name.length && i < MSP_BUFFER_SIZE; i++) {
- buffer.push8(FC.CONFIG.name.codePointAt(i));
- }
+ const MSP_BUFFER_SIZE = 64;
+ const encoded = new TextEncoder().encode(FC.CONFIG.name || "");
+ const size = Math.min(MSP_BUFFER_SIZE, encoded.length);
+ for (let i = 0; i < size; i++) {
+ buffer.push8(encoded[i]);
+ }📝 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.
| case MSPCodes.MSP_SET_NAME: | |
| const MSP_BUFFER_SIZE = 64; | |
| for (let i = 0; i < FC.CONFIG.name.length && i < MSP_BUFFER_SIZE; i++) { | |
| buffer.push8(FC.CONFIG.name.charCodeAt(i)); | |
| buffer.push8(FC.CONFIG.name.codePointAt(i)); | |
| } | |
| break; | |
| case MSPCodes.MSP_SET_NAME: | |
| const MSP_BUFFER_SIZE = 64; | |
| const encoded = new TextEncoder().encode(FC.CONFIG.name || ""); | |
| const size = Math.min(MSP_BUFFER_SIZE, encoded.length); | |
| for (let i = 0; i < size; i++) { | |
| buffer.push8(encoded[i]); | |
| } | |
| break; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 2252-2252: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2251 to 2256 the SET_NAME handling uses
codePointAt + push8 which truncates multi‑byte UTF‑8 characters and walks UTF‑16
code units; replace this with UTF‑8 encoding and a byte-length limit: encode
FC.CONFIG.name to a UTF‑8 byte array (TextEncoder or Buffer.from), copy up to 64
bytes into the buffer by pushing each byte, and avoid splitting a multi‑byte
sequence by truncating on the encoded byte array boundary (i.e., simply limit
the encoded bytes to 64 before pushing).
| for (let i = 0; i < FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.length; i++) { | ||
| buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.charCodeAt(i)); | ||
| buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i)); | ||
| } | ||
|
|
There was a problem hiding this comment.
VTX power level label: encode bytes, don’t push code points
- const powerLabelLength = data.readU8();
- FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label = "";
- for (let i = 0; i < powerLabelLength; i++) {
- FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label += String.fromCharCode(data.readU8());
- }
+ const powerLabelLength = data.readU8();
+ FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label = "";
+ for (let i = 0; i < powerLabelLength; i++) {
+ FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label += String.fromCharCode(data.readU8());
+ }And when writing (below at Lines 2361-2362), use UTF‑8 bytes:
- for (let i = 0; i < FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.length; i++) {
- buffer.push8(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label.codePointAt(i));
- }
+ {
+ const encoded = new TextEncoder().encode(FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label || "");
+ for (let i = 0; i < encoded.length; i++) {
+ buffer.push8(encoded[i]);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2360 to 2363, the loop is pushing
Unicode code points instead of the UTF‑8 encoded bytes for
FC.VTXTABLE_POWERLEVEL.vtxtable_powerlevel_label; replace the per-codePoint loop
with obtaining the UTF‑8 byte sequence for the label (e.g. via
Buffer.from(label, 'utf8') or TextEncoder) and then push each byte with
buffer.push8 so the bytes written are proper UTF‑8 encoded bytes rather than raw
code point values.
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.length); | ||
| for (let i = 0; i < FC.VTXTABLE_BAND.vtxtable_band_name.length; i++) { | ||
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.charCodeAt(i)); | ||
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i)); | ||
| } | ||
|
|
There was a problem hiding this comment.
VTX band name: encode to bytes (UTF‑8) before pushing
- buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.length);
- for (let i = 0; i < FC.VTXTABLE_BAND.vtxtable_band_name.length; i++) {
- buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i));
- }
+ {
+ const encoded = new TextEncoder().encode(FC.VTXTABLE_BAND.vtxtable_band_name || "");
+ buffer.push8(encoded.length);
+ for (let i = 0; i < encoded.length; i++) {
+ buffer.push8(encoded[i]);
+ }
+ }📝 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.
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.length); | |
| for (let i = 0; i < FC.VTXTABLE_BAND.vtxtable_band_name.length; i++) { | |
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.charCodeAt(i)); | |
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_name.codePointAt(i)); | |
| } | |
| { | |
| const encoded = new TextEncoder().encode(FC.VTXTABLE_BAND.vtxtable_band_name || ""); | |
| buffer.push8(encoded.length); | |
| for (let i = 0; i < encoded.length; i++) { | |
| buffer.push8(encoded[i]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2369 to 2373, the code uses
string.codePointAt and string.length (UTF-16 code unit based) when writing the
VTX band name, which breaks non‑ASCII characters; instead, encode
FC.VTXTABLE_BAND.vtxtable_band_name to UTF‑8 bytes (e.g. via TextEncoder in
browser/node or Buffer.from(...,'utf8') in Node), write the byte length (not the
JS string length) with buffer.push8, then iterate over the resulting byte array
and push each byte with buffer.push8.
| if (FC.VTXTABLE_BAND.vtxtable_band_letter != "") { | ||
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.charCodeAt(0)); | ||
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0)); | ||
| } else { | ||
| buffer.push8(" ".charCodeAt(0)); | ||
| buffer.push8(" ".codePointAt(0)); | ||
| } |
There was a problem hiding this comment.
VTX band letter should be ASCII; avoid codePointAt and multi-byte ambiguity
The band letter is a single ASCII char by protocol. Use charCodeAt with safe fallback.
- if (FC.VTXTABLE_BAND.vtxtable_band_letter != "") {
- buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0));
- } else {
- buffer.push8(" ".codePointAt(0));
- }
+ const letter = (FC.VTXTABLE_BAND.vtxtable_band_letter || " ").charAt(0);
+ buffer.push8(letter.charCodeAt(0) & 0xff);📝 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.
| if (FC.VTXTABLE_BAND.vtxtable_band_letter != "") { | |
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.charCodeAt(0)); | |
| buffer.push8(FC.VTXTABLE_BAND.vtxtable_band_letter.codePointAt(0)); | |
| } else { | |
| buffer.push8(" ".charCodeAt(0)); | |
| buffer.push8(" ".codePointAt(0)); | |
| } | |
| const letter = (FC.VTXTABLE_BAND.vtxtable_band_letter || " ").charAt(0); | |
| buffer.push8(letter.charCodeAt(0) & 0xff); |
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2374-2378, the current code uses
codePointAt which can return multi-byte code points; replace that usage with
charCodeAt(0) to ensure an ASCII byte is written, and if charCodeAt yields
NaN/undefined (empty string) push the ASCII space byte (0x20) as a safe
fallback; keep using buffer.push8 to write the final byte.
haslinghuis
force-pushed
the
update-msp
branch
from
d3e4dda to
3135b28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/js/msp.js (3)
61-61: Make timeout intent explicit and configurable.Rename to TIMEOUT_MS and consider sourcing from a central config (or environment) to tune per transport (USB vs BLE).
- TIMEOUT: 1000, + TIMEOUT_MS: 1000, // request timeout in millisecondsYou’d also need to replace
.TIMEOUTusages accordingly.
319-336: LGTM on consts; avoid magic numbers for header bytes.Use symbolic constants to improve readability and avoid drift across encoders.
- bufView[0] = 36; // $ - bufView[1] = 77; // M - bufView[2] = 60; // < + bufView[0] = this.symbols.BEGIN; // $ + bufView[1] = this.symbols.PROTO_V1; // M + bufView[2] = this.symbols.TO_MWC; // <Optionally apply the same to encode_message_v2.
387-395: Good duplicate-detection; cache CRC per request to avoid O(n) recompute.Store CRC alongside each queued request and compare that, instead of recalculating for all items each call.
- const view = new Uint8Array(bufferOut); - const keyCrc = this.crc8_dvb_s2_data(view, 0, view.length); - const requestExists = this.callbacks.some( - (i) => - i.code === code && - i.requestBuffer?.byteLength === bufferOut.byteLength && - this.crc8_dvb_s2_data(new Uint8Array(i.requestBuffer), 0, i.requestBuffer.byteLength) === keyCrc, - ); + const view = new Uint8Array(bufferOut); + const requestCrc = this.crc8_dvb_s2_data(view, 0, view.length); + const requestExists = this.callbacks.some( + (i) => + i.code === code && + i.requestBuffer?.byteLength === bufferOut.byteLength && + i.requestCrc === requestCrc, + ); ... - requestBuffer: bufferOut, + requestBuffer: bufferOut, + requestCrc,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(5 hunks)src/js/msp/MSPHelper.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/msp/MSPHelper.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T20:41:42.451Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Applied to files:
src/js/msp.js
🧬 Code graph analysis (1)
src/js/msp.js (3)
src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/data_storage.js (1)
CONFIGURATOR(7-38)src/js/gui.js (1)
GUI(465-465)
🔇 Additional comments (2)
src/js/msp.js (2)
290-292: LGTM: strict membership check.Using strict equality with indexOf improves correctness.
421-425: LGTM: send callback only on full frame write.Guarding callback_sent by bytesSent === buffer length is correct.
src/js/msp.js
Outdated
| send_message(code, data, callback_sent, callback_msp, doCallbackOnError) { | ||
| const connected = serial.connected; | ||
|
|
||
| if (code === undefined || !connected || CONFIGURATOR.virtualMode) { | ||
| if (code === undefined || !serial.connected || CONFIGURATOR.virtualMode) { | ||
| if (callback_msp) { | ||
| callback_msp(); | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Dead/legacy parameter: doCallbackOnError.
This parameter was previously removed as unused; it’s reintroduced here and still not consumed elsewhere (only stored on obj). Drop it to reduce API surface and confusion.
Based on learnings
- send_message(code, data, callback_sent, callback_msp, doCallbackOnError) {
+ send_message(code, data, callback_sent, callback_msp) {
...
- callback: callback_msp,
- callbackOnError: doCallbackOnError,
+ callback: callback_msp,Please confirm there are no call sites passing the 5th arg.
🏁 Script executed:
#!/bin/bash
# Find call sites that still pass a 5th argument to send_message
rg -n -C2 --type=js '\bsend_message\s*\(' | gawk '{
line=$0
match(line, /\bsend_message\s*\((.*)\)/, m)
if (m[1] != "") {
args=m[1]
# naive split by comma (good enough to surface suspects)
n=gsub(/,/, ",", args)
if (n >= 4) print FILENAME ":" $0
}
}'Length of output: 108
🏁 Script executed:
#!/bin/bash
# Search for calls to send_message with five arguments
ast-grep --pattern $'send_message($_, $_, $_, $_, $_)' --glob '*.js'Length of output: 187
🏁 Script executed:
#!/bin/bash
# Search for calls to send_message with five arguments across JS files
ast-grep --pattern $'send_message($_, $_, $_, $_, $_)' --globs '*.js' .Length of output: 0
Remove unused doCallbackOnError parameter from send_message
The parameter is never passed or used—drop it to simplify the API in src/js/msp.js:
- send_message(code, data, callback_sent, callback_msp, doCallbackOnError) {
+ send_message(code, data, callback_sent, callback_msp) {
…
- callbackOnError: doCallbackOnError,📝 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.
| send_message(code, data, callback_sent, callback_msp, doCallbackOnError) { | |
| const connected = serial.connected; | |
| if (code === undefined || !connected || CONFIGURATOR.virtualMode) { | |
| if (code === undefined || !serial.connected || CONFIGURATOR.virtualMode) { | |
| if (callback_msp) { | |
| callback_msp(); | |
| } | |
| return false; | |
| } | |
| send_message(code, data, callback_sent, callback_msp) { | |
| if (code === undefined || !serial.connected || CONFIGURATOR.virtualMode) { | |
| if (callback_msp) { | |
| callback_msp(); | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/js/msp.js around lines 378 to 385, the send_message function has an
unused parameter doCallbackOnError; remove this parameter from the function
signature and any internal references (none present currently) and update any
call sites that pass a fourth argument to send_message to stop supplying it so
the API stays consistent; run a quick grep across the repo for send_message( to
update callers and run tests/lint to ensure no remaining references.
| if (!requestExists) { | ||
| obj.timer = setTimeout(() => { | ||
| console.warn( | ||
| `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} TIMEOUT: ${ | ||
| this.timeout | ||
| } QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`, | ||
| `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`, | ||
| ); | ||
| serial.send(bufferOut, (_sendInfo) => { | ||
| obj.stop = performance.now(); | ||
| const executionTime = Math.round(obj.stop - obj.start); | ||
| this.timeout = Math.max(this.MIN_TIMEOUT, Math.min(executionTime, this.MAX_TIMEOUT)); | ||
| serial.send(bufferOut, (sendInfo) => { | ||
| if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) { | ||
| callback_sent(); | ||
| } | ||
| }); | ||
| }, this.timeout); | ||
| }, this.TIMEOUT); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Timeout currently triggers an unconditional resend; high risk of duplicate writes and log noise.
- The timer isn’t cleared on successful response (no visible code does this), so it will fire and resend even after a response.
- Blind retries can cause non‑idempotent MSP “set” commands to be applied multiple times.
Prefer failing the request on timeout (and clearing it) instead of resending here. If you must retry, gate it to read‑only (no‑payload) requests and ensure timers are canceled on response.
- if (!requestExists) {
- obj.timer = setTimeout(() => {
- console.warn(
- `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`,
- );
- serial.send(bufferOut, (sendInfo) => {
- if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) {
- callback_sent();
- }
- });
- }, this.TIMEOUT);
- }
+ if (!requestExists) {
+ obj.timer = setTimeout(() => {
+ console.warn(
+ `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`,
+ );
+ // Drop the request and signal an error
+ const idx = this.callbacks.indexOf(obj);
+ if (idx !== -1) this.callbacks.splice(idx, 1);
+ if (obj.callback) obj.callback(undefined);
+ }, this.TIMEOUT_MS ?? this.TIMEOUT);
+ }Additionally, ensure timers are cleared when a matching response arrives (see suggestion to resolve/purge callbacks on receive). I can draft that wiring if desired.
🏁 Script executed:
#!/bin/bash
# Verify any code clears per-request timers on response
rg -n -C3 --type=js 'clearTimeout\(|callbacks\.(splice|shift|filter)\('Length of output: 10870
🏁 Script executed:
#!/bin/bash
rg -n clearTimeout src/js/msp.jsLength of output: 45
Eliminate unguarded retry on timeout and clear per-request timers on response
- In src/js/msp.js (≈404–415), remove the
serial.send(…)retry in the timeout handler. Instead do:clearTimeout(obj.timer); const idx = this.callbacks.indexOf(obj); if (idx !== -1) this.callbacks.splice(idx, 1); obj.callback(undefined);
- In your incoming-response logic (where you match an MSP reply to its callback), call
clearTimeout(obj.timer)and remove that callback fromthis.callbacksbefore invoking it to prevent any stale timers firing.
🤖 Prompt for AI Agents
In src/js/msp.js around lines 404–415, the timeout handler currently retries by
calling serial.send; remove that retry and instead clearTimeout(obj.timer), find
and splice out obj from this.callbacks (if present), and invoke
obj.callback(undefined) to notify the caller of the timeout; additionally,
update the incoming-response matching logic so that when a response matches a
callback you first clearTimeout(obj.timer) and remove that callback from
this.callbacks before calling obj.callback(...) to avoid stale timers firing
after fulfillment.
|
|
Preview URL: https://pr4632.betaflight-app.pages.dev |
|
Superseded by 10d6584 |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor