Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99b721db60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts UART/stdio behavior and host CDC example logic to improve reliability of CDC echo testing on fast MCUs (notably IMXRT), and adds configurability to HIL retry behavior.
Changes:
- Make some BSP UART/stdio helpers non-blocking-friendly (e.g.,
board_uart_write()on IMXRT;board_putchar()now returnsint). - Add retry/looping behavior around UART writes (
sys_write) and update host CDC example/config to better handle rapid transfers. - Update HIL runner to make retry count configurable and shorten per-chunk CDC echo wait time.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/hil/hil_test.py | Adds CLI-configurable retry count and adjusts CDC echo wait timing. |
| hw/bsp/board.c | Changes sys_write() to retry partial/non-blocking writes; updates board_putchar() signature/behavior. |
| hw/bsp/board_api.h | Updates UART and board_putchar() API comments/signature to match new behavior. |
| hw/bsp/imxrt/family.c | Makes IMXRT UART write path return partial writes (non-blocking style). |
| hw/bsp/rp2040/family.c | Updates board_putchar() to return int consistently with API. |
| hw/bsp/espressif/boards/family.c | Updates board_putchar() to return int consistently with API. |
| examples/host/cdc_msc_hid/src/tusb_config.h | Increases host task queue size and reduces CDC instance count for this example. |
| examples/host/cdc_msc_hid/src/cdc_app.c | Refactors CDC console<->USBH forwarding to better cope with fast transfers. |
| examples/host/cdc_msc_hid_freertos/src/cdc_app.c | Renames console input helper for consistency. |
| examples/host/msc_file_explorer/src/main.c | Removes large comment block / unused include. |
| examples/CMakeLists.txt | Removes dead commented-out metrics command. |
Comments suppressed due to low confidence (1)
examples/host/cdc_msc_hid_freertos/src/cdc_app.c:62
board_getchar()is documented/implemented as returning a negative value when no character is available. Usingch <= 0treats NUL (0) as "no input" and is inconsistent with the non-FreeRTOS variant in this PR (which usesch < 0). Consider changing this check toch < 0for correctness and consistency.
static size_t console_read(uint8_t *buf, size_t bufsize) {
size_t count = 0;
while (count < bufsize) {
int ch = board_getchar();
if (ch <= 0) {
break;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| lpcxpresso1769/board_test | 3,544 → 3,904 (+360) | — | — | — | 3,916 → 4,276 (+360) | +9.2% |
| raspberrypi_cm4/board_test | 49,092 → 49,156 (+64) | 1,545 → 5,577 (+4,032) | — | — | 50,637 → 54,733 (+4,096) | +8.1% |
| stm32c071nucleo/board_test | 7,324 → 7,796 (+472) | 144 → 160 (+16) | — | — | 7,660 → 8,148 (+488) | +6.4% |
| stm32c071nucleo/dfu_runtime | 11,392 → 12,008 (+616) | 488 → 504 (+16) | — | — | 12,072 → 12,704 (+632) | +5.2% |
| stlinkv3mini/board_test | 7,896 → 8,412 (+516) | — | — | 344 → 388 (+44) | 10,732 → 11,288 (+556) | +5.2% |
| stm32g0b1nucleo/board_test | 9,360 → 9,832 (+472) | 140 → 156 (+16) | — | — | 9,696 → 10,184 (+488) | +5.0% |
| stm32c071nucleo/hid_generic_inout | 12,420 → 13,036 (+616) | 316 → 332 (+16) | — | — | 12,928 → 13,560 (+632) | +4.9% |
| stm32f070rbnucleo/board_test | 8,616 → 9,112 (+496) | — | 104 → 240 (+136) | 480 → 388 (-92) | 11,460 → 12,004 (+544) | +4.7% |
| stm32c071nucleo/hid_multiple_interface | 13,048 → 13,664 (+616) | 488 → 504 (+16) | — | — | 13,728 → 14,360 (+632) | +4.6% |
| stm32c071nucleo/hid_boot_interface | 13,104 → 13,720 (+616) | 444 → 460 (+16) | — | — | 13,740 → 14,372 (+632) | +4.6% |
…stency across all families
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
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.
Summary
Fix HIL CDC echo test failures caused by blocking board_uart_write() / printf() in USB callbacks, which stalls the main loop and
causes UART RX FIFO overrun. Also standardize board_uart_read()/board_uart_write() as non-blocking across all BSP families.
Root cause
The host CDC echo example (cdc_msc_hid) forwards USB?UART using printf() inside tuh_cdc_rx_cb(). printf() calls
board_uart_write() which was blocking (waits for each byte to transmit at 115200 baud). During this ~3ms block, incoming UART
data overflows the hardware RX FIFO, losing bytes. The test sees truncated echo data.
Changes
Host CDC echo app (examples/host/cdc_msc_hid/src/cdc_app.c):
BSP board_uart_write() ? non-blocking for all families:
#if/#elif ? eliminates clang -Wunreachable-code errors
BSP board.c sys_write():