Serial monitor suspends and resumes when (un)plugging other serial port

[matthijskooijman]

I've noticed an issue where the serial port gets closed unexpectedly by the serial monitor. To reproduce:

• Attach two USB serial ports (e.g. two Arduinos)
• Open up the IDE serial monitor for one of them
• Unplug the other
• The serial monitor will suspend (become disabled, close the serial port) and resume (reopen serial port, reset Arduino if applicable).

This problem does not seem to occur always, but often, because there's a race condition involved.

Under the hood, what happens is:

• Every second, SerialDiscovery refreshes the list of serial ports
• Also every second, AbstractMonitor checks to see if the port is still there (or if it went away, to see if it returned).
• These timers fire pretty much at the same time, so they run concurrently. This means that AbstractMonitor requests a list halfway through the refresh done by SerialDiscovery. This should be fixed by adding some synchronized statements.
• The window for this race condition is enlarged by another bug. During refresh, SerialDiscovery incorrectly matches full port strings returned by liblistserials (including vidpid, e.g. /dev/ttyACM0_16D0_0F44) against BoardPort.toString() (which returns just the port path without the vidpid). This means that whenever the port list changes, it treats all ports as being disconnected and reconnected as new, unique, ports.
• In particular, this causes all existing ports to be marked as offline early in the refresh process. If AbstractMonitor then requests a list of (online) serial ports, this will return the empty list, causing the monitor to close.

The minimal fix, I suspect, would be:

• Fix the port name comparison by splitting the string returned by liblistserial early, and then use just the port name for comparisons.
• Make all access of serialBoardPorts synchronized (on second thought: Maybe better to actually replace the serialBoardPorts array completely rather than clearing and refilling it when it changes, since that is probably also atomic, and also makes sure that any callers that have a previous version of the list will not see their list change all of a sudden (on closer inspection, it seems that a copy is already returned now, so this does not really matter).

However, looking around this code, it does seem like it's a bit of mess. I would think some refactoring might be helpful, except that I suspect that (serial) port discovery will soon be delegated to arduino-cli? @cmaglie, I suspect you can comment on that?

Regardless, some things I think could be improved are:

• Instead of listing ports and looking in the list for the serial monitor port, one could simply look at the boardPort.isOnline() which (if the above is fixed) should switch to false and back to true.
• Add an event handler for isOnline changes instead of having a timer in AbstractMonitor, to make sure a quick offline/online transition is not missed.
• Currently, serial port listing returns a port name, vid and pid, but a few lines further down, resolveDeviceByVendorIdProductId() is called which again looks up the vidpid for the port name. The latter could probably be skipped and just use the vidpid that are already known. Update: On closer inspection, it seems that the latter actually also returns info on the serial number and product name, while the serial port list only has vidpi.
• I think there is some substring matching that could end up with false positives iSerial contains something that looks like a PID, or something like that.

[facchinm]

Hi @matthijskooijman ,
while in the long run the discovery is going to be pluggable, a small workaround to overcome the disconnection of connected ports would not hurt 😉
Do you already have a patch ready? Otherwise I'm tackling it

[matthijskooijman]

Do you already have a patch ready? Otherwise I'm tackling it

Nope, I have not done anything yet other than investigate and report above.

[facchinm]

Great, I'm on it

[facchinm]

PR submitted #9862.
I squashed 3 commits in one since the changes were deeply tied but I can "desquash" if you think it's better for the future reader.