fix serial port open

[error414]

This PR needs review by someone who knows JS well.

I noticed that opening serial port is slow. The reasson is that serial port is not opened and configurator tries to write data serial port. It causes restart MSP quering and delay for opening configurator.

The flow is:

Connecting to: COM5
connection.js:88  Connection opened with ID: 1, Baud: 115200

connectionSerial.js:121  Serial write error: Invalid serial port or port closed <-- IT'S NOT CORRECT
serial_queue.js:285      Force free hard lock
serial_queue.js:146      MSP data request timed-out: 1

intervals.js:114  Killing all intervals except:
  - global_data_refresh
  - msp-load-update
  - ltm-connection-check

fc.js:205  Generating AUX_CONFIG

I would say it's caused by calling resolve before port is fully opened, right aproach call resolve when port is fully opened. See pull request code.

After my patch is connection to FC much faster and log look more clean

Connecting to: COM5
connection.js:88  Connection opened with ID: 1, Baud: 115200

intervals.js:114  Killing all intervals except:
 - global_data_refresh
 - msp-load-update
 - ltm-connection-check

fc.js:205  Generating AUX_CONFIG

intervals.js:114  Killing all intervals except:
 - global_data_refresh
 - msp-load-update
 - ltm-connection-check

[github-actions]

Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

• maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.

• maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.

---

This is an automated suggestion to help route contributions to the appropriate branch.

[qodo-code-review]

PR Compliance Guide 🔍

All compliance sections have been disabled in the configurations.

[sensei-hacker]

Thanks for this. I reworked the serial port handling some here too:

https://github.com/iNavFlight/inav-configurator/pull/2433/changes

[sensei-hacker]

To really do it right, we might want to also add serialport.on('error' along with serialport.on('open':

  this._serialport.on('open', () => {
      resolve({error: false, id: this._id++});
  });
  this._serialport.on('error', (err) => {
      resolve({error: true, errorMsg: err});
  });

That way we don't have an unresolved promise hanging around.

[error414]

there is error handling,

inav-configurator/js/main/serial.js

Line 16 in db9a6a1

this._serialport.on('error', error => {

[sensei-hacker]

there is error handling,

inav-configurator/js/main/serial.js

Line 16 in db9a6a1

this._serialport.on('error', error => {

Good catch, there IS an error handler. It just leaves the promise unresolved and the then() not firing.
We could add:

resolve({error: true, errorMsg: error.message || error});

Or to be defensive about it to make sure we never try to double-resolve.
This may protect against going to callback hell if the USB cable is flaky:

  let resolved = false; // Prevent double-resolution

  this._serialport.on('error', error => {
      if (!window.isDestroyed()) {
          window.webContents.send('serialError', error);
      }
      // Also resolve the Promise on error
      if (!resolved) {
          resolved = true;
          resolve({error: true, errorMsg: error.message || error});
      }
  });

  this._serialport.on('open', () => {
      if (!resolved) {
          resolved = true;
          resolve({error: false, id: this._id++});
      }
  });

[sensei-hacker]

/ask qodo, do you have comments on the above discussion?

[qodo-code-review]

Ask❓

qodo, do you have comments on the above discussion?

Answer:

Yes — the change to resolve only after the "open" event is a good direction, but a couple of details are worth tightening up for robustness and consistency:

1. Event timing and duplicate resolve

• If the port is already open before you attach the "open" listener (some drivers emit it immediately after construction or "open" call), there’s a slight risk that the event was missed, and the promise never resolves.
• To guard against multiple resolves (e.g., error then open), track a local "settled" flag, and clean up listeners once settled.

2. Error handling parity

• Right now, you resolve({ error: true }) only from the try/catch around new SerialPort, not from async "error" events emitted by the serial port before "open". Add an "error" listener to resolve the promise with an error if opening fails.

3. Listener cleanup

• After the promise resolves or rejects, remove the "open" and "error" listeners used for the initial handshake to avoid leaks. Keep the "data" listener as intended for runtime communication.

4. Maintain previous behavior of data forwarding

• Your change preserves the data listener, which is good. Just ensure data isn’t processed before open if that could occur with some transports. If that’s a concern, buffer "data" until open, or only attach "data" after open.

Example adjustment:

• Pseudocode sketch showing the above ideas:
  • let settled = false
  • const onOpen = () => { if (!settled) { settled = true; cleanup(); resolve({ error: false, id: this._id++ }); } }
  • const onError = (err) => { if (!settled) { settled = true; cleanup(); resolve({ error: true, errorMsg: err?.message || err }); } }
  • attach 'open' and 'error' listeners before/just after constructing/initiating open
  • function cleanup() { this._serialport.off('open', onOpen); this._serialport.off('error', onError); }

5. Consider edge cases

• If the port instance is created in a state where it’s already open, check this._serialport.isOpen (if using serialport package v10+) and resolve synchronously in that case.
• If you rely on "this._id++" being incremented per successful connection, ensure it’s only incremented on success (your change already enforces that, which is good).

Net: The core idea (resolve on "open") fixes the race that caused early writes and timeouts. Adding an "error" path and cleanup will make it production-safe across platforms/drivers.

[sensei-hacker]

I've found qodo is right about half the time, so the comments from the bot shouldn't be accepted at face value, but are things to consider.

[error414]

I amend your suggestion it meakes sense. I'm not fun of remove listeners, it makes code more dificult to read.