Skip to content

Cleanup serial #4670

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

Merged
merged 17 commits into from
Oct 20, 2025
Merged

Cleanup serial #4670

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4a04960
Cleanup serial
haslinghuis Oct 19, 2025
5f041f8
CR review
haslinghuis Oct 19, 2025
af3cf17
Revert non working CR suggestion :(
haslinghuis Oct 19, 2025
99b2ceb
Some Sonar
haslinghuis Oct 19, 2025
c75c3c8
CR review II
haslinghuis Oct 19, 2025
2b8ff38
CR review III
haslinghuis Oct 19, 2025
4bad40b
Sonar II
haslinghuis Oct 19, 2025
6ef9729
Fix setMask intermittent race condition
haslinghuis Oct 19, 2025
f749d0b
Fix more
haslinghuis Oct 19, 2025
16afa83
Coderabbitai fix (protocolType is a getter, not a method)
haslinghuis Oct 19, 2025
5d952ee
Improve error message
haslinghuis Oct 19, 2025
563e89f
CR nitpick
haslinghuis Oct 19, 2025
063e8b4
CR suggestion
haslinghuis Oct 19, 2025
14a01a0
Revert CR suggestion :(
haslinghuis Oct 19, 2025
c62d4c5
Rename protocolType getter
haslinghuis Oct 19, 2025
13dc315
Nitpicks
haslinghuis Oct 20, 2025
8ea554a
Update CR previous suggestion
haslinghuis Oct 20, 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
19 changes: 11 additions & 8 deletions src/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import "./jqueryPlugins";
import $ from "jquery";
import "../components/init.js";
import { gui_log } from "./gui_log.js";
// same, msp seems to be everywhere used from global scope
import "./msp/MSPHelper.js";
import { i18n } from "./localization.js";
import GUI, { TABS } from "./gui.js";
import { get as getConfig, set as setConfig } from "./ConfigStorage.js";
Expand Down Expand Up @@ -184,13 +182,18 @@ function startProcess() {
return;
}

if (GUI.allowedTabs.indexOf(tab) < 0 && tab === "firmware_flasher") {
if (GUI.connected_to || GUI.connecting_to) {
$("a.connection_button__link").click();
if (!GUI.allowedTabs.includes(tab)) {
if (tab === "firmware_flasher") {
// Special handling for firmware flasher tab
if (GUI.connected_to || GUI.connecting_to) {
$("a.connection_button__link").trigger("click");
}
// This line is required but it triggers opening the firmware flasher tab again
$("a.firmware_flasher_button__link").trigger("click");
} else {
gui_log(i18n.getMessage("tabSwitchUpgradeRequired", [tabName]));
return;
}
} else if (GUI.allowedTabs.indexOf(tab) < 0) {
gui_log(i18n.getMessage("tabSwitchUpgradeRequired", [tabName]));
return;
}

GUI.tab_switch_in_progress = true;
Expand Down
9 changes: 0 additions & 9 deletions src/js/protocols/WebSerial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,15 +378,6 @@ class WebSerial extends EventTarget {
return { bytesSent: 0 };
}
}

/**
* Clean up resources when the protocol is no longer needed
*/
cleanup() {
if (this.connected) {
this.disconnect();
}
}
}

// Export the class itself, not an instance
Expand Down
225 changes: 54 additions & 171 deletions src/js/serial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,83 +15,40 @@ class Serial extends EventTarget {

this.logHead = "[SERIAL]";

// Initialize the available protocols
this._webSerial = new WebSerial();
this._webBluetooth = new WebBluetooth();
this._webSocket = new Websocket();
this._virtual = new VirtualSerial();

// Update protocol map to use consistent naming
this._protocolMap = {
webserial: this._webSerial,
webbluetooth: this._webBluetooth,
websocket: this._webSocket,
virtual: this._virtual,
};
// Initialize protocols with metadata for easier lookup
this._protocols = [
{ name: "webserial", instance: new WebSerial() },
{ name: "webbluetooth", instance: new WebBluetooth() },
{ name: "websocket", instance: new Websocket() },
{ name: "virtual", instance: new VirtualSerial() },
];

// Forward events from all protocols to the Serial class
this._setupEventForwarding();
}

// Add a getter method to safely access the protocol map
_getProtocolByType(type) {
if (!type) {
return this._protocol;
}

const protocol = this._protocolMap[type.toLowerCase()];

if (!protocol) {
console.warn(`${this.logHead} Unknown protocol type: ${type}`);
}

return protocol || null;
}

/**
* Get the protocol type as a string
* @param {Object} protocol - Protocol instance
* @returns {string} - Protocol type name
*/
_getProtocolType(protocol) {
if (protocol === this._webSerial) {
return "webserial";
}
if (protocol === this._webBluetooth) {
return "webbluetooth";
}
if (protocol === this._webSocket) {
return "websocket";
}
if (protocol === this._virtual) {
return "virtual";
}
return "unknown";
}

/**
* Set up event forwarding from all protocols to the Serial class
*/
_setupEventForwarding() {
const protocols = [this._webSerial, this._webBluetooth, this._webSocket, this._virtual];
const events = ["addedDevice", "removedDevice", "connect", "disconnect", "receive"];

protocols.forEach((protocol) => {
if (protocol && typeof protocol.addEventListener === "function") {
events.forEach((eventType) => {
protocol.addEventListener(eventType, (event) => {
for (const { name, instance } of this._protocols) {
if (typeof instance?.addEventListener === "function") {
for (const eventType of events) {
instance.addEventListener(eventType, (event) => {
let newDetail;
if (event.type === "receive") {
// For 'receive' events, we need to handle the data differently
newDetail = {
data: event.detail,
protocolType: this._getProtocolType(protocol),
protocolType: name,
};
} else {
// For other events, we can use the detail directly
newDetail = {
...event.detail,
protocolType: this._getProtocolType(protocol),
protocolType: name,
};
}

Expand All @@ -104,54 +61,29 @@ class Serial extends EventTarget {
}),
);
});
});
}
}
});
}
}

/**
* Selects the appropriate protocol based on port path
* @param {string|null} portPath - Optional port path to determine protocol
* @param {boolean} forceDisconnect - Whether to force disconnect from current protocol
* @param {string|null} portPath - Port path to determine protocol
*/
selectProtocol(portPath = null, forceDisconnect = true) {
selectProtocol(portPath) {
// Determine which protocol to use based on port path
let newProtocol;

if (portPath) {
// Select protocol based on port path
if (portPath === "virtual") {
console.log(`${this.logHead} Using virtual protocol (based on port path)`);
newProtocol = this._virtual;
} else if (portPath === "manual" || /^(tcp|ws):\/\/([A-Za-z0-9.-]+)(?::(\d+))?$/.test(portPath)) {
console.log(`${this.logHead} Using websocket protocol (based on port path)`);
newProtocol = this._webSocket;
} else if (portPath.startsWith("bluetooth")) {
console.log(`${this.logHead} Using bluetooth protocol (based on port path: ${portPath})`);
newProtocol = this._webBluetooth;
} else {
console.log(`${this.logHead} Using web serial protocol (based on port path: ${portPath})`);
newProtocol = this._webSerial;
}
const s = typeof portPath === "string" ? portPath : "";
// Default to webserial for typical serial device identifiers.
if (s === "virtual") {
return this._protocols.find((p) => p.name === "virtual")?.instance;
}

// If we're switching to a different protocol
if (this._protocol !== newProtocol) {
// Clean up previous protocol if exists
if (this._protocol && forceDisconnect) {
// Disconnect if connected
if (this._protocol.connected) {
console.log(`${this.logHead} Disconnecting from current protocol before switching`);
this._protocol.disconnect();
}
}

// Set new protocol
this._protocol = newProtocol;
console.log(`${this.logHead} Protocol switched successfully to:`, this._protocol);
if (s === "manual" || /^(tcp|ws|wss):\/\/[A-Za-z0-9.-]+(?::\d+)?(\/.*)?$/.test(s)) {
return this._protocols.find((p) => p.name === "websocket")?.instance;
}

return this._protocol;
if (s.startsWith("bluetooth")) {
return this._protocols.find((p) => p.name === "webbluetooth")?.instance;
}
return this._protocols.find((p) => p.name === "webserial")?.instance;
}

/**
Expand All @@ -161,38 +93,15 @@ class Serial extends EventTarget {
*/
async connect(path, options, callback) {
// Select the appropriate protocol based directly on the port path
this.selectProtocol(path);

if (!this._protocol) {
console.error(`${this.logHead} No valid protocol selected for connection`);
return false;
}

// Check if already connected
if (this._protocol.connected) {
console.warn(`${this.logHead} Protocol already connected, not connecting again`);

// If we're already connected to the requested port, return success
const connectedPort = this._protocol.getConnectedPort?.();
if (connectedPort && connectedPort.path === path) {
console.log(`${this.logHead} Already connected to the requested port`);
return true;
}

// If we're connected to a different port, disconnect first
console.log(`${this.logHead} Connected to a different port, disconnecting first`);
const success = await this.disconnect();
if (!success) {
console.error(`${this.logHead} Failed to disconnect before reconnecting`);
return false;
}

console.log(`${this.logHead} Reconnecting to new port:`, path);
return this._protocol.connect(path, options, callback);
let result = false;
try {
this._protocol = this.selectProtocol(path);
result = await this._protocol.connect(path, options);
} catch (error) {
console.error(`${this.logHead} Error during connection:`, error);
}

console.log(`${this.logHead} Connecting to port:`, path, "with options:", options);
return this._protocol.connect(path, options, callback);
callback?.(result);
return result;
}

/**
Expand All @@ -201,35 +110,14 @@ class Serial extends EventTarget {
* @returns {Promise<boolean>} - Promise resolving to true if disconnection was successful
*/
async disconnect(callback) {
// Return immediately if no protocol is selected
if (!this._protocol) {
console.warn(`${this.logHead} No protocol selected, nothing to disconnect`);
if (callback) callback(false);
return false;
}

let result = false;
try {
// Handle case where we're already disconnected
if (!this._protocol.connected) {
console.log(`${this.logHead} Already disconnected, performing cleanup`);
if (callback) {
callback(true);
}
return true;
}

// Create a promise that will resolve/reject based on the protocol's disconnect result
const success = await this._protocol.disconnect();

if (callback) callback(success);
return success;
result = await this._protocol?.disconnect();
} catch (error) {
console.error(`${this.logHead} Error during disconnect:`, error);
if (callback) {
callback(false);
}
return Promise.resolve(false);
}
callback?.(result);
return result;
}

/**
Expand All @@ -241,7 +129,6 @@ class Serial extends EventTarget {
if (callback) callback({ bytesSent: 0 });
return { bytesSent: 0 };
}

return this._protocol.send(data, callback);
}

Expand All @@ -253,7 +140,7 @@ class Serial extends EventTarget {
async getDevices(protocolType = null) {
try {
// Get the appropriate protocol
const targetProtocol = this._getProtocolByType(protocolType);
const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance;

if (!targetProtocol) {
console.warn(`${this.logHead} No valid protocol for getting devices`);
Expand All @@ -265,7 +152,7 @@ class Serial extends EventTarget {
return [];
}

return targetProtocol.getDevices() || [];
return targetProtocol.getDevices?.() || [];
} catch (error) {
console.error(`${this.logHead} Error getting devices:`, error);
return [];
Expand All @@ -278,26 +165,15 @@ class Serial extends EventTarget {
* @param {string} protocolType - Optional protocol type ('serial', 'bluetooth', etc.)
* @returns {Promise<Object>} - Promise resolving to the selected device
*/
async requestPermissionDevice(showAllDevices = false, protocolType = null) {
async requestPermissionDevice(showAllDevices = false, protocolType) {
let result = false;
try {
// Get the appropriate protocol
const targetProtocol = this._getProtocolByType(protocolType);

if (!targetProtocol) {
console.warn(`${this.logHead} No valid protocol for permission request`);
return null;
}

if (typeof targetProtocol.requestPermissionDevice !== "function") {
console.error(`${this.logHead} Selected protocol does not support permission requests`);
return null;
}

return targetProtocol.requestPermissionDevice(showAllDevices);
const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance;
result = await targetProtocol?.requestPermissionDevice(showAllDevices);
} catch (error) {
console.error(`${this.logHead} Error requesting device permission:`, error);
return null;
}
return result;
}

/**
Expand All @@ -311,7 +187,14 @@ class Serial extends EventTarget {
* Get connection status
*/
get connected() {
return this._protocol ? this._protocol.connected : false;
return this._protocol?.connected || false;
}

/**
* Get connectionId
*/
get connectionId() {
return this._protocol?.connectionId || null;
}
}

Expand Down
Loading