-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
mpremote: Don't apply Espressif DTR/RTS quirk to TinyUSB CDC device. #17999
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
mpremote: Don't apply Espressif DTR/RTS quirk to TinyUSB CDC device. #17999
Conversation
|
Code size report: |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
@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. |
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. |
|
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). |
|
Yes, I think we'll need a v1.26.1. |
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. So if we built the logic to add :
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 ) |
|
(Sorry I wrote a reply here earlier but GitHub seems to have eaten it.)
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. |
dpgeorge
left a comment
There was a problem hiding this 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]>
092d0f3 to
1aaf6ed
Compare
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]>
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]>
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]>
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).
mpremotewith 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).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:
The following configurations do not change:
(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
mpremote connect COMxdoesn't trigger a spurious reset.Trade-offs and Alternatives
This work was funded through GitHub Sponsors.