From 778c4239b1972658e4566c2c62f91c5c8d6d88f2 Mon Sep 17 00:00:00 2001 From: Grissiom Date: Thu, 9 May 2013 23:18:58 +0800 Subject: [PATCH 1/6] cdc_vcom: fix the bug that use ringbuffer pool as usb packet buffer This commit set the buffer for packet to CDC_MaxPacketSize which is a reasonable value for it. However, maybe we should make CDC_{RX,TX}_BUFSIZE configurable as well. --- .../drivers/usb/usbdevice/class/cdc_vcom.c | 53 +++++++++++++------ 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/components/drivers/usb/usbdevice/class/cdc_vcom.c b/components/drivers/usb/usbdevice/class/cdc_vcom.c index 497e6e30e..0e0b3c7d1 100644 --- a/components/drivers/usb/usbdevice/class/cdc_vcom.c +++ b/components/drivers/usb/usbdevice/class/cdc_vcom.c @@ -20,14 +20,20 @@ #ifdef RT_USB_DEVICE_CDC -#define CDC_RX_BUFSIZE 64 -#define CDC_TX_BUFSIZE 2048 -static rt_uint8_t rx_pool[CDC_RX_BUFSIZE]; -static rt_uint8_t tx_pool[CDC_TX_BUFSIZE]; +#define CDC_RX_BUFSIZE 2048 +#define CDC_TX_BUFSIZE 1024 +static rt_uint8_t rx_rbp[CDC_RX_BUFSIZE]; +static rt_uint8_t tx_rbp[CDC_TX_BUFSIZE]; static struct rt_ringbuffer rx_ringbuffer; static struct rt_ringbuffer tx_ringbuffer; -static struct rt_serial_device vcom_serial; static struct serial_ringbuffer vcom_int_rx; + +static struct rt_serial_device vcom_serial; + +#define CDC_MaxPacketSize 64 +static rt_uint8_t rx_buf[CDC_MaxPacketSize]; +static rt_uint8_t tx_buf[CDC_MaxPacketSize]; + static rt_bool_t vcom_connected = RT_FALSE; static struct udevice_descriptor dev_desc = @@ -38,7 +44,7 @@ static struct udevice_descriptor dev_desc = USB_CLASS_CDC, //bDeviceClass; 0x00, //bDeviceSubClass; 0x00, //bDeviceProtocol; - 0x40, //bMaxPacketSize0; + CDC_MaxPacketSize, //bMaxPacketSize0; _VENDOR_ID, //idVendor; _PRODUCT_ID, //idProduct; USB_BCD_DEVICE, //bcdDevice; @@ -160,7 +166,8 @@ static rt_err_t _ep_in_handler(udevice_t device, uclass_t cls, rt_size_t size) eps = (cdc_eps_t)cls->eps; mps = eps->ep_in->ep_desc->wMaxPacketSize; size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); - if(size == 0) return RT_EOK; + if(size == 0) + return RT_EOK; length = size > mps ? mps : size; @@ -192,6 +199,7 @@ static rt_err_t _ep_out_handler(udevice_t device, uclass_t cls, rt_size_t size) eps = (cdc_eps_t)cls->eps; /* receive data from USB VCOM */ level = rt_hw_interrupt_disable(); + rt_ringbuffer_put(&rx_ringbuffer, eps->ep_out->buffer, size); rt_hw_interrupt_enable(level); @@ -337,8 +345,8 @@ static rt_err_t _class_run(udevice_t device, uclass_t cls) RT_DEBUG_LOG(RT_DEBUG_USB, ("cdc class run\n")); eps = (cdc_eps_t)cls->eps; - eps->ep_in->buffer=tx_pool; - eps->ep_out->buffer=rx_pool; + eps->ep_in->buffer = tx_buf; + eps->ep_out->buffer = rx_buf; dcd_ep_read(device->dcd, eps->ep_out, eps->ep_out->buffer, eps->ep_out->ep_desc->wMaxPacketSize); @@ -373,21 +381,22 @@ static rt_err_t _class_sof_handler(udevice_t device, uclass_t cls) { rt_uint32_t level; rt_size_t size; - static rt_uint32_t frame_count = 0; + /*static rt_uint32_t frame_count = 0;*/ cdc_eps_t eps; if(vcom_connected != RT_TRUE) return -RT_ERROR; eps = (cdc_eps_t)cls->eps; - if (frame_count ++ == 5) + /*if (frame_count ++ == 5)*/ { rt_size_t mps = eps->ep_in->ep_desc->wMaxPacketSize; /* reset the frame counter */ - frame_count = 0; + /*frame_count = 0;*/ size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); - if(size == 0) return -RT_EFULL; + if(size == 0) + return -RT_EFULL; size = size > mps ? mps : size; @@ -406,6 +415,7 @@ static struct uclass_ops ops = { _class_run, _class_stop, + /*RT_NULL,*/ _class_sof_handler, }; @@ -534,8 +544,17 @@ static rt_err_t _vcom_control(struct rt_serial_device *serial, static int _vcom_putc(struct rt_serial_device *serial, char c) { rt_uint32_t level; + int cnt = 0; - if (vcom_connected != RT_TRUE) return 0; + if (vcom_connected != RT_TRUE) + return 0; + + while (RT_RINGBUFFER_EMPTY(&tx_ringbuffer) == 0) + { + rt_kprintf("wait for %d\n", cnt++); + if (vcom_connected != RT_TRUE) + return 0; + } level = rt_hw_interrupt_disable(); if (RT_RINGBUFFER_EMPTY(&tx_ringbuffer)) @@ -579,8 +598,8 @@ void rt_usb_vcom_init(void) struct serial_configure config; /* initialize ring buffer */ - rt_ringbuffer_init(&rx_ringbuffer, rx_pool, CDC_RX_BUFSIZE); - rt_ringbuffer_init(&tx_ringbuffer, tx_pool, CDC_TX_BUFSIZE); + rt_ringbuffer_init(&rx_ringbuffer, rx_rbp, CDC_RX_BUFSIZE); + rt_ringbuffer_init(&tx_ringbuffer, tx_rbp, CDC_TX_BUFSIZE); config.baud_rate = BAUD_RATE_115200; config.bit_order = BIT_ORDER_LSB; @@ -595,7 +614,7 @@ void rt_usb_vcom_init(void) /* register vcom device */ rt_hw_serial_register(&vcom_serial, "vcom", - RT_DEVICE_FLAG_RDWR | RT_DEVICE_FLAG_INT_RX | RT_DEVICE_FLAG_STREAM, + RT_DEVICE_FLAG_RDWR | RT_DEVICE_FLAG_INT_RX, RT_NULL); } From ffb1e620207942e941f2fb1b9c390404658e0cf3 Mon Sep 17 00:00:00 2001 From: Grissiom Date: Fri, 10 May 2013 15:17:59 +0800 Subject: [PATCH 2/6] cdc_vcom: send a zero-length-packet at the end is transaction --- components/drivers/usb/usbdevice/class/cdc_vcom.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/components/drivers/usb/usbdevice/class/cdc_vcom.c b/components/drivers/usb/usbdevice/class/cdc_vcom.c index 0e0b3c7d1..4cb3ec555 100644 --- a/components/drivers/usb/usbdevice/class/cdc_vcom.c +++ b/components/drivers/usb/usbdevice/class/cdc_vcom.c @@ -31,7 +31,9 @@ static struct serial_ringbuffer vcom_int_rx; static struct rt_serial_device vcom_serial; #define CDC_MaxPacketSize 64 +ALIGN(RT_ALIGN_SIZE) static rt_uint8_t rx_buf[CDC_MaxPacketSize]; +ALIGN(RT_ALIGN_SIZE) static rt_uint8_t tx_buf[CDC_MaxPacketSize]; static rt_bool_t vcom_connected = RT_FALSE; @@ -148,6 +150,7 @@ const static char* _ustring[] = "Interface", }; +static rt_bool_t _in_sending; /** * This function will handle cdc bulk in endpoint request. * @@ -167,7 +170,14 @@ static rt_err_t _ep_in_handler(udevice_t device, uclass_t cls, rt_size_t size) mps = eps->ep_in->ep_desc->wMaxPacketSize; size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); if(size == 0) + { + if (_in_sending) + { + _in_sending = RT_FALSE; + dcd_ep_write(device->dcd, eps->ep_in, RT_NULL, 0); + } return RT_EOK; + } length = size > mps ? mps : size; @@ -398,6 +408,7 @@ static rt_err_t _class_sof_handler(udevice_t device, uclass_t cls) if(size == 0) return -RT_EFULL; + _in_sending = RT_TRUE; size = size > mps ? mps : size; level = rt_hw_interrupt_disable(); @@ -415,7 +426,6 @@ static struct uclass_ops ops = { _class_run, _class_stop, - /*RT_NULL,*/ _class_sof_handler, }; From 5de59429fed2e0b8d55e6e705777ad2ecf9bef2f Mon Sep 17 00:00:00 2001 From: Grissiom Date: Fri, 10 May 2013 17:35:33 +0800 Subject: [PATCH 3/6] usbdevice/core: misc cleanup --- components/drivers/usb/usbdevice/core/core.c | 32 +++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/components/drivers/usb/usbdevice/core/core.c b/components/drivers/usb/usbdevice/core/core.c index 2f4a21eee..104f81639 100644 --- a/components/drivers/usb/usbdevice/core/core.c +++ b/components/drivers/usb/usbdevice/core/core.c @@ -1294,25 +1294,24 @@ static void rt_usbd_thread_entry(void* parameter) uep_t ep; /* receive message */ - if(rt_mq_recv(usb_mq, &msg, sizeof(struct udev_msg), RT_WAITING_FOREVER) - != RT_EOK ) continue; + if(rt_mq_recv(usb_mq, + &msg, sizeof(struct udev_msg), + RT_WAITING_FOREVER) != RT_EOK ) + continue; + + device = rt_usbd_find_device(msg.dcd); + if(device == RT_NULL) + { + rt_kprintf("invalid usb device\n"); + continue; + } switch (msg.type) { case USB_MSG_SETUP_NOTIFY: - device = rt_usbd_find_device(msg.dcd); - if(device != RT_NULL) - _setup_request(device, (ureq_t)msg.content.setup_msg.packet); - else - rt_kprintf("invalid usb device\n"); + _setup_request(device, (ureq_t)msg.content.setup_msg.packet); break; case USB_MSG_DATA_NOTIFY: - device = rt_usbd_find_device(msg.dcd); - if(device == RT_NULL) - { - rt_kprintf("invalid usb device\n"); - break; - } ep = rt_usbd_find_endpoint(device, &cls, msg.content.ep_msg.ep_addr); if(ep != RT_NULL) ep->handler(device, cls, msg.content.ep_msg.size); @@ -1320,13 +1319,10 @@ static void rt_usbd_thread_entry(void* parameter) rt_kprintf("invalid endpoint\n"); break; case USB_MSG_SOF: - device = rt_usbd_find_device(msg.dcd); - if(device != RT_NULL) - _sof_notify(device); - else - rt_kprintf("invalid usb device\n"); + _sof_notify(device); break; default: + rt_kprintf("unknown msg type\n"); break; } } From d23ee75d2e36a145d62dff7bb2b738a29779c716 Mon Sep 17 00:00:00 2001 From: Grissiom Date: Fri, 10 May 2013 17:50:07 +0800 Subject: [PATCH 4/6] cdc_vcom: send data as many as possible The data filled into dcd_ep_write does not to be limited by MaxPacketSize. --- .../drivers/usb/usbdevice/class/cdc_vcom.c | 68 ++++++++++--------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/components/drivers/usb/usbdevice/class/cdc_vcom.c b/components/drivers/usb/usbdevice/class/cdc_vcom.c index 4cb3ec555..60f840d29 100644 --- a/components/drivers/usb/usbdevice/class/cdc_vcom.c +++ b/components/drivers/usb/usbdevice/class/cdc_vcom.c @@ -21,7 +21,7 @@ #ifdef RT_USB_DEVICE_CDC #define CDC_RX_BUFSIZE 2048 -#define CDC_TX_BUFSIZE 1024 +#define CDC_TX_BUFSIZE 2048 static rt_uint8_t rx_rbp[CDC_RX_BUFSIZE]; static rt_uint8_t tx_rbp[CDC_TX_BUFSIZE]; static struct rt_ringbuffer rx_ringbuffer; @@ -32,9 +32,9 @@ static struct rt_serial_device vcom_serial; #define CDC_MaxPacketSize 64 ALIGN(RT_ALIGN_SIZE) -static rt_uint8_t rx_buf[CDC_MaxPacketSize]; +static rt_uint8_t rx_buf[CDC_RX_BUFSIZE]; ALIGN(RT_ALIGN_SIZE) -static rt_uint8_t tx_buf[CDC_MaxPacketSize]; +static rt_uint8_t tx_buf[CDC_TX_BUFSIZE]; static rt_bool_t vcom_connected = RT_FALSE; @@ -151,6 +151,7 @@ const static char* _ustring[] = }; static rt_bool_t _in_sending; + /** * This function will handle cdc bulk in endpoint request. * @@ -162,31 +163,33 @@ static rt_bool_t _in_sending; static rt_err_t _ep_in_handler(udevice_t device, uclass_t cls, rt_size_t size) { rt_uint32_t level; - rt_size_t length; cdc_eps_t eps; - rt_size_t mps; eps = (cdc_eps_t)cls->eps; - mps = eps->ep_in->ep_desc->wMaxPacketSize; + level = rt_hw_interrupt_disable(); size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); if(size == 0) { if (_in_sending) { _in_sending = RT_FALSE; + /* don't have data right now. Send a zero-length-packet to + * terminate the transaction. + * + * FIXME: actually, this might not be the right place to send zlp. + * Only the rt_device_write could know how much data is sending. */ dcd_ep_write(device->dcd, eps->ep_in, RT_NULL, 0); } + rt_hw_interrupt_enable(level); return RT_EOK; } + _in_sending = RT_TRUE; - length = size > mps ? mps : size; - - level = rt_hw_interrupt_disable(); - rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, length); + rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, size); rt_hw_interrupt_enable(level); /* send data to host */ - dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, length); + dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, size); return RT_EOK; } @@ -391,33 +394,25 @@ static rt_err_t _class_sof_handler(udevice_t device, uclass_t cls) { rt_uint32_t level; rt_size_t size; - /*static rt_uint32_t frame_count = 0;*/ cdc_eps_t eps; - if(vcom_connected != RT_TRUE) return -RT_ERROR; + if (vcom_connected != RT_TRUE) + return -RT_ERROR; eps = (cdc_eps_t)cls->eps; - /*if (frame_count ++ == 5)*/ - { - rt_size_t mps = eps->ep_in->ep_desc->wMaxPacketSize; - /* reset the frame counter */ - /*frame_count = 0;*/ + size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); + if(size == 0) + return -RT_EFULL; - size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); - if(size == 0) - return -RT_EFULL; + _in_sending = RT_TRUE; - _in_sending = RT_TRUE; - size = size > mps ? mps : size; + level = rt_hw_interrupt_disable(); + rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, size); + rt_hw_interrupt_enable(level); - level = rt_hw_interrupt_disable(); - rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, size); - rt_hw_interrupt_enable(level); - - /* send data to host */ - dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, size); - } + /* send data to host */ + dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, size); return RT_EOK; } @@ -554,17 +549,24 @@ static rt_err_t _vcom_control(struct rt_serial_device *serial, static int _vcom_putc(struct rt_serial_device *serial, char c) { rt_uint32_t level; - int cnt = 0; + int cnt = 500; if (vcom_connected != RT_TRUE) return 0; - while (RT_RINGBUFFER_EMPTY(&tx_ringbuffer) == 0) + /* if the buffer is full, there is a chance that the host would pull some + * data out soon. But we cannot rely on that and if we wait to long, just + * return. */ + for (cnt = 500; + RT_RINGBUFFER_EMPTY(&tx_ringbuffer) == 0 && cnt; + cnt--) { - rt_kprintf("wait for %d\n", cnt++); + rt_kprintf("wait for %d\n", cnt); if (vcom_connected != RT_TRUE) return 0; } + if (cnt == 0) + return 0; level = rt_hw_interrupt_disable(); if (RT_RINGBUFFER_EMPTY(&tx_ringbuffer)) From 157af94af99e78808aa06b7719b7eea6c879143b Mon Sep 17 00:00:00 2001 From: Grissiom Date: Sun, 12 May 2013 13:06:44 +0800 Subject: [PATCH 5/6] usbdevice/core: small optimization on the event loop Sort the switch cases by the frequency of the events. --- components/drivers/usb/usbdevice/core/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/drivers/usb/usbdevice/core/core.c b/components/drivers/usb/usbdevice/core/core.c index 104f81639..071741463 100644 --- a/components/drivers/usb/usbdevice/core/core.c +++ b/components/drivers/usb/usbdevice/core/core.c @@ -1308,8 +1308,8 @@ static void rt_usbd_thread_entry(void* parameter) switch (msg.type) { - case USB_MSG_SETUP_NOTIFY: - _setup_request(device, (ureq_t)msg.content.setup_msg.packet); + case USB_MSG_SOF: + _sof_notify(device); break; case USB_MSG_DATA_NOTIFY: ep = rt_usbd_find_endpoint(device, &cls, msg.content.ep_msg.ep_addr); @@ -1318,8 +1318,8 @@ static void rt_usbd_thread_entry(void* parameter) else rt_kprintf("invalid endpoint\n"); break; - case USB_MSG_SOF: - _sof_notify(device); + case USB_MSG_SETUP_NOTIFY: + _setup_request(device, (ureq_t)msg.content.setup_msg.packet); break; default: rt_kprintf("unknown msg type\n"); From aa179e44383a20234fd07f79805f571af9936a01 Mon Sep 17 00:00:00 2001 From: Grissiom Date: Sat, 11 May 2013 09:42:40 +0800 Subject: [PATCH 6/6] cdc_vcom: not to start sending data when the data is already sending We start the sending transaction in SOF handler. But if the data is already sending, start an other transaction will cause data lose. Implement a state machine is cdc_vcom and avoid that. --- .../drivers/usb/usbdevice/class/cdc_vcom.c | 62 +++++++++++-------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/components/drivers/usb/usbdevice/class/cdc_vcom.c b/components/drivers/usb/usbdevice/class/cdc_vcom.c index 60f840d29..44e905f31 100644 --- a/components/drivers/usb/usbdevice/class/cdc_vcom.c +++ b/components/drivers/usb/usbdevice/class/cdc_vcom.c @@ -37,6 +37,7 @@ ALIGN(RT_ALIGN_SIZE) static rt_uint8_t tx_buf[CDC_TX_BUFSIZE]; static rt_bool_t vcom_connected = RT_FALSE; +static rt_bool_t vcom_in_sending = RT_FALSE; static struct udevice_descriptor dev_desc = { @@ -150,8 +151,6 @@ const static char* _ustring[] = "Interface", }; -static rt_bool_t _in_sending; - /** * This function will handle cdc bulk in endpoint request. * @@ -163,35 +162,44 @@ static rt_bool_t _in_sending; static rt_err_t _ep_in_handler(udevice_t device, uclass_t cls, rt_size_t size) { rt_uint32_t level; + rt_uint32_t remain; cdc_eps_t eps; eps = (cdc_eps_t)cls->eps; level = rt_hw_interrupt_disable(); - size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); - if(size == 0) + remain = RT_RINGBUFFER_SIZE(&tx_ringbuffer); + if (remain != 0) { - if (_in_sending) - { - _in_sending = RT_FALSE; - /* don't have data right now. Send a zero-length-packet to - * terminate the transaction. - * - * FIXME: actually, this might not be the right place to send zlp. - * Only the rt_device_write could know how much data is sending. */ - dcd_ep_write(device->dcd, eps->ep_in, RT_NULL, 0); - } + + vcom_in_sending = RT_TRUE; + rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, remain); + rt_hw_interrupt_enable(level); + + /* send data to host */ + dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, remain); + + return RT_EOK; + } + + if (size != 0 && + (size % CDC_MaxPacketSize) == 0) + { + /* don't have data right now. Send a zero-length-packet to + * terminate the transaction. + * + * FIXME: actually, this might not be the right place to send zlp. + * Only the rt_device_write could know how much data is sending. */ + vcom_in_sending = RT_TRUE; + rt_hw_interrupt_enable(level); + dcd_ep_write(device->dcd, eps->ep_in, RT_NULL, 0); + return RT_EOK; + } + else + { + vcom_in_sending = RT_FALSE; rt_hw_interrupt_enable(level); return RT_EOK; } - _in_sending = RT_TRUE; - - rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, size); - rt_hw_interrupt_enable(level); - - /* send data to host */ - dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, size); - - return RT_EOK; } /** @@ -399,19 +407,21 @@ static rt_err_t _class_sof_handler(udevice_t device, uclass_t cls) if (vcom_connected != RT_TRUE) return -RT_ERROR; + if (vcom_in_sending) + return RT_EOK; + eps = (cdc_eps_t)cls->eps; size = RT_RINGBUFFER_SIZE(&tx_ringbuffer); - if(size == 0) + if (size == 0) return -RT_EFULL; - _in_sending = RT_TRUE; - level = rt_hw_interrupt_disable(); rt_ringbuffer_get(&tx_ringbuffer, eps->ep_in->buffer, size); rt_hw_interrupt_enable(level); /* send data to host */ + vcom_in_sending = RT_TRUE; dcd_ep_write(device->dcd, eps->ep_in, eps->ep_in->buffer, size); return RT_EOK;