From 372520365ecf1b908d6ada27ed50ae7f108cd876 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 30 Nov 2022 15:54:36 -0600 Subject: [PATCH 1/5] delay clearing WKUPIF This prevents some spurious wakeup interrupts. Mostly, it helps keep debug counters looking reasonable given what's actually happening on the bus. --- .../GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c index b8639b86..d8357cbf 100644 --- a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c +++ b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c @@ -204,9 +204,6 @@ void usbd_isr (void) } if (INTF_WKUPIF & int_flag) { - /* clear wakeup interrupt flag in INTF */ - CLR(WKUPIF); - /* restore the old cur_status */ udev->cur_status = udev->backup_status; @@ -226,6 +223,9 @@ void usbd_isr (void) resume_mcu(udev); } #endif /* LPM_ENABLED */ + + /* clear wakeup interrupt flag in INTF */ + CLR(WKUPIF); } if (INTF_SPSIF & int_flag) { From f654a44c5860a5c72586fa73dc61702a5c3d749f Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 30 Nov 2022 15:51:07 -0600 Subject: [PATCH 2/5] add USB error logging hooks Add error logging hooks for USB. This is conditionalized on -DUSBCORE_ERROR_HOOKS, because they can cause slowdowns in USB environments with lots of errors. --- cores/arduino/USBCore.cpp | 26 ++++++++++++++ .../device/Include/usbd_core.h | 4 +++ .../usbd/Source/usbd_lld_core.c | 36 +++++++++++++++++++ .../usbd/Source/usbd_lld_int.c | 9 +++++ 4 files changed, 75 insertions(+) diff --git a/cores/arduino/USBCore.cpp b/cores/arduino/USBCore.cpp index a5e22c7c..79b2e272 100644 --- a/cores/arduino/USBCore.cpp +++ b/cores/arduino/USBCore.cpp @@ -554,6 +554,27 @@ void USBCore_::setResetHook(void (*hook)()) resetHook = hook; } +#ifdef USBD_ERROR_HOOKS +static void handleSetupErr(uint8_t len) +{ + USBCore().logEP('!', 0, '<', len); + usbd_ep_ram *btable_ep = (usbd_ep_ram *)(USBD_RAM + 2 * (BTABLE_OFFSET & 0xFFF8)); + uint32_t *p = (uint32_t *)(USBD_RAM + 2U * EP0_RX_ADDR); + uint16_t buf[USBD_EP0_MAX_SIZE/2]; + for (int i = 0; i < USBD_EP0_MAX_SIZE/2; i++) { + buf[i] = p[i]; + } + USBCore().hexDump('!', (uint8_t *)buf, USBD_EP0_MAX_SIZE); + auto bytes = (uint16_t)(btable_ep[0].rx_count & EPRCNT_CNT); + USBCore().logEP('.', 0, '!', bytes); +} + +static void handleErr() +{ + USBCore().logStatus("Error"); +} +#endif + void (*oldSuspendHandler)(); void handleSuspend() { @@ -585,6 +606,11 @@ USBCore_::USBCore_() oldResetHandler = usbd.drv_handler->ep_reset; usbd.drv_handler->ep_reset = handleReset; +#ifdef USBD_ERROR_HOOKS + usbd.drv_handler->err = handleErr; + usbd.drv_handler->setup_err = handleSetupErr; +#endif + oldSuspendHandler = usbd.drv_handler->suspend; usbd.drv_handler->suspend = handleSuspend; diff --git a/system/GD32F30x_firmware/GD32F30x_usbd_library/device/Include/usbd_core.h b/system/GD32F30x_firmware/GD32F30x_usbd_library/device/Include/usbd_core.h index 9d02b98a..42c2d676 100644 --- a/system/GD32F30x_firmware/GD32F30x_usbd_library/device/Include/usbd_core.h +++ b/system/GD32F30x_firmware/GD32F30x_usbd_library/device/Include/usbd_core.h @@ -219,6 +219,10 @@ struct _usb_handler void (*suspend) (void); void (*suspend_leave) (void); void (*resume) (usb_dev *udev); +#ifdef USBD_ERROR_HOOKS + void (*err) (void); + void (*setup_err) (uint8_t len); +#endif void (*ep_reset) (usb_dev *udev); void (*ep_setup) (usb_dev *udev, uint8_t buf_kind, uint32_t buf_addr, const usb_desc_ep *ep_desc); diff --git a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_core.c b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_core.c index 663c897b..98ff137c 100644 --- a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_core.c +++ b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_core.c @@ -71,6 +71,10 @@ static void usbd_ep_stall_clear (usb_dev *udev, uint8_t ep_addr); static void usbd_ep_data_write (uint8_t *user_fifo, uint8_t ep_num, uint16_t bytes); static uint16_t usbd_ep_data_read (uint8_t *user_fifo, uint8_t ep_num, uint8_t buf_kind); static void usbd_resume (usb_dev *udev); +#ifdef USBD_ERROR_HOOKS +static void usbd_err (void); +static void usbd_setup_err (uint8_t len); +#endif static void usbd_suspend (void); static void usbd_leave_suspend (void); static uint16_t usbd_ep_status (usb_dev *udev, uint8_t ep_addr); @@ -83,6 +87,10 @@ struct _usb_handler usbd_drv_handler = .suspend = usbd_suspend, .suspend_leave = usbd_leave_suspend, .resume = usbd_resume, +#ifdef USBD_ERROR_HOOKS + .err = usbd_err, + .setup_err = usbd_setup_err, +#endif .set_addr = usbd_address_set, .ep_reset = usbd_ep_reset, .ep_disable = usbd_ep_disable, @@ -158,7 +166,11 @@ static void usbd_core_reset (void) #endif /* LPM_ENABLED */ /* enable all interrupts mask bits */ +#ifdef USBD_ERROR_HOOKS + USBD_CTL |= CTL_STIE | CTL_ERRIE | CTL_WKUPIE | CTL_SPSIE | CTL_SOFIE | CTL_ESOFIE | CTL_RSTIE; +#else USBD_CTL |= CTL_STIE | CTL_WKUPIE | CTL_SPSIE | CTL_SOFIE | CTL_ESOFIE | CTL_RSTIE; +#endif } /*! @@ -613,6 +625,30 @@ static void usbd_resume (usb_dev *udev) } } +#ifdef USBD_ERROR_HOOKS +/*! + \brief error interrupt hook + \param[in] none + \param[out] none + \retval none +*/ +static void usbd_err (void) +{ + return; +} + +/*! + \brief setup error hook + \param[in] len: length of erroneous setup packet + \param[out] none + \retval none +*/ +static void usbd_setup_err (uint8_t len) +{ + (void)len; +} +#endif + /*! \brief set USB device to leave mode \param[in] none diff --git a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c index d8357cbf..efd98393 100644 --- a/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c +++ b/system/GD32F30x_firmware/GD32F30x_usbd_library/usbd/Source/usbd_lld_int.c @@ -144,6 +144,9 @@ void usbd_isr (void) USBD_EP_RX_ST_CLEAR(ep_num); if (count != USB_SETUP_PACKET_LEN) { +#ifdef USBD_ERROR_HOOKS + udev->drv_handler->setup_err(count); +#endif usb_stall_transc(udev); return; @@ -203,6 +206,12 @@ void usbd_isr (void) } } +#ifdef USBD_ERROR_HOOKS + if (INTF_ERRIF & int_flag) { + udev->drv_handler->err(); + CLR(ERRIF); + } +#endif if (INTF_WKUPIF & int_flag) { /* restore the old cur_status */ udev->cur_status = udev->backup_status; From 4ac879b8fc637131953ccfa98718ade041f3b19e Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 30 Nov 2022 15:53:59 -0600 Subject: [PATCH 3/5] add USB debug counters --- cores/arduino/USBCore.cpp | 4 ++++ cores/arduino/USBCore.h | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/cores/arduino/USBCore.cpp b/cores/arduino/USBCore.cpp index 79b2e272..ee1d0343 100644 --- a/cores/arduino/USBCore.cpp +++ b/cores/arduino/USBCore.cpp @@ -540,6 +540,7 @@ void (*oldResetHandler)(usb_dev *usbd); void handleReset(usb_dev *usbd) { USBCore().logStatus("Reset"); + USBCore().nreset++; USBCore().setupClass(0); EPBuffers().init(); USBCore().buildDeviceConfigDescriptor(); @@ -572,6 +573,7 @@ static void handleSetupErr(uint8_t len) static void handleErr() { USBCore().logStatus("Error"); + USBCore().nerror++; } #endif @@ -579,6 +581,7 @@ void (*oldSuspendHandler)(); void handleSuspend() { USBCore().logStatus("Suspend"); + USBCore().nsusp++; oldSuspendHandler(); } @@ -588,6 +591,7 @@ void handleResume() usb_disable_interrupts(); USBCore().logStatus("Resume"); usb_enable_interrupts(); + USBCore().nresume++; oldResumeHandler(); } diff --git a/cores/arduino/USBCore.h b/cores/arduino/USBCore.h index f09ba344..b24df391 100644 --- a/cores/arduino/USBCore.h +++ b/cores/arduino/USBCore.h @@ -175,6 +175,12 @@ class USBCore_ int flush(uint8_t ep); void setResetHook(void (*hook)()); + // Debug counters + volatile uint16_t nreset; + volatile uint16_t nsusp; + volatile uint16_t nresume; + volatile uint16_t nerror; + uint8_t setupCtlOut(usb_req* req); void setupClass(uint16_t wLength); void ctlOut(usb_dev* udev); From 38289e76965b61546554a0f94129e8ab6ce55bc0 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Thu, 8 Dec 2022 22:32:50 -0600 Subject: [PATCH 4/5] WIP timeouts --- cores/arduino/USBCore.cpp | 124 ++++++++++++++++++++++++++++++++++++-- cores/arduino/USBCore.h | 6 ++ 2 files changed, 124 insertions(+), 6 deletions(-) diff --git a/cores/arduino/USBCore.cpp b/cores/arduino/USBCore.cpp index ee1d0343..edf5f061 100644 --- a/cores/arduino/USBCore.cpp +++ b/cores/arduino/USBCore.cpp @@ -133,6 +133,14 @@ void EPBuffer::init(uint8_t ep) this->reset(); this->rxWaiting = false; this->txWaiting = false; + this->timedOut = false; + this->timeout = 100; +} + +template +void EPBuffer::setTimeout(uint16_t timeout) +{ + this->timeout = timeout; } template @@ -236,6 +244,9 @@ void EPBuffer::flush() // fall through case USBD_CONFIGURED: case USBD_SUSPENDED: { + if (this->timedOut) { + break; + } // This will temporarily reenable and disable interrupts auto canWrite = this->waitForWriteComplete(); if (canWrite) { @@ -246,6 +257,7 @@ void EPBuffer::flush() // Only start the next transmission if the device hasn't been // reset. this->txWaiting = true; + this->startTime = millis(); usbd_ep_send(&USBCore().usbDev(), this->ep, (uint8_t *)this->buf, this->len()); USBCore().logEP('>', this->ep, '>', this->len()); } @@ -287,6 +299,7 @@ template void EPBuffer::transcIn() { this->txWaiting = false; + this->timedOut = false; } // Unused? @@ -302,11 +315,21 @@ uint8_t* EPBuffer::ptr() template bool EPBuffer::waitForWriteComplete() { - // auto start = getCurrentMillis(); auto ok = true; + auto desc = *EPBuffers().desc(this->ep); + auto udev = &USBCore().usbDev(); + + /* + * For interrupt EPs, count from now instead of from previous send. + * Otherwise, if a send occurs while the bus is suspended, the packet will + * be cheated of its full timeout. + */ + if (desc.type() == USB_EP_ATTR_INT) { + this->startTime = millis(); + } do { usb_enable_interrupts(); - switch (USBCore().usbDev().cur_status) { + switch (udev->cur_status) { case USBD_DEFAULT: case USBD_ADDRESSED: ok = false; @@ -314,11 +337,95 @@ bool EPBuffer::waitForWriteComplete() default: break; } - // if (getCurrentMillis() - start > 5) { - // EPBuffers().buf(ep).transcIn(); - // ok = false; - // } usb_disable_interrupts(); + if (this->txWaiting && millis() - this->startTime > this->timeout) { + USBCore().logEP('X', this->ep, '>', this->len()); + /* + * For interrupt EPs, overwrite the previous packet. In the common + * case of a key press HID report initiating a remote wakeup, + * transmission of the key release event might be delayed, because + * the host can't poll while the bus is suspended. If the + * application sends a key release event before the key press event + * is transmitted, that might time out. If the key release event is + * discarded due to the timeout, the host will never receive the + * key release event, leading to a stuck or repeating key. + */ + if (desc.type() == USB_EP_ATTR_INT) { + /* + * Set the endpoint status to NAK, in an attempt to prevent + * the queued packet from being transmitted while being + * overwritten. + * + * This must be done in two steps. Unfortunately, the USBD + * peripheral's design doesn't seem to make it possible to + * atomically abort a queued transmission. Even if interrupts + * are disabled, the peripheral state machine continues to run. + * + * The endpoint status control bits are toggle-only (toggle on + * writing 1; unchanged on writing 0). If the TX state + * transitions from VALID to NAK during the read-modify-write, + * a toggle operation (XOR 0b01) intended to transition from + * VALID (0b11) to NAK (0b10) will set the status back to + * VALID. This will cause the previous packet to be sent again. + * + * Instead, set the status to DISABLED (0b00, via XOR 0b11), + * which will become STALL (0b01) if the peripheral transitions + * the status to NAK between the read and the write. This can + * happen if the transmission was in progress and the host ACKs + * exactly between the read and the write to update the status + * bits. + * + * Possible states after the first USBD_EP_TX_STAT_SET: + * + * - DISABLED (metastable if TX in progress) + * - STALL (stable; only possible if we lost the r-m-w race) + * - NAK (stable; only possible if TX completed after toggle) + * + * There is a small theoretical chance that the host will poll + * the endpoint again between the two updates to the status + * bits, which could lead to the host receiving a STALL + * handshake, which would put the data toggle bit out of sync. + * + * The second host poll cannot complete sooner than 34 bit times + * after the ACK from the first host poll. (2 bits inter-packet + * delay, 8 bits sync, 8 bits PID, 16 bits addr/endpoint/CRC, + * about 2.83 microseconds) The second status bit update will + * complete before then, assuming reasonable CPU clock speeds + * and no lengthy interrupt handler executions occurring. + * + * On the other hand, it's unknown when exactly the peripheral + * checks the endpoint status bits to decide which response to + * send to an IN poll. It might latch them at EOP, or SOP, or + * maybe PID, or it might wait until after checking the CRC. So + * the actual available time might be between 0 and 34 bit + * times. + * + * We could globally disable interrupts to mitigate that, if + * necessary. + */ + USBD_EP_TX_STAT_SET(this->ep, EPTX_DISABLED); + if (USBD_EPxCS(this->ep) & EPxCS_TX_STA == EPTX_DISABLED) { + /* + * If the endpoint is disabled, there might still be a + * transmission in progress. Wait for it to complete: + * + * 1 byte sync + 1 byte PID + 64 bytes data + 2 bytes CRC, + * 1 turn-around time, + * 1 byte sync + 1 byte PID (handshake) + * assuming maximum bit stuffing at full speed (12MHz). + */ + delayMicroseconds(56); + } + // In case TX was in progress and completed + USBD_EP_TX_ST_CLEAR(this->ep); + USBD_EP_TX_STAT_SET(this->ep, EPTX_NAK); + break; + } else { + ok = false; + this->timedOut = true; + break; + } + } } while (ok && this->txWaiting); return ok; } @@ -874,6 +981,11 @@ int USBCore_::flush(uint8_t ep) return 0; } +void USBCore_::setTimeout(uint8_t ep, uint16_t timeout) +{ + EPBuffers().buf(ep).setTimeout(timeout); +} + void USBCore_::transcSetupHelper(usb_dev* usbd, uint8_t ep) { USBCore_* core = (USBCore_*)usbd->user_data; diff --git a/cores/arduino/USBCore.h b/cores/arduino/USBCore.h index b24df391..280bf409 100644 --- a/cores/arduino/USBCore.h +++ b/cores/arduino/USBCore.h @@ -95,6 +95,7 @@ class EPBuffer void flush(); uint8_t* ptr(); void enableOutEndpoint(); + void setTimeout(uint16_t timeout); void transcIn(); void transcOut(); @@ -131,6 +132,10 @@ class EPBuffer */ volatile bool currentlyFlushing = false; + volatile uint32_t startTime; + uint16_t timeout; + volatile bool timedOut; + uint8_t ep; }; @@ -174,6 +179,7 @@ class USBCore_ int recv(uint8_t ep); int flush(uint8_t ep); void setResetHook(void (*hook)()); + void setTimeout(uint8_t ep, uint16_t timeout); // Debug counters volatile uint16_t nreset; From 6d2cd343828a2ed33112c84c630c003831ebac40 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Sat, 21 Jan 2023 22:18:51 -0600 Subject: [PATCH 5/5] WIP don't wait for USB config --- cores/arduino/gd32/usb.c | 1 - 1 file changed, 1 deletion(-) diff --git a/cores/arduino/gd32/usb.c b/cores/arduino/gd32/usb.c index e2cfbe06..85137268 100644 --- a/cores/arduino/gd32/usb.c +++ b/cores/arduino/gd32/usb.c @@ -61,7 +61,6 @@ void usb_connect() { nvic_config(); usbd_connect(&usbd); - while (usbd.cur_status != USBD_CONFIGURED) {} } void usb_enable_interrupts()