Fix to avoid lost characters on USBUS CDC ACM STDIO #12402

ant9000 · 2019-10-09T09:28:22Z

This small commit implements a fix for #12384 , and modifies the provided test to make sure it is actually fixed.

sys/usb/usbus/cdc/acm/cdc_acm_stdio.c

benpicco · 2019-10-09T15:58:52Z

Please squash!
Maybe @bergzand has some comments still?

Also tests/usbus_cdc_acm_stdio looks kind of ugly now - maybe there is some better test output you could use? Like Lorem ipsum or the Free Software Song - be creative 😉

benpicco · 2019-10-09T16:23:29Z

What happens when you unplug the device (and it's not powered over that USB connection)

Will it get stuck in that loop forever or will the bytes be discarded?

ant9000 · 2019-10-09T16:32:47Z

What happens when you unplug the device (and it's not powered over that USB connection)
Will it get stuck in that loop forever or will the bytes be discarded?

Latest commit implements the behaviour I was expecting. The driver will buffer data, without blocking. When the bufffer gets full, it will start discarding the oldest bytes.

sys/usb/usbus/cdc/acm/cdc_acm.c

dylad

Just some nitpicks on coding style.

sys/usb/usbus/cdc/acm/cdc_acm.c

MrKevinWeiss · 2019-11-15T10:12:42Z

@bergzand Have your comments been addressed?

fjmolinas · 2020-01-10T08:59:39Z

ping @bergzand!

fjmolinas · 2020-01-13T11:58:21Z

@bergzand will you have time to look at this one before 2020.01.15?

bergzand

Sorry for the delays, two remaining comments from my side. Code otherwise looks okay to me. I'm happy to ack this if somebody can give it a final test.

sys/usb/usbus/cdc/acm/cdc_acm.c

fjmolinas · 2020-01-14T21:22:03Z

@ant9000 would you have edit time to address? I would like to get this in before the hard feature freeze tomorrow.

bergzand

Final round of nitpicks, thank you for your patience so far!

sys/usb/usbus/cdc/acm/cdc_acm.c

bergzand · 2020-01-15T09:55:28Z

This should be good to go, for good measure I will give this a final test this evening. Feel free to squash to a reasonable set of commits in the mean time.

fjmolinas · 2020-01-15T09:57:32Z

Testing:

reproduce issue on master cherry-picking 09d3d4d

The test doesn't seem to be a valid way of reproducing the issue.

2020-01-15 10:46:08,991 # text 1024
2020-01-15 10:46:08,993 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:08,994 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:08,995 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:08,996 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:08,997 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:08,999 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,000 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,001 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,002 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,004 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,006 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,007 # 01234567890123456789012345678901234567890123456789012345678901234567890123456789
2020-01-15 10:46:09,009 # 0123456789012345678901234567890123456789012345678901234567890123

on the other hand if I reproduce the testing procedur in CDC-ACM (serial console) loses characters #12384

MASTER

2020-01-15 10:56:47,933 #  reboot
2020-01-15 10:56:48,016 # Serial port disconnected, waiting to get reconnected...
2020-01-15 10:56:49,031 # Try to reconnect to /dev/ttyACM1 again...
2020-01-15 10:56:49,032 # Reconnected to serial port /dev/ttyACM1
2020-01-15 10:56:49,033 # main(): This is RIOT! (Version: 2020.01-devel-1726-g19774-HEAD)
2020-01-15 10:56:49,033 # RIOT USB CDC ACM shell test
2020-01-15 10:56:49,033 # 1 RIOT USB CDC ACM shell test

THIS PR

2020-01-15 10:54:20,812 #  reboot
2020-01-15 10:54:21,063 # Serial port disconnected, waiting to get reconnected...
2020-01-15 10:54:22,084 # Try to reconnect to /dev/ttyACM1 again...
2020-01-15 10:54:22,085 # Reconnected to serial port /dev/ttyACM1
2020-01-15 10:54:22,085 #  test
2020-01-15 10:54:22,085 # 2 RIOT USB CDC ACM shell test
2020-01-15 10:54:22,085 # 3 RIOT USB CDC ACM shell test
2020-01-15 10:54:22,086 # 4 RIOT USB CDC ACM shell test
2020-01-15 10:54:22,086 # 5 RIOT USB CDC ACM shell test

BTW @bergzand any tips of terminal to use to not get a serial disconnect every time the device is rebooted?

bergzand · 2020-01-15T10:10:19Z

BTW @bergzand any tips of terminal to use to not get a serial disconnect every time the device is rebooted?

Not really, when rebooting the device, the USB connection between the host and device is lost and the host will (and should) remove the ttyACM%u device and add it after re-initializing the USB connection.

fjmolinas · 2020-01-15T10:12:50Z

@bergzand are there any other tests you wold like to be ran? I can perform them for you.

fjmolinas · 2020-01-15T10:33:29Z

Feel free to squash to a reasonable set of commits in the mean time.

@ant9000 please squash! we can avoid running the ci twice.

fixes RIOT-OS#12384

bergzand · 2020-01-15T10:51:44Z

The test doesn't seem to be a valid way of reproducing the issue.

Is the output the same with this PR and on master? On master it should truncate the output at some point, with this PR it should instead remove old content and show output from some point until the end (expressed in slices it would be something like: master: output[:CDC_ACM_BUF_SIZE] and with this PR it would be output[-CDC_ACM_BUF_SIZE:])

@fjmolinas The interesting behavior is in the connects and disconnects of the terminal. The USB peripheral receives an USB notification when an application (such as miniterm.py or pyserial) opens the ttyACM%u device. The code correctly handling buffer limits while the terminal is closed is something that is the most interesting to test here. I think you've tested this in the last test you performed.

Other than that the tests/usbus_cdc_acm_stdio/ should still provide a decent shell experience :)

fjmolinas · 2020-01-15T12:11:50Z

@fjmolinas The interesting behavior is in the connects and disconnects of the terminal. The USB peripheral receives an USB notification when an application (such as miniterm.py or pyserial) opens the ttyACM%u device. The code correctly handling buffer limits while the terminal is closed is something that is the most interesting to test here. I think you've tested this in the last test you performed.

In that case I would count as tested. But I get the same output if just using the text cmd without any serial connect or disconnect.

Other than that the tests/usbus_cdc_acm_stdio/ should still provide a decent shell experience :)

Still the case.

bergzand

Ack!

bergzand self-requested a review October 9, 2019 09:29

bergzand added Area: USB Area: Universal Serial Bus Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 9, 2019

benpicco reviewed Oct 9, 2019

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm_stdio.c Outdated Show resolved Hide resolved

benpicco requested a review from dylad October 9, 2019 15:51

benpicco reviewed Oct 9, 2019

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

benpicco reviewed Oct 9, 2019

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

bergzand requested changes Oct 9, 2019

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm.c Show resolved Hide resolved

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch 6 times, most recently from e73c224 to ccc113c Compare October 11, 2019 06:34

ant9000 mentioned this pull request Oct 11, 2019

CDC-ACM (serial console) loses characters #12384

Closed

USBUS CDC ACM STDIO: flush stdio buffer upon write

b0b2ba6

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch from ccc113c to c6441a8 Compare October 14, 2019 08:59

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 14, 2019

benpicco requested a review from bergzand October 19, 2019 14:34

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch from c6441a8 to ecc39aa Compare October 19, 2019 18:25

dylad reviewed Oct 20, 2019

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

USBUS CDC ACM: discard oldest data if unconnected

01c3043

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch from ecc39aa to 09d3d4d Compare October 20, 2019 17:00

fjmolinas added this to the Release 2020.01 milestone Jan 10, 2020

bergzand requested changes Jan 14, 2020

View reviewed changes

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

sys/usb/usbus/cdc/acm/cdc_acm.c Outdated Show resolved Hide resolved

fjmolinas self-requested a review January 14, 2020 21:22

bergzand requested changes Jan 15, 2020

View reviewed changes

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch from fc3fa5b to 13be6f0 Compare January 15, 2020 09:47

ant9000 added 2 commits January 15, 2020 11:37

USBUS CDC ACM: disable interrupts when using tsrb

6c04cb1

USBUS CDC ACM STDIO: test for the buffering code

d1cc563

fixes RIOT-OS#12384

ant9000 force-pushed the pr/usbus_cdc_acm_stdio_fix branch from d86e6b6 to d1cc563 Compare January 15, 2020 10:38

bergzand approved these changes Jan 15, 2020

View reviewed changes

bergzand merged commit 5fd981b into RIOT-OS:master Jan 15, 2020

ant9000 deleted the pr/usbus_cdc_acm_stdio_fix branch January 16, 2020 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to avoid lost characters on USBUS CDC ACM STDIO #12402

Fix to avoid lost characters on USBUS CDC ACM STDIO #12402

ant9000 commented Oct 9, 2019

benpicco commented Oct 9, 2019

benpicco commented Oct 9, 2019

ant9000 commented Oct 9, 2019

dylad left a comment

MrKevinWeiss commented Nov 15, 2019

fjmolinas commented Jan 10, 2020

fjmolinas commented Jan 13, 2020

bergzand left a comment

fjmolinas commented Jan 14, 2020 •

edited

bergzand left a comment

bergzand commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

bergzand commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

bergzand commented Jan 15, 2020 •

edited

fjmolinas commented Jan 15, 2020

bergzand left a comment

Fix to avoid lost characters on USBUS CDC ACM STDIO #12402

Fix to avoid lost characters on USBUS CDC ACM STDIO #12402

Conversation

ant9000 commented Oct 9, 2019

benpicco commented Oct 9, 2019

benpicco commented Oct 9, 2019

ant9000 commented Oct 9, 2019

dylad left a comment

Choose a reason for hiding this comment

MrKevinWeiss commented Nov 15, 2019

fjmolinas commented Jan 10, 2020

fjmolinas commented Jan 13, 2020

bergzand left a comment

Choose a reason for hiding this comment

fjmolinas commented Jan 14, 2020 • edited

bergzand left a comment

Choose a reason for hiding this comment

bergzand commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

bergzand commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

fjmolinas commented Jan 15, 2020

bergzand commented Jan 15, 2020 • edited

fjmolinas commented Jan 15, 2020

bergzand left a comment

Choose a reason for hiding this comment

fjmolinas commented Jan 14, 2020 •

edited

bergzand commented Jan 15, 2020 •

edited