Add USB CDC port drivers for ESP32, RP2 and STM32#2301
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds USB CDC port drivers (ESP32, RP2, STM32) and new UART implementations/tests, enabling serial_dist over USB CDC and expanding UART support across platforms.
Changes:
- Added USB CDC ACM port drivers and TinyUSB integration for STM32 and RP2; enabled ESP32 USB CDC via ESP-IDF TinyUSB.
- Added new UART NIF/driver implementations for STM32 and RP2 plus STM32 Renode UART test coverage.
- Updated docs/examples/CI to cover USB CDC distribution and new platform capabilities.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platforms/stm32/tests/test_erl_sources/test_uart.erl | Adds an Erlang UART smoke test for STM32. |
| src/platforms/stm32/tests/test_erl_sources/CMakeLists.txt | Packages the new STM32 UART test runnable. |
| src/platforms/stm32/tests/renode/stm32h743.repl | Adds USART2 to Renode platform for loopback testing. |
| src/platforms/stm32/tests/renode/stm32_uart_test.robot | Adds Renode robot test to validate UART TX/RX loopback. |
| src/platforms/stm32/src/lib/usb_irq_handlers.c | Wires STM32-family USB IRQs to TinyUSB interrupt handler. |
| src/platforms/stm32/src/lib/usb_hw_init.h | Declares STM32 USB HW init shim for TinyUSB bring-up. |
| src/platforms/stm32/src/lib/usb_hw_init.c | Implements STM32 USB clock/GPIO/NVIC setup for TinyUSB CDC. |
| src/platforms/stm32/src/lib/usb_descriptors.c | Provides TinyUSB device/config/string descriptors for STM32 CDC. |
| src/platforms/stm32/src/lib/usb_cdc_driver.h | Declares STM32 USB CDC driver poll hook. |
| src/platforms/stm32/src/lib/usb_cdc_driver.c | Implements STM32 USB CDC AtomVM port driver on TinyUSB. |
| src/platforms/stm32/src/lib/uart_driver.c | Adds STM32 UART NIF implementation backed by HAL. |
| src/platforms/stm32/src/lib/tusb_config.h | Adds STM32 TinyUSB configuration header. |
| src/platforms/stm32/src/lib/sys.c | Pumps TinyUSB CDC driver from the platform event poll loop. |
| src/platforms/stm32/src/lib/CMakeLists.txt | Builds/links UART + optional TinyUSB CDC components for STM32. |
| src/platforms/stm32/cmake/stm32_tinyusb.cmake | Fetches/configures TinyUSB device stack for STM32 families. |
| src/platforms/stm32/CMakeLists.txt | Adds build option to enable STM32 USB CDC port driver. |
| src/platforms/rp2/src/lib/usb_descriptors.c | Adds TinyUSB descriptors for RP2 USB CDC. |
| src/platforms/rp2/src/lib/usb_cdc_driver.c | Implements RP2 USB CDC AtomVM port driver on TinyUSB. |
| src/platforms/rp2/src/lib/uartdriver.c | Adds RP2 UART NIF implementation backed by Pico SDK. |
| src/platforms/rp2/src/lib/tusb_config.h | Adds RP2 TinyUSB configuration header. |
| src/platforms/rp2/src/lib/sys.c | Pumps TinyUSB from RP2 sys_poll_events and adds TinyUSB lock helpers. |
| src/platforms/rp2/src/lib/rp2_sys.h | Adds TinyUSB lock API + platform data mutex field. |
| src/platforms/rp2/src/lib/CMakeLists.txt | Builds/links UART + optional TinyUSB CDC sources for RP2. |
| src/platforms/rp2/src/CMakeLists.txt | Routes console to UART when USB CDC driver is enabled. |
| src/platforms/rp2/CMakeLists.txt | Adds build option to enable RP2 USB CDC port driver. |
| src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c | Implements ESP32 USB CDC AtomVM port driver using esp_tinyusb + CDC ACM. |
| src/platforms/esp32/components/avm_builtins/Kconfig | Adds Kconfig option to enable ESP32 USB CDC port driver. |
| src/platforms/esp32/components/avm_builtins/CMakeLists.txt | Links esp_tinyusb when USB CDC driver is enabled. |
| libs/eavmlib/src/uart_hal.erl | Updates UART HAL docs to reflect broader platform support. |
| libs/avm_stm32/src/usb_cdc.erl | Adds STM32 usb_cdc uart_hal implementation. |
| libs/avm_stm32/src/uart.erl | Adds STM32 uart module (high-level uart_hal + low-level NIF API). |
| libs/avm_stm32/src/CMakeLists.txt | Packs STM32 uart + usb_cdc modules into avm_stm32 archive. |
| libs/avm_rp2/src/usb_cdc.erl | Adds RP2 usb_cdc uart_hal implementation. |
| libs/avm_rp2/src/uart.erl | Adds RP2 uart module (high-level uart_hal + low-level NIF API). |
| libs/avm_rp2/src/CMakeLists.txt | Packs RP2 uart + usb_cdc modules into avm_rp2 archive. |
| libs/avm_esp32/src/usb_cdc.erl | Adds ESP32 usb_cdc uart_hal implementation. |
| libs/avm_esp32/src/CMakeLists.txt | Packs ESP32 usb_cdc module into avm_esp32 archive. |
| examples/erlang/usb_disterl.erl | Adds example for distributed Erlang over USB CDC via serial_dist. |
| examples/erlang/stm32/CMakeLists.txt | Packs STM32 sim800l runnable. |
| examples/erlang/sim800l.erl | Adds SIM800L UART demo using new UART APIs. |
| examples/erlang/rp2/CMakeLists.txt | Generates UF2 target for sim800l example on RP2. |
| examples/erlang/CMakeLists.txt | Packs usb_disterl + sim800l runnables. |
| doc/src/distributed-erlang.md | Documents USB CDC distribution and usage patterns. |
| CHANGELOG.md | Notes new UART and USB CDC support across platforms. |
| .github/workflows/stm32-build.yaml | Adds USB CDC on one STM32 build and runs new UART Renode test. |
| .github/workflows/pico-build.yaml | Adds USB CDC build variant; adjusts test/artifact conditions. |
| .github/workflows/esp32-build.yaml | Adds USB CDC build variant and injects required sdkconfig defaults. |
Comments suppressed due to low confidence (3)
src/platforms/stm32/src/lib/usb_cdc_driver.c:1
- The forward declaration marks usb_cdc_driver_create_port as static, but the definition is non-static. This is a conflicting linkage declaration and is typically a compile error. Make the linkage consistent by either removing
staticfrom the prototype or addingstaticto the definition.
src/platforms/stm32/src/lib/usb_cdc_driver.c:1 - The forward declaration marks usb_cdc_driver_create_port as static, but the definition is non-static. This is a conflicting linkage declaration and is typically a compile error. Make the linkage consistent by either removing
staticfrom the prototype or addingstaticto the definition.
src/platforms/stm32/cmake/stm32_tinyusb.cmake:1 STM32_TINYUSB_DCD_PORTis not set anywhere in this CMake file (the selected list isSTM32_TINYUSB_DCD_SOURCES). As written, the message is misleading and may indicate a missing variable or typo. Either defineSTM32_TINYUSB_DCD_PORT(if intended) or print the correct variable (e.g., the chosen source list or a human-readable port name).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
|
Usual caveats: PR Review:
|
| # | Issue | Severity |
|---|---|---|
| B1 | ESP32: local_pid used before declaration (usb_cdc_driver.c:180) |
Blocker (compile error) |
| B2 | STM32: IRQ callback can race close and dereference freed s_cdc_data |
Blocker (UAF in IRQ) |
| B3 | RP2: usb_descriptors.c uses Raspberry Pi-assigned PID 0x2E8A:0x000A (pico-bootrom) |
Blocker (USB-IF conflict) |
| B4 | STM32: stm32_tinyusb.cmake advertises F2/F4/F7, but usb_hw_init.c only supports H5/H7 |
Blocker (build failure) |
| M1 | ESP32: listener-handler runs tinyusb_cdcacm_read synchronously but does not unregister the RX callback or remove queue from FreeRTOS queue-set before deletion in do_close |
Major (UAF risk) |
| M2 | ESP32: when tusb_cdc_acm_init returns ESP_ERR_INVALID_STATE the RX callback isn't explicitly (re)registered |
Major (no reads delivered) |
| M3 | ESP32: context_new(global) return value is never null-checked in create_port |
Major (NULL deref + resource leak) |
| M4 | RP2: usb_cdc_listener_handler reads reader_process_pid unlocked and clears it after sending the reply |
Major (SMP race / lost new reader) |
| M5 | RP2: do_read checks reader_process_pid unlocked, then safe_update_reader_data only locks during the write |
Major (SMP race) |
| M6 | RP2: do_write holds sys_tinyusb_lock across the entire 500 ms write loop |
Major (blocks sys_poll_events, other core) |
| M7 | RP2: do_close never calls queue_free(&cdc_data->rxqueue) |
Major (resource leak) |
| M8 | All platforms: port:call(Port, read, Timeout) + port:call(Port, cancel_read) is racy — a late {ok, Data} reply can land in the mailbox after cancel_read returns |
Major (API correctness) |
| M9 | Open/singleton reservation is not atomic on any platform — concurrent opens can race | Major (SMP) |
| M10 | ESP32: write returns {error, closed} when host not connected; RP2/STM32 silently drop and reply ok |
Major (cross-platform inconsistency) |
| m1 | ESP32: unrecognized command path logs only; RP2/STM32 send {error, unknown_command} — caller hangs until timeout on ESP32 |
Minor |
| m2 | RP2/STM32 fast ok path for "host not connected" skips memory_ensure_free_with_roots (inconsistent with rest of drivers) |
Minor |
| m3 | STM32: usb_descriptors.c uses TinyUSB example VID/PID 0xCAFE:0x4001 |
Minor |
| m4 | STM32 usb_cdc:read/2 calls port:call(Port, cancel_read, Timeout); ESP32/RP2 use the default-timeout form |
Minor |
| m5 | RP2 do_close reply-failure path doesn't unwind the listener it just unregistered |
Minor |
Confirmed compile error (Blocker B1)
In usb_cdc_driver.c (esp32), usb_cdc_interrupt_callback uses local_pid on line 180 but only declares it on line 197:
if (UNLIKELY(memory_init_heap(&heap, bin_size + REF_SIZE + TUPLE_SIZE(2) * 2) != MEMORY_GC_OK)) {
fprintf(stderr, "Failed to allocate memory: %s:%i.\n", __FILE__, __LINE__);
globalcontext_send_message(glb, local_pid, OUT_OF_MEMORY_ATOM); // <-- used here
return listener;
}
...
int local_pid = term_to_local_process_id(reader_pid); // <-- declared hereSuggested fix
diff --git a/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c b/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c
@@ -174,18 +174,17 @@ EventListener *usb_cdc_interrupt_callback(GlobalContext *glb, EventListener *lis
}
int bin_size = term_binary_heap_size(rx_size);
+ int local_pid = term_to_local_process_id(reader_pid);
Heap heap;
if (UNLIKELY(memory_init_heap(&heap, bin_size + REF_SIZE + TUPLE_SIZE(2) * 2) != MEMORY_GC_OK)) {
fprintf(stderr, "Failed to allocate memory: %s:%i.\n", __FILE__, __LINE__);
globalcontext_send_message(glb, local_pid, OUT_OF_MEMORY_ATOM);
return listener;
}
@@ -194,7 +193,6 @@
term result_tuple = term_alloc_tuple(2, &heap);
term_put_tuple_element(result_tuple, 0, ref);
term_put_tuple_element(result_tuple, 1, ok_tuple);
- int local_pid = term_to_local_process_id(reader_pid);
globalcontext_send_message(glb, local_pid, result_tuple);Blocker B2 — STM32: IRQ vs do_close UAF
stm32 usb_cdc_driver.c IRQ callback:
void tud_cdc_rx_cb(uint8_t itf)
{
if (s_cdc_data != NULL && itf == s_cdc_data->itf) {
s_cdc_data->rx_pending = true;
}
}In do_close:
s_cdc_data = NULL;
...
free(cdc_data);There is no IRQ exclusion. An interrupt can latch the old non-NULL pointer just before close clears it, then write rx_pending = true into freed memory after free().
volatile bool rx_pending is also not enough — the IRQ may also need to see the most recent s_cdc_data write.
Suggested fix — brief PRIMASK-based critical sections
diff --git a/src/platforms/stm32/src/lib/usb_cdc_driver.c b/src/platforms/stm32/src/lib/usb_cdc_driver.c
@@
static struct USBCDCData *s_cdc_data = NULL;
+static inline uint32_t usb_cdc_irq_save(void)
+{
+ uint32_t primask = __get_PRIMASK();
+ __disable_irq();
+ return primask;
+}
+
+static inline void usb_cdc_irq_restore(uint32_t primask)
+{
+ __set_PRIMASK(primask);
+}
+
void tud_cdc_rx_cb(uint8_t itf)
{
- if (s_cdc_data != NULL && itf == s_cdc_data->itf) {
- s_cdc_data->rx_pending = true;
- }
+ /* Called from OTG_FS / USB_DRD_FS IRQ via dcd_int_handler. */
+ if (s_cdc_data != NULL && itf == s_cdc_data->itf) {
+ s_cdc_data->rx_pending = true;
+ }
}
static void usb_cdc_check_rx(struct USBCDCData *cdc_data)
{
- if (!cdc_data->rx_pending) {
- return;
- }
- cdc_data->rx_pending = false;
+ uint32_t primask = usb_cdc_irq_save();
+ bool rx_pending = cdc_data->rx_pending;
+ cdc_data->rx_pending = false;
+ usb_cdc_irq_restore(primask);
+
+ if (!rx_pending) {
+ return;
+ }
@@
- s_cdc_data = cdc_data;
+ uint32_t primask = usb_cdc_irq_save();
+ s_cdc_data = cdc_data;
+ usb_cdc_irq_restore(primask);
@@
- s_cdc_data = NULL;
+ uint32_t primask = usb_cdc_irq_save();
+ s_cdc_data = NULL;
+ usb_cdc_irq_restore(primask);This closes the IRQ-vs-task window without introducing a heavier mutex/atomic design that would be inappropriate from IRQ context.
Blocker B3 — RP2 USB PID conflict
usb_descriptors.c (rp2):
#define USBD_VID 0x2E8A
#define USBD_PID 0x000a0x2E8A:0x000A is a Raspberry Pi-assigned PID used for pico-bootrom. Shipping a CDC device with that VID/PID can confuse host-side tooling (picotool, udev rules, etc.) and is not appropriate for a third-party device.
Suggested stopgap fix (until a project-owned VID/PID is registered)
diff --git a/src/platforms/rp2/src/lib/usb_descriptors.c b/src/platforms/rp2/src/lib/usb_descriptors.c
@@
-#define USBD_VID 0x2E8A
-#define USBD_PID 0x000a
+#ifndef USBD_VID
+#define USBD_VID 0xCAFE /* TinyUSB example VID; replace before release */
+#endif
+#ifndef USBD_PID
+#define USBD_PID 0x4002 /* Placeholder PID; do not ship in production */
+#endifThe proper fix is to either obtain a VID/PID, use Pico SDK reserved test PIDs, or expose VID/PID via a CMake/compile-time option so downstream users can override.
Blocker B4 — STM32 build mismatch
stm32_tinyusb.cmake maps h7, h5, f4, f7, f2 to TinyUSB MCU defines. But usb_hw_init.c #if defined(STM32H7XX) || defined(STM32H5XX) is the only path with HW init; the #else branch hard-errors:
#error AVM_USB_CDC_PORT_DRIVER_ENABLED: usb_cdc_hw_init not implemented for this STM32 family.So enabling AVM_USB_CDC_PORT_DRIVER_ENABLED=ON on F2/F4/F7 will configure successfully through CMake and then fail to compile. The CMake support set and the C support set must match.
Suggested fix — restrict CMake to what works today
diff --git a/src/platforms/stm32/cmake/stm32_tinyusb.cmake b/src/platforms/stm32/cmake/stm32_tinyusb.cmake
@@
if (STM32_FAMILY_SHORT STREQUAL "h7")
set(STM32_TINYUSB_MCU "OPT_MCU_STM32H7")
set(STM32_TINYUSB_DCD_SOURCES ${_DWC2_SOURCES})
elseif (STM32_FAMILY_SHORT STREQUAL "h5")
set(STM32_TINYUSB_MCU "OPT_MCU_STM32H5")
set(STM32_TINYUSB_DCD_SOURCES ${_FSDEV_SOURCES})
-elseif (STM32_FAMILY_SHORT STREQUAL "f4")
- set(STM32_TINYUSB_MCU "OPT_MCU_STM32F4")
- set(STM32_TINYUSB_DCD_SOURCES ${_DWC2_SOURCES})
-elseif (STM32_FAMILY_SHORT STREQUAL "f7")
- set(STM32_TINYUSB_MCU "OPT_MCU_STM32F7")
- set(STM32_TINYUSB_DCD_SOURCES ${_DWC2_SOURCES})
-elseif (STM32_FAMILY_SHORT STREQUAL "f2")
- set(STM32_TINYUSB_MCU "OPT_MCU_STM32F2")
- set(STM32_TINYUSB_DCD_SOURCES ${_DWC2_SOURCES})
else()
- message(FATAL_ERROR "AVM_USB_CDC_PORT_DRIVER_ENABLED: TinyUSB MCU mapping not configured for STM32 family ${STM32_FAMILY_SHORT}")
+ message(FATAL_ERROR "AVM_USB_CDC_PORT_DRIVER_ENABLED is currently implemented only for stm32h7 and stm32h5; ${STM32_FAMILY_SHORT} still needs usb_hw_init.c support")
endif()Alternatively keep F2/F4/F7 mapping but add usb_hw_init.c implementations for them.
Major M1 + M3 — ESP32 close path / create_port lifetime
usb_cdc_rx_callbackis registered viatusb_cdc_acm_init(&acm_cfg)butdo_closedoes not call any unregister/deinit; an ISR-context call can still fire andxQueueSendFromISRinto a queue we are about tovQueueDelete. The atomic NULL store ons_cdc_data[itf]only protects late entries to the callback, not entries already past the NULL check.- The rxqueue is added to a queue-set (
xQueueAddToSet) but never removed viaxQueueRemoveFromSetbeforevQueueDelete. context_new(global)at usb_cdc_driver.c:311 is dereferenced without a NULL check; on OOM this NULL-derefs and leaks queue / listener / mutex.- When the CDC interface was already initialized (e.g. console),
tusb_cdc_acm_initreturnsESP_ERR_INVALID_STATEand the code carries on, but the RX callback isn't (re)registered for ourcdc_data. Reads will never wake.
Suggested fix sketch
diff --git a/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c b/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c
@@ struct USBCDCData
uint64_t reader_ref_ticks;
tinyusb_cdcacm_itf_t itf;
+ bool owns_itf;
#ifndef AVM_NO_SMP
Mutex *reader_lock;
#endif
};
@@ Context *usb_cdc_driver_create_port(...)
- esp_err_t err = tusb_cdc_acm_init(&acm_cfg);
+ esp_err_t err = tusb_cdc_acm_init(&acm_cfg);
if (err != ESP_OK && err != ESP_ERR_INVALID_STATE) {
...
return NULL;
}
+ cdc_data->owns_itf = (err == ESP_OK);
+ /*
+ * If the interface was pre-initialized (e.g. console), our acm_cfg
+ * callbacks were ignored — explicitly install ours.
+ */
+ if (!cdc_data->owns_itf) {
+ esp_err_t rerr = tinyusb_cdcacm_register_callback(itf, CDC_EVENT_RX, usb_cdc_rx_callback);
+ if (rerr != ESP_OK) {
+ ESP_LOGE(TAG, "Failed to register CDC RX callback: %s", esp_err_to_name(rerr));
+ vQueueDelete(cdc_data->rxqueue);
+#ifndef AVM_NO_SMP
+ smp_mutex_destroy(cdc_data->reader_lock);
+#endif
+ free(cdc_data);
+ return NULL;
+ }
+ }
@@
- sys_register_listener(global, &cdc_data->listener);
-
- Context *ctx = context_new(global);
+ Context *ctx = context_new(global);
+ if (IS_NULL_PTR(ctx)) {
+ tinyusb_cdcacm_unregister_callback(itf, CDC_EVENT_RX);
+ xQueueRemoveFromSet(cdc_data->rxqueue, event_set);
+ vQueueDelete(cdc_data->rxqueue);
+#ifndef AVM_NO_SMP
+ smp_mutex_destroy(cdc_data->reader_lock);
+#endif
+ free(cdc_data);
+ return NULL;
+ }
+ sys_register_listener(global, &cdc_data->listener);
@@ static void usb_cdc_driver_do_close(...)
atomic_store_explicit(&s_cdc_data[cdc_data->itf], NULL, memory_order_release);
+ /* Stop further RX callbacks BEFORE deleting the queue. */
+ tinyusb_cdcacm_unregister_callback(cdc_data->itf, CDC_EVENT_RX);
+ /* If we own the interface and IDF exposes a deinit, call it here. */
sys_unregister_listener(glb, &cdc_data->listener);
+ xQueueRemoveFromSet(cdc_data->rxqueue, event_set);
vQueueDelete(cdc_data->rxqueue);
- term pending_reader_pid = cdc_data->reader_process_pid;
- uint64_t pending_reader_ref_ticks = cdc_data->reader_ref_ticks;
- cdc_data->reader_process_pid = term_invalid_term();
+ SMP_MUTEX_LOCK(cdc_data->reader_lock);
+ term pending_reader_pid = cdc_data->reader_process_pid;
+ uint64_t pending_reader_ref_ticks = cdc_data->reader_ref_ticks;
+ cdc_data->reader_process_pid = term_invalid_term();
+ cdc_data->reader_ref_ticks = 0;
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);Also fix the ESP32 unknown-command parity:
@@ static NativeHandlerResult usb_cdc_driver_consume_mailbox(Context *ctx)
default:
ESP_LOGW(TAG, "Unrecognized command.");
+ usb_cdc_send_error_reply(ctx, gen_message.pid, gen_message.ref,
+ ATOM_STR("\xF", "unknown_command"));
break;Major M4 + M5 — RP2 SMP races on reader_process_pid
usb_cdc_listener_handler reads cdc_data->reader_process_pid and reader_ref_ticks without holding reader_lock, then later calls safe_update_reader_data(...) to clear — meaning between the message send and the clear, a newly installed reader can be overwritten. do_read has the same unlocked-check race.
Suggested fix
diff --git a/src/platforms/rp2/src/lib/usb_cdc_driver.c b/src/platforms/rp2/src/lib/usb_cdc_driver.c
@@ static EventListener *usb_cdc_listener_handler(GlobalContext *glb, EventListener *base_listener)
- if (cdc_data->reader_process_pid == term_invalid_term()) {
- return base_listener;
- }
-
- uint8_t buf[USB_CDC_BUF_SIZE];
- sys_tinyusb_lock(glb);
- uint32_t rx_size = tud_cdc_n_read(cdc_data->itf, buf, sizeof(buf));
- sys_tinyusb_unlock(glb);
- if (rx_size == 0) {
- return base_listener;
- }
+ SMP_MUTEX_LOCK(cdc_data->reader_lock);
+ if (cdc_data->reader_process_pid == term_invalid_term()) {
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
+ return base_listener;
+ }
+ uint8_t buf[USB_CDC_BUF_SIZE];
+ sys_tinyusb_lock(glb);
+ uint32_t rx_size = tud_cdc_n_read(cdc_data->itf, buf, sizeof(buf));
+ sys_tinyusb_unlock(glb);
+ if (rx_size == 0) {
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
+ return base_listener;
+ }
+ term reader_pid = cdc_data->reader_process_pid;
+ uint64_t reader_ref_ticks = cdc_data->reader_ref_ticks;
+ cdc_data->reader_process_pid = term_invalid_term();
+ cdc_data->reader_ref_ticks = 0;
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
@@
- term ref = term_from_ref_ticks(cdc_data->reader_ref_ticks, &heap);
+ term ref = term_from_ref_ticks(reader_ref_ticks, &heap);
@@
- int local_pid = term_to_local_process_id(cdc_data->reader_process_pid);
+ int local_pid = term_to_local_process_id(reader_pid);
globalcontext_send_message(glb, local_pid, result_tuple);
- memory_destroy_heap(&heap, glb);
- safe_update_reader_data(cdc_data, NO_READER, NO_REF);
+ memory_destroy_heap(&heap, glb);
@@ static void usb_cdc_driver_do_read(Context *ctx, GenMessage gen_message)
- if (cdc_data->reader_process_pid != term_invalid_term()) {
+ SMP_MUTEX_LOCK(cdc_data->reader_lock);
+ if (cdc_data->reader_process_pid != term_invalid_term()) {
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
...
return;
}
@@
uint8_t buf[USB_CDC_BUF_SIZE];
sys_tinyusb_lock(glb);
uint32_t rx_size = tud_cdc_n_read(cdc_data->itf, buf, sizeof(buf));
sys_tinyusb_unlock(glb);
if (rx_size > 0) {
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
...
port_send_reply(ctx, pid, ref, ok_tuple);
} else {
- safe_update_reader_data(cdc_data, pid, ref_ticks);
+ cdc_data->reader_process_pid = pid;
+ cdc_data->reader_ref_ticks = ref_ticks;
+ SMP_MUTEX_UNLOCK(cdc_data->reader_lock);
}
}Major M6 — RP2 long TinyUSB lock hold in do_write
do_write (rp2) calls sys_tinyusb_lock once before the write loop and unlocks only after it ends — possibly 500 ms later. In the meantime, sys_poll_events on the other core can be stuck spinning on the same lock and TinyUSB receive/processing cannot make progress.
Suggested fix — lock only around the TinyUSB calls
diff --git a/src/platforms/rp2/src/lib/usb_cdc_driver.c b/src/platforms/rp2/src/lib/usb_cdc_driver.c
@@ static void usb_cdc_driver_do_write(...)
- sys_tinyusb_lock(glb);
- if (!tud_cdc_n_connected(cdc_data->itf)) {
- sys_tinyusb_unlock(glb);
- free(buffer);
- port_send_reply(ctx, pid, ref, OK_ATOM);
- return;
- }
-
size_t written = 0;
uint32_t deadline = to_ms_since_boot(get_absolute_time()) + USB_CDC_WRITE_TIMEOUT_MS;
bool host_gone = false;
bool timed_out = false;
while (written < buffer_size) {
- if (!tud_cdc_n_connected(cdc_data->itf)) {
- host_gone = true;
- break;
- }
- uint32_t chunk = tud_cdc_n_write(cdc_data->itf,
- (const uint8_t *) buffer + written,
- buffer_size - written);
- tud_cdc_n_write_flush(cdc_data->itf);
+ sys_tinyusb_lock(glb);
+ bool connected = tud_cdc_n_connected(cdc_data->itf);
+ uint32_t chunk = 0;
+ if (connected) {
+ chunk = tud_cdc_n_write(cdc_data->itf,
+ (const uint8_t *) buffer + written,
+ buffer_size - written);
+ tud_cdc_n_write_flush(cdc_data->itf);
+ if (chunk == 0) {
+ tud_task();
+ }
+ }
+ sys_tinyusb_unlock(glb);
+
+ if (!connected) {
+ host_gone = true;
+ break;
+ }
if (chunk > 0) {
written += chunk;
deadline = to_ms_since_boot(get_absolute_time()) + USB_CDC_WRITE_TIMEOUT_MS;
} else {
if ((int32_t) (to_ms_since_boot(get_absolute_time()) - deadline) >= 0) {
timed_out = true;
break;
}
- tud_task();
+ sleep_ms(1);
}
}
- sys_tinyusb_unlock(glb);Major M7 — RP2 do_close leaks queue_t storage
queue_init(&cdc_data->rxqueue, sizeof(uint32_t), EVENT_QUEUE_LEN) allocates backing storage. do_close never calls queue_free(&cdc_data->rxqueue) — the buffer leaks every time the port is closed.
Suggested fix
diff --git a/src/platforms/rp2/src/lib/usb_cdc_driver.c b/src/platforms/rp2/src/lib/usb_cdc_driver.c
@@ static void usb_cdc_driver_do_close(...)
sys_unregister_listener(glb, &cdc_data->listener);
+ queue_free(&cdc_data->rxqueue);Major M8 — read/2 timeout + cancel_read is racy on all three platforms
All three modules:
read(Port, Timeout) ->
case port:call(Port, read, Timeout) of
{error, timeout} ->
port:call(Port, cancel_read),
{error, timeout};
Result ->
Result
end.This is racy in a fundamental way:
port:call/3times out locally in the caller.- The driver replies to the original
readref a moment later (data just arrived). - The caller then sends
cancel_read; the driver clears its reader slot. - The caller now has an unmatched
{Ref, {ok, Data}}message stuck in its mailbox.
Because the C drivers only track "one pending reader" with no per-request matching, cancel_read cancels "whatever is pending", which may already have been replied to. There is no way to distinguish "read still pending" from "read just replied".
Suggested fix — cancel_read should target a specific request
- In Erlang, send
cancel_readcarrying the original read ref. - In C, only clear the stored reader if its ref matches.
- Also issue a final selective
receiveinread/2to flush a late-arriving{Ref, _}reply on timeout (matching by ref).
Minimal consistency-only fix in the meantime
diff --git a/libs/avm_stm32/src/usb_cdc.erl b/libs/avm_stm32/src/usb_cdc.erl
@@
case port:call(Port, read, Timeout) of
{error, timeout} ->
- port:call(Port, cancel_read, Timeout),
+ port:call(Port, cancel_read),
{error, timeout};
Result ->
Result
end.Major M9 — Open/singleton reservation is not atomic
- ESP32 usb_cdc_driver.c:241-244 loads
s_cdc_data[itf]then later stores at :313. Concurrent opens of the same interface can both pass the check and race to publish; the loser still allocated/registered everything. - RP2/STM32 use a plain
static struct USBCDCData *s_cdc_data = NULLwith no synchronization at all.
Suggested fix — atomic compare-and-swap reservation
Use atomic_compare_exchange_strong (ESP32 already has _Atomic) to reserve the slot up-front; on RP2/STM32 wrap the check+publish in a mutex.
Major M10 — write semantics when host is not connected
- RP2
do_write: drops silently, repliesok. - STM32
do_write: drops silently, repliesok. - ESP32
do_write: returns{error, closed}(callstud_cdc_n_connectedper chunk).
Same API, different behavior. The PR should pick one — the comment in the RP2 driver explains why "drop + ok" is desirable for serial_dist. If that's the chosen semantics, ESP32 needs to match.
Suggested fix (ESP32 → match RP2/STM32 "drop + ok")
diff --git a/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c b/src/platforms/esp32/components/avm_builtins/usb_cdc_driver.c
@@ static void usb_cdc_driver_do_write(...)
- while (written < buffer_size) {
- if (!tud_cdc_n_connected(cdc_data->itf)) {
- free(buffer);
- usb_cdc_send_error_reply(ctx, pid, ref, ATOM_STR("\x6", "closed"));
- return;
- }
+ if (!tud_cdc_n_connected(cdc_data->itf)) {
+ /* Host hasn't opened the CDC tty yet: silently drop and reply ok,
+ * matching RP2/STM32 behavior and what serial_dist expects at boot. */
+ free(buffer);
+ if (UNLIKELY(memory_ensure_free_with_roots(ctx, TUPLE_SIZE(2), 1, &ref, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
+ globalcontext_send_message(glb, local_pid, OUT_OF_MEMORY_ATOM);
+ return;
+ }
+ port_send_reply(ctx, pid, ref, OK_ATOM);
+ return;
+ }
+ while (written < buffer_size) {
size_t to_write = buffer_size - written;Minor m1 — ESP32 unknown-command parity (covered in B1/M1 diff above)
Minor m2 — Missing memory_ensure_free_with_roots on RP2/STM32 fast ok
rp2 do_write:340 and stm32 do_write:302 call port_send_reply(..., OK_ATOM) without memory_ensure_free_with_roots. This is inconsistent with every other reply path in these files; ref allocations downstream may fail under heap pressure.
Minor m3 — STM32 VID/PID = TinyUSB example
0xCAFE:0x4001 is the TinyUSB example VID/PID. Less harmful than the RP2 Raspberry Pi PID, but still placeholder-quality for a release.
Minor m4 — cancel_read timeout consistency (covered in M8 diff)
Minor m5 — RP2 do_close partial-failure reply
if (UNLIKELY(memory_ensure_free_with_roots(...) != MEMORY_GC_OK)) {
globalcontext_send_message(glb, term_to_local_process_id(pid), OUT_OF_MEMORY_ATOM);
free(cdc_data);
ctx->platform_data = NULL;
return;
}The listener has already been unregistered, the rxqueue is still live (no queue_free), and cdc_data is freed. The on-failure path is inconsistent with the success path.
Cross-platform consistency checklist before merge
- Write semantics when host is not connected — pick "drop + ok" or "closed", apply uniformly.
- Unknown command — uniformly return
{error, unknown_command}. read/2timeout cancel — adopt a per-requestcancel_readso late replies don't leak.- Singleton/open reservation — atomic publish on all three.
- VID/PID policy — either project-owned VID/PID or compile-time configurable.
Recommended merge gate
Must fix before merge:
- B1 ESP32 compile error
- B2 STM32 IRQ/task UAF
- B3 RP2 Raspberry Pi PID
- B4 STM32 CMake/source family mismatch
- M1 ESP32 close-path lifetime / queue-set / context_new null check / explicit RX callback registration
- M4–M5 RP2 SMP races on
reader_process_pid - M6 RP2 TinyUSB lock scope in
do_write - M7 RP2
queue_freeleak - M10 write semantics consistency
Strongly recommended in the same PR:
- Redesigned
read/2+ per-requestcancel_read(M8) - Atomic open singleton reservation (M9)
- ESP32 unknown-command parity (m1)
memory_ensure_free_with_rootsparity on the RP2/STM32 fast-ok paths (m2)- VID/PID configurable / non-conflicting (B3 stopgap + STM32 m3)
Suggested follow-up commits (after the above lands)
- Add an
examples/erlang/CMakeLists.txtentry for a non-distributedusb_cdcecho example. - Update
doc/src/distributed-erlang.mdwith an explicit "VID/PID configuration" note (the patch already adds prose there). - Consider exposing a SET_LINE_CODING callback to signal DTR/RTS to the Erlang side (some hosts rely on it).
- Add CI build for
AVM_USB_CDC_PORT_DRIVER_ENABLED=ONon a representative RP2 / STM32H7 target so this regresses loudly if the build glue rots.
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
|
Great work, amp is only complaining about using the rp2 pid/vid - seems like it's correct to avoid using that? - should we use the tinyusb example values for now both/all platforms - and then get our own via https://pid.codes/howto/ (free for OSS) in a followup PR? PR Review: USB CDC port drivers for ESP32, RP2 and STM32Author: Paul Guyot pguyot@kallisys.net What changed since the first reviewThe contributor force-pushed Quick verdict: close to mergeable. All originally-reported blockers are either fixed or have been re-evaluated and found not to apply on the actual platform. One major item (RP2 default VID/PID still ships the Raspberry Pi pico-bootrom identity) and one minor (STM32 default VID/PID still the TinyUSB example) remain. Status of original findings
Remaining issues to fix before mergeB2 — Withdrawn after re-analysis (was: STM32
|
We could apply for a free PID/VID, however we only provide CDC here and we don't have any windows driver issue. Also, I double checked Raspberry Pi and Espressif documentation. We should definitely use their VID/PID. ST also provides free sublicensing of PID for small projects. I updated the documentation accordingly. I also added CMake-based customization of the three strings which is a nice feature. |
pguyot
force-pushed
the
w19/add-usb-cdc
branch
2 times, most recently
from
0ce733b to
d8ac99f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Continuation of:
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later