[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208

chipweinberger · 2022-11-18T23:44:35Z

Older PR: #10099

Tested working on a ESP32-S3.

Uses Ivan's suggestions for Usb Serial JTAG TX with the driver.

Instead of introducing this new function, would it instead make more sense to fix the TX-blocking-indefinitely behavior?

This is already done in usb_serial_jtag_tx_char with the TX_FLUSH_TIMEOUT_US check, we exit and drop the character if the flush doesn't happen for too long.

Seems like the same behavior could be achieved in usbjtag_tx_char_via_driver, by passing pdMS_TO_TICKS(TX_FLUSH_TIMEOUT_US / 1000) instead of portMAX_DELAY?

chipweinberger · 2022-11-18T23:49:37Z

This PR was reopened due to renaming the branch.

chipweinberger · 2022-11-23T09:13:41Z

@igrr bump.

igrr · 2022-12-08T07:19:10Z

components/driver/usb_serial_jtag.c

+
+    // try to send without blocking
+    if (xRingbufferSend(sjtag->tx_ring_buf, (void*) (src), size, 0)){
+       goto success;


Nitpick: 4 spaces of indentation, and a space before the opening curly brace on the line above.

(Please try to adjust the code so it looks like the rest of the code nearby.)

igrr · 2022-12-08T07:21:09Z

components/driver/usb_serial_jtag.c

+      if (xRingbufferSend(sjtag->tx_ring_buf, (void*) (src), size, ticks_to_wait)){
+         goto success;
+      } else {
+         sjtag->tx_blocking_attempts++;


Since this variable is only ever compared to zero, can it be a boolean?

it could, but I found this variable name easier to reason about. perhaps: bool tx_has_tried_blocking;? lmk what you think of that name

has_tried_blocking sounds understandable.

igrr · 2022-12-08T07:22:27Z

docs/en/api-guides/usb-serial-jtag-console.rst

@@ -53,26 +53,26 @@ The USB Serial/JTAG Controller is able to put the {IDF_TARGET_NAME} into downloa
 Limitations
 ===========

-There are several limitations to the USB console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow.
+There are several limitations to the USB Serial/JTAG console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow.


I'm not sure if mentioning JTAG here is less confusing. The JTAG part seems to have nothing to do with the CDC console?

USB console to me sounds ambiguous. Does it also refer to the USB OTG CDC Console?

"USB Serial/JTAG Console" != "USB OTG CDC Console". No?

Technically they are both "CDC", just implemented using two different peripherals. I thought that the title of the document makes it clear which one we are talking about, but maybe someone would land into the middle of the document from a Google search.

Let's keep the changes you made. If i get disagreement about this during the internal review, i will let you know.

igrr · 2022-12-08T07:25:21Z

docs/en/api-guides/usb-serial-jtag-console.rst


-3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different if the ESP-IDF application does not listen for incoming bytes. An USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will block until the application reads the bytes. This can lead to a non-responsive looking terminal program.
+3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different. A USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will do a one-time wait of up to 50 milliseconds hoping for the terminal to read the bytes. This can add what appears to be a short "pause" in your ESP-IDF application.


I think the phrase "A USB-to-serial bridge chip will just send the bytes to a (not listening) chip," talks about the data transfer from PC to the ESP. The second part of the sentence now talks about the data transfer from ESP to PC.

I think the original sentence about PC terminal being unresponsive when the ESP doesn't "listen" is still valid here. You can expand the paragraph to explain the delay behavior for the transfer in the other direction.

Ah, got it. confusing. maybe I can improve the wording.

igrr · 2022-12-08T07:28:06Z

Just a few minor notes @chipweinberger, looks good otherwise!

keenanjohnson · 2022-12-09T02:45:26Z

+1 and thanks all for fixing this as it resolves some usb issues I have seen with the s3. Let me know if there is anything I can do to help push this along.

igrr · 2022-12-14T18:50:39Z

sha=e68d81e25518650c7c1e1f9f7cbb2ed26f4128cf

chipweinberger · 2023-01-26T20:03:04Z

@igrr, when will this land on master? I've never really understood what PR-Sync-Merge means or what happens from here.

chipweinberger · 2023-02-14T02:07:16Z

bump :)

Merges #10208

chipweinberger · 2023-02-24T23:17:23Z

Closing. Merged.

720b8d9

Merges #10208

chipweinberger mentioned this pull request Nov 18, 2022

[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8700) #10142

Closed

espressif-bot added the Status: Opened Issue is new label Nov 18, 2022

github-actions bot changed the title ~~[USB Serial/JTAG Driver] use time-limited blocking for TX~~ [USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) Nov 18, 2022

chipweinberger force-pushed the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch from fa12dd3 to de6fb9f Compare November 23, 2022 09:12

chipweinberger force-pushed the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch from de6fb9f to b21b1df Compare November 24, 2022 04:48

espressif-bot assigned igrr Dec 7, 2022

chipweinberger mentioned this pull request Dec 8, 2022

ESP32-S3+CONFIG_CONSOLE_USB_SERIAL_JTAG: System hangs unless host serial port is opened (IDFGH-8906) #10324

Open

3 tasks

igrr requested changes Dec 8, 2022

View reviewed changes

[USB Serial/JTAG Driver] use time-limited blocking for TX

e68d81e

chipweinberger force-pushed the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch from b21b1df to e68d81e Compare December 8, 2022 08:13

igrr approved these changes Dec 14, 2022

View reviewed changes

igrr added the PR-Sync-Merge Pull request sync as merge commit label Dec 14, 2022

This was referenced Jan 2, 2023

serial output using ESP32-S3 usb_serial_jtag controller is unreliable (IDFGH-7781) #9318

Closed

[v4.4 branch] [Serial JTAG console] calling esp_vfs_usb_serial_jtag_use_driver() causes device to freeze (IDFGH-8411) #9877

Closed

espressif-bot assigned mythbuster5 and unassigned igrr Feb 24, 2023

espressif-bot pushed a commit that referenced this pull request Feb 24, 2023

usb_serial_jtag: Fix bug of blocking TX xfer when using driver,

720b8d9

Merges #10208

chipweinberger closed this Feb 24, 2023

espressif-bot pushed a commit that referenced this pull request Mar 31, 2023

usb_serial_jtag: Fix bug of blocking TX xfer when using driver,

8e3b4f6

Merges #10208

espressif-bot pushed a commit that referenced this pull request Apr 22, 2023

usb_serial_jtag: Fix bug of blocking TX xfer when using driver,

897082d

Merges #10208

chipweinberger deleted the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch May 20, 2023 10:50

chipweinberger restored the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch May 20, 2023 10:50

chipweinberger deleted the user/chip/usb-serial-jtag-time-limited-blocking-tx-try-2 branch May 20, 2023 10:50

chipweinberger mentioned this pull request Jun 9, 2023

[Console] 🚀🚀 Proposal: Console Needs More Flexibility 🚀🚀 (IDFGH-8532) #9986

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208

[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208

chipweinberger commented Nov 18, 2022 •

edited

Loading

chipweinberger commented Nov 18, 2022 •

edited

Loading

chipweinberger commented Nov 23, 2022

igrr Dec 8, 2022

igrr Dec 8, 2022

chipweinberger Dec 8, 2022 •

edited

Loading

igrr Dec 8, 2022

igrr Dec 8, 2022

chipweinberger Dec 8, 2022

igrr Dec 8, 2022

igrr Dec 8, 2022

chipweinberger Dec 8, 2022

igrr commented Dec 8, 2022

keenanjohnson commented Dec 9, 2022

igrr commented Dec 14, 2022

chipweinberger commented Jan 26, 2023

chipweinberger commented Feb 14, 2023

chipweinberger commented Feb 24, 2023


		3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different if the ESP-IDF application does not listen for incoming bytes. An USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will block until the application reads the bytes. This can lead to a non-responsive looking terminal program.
		3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different. A USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will do a one-time wait of up to 50 milliseconds hoping for the terminal to read the bytes. This can add what appears to be a short "pause" in your ESP-IDF application.

[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208

[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208

Conversation

chipweinberger commented Nov 18, 2022 • edited Loading

chipweinberger commented Nov 18, 2022 • edited Loading

chipweinberger commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipweinberger Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrr commented Dec 8, 2022

keenanjohnson commented Dec 9, 2022

igrr commented Dec 14, 2022

chipweinberger commented Jan 26, 2023

chipweinberger commented Feb 14, 2023

chipweinberger commented Feb 24, 2023

chipweinberger commented Nov 18, 2022 •

edited

Loading

chipweinberger commented Nov 18, 2022 •

edited

Loading

chipweinberger Dec 8, 2022 •

edited

Loading