Skip to content
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.

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

Serial Monitor should handle disconnects more gracefully #3939

Closed
cinderblock opened this issue Apr 25, 2021 · 20 comments
Closed

Serial Monitor should handle disconnects more gracefully #3939

cinderblock opened this issue Apr 25, 2021 · 20 comments

Comments

@cinderblock
Copy link

cinderblock commented Apr 25, 2021

Edit from maintainers - Temporary solution: https://docs.platformio.org/en/latest/faq/advanced-serial-monitor.html


Serial Monitor should handle disconnects more gracefully

This is a bug report/feature request to improve the serial port functionality in Platform IO.

Configuration

Operating system: Windows 10 x64

PlatformIO Version (platformio --version): PlatformIO Core, version 5.1.1

Description of problem

Using serial monitor on a USB based serial port crashes with Exception

Steps to Reproduce

  1. Start a serial monitor to a device with a USB based serial port: pio device monitor
  2. Reset the device

Actual Results

Exception in thread rx:
Traceback (most recent call last):
  File "C:\Users\camer\.platformio\python3\lib\threading.py", line 926, in _bootstrap_inner
    self.run()
  File "C:\Users\camer\.platformio\python3\lib\threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "c:\users\camer\.platformio\penv\lib\site-packages\serial\tools\miniterm.py", line 445, in reader
    data = self.serial.read(self.serial.in_waiting or 1)
  File "c:\users\camer\.platformio\penv\lib\site-packages\serial\serialwin32.py", line 257, in in_waiting
    raise SerialException("ClearCommError failed ({!r})".format(ctypes.WinError()))
serial.serialutil.SerialException: ClearCommError failed (PermissionError(13, 'The device does not recognize the command.', None, 22))

Expected Results

An error message designed for a novice user to understand.

Ideally also (optional) automatic reconnect when the device comes back.

@ivankravets ivankravets added this to the 5.2.1 milestone Sep 13, 2021
@ivankravets ivankravets self-assigned this Sep 13, 2021
@ivankravets ivankravets modified the milestones: 5.2.1, 5.2.2 Oct 11, 2021
@ivankravets ivankravets modified the milestones: 5.2.3, Backlog Oct 25, 2021
@jcw
Copy link

jcw commented Apr 28, 2022

For references, here is the slightly different traceback on MacOS:

--- exit ---
Traceback (most recent call last):
  File "/usr/local/Cellar/platformio/5.2.5/libexec/lib/python3.9/site-packages/serial/serialposix.py", line 575, in read
    buf = os.read(self.fd, size - len(read))
OSError: [Errno 6] Device not configured

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/threading.py", line 973, in _bootstrap_inner
    self.run()
  File "/usr/local/Cellar/[email protected]/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/local/Cellar/platformio/5.2.5/libexec/lib/python3.9/site-packages/serial/tools/miniterm.py", line 499, in reader
    data = self.serial.read(self.serial.in_waiting or 1)
  File "/usr/local/Cellar/platformio/5.2.5/libexec/lib/python3.9/site-packages/serial/serialposix.py", line 581, in read
    raise SerialException('read failed: {}'.format(e))
serial.serialutil.SerialException: read failed: [Errno 6] Device not configured

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

This is a basic thing (e.g. done by the Arduino IDE monitor), so I'd like to see it.

Most UI's would use some connection status indicator (Arduino IDE uses backgroud color), but for a simple text terminal it should be enough to print <disconnect>.

Maybe this is complicated by device / port autodetection by device monitor, but a first pass would have it only reconnect to the same port.

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

pyserial itself doesn't have any special reconnect support: pyserial/pyserial#558

if monitor implements this, it will have to be in a crude way like suppress the exception and silently retry connection every second

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

monitor is using the "miniterm" tool of pyserial. It's basically a stand-alone program, and exceptions happen in background threads. So it will be hard to control error output.

options for implementing reconnect:

  1. live with the spamming on the output when there are disconnects, and during retry polling
  2. try to filter stderr to suppress the spam
  3. fork miniterm (about 1,000 loc) and enhance it for reconnect
  4. contribute reconnect option to miniterm and use it <--

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

I implemented --reconnect option for pyserial miniterm... but then realized this project appears unmaintained (no commits in 6 months, backlog of PR's) 😢

pyserial/pyserial#651

@ivankravets
Copy link
Member

@belm0, thank you so much for your contribution to the OSS.

How about having a separate PR that just catches exceptions or exits gracefully without reconnecting? Typically, this issue arises when a user does not close the serial terminal but unplugs the board.

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

I think a common case is uploading to a device. It will reset the device and cause a lost connection. This means that the monitor must be restarted after each upload.

Since I now have the enhanced miniterm written anyway, I'll try a PR that adds that code locally to platformio.

@ivankravets
Copy link
Member

You can't upload to the device if you don't close the serial monitor. Otherwise, you will receive busy resources error.

@belm0
Copy link
Contributor

belm0 commented May 15, 2022

I see-- it's my misunderstanding. Uploading and monitoring have to be coordinated.

How about having a separate PR that just catches exceptions or exits gracefully

It's not possible to suppress the exception trace with the current miniterm, unless the stderr is filtered somehow. So this too would need a miniterm fork.

@ivankravets
Copy link
Member

Is it possible to refactor your PR to suppress unhandled exceptions?

@jcw
Copy link

jcw commented May 15, 2022

Uploading and monitoring have to be coordinated.

It depends ... with ST-Link, a uC reset does not generate a USB reset/re-enumerate condition, so re-flashing will keep a terminal session intact if it's running in a separate window. I'm not sure this applies here, just wanted to flag this case.

@egnor
Copy link

egnor commented Jun 7, 2022

I implemented --reconnect option for pyserial miniterm... but then realized this project appears unmaintained (no commits in 6 months, backlog of PR's) cry
pyserial/pyserial#651

😢

My very very humble outsider 🪙🪙 would be-- miniterm is not magical, it's got a bunch of weird warts/bugs, and most of its features (menus that let you dynamically change settings and switch to new ports and such) aren't particularly interesting in this use case. Replacing it with something solid that just shuffles data between the serial port and the terminal (without blocking), handles disconnection gracefully, and allows robust cancellation would seem like a decent move to me.

@ivankravets ivankravets modified the milestones: Backlog, 6.1 Jun 7, 2022
@ivankravets
Copy link
Member

Replacing it with something solid that just shuffles data between the serial port and the terminal

Agree with you. We will work on this.

@belm0
Copy link
Contributor

belm0 commented Jun 7, 2022

Agreed that miniterm is not magical. It's just an example use of the pyserial library, and they added it to the public API for better or worse.

While it's a noble intention to replace it with something simple (and perhaps remove pyserial dependency altogether), it's not necessary. There are probably more important places for platformio developers to invest their time.

Forking the miniterm example for our own purposes seems fine (as it uses the public pyserial API). Ivan, please consider again this #4268 (comment).

@cinderblock
Copy link
Author

@ivankravets The suggested "advanced" serial monitor leaves a lot to be desired. The main feature of pio automagically controlling the serial monitor during upload is sorely missing with 3rd party serial monitors.

@ivankravets ivankravets removed this from the 6.1 milestone Jun 10, 2022
@ivankravets ivankravets added this to the 6.0.3 milestone Jun 10, 2022
@ivankravets
Copy link
Member

ivankravets commented Jun 11, 2022

Hi All!

Great news! Huge refactoring to the device monitor logic and it seems this issue has been solved. Also, automatic reconnection has been implemented.

Please re-test with pio upgrade --dev.

Does it work as expected now?

@m1cr0lab
Copy link

@ivankravets Thank you very much for this correction!

@zalexzperez
Copy link

Does it work as expected now?

Hello,

I still have the issue on my ESP32-S3 using Windows 11 computer.
After resetting the board, I click on a blank space in the terminal and then press any keyboard key, this is shown:

Disconnected (ClearCommError failed (PermissionError(13, 'The device does not recognize the command.', None, 22)))
Reconnecting to COM7     Connected!

I tried pio upgrade --dev, it did update but the problem is still there.

@brentthierens
Copy link

Same problem here. When I reset the board, the output is just gone. It doesn't auto reconnect. Manually entering something will cause this message and the device works again. Also WIN11 if that matters.

@mozzhead164
Copy link

Also same behaviour on an Adafruit MatrixPortal S3, if I use ESP.restart() in my script, the Serial Monitor disconnects when the ESP32S3 reboots. Also have to manually reset after uploading code for program to start.

platform = espressif32
board = adafruit_matrixportal_esp32s3
framework = arduino
board_build.mcu = esp32s3
build_flags =
	-D ARDUINO_USB_CDC_ON_BOOT=1
monitor_speed = 115200
monitor_port = COM4

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

Successfully merging a pull request may close this issue.

9 participants