Skip to content

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

Closed
@matthijskooijman

Description

@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.

Activity

facchinm

facchinm commented on Mar 10, 2020

@facchinm
Member

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

matthijskooijman commented on Mar 10, 2020

@matthijskooijman
CollaboratorAuthor

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

facchinm commented on Mar 10, 2020

@facchinm
Member

Great, I'm on it

facchinm

facchinm commented on Mar 10, 2020

@facchinm
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Serial monitor suspends and resumes when (un)plugging other serial port · Issue #9785 · arduino/Arduino