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

USB CDC detect dis-/connect Events (IDFGH-6065) #7747

Closed
moefear85 opened this issue Oct 21, 2021 · 10 comments
Closed

USB CDC detect dis-/connect Events (IDFGH-6065) #7747

moefear85 opened this issue Oct 21, 2021 · 10 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF

Comments

@moefear85
Copy link

moefear85 commented Oct 21, 2021

Currently, there are are no dis-/connect events for usb cdc. Checking DTR/RTS is not a reliable way to detect usb dis-/connects, since many tools, including esptool.py, make non-standard use of them. This means there is no definite, unambiguous way to differentiate a dtr/rts toggle from a dis-/connect. Hence any call to tinyusb_cdcacm_write_flush(...) while the USB is disconnected, silently hangs. Using a timeout isn't a reliable solution, because it still causes hangs until the timeout is triggered, which would require very small values for it, but there are cases where larger timeouts are necessary under the assumption that the usb is still connected. Hence timeouts are also not a reliable/unambiguous way to detect USB disconnects.

Optionally, an additional esp-idf function that when called, returns a value that indicates USB connection status, would be a good idea.

@moefear85 moefear85 added the Type: Feature Request Feature request for IDF label Oct 21, 2021
@moefear85 moefear85 changed the title USB CDC detect dis-/connects USB CDC detect dis-/connect Events Oct 21, 2021
@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 21, 2021
@github-actions github-actions bot changed the title USB CDC detect dis-/connect Events USB CDC detect dis-/connect Events (IDFGH-6065) Oct 21, 2021
@igrr
Copy link
Member

igrr commented Oct 21, 2021

Linking the related upstream issue: hathach/tinyusb#482

@moefear85
Copy link
Author

TinyUSB does provide disconnect events for USB... the issue is simply that esp additions to CDC-ACM don't pass these on. One can manually register for those events though, look into components/tinyusb/tinyusb/usbd.c.

@dreamtrek
Copy link

got the same issue. I found unplugging event in usbd.c, but it never trigged.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jun 1, 2022
@tore-espressif
Copy link
Collaborator

Hello @moefear85 @dreamtrek ,

"Device Connected" event is implemented an you can use in two ways:

  1. Defining void tud_mount_cb(void); function.This function will be called when ESP32Sx is connected to USB Host
  2. Calling bool tud_connected(void); that will return current connection status

For "Device Disconnected" event it is a little more complicated: USB mandates self-powered devices (this is your case) to monitor USB bus voltage. If you don't have USB V_BUS connected to ESP, you will never get the disconnection event.

As a workaround you can define void tud_suspend_cb(bool remote_wakeup_en); that will be called when self-powered device without V_BUS sensing is disconnected from USB Host.

I will leave this issue open as we need to provide an example of USB device with V_BUS sensing

@moefear85
Copy link
Author

moefear85 commented Jun 1, 2022

@tore-espressif

I understand detecting disconnect events is relatively complicated if one wants to strictly adhere to the standard. But I already discovered long ago that tinyusb actually properly detects these events as it logs them anyways, even without monitoring any lines, except the event isn't officially exposed. That's the point I made above. It would be useful though if esp-idf would expose it officially. I forgot now the name of the relevant event, but later I can patch it and create a PR. Right now the bugs in the uart subsys are more important. idf5 seems to be silently inserting BREAKs or malformed frames where they don't belong, which is inhibiting target chips from responding to any flashing attempts over an esp-based usb-uart-bridge.

@tore-espressif
Copy link
Collaborator

tore-espressif commented Jun 1, 2022

I don't understand what you mean by 'officially exposed`. The TinyUSB API is fully exposed, you can use it. (But it doesn't work without V_BUS sensing on ESP. The line monitoring is done by the USB-OTG peripheral, not TinyUSB)

Right now the bugs in the uart subsys are more important

We can discuss UART in different thread.

@moefear85
Copy link
Author

I mean that there is no explicit callback function that one can assign for the event. It is consumed fully internally by tinyusb. ofcourse anyone can modify any part of the code, even private includes, but these aren't proper solutions. to be fully proper, such callbacks for disconnects would even have to be locatable in the documentation. but they aren't at the moment, because the event exists, but not a method to assign a callback like there is for CDC line coding and rx events.

@tore-espressif
Copy link
Collaborator

Agreed, two forms of callbacks is definitely misleading.

We plan to refactor the whole USB device side, I'll keep this in mind, thanks again!

@moefear85
Copy link
Author

moefear85 commented Jun 1, 2022

Upon closer inspection of usbd.c, and seeing the line 583:

if (tud_suspend_cb) tud_suspend_cb(...);

I realize why defining tud_suspend_cb works as a solution. Further testing also revealed DCD_EVENT_UNPLUGGED is never triggered because of reasons you mentioned above. The first time I looked at the code in the past, I just assumed since they are there, they get triggered. I see now this assumption was false. thx for the tip. Still, recieving DCD_EVENT_SUSPEND and DCD_EVENT_BUS_RESET are close and useful enough. But there is no assignable callback for it, so something like:

if (tud_reset_cb) tud_reset_cb(...);

would also be useful. Also, as a suggestion for the future when unifying the callback system, wouldn't the default event loop be ideal for this purpose? thx anyways. I'll close this for now since it contains enough information for others to move forward and prevent unwanted blocking when the usb cable is unplugged.

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Oct 18, 2022
@wuyuanyi135
Copy link
Contributor

I was investigating the same thing these days. I found checking tud_cdc_n_connected reflects the correct state of whether the CDC port has been opened or not.

espressif-bot pushed a commit that referenced this issue Oct 20, 2022
VBUS voltage monitoring is mandated by USB specification for self-powered devices.
This implementation maps selected GPIO to bvalid signal of USB-OTG peripheral.

Closes #7747
espressif-bot pushed a commit that referenced this issue Nov 3, 2022
VBUS voltage monitoring is mandated by USB specification for self-powered devices.
This implementation maps selected GPIO to bvalid signal of USB-OTG peripheral.

Closes #7747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF
Projects
None yet
Development

No branches or pull requests

7 participants