Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/tinyusb: implement stdio via CDC ACM #18804

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 26, 2022

Contribution description

This hooks up tinyUSB CDC ACM to stdio.
Some more work is needed so this can be used as default stdio on e.g. esp32s2-mini:

  • [ ] Use XFA for common usb_descriptors.c

So we can use application unmodified without providing a custom usb_descriptors.c.

Testing procedure

Flash the tests/pkg_tinyusb_cdc_acm_stdio application on any supported board.

Issues/PRs references

@benpicco benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 26, 2022
@github-actions github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: pkg Area: External package ports Area: tests Area: tests and testing framework Platform: AVR Platform: This PR/issue effects AVR-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Oct 26, 2022
@gschorcht
Copy link
Contributor

Funny, I was working on the same feature in my branch pkg/tinyusb_stdio commit a3048979f18372fc67117989ac3f026d92a4de41 😉 I would drop my work in favor of yours because yours seems to be finished while mine was still in progress.

@gschorcht
Copy link
Contributor

So we can use application unmodified without providing a custom usb_descriptors.c.

BTW, I'm working on a pkg/contrib/tinyusb_descriptors.c implementation which defines the descriptors and handles them for standard cases according to used device classes (defined by the tinyusb_class_* modules) so that an application hasn't to define any usb_descriptors.c.

pkg/tinyusb/doc.txt Show resolved Hide resolved
makefiles/stdio.inc.mk Outdated Show resolved Hide resolved
makefiles/stdio.inc.mk Outdated Show resolved Hide resolved
pkg/tinyusb/Makefile.dep Outdated Show resolved Hide resolved
tests/pkg_tinyusb_cdc_acm_stdio/Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,317 @@
/*
Copy link
Contributor

@gschorcht gschorcht Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this file, it makes little sense to redefine it again, since most of the parts are always the same. Furthermore, an application can't use other device classes if module tinyusb_cdc_acm_stdio is used.

That's why I'm working on a common tinyusb_descriptors.c file that uses the configuration and the Kconfig CONFIG_* symbols from the usbus to implement the descriptors. Once I have finished that work, we should be able to drop this file and it should be possible to use tinyusb_stdio together with other device class just by enabling the according tinyusb_class_* modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you already have a common tinyusb_descriptors.c in the works, I'll just wait for that 😃

Moving this to XFA seems to be a bit tricky, you'll likely be faster with providing a generic solution as you already have experience with this package.

@benpicco
Copy link
Contributor Author

I renamed the module stdio_tinyusb_cdc_acm for consistency, but now I'm getting

Error - using unknown modules: stdio_tinyusb_cdc_acm
make: *** [/home/benpicco/dev/RIOT/tests/pkg_tinyusb_cdc_acm_stdio/../../Makefile.include:734: ..module-check] Error 1
make: *** Waiting for unfinished jobs....
/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/tests/pkg_tinyusb_cdc_acm_stdio/bin/same54-xpro/cpu/cpu.o: in function `cpu_init':
/home/benpicco/dev/RIOT/cpu/samd5x/cpu.c:386: undefined reference to `stdio_init'
/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/tests/pkg_tinyusb_cdc_acm_stdio/bin/same54-xpro/newlib_syscalls_default/syscalls.o: in function `_read_r':
/home/benpicco/dev/RIOT/sys/newlib_syscalls_default/syscalls.c:507: undefined reference to `stdio_read'
/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/tests/pkg_tinyusb_cdc_acm_stdio/bin/same54-xpro/newlib_syscalls_default/syscalls.o: in function `_write_r':
/home/benpicco/dev/RIOT/sys/newlib_syscalls_default/syscalls.c:520: undefined reference to `stdio_write'
collect2: error: ld returned 1 exit status

what kind of auto-magic am I missing?

@gschorcht
Copy link
Contributor

gschorcht commented Oct 28, 2022

Target stdio_tinyusb_cdc_acm is not build because it is not in the list of filtered modules in target all.

diff --git a/pkg/tinyusb/Makefile b/pkg/tinyusb/Makefile
index 770f956d1a..6bc7c7afd7 100644
--- a/pkg/tinyusb/Makefile
+++ b/pkg/tinyusb/Makefile
@@ -11,7 +11,7 @@ PSRC = $(PKG_SOURCE_DIR)/src
 
 .PHONY: all
 
-all: $(filter tinyusb_%,$(USEMODULE))
+all: $(filter stdio_tinyusb_cdc_acm tinyusb_%,$(USEMODULE))
        $(QQ)"$(MAKE)" -C $(PSRC) -f $(RIOTBASE)/Makefile.base MODULE=tinyusb
 
 stdio_tinyusb_cdc_acm:

@benpicco benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 2, 2022
@gschorcht
Copy link
Contributor

@benpicco

BOARD=native make -j8 -C examples/hello-world/

fails due to multiple definitions of several stdio symbols.

@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 8, 2022
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 9, 2022
@riot-ci
Copy link

riot-ci commented Nov 9, 2022

Murdock results

✔️ PASSED

ea0cf85 tests/pkg_tinyusb_cdc_acm_stdio: add test for stdio via CDC ACM

Success Failures Total Runtime
2002 0 2002 07m:05s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

pkg/tinyusb/doc.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to squash it directly.

pkg/tinyusb/doc.txt Outdated Show resolved Hide resolved
pkg/tinyusb/doc.txt Outdated Show resolved Hide resolved
@gschorcht gschorcht merged commit de266af into RIOT-OS:master Nov 9, 2022
@benpicco benpicco deleted the tinyusb_cdc_acm_stdio branch November 9, 2022 14:07
@benpicco
Copy link
Contributor Author

benpicco commented Nov 9, 2022

thank you for the common USB descriptors!

@gschorcht
Copy link
Contributor

Next task on my ToDo list is a tinyUSB netdev driver for CDC ECM.

gschorcht added a commit to gschorcht/RIOT-Xtensa-ESP that referenced this pull request Nov 13, 2022
With PR RIOT-OS#18804 the approach to override the default tinyUSB configuration was changed. The update of the documentation was forgotten.
@gschorcht
Copy link
Contributor

@benpicco Now that we have the stdio implementation for tinyUSB, we could theoretically enable the tinyusb_device feature even for boards that have only one USB interface and use stdio_cdc_acm when the highlevel_stdio feature is enabled.

Before this PR, we couldn't enable tinyUSB for such boards due to the conflict between periph_usbdev and tinusb_device. Now we could select

  • stdio_tinyusb_cdc_acm if highlevel_stdio and tinyusb_device are enabled or
  • stdio_cdc_acm if highlevel_stdio and periph_usbdev are enable

What do you think?

@benpicco
Copy link
Contributor Author

Sounds good!

@gschorcht
Copy link
Contributor

Sounds good!

I will provide a PR in next few days.

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Einhornhool pushed a commit to Einhornhool/RIOT that referenced this pull request Jan 24, 2023
With PR RIOT-OS#18804 the approach to override the default tinyUSB configuration was changed. The update of the documentation was forgotten.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: pkg Area: External package ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants