Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new HIL host-side CDC test to validate CDC enumeration + data path (via host cdc_msc_hid example), and updates HIL configuration / BSP helpers to better support the test environment.
Changes:
- Add a new HIL test
host/cdc_msc_hidthat waits for CDC mount and performs a UART-console-driven CDC echo check. - Extend HIL board config (
tinyusb.json)dev_attachedentries with anis_cdcflag to identify CDC-capable attached devices. - Improve STM32H7 BSP support by implementing
board_uart_read()and adding I2C retry logic for the STM32H743EVAL MFX bus access.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/hil/tinyusb.json | Adds is_cdc metadata to attached-device entries for HIL host CDC testing. |
| test/hil/hil_test.py | Introduces the new host/cdc_msc_hid HIL test and registers it in the host test list. |
| hw/bsp/stm32h7/family.c | Implements board_uart_read() for UART-backed console input on STM32H7. |
| hw/bsp/stm32h7/boards/stm32h743eval/board.h | Adds retry/delay logic for I2C mem read/write used by the MFX IO expander. |
| AGENTS.md | Removes a stray blank line in documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # reset device to catch mount messages | ||
| ret = globals()[f'reset_{flasher["name"].lower()}'](board) | ||
| assert ret.returncode == 0, 'Failed to reset device' | ||
|
|
| ser.timeout = 0.1 | ||
|
|
||
| # reset device to catch mount messages | ||
| ret = globals()[f'reset_{flasher["name"].lower()}'](board) | ||
| assert ret.returncode == 0, 'Failed to reset device' | ||
|
|
||
| # Wait for CDC mounted message | ||
| data = b'' | ||
| timeout = ENUM_TIMEOUT | ||
| while timeout > 0: | ||
| new_data = ser.read(ser.in_waiting or 1) | ||
| if new_data: | ||
| data += new_data | ||
| if b'CDC Interface is mounted' in data: | ||
| break | ||
| time.sleep(0.1) | ||
| timeout -= 0.1 | ||
| assert b'CDC Interface is mounted' in data, 'CDC device not mounted on host' | ||
|
|
||
| # Lookup serial chip name from vid_pid | ||
| vid_pid_name = { | ||
| '0403_6001': 'FTDI', '0403_6010': 'FTDI', '0403_6011': 'FTDI', '0403_6014': 'FTDI', | ||
| '10c4_ea60': 'CP210x', '10c4_ea70': 'CP210x', | ||
| '067b_2303': 'PL2303', '067b_23a3': 'PL2303', | ||
| '1a86_7523': 'CH340', '1a86_7522': 'CH340', | ||
| '1a86_55d3': 'CH9102', '1a86_55d4': 'CH9102', | ||
| } | ||
| dev = cdc_devs[0] | ||
| chip_name = vid_pid_name.get(dev['vid_pid'], dev['vid_pid']) | ||
| for l in data.decode('utf-8', errors='ignore').splitlines(): | ||
| if 'CDC Interface is mounted' in l: | ||
| print(f'\r\n {chip_name}: {l} ', end='') | ||
|
|
||
| # CDC echo test via flasher serial | ||
| time.sleep(2) | ||
| ser.reset_input_buffer() | ||
|
|
||
| def rand_ascii(length): | ||
| return "".join(random.choices(string.ascii_letters + string.digits, k=length)).encode("ascii") | ||
|
|
||
| sizes = [8, 32, 64, 128] | ||
| for size in sizes: | ||
| test_data = rand_ascii(size) | ||
| ser.reset_input_buffer() | ||
|
|
||
| # Write byte-by-byte with delay to avoid UART overrun | ||
| for b in test_data: | ||
| ser.write(bytes([b])) | ||
| ser.flush() | ||
| time.sleep(0.001) | ||
|
|
||
| # Read echo back with timeout | ||
| echo = b'' | ||
| t = 5.0 | ||
| while t > 0 and len(echo) < size: | ||
| rd = ser.read(max(1, ser.in_waiting)) | ||
| if rd: | ||
| echo += rd | ||
| time.sleep(0.05) | ||
| t -= 0.05 | ||
| assert echo == test_data, (f'CDC echo wrong data ({size} bytes):\n' | ||
| f' expected: {test_data}\n received: {echo}') | ||
|
|
||
| ser.close() | ||
|
|
||
|
|
![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: a3b2b42176
ℹ️ 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".
| ] | ||
|
|
||
| host_test = [ | ||
| 'host/cdc_msc_hid', |
There was a problem hiding this comment.
Adding host/cdc_msc_hid to the default host test list makes it run on every host: true board, but at least one configured board (stm32f723disco in test/hil/tinyusb.json) routes through hw/bsp/stm32f7/family.c where board_uart_read() is a stub that always returns 0; in that setup board_getchar() never receives injected bytes, so the new CDC echo assertion will fail deterministically. This should be opt-in per board (or conditioned on RX support) to avoid breaking HIL runs.
Useful? React with 👍 / 👎.
| if (HAL_OK == HAL_I2C_Mem_Read(&i2c_handle, DevAddr, Reg, I2C_MEMADD_SIZE_8BIT, pData, Length, 10000)) { | ||
| return 0; | ||
| } | ||
| HAL_Delay(10); |
There was a problem hiding this comment.
Using HAL_Delay(10) in this retry loop can deadlock STM32H7 FreeRTOS startups: board_init() disables SysTick before calling board_init2() in hw/bsp/stm32h7/family.c, so if the first I2C access fails here, HAL_Delay() has no advancing tick source and blocks indefinitely instead of retrying. This retry path needs a delay mechanism that works before the scheduler/tick is running.
Useful? React with 👍 / 👎.
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 sizeNo entries. Changes <1% in sizeNo entries. No changes
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| stm32h743eval/board_test | 17,428 → 17,540 (+112) | — | — | — | 18,160 → 18,272 (+112) | +0.6% |
| stlinkv3mini/board_test | 7,852 → 7,896 (+44) | — | — | — | 8,384 → 8,428 (+44) | +0.5% |
| stm32h743eval/msc_file_explorer | 46,360 → 46,536 (+176) | 3,020 → 3,056 (+36) | — | — | 50,052 → 50,264 (+212) | +0.4% |
| stm32h743eval/dfu | 26,904 → 27,016 (+112) | — | — | — | 28,340 → 28,452 (+112) | +0.4% |
| stlinkv3mini/msc_file_explorer | 36,896 → 37,004 (+108) | 3,004 → 3,040 (+36) | — | — | 40,388 → 40,532 (+144) | +0.4% |
| stm32h743eval/hid_controller | 31,744 → 31,856 (+112) | — | — | — | 33,064 → 33,176 (+112) | +0.3% |
| lpcxpresso1769/msc_file_explorer | 31,156 → 31,260 (+104) | — | — | — | 31,172 → 31,276 (+104) | +0.3% |
| stm32h743eval/device_info | 32,440 → 32,552 (+112) | — | — | — | 33,860 → 33,972 (+112) | +0.3% |
| stm32h743eval/bare_api | 33,024 → 33,136 (+112) | — | — | — | 34,536 → 34,648 (+112) | +0.3% |
| stm32h743eval/midi_rx | 33,664 → 33,776 (+112) | — | — | — | 34,752 → 34,864 (+112) | +0.3% |
Use separate endpoint numbers (EP1 OUT, EP2 IN) on MCUs with shared FIFO that cannot support the same endpoint number in both directions. Also add missing static qualifier to print_musb_info(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Added new tests (test_host_cdc_msc_hid, test_device_midi_test, etc.) to verify enhanced CDC, MSC, LUN, MIDI, and HID features.