Improve msp send #4510
Improve msp send #4510
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 10 minutes and 39 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)
WalkthroughReplaces adaptive MSP timeout/flags with a fixed TIMEOUT and simplifies send_message; removes jumbo-frame tracking; adds dynamic import bootstrap for MSP debug tools and many new debug modules (README, barrel, dashboard, queue monitor, stress tester, test runner); treats test environments as compatible and bumps one test API version. Changes
Sequence Diagram(s)MSP send_message with fixed TIMEOUTsequenceDiagram
participant Caller
participant MSP
participant Serial
participant Timer
Caller->>MSP: send_message(code, data, callback_sent, callback_msp)
MSP->>MSP: if (!serial.connected) -> call callback_msp and return
MSP->>MSP: check duplicate via callbacks.some(...)
alt send permitted
MSP->>MSP: create request { code, start: performance.now(), ... }
MSP->>Timer: setTimeout(timerFn, TIMEOUT)
MSP->>Serial: serial.send(bufferOut, sendCallback)
Serial-->>MSP: sendCallback(bytesSent)
MSP->>MSP: compute executionTime, set obj.stop
MSP->>Caller: call callback_sent only if bytesSent == bufferOut.byteLength
Timer-->>MSP: timerFn fires -> clear timer, log if executionTime > 5000ms, handle timeout (no adaptive scaling)
else duplicate or disconnected
MSP->>Caller: skip send / call callback_msp
end
Dynamic loading of MSP Debug Tools (high-level)sequenceDiagram
participant App as Main App
participant Import as Dynamic Import
participant Tools as MSP Debug Tools
participant Monitor as mspQueueMonitor
participant Dashboard as mspDebugDashboard
participant Stress as mspStressTest
App->>Import: import('./msp/debug/msp_debug_tools.js')
Import-->>Tools: load modules, print console banner
alt dev host (localhost/127.0.0.1)
Tools->>Monitor: dynamic import and startMonitoring(2000)
Tools->>App: console guidance messages
end
Tools->>Dashboard: initialize UI on demand
Dashboard->>Monitor: subscribe for live updates
Dashboard->>Stress: trigger stress tests
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/js/msp.js (1)
402-413:⚠️ Potential issue
_validateSendMessageignoresdoCallbackOnErrorflagError-side invocation of
callback_mspis unconditional. That breaks the original contract implied by thedoCallbackOnErrorparameter and can trigger “double callbacks” or resolve aPromisethat expected an actual response.-_validateSendMessage(code, callback_msp) { +_validateSendMessage(code, callback_msp, doCallbackOnError = false) { … - if (callback_msp) { + if (doCallbackOnError && callback_msp) { callback_msp(); }You’ll have to forward the flag from
send_message.
🧹 Nitpick comments (2)
src/js/msp.js (2)
423-431: Unused field –callbackOnErroris never referenced
_createRequestObjectstorescallbackOnError, but no subsequent code reads it after the refactor. Either remove the property or reinstate the error-handling path.Eliminating dead fields simplifies debugging and reduces cognitive load.
457-467: Aggressive timeout decay may under-shootMIN_TIMEOUT
this.timeout--happens on every send, regardless of frequency, and can push the value belowMIN_TIMEOUTwhentimeouthas already been clamped lower elsewhere.-if (this.timeout > this.MIN_TIMEOUT) { - this.timeout--; -} +this.timeout = Math.max(this.MIN_TIMEOUT, this.timeout - 1);A simple guard prevents negative or sub-minimum timeouts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/js/msp.js (2)
src/js/serial.js (2)
serial(364-364)serial(364-364)src/js/gui.js (1)
GUI(484-484)
🔇 Additional comments (1)
src/js/msp.js (1)
415-418: Duplicate detection is too coarse
_isDuplicateRequestonly checks equality ofcode; it ignores the payload (data).
Two requests with the same MSP code but different parameters will be treated as duplicates and the second payload may never be transmitted.Consider comparing a
(code, dataHash)tuple or always sending whendataLength > 0(even if duplicate).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/js/msp.js (3)
390-398: Unused allocation for duplicates
requestObjis instantiated even when the call is considered a duplicate and then discarded. This happens on every duplicate invocation and can become a noticeable GC hot-spot under heavy traffic.Move the allocation inside the
if (!isDuplicateRequest)block so it is only created when needed.
445-456: Timer remains referenced after giving up – clear it to aid GCWhen
MAX_RETRIESis exceeded the function bails out but does notclearTimeout(requestObj.timer);.
While the timer has already fired, keeping the handle prevents some JS engines from reclaiming it promptly.if (requestObj.attempts >= this.MAX_RETRIES) { console.error(`MSP: Request ${requestObj.code} exceeded max retries (${this.MAX_RETRIES}), giving up`); - // Remove from callbacks to prevent memory leak + clearTimeout(requestObj.timer); // 💡 explicit cleanup + // Remove from callbacks to prevent memory leak this._removeRequestFromCallbacks(requestObj);
411-413: Adaptive back-off step might starve high-latency links
this.timeoutis decreased by a fixed 5 ms on every successful send. On slow links (e.g. Bluetooth) this can quickly shrink below the real RTT, leading to unnecessary retries. Consider decaying by a percentage (e.g.timeout *= 0.9) or pinning a lower bound based on recent RTT measurements instead of a constant 5 ms.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/js/msp.js (3)
src/js/serial.js (2)
serial(364-364)serial(364-364)src/js/data_storage.js (1)
CONFIGURATOR(7-40)src/js/gui.js (1)
GUI(484-484)
haslinghuis
force-pushed
the
improve-msp
branch
3 times, most recently
from
eb77f13 to
4fcad77
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/msp.js (1)
399-420:⚠️ Potential issueDuplicate requests silently drop callbacks
When
isDuplicateRequestistrue, the newcallback_mspis not stored anywhere, so callers issuing concurrent identical requests will never be notified.
This regresses previous behaviour and breaks APIs that rely on per-call completion.Quick remedy: attach additional callbacks to the existing request object instead of discarding them.
-const isDuplicateRequest = this.callbacks.some((instance) => instance.requestKey === requestKey); +const existingReq = this.callbacks.find((i) => i.requestKey === requestKey); +const isDuplicateRequest = Boolean(existingReq); … -if (!isDuplicateRequest) { +if (!isDuplicateRequest) { this._setupTimeout(requestObj, bufferOut); this.callbacks.push(requestObj); +} else if (callback_msp) { + // fan-out: queue extra callback(s) on the first request + (existingReq.extraCallbacks ??= []).push(callback_msp); }And, when the response arrives, invoke
extraCallbacksas well.
🧹 Nitpick comments (4)
src/js/msp.js (4)
376-389: Hash function may collide & yield negative keys
_createRequestKeyuses a simple 32-bit rolling hash that can:
- Produce the same hash for different payloads (higher collision risk on large traffic).
- Return negative numbers, generating keys like
123:-842150451which are harder to eyeball/debug.Consider switching to a safer deterministic scheme, e.g. CRC-32 or base64 of the first 4 bytes plus
length, to lower collision odds and keep keys human-readable.
421-428: Inefficient duplicate scan — O(N) each call
this.callbacks.some()does a linear search every timesend_messageis invoked.
WithMAX_QUEUE_SIZEalready tracked, switching to aMap<requestKey, requestObj>would give O(1) look-ups and simplify removal logic.
433-438: Timer handle lost if_setupTimeoutis called twice
_setupTimeoutoverwritesrequestObj.timerwithout clearing a possibly existing one.
Although current flow prevents re-entrance, guarding against future changes is cheap:+if (requestObj.timer) { + clearTimeout(requestObj.timer); +} requestObj.timer = setTimeout(() => { this._handleTimeout(requestObj, bufferOut); }, this.TIMEOUT);
439-472: Retry path never resetsstart— metrics become meaningless
requestObj.startis set once insend_messagebut never updated on retries, so any later duration calculations accumulate total wall-time, not per-attempt latency.Either reset
startinside_handleTimeoutbefore re-arming the timer or drop it altogether if no longer used.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/js/msp.js (2)
src/js/serial.js (2)
serial(364-364)serial(364-364)src/js/data_storage.js (1)
CONFIGURATOR(7-40)
🔇 Additional comments (1)
src/js/msp.js (1)
69-73:✅ Verification successful
Confirm removal of legacy
this.timeoutreferencesYou have introduced a fixed
TIMEOUTconstant but removed the old adaptivethis.timeout.
Please grep the rest of the file / project for stalethis.timeoutusages to avoid undefined-property bugs at runtime.
🏁 Script executed:
#!/bin/bash # Find any remaining references to the old timeout field rg --line-number $'this\\.timeout'Length of output: 665
No stale
this.timeoutreferences foundA full‐repo search for
this.timeoutonly returns occurrences ofthis.timeout_arrayinsrc/js/gui.js, not the legacythis.timeoutproperty. There are no remaining direct references to the removed adaptive timeout field—this concern can be closed.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/msp.js (1)
482-491:_removeRequestFromCallbacksleaves Map/key tracking unresolvedIf you adopt the
Mapoptimisation above, ensure this method also deletes therequestKeyentry to keep the structures in sync.
🧹 Nitpick comments (4)
src/js/msp.js (4)
69-73: Expose retry parameters instead of hard-coding them
MAX_RETRIES,MAX_QUEUE_SIZE, andTIMEOUTare baked into the module, making it impossible to tune behaviour from the outside (e.g. per-board, per-environment, unit-tests). Consider reading them fromCONFIGURATOR, a constructor argument, or at least exporting setters.
399-420: Duplicate check performsO(n)scans on every call
this.callbacks.some(...)linearly scans the queue. Under heavy traffic (e.g. telemetry floods) the cost grows quadratically.Two easy wins:
- Keep a
Map<requestKey, requestObj>to getO(1)look-ups and removals.- Build
bufferOutafter the duplicate check to avoid unnecessary encoding work when the request is dropped.
433-438: Timer double-cleared – harmless but needless
_handleTimeoutcallsclearTimeout(requestObj.timer)(l.464) and_removeRequestFromCallbacksdoes the same. The second call is redundant and can be omitted to save a micro-op.
439-480: Constant timeout may hammer dead linksRetries fire every fixed
TIMEOUTms. On an unplugged device this means 10 exact re-transmits per second, wasting bandwidth and log space.
An exponential back-off (e.g.TIMEOUT * 2 ** attempts) curbs network noise and still converges quickly when the link is restored.
🛑 Comments failed to post (1)
src/js/msp.js (1)
376-389: 🛠️ Refactor suggestion
_createRequestKeycollision risk & signed-shift bugThe DJB-style hash on a
Uint8Arraywill inevitably collide for different payloads and, because& 0xffffffffyields a signed 32-bit value, the string key can even become negative.
A wrong “duplicate” classification silently drops tracking / retries.- let hash = 0; - for (const byte of data) { - hash = ((hash << 5) - hash + byte) & 0xffffffff; - } - return `${code}:${hash}`; + // Use a stable unsigned hash (FNV-1a) to minimise collisions + let hash = 2166136261 >>> 0; + for (const byte of data) { + hash ^= byte; + hash = Math.imul(hash, 16777619) >>> 0; + } + return `${code}:${hash.toString(16)}`;📝 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.// Helper function to create a unique key for request identification _createRequestKey(code, data) { if (!data || data.length === 0) { return `${code}:empty`; } // Use a stable unsigned hash (FNV-1a) to minimise collisions let hash = 2166136261 >>> 0; for (const byte of data) { hash ^= byte; hash = Math.imul(hash, 16777619) >>> 0; } return `${code}:${hash.toString(16)}`; },🤖 Prompt for AI Agents
In src/js/msp.js around lines 376 to 389, the _createRequestKey function uses a DJB-style hash that can produce collisions and negative values due to signed 32-bit shifts. To fix this, replace the hash calculation with a more robust hashing method that avoids collisions and ensures the hash is always unsigned and positive. For example, use a standard hashing library or implement a non-signed hash function that produces a unique, non-negative key string for the given data.
src/js/msp.js
Outdated
| } | ||
| // Create unique key combining code and data | ||
| const requestKey = this._createRequestKey(code, data); | ||
| const isDuplicateRequest = this.callbacks.some((instance) => instance.requestKey === requestKey); |
There was a problem hiding this comment.
why not return here if a dupe is detected ? saves your the conditions below. looks like the code is not following "one return per function" convention anyways.
There was a problem hiding this comment.
and, why not "rate-limit" the events from sliders at the source ? i bet there are different event types that need different handling anyways.... like those motor sliders, where probably first and last event is very important, but all events in between can be lost without any noticeable effect for the user, but then there are events that cannot be lost, like transmission of cli stuff ?
src/js/msp.js
Outdated
| this._setupTimeout(requestObj, bufferOut); | ||
| this.callbacks.push(requestObj); | ||
| } | ||
|
|
||
| this.callbacks.push(obj); | ||
|
|
||
| // always send messages with data payload (even when there is a message already in the queue) | ||
| if (data || !requestExists) { | ||
| if (this.timeout > this.MIN_TIMEOUT) { | ||
| this.timeout--; | ||
| } | ||
|
|
||
| // Send message if it has data or is a new request | ||
| if (data || !isDuplicateRequest) { | ||
| serial.send(bufferOut, (sendInfo) => { |
There was a problem hiding this comment.
(note the line range if this comment)
i would probably start the timeout and call send only if the queue is empty, or if the previous "send" succeeded?
otherwise the buffers of the serial protocols are immediately filled and can overflow
src/js/msp.js
Outdated
| ); | ||
|
|
||
| // Check if max retries exceeded OR queue is too large | ||
| if (requestObj.attempts >= this.MAX_RETRIES || this.callbacks.length > this.MAX_QUEUE_SIZE) { |
There was a problem hiding this comment.
i would perform the queue size check prior to insertion of the objects, in the send_message function
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/js/msp.js (4)
69-73: Freeze or namespace retry-config constants to prevent accidental mutationDefining
MAX_RETRIES,MAX_QUEUE_SIZEandTIMEOUTdirectly on the mutableMSPobject means any consumer can overwrite them at runtime:MSP.TIMEOUT = 1; // 💥If you really want them tweakable, expose an explicit setter.
Otherwise, lock them down:- MAX_RETRIES: 10, - MAX_QUEUE_SIZE: 50, - TIMEOUT: 1000, + MAX_RETRIES: 10, + MAX_QUEUE_SIZE: 50, + TIMEOUT: 1000, } + +Object.freeze(MSP); // or freeze only MSP.retryConfig sub-object
376-389: Hashing scheme is fragile & collision-prone
_createRequestKeybuilds a 32-bit additive hash; for large or similar payloads collisions are easy, producing false “duplicates”.
Additionallyhashmay end up negative, so keys like42:-123and42:327375look different but are produced from different sessions of the same data length.Consider:
- Use a stable digest (e.g. DJB2, MurmurHash) or
crypto.subtle.digest('SHA-1', data)when available.- Fall back to
btoa(String.fromCharCode(...data))for short payloads.- At minimum,
>>> 0to keep the value unsigned.
421-428: Guardcallback_sentwith a functional check
promise()passesfalse, socallback_sent()may be a boolean.
A trivial typeof guard avoids accidentalfalse is not a functionmistakes if the signature changes upstream.-if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) { +if (sendInfo.bytesSent === bufferOut.byteLength && typeof callback_sent === "function") { callback_sent(); }
479-489: O(N²) removal can hurt with large queues
indexOf+spliceis fine for the 50-item cap, but if the limit is raised later it becomes quadratic.
Storing callbacks in aMap<requestKey, requestObj>would give O(1) insert/remove/lookup, simplify duplicate handling, and eliminate linear scans elsewhere.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(2 hunks)
🔇 Additional comments (2)
src/js/msp.js (2)
433-438: Timeout set-up looks good
_setupTimeoutis concise and centralises timer arming – nice.
439-477: Retry handler: no back-off & relies onsendInfo.bytesSent
- A fixed 1 s retry interval can hammer sluggish links. Consider exponential back-off or
TIMEOUT * (1 + attempts)to avoid congestion.- If the serial driver does not populate
bytesSent,sendInfo.bytesSent === bufferOut.byteLengthevaluatesfalse⇒ the request aborts immediately. Validate field presence or fall back to truthiness.- if (sendInfo.bytesSent === bufferOut.byteLength) { + if (!sendInfo || sendInfo.bytesSent === undefined || sendInfo.bytesSent === bufferOut.byteLength) {
- You re-assign
requestObj.timerbut never clear it inside_handleTimeout.
Although the fired timer auto-invalidates, explicitlyclearTimeout(requestObj.timer)before re-arming removes any doubt.
|
checking the coderrabbit "nitpicks"? i'm finding them useful outside of this project. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/js/msp.js (2)
396-416: Log uses stale propertythis.timeoutThe warning logs
this.timeout, which no longer exists. Usethis.TIMEOUT.- `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} TIMEOUT: ${this.TIMEOUT} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`,
60-60: Consider configurable TIMEOUTA single 1000 ms works for consistency, but exposing it via settings/env (with a sane default) aids testing across devices/links without code changes.
Do you want a small patch to read an override from local storage or a query flag for the debug tools?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(4 hunks)src/js/msp/debug/MSP_DEBUG_README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/msp/debug/MSP_DEBUG_README.md
🧰 Additional context used
🧬 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 (3)
src/js/msp.js (3)
423-424: Good: only signalcallback_senton full sendGuarding on
bytesSent === bufferOut.byteLengthavoids false positives.
288-290: Strict equality is correct hereNice micro-fix to avoid accidental truthiness traps.
67-67: Comment clarity LGTMNaming the CLI buffer intent helps.
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 (2)
test/js/msp/MSPHelper.test.js (1)
14-37: Test contradicts new “no callback on CRC error” behavior
process_datanow suppresses callbacks on CRC failure, but this test still expects the callback to fire. Either update the test (if the new behavior is intended) or revert the MSPHelper change. Recommended here: align the test to expect no callback.Apply this diff if keeping the new behavior:
- expect(callbackCalled).toEqual(true); + expect(callbackCalled).toEqual(false);src/js/msp/MSPHelper.js (1)
1757-1776: Suppressing callbacks on CRC errors may regress retries and toolingCallbacks are now skipped when
crcError === true. Several callsites (e.g., dataflash reads) previously used the callback to trigger retries and error handling; suppressing it can stall flows until timeout and breaks the existing “error surfaced via callback” contract.Prefer always invoking the callback and letting the consumer decide:
- if (!crcError) { - // fire callback - if (callback) { - callback({ command: code, data: data, length: data.byteLength, crcError: crcError }); - } - } else { - console.warn(`code: ${code} - crc failed. No callback`); - } + // Always notify; consumer handles crcError and retries. + if (callback) { + const payload = crcError + ? { command: code, data: new DataView(new ArrayBuffer(0)), length: 0, crcError: true } + : { command: code, data, length: data.byteLength, crcError: false }; + callback(payload); + }
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2709-2747: Minor: redundant bit_set usage
mask |= bit_set(mask, idx)is redundant;bit_setlikely returnsmask | (1<<idx). Use assignment for clarity.- mask |= bit_set(mask, bitIndex + 12); + mask = bit_set(mask, bitIndex + 12);(Apply similarly to other bit_set sites in this block.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/msp.js(4 hunks)src/js/msp/MSPHelper.js(1 hunks)test/js/msp/MSPHelper.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/msp.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/js/msp/MSPHelper.test.js (1)
src/js/fc.js (1)
FC(131-990)
🔇 Additional comments (5)
test/js/msp/MSPHelper.test.js (2)
6-6: Bump to API_VERSION_1_47 looks goodImport update is consistent with the PR goal to test against latest firmware.
81-81: Setting FC.CONFIG.apiVersion to 1.47 is correctThis ensures the MSP_BOARD_INFO expectations match the newer payload layout.
src/js/msp/MSPHelper.js (3)
11-11: API_VERSION_1_47 import is correctNeeded for new 1.47 feature gates added below.
242-244: Verified: FC schema includes the 1.47 fields — no changes requirednumberOfRateProfiles, RC_TUNING.throttle_HOVER, SENSOR_CONFIG.opticalflow_hardware (and SENSOR_CONFIG_ACTIVE via spread), and MCU_INFO {id,name} are defined in src/js/fc.js (numberOfRateProfiles L35; throttle_HOVER L299; opticalflow_hardware L596; MCU_INFO L601); MSPHelper reads/writes them (src/js/msp/MSPHelper.js L242–244, L472–474, L1230–1249, L1854–1857, L2237–2240); VirtualFC provides defaults (src/js/VirtualFC.js L269–272).
2455-2509: dataflashRead may no longer retry on CRC errorThis path relies on the response callback to detect
crcErrorand callonDataCallback(address, null)to force a retry. With callbacks suppressed on CRC error, this logic won’t run; reads may hang until timeout.Please confirm
MSP.send_messagenow invokes the MSP callback on timeout with an error indicator sodataflashReadcan retry. Otherwise, keep firing the callback fromprocess_dataeven on CRC errors (see prior diff).
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 (2)
src/js/msp.js (2)
395-401: Fix logging: replace this.timeout with this.TIMEOUT (this.timeout is undefined)this.timeout is undefined and will log "undefined" — change to this.TIMEOUT. Location: src/js/msp.js:397
Apply:
- `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} TIMEOUT: ${this.TIMEOUT} QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`,
393-413: Add bounded retries and re‑arm timer on MSP timeout; drop dead requests after N attemptsFile: src/js/msp.js
Lines: 393-413- 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)})`, - ); - serial.send(bufferOut, (_sendInfo) => { - obj.stop = performance.now(); - const executionTime = Math.round(obj.stop - obj.start); - // We should probably give up connection if the request takes too long ? - if (executionTime > 5000) { - console.warn( - `MSP: data request took too long: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} EXECUTION TIME: ${executionTime}ms`, - ); - } - - clearTimeout(obj.timer); // prevent leaks - }); - }, this.TIMEOUT); - } + if (!requestExists) { + const MAX_RETRIES = 3; + const scheduleRetry = () => { + obj.timer = setTimeout(() => { + obj.attempts = (obj.attempts || 0) + 1; + console.warn( + `MSP: data request timed-out: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} TIMEOUT: ${this.TIMEOUT} ATTEMPT: ${obj.attempts}/${MAX_RETRIES} 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); + if (executionTime > 5000) { + console.warn( + `MSP: data request took too long: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} EXECUTION TIME: ${executionTime}ms`, + ); + } + + if (obj.attempts < MAX_RETRIES) { + scheduleRetry(); + } else { + clearTimeout(obj.timer); + const idx = this.callbacks.indexOf(obj); + if (idx !== -1) this.callbacks.splice(idx, 1); // drop dead request + // Optional: emit an explicit error to the original caller (e.g. callback_msp) instead of silent drop + } + }); + }, this.TIMEOUT); + }; + scheduleRetry(); + }Single resend currently leaves requests stuck; this adds bounded retries and removes the callback entry after MAX_RETRIES. Confirm desired UX on final failure: silent drop vs. invoking callback_msp with an error payload.
♻️ Duplicate comments (1)
src/js/msp.js (1)
415-419: Fix duplicate callback handling to prevent queue growthsend_message pushes duplicate callback objects when a request with the same code already exists (no timer is set for duplicates), while the response path only removes entries from dataHandler.callbacks (MSPHelper). As a result, entries in MSP.callbacks can accumulate.
- Problem: duplicates are pushed without timers in src/js/msp.js (send_message — ~lines 382–415).
- Evidence: removal happens in src/js/msp/MSPHelper.js (process_data loop that splices dataHandler.callbacks — ~lines 1757–1767), not from MSP.callbacks.
- Fix (choose one): attach duplicate callbacks to the existing request object (store callbacks as an array) OR ensure duplicates get timers and update the response handler to remove all matching entries (splice all items with same code/requestBuffer) when a reply arrives.
- Also: make timer cleanup consistent (use clearTimeout for timeouts).
🧹 Nitpick comments (1)
src/js/msp.js (1)
382-385: Dedupe on code + payload size (low-risk improvement)Code-only dedupe can misclassify concurrent requests that share code but differ in payload. Without changing the callback resolution model, you can cheaply include payload size by leveraging the already-built bufferOut.
Apply this minimal diff (order matters):
- const requestExists = this.callbacks.some((instance) => instance.code === code); - - const bufferOut = code <= 254 ? this.encode_message_v1(code, data) : this.encode_message_v2(code, data); + const bufferOut = code <= 254 ? this.encode_message_v1(code, data) : this.encode_message_v2(code, data); + const requestExists = this.callbacks.some( + (instance) => instance.code === code && instance.requestBuffer?.byteLength === bufferOut.byteLength + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-19T20:41:42.428Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.428Z
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
📚 Learning: 2025-09-19T20:42:20.314Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.314Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/msp.js
📚 Learning: 2025-09-19T20:41:44.246Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.246Z
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.
Applied to files:
src/js/msp.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
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 (5)
src/js/msp.js (5)
60-60: Fixed, single source of truth for timeout looks goodDefining a constant timeout is a good simplification for stability.
65-68: No action neededComment-only/tiny change.
286-289: Idempotent listener registrationUsing strict equality with indexOf prevents duplicate handlers. Looks good.
374-380: Signature cleanup: removed unused parameterDropping doCallbackOnError simplifies the API and reflects actual usage. Early return still invokes callback_msp for fast-fail UX. LGTM.
420-421: Send confirmation gate is correctGuarding callback_sent on exact byte count avoids false positives. LGTM.
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.js (1)
394-414: Unbounded pending on no-response: add bounded retry + cleanupVerified: requests are pushed into this.callbacks (src/js/msp.js) and only removed when MSPHelper processes a reply (clearInterval + splice in src/js/msp/MSPHelper.js ~1764). callbacks_cleanup exists (src/js/msp.js ~440) but if the target never replies a callback entry stays in the queue indefinitely. Add a small retry budget and remove stale entries after retries.
Minimal change suggestion (unchanged from original):
const obj = { code, requestBuffer: bufferOut, callback: callback_msp, + attempts: 0, start: performance.now(), };obj.timer = setTimeout(() => { + obj.attempts += 1; 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)})`, ); - serial.send(bufferOut, (_sendInfo) => { + const MAX_RETRIES = 1; // one resend after the initial send + if (obj.attempts > MAX_RETRIES) { + clearTimeout(obj.timer); + const idx = this.callbacks.indexOf(obj); + if (idx > -1) this.callbacks.splice(idx, 1); + return; + } + serial.send(bufferOut, (_sendInfo) => { obj.stop = performance.now(); const executionTime = Math.round(obj.stop - obj.start); // We should probably give up connection if the request takes too long ? if (executionTime > 5000) { console.warn( `MSP: data request took too long: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} EXECUTION TIME: ${executionTime}ms`, ); } clearTimeout(obj.timer); // prevent leaks }); }, this.TIMEOUT);
♻️ Duplicate comments (1)
src/js/msp.js (1)
416-416: Avoid pushing duplicates without callbacksPushing every duplicate grows the queue unnecessarily when no
callback_mspis provided.- this.callbacks.push(obj); + // Avoid queuing noop duplicates + if (!requestExists || callback_msp) { + this.callbacks.push(obj); + }
🧹 Nitpick comments (1)
src/js/msp.js (1)
60-60: Fix TIMEOUT reference in log messageThe warning logs
${this.timeout}but the constant isTIMEOUT, so the message showsundefined.- `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} TIMEOUT: ${ + this.TIMEOUT + } QUEUE: ${this.callbacks.length} (${this.callbacks.map((e) => e.code)})`,Also applies to: 394-414
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-19T20:41:42.428Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.428Z
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
📚 Learning: 2025-09-19T20:42:20.314Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.314Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/msp.js
📚 Learning: 2025-09-19T20:41:44.246Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.246Z
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.
Applied to files:
src/js/msp.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
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 (5)
src/js/msp.js (5)
383-386: Duplicate detection scope is acceptable (code + payload size)This aligns with the safer, simple approach noted in your learnings; avoids the tab-switch regressions from requestKey schemes.
421-422: bytesSent guard before callback_sent is solidPrevents false positives on partial writes.
286-288: Strict equality check LGTMPrevents accidental truthiness matches for listener existence.
65-65: Minor doc polish LGTMComment fix improves clarity.
374-380: Good cleanup — removed unused doCallbackOnError param; early-return behavior is consistentSandbox verification failed (search scripts errored). Run: rg -nP --type=js 'send_message\s*([^)],[^)],[^)],[^)],[^)]*)' and confirm no callers pass five arguments to send_message.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/msp.js (1)
394-412: Single retry then potential hang/leak — add a terminal outcome
Timer resends once and clears its own timer, but if that resend also fails and no response ever arrives, obj remains in callbacks indefinitely. Over time this can bloat the queue and degrade lookups.Consider a bounded retry budget plus cleanup. Example:
+// near TIMEOUT +RETRY_LIMIT: 1, … if (!requestExists) { obj.timer = setTimeout(() => { + obj.attempts = (obj.attempts || 0) + 1; 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); // We should probably give up connection if the request takes too long ? if (executionTime > 5000) { console.warn( `MSP: data request took too long: ${code} ID: ${serial.connectionId} TAB: ${GUI.active_tab} EXECUTION TIME: ${executionTime}ms`, ); } - clearTimeout(obj.timer); // prevent leaks + clearTimeout(obj.timer); + if (obj.attempts <= this.RETRY_LIMIT) { + // re-arm a final timeout + obj.timer = setTimeout(() => { + // give up: drop the request to avoid leaks + clearTimeout(obj.timer); + const idx = this.callbacks.indexOf(obj); + if (idx > -1) this.callbacks.splice(idx, 1); + console.warn(`MSP: max retries exceeded for code ${code}`); + }, this.TIMEOUT); + } }); }, this.TIMEOUT); }If you prefer zero follow-up retries, skip the resend and just fail/cleanup after TIMEOUT.
🧹 Nitpick comments (3)
src/js/msp.js (3)
60-60: Fixed 1s TIMEOUT — consider making it configurable per transport/profile
Hard‑coded 1000ms works, but links vary (WebSerial vs BLE). Consider a small config surface (e.g., MSP.TIMEOUT from settings/env) with a sane default of 1000ms. Keeps tests and future tuning simpler.
383-386: Duplicate detection may collide on same-length payloads
requestExists matches on (code && bufferOut.byteLength). Two different payloads of equal length will be treated as duplicates, which alters timer/setup behavior and could skew latency/backoff for the second request.Minimal hardening without touching callback resolution:
-const requestExists = this.callbacks.some( - (instance) => instance.code === code && instance.requestBuffer?.byteLength === bufferOut.byteLength, -); +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 +);This stays local (no new keys/indices) and reduces false positives while preserving the simple resolution model that previously proved stable.
311-353: Nit: prefer const over let for non-reassigned buffers
Minor readability/intent improvement only.- let bufferOut = new ArrayBuffer(bufferSize); - let bufView = new Uint8Array(bufferOut); + const bufferOut = new ArrayBuffer(bufferSize); + const bufView = new Uint8Array(bufferOut);Applies similarly in encode_message_v2.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp.js(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-19T20:41:42.428Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.428Z
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
📚 Learning: 2025-09-19T20:42:20.314Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.314Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/msp.js
📚 Learning: 2025-09-19T20:41:44.246Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.246Z
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.
Applied to files:
src/js/msp.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
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 (5)
src/js/msp.js (5)
65-65: Comment/intent clarity: LGTM
Typo fix and intent (“character output”) are clear.
286-286: Strict equality in listener registration: LGTM
Correct, avoids accidental truthy matches.
414-414: Incorrect — duplicates are already removed by process_dataprocess_data iterates dataHandler.callbacks in reverse and splices every entry whose code matches (clearInterval + splice + callback), so duplicate entries are removed. I couldn't find any callbacks.push in src/js/msp; the referenced this.callbacks.push(obj) is not present. See src/js/msp/MSPHelper.js around lines ~1734 and ~1764–1767.
Likely an incorrect or invalid review comment.
419-420: No change needed — send() callbacks report full buffer length on completion.
Serial.send delegates to _protocol.send; WebBluetooth, WebSerial and WebSocket implementations call the callback with bytesSent: data.byteLength (and update protocol.bytesSent), so comparing sendInfo.bytesSent === bufferOut.byteLength is correct.
374-381: Confirm no callers rely on silent early-return from send_messagesend_message now early-returns when (code === undefined || !serial.connected || CONFIGURATOR.virtualMode) but still calls callback_msp(); confirm no callers relied on the previous “silent” behavior after doCallbackOnError was removed. File: src/js/msp.js (lines 374–381).
Run locally to find callsites: rg -nP --hidden -S --glob '!node_modules' -g '.js' -g '.ts' 'send_message\s*(' || true
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1768-1775: Suppressing callbacks on CRC error breaks retries and testsThis change prevents invoking the callback on CRC errors. It breaks:
- dataflashRead()’s retry path (expects callback on CRC error)
- The existing test “refuses to process data with crc-error” (expects the callback to fire with crcError=true)
Recommend always invoking the callback and letting callers decide using the crcError flag.
Apply this diff:
- if (!crcError) { - // fire callback - if (callback) { - callback({ command: code, data: data, length: data.byteLength, crcError: crcError }); - } - } else { - console.warn(`code: ${code} - crc failed. No callback`); - } + // Always notify; let the callee decide based on crcError + if (callback) { + callback({ command: code, data: data, length: data.byteLength, crcError }); + }
♻️ Duplicate comments (4)
src/js/msp/debug/msp_test_runner.js (1)
320-333: Drive overflow from actual MAX_QUEUE_SIZE (not a fixed 100)Use the device’s configured queue size to avoid false positives/negatives.
- // Send more requests than queue can handle - for (let i = 0; i < 100; i++) { - promises.push(window.MSP.promise(101, null).catch((err) => ({ error: err.message }))); - } + const overBy = 10; + const maxQueue = (window.MSP && Number.isFinite(window.MSP.MAX_QUEUE_SIZE)) + ? window.MSP.MAX_QUEUE_SIZE + : 100; + const attempts = maxQueue + overBy; + for (let i = 0; i < attempts; i++) { + promises.push(window.MSP.promise(101, null).catch((err) => ({ error: err.message }))); + } @@ - return { - requestsSent: 100, - successful, - rejected: 100 - successful, - }; + return { + requestsSent: attempts, + successful, + rejected: attempts - successful, + };src/js/msp/debug/msp_stress_test.js (3)
41-54: Derive status from the test and its semantics (don’t hardcode success)Generic checks misclassify scenarios (e.g., duplicates should be rejected). Also, suite marks status using getTestStatus(result) without context.
Apply:
- getTestStatus(result) { - if (!result || typeof result !== "object") return "FAILED"; - if (result.error) return "FAILED"; - if (result.memoryLeakDetected === true) return "FAILED"; - if (result.overflowHandled === false) return "FAILED"; - if (result.timeoutOccurred === true && result.recoveryTime > 2000) return "FAILED"; - if (result.recoverySuccessful === false) return "FAILED"; - if (result.failed && result.failed > 0) return "FAILED"; - if (result.duplicateRejections && result.duplicateRejections > 0) return "FAILED"; - if (result.leaked && result.leaked > 0) return "FAILED"; - if (result.failedWhileDisconnected && result.failedWhileDisconnected > 0 && result.recoverySuccessful === false) return "FAILED"; - // Add more checks as needed for new failure signals - return "PASSED"; - } + getTestStatus(testName, result) { + if (!result || typeof result !== "object") return "FAILED"; + if (result.error) return "FAILED"; + switch (testName) { + case "Duplicate Request Handling": { + const sent = result.duplicatesSent ?? 0; + const dupRej = result.duplicateRejections ?? 0; + const successful = result.successful ?? 0; + return sent > 0 && dupRej === sent && successful === 0 ? "PASSED" : "FAILED"; + } + case "Timeout Recovery": { + const timeout = result.timeoutOccurred === true; + const recovered = result.recoverySuccessful !== false; + const cleaned = result.queueCleanedUp !== false; + const withinSLO = typeof result.recoveryTime !== "number" || result.recoveryTime <= 2000; + return timeout && recovered && cleaned && withinSLO ? "PASSED" : "FAILED"; + } + case "Memory Leak Detection": + return result.memoryLeakDetected ? "FAILED" : "PASSED"; + case "Queue Overflow Handling": + return result.overflowHandled === true ? "PASSED" : "FAILED"; + case "Connection Disruption": + return result.recoverySuccessful !== false ? "PASSED" : "FAILED"; + case "Performance Under Load": + return typeof result.successRate === "number" ? (result.successRate >= 0.9 ? "PASSED" : "FAILED") : "PASSED"; + default: + break; + } + if (typeof result.failed === "number" && result.failed > 0) return "FAILED"; + if (result.memoryLeakDetected === true) return "FAILED"; + if (result.overflowHandled === false) return "FAILED"; + if (result.recoverySuccessful === false) return "FAILED"; + if (result.timeoutOccurred === true && typeof result.recoveryTime === "number" && result.recoveryTime > 2000) return "FAILED"; + if (typeof result.leaked === "number" && result.leaked > 0) return "FAILED"; + return "PASSED"; + } @@ - status: this.getTestStatus(result), + status: this.getTestStatus(testDef.name, result),Also applies to: 89-96
666-673: Single-test path also hardcodes PASSED; reuse status derivationMirror the suite behavior.
- const result = await testMethod(); - return { - name: testName, - status: "PASSED", - result, - metrics: this.monitor.getStatus(), - }; + const result = await testMethod(); + return { + name: testName, + status: this.getTestStatus( + { + "queue-flooding": "Queue Flooding", + "rapid-fire": "Rapid Fire Requests", + "duplicates": "Duplicate Request Handling", + "timeout-recovery": "Timeout Recovery", + "memory-leaks": "Memory Leak Detection", + "concurrent-mixed": "Concurrent Mixed Requests", + "queue-overflow": "Queue Overflow Handling", + "connection-disruption": "Connection Disruption", + "performance-load": "Performance Under Load", + }[testName] || testName, + result + ), + result, + metrics: this.monitor.getStatus(), + };
399-441: Simulated disconnection toggles the wrong objectsend_message likely checks the module-scope serial.connected, not this.msp.serial.connected; current test may not exercise the branch.
Proposed change (adjust import path as needed):
+// import the same serial used by MSP send path +import { serial } from "../msp.js"; // verify actual path @@ - const originalConnected = this.msp.serial?.connected; + const originalConnected = serial?.connected; @@ - if (this.msp.serial) { - this.msp.serial.connected = false; - } + if (serial) serial.connected = false; @@ - if (this.msp.serial) { - this.msp.serial.connected = originalConnected; - } + if (serial) serial.connected = originalConnected; @@ - if (this.msp.serial) { - this.msp.serial.connected = originalConnected; - } + if (serial) serial.connected = originalConnected;#!/bin/bash # Locate the canonical serial used by MSP and how connected is referenced rg -n -C2 --type=js '\bserial\b.*connected' src | sed -n '1,200p'
🧹 Nitpick comments (19)
src/js/utils/checkBrowserCompatibility.js (2)
31-35: Harden Chromium detection (UA‑CH brands) and use strict equalitySome Chromium browsers expose brands like "Google Chrome" or "Microsoft Edge" instead of only "Chromium". Also, prefer
===.Apply:
- if (navigator.userAgentData) { - return navigator.userAgentData.brands.some((brand) => { - return brand.brand == "Chromium"; - }); - } + if (navigator.userAgentData) { + return navigator.userAgentData.brands.some(({ brand }) => + ["Chromium", "Google Chrome", "Microsoft Edge"].includes(brand) + ); + }
77-85: Silence logs under test to reduce noisy outputNow that tests short‑circuit via
isTestEnvironment, gate logs to keep CI output clean.- console.log("User Agent: ", navigator.userAgentData); - console.log("Native: ", isNative); - console.log("Chromium: ", isChromium); - console.log("Web Serial: ", isWebSerial); - console.log("OS: ", getOS()); - - console.log("Android: ", isAndroid()); - console.log("iOS: ", isIOS()); - console.log("Capacitor web: ", isCapacitorWeb()); + if (!isTestEnvironment) { + console.log("User Agent: ", navigator.userAgentData); + console.log("Native: ", isNative); + console.log("Chromium: ", isChromium); + console.log("Web Serial: ", isWebSerial); + console.log("OS: ", getOS()); + console.log("Android: ", isAndroid()); + console.log("iOS: ", isIOS()); + console.log("Capacitor web: ", isCapacitorWeb()); + }src/js/msp/MSPHelper.js (1)
2455-2508: MSP.send_message signature changed; remove the 5th argument
send_messageno longer accepts a 5th parameter. Passingtruehere is a no-op and can mislead future readers. Also, given the CRC-callback behavior change above, this call currently won’t retry.Apply this diff:
MSP.send_message( MSPCodes.MSP_DATAFLASH_READ, outData, false, - function (response) { + function (response) { … - }, - true, + } );src/js/msp.js (1)
385-391: Duplicate detection is O(n · payload) per send; cache CRC on request objectsYou recompute CRC for every in-flight entry. Cache a
requestCrcon each request to reduce per-send work.Apply this diff:
- const requestExists = this.callbacks.some( + 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, + i.requestCrc === keyCrc, ); … const obj = { code, requestBuffer: bufferOut, + requestCrc: keyCrc, callback: callback_msp, start: performance.now(), };src/js/msp/debug/msp_queue_monitor.js (2)
94-113: Duplicate metric uses req.data which is never setMSP stores
requestBuffer(and notdata). This logic will undercount duplicates with payloads.Suggestion: compare against
req.requestBuffer(length + CRC) to approximate payload equality.
432-446: JSON reports will stringify Maps as {}
requestsByCodeanderrorsByTypeare Maps. Convert to plain objects in the report to preserve data.Apply this diff:
- rawMetrics: this.metrics, + rawMetrics: { + ...this.metrics, + requestsByCode: Object.fromEntries(this.metrics.requestsByCode), + errorsByType: Object.fromEntries(this.metrics.errorsByType), + },src/js/main.js (1)
21-29: Gate debug tools behind DEV to avoid shipping in prodThis unconditional dynamic import will load debug tooling in production builds. If that’s not intended, gate with
import.meta.env.DEV(and optionally localhost).Apply this diff:
-import("./msp/debug/msp_debug_tools.js") +if (import.meta.env.DEV) { + import("./msp/debug/msp_debug_tools.js") .then(() => { console.log("🔧 MSP Debug Tools loaded for development environment"); console.log("• Press Ctrl+Shift+M to toggle debug dashboard"); console.log("• Use MSPTestRunner.help() for all commands"); }) .catch((err) => { console.warn("Failed to load MSP debug tools:", err); }); +}src/js/msp/debug/msp_debug_tools.js (2)
7-11: Redundant import of msp_queue_monitorYou statically import the monitor and also dynamically import it later. Drop one to avoid confusion.
36-52: Dev detection via hostname onlyConsider also checking
import.meta.env.DEVto avoid accidental activation in locally served prod bundles.src/js/msp/debug/msp_debug_dashboard.js (7)
794-806: Fix repeated DOM writes: compare strings, not mixed typestextContent is a string; comparing it to numbers/objects forces unnecessary updates every tick.
Apply:
- if (element && element.textContent !== value) { - element.textContent = value; - } + if (element) { + const newText = String(value ?? ""); + if (element.textContent !== newText) { + element.textContent = newText; + } + }
131-134: Guard against early execution before document.body existsCreating the singleton at import time may run before DOM is ready.
- document.body.appendChild(this.container); + if (document.body) { + document.body.appendChild(this.container); + } else { + window.addEventListener('DOMContentLoaded', () => { + document.body.appendChild(this.container); + }, { once: true }); + }
192-205: Deduplicate conflicting CSS rules (.test-result-item, .alert-item, #updates-status)Same selectors are defined twice with different styles; order-dependent and brittle.
Remove one set and keep a single definition per selector (prefer the more detailed ones at lines 375–399, 411–418, 420–432).
Also applies to: 342-357, 411-418, 420-432
857-864: Harden queue item rendering when fields are missingitem.age can be undefined/NaN; render a placeholder to avoid “NaNms”.
- <span>Age: ${Math.round(item.age)}ms</span> + <span>Age: ${ + Number.isFinite(item.age) ? Math.round(item.age) + "ms" : "—" + }</span>
1156-1191: Gate global debug surface from production buildsAvoid shipping a powerful global API in prod.
-// Add global shortcut and console commands -window.MSPDebug = { +// Add global shortcut and console commands (dev only) +if ((typeof process === 'undefined') || process?.env?.NODE_ENV !== 'production') { + window.MSPDebug = { dashboard: mspDebugDashboard, monitor: mspQueueMonitor, stressTest: mspStressTest, @@ -}; + }; +}
884-887: Remove redundant alias method_updateChart merely forwards to updateChart; drop it.
- _updateChart(status) { - this.updateChart(status); - } + // (removed)Also update the single call site to call updateChart directly.
613-625: Unify default monitoring intervalThe button uses 500ms, global startMonitoring() passes no interval. Pick one default to avoid confusion.
Also applies to: 1164-1166
src/js/msp/debug/msp_test_runner.js (2)
43-49: Make periodic logging time-based, not modulo Date.now()Modulo can fire multiple times or starve depending on tick timing.
- // Log every 10 seconds if monitoring - if (Date.now() % 10000 < 500) { - console.log( + // Log ~every 10s + const now = Date.now(); + this._lastQuickLogTs = this._lastQuickLogTs ?? 0; + if (now - this._lastQuickLogTs >= 10_000) { + this._lastQuickLogTs = now; + console.log( `📊 MSP Status: Queue=${status.currentQueueSize}/${status.maxQueueSize}, Requests=${status.metrics.totalRequests}, AvgTime=${Math.round(status.metrics.avgResponseTime)}ms`, ); }
220-286: Health-check robustnessConsider a bounded overall timeout to avoid hanging if MSP stalls.
src/js/msp/debug/msp_stress_test.js (1)
132-141: Use device MAX_QUEUE_SIZE for flood test, not a magic 110Avoid under/over-stressing on different builds.
- const requestCount = 110; // More than default MAX_QUEUE_SIZE + const requestCount = (this.msp.MAX_QUEUE_SIZE || 100) + 10;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/js/main.js(2 hunks)src/js/msp.js(4 hunks)src/js/msp/MSPHelper.js(1 hunks)src/js/msp/debug/MSP_DEBUG_README.md(1 hunks)src/js/msp/debug/index.js(1 hunks)src/js/msp/debug/msp_debug_dashboard.js(1 hunks)src/js/msp/debug/msp_debug_tools.js(1 hunks)src/js/msp/debug/msp_queue_monitor.js(1 hunks)src/js/msp/debug/msp_stress_test.js(1 hunks)src/js/msp/debug/msp_test_runner.js(1 hunks)src/js/utils/checkBrowserCompatibility.js(1 hunks)test/js/msp/MSPHelper.test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-19T20:41:42.428Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.428Z
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/MSPHelper.jssrc/js/msp.jssrc/js/msp/debug/msp_queue_monitor.js
📚 Learning: 2025-09-19T20:42:20.314Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.314Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/msp.js
📚 Learning: 2025-09-19T20:41:44.246Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.246Z
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.
Applied to files:
src/js/msp.js
📚 Learning: 2025-08-27T19:36:13.733Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4579
File: src/js/tabs/pid_tuning.js:80-82
Timestamp: 2025-08-27T19:36:13.733Z
Learning: MSP_STATUS_EX calls in serial_backend.js and setup.js are intentionally unguarded because they are used for status fetching on page load to initialize profiles and are safe to call on older firmware versions. Only conditional/optional MSP_STATUS_EX usage (like in pid_tuning.js) needs version gating to API_VERSION_25_12.
Applied to files:
src/js/msp.js
📚 Learning: 2025-08-26T23:02:46.827Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp/debug/msp_queue_monitor.js:140-156
Timestamp: 2025-08-26T23:02:46.827Z
Learning: The user haslinghuis indicated that defensive programming suggestions for performance.now() and requestObj.start in MSP queue monitoring code were unnecessary as the code was previously approved. The user prefers not to add defensive guards in this context.
Applied to files:
src/js/msp/debug/msp_queue_monitor.js
🔇 Additional comments (14)
src/js/utils/checkBrowserCompatibility.js (1)
71-76: Test override for compatibility check looks goodSolid guard against ReferenceError and a pragmatic way to keep tests passing irrespective of WebSerial/USB/BLE support.
test/js/msp/MSPHelper.test.js (3)
6-6: API version bump to 1.47Import update looks correct and matches the production code path.
81-81: Aligns test setup with API 1.47Setting
FC.CONFIG.apiVersion = API_VERSION_1_47;is consistent with the new parsing branches.
14-37: Test expectation vs. implementationThis test expects the callback to fire even on CRC error. Current MSPHelper suppresses error callbacks, so this will fail. Either keep the test as-is and restore callback-on-error (recommended), or change the test to expect no callback.
src/js/msp.js (2)
374-381: Early-return callback behaviorUnconditionally invoking
callback_msp()on invalid/virtual/disconnected is acceptable after removingdoCallbackOnError. Keeps callers progressing.
399-417: Single resend after TIMEOUT — confirm intended retry policyTimer resends once and doesn’t re-arm. If the second packet is lost, the request hangs. Confirm this fixed policy is intentional for buffer-overflow mitigation.
src/js/main.js (1)
123-126: Safer first-run guard
getConfig("firstRun") ?? {}avoids NPEs on first launch. Good change.src/js/msp/debug/index.js (1)
9-15: Barrel export looks cleanCentralizing debug exports simplifies dynamic imports and tree-shaking.
src/js/msp/debug/MSP_DEBUG_README.md (1)
1-365: Solid documentationClear quick-start, commands, and troubleshooting. Matches the introduced tooling.
src/js/msp/debug/msp_debug_dashboard.js (1)
439-501: LGTM: event wiring and throttled redrawWell-contained listeners, resize handling, and rAF throttling look solid.
src/js/msp/debug/msp_test_runner.js (2)
51-54: LGTM: listener lifecycleGood: single listener stored and removed on stop to prevent duplicates.
Also applies to: 65-75
399-431: Nice UX touchConcise help text with commands/tests list. Matches stress test API.
src/js/msp/debug/msp_stress_test.js (2)
368-387: LGTM: overflow test computes rejections from settled promisesSolid approach; uses attemptedRequests/maxQueue/finalQueueSize for diagnostics.
268-299: Timeout test depends on mutating MSP.TIMEOUT; confirm it still works after “fixed timeout” refactorIf TIMEOUT is no longer honored or is read-only, this test will give false results.
Run:
If it’s constant or derived elsewhere, consider injecting a delay/fault rather than mutating TIMEOUT.
|
|
Preview URL: https://306a51c9.betaflight-configurator.pages.dev |
|
@coderabbitai any suggested changes left ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@haslinghuis Based on my check of the current PR status, I can see that there are no outstanding unresolved review comments or suggestions from CodeRabbit remaining on this PR. All previous review threads appear to have been addressed or resolved. The full review was successfully completed, and the changes look good to proceed. You should be ready to merge when you're satisfied with the current state of the code. 🏁 Script executed: Length of output: 304814 🏁 Script executed: Length of output: 674 🏁 Script executed: Length of output: 674 🏁 Script executed: Length of output: 421 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 66 @haslinghuis Based on my review of the latest PR status, I can see that CodeRabbit's most recent activity was a "COMMENTED" status (not "CHANGES_REQUESTED") on September 19, 2025, which was after their last review requesting changes. This suggests that the critical issues have been addressed. The last "CHANGES_REQUESTED" review from CodeRabbit was followed by your response and then a final "COMMENTED" status from CodeRabbit, which typically indicates informational feedback rather than blocking issues. From the extensive review history, CodeRabbit has provided feedback on:
The current state appears to be that all major requested changes have been addressed. The PR should be ready to proceed unless there are any specific concerns you'd like to address. You may want to do a final check of the CI/build status to ensure everything is passing before merging. 🏁 Script executed: Length of output: 56 🏁 Script executed: Length of output: 420 @haslinghuis Great news! Based on my analysis of the current PR state: ✅ No outstanding suggested changes remain. The PR is in a fully mergeable state:
All previous review suggestions from CodeRabbit appear to have been addressed. The last "CHANGES_REQUESTED" status was followed by subsequent "COMMENTED" status, indicating the concerns were resolved. You're ready to merge this PR whenever you're satisfied with the implementation! 🎉 |
|
- duplicate shortcut
- exported report
[msp-report-2025-09-20T13-53-11.json](https://github.com/user-attachments/files/22441110/msp-report-2025-09-20T13-53-11.json)
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests