SAMx7x dcd_edpt_xfer returns false on ZLP with dma support

[RamDav]

Operating System

Others

Commit SHA

3af1bec

Board

same7xplained

Firmware

Custom SAME70 firmware.

What happened ?

i have a custom class which uses usbd_edpt_xfer to start read transfers on an endpoint.
If the usb device has already received an zlp then a call to dcd_edpt_xfer returns false which causes the call usbd_edpt_xfer to run into a TU_BREAKPOINT return false and never execute the DCD_EVENT_XFER_COMPLETE event.
This is handeled as an error in the usbd layer in usbd_edpt_xfer which not really is the case in this scenario.
I fixed this temporarily by manually calling the appropriate funtions for a zero length data reception on getting a false after calling usbd_edpt_xfer. but this is only a dirty hack to circumvent this. what should be done here manually raising the DCD_EVENT_XFER_COMPLETE event and returning a true to not cause the usb stack to have any issues?
the implementation with this error is in src/portable/microchip/samx7x/dcd_samx7x.c: 653.

How to reproduce ?

On reception of zero length package on an endpoint currently testing with custom win usb driver.

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

Not available

Screenshots

Not available

I have checked existing issues, discussion and documentation

• I confirm I have checked existing issues, discussion and documentation.

[HiFiPhile]

It has been a long time since I wrote the driver and I haven't work on SAM7X afterwards, it looks strange why I put a return here 🤣

[RamDav]

if resolved the issue with this fix.
is this a valid way?

src/portable/microchip/samx7x/dcd_samx7x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/portable/microchip/samx7x/dcd_samx7x.c b/src/portable/microchip/samx7x/dcd_samx7x.c
index 51308f7a2..f23e24904 100644
--- a/src/portable/microchip/samx7x/dcd_samx7x.c
+++ b/src/portable/microchip/samx7x/dcd_samx7x.c
@@ -646,11 +646,11 @@ bool dcd_edpt_xfer (uint8_t rhport, uint8_t ep_addr, uint8_t * buffer, uint16_t
       return true;
     }
     __set_PRIMASK(irq_state);
-
     // Here a ZLP has been received
     // and the DMA transfer must be not started.
     // It is the end of transfer
-    return false;
+    // TX COMPLETE
+    dcd_event_xfer_complete(rhport, ep_addr, 0, XFER_RESULT_SUCCESS, false);
   } else
   {
     if (dir == TUSB_DIR_OUT)
@@ -740,7 +740,7 @@ bool dcd_edpt_xfer_fifo (uint8_t rhport, uint8_t ep_addr, tu_fifo_t * ff, uint16
     // Here a ZLP has been received
     // and the DMA transfer must be not started.
     // It is the end of transfer
-    return false;
+    dcd_event_xfer_complete(rhport, ep_addr, 0, XFER_RESULT_SUCCESS, false);
   } else
   {
     if (dir == TUSB_DIR_OUT)

[HiFiPhile]

I was testing CDC example until I realized host refused to send ZLP...

Unable to reproduce with a quick echo test on webusb_serial example:

Test details

Patch:

diff --git a/examples/device/webusb_serial/src/main.c b/examples/device/webusb_serial/src/main.c
index 155768b76..fc8213344 100644
--- a/examples/device/webusb_serial/src/main.c
+++ b/examples/device/webusb_serial/src/main.c
@@ -111,7 +111,6 @@ static void echo_all(const uint8_t buf[], uint32_t count) {
   // echo to web serial
   if (web_serial_connected) {
     tud_vendor_write(buf, count);
-    tud_vendor_write_flush();
   }
 
   // echo to cdc
@@ -194,9 +193,7 @@ bool tud_vendor_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_control_requ
           blink_interval_ms = BLINK_ALWAYS_ON;
 
           tud_vendor_write_str("\r\nWebUSB interface connected\r\n");
-          tud_vendor_write_flush();
         } else {
-          tud_vendor_write_clear(); // anything left in the buffer is now thrown out
           blink_interval_ms = BLINK_MOUNTED;
         }
 
@@ -217,11 +214,7 @@ void tud_vendor_rx_cb(uint8_t idx, const uint8_t *buffer, uint32_t bufsize) {
   (void)buffer;
   (void)bufsize;
 
-  while (tud_vendor_available()) {
-    uint8_t        buf[64];
-    const uint32_t count = tud_vendor_read(buf, sizeof(buf));
-    echo_all(buf, count);
-  }
+  tud_vendor_n_write(idx, buffer, bufsize);
 }
 
 //--------------------------------------------------------------------+
diff --git a/examples/device/webusb_serial/src/tusb_config.h b/examples/device/webusb_serial/src/tusb_config.h
index b6cdf7e56..c3db1b072 100644
--- a/examples/device/webusb_serial/src/tusb_config.h
+++ b/examples/device/webusb_serial/src/tusb_config.h
@@ -106,6 +106,9 @@
 #define CFG_TUD_VENDOR_RX_BUFSIZE (TUD_OPT_HIGH_SPEED ? 512 : 64)
 #define CFG_TUD_VENDOR_TX_BUFSIZE (TUD_OPT_HIGH_SPEED ? 512 : 64)
 
+#define CFG_TUD_VENDOR_EPSIZE (TUD_OPT_HIGH_SPEED ? 512 : 64)
+
+#define CFG_TUD_VENDOR_TXRX_BUFFERED 0
 
 #ifdef __cplusplus
  }

program:

#!/usr/bin/python3

import time
import os

import libusb_package
import usb.util

TIMEOUT = 0

def main():
    intf = None

    def match_intf(intf_desc):
        if intf_desc.iInterface == 0:
            return False
        try:
            return usb.util.get_string(intf_desc.device, intf_desc.iInterface) == "TinyUSB WebUSB"
        except Exception:
            return False

    for dev in libusb_package.find(find_all=True):
        intf = usb.util.find_descriptor(dev[0], custom_match=match_intf)
        if intf is not None:
            break

    if intf:
        ep_in  = usb.util.find_descriptor(intf, custom_match = \
        lambda e: usb.util.endpoint_direction(e.bEndpointAddress) == usb.util.ENDPOINT_IN)

        ep_out = usb.util.find_descriptor(intf, custom_match = \
        lambda e: usb.util.endpoint_direction(e.bEndpointAddress) == usb.util.ENDPOINT_OUT)

        dev = intf.device

        def ep_read(len):
            try:
                return bytes(ep_in.read(len, 100))
            except:
                return b''
        def ep_write(buf):
            try:
                ep_out.write(buf, 100)
            except:
                pass

        ep_read(1024)
        time.sleep(TIMEOUT)

        print("send 512 bytes")
        data = os.urandom(512)
        ep_write(data)
        returned = ep_read(len(data))
        assert returned == data, f"512-byte echo mismatch: got {len(returned)} bytes"

        print("done")
        time.sleep(TIMEOUT)

        print("send zlp (no data)")
        # write zero-length (no-op) and clear any pending input
        ep_write(b'')
        ep_read(512)
        time.sleep(TIMEOUT)

        print("send 50 bytes")
        data = os.urandom(50)
        ep_write(data)
        returned = ep_read(len(data))
        assert returned == data, f"50-byte echo mismatch: got {len(returned)} bytes"

        print("done")
        time.sleep(TIMEOUT)

        print("send zlp (no data)")
        ep_write(b'')
        ep_read(512)
        print("done")
        time.sleep(TIMEOUT)

        print("send 200 bytes")
        data = os.urandom(200)
        ep_write(data)
        returned = ep_read(len(data))
        assert returned == data, f"200-byte echo mismatch: got {len(returned)} bytes"
        print("done")
        time.sleep(TIMEOUT)


if __name__ == "__main__":
    main()

I your custom class, are you sure previous transfer is finished (_rx_cb is called) before launching the next ? Even if you are expected receiving a ZLP you still need to call usbd_edpt_xfer with endpoint size instead of 0.

[RamDav]

Thanks a lot for your time,
i will inspect this further and surely that would be the best case that i'm simply doing something wrong in my application.
My current implementation is working with a pool of buffer where received messages are stored.
I have a send and receive queue the problem on my side is occuring on the receive queue.
I'm taking an empty buffer of 2KiB which runs 4 512 Byte transfers. and start the transfer on this endpoint. the buffer is put into a full buffer fifo which is emptied from another loop as soon as the xfer callback is received on this endpoint. I'm not currenlty checking for further empty receive buffers in the xfer callback. This is handeled by a state machine which checks a xfer complete callback in a waiting state and checks for a new empty buffer after that and starts the next transfer if applicable. My guess is that i'm handling edpt_claim and release wrongly.

[RamDav]

i compared this with the current asf 4 usbhs_device driver here ist the same check as in your code and

		// Disable IRQs to have a short sequence
		// between read of EOT_STA and DMA enable
		flags = cpu_irq_save();
		if (!(udd_endpoint_dma_get_status(ep)
				& USBHS_DEVDMASTATUS_END_TR_ST)) {
			dbg_print("dmaS%x ", ep);
			udd_endpoint_dma_set_control(ep, udd_dma_ctrl);
			ptr_job->buf_cnt += next_trans;
			ptr_job->buf_load = next_trans;
			udd_enable_endpoint_dma_interrupt(ep);
			cpu_irq_restore(flags);
			return;
		}
		cpu_irq_restore(flags);
<------- HERE ---->
		// Here a ZLP has been recieved
		// and the DMA transfer must be not started.
		// It is the end of transfer
		ptr_job->buf_size = ptr_job->buf_cnt;
	}
	if (Is_udd_endpoint_in(ep)) {
		if (ptr_job->b_shortpacket) {
			dbg_print("zlpS%x ", ep);
			// Need to send a ZLP (No possible with USB DMA)
			// enable interrupt to wait a free bank to sent ZLP
			udd_ack_in_send(ep);
			if (Is_udd_write_enabled(ep)) {
				// Force interrupt in case of ep already free
				udd_raise_in_send(ep);
			}
			udd_enable_in_send_interrupt(ep);
			udd_enable_endpoint_interrupt(ep);
			return;
		}
	} else {
		_dcache_invalidate(ptr_job->buf, ptr_job->buf_size);
	}
	dbg_print("dmaE ");
	// Call callback to signal end of transfer
<------ THIS IS CALLED --->
	udd_ep_finish_job(ptr_job, false, ep);
}

and in this implementation it ends in udd_ep_finish_job so my guess is that this is needed if an zlp is already in the usb peripheral when starting an out transfer.
so the best way should be to call the xfer complete event here.

[RamDav]

i have some further fixes where i had problems with sending zlps on in transfers very shortly after the last in transfer with a full usb transfers.
it's stated in in the asf4 that the defautl way of sending zlp packages does not work with dma so there is a implementation to force sending the zlp via normal transfers which i also added by adapting it from the asf.

 src/portable/microchip/samx7x/dcd_samx7x.c | 44 ++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/src/portable/microchip/samx7x/dcd_samx7x.c b/src/portable/microchip/samx7x/dcd_samx7x.c
index 808bb3251..a11f0caef 100644
--- a/src/portable/microchip/samx7x/dcd_samx7x.c
+++ b/src/portable/microchip/samx7x/dcd_samx7x.c
@@ -339,6 +339,11 @@ static void dcd_ep_handler(uint8_t ep_ix)
       } else
       {
         // TX complete
+        // ZLP on dma supported endpoint clear fifocon now
+        if (EP_DMA_SUPPORT(ep_ix) && xfer->total_len == 0)
+        {
+          USB_REG->DEVEPTIDR[ep_ix] = DEVEPTIDR_FIFOCONC;
+        }
         dcd_event_xfer_complete(0, 0x80 + ep_ix, xfer->total_len, XFER_RESULT_SUCCESS, true);
         // Disable the interrupt
         USB_REG->DEVEPTIDR[ep_ix] = DEVEPTIDR_TXINEC;
@@ -716,7 +721,25 @@ bool dcd_edpt_xfer (uint8_t rhport, uint8_t ep_addr, uint8_t * buffer, uint16_t
       USB_REG->DEVEPTIER[epnum] = DEVEPTIER_RXOUTES;
     } else
     {
-      dcd_transmit_packet(xfer,epnum);
+      if (EP_DMA_SUPPORT(epnum) && total_bytes == 0)
+      {
+        // ZLP on dma supported endpoint:
+        // FIFOCONC has to be cleared in TXINI-Interrupt,
+        // if it is cleared here then no interrup is generated.
+        // check for free bank an force interrupt if required
+        USB_REG->DEVEPTICR[epnum] = DEVEPTICR_TXINIC;
+        if (USB_REG->DEVEPTISR[epnum] & DEVEPTISR_RWALL)
+        {
+          // bank is alredy emptied
+          USB_REG->DEVEPTIFR[epnum] = DEVEPTIFR_TXINIS; // raise TXINI
+        }
+        USB_REG->DEVEPTIER[epnum] = DEVEPTIER_TXINES;
+        USB_REG->DEVIER = (DEVIER_PEP_0 << (epnum));
+      }
+      else
+      {
+        dcd_transmit_packet(xfer, epnum);
+      }
     }
   }
   return true;
@@ -806,7 +829,24 @@ bool dcd_edpt_xfer_fifo (uint8_t rhport, uint8_t ep_addr, tu_fifo_t * ff, uint16
       USB_REG->DEVEPTIER[epnum] = DEVEPTIER_RXOUTES;
     } else
     {
-      dcd_transmit_packet(xfer,epnum);
+      if (EP_DMA_SUPPORT(epnum) && total_bytes == 0)
+      {
+        // ZLP on dma supported endpoint:
+        // FIFOCONC has to be cleared in TXINI-Interrupt,
+        // if it is cleared here then no interrup is generated.
+        // check for free bank an force interrupt if required
+        USB_REG->DEVEPTICR[epnum] = DEVEPTICR_TXINIC;
+        if (USB_REG->DEVEPTISR[epnum] & DEVEPTISR_TXINI)
+        {
+          // bank is alredy emptied
+          USB_REG->DEVEPTIFR[epnum] = DEVEPTIFR_TXINIS; // raise TXINI
+        }
+        USB_REG->DEVEPTIER[epnum] = DEVEPTIER_TXINES;   // enable interrupt
+      }
+      else
+      {
+        dcd_transmit_packet(xfer, epnum);
+      }
     }
   }
   return true;

and i implemented a version where close ep is supported by adapting the code from the asf4.

 src/common/tusb_mcu.h                      |  1 +
 src/portable/microchip/samx7x/dcd_samx7x.c | 80 ++++++++++++++++++++++++++----
 2 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/src/common/tusb_mcu.h b/src/common/tusb_mcu.h
index 002cd3a0e..4241a7b04 100644
--- a/src/common/tusb_mcu.h
+++ b/src/common/tusb_mcu.h
@@ -162,6 +162,7 @@
 
 #elif TU_CHECK_MCU(OPT_MCU_SAMX7X)
   #define TUP_DCD_ENDPOINT_MAX 10
+  #define TUP_DCD_EDPT_CLOSE_API
   #define TUP_RHPORT_HIGHSPEED 1
   #define TUD_ENDPOINT_ONE_DIRECTION_ONLY
 
diff --git a/src/portable/microchip/samx7x/dcd_samx7x.c b/src/portable/microchip/samx7x/dcd_samx7x.c
index 51308f7a2..808bb3251 100644
--- a/src/portable/microchip/samx7x/dcd_samx7x.c
+++ b/src/portable/microchip/samx7x/dcd_samx7x.c
@@ -76,6 +76,8 @@ typedef struct {
 static tusb_speed_t get_speed(void);
 static void dcd_transmit_packet(xfer_ctl_t * xfer, uint8_t ep_ix);
 
+static void dcd_edpt_abort(uint8_t rhport, uint8_t ep_addr);
+
 // DMA descriptors shouldn't be placed in ITCM !
 CFG_TUD_MEM_SECTION static dma_desc_t dma_desc[6];
 
@@ -555,21 +557,77 @@ bool dcd_edpt_open (uint8_t rhport, tusb_desc_endpoint_t const * ep_desc)
 
 void dcd_edpt_close_all (uint8_t rhport)
 {
-  (void) rhport;
-  // TODO implement dcd_edpt_close_all()
+  dcd_int_disable(rhport);
+  for(int i = 0; i < EP_MAX; i++){
+    // Read direction from hardware configuration register
+    uint8_t dir = (USB_REG->DEVEPTCFG[i] & DEVEPTCFG_EPDIR) ? TUSB_DIR_IN : TUSB_DIR_OUT;
+    dcd_edpt_close(rhport, tu_edpt_addr(i, dir));
+  }
+  dcd_int_enable(rhport);
 }
 
-bool dcd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) {
-  (void) rhport;
-  (void) ep_addr;
-  (void) largest_packet_size;
-  return false;
+static void dcd_edpt_abort(uint8_t rhport, uint8_t ep_addr)
+{
+    (void)rhport;
+    const unsigned ep_num  = tu_edpt_number(ep_addr);
+    const unsigned ep_dir = tu_edpt_dir(ep_addr);
+
+    if (EP_DMA_SUPPORT(ep_num))
+    {
+        uint8_t dma_idx = ep_num - 1;
+
+        // Disable DMA interrupt
+        USB_REG->DEVIDR = (_U_(0x1) << (24 + ep_num));
+
+        // Stop DMA channel and wait until it is no longer active
+        USB_REG->DEVDMA[dma_idx].DEVDMACONTROL = 0;
+        while (USB_REG->DEVDMA[dma_idx].DEVDMASTATUS & DEVDMASTATUS_CHANN_ACT);
+
+        // Clear DMA address and status
+        USB_REG->DEVDMA[dma_idx].DEVDMAADDRESS = 0;
+        volatile uint32_t status = USB_REG->DEVDMA[dma_idx].DEVDMASTATUS;
+        USB_REG->DEVDMA[dma_idx].DEVDMASTATUS = status;
+        USB_REG->DEVICR = (_U_(0x1) << (24 + ep_num));
+
+        // Disable DMA request on endpoint
+        USB_REG->DEVEPTIDR[ep_num] = DEVEPTIDR_EPDISHDMAC;
+    }
+
+    // Disable endpoint interrupt
+    USB_REG->DEVIDR = (_U_(0x1) << (12 + ep_num));
+    __DSB();
+    __ISB();
+
+    // Kill all pending IN banks so no stale data is sent after re-open
+    if (ep_dir == TUSB_DIR_IN)
+    {
+        while (USB_REG->DEVEPTISR[ep_num] & DEVEPTISR_NBUSYBK)
+        {
+            USB_REG->DEVEPTIER[ep_num] = DEVEPTIER_KILLBKS;
+            while (USB_REG->DEVEPTIMR[ep_num] & DEVEPTIMR_KILLBK);
+        }
+    }
+
+    //bool b_is_active = xfer_status[ep_num].buffer != NULL || xfer_status[ep_num].total_len != 0;
+    // Reset transfer state so the endpoint can be reused immediately
+    xfer_status[ep_num].buffer     = NULL;
+    xfer_status[ep_num].total_len  = 0;
+    xfer_status[ep_num].queued_len = 0;
+    //if(b_is_active)
+    //  dcd_event_xfer_complete(rhport, ep_addr, 0, XFER_RESULT_FAILED, false);
 }
 
-bool dcd_edpt_iso_activate(uint8_t rhport, tusb_desc_endpoint_t const * desc_ep) {
-  (void) rhport;
-  (void) desc_ep;
-  return false;
+void dcd_edpt_close(uint8_t rhport, uint8_t ep_addr)
+{
+    const unsigned ep_num = tu_edpt_number(ep_addr);
+
+    // Abort any ongoing transfer first — stops DMA, kills pending banks, resets state
+    dcd_edpt_abort(rhport, ep_addr);
+
+    // Disable and reset endpoint hardware state machine
+    USB_REG->DEVEPT &= ~(_U_(0x1) << ep_num);
+    USB_REG->DEVEPT |=  (_U_(0x1) << (16 + ep_num));
+    USB_REG->DEVEPT &= ~(_U_(0x1) << (16 + ep_num));
 }
 
 static void dcd_transmit_packet(xfer_ctl_t * xfer, uint8_t ep_ix)

[HiFiPhile]

i compared this with the current asf 4 usbhs_device driver here ist the same check as in your code and

Thank you for looking at ASF4's code, as TUSB doesn't have queue depth the next transfer can't be launched until previous is finished, I see no reason why END_TR_ST is still pending when next transfer is launched.

Could you make a reproducible example (see Test Details above) as I didn't issue sending/receiving ZLP.

I've some update on #3519

[RamDav]

hello ok i will check with the next commit. the main problem is that i'm already takning longer switching from asf drivers to tinyusb in my project and i don't think i'll have time to generate a test case in the next weeks. i'll look into this as soon as i have time.

[HiFiPhile]

Hi, do you have any update ?

[RamDav]

I'm sorry i currently don't have time for this. But i will return to this as soon as time permits it.