RP2350: fix USB CDC stability (competing tud_task() timers)

[sensei-hacker]

Summary

The RP2350 USB serial connection was dropping ~2 seconds after the configurator connected, preventing the full tab set from loading. Root cause was two concurrent tud_task() callers corrupting TinyUSB internal state.

Changes

• cmake/rp2350.cmake: Add LIB_TINYUSB_DEVICE=1 to RP2350_DEFINITIONS. Without this flag, pico_stdio_usb calls stdio_usb_init() which sets up its own low-priority IRQ + repeating alarm calling tud_task(), racing with INAV's own 1 ms timer. The race corrupts TinyUSB state and causes USB CDC to drop after a random interval (~2 s).

• src/main/target/RP2350_PICO/pico_sdk_config.h: Change LIB_PICO_STDIO_USB from 1 to 0. When enabled, printf() writes to the same CDC TX FIFO as MSP responses, injecting debug text into the MSP packet stream and causing the configurator to disconnect.

• src/main/drivers/serial_usb_vcp_rp2350.c: Add the three mandatory TinyUSB descriptor callbacks (tud_descriptor_device_cb, tud_descriptor_configuration_cb, tud_descriptor_string_cb). Setting LIB_TINYUSB_DEVICE=1 disables pico_stdio_usb/stdio_usb_descriptors.c (guarded by #if !defined(LIB_TINYUSB_DEVICE)), which previously provided these callbacks. The new descriptors use VID=0x2E8A, PID=0x000B (Pico 2 CDC), the target's USBD_PRODUCT_STRING as the product name, and the unique board ID as the serial number.

• src/main/drivers/system_rp2350.c: Update comments to accurately describe the single-caller tud_task() architecture.

Testing

• Flashed to Raspberry Pi Pico 2 (RP2350) via SWD (OpenOCD + debugprobe)
• USB device enumerates as Raspberry Pi RP2350_PICO with unique board serial number
• 30/30 MSP_IDENT requests successful over 30 seconds — no connection drops
• INAV 9.0.0 / FC variant "INAV" confirmed via MSP
• lsusb shows VID/PID 2e8a:000b (correct for Pico 2 CDC application)

[qodo-code-review]

Review Summary by Qodo

RP2350: Fix USB CDC stability and correct pin assignments

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Fix USB CDC stability by eliminating competing tud_task() timers
  - Define LIB_TINYUSB_DEVICE=1 to prevent pico_stdio_usb from spawning duplicate timer
  - Disable LIB_PICO_STDIO_USB to prevent debug output corrupting MSP packets
  - Implement mandatory TinyUSB descriptor callbacks in serial_usb_vcp_rp2350.c
• Correct motor and UART pin assignments to match Betaflight RP2350A reference
  - Renumber motors to GP10–13 (from GP8–11) for Betaflight compatibility
  - Move UART2 to GP6/7 and UART3 to GP8/9; relocate SPI0 to GP2/3
  - Use UART_FUNCSEL_NUM() for proper GPIO function selection
• Update comments and documentation for clarity

Diagram

flowchart LR
  A["pico_stdio_usb<br/>competing timer"] -->|"LIB_TINYUSB_DEVICE=1<br/>disables"| B["Single tud_task()<br/>caller"]
  C["Debug printf()<br/>to CDC FIFO"] -->|"LIB_PICO_STDIO_USB=0<br/>disables"| D["Clean MSP<br/>packet stream"]
  E["Missing TinyUSB<br/>descriptors"] -->|"Add callbacks<br/>in serial_usb_vcp"| F["USB device<br/>enumerates"]
  G["Motor pins<br/>GP8-11"] -->|"Renumber to<br/>GP10-13"| H["Betaflight<br/>compatible"]
  I["UART2 on GP2/3<br/>SPI0 on GP6/7"] -->|"Swap pins<br/>per datasheet"| J["Correct function<br/>mapping"]

Loading

File Changes

1. cmake/rp2350.cmake ⚙️ Configuration changes +5/-0

Define LIB_TINYUSB_DEVICE to prevent timer race

• Add LIB_TINYUSB_DEVICE=1 definition to prevent pico_stdio_usb from initializing TinyUSB and
 spawning a competing tud_task() timer
• Add explanatory comments describing the race condition and its fix

cmake/rp2350.cmake

---

2. src/main/target/RP2350_PICO/pico_sdk_config.h ⚙️ Configuration changes +4/-1

Disable USB stdio to prevent packet corruption

• Change LIB_PICO_STDIO_USB from 1 to 0 to disable USB stdio
• Add comments explaining that printf() output was corrupting MSP packet stream

src/main/target/RP2350_PICO/pico_sdk_config.h

---

3. src/main/target/RP2350_PICO/target.h 🐞 Bug fix +21/-16

Correct UART and SPI pin assignments per datasheet

• Update UART2 pins from GP2/3 to GP6/7 (correct RP2350 datasheet mapping)
• Update UART3 pins from GP12/13 to GP8/9 (freeing GP12/13 for motors)
• Update SPI0 pins from GP6/7 to GP2/3 (now available after UART2 move)
• Update motor pin documentation from GP8–11 to GP10–13
• Update comments to reflect correct GPIO function selections (F11 for UART_AUX)

src/main/target/RP2350_PICO/target.h

---

View more (4)

4. src/main/target/RP2350_PICO/target.c ✨ Enhancement +10/-6

Renumber motors to GP10–13 for Betaflight compatibility

• Renumber motor assignments: move from GP8–11 to GP10–13 to match Betaflight RP2350A
• Reorder timerHardware[] entries to group motors (GP10–13) and servos (GP8–9, GP14–15)
• Update comments to clarify motor/servo pin layout and Betaflight compatibility

src/main/target/RP2350_PICO/target.c

---

5. src/main/drivers/serial_uart_rp2350.c 🐞 Bug fix +4/-4

Use UART_FUNCSEL_NUM for correct GPIO function selection

• Replace hardcoded GPIO_FUNC_UART with UART_FUNCSEL_NUM() macro for UART0 and UART1
• Allows proper selection of GPIO function F11 (UART_AUX) for pins where bit 1 is set (GP6, GP7)

src/main/drivers/serial_uart_rp2350.c

---

6. src/main/drivers/serial_usb_vcp_rp2350.c 🐞 Bug fix +131/-3

Implement TinyUSB descriptor callbacks for CDC

• Add three mandatory TinyUSB descriptor callbacks: tud_descriptor_device_cb,
 tud_descriptor_configuration_cb, tud_descriptor_string_cb
• Implement USB device descriptor with VID 0x2E8A (Raspberry Pi), PID 0x000B (Pico 2 CDC)
• Generate unique serial number from board ID using pico_get_unique_board_id_string()
• Add configuration descriptor for CDC interface with proper endpoint definitions
• Update file header comments to explain descriptor callback responsibility

src/main/drivers/serial_usb_vcp_rp2350.c

---

7. src/main/drivers/system_rp2350.c 📝 Documentation +9/-5

Update comments for single-caller tud_task architecture

• Update comments to clarify that LIB_TINYUSB_DEVICE=1 prevents pico_stdio_usb from spawning
 competing timer
• Explain that the 1ms hardware alarm is the sole caller of tud_task()
• Add note that stdio_init_all() is effectively a no-op for USB when LIB_PICO_STDIO_USB=0

src/main/drivers/system_rp2350.c

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. tud_task multi-caller remains ☑ 🐞 Bug ⛯ Reliability

Description

The PR documents/assumes a single tud_task() caller (the 1ms timer), but the USB VCP driver still
calls tud_task() from foreground code paths. This can create re-entrant TinyUSB execution and
corrupt shared state, especially with the new static-buffer string-descriptor callback.

Code

src/main/drivers/system_rp2350.c[R116-120]

+    // Drive USB CDC from a dedicated 1ms hardware alarm so tud_task() runs even
+    // when the cooperative scheduler is busy (e.g. during long tasks or CLI mode).
+    // Only ONE caller of tud_task() exists: this timer.  pico_stdio_usb is disabled
+    // (LIB_PICO_STDIO_USB=0) so stdio_usb_init() registers no competing IRQ/alarm.
   if (!add_repeating_timer_ms(-1, usb_task_timer_cb, NULL, &usb_task_timer)) {

Evidence

tud_task() is executed from the repeating timer callback, while the USB VCP implementation also
calls tud_task() in normal call paths (available/write/flush). This contradicts the updated “only
ONE caller” architecture comment and creates a realistic preemption/reentrancy hazard; additionally,
the new tud_descriptor_string_cb() returns a pointer to a static buffer which is unsafe if
re-entered.

src/main/drivers/system_rp2350.c[87-92]
src/main/drivers/system_rp2350.c[116-120]
src/main/drivers/serial_usb_vcp_rp2350.c[86-91]
src/main/drivers/serial_usb_vcp_rp2350.c[109-130]
src/main/drivers/serial_usb_vcp_rp2350.c[326-351]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tud_task()` is currently called from both the 1ms repeating timer and foreground USB VCP paths, contradicting the intended “single caller” design and creating a reentrancy hazard. This is especially risky because the newly added `tud_descriptor_string_cb()` uses a static buffer and lazy initialization.
## Issue Context
The PR’s goal is to remove competing `tud_task()` timers/callers to avoid TinyUSB state corruption.
## Fix Focus Areas
- src/main/drivers/serial_usb_vcp_rp2350.c[86-160]
- src/main/drivers/system_rp2350.c[87-128]
- src/main/drivers/serial_usb_vcp_rp2350.c[326-351]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. String buffer may truncate 🐞 Bug ⛯ Reliability

Description

The new string-descriptor callback uses a fixed 20 UTF-16 code-unit buffer while the serial string
buffer is sized from PICO_UNIQUE_BOARD_ID_SIZE_BYTES. If the board-ID string (or product string)
exceeds 19 characters, the descriptor will be truncated, risking unstable host identification or
confusing USB enumeration behavior.

Code

src/main/drivers/serial_usb_vcp_rp2350.c[R324-347]

+#define USBD_DESC_STR_MAX 20
+
+const uint16_t *tud_descriptor_string_cb(uint8_t index, uint16_t langid)
+{
+    (void)langid;
+    static uint16_t desc_str[USBD_DESC_STR_MAX];
+
+    if (!usbd_serial_str[0]) {
+        pico_get_unique_board_id_string(usbd_serial_str, sizeof(usbd_serial_str));
+    }
+
+    uint8_t len;
+    if (index == 0) {
+        desc_str[1] = 0x0409;  // English
+        len = 1;
+    } else {
+        if (index >= ARRAYLEN(usbd_desc_str) || !usbd_desc_str[index]) {
+            return NULL;
+        }
+        const char *str = usbd_desc_str[index];
+        for (len = 0; len < USBD_DESC_STR_MAX - 1 && str[len]; len++) {
+            desc_str[1 + len] = str[len];
+        }
+    }

Evidence

The serial string storage is sized dynamically from PICO_UNIQUE_BOARD_ID_SIZE_BYTES, but
tud_descriptor_string_cb() converts strings into a separate fixed-size desc_str[20] buffer and
hard-stops at USBD_DESC_STR_MAX - 1, truncating any longer string.

src/main/drivers/serial_usb_vcp_rp2350.c[304-311]
src/main/drivers/serial_usb_vcp_rp2350.c[324-346]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tud_descriptor_string_cb()` uses a fixed-size UTF-16 buffer (`USBD_DESC_STR_MAX = 20`) which can truncate longer strings (notably the dynamically generated unique-board-id serial string).
## Issue Context
The PR switched to custom descriptor callbacks and now populates the USB serial string from `pico_get_unique_board_id_string()`.
## Fix Focus Areas
- src/main/drivers/serial_usb_vcp_rp2350.c[304-351]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more