-
Notifications
You must be signed in to change notification settings - Fork 37
feature(tinyusb): Software VBUS monitoring feature on ESP32P4 (part1/2) (IEC-437) #305
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
base: master
Are you sure you want to change the base?
Conversation
c8dac7f to
ed53b4e
Compare
a2cf813 to
649edbe
Compare
ed53b4e to
888ae35
Compare
888ae35 to
03caa71
Compare
03caa71 to
f439723
Compare
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.
Pull Request Overview
This PR introduces the first part of a software VBUS monitoring feature for ESP32P4, specifically targeting USB OTG 2.0 high-speed functionality. It addresses the limitation where ESP32P4's USB OTG 2.0 doesn't have hardware detection of VBUS BVALID signal by implementing a software-based solution with configurable debouncing.
- Added VBUS monitor API infrastructure with debounce configuration
- Refactored test application from
dconn_detectiontovbus_monitorwith comprehensive test coverage - Updated documentation and configuration to support the new VBUS monitoring feature
Reviewed Changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tinyusb_vbus_monitor.c | New VBUS monitoring implementation (currently stub returning ESP_ERR_NOT_SUPPORTED) |
| tinyusb_task.c | Integration of VBUS monitor initialization and cleanup into TinyUSB task lifecycle |
| tinyusb.c | Added VBUS monitor configuration validation and setup for different USB OTG versions |
| test_apps/vbus_monitor/* | Comprehensive test suite replacing dconn_detection with extensive VBUS monitoring tests |
| include files | Added VBUS monitor configuration structures and API declarations |
| README.md | Documentation updates explaining VBUS monitoring setup and ESP32P4 limitations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
device/esp_tinyusb/test_apps/vbus_monitor/main/test_vbus_monitor.c
Outdated
Show resolved
Hide resolved
device/esp_tinyusb/test_apps/vbus_monitor/main/test_vbus_monitor.c
Outdated
Show resolved
Hide resolved
f439723 to
fee7783
Compare
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.
Pull Request Overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
device/esp_tinyusb/test_apps/vbus_monitor/main/test_vbus_monitor.c
Outdated
Show resolved
Hide resolved
device/esp_tinyusb/test_apps/vbus_monitor/main/test_vbus_monitor.c
Outdated
Show resolved
Hide resolved
fee7783 to
33acda8
Compare
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.
Pull Request Overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
device/esp_tinyusb/tinyusb.c:1
- Line 106 has incorrect condition. It should check
config->phy.vbus_monitor_io >= GPIO_NUM_0 && config->phy.vbus_monitor_io < GPIO_NUM_MAXinstead of justconfig->phy.self_powered.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f573418 to
a8fbded
Compare
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.
Bug: Memory leak and state corruption in task start error path
Memory leak and state corruption in error handling path of tinyusb_task_start(). If task creation succeeds but the task fails to notify within the timeout (line 177-180), the error path at line 180-182 frees task_ctx but does not reset the _task_is_running flag. This has two problems:
-
The _task_is_running flag remains true, preventing any future calls to tinyusb_task_start() from succeeding (they will fail with ESP_ERR_INVALID_STATE at line 138).
-
If the task eventually starts and successfully calls xTaskNotifyGive() after the timeout, it will continue running with a freed task_ctx pointer that was already freed at line 181. While the current implementation doesn't access task_ctx after notification, this creates a dangerous dangling pointer situation.
The error path should reset _task_is_running to false within a critical section before freeing task_ctx.
device/esp_tinyusb/tinyusb_task.c#L179-L182
esp-usb/device/esp_tinyusb/tinyusb_task.c
Lines 179 to 182 in 057986d
| ESP_LOGE(TAG, "Task wasn't able to start TinyUSB stack"); | |
| ret = ESP_ERR_TIMEOUT; | |
| goto err; | |
| } |
057986d to
d311914
Compare
d311914 to
623809e
Compare
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.
Bug: Dangling _task_is_running Flag on Error Path
Inconsistent state on error: When the error path at label err: is reached (either from task creation failure at line 169 or timeout at line 180), the function frees task_ctx and returns, but does not reset the _task_is_running flag to false. This flag was set to true at line 140, but will remain set even though the task was never successfully started. This prevents subsequent calls to tinyusb_task_start from succeeding. The fix should reset _task_is_running to false within a critical section before freeing memory and returning.
device/esp_tinyusb/tinyusb_task.c#L185-L188
esp-usb/device/esp_tinyusb/tinyusb_task.c
Lines 185 to 188 in 623809e
| err: | |
| heap_caps_free(task_ctx); | |
| return ret; |
623809e to
d298a66
Compare
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.
Bug: Task Running Flag Not Reset on Error Path
The error path at label err: does not reset _task_is_running flag before returning. This error path is reached when task creation fails (line 176) or when the notification times out (line 183). Since _task_is_running was set to true at line 140, it must be cleared in the error path to prevent the system from being left in an inconsistent state where subsequent calls to tinyusb_task_start() will fail at the check on line 139.
device/esp_tinyusb/tinyusb_task.c#L185-L188
esp-usb/device/esp_tinyusb/tinyusb_task.c
Lines 185 to 188 in d298a66
| err: | |
| heap_caps_free(task_ctx); | |
| return ret; |
75b25da to
97354e1
Compare
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.
Pull Request Overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
device/esp_tinyusb/tinyusb.c:1
- The error message is misleading. The condition checks if
self_poweredis false, meaning the error triggers when the device is not self-powered, but the message suggests VBUS monitoring isn't supported. The message should clarify that VBUS monitoring GPIO must be valid whenself_poweredis true, e.g., 'VBUS monitor GPIO must be a valid GPIO number when self_powered is true'.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
97354e1 to
15fb575
Compare
|
Hi @igi540 This PR is ready. PTAL. |
| */ | ||
| static void test_vbus_monitor_control_signal_connect(void) | ||
| { | ||
| esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ONE_INPUT, USB_BVALID_PAD_IN_IDX, false); |
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.
Just out of curiosity: Isn't there any public api avaliable for this, instead of calling rom code?
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.
Would you mind adding some details, how to run the test app? I wanted to try it, but the very first test case failed immediately so I think I am missing some steps. Especially as you mentioned that we can have some special setup in CI, to be able to run this tests.
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.
Which case and what target?
Please, keep in mind the PR description:
refactored test app dconn_detection -> vbus_monitor, enabled for esp32s2
The goal of this PR is to enable the vbus_monitor test on S2 in the current workflow and runners setup (because before we even didn't run it), which is successfully done in the current CI check in this PR (Only emulated test case scenario):
https://github.com/espressif/esp-usb/actions/runs/19167023528/job/54796062942?pr=305#step:7:2316
If you want to run it on ESP32-P4 or to use some other specific test cases, which require the hardware modifications, you have to take some actions.
I added a description of what might be done in the test flow, for example:
2025-11-07 12:26:49 Running Emulated VBUS, verify attach/detach events callback (via Bvalid signal)...
2316
2025-11-07 12:26:49 VBUS monitor control setup:
2317
2025-11-07 12:26:49 - might be triggered by signal multiplexing.
2318
2025-11-07 12:26:49 - might be triggered by manipulating the GOTGCTL register.
2319
2025-11-07 12:26:49 - might be triggered by GPIO5.
2320
2025-11-07 12:26:49 To use this, connect GPIO5 to VBUS monitor GPIO4.
The manual test case Real VBUS USB OTG 2.0, verify attach/detach events callback (requires manual handling) has the description in the code (refer the https://github.com/espressif/esp-usb/pull/305/files#diff-8efc37da57c45c275eb64a1473b2f2ca2de2d2e31ddd1b3acd7e2413e99eeb0dR348):
* Note: this test requires external hardware (resistor divider or similar) to connect VBUS to the VBUS monitor GPIO
* and an operator, who will manually connect/disconnect the device from the USB Host port according the prompts
But if this is not enough, let me know.
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.
Sorry, I missed the description that this PR enables tests only for s2, I was using p4. Will try to run with s2
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.
Hi @peter-marcisovsky ,
any luck?
| #define TEST_BVALID_VIA_SIGNAL_MUX_ENABLED 1 | ||
|
|
||
| #else | ||
| #define TEST_BVALID_VIA_SIGNAL_MUX_ENABLED 0 |
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.
For which target is this going to be defined?
At the beginning of the file, you set SOC_USB_OTG_SUPPORTED and then listed all USB OTG capable targets in #if, thus is #else is never executed.
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.
This is for any other target in the future that I am not able to test right now.
I tested it with S2 (in CI), S3 (loclally) and P4 (locally and in CI in the follow-up PRs).
Assuming that H4 has the same controller it should be possible to run the test with multiplexing.
If we add any other target in the future, we need to:
- Verify that the multiplexing is possible
- Add the idf target to the list
Otherwise, we won't have this option in the test case scenarios.
The test case Emulated VBUS USB OTG 1.1, verify attach/detach events callback (via GOTGCTL register) isn't relevant to the current logic, as we don't use it in the final VBUS monitoring feature for USB OTG 1.1.
But, this test shows the difference between USB OTG 1.1 and USB OTG 2.0 (relevant for ESP32-P4, ECO4, when both options are possible). And it is also applicable to the ESP32-S2/S3, because we have there the same 1.1 controller.
This is an extra case, that might help with:
- Bringing up new hardware
- Helping to detect differences between controllers in different CPUs
- Testing the potential feature replacement, if we will force any changes in the hardware in the future (for example: GPIO multiplexing won't be present and we need to migrate all targets to software implementation)
|
thanks for the review! This PR is exactly as it was described in the memo:
And let me know if you have any other blocking notes. I will rebase the PR soon, because it has the conflicts in CHANGELOG.md (release of esp_tinyusb v2.0.1~1). |
tore-espressif
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.
Please wait with merging, I'm still working on the review :)
| esp_err_t tinyusb_task_start(tinyusb_port_t port, | ||
| const tinyusb_task_config_t *task_cfg, | ||
| const tinyusb_desc_config_t *desc_cfg, | ||
| const tinyusb_vbus_monitor_config_t *vbus_monitor_cfg); |
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.
Not for myself: Why does tinyusb task need vbus_monitor_cfg?
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.
Because we will start the VBUS monitor feature inside the dedicated task.
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.
Why do we start it from the dedicated task though? Is there any advantage I'm missing?
From what I can see we can init VBUS monitoring simply from tinyusb_driver_install(). Instead of passing it
tinyusb_driver_install -> tinyusb_task_start -> xTaskCreatePinnedToCore -> tinyusb_device_task
It would also decouple VBUS monitoring from the task, as they are 2 separate things, aren't they?
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.
Yes, there are couple of things that are not obvious from the first glance:
-
Making the VBUS monitor feature fully separate (rejected, as it creates the conflict with the access to GOTGCTL register). For ECO4 we need to drive the GOTGCTL register which is also might be changed in the
tusb_init(). So, enable the VBUS before thetusb_init()might not work, as the register will be modified in TinyUSB. Thus, we need to enable the feature after thetusb_init()call to control the state of GOTGCTL register and might be able to change it inesp_tinyusb, if necessary. -
Enable the feature after creating the
tusb_init()which is called after the dedicated task creation (rejected, as it creates the additional not-necessary logic in vbus monitor init error path). We can get the error during the VBUS monitoring initialization. In this way, we need to stop and delete the task and return the error for the upper layer. There is a dependency of the task from the feature -> the decision to move the feature inside the dedicated task was made. The same way as with descriptors config, which also should be available all the time, whiletud_task()is working. -
Including the vbus monitor configuration in the task config (rejected, as it creates the duplicate configuration in public structs). This will break the logic with ESP32-S2/S3 where we apply this config during the PHY initialization. Also, task config is public, so the change in there will add the same variables as we have in PHY-related config. Thus, the decision to keep the config in PHY and share it with dedicated task by adding another argument in the internal function was made.
Right now, there are three arguments, that we pass to the dedicated task in the non-publicly available call: tinyusb_task_start(). If we want to simplify the process and not to forget anything, it might be simplified to the additional struct with refactoring, which doesn't break anything.
…L override driving for VBUS monitoring - Added test cases for USB OTG 1.1 (esp32s2, esp32s3, esp32p4) - Added test cases for USB OTG 2.0 (esp32p4) - Enabled CI tests for esp32s2
15fb575 to
4183962
Compare
Description
VBUS monitoring on ESP32-P4 - Part 1/2
dconn_detection->vbus_monitor, enabled for esp32s2pinned the TinyUSB version to v0.18 (Solved by releaseversion: '^0.18.0')v0.19.0~2of espressif/tinyusbRelated
Testing
ESP32S2/S3
ESP32P4
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Adds software VBUS monitoring for ESP32P4 high‑speed USB with debounce, updates docs/tests, and pins TinyUSB to v0.18.
tinyusb_vbus_monitor.c(+ header) and config fieldtinyusb_phy_config_t.vbus_monitor_debounce_mswith defaultTINYUSB_DEFAULT_DEBOUNCE_MS.tinyusb.cvalidates self‑powered VBUS GPIO/debounce, prepares VBUS monitor config, and passes it totinyusb_task_start();tinyusb_task.cinitializes/deinitializes VBUS monitor (new param intinyusb_task_start()and context).CMakeLists.txt: includetinyusb_vbus_monitor.conly for targetesp32p4.tinyusb_task_start()gainsconst tinyusb_vbus_monitor_config_t *vbus_monitor_cfgparameter; new private headerinclude_private/tinyusb_vbus_monitor.h.README.md: instructions for ESP32‑P4 HS VBUS monitoring (debounce, GPIO ISR requirement, examples).CHANGELOG.md: note VBUS monitoring feature.idf_component.yml: pintinyusbto^0.18.0.dconn_detectionwith newvbus_monitortest app; add multiple attach/detach scenarios (signal mux, GOTGCTL, GPIO/manual), pytest wrapper and configs.Written by Cursor Bugbot for commit 15fb575. This will update automatically on new commits. Configure here.