USB: cdc_acm: use ringbuf library and fixes #14677
Conversation
subsys/usb/class/cdc_acm.c
Outdated
| usb_read(ep, dev_data->rx_buf, bytes_to_read, &read); | ||
|
|
||
| if (!ring_buf_put(dev_data->rx_ringbuf, dev_data->rx_buf, read)) { | ||
| LOG_WRN("Rinf buffer full"); |
There was a problem hiding this comment.
New data will be dropped? Also we can not block here as it could be called from ISR (drivers without worker or own thread). I would change it to LOG_ERR.
There was a problem hiding this comment.
@jfischer-phytec-iot why do you think it can block? The new data would be dropped the same way it was done before, functionality should not change.
There was a problem hiding this comment.
Changed to LOG_ERR()
subsys/usb/class/cdc_acm.c
Outdated
| u8_t rx_ready; /* Rx ready status */ | ||
| u8_t tx_irq_ena; /* Tx interrupt enable status */ | ||
| u8_t rx_irq_ena; /* Rx interrupt enable status */ | ||
| s8_t tx_ready; |
There was a problem hiding this comment.
The commit does not describe the motivation behind the change from unsigned to signed. Why not bit field?
There was a problem hiding this comment.
This is to eliminate 'U' after assignments.
There was a problem hiding this comment.
I suggest to use bool or bit field.
subsys/usb/class/cdc_acm.c
Outdated
| @@ -1075,9 +1042,11 @@ static const struct uart_driver_api cdc_acm_driver_api = { | |||
| #endif /* CONFIG_USB_COMPOSITE_DEVICE */ | |||
|
|
|||
| #define DEFINE_CDC_ACM_DEV_DATA(x) \ | |||
| RING_BUF_DECLARE(rx_ringbuf_##x, 512); \ | |||
There was a problem hiding this comment.
Is it an option to make the ring buffer size configurable in Kconfig? I tested the patch and ran into issues when the buffer was filled and started dropping data before the callback was served from the system workqueue.
There was a problem hiding this comment.
I can make it configurable.
finikorg
changed the title
Codecov Report
@@ Coverage Diff @@
## master #14677 +/- ##
=======================================
Coverage 52.51% 52.51%
=======================================
Files 309 309
Lines 45048 45048
Branches 10419 10419
=======================================
Hits 23657 23657
Misses 16583 16583
Partials 4808 4808Continue to review full report at Codecov.
|
|
Tested the throughput, |
jfischer-no
added
bug
Remove 'U' after every assignment and make code more logical. Signed-off-by: Andrei Emeltchenko <[email protected]>
Use ringbuf library for handling RX CDC ACM path. Remove old code artifacts handling specific hardware. Fixes zephyrproject-rtos#14288 Signed-off-by: Andrei Emeltchenko <[email protected]>
If there are several queued interrupts we can miss some of them. Use while() loop to catch them all. Signed-off-by: Andrei Emeltchenko <[email protected]>
Add new configuration parameter USB_CDC_ACM_RINGBUF_SIZE, default to 1024. Signed-off-by: Andrei Emeltchenko <[email protected]>
Instead of relying on overlays which requires to write overlay for every board (100+) define console device name. Fixes zephyrproject-rtos#14698 Signed-off-by: Andrei Emeltchenko <[email protected]>
Remove old artefacts and use ringbuf for RX path.
Fixes #14697
Fixes #14698
Might Fixes #14288