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 all commits
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
58 changes: 22 additions & 36 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 @@ -377,63 +375,51 @@ const MSP = {

serial.send(bufferOut);
},
send_message(code, data, callback_sent, callback_msp, doCallbackOnError) {
const connected = serial.connected;

if (code === undefined || !connected || CONFIGURATOR.virtualMode) {
send_message(code, data, callback_sent, callback_msp) {
if (code === undefined || !serial.connected || CONFIGURATOR.virtualMode) {
if (callback_msp) {
callback_msp();
}
return false;
}

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,
requestBuffer: bufferOut,
callback: callback_msp,
callbackOnError: doCallbackOnError,
start: performance.now(),
};

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

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
5 changes: 2 additions & 3 deletions src/js/msp/MSPHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1762,14 +1762,13 @@ MspHelper.prototype.process_data = function (dataHandler) {
if (dataHandler.callbacks[i]?.code === code) {
// save callback reference
const callback = dataHandler.callbacks[i].callback;
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);
if (!crcError || callbackOnError) {
if (!crcError) {
// fire callback
if (callback) {
callback({ command: code, data: data, length: data.byteLength, crcError: crcError });
Expand Down