Skip to content

Conversation

@projectgus
Copy link
Contributor

@projectgus projectgus commented Aug 28, 2025

Summary

The DTR quirk workaround from #10347 is needed for the Espressif Serial/JTAG device, but not for TinyUSB - in fact DTR must be set for TinyUSB to correctly determine if the serial port is open (and leads to issues with lost bytes otherwise).

  • On the current master branch and v1.26 release with Espressif TinyUSB >=v0.18.1, using mpremote with ESP32-S3 native serial port can result in a hang due to the Windows workaround. Specifically: if the FIFO is full, this logic in mp_usb_cdc.c determines the port is not connected and busy-loops indefinitely waiting for the FIFO to empty).
  • With upstream TinyUSB or TinyUSB <v0.18, this problem can instead cause lost bytes (as the FIFO will drop old bytes if full and it thinks it is disconnected).

The fix is to not apply the quirk in this case - it's needed to avoid a spurious reset under Windows with the Espressif Serial/JTAG device, but not needed for TinyUSB (no spurious reset observed with mpremote v1.25, pre-#10347).

This fix may help with issues discussed in https://github.com/orgs/micropython/discussions/17465 and in comments of #17865, but only on Windows (I think), as the other hosts will set DTR when they open the port.

Behaviour Change

For Windows hosts only, the following configurations change behaviour:

Type of interface Before this PR After this PR
USB-CDC Interface, Espressif VID, MicroPython TinyUSB PID Workaround Applied Workaround Not Applied (Fixes Bug)
USB-CDC Interface, Espressif VID, ESP-IDF TinyUSB PID Workaround Applied Workaround Not Applied (Fixes Bug)
USB-CDC Interface, Espressif VID, Any other PID Workaround Applied Workaround Not Applied (unknown if any examples exist)

The following configurations do not change:

Type of interface Before this PR After this PR
USB-CDC Interface, Espressif VID, USB Serial/JTAG Peripheral PID Workaround Applied Workaround Applied
USB-CDC Interface, any other VID/PID combo Workaround Not Applied Workaround Not Applied
Any non-USB-CDC interface (i.e. all dedicated USB/serial chips) Workaround Applied Workaround Applied

(Note that on Windows the test for whether something is a standard USB-CDC Interface is whether the driver used is from Microsoft rather than the chip vendor.)

Testing

  • Verified that on Windows 11 mpremote v1.26 would hang an ESP32-S3 + Octal SPIRAM board running current master (using steps provided by @gohai here, thanks @gohai!)
  • Verified that on Windows 11 mpremote v1.25 would not hang that board, and mpremote connect COMx doesn't trigger a spurious reset.
  • Verified that on Windows 11 mpremote v1.25 could trigger a spurious reset connecting to the Espressif Serial/JTAG interface of ESP32-C6. This is one of the issues fixed in tools/pyboard.py : Set DTR on Windows to avoid ESPxx hard reset. #10347.
  • Verified that using mpremote built from this branch, neither the hang nor the spurious reset occurs.

Trade-offs and Alternatives

  • We could try and find some other heuristic for "port is open", but it's hard to think of one that might not sometimes cause unnecessary long delays waiting for the host to read data over USB (when the port is actually closed).
  • I'm hopeful there might be a better way to manage DTR/RTS on Windows, but it's so fiddly that there may not be...

This work was funded through GitHub Sponsors.

@projectgus projectgus added the tools Relates to tools/ directory in source, or other tooling label Aug 28, 2025
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (8c47e44) to head (092d0f3).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #17999   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22296    22296           
=======================================
  Hits        21937    21937           
  Misses        359      359           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Josverl
Copy link
Contributor

Josverl commented Aug 28, 2025

@projectgus , I have been working on an alternative way to ID the serial ports based on VID/PID : #17800

While that guarantees that the dtr won't be set for usb-cdc, there is a risk of missing out on some serial devices.
We can bake in the known ones, and load additional ones from the Mpremote config.py now that it works across platforms

@projectgus
Copy link
Contributor Author

@projectgus , I have been working on an alternative way to ID the serial ports based on VID/PID : #17800

While that guarantees that the dtr won't be set for usb-cdc, there is a risk of missing out on some serial devices. We can bake in the known ones, and load additional ones from the Mpremote config.py now that it works across platforms

Thanks @Josverl. I did see this, but I have some concern that the list of possible ESP* USB/serial chips will trend in the limit towards the current logic (i.e. someone somewhere has made an ESP board with approximately every USB/serial converter on the market).

I actually have another idea for how to manage this, I'll try to put up a draft PR soon and see what you think.

@projectgus
Copy link
Contributor Author

projectgus commented Aug 29, 2025

Whether or not we merge #18001, I suggest we still consider this PR for a v1.26.1 release (as it's much lower risk of regression).

@dpgeorge
Copy link
Member

Yes, I think we'll need a v1.26.1.

@Josverl
Copy link
Contributor

Josverl commented Aug 29, 2025

someone somewhere has made an ESP board with approximately every USB/serial converter on the market.

I agree that the list is likely endless , and unobtainable - but I do not think that we can truncate the list to just one VID/PID either.
While I do not have any Pycom device, I think that would not be detected , while it could be detected.

So if we built the logic to add :

  • a, known but incomplete, list of VID/PIDs ,
  • a way to change the behaviour by either :
    • parameters : --dtr high|low|none --rts high|low|none ( careful of inversion logic on ESPxx boards)
    • or config ( VID / PID list )

I think we can offer both simple reliability for most boards , and ability to work with any boards.

( bit HIL testing takes time- and timeframes matter - so its OK to smash the Big bugs first, and worry about the little critters later if you want to release 1.26.1 )

@projectgus
Copy link
Contributor Author

(Sorry I wrote a reply here earlier but GitHub seems to have eaten it.)

its OK to smash the Big bugs first, and worry about the little critters later if you want to release 1.26.1 )

Yes, this PR is only an attempt to fix the 1.26 regression introduced by #10347, not to solve all Windows USB/Serial issues. I've updated the description with some tables to make it clearer what does and doesn't change with this PR.

Regarding the big picture on Windows USB/serial logic: I think we can discuss this in the comments on #17800 and #18001. I still want to figure out if/when the latter approach doesn't work, if it turns out to be unsuitable then something like #17800 seems like the next best option. Adding a way to override the mpremote DTR & RTS defaults seems like a good feature regardless, fully agree.

Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this "hot fix" for now, and then replace it with the full solution #18001.

The DTR quirk workaround from dea949e is needed for the Espressif
Serial/JTAG device, but not for TinyUSB - in fact DTR must be set for
TinyUSB to correctly determine if the serial port is open (and leads to
issues with lost bytes otherwise).

See discussion in PR micropython#17999.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
@dpgeorge dpgeorge force-pushed the bugfix/mpremote_tinyusb_hang branch from 092d0f3 to 1aaf6ed Compare September 10, 2025 06:56
@dpgeorge dpgeorge merged commit 1aaf6ed into micropython:master Sep 10, 2025
70 of 71 checks passed
dpgeorge pushed a commit that referenced this pull request Sep 11, 2025
The DTR quirk workaround from dea949e is needed for the Espressif
Serial/JTAG device, but not for TinyUSB - in fact DTR must be set for
TinyUSB to correctly determine if the serial port is open (and leads to
issues with lost bytes otherwise).

See discussion in PR #17999.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
trwbox pushed a commit to trwbox/micropython that referenced this pull request Sep 12, 2025
The DTR quirk workaround from dea949e is needed for the Espressif
Serial/JTAG device, but not for TinyUSB - in fact DTR must be set for
TinyUSB to correctly determine if the serial port is open (and leads to
issues with lost bytes otherwise).

See discussion in PR micropython#17999.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
membrowse pushed a commit to membrowse/micropython that referenced this pull request Oct 27, 2025
The DTR quirk workaround from dea949e is needed for the Espressif
Serial/JTAG device, but not for TinyUSB - in fact DTR must be set for
TinyUSB to correctly determine if the serial port is open (and leads to
issues with lost bytes otherwise).

See discussion in PR micropython#17999.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools Relates to tools/ directory in source, or other tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants