feat(firmware/c_board): Migrate USB stack from HAL CDC to TinyUSB#20
Merged
feat(firmware/c_board): Migrate USB stack from HAL CDC to TinyUSB#20
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Walkthrough将 STM32 HAL 的 ST USB 设备栈替换为 TinyUSB：新增 tinyusb 子模块与配置、在 CMake 中集成 TinyUSB，移除 ST USB Device 中间件与相关应用代码，新增 TinyUSB 描述符与回调，调整中断与主循环调用 tud_task()，并改造 Vendor/缓冲/构建相关逻辑。 Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Loop\nfirmware/c_board/src/app.cpp
participant TinyUSB as TinyUSB Stack\n(tinyusb)
participant DWC2 as DWC2 Driver\n(dcd_dwc2.c)
participant HAL as STM32 HAL\n(USB OTG FS)
participant Vendor as Vendor Handler\nfirmware/c_board/src/usb/vendor.cpp
Main->>TinyUSB: tud_task()
activate TinyUSB
TinyUSB->>DWC2: 调度/触发端点处理
activate DWC2
DWC2->>HAL: 硬件访问 / 中断上下文
HAL-->>DWC2: 中断/状态
DWC2-->>TinyUSB: 中断处理完成
deactivate DWC2
alt 接收数据
TinyUSB->>Vendor: tud_vendor_rx_cb(buffer, size)
activate Vendor
Vendor->>Vendor: handle_downlink(数据段, finished)
deactivate Vendor
end
alt 事件（挂载/挂起/恢复/卸载）
TinyUSB->>Vendor: tud_mount_cb / tud_suspend_cb / tud_resume_cb / tud_umount_cb
Vendor->>Vendor: 更新状态
end
deactivate TinyUSB
Estimated code review effort🎯 4 (复杂) | ⏱️ ~60 分钟 Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
firmware/c_board/bsp/HAL/rmcs_slave.ioc (1)
232-232:⚠️ Potential issue | 🟡 Minor
ProjectManager.functionlistsort包含已删除的MX_USB_DEVICE_Init条目
USB_DEVICE外设已在本 PR 中移除，但第 232 行functionlistsort仍保留了4-MX_USB_DEVICE_Init-USB_DEVICE-false-HAL-false。下次在 CubeMX 中重新打开此.ioc文件时，该陈旧条目可能导致代码重新生成行为异常。🐛 建议修复
-ProjectManager.functionlistsort=1-SystemClock_Config-RCC-false-HAL-false,2-MX_GPIO_Init-GPIO-false-HAL-true,3-MX_SPI1_Init-SPI1-false-HAL-true,4-MX_USB_DEVICE_Init-USB_DEVICE-false-HAL-false,5-MX_CAN1_Init-CAN1-false-HAL-true,6-MX_CAN2_Init-CAN2-false-HAL-true,7-MX_USART1_UART_Init-USART1-false-HAL-true,8-MX_USART3_UART_Init-USART3-false-HAL-true,9-MX_USART6_UART_Init-USART6-false-HAL-true,10-MX_TIM5_Init-TIM5-false-HAL-true +ProjectManager.functionlistsort=1-SystemClock_Config-RCC-false-HAL-false,2-MX_GPIO_Init-GPIO-false-HAL-true,3-MX_SPI1_Init-SPI1-false-HAL-true,4-MX_USB_OTG_FS_PCD_Init-USB_OTG_FS-false-HAL-true,5-MX_CAN1_Init-CAN1-false-HAL-true,6-MX_CAN2_Init-CAN2-false-HAL-true,7-MX_USART1_UART_Init-USART1-false-HAL-true,8-MX_USART3_UART_Init-USART3-false-HAL-true,9-MX_USART6_UART_Init-USART6-false-HAL-true,10-MX_TIM5_Init-TIM5-false-HAL-true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/HAL/rmcs_slave.ioc` at line 232, Remove the stale MX_USB_DEVICE_Init entry from the ProjectManager.functionlistsort string: delete the segment "4-MX_USB_DEVICE_Init-USB_DEVICE-false-HAL-false" and adjust the indices of the following entries (e.g., 5→4, 6→5, etc.) so the sequence remains contiguous; ensure the remaining entries (e.g., MX_CAN1_Init, MX_CAN2_Init, MX_USART1_UART_Init, MX_USART3_UART_Init, MX_USART6_UART_Init, MX_TIM5_Init) keep their original subsystem/tags and HAL flags unchanged.
🧹 Nitpick comments (2)
firmware/c_board/src/usb/usb_descriptors.hpp (1)
34-64:get_string_descriptor非线程安全 — 在裸机单任务环境下可接受，但值得记录。
descriptor_string_buffer_是就地修改的共享成员。当前裸机环境中 TinyUSB 描述符回调只在tud_task()上下文调用，不存在竞争。如果未来引入 RTOS 或 ISR 中调用此函数，将产生数据竞争。建议加一行注释说明此假设。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/usb/usb_descriptors.hpp` around lines 34 - 64, get_string_descriptor mutates shared member descriptor_string_buffer_ and is not thread-safe; add a clear comment in the get_string_descriptor function explaining the assumption that this callback is only invoked from the single-threaded tinyusb context (tud_task()) and that concurrent access from RTOS tasks or ISRs would cause data races; mention descriptor_string_buffer_, serial_string_, and TinyUSB/tud_task() to help future maintainers locate the safe-context assumption and consider protecting or copying the buffer if the code is later called from multiple contexts.firmware/c_board/src/usb/vendor.hpp (1)
99-105: 加速度计和陀螺仪回调留空 — 确认是否需要后续实现。当前为空实现（no-op），如果后续需要从主机侧下发 IMU 配置，需要补充。建议添加
// TODO注释标记意图。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/usb/vendor.hpp` around lines 99 - 105, The two empty no-op callbacks accelerometer_deserialized_callback(...) and gyroscope_deserialized_callback(...) should be annotated to show the no-op is intentional and to guide future work: add a TODO comment inside each function noting that IMU configuration from the host is not yet implemented and, if applicable, outline the intended behavior (e.g., parse the data and forward to IMU config handler or update settings via set_imu_config()/apply_imu_settings()); this makes intent explicit and reminds future contributors to implement the handling when host-side IMU config support is added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitmodules:
- Around line 4-6: The submodule entry for firmware/c_board/bsp/tinyusb points
to a personal fork (url = https://github.com/qzhhhi/tinyusb) which creates
supply-chain risk; update the .gitmodules entry for the submodule at path
"firmware/c_board/bsp/tinyusb" to point to the upstream official repository
(e.g., https://github.com/hathach/tinyusb) or, if the fork contains necessary
hardware-specific patches, restore the submodule to upstream and add a
PATCHES.md (or note in README adjacent to "firmware/c_board/bsp/tinyusb") that
lists the exact commits/patches and rationale and include a plan/links for
upstreaming those patches (submit a PR to hathach/tinyusb).
In `@firmware/c_board/bsp/tinyusb`:
- Line 1: The repo's tinyusb submodule points to a personal fork URL in
.gitmodules at commit bb24b0c without documentation; either switch the submodule
remote to the official upstream (https://github.com/hathach/tinyusb.git) and
reference a stable upstream tag (e.g., 0.19.0) while keeping project-specific
config in firmware/c_board/include/tusb_config.h, or document the fork rationale
and diffs by adding a TINYUSB_FORK_NOTES.md under firmware/c_board that lists
the upstream base commit/tag, the forked remote URL, and a concise list of code
patches (files/functions affected) that require the fork (include commit SHAs
and why they are necessary) so reviewers and future maintainers can trace and
merge upstream fixes.
In `@firmware/c_board/src/usb/vendor.cpp`:
- Around line 24-31: tud_umount_cb is currently empty but must tell the Vendor
to clear in-progress transfers; add a call in tud_umount_cb to a Vendor method
(e.g., Vendor::ResetTransmission or Vendor::OnUsbUnmount) that atomically clears
transmitting_batch_ and sets transmitted_size_ to 0 (and releases any batch
resources), and ensure this method is safe to call while
device_ready()/tud_ready() may return false after unmount. Locate the Vendor
class that owns transmitting_batch_ and transmitted_size_ and implement the
reset method and invoke it from tud_umount_cb.
---
Outside diff comments:
In `@firmware/c_board/bsp/HAL/rmcs_slave.ioc`:
- Line 232: Remove the stale MX_USB_DEVICE_Init entry from the
ProjectManager.functionlistsort string: delete the segment
"4-MX_USB_DEVICE_Init-USB_DEVICE-false-HAL-false" and adjust the indices of the
following entries (e.g., 5→4, 6→5, etc.) so the sequence remains contiguous;
ensure the remaining entries (e.g., MX_CAN1_Init, MX_CAN2_Init,
MX_USART1_UART_Init, MX_USART3_UART_Init, MX_USART6_UART_Init, MX_TIM5_Init)
keep their original subsystem/tags and HAL flags unchanged.
---
Nitpick comments:
In `@firmware/c_board/src/usb/usb_descriptors.hpp`:
- Around line 34-64: get_string_descriptor mutates shared member
descriptor_string_buffer_ and is not thread-safe; add a clear comment in the
get_string_descriptor function explaining the assumption that this callback is
only invoked from the single-threaded tinyusb context (tud_task()) and that
concurrent access from RTOS tasks or ISRs would cause data races; mention
descriptor_string_buffer_, serial_string_, and TinyUSB/tud_task() to help future
maintainers locate the safe-context assumption and consider protecting or
copying the buffer if the code is later called from multiple contexts.
In `@firmware/c_board/src/usb/vendor.hpp`:
- Around line 99-105: The two empty no-op callbacks
accelerometer_deserialized_callback(...) and
gyroscope_deserialized_callback(...) should be annotated to show the no-op is
intentional and to guide future work: add a TODO comment inside each function
noting that IMU configuration from the host is not yet implemented and, if
applicable, outline the intended behavior (e.g., parse the data and forward to
IMU config handler or update settings via
set_imu_config()/apply_imu_settings()); this makes intent explicit and reminds
future contributors to implement the handling when host-side IMU config support
is added.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
host/src/transport/usb.cpp (1)
425-433: 建议仅在收到实际数据时更新时间戳，以避免 ZLP 重置间隔计时器当前实现对每次 USB 回调（包括
actual_length == 0的零长度包）都更新last_rx_callback_timepoint_。若设备固件复位期间 USB 栈仍生成 ZLP（例如 bulk 端点的流控行为），这些空包会持续重置计时窗口，导致固件恢复后首个真实数据包的间隔 < 1 秒，从而绕过丢弃逻辑——正好是该机制想要防止的情况。将时间戳更新移至
actual_length > 0分支内，可使计时器精确反映「上次收到有效数据的时刻」：♻️ 建议改法
- const auto now = std::chrono::steady_clock::now(); - const bool should_drop = now > last_rx_callback_timepoint_ + std::chrono::seconds{1}; - last_rx_callback_timepoint_ = now; - - if (!should_drop && transfer->actual_length > 0) { + if (transfer->actual_length > 0) { + const auto now = std::chrono::steady_clock::now(); + const bool should_drop = now > last_rx_callback_timepoint_ + std::chrono::seconds{1}; + last_rx_callback_timepoint_ = now; + + if (!should_drop) { const auto* first = reinterpret_cast<std::byte*>(transfer->buffer); const auto size = static_cast<std::size_t>(transfer->actual_length); receive_callback_({first, size}); - } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@host/src/transport/usb.cpp` around lines 425 - 433, The timestamp update currently happens for every USB callback which lets zero-length packets (transfer->actual_length == 0) reset last_rx_callback_timepoint_ and defeat the drop logic; change the logic so you compute should_drop using last_rx_callback_timepoint_ as-is, but only assign last_rx_callback_timepoint_ = now inside the branch that handles actual data (i.e., when transfer->actual_length > 0) before calling receive_callback_; do not update last_rx_callback_timepoint_ for ZLPs so the interval reflects the last received valid data and the existing should_drop check remains correct.firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp (2)
25-28: 启动时is_locked_初始值为true（按当前语义即"可分配"），ISR 在首次try_transmit()锁定之前可能写入陈旧数据。缓冲区初始化后即接受分配，但 USB 尚未就绪。在主循环首次调用
try_transmit()之前，CAN ISR 可能已经向缓冲区写入数据。虽然后续try_unlock_and_clear()会清除这些陈旧数据，但如果缓冲区在启动期间被写满（kBatchCount - 1个 batch），新的 ISR 数据会被静默丢弃。如果这是预期行为，建议加个简短注释说明启动时的状态语义。如果不希望启动时接受分配，可将初始值改为
false（即锁定/拒绝状态），等 USB 就绪后再解锁。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp` around lines 25 - 28, The buffer accepts allocations at startup because is_locked_ is initialized to allow allocation; to prevent ISR writing stale data before USB is ready, initialize is_locked_ to the locked state (so allocate() returns {} until USB is ready) and only set it to unlocked after USB is ready (e.g. when try_transmit()/USB init path calls the unlock or try_unlock_and_clear()); if the current behavior is intentional instead, add a short comment near is_locked_ and allocate() explaining the startup semantic and the kBatchCount-1 overflow risk so reviewers understand why allocations are allowed during init.
123-136:is_locked_布尔语义与名称相反，极易引起误解。当前
is_locked_为true时，allocate()正常工作（即缓冲区处于可用/未锁定状态）；is_locked_为false时，allocate()拒绝分配（即缓冲区处于锁定状态）。这与字段名is_locked_的直觉含义完全相反——读者会自然地认为is_locked_ == true表示"已锁定"。建议将字段重命名为
is_accepting_或翻转布尔极性，使is_locked_ == true真正表示锁定（拒绝分配）。♻️ 建议：翻转布尔语义使名称与行为一致
- std::atomic<bool> is_locked_{true}; + std::atomic<bool> is_locked_{false};同步修改
allocate()：- if (!is_locked_.load(std::memory_order::relaxed)) + if (is_locked_.load(std::memory_order::relaxed)) return {};
try_lock()：- void try_lock() { is_locked_.store(false, std::memory_order::relaxed); } + void try_lock() { is_locked_.store(true, std::memory_order::relaxed); }
try_unlock_and_clear()：void try_unlock_and_clear() { - if (is_locked_.load(std::memory_order::relaxed)) + if (!is_locked_.load(std::memory_order::relaxed)) return; clear(); - is_locked_.store(true, std::memory_order::relaxed); + is_locked_.store(false, std::memory_order::relaxed); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp` around lines 123 - 136, The boolean field is_locked_ has inverted semantics (true means available) which is confusing; change it to a positively named flag (e.g., is_accepting_) or invert its polarity so true means "locked"/not accepting. Update the field name and all uses in allocate(), try_lock(), and try_unlock_and_clear(): initialize the flag to the correct default, flip/store the boolean values passed in those methods (use the same memory_order semantics), and adjust the conditional in try_unlock_and_clear() (and any allocate() checks) so the logic reads naturally (true => locked/refuse allocation, false => accepting). Also update the comment in try_unlock_and_clear() to reflect the new flag meaning and ensure clear() behavior remains the same.firmware/rmcs_board/src/usb/vendor.hpp (1)
69-79: 删除关键字段的 assert_debug 调用不会阻止 USB 写入操作在 release 构建中执行。
assert_debug宏在 release 模式下仅使用[[assume(condition)]]提示编译器，但不会阻止函数参数的求值。在assert_debug(tud_vendor_n_write(0, src, target_size) == target_size)中，tud_vendor_n_write()的调用会在assert_debug函数被调用之前执行，因此数据会被正常发送。不过，将副作用与断言混合仍然是不良实践。建议分离调用和断言以提高代码清晰度：
建议改进：分离副作用与断言
if (target_size) { const auto* src = reinterpret_cast<const uint8_t*>(data.data() + transmitted_size_); - core::utility::assert_debug(tud_vendor_n_write(0, src, target_size) == target_size); + auto sent = tud_vendor_n_write(0, src, target_size); + core::utility::assert_debug(sent == target_size); } else { - core::utility::assert_debug(tud_vendor_n_write_zlp(0)); + [[maybe_unused]] auto ok = tud_vendor_n_write_zlp(0); + core::utility::assert_debug(ok); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/vendor.hpp` around lines 69 - 79, The assert_debug currently wraps calls with side effects (tud_vendor_n_write and tud_vendor_n_write_zlp) so those functions execute even in release builds; separate the side-effect calls from the assertions: compute max_packet_size and target_size as you already do, then if target_size > 0 call tud_vendor_n_write(0, src, target_size) into a local variable (e.g. written = tud_vendor_n_write(...)) and assert_debug(written == target_size); otherwise call tud_vendor_n_write_zlp(0) into a local bool/result and assert_debug(result) — this ensures the write is invoked explicitly and the assertion only checks the result without hiding side effects inside assert_debug and makes the logic around transmitting_batch_, transmitted_size_, and tud_speed_get/TUSB_SPEED_HIGH clearer.firmware/c_board/src/usb/interrupt_safe_buffer.hpp (1)
30-31:is_locked_的语义与变量名称相反，try_lock()命名违反 C++ 惯例存在两个命名问题：
is_locked_语义倒置：变量初始化为true（Line 137），allocate()在!is_locked_时拒绝写入（Lines 30–31），try_lock()存入false（Line 124），try_unlock_and_clear()存入true表示"解锁"（Line 133）。也就是说：
is_locked_ == false→ 缓冲区处于锁定状态（ISR 无法写入）is_locked_ == true→ 缓冲区处于活跃状态（ISR 可写入）变量名暗示
true= 锁定，但实际语义恰好相反。建议改名为is_active_或accepts_allocations_，或反转内部逻辑使true对应锁定状态。
try_lock()违反 C++ 命名惯例：标准库（如std::mutex::try_lock()）中try_lock()返回bool以表示是否成功。此函数始终成功且返回void，建议直接命名为lock()。注意try_unlock_and_clear()保留try_前缀是合理的（因为它在is_locked_为真时会提前返回），但与try_lock()放在一起会造成语义上的不一致。♻️ 建议的重命名方案
- std::atomic<bool> is_locked_{true}; + std::atomic<bool> is_active_{true};std::span<std::byte> allocate(size_t size) noexcept override { core::utility::assert_debug(size <= core::protocol::kProtocolBufferSize); - if (!is_locked_.load(std::memory_order::relaxed)) + if (!is_active_.load(std::memory_order::relaxed)) return {};- void try_lock() { is_locked_.store(false, std::memory_order::relaxed); } + void lock() { is_active_.store(false, std::memory_order::relaxed); }void try_unlock_and_clear() { - if (is_locked_.load(std::memory_order::relaxed)) + if (is_active_.load(std::memory_order::relaxed)) return; clear(); - is_locked_.store(true, std::memory_order::relaxed); + is_active_.store(true, std::memory_order::relaxed); }Also applies to: 124-124, 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/usb/interrupt_safe_buffer.hpp` around lines 30 - 31, The is_locked_ flag's polarity and the name of try_lock() are misleading: is_locked_ is initialized true but treated as "active/accepting allocations" when true and "locked" when false; change the flag to a positively named boolean (e.g., is_active_ or accepts_allocations_) or invert its logic so true means locked, and update all uses in allocate(), try_lock(), and try_unlock_and_clear() accordingly; also rename try_lock() to lock() (since it always succeeds and returns void) while keeping try_unlock_and_clear() as-is, and update any initialization site and all references to the old names to preserve correct semantic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/src/usb/vendor.hpp`:
- Around line 67-72: The USB write calls are executed inside
core::utility::assert_debug which may be disabled in release builds, so extract
the side-effectful calls out of the assert: call tud_vendor_n_write(0, src,
target_size) (using src = reinterpret_cast<const uint8_t*>(data.data() +
transmitted_size_)) when target_size is non-zero and call
tud_vendor_n_write_zlp(0) otherwise, store their return values in a local
variable (e.g., wrote or result), and then use
core::utility::assert_debug(result == target_size) or
core::utility::assert_debug(result) to assert success; keep the existing
variables transmitted_size_, data, target_size and functions tud_vendor_n_write
/ tud_vendor_n_write_zlp in your changes.
---
Nitpick comments:
In `@firmware/c_board/src/usb/interrupt_safe_buffer.hpp`:
- Around line 30-31: The is_locked_ flag's polarity and the name of try_lock()
are misleading: is_locked_ is initialized true but treated as "active/accepting
allocations" when true and "locked" when false; change the flag to a positively
named boolean (e.g., is_active_ or accepts_allocations_) or invert its logic so
true means locked, and update all uses in allocate(), try_lock(), and
try_unlock_and_clear() accordingly; also rename try_lock() to lock() (since it
always succeeds and returns void) while keeping try_unlock_and_clear() as-is,
and update any initialization site and all references to the old names to
preserve correct semantic behavior.
In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp`:
- Around line 25-28: The buffer accepts allocations at startup because
is_locked_ is initialized to allow allocation; to prevent ISR writing stale data
before USB is ready, initialize is_locked_ to the locked state (so allocate()
returns {} until USB is ready) and only set it to unlocked after USB is ready
(e.g. when try_transmit()/USB init path calls the unlock or
try_unlock_and_clear()); if the current behavior is intentional instead, add a
short comment near is_locked_ and allocate() explaining the startup semantic and
the kBatchCount-1 overflow risk so reviewers understand why allocations are
allowed during init.
- Around line 123-136: The boolean field is_locked_ has inverted semantics (true
means available) which is confusing; change it to a positively named flag (e.g.,
is_accepting_) or invert its polarity so true means "locked"/not accepting.
Update the field name and all uses in allocate(), try_lock(), and
try_unlock_and_clear(): initialize the flag to the correct default, flip/store
the boolean values passed in those methods (use the same memory_order
semantics), and adjust the conditional in try_unlock_and_clear() (and any
allocate() checks) so the logic reads naturally (true => locked/refuse
allocation, false => accepting). Also update the comment in
try_unlock_and_clear() to reflect the new flag meaning and ensure clear()
behavior remains the same.
In `@firmware/rmcs_board/src/usb/vendor.hpp`:
- Around line 69-79: The assert_debug currently wraps calls with side effects
(tud_vendor_n_write and tud_vendor_n_write_zlp) so those functions execute even
in release builds; separate the side-effect calls from the assertions: compute
max_packet_size and target_size as you already do, then if target_size > 0 call
tud_vendor_n_write(0, src, target_size) into a local variable (e.g. written =
tud_vendor_n_write(...)) and assert_debug(written == target_size); otherwise
call tud_vendor_n_write_zlp(0) into a local bool/result and assert_debug(result)
— this ensures the write is invoked explicitly and the assertion only checks the
result without hiding side effects inside assert_debug and makes the logic
around transmitting_batch_, transmitted_size_, and tud_speed_get/TUSB_SPEED_HIGH
clearer.
In `@host/src/transport/usb.cpp`:
- Around line 425-433: The timestamp update currently happens for every USB
callback which lets zero-length packets (transfer->actual_length == 0) reset
last_rx_callback_timepoint_ and defeat the drop logic; change the logic so you
compute should_drop using last_rx_callback_timepoint_ as-is, but only assign
last_rx_callback_timepoint_ = now inside the branch that handles actual data
(i.e., when transfer->actual_length > 0) before calling receive_callback_; do
not update last_rx_callback_timepoint_ for ZLPs so the interval reflects the
last received valid data and the existing should_drop check remains correct.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp (1)
123-134:try_unlock_and_clear()中is_locked_.clear(relaxed)可能因编译器重排导致 ISR 看到不一致状态。
clear()内部使用了atomic_signal_fence(release)来防止 batch 的reset()操作被重排到out_.store()之后。但 release fence 只阻止之前的操作越过 fence，不阻止之后的操作（即is_locked_.clear()）被移到 fence 之前。在理论上，编译器可以将第 132 行的
is_locked_.clear(relaxed)重排到clear()内部的 batchreset()之前。如果此时 ISR 触发，allocate()会看到is_locked_已清除，但 batch 尚未完全重置，可能导致写入脏数据。建议在
is_locked_.clear()前增加一个 signal fence，与文件中已有的模式保持一致：🛡️ 建议修改
bool try_unlock_and_clear() { if (!is_locked_.test(std::memory_order::relaxed)) return false; // Unlocking drops stale queued batches from the last not-ready cycle before // new ISR writes are accepted. clear(); + std::atomic_signal_fence(std::memory_order_release); is_locked_.clear(std::memory_order::relaxed); return true; }注：
clear()内部的 fence 保护的是reset()→out_.store()的顺序，而此处需要额外保证clear()的所有副作用在is_locked_.clear()之前对 ISR 可见。在单核裸机环境下，GCC/Clang 实践中不太可能做如此激进的重排，但从 C++ 内存模型的角度来看，加一个 fence 更严谨。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp` around lines 123 - 134, In try_unlock_and_clear(), prevent compiler reordering that could move is_locked_.clear(std::memory_order::relaxed) before clear()'s reset by inserting an atomic_signal_fence(std::memory_order::release) immediately before calling is_locked_.clear(); this mirrors the fence used inside clear() and ensures clear()'s side-effects (reset() → out_.store()) are observed by the ISR before the lock bit is cleared, protecting allocate() from seeing an unlocked flag while batch reset is still in progress; update the try_unlock_and_clear() function to call atomic_signal_fence(release) then is_locked_.clear(relaxed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp`:
- Around line 123-134: In try_unlock_and_clear(), prevent compiler reordering
that could move is_locked_.clear(std::memory_order::relaxed) before clear()'s
reset by inserting an atomic_signal_fence(std::memory_order::release)
immediately before calling is_locked_.clear(); this mirrors the fence used
inside clear() and ensures clear()'s side-effects (reset() → out_.store()) are
observed by the ISR before the lock bit is cleared, protecting allocate() from
seeing an unlocked flag while batch reset is still in progress; update the
try_unlock_and_clear() function to call atomic_signal_fence(release) then
is_locked_.clear(relaxed).
qzhhhi
force-pushed
the
dev/cboard-tinyusb
branch
from
35cdb42 to
ae6be2b
Compare
Replace the ST USB Device Library (CDC middleware) with TinyUSB, following the migration path outlined in commit 2d6076d. What changed - Added TinyUSB as a git submodule (DWC2 portable driver for STM32F4 USB OTG FS) and integrated it into the CMake build. - Removed the entire ST USB Device Library middleware and USB_DEVICE application layer (~6500 lines deleted). - Reconfigured CubeMX to expose raw USB_OTG_FS PCD instead of the CDC device class; regenerated HAL accordingly. - Rewrote USB descriptors as TinyUSB callbacks with vendor-class device identity (VID 0xA11C, PID 0xD401) and UID-derived serial. - Rewrote vendor.cpp/hpp to use TinyUSB vendor class API (tud_vendor_n_write, tud_vendor_rx_cb) in direct mode. - Redirected OTG_FS IRQ from HAL_PCD_IRQHandler to tusb_int_handler; added tud_task() to the main loop. Not yet completed - WCID / Microsoft OS Descriptor support is not wired in yet; Windows auto-recognition still requires follow-up work. - No on-device regression testing has been performed.
To prevent residual data from entering the protocol layer during USB disconnects/reconnects, this commit introduces a "flush and drop" mechanism on both the firmware and host sides: - Firmware: Locks the upstream buffer when USB is not ready. Upon recovery, it flushes the buffer before unlocking. This prevents stale batches queued during the disconnect window from being transmitted after reconnection. - Host: Introduces a time-window check in the RX callback. After a connection or reconnection, it forces the dropping of the first received data block to ensure protocol-layer isolation. This approach effectively prevents half/truncated packets and stale data from reaching the deserialization flow, with the acceptable trade-off of intentionally losing the first packet. - InterruptSafeBuffer: add lock + clear + unlock flow; reject allocation while locked - Vendor::try_transmit: split ready checks and trigger clear before resuming dequeue - Host USB transport: track last RX callback time and drop the first callback after a long gap
qzhhhi
force-pushed
the
dev/cboard-tinyusb
branch
from
ae6be2b to
4918f8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
host/src/transport/usb.cpp (1)
425-433:⚠️ Potential issue | 🟠 Major节流判断逻辑与预期语义相反，设备发送频率 ≤ 1 Hz 时将静默丢弃所有数据
当前逻辑：
const bool should_drop = now > last_rx_callback_timepoint_ + std::chrono::seconds{1};
should_drop = true当距上次包超过 1 秒（即包到达时存在空闲间隔）→ 丢弃!should_drop时才调用receive_callback_，意即只有包连续密集到达（间隔 < 1s）时才转发这与通常的"节流"（限制回调调用的最高频率）完全相反。实际效果是：
- 启动时第一个包永远被丢弃（
min()初始化触发）- 若设备每秒发包 ≤ 1 次，每次到来时距上次均超过 1 秒，所有包均被静默丢弃
若意图是过滤重连/启动后首个可能残缺的包（burst warm-up guard），请将判断条件翻转，并添加注释说明；若意图确实是"只在连续流中才处理数据"，需评估 ≤ 1 Hz 的设备场景是否永久无数据。
此外，
last_rx_callback_timepoint_在should_drop == true时依然更新，实际语义是"上次收到包的时间"而非"上次调用回调的时间"，变量命名存在误导性。🐛 若意图是过滤首包（保留后续数据流），建议修正
- const bool should_drop = now > last_rx_callback_timepoint_ + std::chrono::seconds{1}; + // Drop the very first packet after a reconnect/startup to discard potentially + // stale data; once packets arrive continuously (gap < 1 s) forward all of them. + const bool is_after_gap = now - last_rx_callback_timepoint_ > std::chrono::seconds{1}; last_rx_callback_timepoint_ = now; - if (!should_drop && transfer->actual_length > 0) { + if (!is_after_gap && transfer->actual_length > 0) {若意图是标准节流（至多 1 次/秒），则应在
should_drop == false时不更新时间戳（或翻转整个判断逻辑）：- const bool should_drop = now > last_rx_callback_timepoint_ + std::chrono::seconds{1}; - last_rx_callback_timepoint_ = now; - - if (!should_drop && transfer->actual_length > 0) { + // Rate-limit: invoke at most once per second. + if (transfer->actual_length > 0 && + now - last_rx_callback_timepoint_ >= std::chrono::seconds{1}) { + last_rx_callback_timepoint_ = now; receive_callback_({first, size}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@host/src/transport/usb.cpp` around lines 425 - 433, The throttle logic is inverted and updates last_rx_callback_timepoint_ even when dropping packets; change the condition so we drop when the time since last callback is less than 1s (i.e. allow callback only when now >= last_rx_callback_timepoint_ + 1s), only update last_rx_callback_timepoint_ when you actually call receive_callback_, and add a brief comment on the intended semantics (throttle to max 1 callback/sec or, if you meant to drop only the very first warm-up packet, implement that explicit check instead); refer to last_rx_callback_timepoint_, should_drop (or remove it), receive_callback_, and transfer->actual_length to locate and fix the code.
🧹 Nitpick comments (5)
firmware/c_board/src/usb/vendor.hpp (1)
112-112: 考虑为协议错误提供更宽松的恢复路径
error_callback会在反序列化器遇到任何协议错误（例如宿主端发送的格式错误帧、包损坏）时被调用。当前实现直接调用assert_failed_always()，会无条件停机。基于项目学习记录中"调试断言应仅用于捕获内部逻辑错误和不变式违反，不应用于运行时可能出现的输入验证场景"的原则，协议错误属于运行时事件，建议在错误时重置反序列化器状态而不是直接 assert-halt，以提升固件健壮性。♻️ 可选的容错实现
- void error_callback() override { core::utility::assert_failed_always(); } + void error_callback() override { + // 协议错误为运行时事件，重置解析器状态以等待下一个合法帧 + deserializer_ = core::protocol::Deserializer{*this}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/usb/vendor.hpp` at line 112, error_callback currently calls core::utility::assert_failed_always(), which unconditionally halts on any protocol/deserialization error; change it to implement a relaxed recovery path: inside error_callback (the override in the class that implements the deserializer callback), clear or reset the deserializer state (e.g., reset parser indices/buffers), optionally increment a protocol error counter and emit a runtime log/event, and return without asserting so the firmware continues running and can recover from malformed/partial frames; locate the error_callback override and replace the assert call with calls to the deserializer reset routine (or implement one if missing), logging, and a harmless return.firmware/c_board/src/usb/interrupt_safe_buffer.hpp (1)
126-135:try_unlock_and_clear()中test()与clear()非原子组合——当前安全但脆弱。
is_locked_.test()(line 127) 和is_locked_.clear()(line 133) 之间不是原子操作。在当前裸机单线程主循环模型下是安全的（try_lock和try_unlock_and_clear都只在主线程调用），但如果将来引入 RTOS 或多个调用者，这里会产生竞态。建议添加简短注释说明此前提。建议添加注释
bool try_unlock_and_clear() { + // Caller contract: must only be called from the main thread (no concurrent callers). if (!is_locked_.test(std::memory_order::relaxed)) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/usb/interrupt_safe_buffer.hpp` around lines 126 - 135, The calls is_locked_.test(...) and is_locked_.clear(...) inside try_unlock_and_clear() are not atomic together and rely on the current single-threaded main-loop assumption; add a concise comment in try_unlock_and_clear() (referencing is_locked_.test, clear, is_locked_.clear and try_lock) stating that this non-atomic test/clear sequence is safe only because try_lock and try_unlock_and_clear are called exclusively from the main thread (no RTOS/ISR concurrency), and note that if RTOS or multi-caller use is introduced the sequence must be made atomic (e.g., by using atomic compare_exchange or a mutex) to avoid races.firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp (1)
16-141: 两个板级InterruptSafeBuffer近乎完全重复，建议提取共享实现。
firmware/c_board/src/usb/interrupt_safe_buffer.hpp和本文件的InterruptSafeBuffer约 95% 代码相同（包括本次新增的clear()、try_lock()、try_unlock_and_clear()），唯一差异在于 buffer-full 时的通知钩子（c_board 调用led::led->uplink_buffer_full()，rmcs_board 留了 TODO）。可以考虑将通用逻辑提取到一个共享基类或 CRTP/策略模板中，通过模板参数或虚函数注入板级差异（通知钩子），避免同步维护两份几乎一致的代码。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp` around lines 16 - 141, InterruptSafeBuffer is duplicated across boards—extract the common logic (class InterruptSafeBuffer: allocate, Batch, pop_batch, release_batch, clear, try_lock, try_unlock_and_clear, fields in_, out_, batches_, is_locked_) into a shared base (either a non-virtual template/CRTP base or a concrete base class) and inject the board-specific "buffer full" behavior via a small virtual hook or policy template parameter (e.g., notify_buffer_full() or a Policy::on_buffer_full()). Replace the two near-identical classes with thin board-specific subclasses/templates that implement the hook (one calling led::led->uplink_buffer_full(), the other no-op or TODO), keeping all existing method names (allocate, clear, try_lock, try_unlock_and_clear, pop_batch, Batch, release_batch) so callers need no changes.firmware/rmcs_board/src/usb/vendor.hpp (2)
92-120: 回调分发逻辑中default分支直接触发assert_failed_always()——未知 ID 将导致硬停机。
can_deserialized_callback(line 99) 和uart_deserialized_callback(line 108) 对未知id直接触发断言失败。在固件环境下，如果上位机发送了未知的 field ID（例如协议版本不匹配），设备会直接崩溃。对于生产固件，建议至少考虑用日志/LED 指示替代硬停机，或者在非调试模式下忽略未知 ID。不过如果当前阶段还在开发调试中，这种"fail-fast"策略也是可以接受的。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/vendor.hpp` around lines 92 - 120, can_deserialized_callback 和 uart_deserialized_callback 的 default 分支当前直接调用 assert_failed_always()，会因未知 field ID 导致设备硬停机；请改为在默认分支记录一条包含 id 的错误/警告日志并安全返回或忽略该事件（生产固件为忽略/记录，调试构建可保留 fail-fast 行为），也可以在必要时触发非致命指示（例如 LED）并复用现有的 error_callback 路径；参考并修改 can_deserialized_callback、uart_deserialized_callback 和可能的 error_callback，替换或包裹 assert_failed_always() 为基于构建宏（例如 NDEBUG）选择记录+返回或断言的实现。
53-89:try_transmit()的锁-解锁-传输流程设计合理，有几点值得注意。整体流程：
- 设备未就绪 → 锁定缓冲区阻止 ISR 写入 → 正确
- 端点无可用空间 → 直接返回 → 正确
- 解锁并清理陈旧数据 → 获取新 batch → 正确
- 分包发送 + ZLP 终结逻辑 → USB 传输语义正确
Line 55:
try_lock()的返回值被丢弃是合理的——意图是"确保已锁定"，重复调用幂等（已锁定时返回 false，无副作用）。Line 71:
max_packet_size每次调用都通过tud_speed_get()动态计算。虽然对支持 FULL/HIGH 双速的板子来说是必要的，但速度在枚举后不会改变。可以考虑在 mount 回调中缓存一次，避免每次传输重复查询（影响很小，纯建议）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/usb/vendor.hpp` around lines 53 - 89, The code recomputes max_packet_size each time in try_transmit() by calling tud_speed_get(); change this to compute and cache the max packet size once during USB mount/initialization (e.g., in the USB mount callback such as tud_mount_cb or your board-specific mount handler) into a member like usb_max_packet_size_ (or cached_max_packet_size) and have try_transmit() use that cached value instead of calling tud_speed_get() on each invocation; ensure the cached value is set to 512 for TUSB_SPEED_HIGH and 64 otherwise and that it is updated on re-enumeration if you handle that event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/src/usb/vendor.hpp`:
- Around line 34-37: 在 Vendor 构造中直接调用 tusb_rhport_init(0, nullptr) 可以改为显式传入
&init_config 与 rmcs_board 风格保持一致；更重要的是实现 tud_umount_cb() 回调以在 USB 断开时清理传输状态：在
tud_umount_cb() 中将 transmitting_batch_ 置为 nullptr 并将 transmitted_size_
归零，或用等效清理函数重置这两个成员（确保与 Vendor() 中的初始化逻辑一致且不会引入竞态）。
- Around line 47-61: The try_transmit() path leaves transmitting_batch_ and
transmitted_size_ stale when tud_ready() is false, allowing old batch
pointers/offsets to be used after reconnect; fix by clearing state on
disconnect: in tud_umount_cb() reset transmitting_batch_ = nullptr and
transmitted_size_ = 0 (and ensure any held lock is released via
transmit_buffer_.try_unlock_and_clear()), or alternatively at the start of the
!tud_ready() branch in try_transmit() do the same reset and unlock so stale
batches cannot be reused after USB disconnect/reconnect.
---
Outside diff comments:
In `@host/src/transport/usb.cpp`:
- Around line 425-433: The throttle logic is inverted and updates
last_rx_callback_timepoint_ even when dropping packets; change the condition so
we drop when the time since last callback is less than 1s (i.e. allow callback
only when now >= last_rx_callback_timepoint_ + 1s), only update
last_rx_callback_timepoint_ when you actually call receive_callback_, and add a
brief comment on the intended semantics (throttle to max 1 callback/sec or, if
you meant to drop only the very first warm-up packet, implement that explicit
check instead); refer to last_rx_callback_timepoint_, should_drop (or remove
it), receive_callback_, and transfer->actual_length to locate and fix the code.
---
Duplicate comments:
In `@firmware/c_board/src/usb/vendor.hpp`:
- Around line 67-72: The USB write calls inside the conditional are
intentionally wrapped with core::utility::assert_debug and must be evaluated in
both debug and release; add a short inline comment above this block stating that
core::utility::assert_debug is a function (not a macro) and that
tud_vendor_n_write(0, src, target_size) and tud_vendor_n_write_zlp(0) have
required side effects and will be executed in all builds to avoid future
accidental refactors removing these calls (reference variables/functions:
target_size, src, transmitted_size_, core::utility::assert_debug,
tud_vendor_n_write, tud_vendor_n_write_zlp).
---
Nitpick comments:
In `@firmware/c_board/src/usb/interrupt_safe_buffer.hpp`:
- Around line 126-135: The calls is_locked_.test(...) and is_locked_.clear(...)
inside try_unlock_and_clear() are not atomic together and rely on the current
single-threaded main-loop assumption; add a concise comment in
try_unlock_and_clear() (referencing is_locked_.test, clear, is_locked_.clear and
try_lock) stating that this non-atomic test/clear sequence is safe only because
try_lock and try_unlock_and_clear are called exclusively from the main thread
(no RTOS/ISR concurrency), and note that if RTOS or multi-caller use is
introduced the sequence must be made atomic (e.g., by using atomic
compare_exchange or a mutex) to avoid races.
In `@firmware/c_board/src/usb/vendor.hpp`:
- Line 112: error_callback currently calls
core::utility::assert_failed_always(), which unconditionally halts on any
protocol/deserialization error; change it to implement a relaxed recovery path:
inside error_callback (the override in the class that implements the
deserializer callback), clear or reset the deserializer state (e.g., reset
parser indices/buffers), optionally increment a protocol error counter and emit
a runtime log/event, and return without asserting so the firmware continues
running and can recover from malformed/partial frames; locate the error_callback
override and replace the assert call with calls to the deserializer reset
routine (or implement one if missing), logging, and a harmless return.
In `@firmware/rmcs_board/src/usb/interrupt_safe_buffer.hpp`:
- Around line 16-141: InterruptSafeBuffer is duplicated across boards—extract
the common logic (class InterruptSafeBuffer: allocate, Batch, pop_batch,
release_batch, clear, try_lock, try_unlock_and_clear, fields in_, out_,
batches_, is_locked_) into a shared base (either a non-virtual template/CRTP
base or a concrete base class) and inject the board-specific "buffer full"
behavior via a small virtual hook or policy template parameter (e.g.,
notify_buffer_full() or a Policy::on_buffer_full()). Replace the two
near-identical classes with thin board-specific subclasses/templates that
implement the hook (one calling led::led->uplink_buffer_full(), the other no-op
or TODO), keeping all existing method names (allocate, clear, try_lock,
try_unlock_and_clear, pop_batch, Batch, release_batch) so callers need no
changes.
In `@firmware/rmcs_board/src/usb/vendor.hpp`:
- Around line 92-120: can_deserialized_callback 和 uart_deserialized_callback 的
default 分支当前直接调用 assert_failed_always()，会因未知 field ID 导致设备硬停机；请改为在默认分支记录一条包含 id
的错误/警告日志并安全返回或忽略该事件（生产固件为忽略/记录，调试构建可保留 fail-fast 行为），也可以在必要时触发非致命指示（例如
LED）并复用现有的 error_callback 路径；参考并修改
can_deserialized_callback、uart_deserialized_callback 和可能的 error_callback，替换或包裹
assert_failed_always() 为基于构建宏（例如 NDEBUG）选择记录+返回或断言的实现。
- Around line 53-89: The code recomputes max_packet_size each time in
try_transmit() by calling tud_speed_get(); change this to compute and cache the
max packet size once during USB mount/initialization (e.g., in the USB mount
callback such as tud_mount_cb or your board-specific mount handler) into a
member like usb_max_packet_size_ (or cached_max_packet_size) and have
try_transmit() use that cached value instead of calling tud_speed_get() on each
invocation; ensure the cached value is set to 512 for TUSB_SPEED_HIGH and 64
otherwise and that it is updated on re-enumeration if you handle that event.
Comment on lines
+34
to
+37
| Vendor() { | ||
| usb::usb_descriptors.init(); | ||
| core::utility::assert_always(tusb_rhport_init(0, nullptr)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tusb_rhport_init usage and CFG_TUSB_RHPORT0_MODE configuration
echo "=== Searching for tusb_rhport_init declarations and usage ==="
rg -n "tusb_rhport_init" --type=c --type=h --type=cpp -C2
echo -e "\n=== Searching for CFG_TUSB_RHPORT0_MODE configuration ==="
rg -n "CFG_TUSB_RHPORT0_MODE" -C2
echo -e "\n=== Checking TinyUSB config header location ==="
fd "tusb_config" --type=h
echo -e "\n=== Examining vendor.hpp structure ==="
fd "vendor.hpp" -x wc -l {}Repository: Alliance-Algorithm/librmcs
Length of output: 2486
🏁 Script executed:
#!/bin/bash
# Examine the target file to understand the full context
echo "=== Reading firmware/c_board/src/usb/vendor.hpp ==="
if [ -f "firmware/c_board/src/usb/vendor.hpp" ]; then
wc -l "firmware/c_board/src/usb/vendor.hpp"
cat -n "firmware/c_board/src/usb/vendor.hpp" | head -120
else
echo "File not found at firmware/c_board/src/usb/vendor.hpp"
fd "vendor.hpp" -x wc -l {}
fd "vendor.hpp" | head -3 | xargs -I {} bash -c 'echo "File: {}"; wc -l {}'
fiRepository: Alliance-Algorithm/librmcs
Length of output: 4918
🏁 Script executed:
#!/bin/bash
# Look for vendor.cpp and tud_umount_cb implementation
echo "=== Searching for tud_umount_cb callback ==="
rg -n "tud_umount_cb" --type=c --type=cpp -C5
echo -e "\n=== Searching for transmitting_batch_ reset/cleanup ==="
rg -n "transmitting_batch_" --type=c --type=cpp -C2
echo -e "\n=== Searching for tusb_init usage ==="
rg -n "tusb_init" --type=c --type=cpp -C2Repository: Alliance-Algorithm/librmcs
Length of output: 4859
补充 tud_umount_cb() 实现以重置传输状态
第 36 行调用 tusb_rhport_init(0, nullptr) 虽然依赖编译期 CFG_TUSB_RHPORT0_MODE 配置且合法，但不如 rmcs_board 中显式传递 &init_config 参数清晰。更关键的问题是：vendor.cpp 中 tud_umount_cb() 回调为空实现，导致 USB 断开连接时 transmitting_batch_ 和 transmitted_size_ 无法重置。若 USB 在传输批次中途断开，重新连接后会从中断位置继续发送旧批次，造成数据损坏。应在 tud_umount_cb() 中添加：
transmitting_batch_ = nullptr;
transmitted_size_ = 0;或通过其他机制确保 USB 断开时清理这两个状态变量。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/src/usb/vendor.hpp` around lines 34 - 37, 在 Vendor 构造中直接调用
tusb_rhport_init(0, nullptr) 可以改为显式传入 &init_config 与 rmcs_board 风格保持一致；更重要的是实现
tud_umount_cb() 回调以在 USB 断开时清理传输状态：在 tud_umount_cb() 中将 transmitting_batch_ 置为
nullptr 并将 transmitted_size_ 归零，或用等效清理函数重置这两个成员（确保与 Vendor() 中的初始化逻辑一致且不会引入竞态）。
Comment on lines
+47
to
+61
| bool try_transmit() { | ||
| if (!tud_ready()) { | ||
| transmit_buffer_.try_lock(); | ||
| return false; | ||
| } | ||
|
|
||
| if (!tud_vendor_n_write_available(0)) | ||
| return false; | ||
|
|
||
| if (!transmitting_batch_) { | ||
| transmit_buffer_.try_unlock_and_clear(); | ||
| transmitting_batch_ = transmit_buffer_.pop_batch(); | ||
| } | ||
| if (!transmitting_batch_) | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "vendor\.(hpp|cpp)" --type fRepository: Alliance-Algorithm/librmcs
Length of output: 219
🏁 Script executed:
# Check the firmware/c_board/src/usb/vendor.hpp file around lines 47-61
cat -n firmware/c_board/src/usb/vendor.hpp | head -80Repository: Alliance-Algorithm/librmcs
Length of output: 3115
🏁 Script executed:
# Continue reading the file to see class members and more context
tail -n +80 firmware/c_board/src/usb/vendor.hpp | head -50Repository: Alliance-Algorithm/librmcs
Length of output: 1582
🏁 Script executed:
# Now check the vendor.cpp file for tud_umount_cb and other USB callbacks
cat -n firmware/c_board/src/usb/vendor.cppRepository: Alliance-Algorithm/librmcs
Length of output: 1096
🏁 Script executed:
# Check if there are any other USB callback handlers or state reset mechanisms
rg -n "transmitting_batch_|transmitted_size_|tud_umount_cb|tud_mount_cb" firmware/c_board/src/usb/ -A2 -B2Repository: Alliance-Algorithm/librmcs
Length of output: 2790
🏁 Script executed:
# Check InterruptSafeBuffer implementation to understand the lifecycle
cat -n firmware/c_board/src/usb/interrupt_safe_buffer.hpp | head -100Repository: Alliance-Algorithm/librmcs
Length of output: 3962
🏁 Script executed:
# Continue reading InterruptSafeBuffer to see full implementation
tail -n +100 firmware/c_board/src/usb/interrupt_safe_buffer.hppRepository: Alliance-Algorithm/librmcs
Length of output: 1514
🏁 Script executed:
# Check if there's any other mechanism that might reset transmitting_batch_ on USB state changes
rg -n "tud_mount_cb|tud_umount_cb|tud_suspend_cb|tud_resume_cb" firmware/c_board/src/usb/ -A5Repository: Alliance-Algorithm/librmcs
Length of output: 699
USB 断开时未清理批次传输中间状态，重连后可能发送残旧数据
当 !tud_ready()（USB 断开）且 transmitting_batch_ 非空时，代码仅调用 transmit_buffer_.try_lock() 返回 false，但 transmitting_batch_ 和 transmitted_size_ 不会被重置。重连后，第 56 行 !transmitting_batch_ 判断为假，try_unlock_and_clear() 永不执行，导致旧批次指针和传输偏移量继续保留。try_transmit() 随后使用已过期的批次指针继续从旧偏移发送数据，可能将不完整或已被重用的缓冲区内容发送到主机。
firmware/c_board/src/usb/vendor.cpp 中的 tud_umount_cb() 为空实现，无法在 USB 断开时清理这两个成员变量。应在 tud_umount_cb() 中重置 transmitting_batch_ 为 nullptr 和 transmitted_size_ 为 0，或在 try_transmit() 首次进入 !tud_ready() 分支时直接清理。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/src/usb/vendor.hpp` around lines 47 - 61, The try_transmit()
path leaves transmitting_batch_ and transmitted_size_ stale when tud_ready() is
false, allowing old batch pointers/offsets to be used after reconnect; fix by
clearing state on disconnect: in tud_umount_cb() reset transmitting_batch_ =
nullptr and transmitted_size_ = 0 (and ensure any held lock is released via
transmit_buffer_.try_unlock_and_clear()), or alternatively at the start of the
!tud_ready() branch in try_transmit() do the same reset and unlock so stale
batches cannot be reused after USB disconnect/reconnect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the ST USB Device Library (CDC middleware) with TinyUSB, following the migration path outlined in commit 2d6076d.
What changed
Not yet completed
USB 堆栈从 HAL CDC 迁移至 TinyUSB（摘要更新）
本 PR 将固件的 USB 实现从 ST 官方 USB Device（CDC 中间件）全面迁移到 TinyUSB，并集成 DWC2 portable driver（作为子模块）。构建、HAL 配置、描述符与应用层均完成相应重构，旧的 STM32 USB_Device 中间件及其大量源文件被移除。
主要变更点
依赖与构建
移除 ST USB 中间件
HAL / CubeMX 与中断路径改动
TinyUSB 描述符与设备身份
供应商类驱动重写
缓冲与中断安全
主机端行为调整
已知未完成项与风险
影响范围
变更量化（近似）
建议（合并前考虑）