usbus: Add CDC-ACM (Serial console) function by bergzand · Pull Request #11085 · RIOT-OS/RIOT

bergzand · 2019-02-28T20:51:03Z

Contribution description

This PR extends the USBUS stack with RIOT serial console over USB.

Output from stdio_write is buffered until a host opens the serial device. This can be configured with USBUS_CDCACM_BUFFER_FOR_DTE. In practice this means that no output produced by stdio_write is discarded.

Testing procedure

Add USEMODULE += stdio_cdc_acm to a project.

Issues/PRs references

depends on #11075

Todo

Prevent hangs when the tsrb is full

kaspar030 · 2019-02-28T21:20:21Z

Output from stdio_write is buffered until a host opens the serial device.

@cladmi that simplifies our job, right? 😉

kaspar030 · 2019-02-28T21:25:02Z

Seems like a dependency to "auto_init_usbus" is missing, just adding stdio_cdc_acm doesn't work.

bergzand · 2019-06-06T09:37:05Z

rebased!

bergzand · 2019-06-06T09:51:57Z

Currently in a broken state, I've neglected this PR a bit while reworking the USBUS API.

MrKevinWeiss · 2019-06-13T12:21:40Z

release status ping, should I change the milestone?

dylad · 2019-06-13T12:24:25Z

@MrKevinWeiss This might be merged before the end of the month.

bergzand · 2019-06-14T21:10:39Z

@dylad Rebased on top of master. It should compile and work again. There is still a lot of missing docs and some magic constants scattered here and there.

bergzand · 2019-06-14T21:11:10Z

Also added a test application as demonstrator

dylad

First round, just some basic stuff

sys/auto_init/usb/auto_init_usb.c

sys/include/usb/cdc/acm.h

sys/include/usb/usbus/cdc/acm.h

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

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

dylad · 2019-06-15T10:10:23Z

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

+    int fd;
+    fd = vfs_bind(STDIN_FILENO, O_RDONLY, &uart_stdio_vfs_ops, (void *)STDIN_FILENO);
+    if (fd < 0) {
+        /* How to handle errors on init? */


Good question tough...

Not my question though, I've copied it from other stdio implementation, the question remained unanswered so far…

Reworked the vfs bits to mimick the implementation from stdio_uart closely.

dylad · 2019-06-15T10:12:27Z

note to other maintainers: I won't be the one to ACK this PR as I authored some part of this code.

tests/usbus_cdc_acm_stdio/Makefile

aabadie

Maybe add a README to the test application like for CDC ECM?
Also I'm wondering how USB stdio is selected over UART (which is the default on samr21-xpro for example). Is this done magically by the build system ?

aabadie · 2019-06-17T07:22:50Z

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

+#include "usb/usbus/cdc/acm.h"
+#include "usb/usbus/control.h"
+
+#include <string.h>


This should be the first include

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

bergzand · 2019-06-17T07:49:40Z

Maybe add a README to the test application like for CDC ECM?

Yeah, should definitely be included.

Also I'm wondering how USB stdio is selected over UART (which is the default on samr21-xpro for example). Is this done magically by the build system ?

Somewhat magically, by only selecting stdio_uart if neither of stdio_rtt and stdio_cdc_acm is selected

I've reworked some small aspects of the code here over the weekend to get line control working. I'll push the changes asap.

Comments were address. I leave it to you.

miri64

Potpourri review (a little of everything: style, clarification questions, ...). Not much to say about the code quality. From what I see it looks correct.

Makefile.dep

makefiles/pseudomodules.inc.mk

sys/include/usb/cdc.h

sys/auto_init/usb/auto_init_usb.c

sys/include/usb/usbus/cdc/acm.h

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

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

miri64 · 2019-07-30T14:48:25Z

Ping @bergzand?

bergzand · 2019-09-30T13:14:29Z

Maybe add a README to the test application like for CDC ECM?

Added

bergzand · 2019-09-30T13:17:07Z

I think I've addressed all the comments here. Anybody willing to give it a last review or give it an ACK?

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

benpicco · 2019-09-30T13:31:50Z

What about the dependency on auto_init_usbus ?

stdio_cdc_acm is not very useful without it.

chrysn · 2019-09-30T13:38:09Z

What about the dependency on auto_init_usbus ?

It needs some initialization, and AFAICT there's no idiom in the dependencies system yet for "pull it in unless the application wants to do it manually", short of which there's no way to do manual initialization once a module pulls in auto_init. (Proposed in #11085 (comment) but probably exceeds this PR).

I'd suggest asking applications to take in auto_init_usbus in their modules list explicitly until such a mechanism exists (as is done with most other auto_inits)

benpicco · 2019-09-30T13:43:21Z

Ok, fair enough. If a board requires stdio_cdc_acm we can just add both to Makefile.dep anyway.

Now just replace that 64 by a define and you'll have my ACK - those magic numbers make me nervous.

benpicco

Code looks good, tested successfully on both Linux and Windows with nrf52.

Please squash!

bergzand · 2019-09-30T16:21:00Z

Please squash!

Squashed!

benpicco

One more nit - just squash directly.

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

bergzand · 2019-09-30T16:42:35Z

Rebased to master while I'm at it

dylad · 2019-09-30T19:24:46Z

Run a quick test on SAME54-XPRO, shell is working but early printf aren't display.
If a reboot (using a reset or reboot command from shell), the line gets disconnect then reconnect but nothing is print from the early boot.

Looks like there is some garbage

> reboot
2019-09-30 21:23:31,648 # Serial port disconnected, waiting to get reconnected...
2019-09-30 21:23:32,653 # Try to reconnect to /dev/ttyACM1 again...
2019-09-30 21:23:32,655 # Reconnected to serial port /dev/ttyACM1
ps
2019-09-30 21:23:36,115 #  rps
2019-09-30 21:23:36,121 # 	pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
2019-09-30 21:23:36,124 # 	  - | isr_stack            | -        - |   - |    512 (  108) | 0x20000000 | 0x200001c8
2019-09-30 21:23:36,125 # 	  1 | idle                 | pending  Q |  15 |    256 (  156) | 0x200004f8 | 0x20000574 
2019-09-30 21:23:36,127 # 	  2 | main                 | running  Q |   7 |   1536 (  692) | 0x200005f8 | 0x20000944 
2019-09-30 21:23:36,129 # 	  3 | usbus                | bl anyfl _ |   1 |   1024 (  392) | 0x20000bf8 | 0x20000f0c 
2019-09-30 21:23:36,130 # 	    | SUM                  |            |     |   3328 ( 1348)
> reboot
2019-09-30 21:23:38,423 # Serial port disconnected, waiting to get reconnected...
2019-09-30 21:23:39,429 # Try to reconnect to /dev/ttyACM1 again...
2019-09-30 21:23:39,431 # Reconnected to serial port /dev/ttyACM1
ps
2019-09-30 21:23:42,140 #  rebooThis is RSBps
2019-09-30 21:23:42,142 # shell: command not found: main():
>

bergzand · 2019-09-30T19:33:28Z

@dylad Not sure I can reproduce your issue, this is what I get on a samr21-xpro:

> ps
2019-09-30 21:31:18,232 #  ps
2019-09-30 21:31:18,234 #       pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
2019-09-30 21:31:18,235 #         - | isr_stack            | -        - |   - |    512 (  148) | 0x20000000 | 0x200001b8
2019-09-30 21:31:18,236 #         1 | idle                 | pending  Q |  15 |    256 (  156) | 0x200004f8 | 0x2000055c 
2019-09-30 21:31:18,237 #         2 | main                 | running  Q |   7 |   1536 (  788) | 0x200005f8 | 0x200008e4 
2019-09-30 21:31:18,238 #         3 | usbus                | bl anyfl _ |   1 |   1024 (  448) | 0x20000bf8 | 0x20000efc 
2019-09-30 21:31:18,238 #           | SUM                  |            |     |   3328 ( 1540)
> reboot
2019-09-30 21:31:20,986 # Serial port disconnected, waiting to get reconnected...
2019-09-30 21:31:22,009 # Try to reconnect to /dev/ttyACM2 again...
2019-09-30 21:31:22,010 # Reconnected to serial port /dev/ttyACM2
2019-09-30 21:31:22,011 #  rebootmain(): This is RIOT! (Version: 2019.10-devel-1127-gc6447b-pr/usb/cdcacm)
2019-09-30 21:31:22,012 # RIOT USB CDC ACM shell test
> ps
2019-09-30 21:31:23,192 #  ps
2019-09-30 21:31:23,194 #       pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
2019-09-30 21:31:23,195 #         - | isr_stack            | -        - |   - |    512 (  148) | 0x20000000 | 0x200001b8
2019-09-30 21:31:23,196 #         1 | idle                 | pending  Q |  15 |    256 (  156) | 0x200004f8 | 0x2000055c 
2019-09-30 21:31:23,197 #         2 | main                 | running  Q |   7 |   1536 (  788) | 0x200005f8 | 0x200008e4 
2019-09-30 21:31:23,198 #         3 | usbus                | bl anyfl _ |   1 |   1024 (  448) | 0x20000bf8 | 0x20000efc 
2019-09-30 21:31:23,198 #           | SUM                  |            |     |   3328 ( 1540)

I expect the semi-garbled reboot at 2019-09-30 21:31:22,011 to be caused by buffering by pyterm.

dylad · 2019-09-30T19:45:27Z

@bergzand I had the same output using minicom so I don't think it is related to pyterm.

benpicco · 2019-10-01T09:16:57Z

The output looks good with putty.exe on same54-xpro.

Can't really test the reset behavior as PuTTY will just exit with an error when the device gets disconnected.

But for garbled output during early boot I'd rather blame some unstable clock.
We shouldn't delay this indefinitely because of that - we can still patch it later.

benpicco · 2019-10-01T09:23:56Z

As discussed on IRC, let's just go ahead with this.
Possible clock / early boot issues can still be sorted out in a follow-up PR, it's beyond the scope of a PR that provides the initial functionality.

bergzand · 2019-10-01T09:25:39Z

Thanks everyone!

keestux · 2019-10-01T18:30:55Z

Thank to you, Koen. Great work

bergzand added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 28, 2019

bergzand requested review from dylad and kaspar030 February 28, 2019 20:51

bergzand added the Area: USB Area: Universal Serial Bus label Mar 18, 2019

MrKevinWeiss added this to the Release 2019.07 milestone Jun 3, 2019

miri64 mentioned this pull request Jun 5, 2019

tsrb: change input type to uint8_t #11634

Merged

dylad removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 5, 2019

miri64 mentioned this pull request Jun 5, 2019

feather-m0: add usbdev feature #11636

Merged

bergzand force-pushed the pr/usb/cdcacm branch from b7e0ced to d2acdf2 Compare June 6, 2019 09:36

bergzand force-pushed the pr/usb/cdcacm branch from d2acdf2 to 728642d Compare June 14, 2019 21:08

dylad previously requested changes Jun 15, 2019

View reviewed changes

miri64 reviewed Jun 15, 2019

View reviewed changes

tests/usbus_cdc_acm_stdio/Makefile Outdated Show resolved Hide resolved

aabadie reviewed Jun 17, 2019

View reviewed changes

miri64 previously requested changes Jun 20, 2019

View reviewed changes

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

miri64 self-assigned this Jul 30, 2019

benpicco reviewed Sep 30, 2019

View reviewed changes

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

benpicco reviewed Sep 30, 2019

View reviewed changes

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

benpicco approved these changes Sep 30, 2019

View reviewed changes

bergzand force-pushed the pr/usb/cdcacm branch from 4a0fa5b to 325cc4e Compare September 30, 2019 16:20

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

benpicco reviewed Sep 30, 2019

View reviewed changes

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

bergzand force-pushed the pr/usb/cdcacm branch from 325cc4e to 1ecb165 Compare September 30, 2019 16:40

bergzand added 3 commits September 30, 2019 18:40

usb cdc acm: add common defines for cdc acm

802012c

usbus: Add cdc acm function

852b7c8

USBUS cdc acm: Add STDIO wrapper for CDC ACM

59743ae

bergzand force-pushed the pr/usb/cdcacm branch from 1ecb165 to 4b7feb1 Compare September 30, 2019 16:42

test/usbus_cdc_acm_stdio: USB CDC ACM STDIO test

c6447b7

bergzand force-pushed the pr/usb/cdcacm branch from 4b7feb1 to c6447b7 Compare September 30, 2019 16:54

benpicco merged commit 3f0dfc1 into RIOT-OS:master Oct 1, 2019

bergzand deleted the pr/usb/cdcacm branch October 1, 2019 09:44

ant9000 mentioned this pull request Oct 7, 2019

CDC-ACM (serial console) loses characters #12384

Closed

Conversation

bergzand commented Feb 28, 2019

Contribution description

Testing procedure

Issues/PRs references

Todo

Uh oh!

kaspar030 commented Feb 28, 2019

Uh oh!

kaspar030 commented Feb 28, 2019

Uh oh!

bergzand commented Jun 6, 2019

Uh oh!

bergzand commented Jun 6, 2019

Uh oh!

MrKevinWeiss commented Jun 13, 2019

Uh oh!

dylad commented Jun 13, 2019

Uh oh!

bergzand commented Jun 14, 2019

Uh oh!

bergzand commented Jun 14, 2019

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylad Jun 15, 2019

Choose a reason for hiding this comment

Uh oh!

bergzand Jun 15, 2019

Choose a reason for hiding this comment

Uh oh!

bergzand Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

dylad commented Jun 15, 2019

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bergzand commented Jun 17, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Jul 30, 2019

Uh oh!

bergzand commented Sep 30, 2019

Uh oh!

bergzand commented Sep 30, 2019

Uh oh!

Uh oh!

Uh oh!

benpicco commented Sep 30, 2019

Uh oh!

chrysn commented Sep 30, 2019

Uh oh!

benpicco commented Sep 30, 2019

Uh oh!

benpicco commented Oct 1, 2019 •

edited

Loading