Conversation
WalkthroughThe changes introduce a new USB CDC ACM device example using TinyUSB, replacing the previous Changes
Sequence Diagram(s)sequenceDiagram
participant Host (PC)
participant USB Device (CDC ACM Example)
participant TinyUSB Stack
participant FreeRTOS
Host (PC)->>USB Device (CDC ACM Example): Connect USB cable
USB Device (CDC ACM Example)->>TinyUSB Stack: Initialize USB PHY and TinyUSB
TinyUSB Stack->>USB Device (CDC ACM Example): Device mount callback
USB Device (CDC ACM Example)->>FreeRTOS: Create USB and CDC tasks
FreeRTOS->>TinyUSB Stack: Run device and CDC tasks
Host (PC)->>TinyUSB Stack: Send data over CDC interface
TinyUSB Stack->>USB Device (CDC ACM Example): Notify data received
USB Device (CDC ACM Example)->>TinyUSB Stack: Echo data back to host
TinyUSB Stack->>Host (PC): Send echoed data
Estimated code review effort2 (~20 minutes) Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
examples/usb/device/cdc_acm/README.md (1)
1-3: Minor wording & spacing polish.-# USB DEVICE CDC ACM - -This example demonstrates how to develop CDC ACM based on native TinyUSB. After compilation, you will see a COM port on Windows. When you send data using a serial port tool, you will receive the exact same data back. +# USB Device – CDC ACM + +This example shows how to build a CDC-ACM device with native TinyUSB. +After flashing, a virtual COM port appears (Windows) or `/dev/ttyACM*` on Linux/macOS. +Any data sent to the port is echoed back verbatim.Re-phrasing removes “exact same” and mentions non-Windows hosts.
examples/usb/device/cdc_acm/main/CMakeLists.txt (1)
6-10: Edge-case: both local and managed TinyUSB present.If a user keeps a local
components/tinyusband listsespressif/tinyusbindep.manifest, both names appear inBUILD_COMPONENTS.
The current logic will silently pick the local one; this may mask version mismatches.-if(tinyusb IN_LIST build_components) - set(tinyusb_name tinyusb) -else() - set(tinyusb_name espressif__tinyusb) -endif() +# Prefer managed component when both exist to guarantee declared version. +if(espressif__tinyusb IN_LIST build_components) + set(tinyusb_name espressif__tinyusb) +elseif(tinyusb IN_LIST build_components) + set(tinyusb_name tinyusb) +else() + message(FATAL_ERROR "TinyUSB component not found") +endif()examples/usb/device/cdc_acm/main/cdc_acm.c (3)
50-64: Consider improving CDC task robustness.The CDC task implementation works but could benefit from better buffer management and error handling.
void tud_cdc_task(void *arg) { while (1) { if (tud_cdc_available()) { - uint8_t buf[64]; + uint8_t buf[CFG_TUD_CDC_RX_BUFSIZE]; // Use config-defined buffer size int len = tud_cdc_read(buf, sizeof(buf)); if (len > 0) { ESP_LOGI(TAG, "Received %d bytes", len); - tud_cdc_write(buf, len); - tud_cdc_write_flush(); + if (tud_cdc_write(buf, len) == len) { + tud_cdc_write_flush(); + } else { + ESP_LOGW(TAG, "Failed to write all data back"); + } } } vTaskDelay(pdMS_TO_TICKS(1)); } }
66-69: Consider implementing CDC receive callback.The
tud_cdc_rx_cbfunction is empty. While this might be intentional for a simple echo example, consider if any initialization or state management is needed.void tud_cdc_rx_cb(uint8_t itf) { (void) itf; + // CDC receive callback - could be used for flow control or buffering }
71-80: Replace magic numbers with named constants.The task stack sizes and priorities are hard-coded. Consider defining these as constants for better maintainability.
+#define TUSB_DEVICE_TASK_STACK_SIZE (4096) +#define CDC_TASK_STACK_SIZE (5 * 1024) +#define TASK_PRIORITY (5) + void app_main(void) { usb_phy_init(); if (!tusb_init()) { ESP_LOGE(TAG, "TinyUSB init failed"); return; } - xTaskCreate(tusb_device_task, "tusb_device_task", 4096, NULL, 5, NULL); - xTaskCreate(tud_cdc_task, "tud_cdc_task", 5 * 1024, NULL, 5, NULL); + xTaskCreate(tusb_device_task, "tusb_device_task", TUSB_DEVICE_TASK_STACK_SIZE, NULL, TASK_PRIORITY, NULL); + xTaskCreate(tud_cdc_task, "tud_cdc_task", CDC_TASK_STACK_SIZE, NULL, TASK_PRIORITY, NULL); }examples/usb/device/cdc_acm/main/tusb/usb_descriptors.c (2)
7-29: Fix incorrect comment about Video IAD.The comment on line 12-13 mentions "Interface Association Descriptor (IAD) for Video" which is incorrect for a CDC device. This is misleading and should be corrected.
- // Use Interface Association Descriptor (IAD) for Video - // As required by USB Specs IAD's subclass must be common class (2) and protocol must be IAD (1) + // CDC device class .bDeviceClass = TUSB_CLASS_CDC, .bDeviceSubClass = TUSB_CLASS_UNSPECIFIED, .bDeviceProtocol = TUSB_CLASS_UNSPECIFIED,
70-75: Consider using chip-specific serial number.The serial number is hard-coded as "123456". For production devices, consider using the chip's unique ID.
char const *string_desc_arr [] = { (const char[]) { 0x09, 0x04 }, // 0: is supported language is English (0x0409) "TinyUSB", // 1: Manufacturer "TinyUSB Device", // 2: Product - "123456", // 3: Serials, should use chip ID + "123456", // 3: Serials, TODO: use chip ID };examples/usb/device/cdc_acm/main/tusb/tusb_config.h (1)
102-103: Consider using more descriptive USB identifiers.The USB VID (0xcafe) and PID (0x4001) appear to be development/test values. Ensure these are appropriate for the intended use case.
For production use, consider registering proper USB identifiers or using ESP32-specific VID/PID pairs if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
examples/usb/device/cdc_acm/CMakeLists.txt(1 hunks)examples/usb/device/cdc_acm/README.md(1 hunks)examples/usb/device/cdc_acm/main/CMakeLists.txt(1 hunks)examples/usb/device/cdc_acm/main/cdc_acm.c(1 hunks)examples/usb/device/cdc_acm/main/idf_component.yml(2 hunks)examples/usb/device/cdc_acm/main/tusb/tusb_config.h(1 hunks)examples/usb/device/cdc_acm/main/tusb/usb_descriptors.c(1 hunks)examples/usb/device/cdc_acm/sdkconfig.defaults(1 hunks)examples/usb/device/cdc_device/.gitignore(0 hunks)examples/usb/device/cdc_device/main/CMakeLists.txt(0 hunks)examples/usb/device/cdc_device/main/cdc/cdc_config.c(0 hunks)examples/usb/device/cdc_device/main/cdc/include/cdc_config.h(0 hunks)examples/usb/device/cdc_device/main/main.c(0 hunks)examples/usb/device/cdc_device/sdkconfig.defaults(0 hunks)
💤 Files with no reviewable changes (6)
- examples/usb/device/cdc_device/main/CMakeLists.txt
- examples/usb/device/cdc_device/sdkconfig.defaults
- examples/usb/device/cdc_device/main/cdc/include/cdc_config.h
- examples/usb/device/cdc_device/main/main.c
- examples/usb/device/cdc_device/.gitignore
- examples/usb/device/cdc_device/main/cdc/cdc_config.c
🧰 Additional context used
🪛 LanguageTool
examples/usb/device/cdc_acm/README.md
[grammar] ~1-~1: Use proper spacing conventions.
Context: # USB DEVICE CDC ACM This example demonstrates how to develop...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[style] ~3-~3: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... serial port tool, you will receive the exact same data back.
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[grammar] ~3-~3: Use proper spacing conventions.
Context: ...u will receive the exact same data back.
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
examples/usb/device/cdc_acm/CMakeLists.txt (1)
6-6: Project renamed correctly – no action needed.
project(cdc_acm)aligns with the new folder name and avoids build-time confusion.examples/usb/device/cdc_acm/sdkconfig.defaults (1)
4-4: Confirm 1 kHz tick rate is intentional.
CONFIG_FREERTOS_HZ=1000bumps the RTOS tick from the ESP-IDF default (commonly 100 Hz).
Higher rates improve USB polling latency but increase CPU wake-ups and power consumption. Make sure this trade-off is acceptable for the target board and power budget.examples/usb/device/cdc_acm/main/idf_component.yml (1)
5-5: Double-check version strings.
- IDF version
'>=4.1.0'is fine, but you can tighten the upper bound if the example relies on 5.x-only APIs.- The TinyUSB spec
^0.18.0~2uses the Debian “revision” suffix (~2). Ensure the component registry actually publishes that tag; otherwise the resolver will fail.If in doubt, stick to plain semver:
espressif/tinyusb: "^0.18.0"Also applies to: 17-17
examples/usb/device/cdc_acm/main/cdc_acm.c (1)
29-48: USB lifecycle callbacks implementation looks good.The callback functions properly handle USB device state changes with appropriate logging. The parameter handling for unused variables is correct.
examples/usb/device/cdc_acm/main/tusb/usb_descriptors.c (3)
52-58: Configuration descriptor implementation looks correct.The configuration descriptor properly defines CDC control and data interfaces with appropriate endpoint configurations. The conditional high-speed endpoint size handling is well implemented.
79-114: String descriptor callback implementation is robust.The string descriptor callback properly handles UTF-16 conversion, bounds checking, and the special case for language ID. The 63-character limit prevents buffer overflow.
20-22: USB VID/PID definitions verified
USB_VID(0xcafe) andUSB_PID(0x4001) are defined inexamples/usb/device/cdc_acm/main/tusb/tusb_config.h(lines 102–103), matching their use inusb_descriptors.c.
No further action required.examples/usb/device/cdc_acm/main/tusb/tusb_config.h (5)
1-32: License header and includes are appropriate.The MIT license header is correct and the necessary includes are present with proper C++ guards.
38-44: USB speed configuration is well implemented.The conditional compilation for high-speed vs full-speed USB is correctly implemented using the SDK configuration.
50-53: MCU type validation is essential.The compile-time check for
CFG_TUSB_MCUensures proper MCU configuration, which is critical for USB functionality.
105-110: Class configuration correctly enables only CDC.The configuration appropriately enables only the CDC class while disabling others (MSC, HID, MIDI, VENDOR), which is correct for this example.
112-114: Buffer size configuration is optimal.The CDC FIFO buffer sizes are appropriately configured based on USB speed capability, using 512 bytes for high-speed and 64 bytes for full-speed connections.
| target_sources(${tusb_lib} PUBLIC "${PROJECT_DIR}/main/tusb/usb_descriptors.c") | ||
|
|
||
| cmake_policy(SET CMP0079 NEW) | ||
| target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB}) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Link order may invert dependency.
target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB}) makes TinyUSB depend on your component, not the other way around.
If cdc_acm.c references TinyUSB symbols this is harmless, but if TinyUSB needs symbols from this component you’ll get undefined symbols late in the link stage. Validate with a clean build.
Alternative: let the current component link against TinyUSB instead:
-target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB})
+target_link_libraries(${COMPONENT_LIB} PRIVATE ${tusb_lib})🏁 Script executed:
#!/bin/bash
# Inspect the CMakeLists.txt to verify definitions and usage of tusb_lib and COMPONENT_LIB
rg -n "tusb_lib" examples/usb/device/cdc_acm/main/CMakeLists.txt
rg -n "COMPONENT_LIB" examples/usb/device/cdc_acm/main/CMakeLists.txt
sed -n '1,200p' examples/usb/device/cdc_acm/main/CMakeLists.txtLength of output: 1317
Reverse link order to avoid inverting dependency
Linking the TinyUSB component to your example makes TinyUSB depend on your component rather than the other way around. To ensure your CDC-ACM component pulls in TinyUSB (and not vice-versa), swap the arguments:
File: examples/usb/device/cdc_acm/main/CMakeLists.txt
Line: 17
- target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB})
+ target_link_libraries(${COMPONENT_LIB} PRIVATE ${tusb_lib})This way, your component library links against TinyUSB, avoiding any undefined‐symbol surprises at link time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB}) | |
| # examples/usb/device/cdc_acm/main/CMakeLists.txt (line 17) | |
| -target_link_libraries(${tusb_lib} PRIVATE ${COMPONENT_LIB}) | |
| +target_link_libraries(${COMPONENT_LIB} PRIVATE ${tusb_lib}) |
🤖 Prompt for AI Agents
In examples/usb/device/cdc_acm/main/CMakeLists.txt at line 17, the
target_link_libraries call has the arguments reversed, causing TinyUSB to depend
on your component instead of your component depending on TinyUSB. Fix this by
swapping the arguments so that your component library links against the TinyUSB
library, ensuring correct dependency direction and preventing undefined symbol
errors during linking.
| static void tusb_device_task(void *arg) | ||
| { | ||
| while (1) { | ||
| tud_task(); // TinyUSB device task | ||
| } | ||
| } |
There was a problem hiding this comment.
Add task delay to prevent CPU starvation.
The infinite loop in tusb_device_task runs without any delay, which could starve other FreeRTOS tasks and cause poor system performance.
static void tusb_device_task(void *arg)
{
while (1) {
tud_task(); // TinyUSB device task
+ vTaskDelay(pdMS_TO_TICKS(1)); // Small delay to prevent CPU starvation
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void tusb_device_task(void *arg) | |
| { | |
| while (1) { | |
| tud_task(); // TinyUSB device task | |
| } | |
| } | |
| static void tusb_device_task(void *arg) | |
| { | |
| while (1) { | |
| tud_task(); // TinyUSB device task | |
| vTaskDelay(pdMS_TO_TICKS(1)); // Small delay to prevent CPU starvation | |
| } | |
| } |
🤖 Prompt for AI Agents
In examples/usb/device/cdc_acm/main/cdc_acm.c around lines 22 to 27, the
infinite loop in tusb_device_task lacks any delay, causing potential CPU
starvation of other FreeRTOS tasks. Add a small delay, such as vTaskDelay with
an appropriate tick count, inside the while loop to yield CPU time and allow
other tasks to run smoothly.
There was a problem hiding this comment.
@YanKE01 I understand, I'll respect your decision on this. You may have specific reasons for keeping the task without delay.
(´･ω･`)
| static void usb_phy_init(void) | ||
| { | ||
| usb_phy_handle_t phy_hdl; | ||
| usb_phy_config_t phy_conf = { | ||
| .controller = USB_PHY_CTRL_OTG, | ||
| .otg_mode = USB_OTG_MODE_DEVICE, | ||
| .target = USB_PHY_TARGET_INT | ||
| }; | ||
| usb_new_phy(&phy_conf, &phy_hdl); | ||
| } |
There was a problem hiding this comment.
Add error handling for USB PHY initialization.
The usb_new_phy function can fail, but the return value is not checked. This could lead to undefined behavior if PHY initialization fails.
static void usb_phy_init(void)
{
usb_phy_handle_t phy_hdl;
usb_phy_config_t phy_conf = {
.controller = USB_PHY_CTRL_OTG,
.otg_mode = USB_OTG_MODE_DEVICE,
.target = USB_PHY_TARGET_INT
};
- usb_new_phy(&phy_conf, &phy_hdl);
+ esp_err_t ret = usb_new_phy(&phy_conf, &phy_hdl);
+ if (ret != ESP_OK) {
+ ESP_LOGE(TAG, "Failed to initialize USB PHY: %s", esp_err_to_name(ret));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void usb_phy_init(void) | |
| { | |
| usb_phy_handle_t phy_hdl; | |
| usb_phy_config_t phy_conf = { | |
| .controller = USB_PHY_CTRL_OTG, | |
| .otg_mode = USB_OTG_MODE_DEVICE, | |
| .target = USB_PHY_TARGET_INT | |
| }; | |
| usb_new_phy(&phy_conf, &phy_hdl); | |
| } | |
| static void usb_phy_init(void) | |
| { | |
| usb_phy_handle_t phy_hdl; | |
| usb_phy_config_t phy_conf = { | |
| .controller = USB_PHY_CTRL_OTG, | |
| .otg_mode = USB_OTG_MODE_DEVICE, | |
| .target = USB_PHY_TARGET_INT | |
| }; | |
| esp_err_t ret = usb_new_phy(&phy_conf, &phy_hdl); | |
| if (ret != ESP_OK) { | |
| ESP_LOGE(TAG, "Failed to initialize USB PHY: %s", esp_err_to_name(ret)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In examples/usb/device/cdc_acm/main/cdc_acm.c around lines 11 to 20, the
function usb_new_phy is called without checking its return value, which can lead
to undefined behavior if initialization fails. Modify the code to capture the
return value of usb_new_phy, check if it indicates an error, and handle the
error appropriately, such as logging the failure and preventing further
execution or cleanup as needed.
YanKE01
force-pushed
the
feat/add_usbd_cdc_acm
branch
from
a5601c4 to
22c47bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
examples/usb/device/cdc_acm/README.md (2)
3-3: Avoid filler wording“exact same” is redundant.
-...you will receive the exact same data back. +...you will receive the same data back.
1-4: README is too terse—add build & usage instructionsConsider extending the README with:
• How to build (
idf.py set-target …,idf.py build) and flash the example.
• Expected serial parameters (baud-rate, flow control).
• Notes for Linux/macOS users (e.g.,/dev/ttyACM*device, udev rules) so it isn’t Windows-only.This will make the example immediately usable without hunting for extra information.
build_config.yml (1)
154-156: Validate USB-host example support on ESP32-S3
examples/usb/host/msc_hosttargetsesp32s3.
USB-host mode needs the internal PHY and (on S3) an external 5-V VBUS source. If the example assumes hardware not available on all S3 dev-kits, CI may still pass but users will hit a runtime fault.Consider adding a short README note inside the example (or a kconfig prompt) that lists the required board/voltage jumper to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build_config.yml(2 hunks)examples/usb/device/cdc_acm/CMakeLists.txt(1 hunks)examples/usb/device/cdc_acm/README.md(1 hunks)examples/usb/device/cdc_acm/main/CMakeLists.txt(1 hunks)examples/usb/device/cdc_acm/main/cdc_acm.c(1 hunks)examples/usb/device/cdc_acm/main/idf_component.yml(2 hunks)examples/usb/device/cdc_acm/main/tusb/tusb_config.h(1 hunks)examples/usb/device/cdc_acm/main/tusb/usb_descriptors.c(1 hunks)examples/usb/device/cdc_acm/sdkconfig.defaults(1 hunks)examples/usb/device/cdc_device/.gitignore(0 hunks)examples/usb/device/cdc_device/main/CMakeLists.txt(0 hunks)examples/usb/device/cdc_device/main/cdc/cdc_config.c(0 hunks)examples/usb/device/cdc_device/main/cdc/include/cdc_config.h(0 hunks)examples/usb/device/cdc_device/main/main.c(0 hunks)examples/usb/device/cdc_device/sdkconfig.defaults(0 hunks)
🪛 LanguageTool
examples/usb/device/cdc_acm/README.md
[style] ~3-~3: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... serial port tool, you will receive the exact same data back.
(EN_WORDINESS_PREMIUM_EXACT_SAME)
💤 Files with no reviewable changes (6)
- examples/usb/device/cdc_device/sdkconfig.defaults
- examples/usb/device/cdc_device/main/CMakeLists.txt
- examples/usb/device/cdc_device/.gitignore
- examples/usb/device/cdc_device/main/main.c
- examples/usb/device/cdc_device/main/cdc/include/cdc_config.h
- examples/usb/device/cdc_device/main/cdc/cdc_config.c
✅ Files skipped from review due to trivial changes (1)
- examples/usb/device/cdc_acm/main/tusb/usb_descriptors.c
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/usb/device/cdc_acm/sdkconfig.defaults
- examples/usb/device/cdc_acm/CMakeLists.txt
- examples/usb/device/cdc_acm/main/idf_component.yml
- examples/usb/device/cdc_acm/main/CMakeLists.txt
- examples/usb/device/cdc_acm/main/cdc_acm.c
- examples/usb/device/cdc_acm/main/tusb/tusb_config.h
🧰 Additional context used
🪛 LanguageTool
examples/usb/device/cdc_acm/README.md
[style] ~3-~3: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... serial port tool, you will receive the exact same data back.
(EN_WORDINESS_PREMIUM_EXACT_SAME)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
build_config.yml (1)
138-145: Ensure the new paths are valid and old references were fully dropped
examples/usb/device/bulk_transandexamples/usb/device/cdc_acmreplace the formercdc_deviceentry.
Please double-check that:
examples/usb/device/cdc_deviceno longer appears anywhere in this file (or CI will try to build a non-existent project).- Both new directories actually exist in the repo and contain a valid
CMakeLists.txt, otherwise the build matrix will fail.A quick
rg 'cdc_device'/fd cdc_devicein the tree is usually enough to confirm.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation