Fix stm32f723disco host/cdc_msc_hid HIL: UART RX starvation + DWC2 DMA split-IN NAK storm#3677
Open
hathach wants to merge 11 commits into
Open
Fix stm32f723disco host/cdc_msc_hid HIL: UART RX starvation + DWC2 DMA split-IN NAK storm#3677hathach wants to merge 11 commits into
hathach wants to merge 11 commits into
Conversation
The F7 USART has no hardware RX FIFO, so its single-byte RDR relies entirely on the RXNE interrupt. board_init() pinned that ISR at the lowest NVIC priority while the USB OTG IRQ ran at the default priority 0, so under heavy HS host traffic the RX ISR was starved, RDR overran (ORE), and ~85% of 115200-baud console bytes were dropped. This showed up as the host/cdc_msc_hid HIL CDC echo mismatch on stm32f723disco. Raise the USART RX IRQ to priority 0 so it preempts the USB IRQ (the ISR only reads RDR into a lock-free fifo and makes no RTOS call, so this is safe even under FreeRTOS), demote OTG_FS/HS to priority 1 in the bare-metal path, and grow the RX ring buffer 32 -> 256 B to absorb a full forwarding burst when the main loop briefly stalls. Validated on the CI HIL rig: host/cdc_msc_hid default variant now passes 5/5 (was 0%), device/cdc_msc unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…storm In buffer DMA mode, handle_channel_in_dma() re-enabled a split bulk/control IN channel immediately on every NAK. When a full-speed device behind a hub keeps NAKing an idle bulk IN (e.g. a CDC stream polled with no data to send), this becomes an unthrottled start-split -> complete-split -> NAK loop running at full ISR rate. It starves the task context, so the host can no longer forward data to the device, the device never has anything to return, and the endpoint wedges permanently. This stalled host/cdc_msc_hid on stm32f723disco (HS host + hub) in the DMA build, while the slave build was fine. Mirror the slave path, which the Databook also sanctions (p73: channel disable is allowed for NAK on split channels; 3.5 "Halting a Channel"): on a split IN NAK, disable the channel and re-issue the start-split on the resulting halt interrupt instead of re-enabling it immediately. The disable's arbitration/flush latency throttles the poll and yields to the task, with no frame deferral so split timing is preserved. HIL host/cdc_msc_hid on stm32f723disco DMA build now passes. The change is confined to the DMA split IN NAK path, so the slave build and device mode are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 0c1d8dc90c
ℹ️ 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 addresses a HIL failure on stm32f723disco for the host/cdc_msc_hid example by (1) preventing STM32F7 UART RX starvation under heavy USB HS host interrupt load and (2) throttling a DWC2 buffer-DMA split bulk/control IN NAK retry path to avoid an interrupt storm when polling an idle full-speed device behind a hub.
Changes:
- DWC2 HCD (DMA mode): disable split IN channels on NAK and re-arm on HALTED to avoid unthrottled NAK retry ISR loops.
- STM32F7 BSP: increase UART RX ring buffer size and adjust NVIC priorities so UART RX can preempt USB OTG in bare-metal builds.
- Documentation: fix a path in
AGENTS.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/portable/synopsys/dwc2/hcd_dwc2.c |
Adds split-IN NAK throttling logic in DMA mode via channel disable + re-arm on HALTED. |
hw/bsp/stm32f7/family.c |
Adjusts UART RX buffering and interrupt priorities to prevent RX overrun under heavy USB host traffic. |
AGENTS.md |
Updates a reference path string. |
💡 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 sizeNo entries. No changes
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codespell flagged ORE (USART overrun-error flag) as a typo; it is the correct register flag name (USART_ISR_ORE). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…demotion to UART_ID Copilot + Codex flagged that the nak_disabled halt path could re-arm a channel during endpoint close (edpt_close also disables the channel), orphaning a DMA transfer. Complete the transfer instead of re-arming when xfer->closing is set. Copilot also noted the bare-metal OTG priority demotion applied to all stm32f7 boards; scope it to boards with a UART console (UART_ID). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| stlinkv3mini/hid_composite_freertos | — | — | — | 4,492 → 4,716 (+224) | 6,456 → 6,680 (+224) | +3.5% |
| stlinkv3mini/audio_test_freertos | — | — | — | 5,848 → 6,072 (+224) | 7,840 → 8,064 (+224) | +2.9% |
| stlinkv3mini/midi_test_freertos | — | — | — | 6,220 → 6,444 (+224) | 8,192 → 8,416 (+224) | +2.7% |
| stlinkv3mini/audio_4_channel_mic_freertos | — | — | — | 7,488 → 7,712 (+224) | 9,480 → 9,704 (+224) | +2.4% |
| stlinkv3mini/board_test | 8,412 → 8,420 (+8) | — | — | 388 → 612 (+224) | 11,612 → 11,844 (+232) | +2.0% |
| stlinkv3mini/dfu_runtime | 13,916 → 13,924 (+8) | — | — | 788 → 1,012 (+224) | 17,796 → 18,028 (+232) | +1.3% |
| stlinkv3mini/hid_generic_inout | 14,984 → 14,992 (+8) | — | — | 992 → 1,216 (+224) | 18,900 → 19,132 (+232) | +1.2% |
| stlinkv3mini/cdc_msc_freertos | — | — | — | 8,380 → 8,604 (+224) | 18,544 → 18,768 (+224) | +1.2% |
| stlinkv3mini/hid_multiple_interface | 15,628 → 15,636 (+8) | — | — | 868 → 1,092 (+224) | 19,588 → 19,820 (+232) | +1.2% |
| stlinkv3mini/hid_boot_interface | 15,688 → 15,696 (+8) | — | — | 868 → 1,092 (+224) | 19,604 → 19,836 (+232) | +1.2% |
Collaborator
|
I think it's worth to add a max NAK retry limit per frame like fsdev, especially on slave mode CPU is very high. |
Owner
Author
seem like I found one of the hil issue 😄 |
Collaborator
Thats a lot haha. |
…-uart-rx-priority
Owner
Author
half the device is actually jlink/flasher :( |
Move the USART RX NVIC priority into the OPT_OS_NONE and FreeRTOS init branches alongside the OTG demotion, so the relative ordering is set in one place. Under FreeRTOS, OTG drops to max-syscall+1 so the USART RX ISR (at max-syscall) preempts it; bare-metal keeps OTG=1 / USART=0. Guard the USARTn_IRQn references with UART_ID since that symbol is only defined for boards with a UART console; OTG demotion stays unconditional (all stm32f7 boards define UART_ID, so behavior is unchanged). Validated on stm32f723disco HIL (both flag variants): Total failed: 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In buffer-DMA mode a split bulk-OUT XactErr was retried immediately, so 3 back-to-back failures could exhaust HCD_XFER_ERROR_MAX and fail the transfer, dropping a data segment (a rare CDC-echo-truncation mode for a full-speed device behind a hub). Mirror the slave path: on a split-OUT XactErr, rewind the buffer pointers, channel_disable() the channel, and re-issue the start-split on the resulting halt, so the disable's flush/arbitration latency gives the hub's transaction translator a recovery gap instead of immediately re-firing the failing transaction. Scoped to split (hcsplt.split_en); non-split XactErr keeps the immediate re-init retry per Programming Guide v4.20a 5.1.1.2. The nak_disabled flag is generalized to retry_disabled, shared by the IN-NAK and OUT-XactErr throttles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
handle_channel_out_dma() only handled NAK when bundled with XACT_ERR; a pure split-OUT NAK left the channel halted and the transfer stalled, dropping data. This was the dominant cause of intermittent CDC echo truncation for a full-speed device behind a hub (isolation on stm32f723disco HIL, raw -r1: NAK-fix-only 0/10 vs XactErr-fix-only 2/10). Per the DWC2 Programming Guide v4.20a 5.1.4.2, on a split OUT NAK rewind the buffer pointers and retry the start-split. Scoped to split (hcsplt.split_en): non-split OUT NAK is auto-handled by the core (5.1.2.2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
hathach
force-pushed
the
claude/stm32f7-host-uart-rx-priority
branch
from
3cf4d1f to
494f602
Compare
Contributor
Code reviewFound 1 style issue: missing spaces around the binary + operator in configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY+1 (lines 159-160 of the stm32f7 family.c change). The project clang-format config (LLVM base style) requires spaces around binary operators, and pre-commit would flag this. Suggested fix: configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY + 1 (with spaces). No bugs found. Checked for CLAUDE.md compliance. |
Contributor
Code review1 issue found (style/clang-format) Missing spaces around binary Current: NVIC_SetPriority(OTG_FS_IRQn, configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY+1);
NVIC_SetPriority(OTG_HS_IRQn, configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY+1);Suggested: NVIC_SetPriority(OTG_FS_IRQn, configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY + 1);
NVIC_SetPriority(OTG_HS_IRQn, configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY + 1);The project uses LLVM-based clang-format which requires spaces around binary operators. Running No bugs found. Checked for CLAUDE.md compliance. |
…ions channel_disable() is a no-op for periodic split channels, so throttling a periodic split-OUT XactErr through it would never raise the halt that re-arms retry_disabled, wedging the channel. Guard the throttle with !channel_is_periodic (matching the IN NAK throttle); periodic and non-split fall back to the immediate re-init retry. No effect on the bulk CDC path (non-periodic). Also fix references (Databook -> Programming Guide v4.20a 3.5 "Halting a Channel" p73; 5.1.1.2 -> 5.1.2.3 for non-split OUT) and tighten the throttle comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hardware-in-the-loop (HIL) Test Reporthfp-iar
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run hfp.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run tinyusb.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run |
Owner
Author
will try to do that with follow up PR. I am trying to get this merge while fighting with hil on ci rig |
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.
Fixes the
host/cdc_msc_hidHIL CDC-echo failure on stm32f723disco (HS host with a hub; CDC+MSC behind it, so the FS CH9102 CDC is reached via USB split transactions). Two independent bugs:1. stm32f7 host UART RX byte loss (
7ae26d7)The F7 USART has no hardware RX FIFO, so its single-byte
RDRrelies entirely on the RXNE interrupt.board_init()pinned that ISR at the lowest NVIC priority while the USB OTG IRQ ran at the default highest priority (0), so under heavy HS host traffic the RX ISR was starved,RDRoverran (ORE), and ~85% of 115200-baud console bytes were dropped — surfacing as the CDC echo mismatch.RDRinto a lock-free fifo and makes no RTOS call, so this is safe even under FreeRTOS).2. DWC2 DMA-mode split bulk-IN NAK storm (
722f13c)In buffer-DMA mode,
handle_channel_in_dma()re-enabled a split bulk/control IN channel immediately on every NAK. When a full-speed device behind a hub keeps NAKing an idle bulk IN (a CDC stream polled with no data), this becomes an unthrottled start-split → complete-split → NAK ISR loop that starves the task context, so the host can no longer forward data, the device never echoes, and the endpoint wedges permanently. The slave build was unaffected because itchannel_disable()s on NAK and retries on the separate halt interrupt.Mirror the slave path, which the Synopsys DWC2 Programming Guide v4.20a also sanctions (p73: channel disable is permitted for NAK on split channels; §3.5 "Halting a Channel"): on a split IN NAK, disable the channel and re-issue the start-split on the resulting halt interrupt. The disable's arbitration/flush latency throttles the poll and yields to the task, with no frame deferral, so split timing is preserved. The change is confined to the DMA split IN NAK path.
Validation (CI HIL rig)
Full board suite, both flag variants (default/slave and
CFG_TUH_DWC2_DMA_ENABLE), rebuilt fresh — 5/5 runs Total failed: 0, no device or host regressions.Throughput unaffected / improved:
msc_file_explorer, HS): 18.1 MB/s slave, 19.7 MB/s DMA(Also includes a one-line doc path fix in
AGENTS.md.)🤖 Generated with Claude Code