From cb224400931b7fbc3477a87a258c0602092abe6b Mon Sep 17 00:00:00 2001 From: hathach Date: Fri, 17 Jul 2026 17:58:25 +0700 Subject: dcd_lpc17_40: address review findings in the iso paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a second max-effort review of the branch: - Drop the dead TUSB_XFER_ISOCHRONOUS case in dcd_edpt_open: iso endpoints are armed via dcd_edpt_iso_alloc/activate (TUP_DCD_EDPT_ISO_ALLOC is defined for this IP), never through dcd_edpt_open, so the case and its dd->isochronous assignment were unreachable and asserted a false invariant. Only bulk/interrupt reach the switch now. - Extend the iso compile gate to the classes that actually arm an iso endpoint: DCD_ISO_ENABLED now includes CFG_TUD_BTH (bth_device.c opens an iso voice endpoint). Without it a BTH build would compile the iso machinery out and fail SET_INTERFACE at runtime. - Un-skip LPC175X_6X in the usbtest example: it shares dcd_lpc17_40.c with LPC40XX verbatim, so the "DCD has no isochronous support" skip reason no longer holds. Build-verified for lpcxpresso1769 (previously blocked by the skip). - TU_ATTR_UNUSED on the ep_id_is_iso helper: every caller is under #if DCD_ISO_ENABLED, so non-iso builds don't reference it and clang's -Wunused-function (fatal in CI) rejected the build — gcc stays quiet. Verified with the full lpc17 and lpc40 example sets under arm-clang. A fifth finding — bounding control_ep_read's PACKET_READY spin with a timeout — was implemented and REVERTED: a naive 100k-iteration bound fires on legitimately-slow control reads and intermittently drops the device (hardware-proven by interleaved A/B testing against the pre-fix binary). The infinite wait is retained; the read is only reached once out_received/ out_queued signal data is present, so the theoretical IRQ-off hang is not reachable in practice. Re-verified on ea4088_quickstart: usbtest 30/30 (repeated) + HIL 14/14. --- examples/device/usbtest/skip.txt | 1 - src/portable/nxp/lpc17_40/dcd_lpc17_40.c | 24 +++++++++++------------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/examples/device/usbtest/skip.txt b/examples/device/usbtest/skip.txt index e789c4b91..792404fe4 100644 --- a/examples/device/usbtest/skip.txt +++ b/examples/device/usbtest/skip.txt @@ -4,7 +4,6 @@ mcu:SAMD11 # DCD has no isochronous support (dcd_edpt_iso_alloc refuses), tier-4 cannot enumerate: mcu:CXD56 mcu:FT90X -mcu:LPC175X_6X mcu:NUC100 mcu:NUC120 mcu:NUC505 diff --git a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c index 6dc2b017c..b577d0e9f 100644 --- a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c +++ b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c @@ -20,8 +20,10 @@ #define DCD_ENDPOINT_MAX 32 // The iso machinery (5th DD word + packet-size memory) costs USB RAM on every build; -// compile it only when a class that can open an iso endpoint is enabled. -#define DCD_ISO_ENABLED (CFG_TUD_AUDIO || CFG_TUD_VIDEO || CFG_TUD_VENDOR) +// compile it only when a class that can open an iso endpoint is enabled. Keep this in +// sync with the classes that actually arm an iso endpoint: audio, video, BTH (voice), +// and vendor (its optional CFG_TUD_VENDOR_EP_ISO_* endpoints, exercised by usbtest). +#define DCD_ISO_ENABLED (CFG_TUD_AUDIO || CFG_TUD_VIDEO || CFG_TUD_VENDOR || CFG_TUD_BTH) typedef struct TU_ATTR_ALIGNED(4) { @@ -64,7 +66,9 @@ TU_VERIFY_STATIC( sizeof(dma_desc_t) == (DCD_ISO_ENABLED ? 20 : 16), "size is no // Hardware fixes endpoint type by number: 3, 6, 9, 12 are the iso-capable ones. // Constant per ep_id (= 2*epnum + dir) — unlike dd->isochronous, which dcd_edpt_xfer // transiently zeroes while rebuilding the DD, this is safe to dispatch on from the ISR. -TU_ATTR_ALWAYS_INLINE static inline bool ep_id_is_iso(uint8_t ep_id) { +// TU_ATTR_UNUSED: every caller is under #if DCD_ISO_ENABLED, so non-iso builds don't +// reference it and clang -Wunused-function (fatal) would otherwise reject the build. +TU_ATTR_UNUSED TU_ATTR_ALWAYS_INLINE static inline bool ep_id_is_iso(uint8_t ep_id) { uint8_t const epnum = (uint8_t)(ep_id >> 1); return (epnum % 3) == 0 && (epnum != 0) && (epnum != 15); } @@ -360,8 +364,9 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) uint8_t const epnum = tu_edpt_number(p_endpoint_desc->bEndpointAddress); uint8_t const ep_id = ep_addr2idx(p_endpoint_desc->bEndpointAddress); - // Endpoint type is fixed to endpoint number - // 1: interrupt, 2: Bulk, 3: Iso and so on + // Endpoint type is fixed to endpoint number (1 interrupt, 2 bulk, 3 iso, ...). + // Iso endpoints are armed via dcd_edpt_iso_alloc/activate, never through here + // (TUP_DCD_EDPT_ISO_ALLOC is defined for this IP), so only bulk/interrupt land here. switch ( p_endpoint_desc->bmAttributes.xfer ) { case TUSB_XFER_INTERRUPT: @@ -372,11 +377,6 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) TU_ASSERT((epnum % 3) == 2 || (epnum == 15)); break; - case TUSB_XFER_ISOCHRONOUS: - // iso machinery is compiled out when no iso-capable class is enabled - TU_ASSERT(DCD_ISO_ENABLED && (epnum % 3) == 0 && (epnum != 0) && (epnum != 15)); - break; - default: break; } @@ -387,9 +387,7 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) //------------- first DD prepare -------------// dma_desc_t* const dd = &_dcd.dd[ep_id]; - tu_memclr(dd, sizeof(dma_desc_t)); - - dd->isochronous = (p_endpoint_desc->bmAttributes.xfer == TUSB_XFER_ISOCHRONOUS) ? 1 : 0; + tu_memclr(dd, sizeof(dma_desc_t)); // non-iso: isochronous stays 0 dd->max_packet_size = ep_size; dd->retired = 1; // invalid at first -- cgit v1.3.1