Skip to content

Improve msp #4632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Improve msp #4632

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3135b28
Improve msp
haslinghuis Sep 25, 2025
96dbd1f
remove unused callbackonerror
haslinghuis Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 21 additions & 34 deletions src/js/msp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,7 @@ const MSP = {
packet_error: 0,
unsupported: 0,

MIN_TIMEOUT: 200,
MAX_TIMEOUT: 2000,
timeout: 200,
TIMEOUT: 1000,

last_received_timestamp: null,
listeners: [],
Expand Down Expand Up @@ -289,7 +287,7 @@ const MSP = {
});
},
listen(listener) {
if (this.listeners.indexOf(listener) == -1) {
if (this.listeners.indexOf(listener) === -1) {
this.listeners.push(listener);
}
},
Expand Down Expand Up @@ -318,8 +316,8 @@ const MSP = {
const dataLength = data ? data.length : 0;
// always reserve 6 bytes for protocol overhead !
const bufferSize = dataLength + 6;
let bufferOut = new ArrayBuffer(bufferSize);
let bufView = new Uint8Array(bufferOut);
const bufferOut = new ArrayBuffer(bufferSize);
const bufView = new Uint8Array(bufferOut);

bufView[0] = 36; // $
bufView[1] = 77; // M
Expand Down Expand Up @@ -378,25 +376,22 @@ const MSP = {
serial.send(bufferOut);
},
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;
}

Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

let requestExists = false;
for (const instance of this.callbacks) {
if (instance.code === code) {
requestExists = true;

break;
}
}

const bufferOut = code <= 254 ? this.encode_message_v1(code, data) : this.encode_message_v2(code, data);
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 obj = {
code: code,
Expand All @@ -409,31 +404,23 @@ const MSP = {
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);
}
Comment on lines 403 to 414
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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.js

Length 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 from this.callbacks before 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.


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--;
}

serial.send(bufferOut, (sendInfo) => {
if (sendInfo.bytesSent === bufferOut.byteLength) {
if (callback_sent) {
callback_sent();
}
if (sendInfo.bytesSent === bufferOut.byteLength && callback_sent) {
callback_sent();
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion src/js/msp/MSPHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1765,7 +1765,7 @@ MspHelper.prototype.process_data = function (dataHandler) {
const callbackOnError = dataHandler.callbacks[i].callbackOnError;

// remove timeout
clearInterval(dataHandler.callbacks[i].timer);
clearTimeout(dataHandler.callbacks[i].timer);

// remove object from array
dataHandler.callbacks.splice(i, 1);
Expand Down