boards: enable tinyUSB for boards that use the `highlevel_stdio` feature with a single USB interface #18998

gschorcht · 2022-11-30T19:55:38Z

Contribution description

This PR enables the tinyUSB feature for boards that have a single USB and use the highlevel_stdio feature and the USBUS CDC ACM interface stdio_cdc_acm.

Since the tinyUSB package and the USBUS implementation are not compatible and can't be used simultaneously, the tinyUSB feature wasn't be enabled for boards that have only a single USB and use the highlevel_stdio feature and the USBUS CDC ACM interface as stdio.

Now that we also have a stdio implementation for the tinyUSB CDC ACM interface (PR #18804), the highlevel_stdio feature uses the stdo_tinyusb_cdc_acm module if any tinyUSB device class and thus the tinyUSB stack is used. The tinyUSB feature can therefore also be enabled for all boards that have only a single USB interface and use the highlevel_stdio feature.

Testing procedure

Use a board with a single USB interface that uses the highlevel_stdio feature and USBUSB CDC ACM, for example:

USEMODULE=tinyusb_device BOARD=arduino-mkr1000 make -C tests/shell flash

Issues/PRs references

Depends on PR #19006
Depends on PR #19007

dylad · 2022-11-30T20:59:58Z

pkg/tinyusb/contrib/tinyusb_descriptors.c

+#define USB_H_USER_IS_RIOT_INTERNAL
+


Out of the scope but why is this needed ?

Yes, if the user doesnt define CONFIG_USB_VIDandCONFIG_USB_PID, the default RIOT peripheral VID/PID 1209:7d00is used only ifUSB_H_USER_IS_RIOT_INTERNALis defined when [sys/include/usb.h](https://github.com/RIOT-OS/RIOT/blob/master/sys/include/usb.h) is included. Otherwise the compilation fails with the error message becauseCONFIG_USB_VIDandCONFIG_USB_PID` are used for the device descriptor:

RIOT/sys/include/usb.h

Lines 48 to 56 in df0c04e

#if !(defined(CONFIG_USB_VID) && defined(CONFIG_USB_PID))

#ifdef USB_H_USER_IS_RIOT_INTERNAL

#define CONFIG_USB_VID INTERNAL_PERIPHERAL_VID

#define CONFIG_USB_PID INTERNAL_PERIPHERAL_PID

#else

#error Please configure your vendor and product IDs. For development, you may \

set USB_VID=${USB_VID_TESTING} USB_PID=${USB_PID_TESTING}.

#endif

#endif

We didn't detect this problem before using tinyUSB in applications other than the tests, because the tests use the test VID/PID.

Also the DFU bootloader sets the VID/PID pair.

riot-ci · 2022-12-01T06:05:47Z

Murdock results

❌ FAILED

b2a2e05 boards: enable tinyUSB feature

Build failures (1)

Application	Target	Toolchain	Runtime (s)	Worker
examples/hello-world	adafruit-itsybitsy-nrf52	gnu	0.81	breeze

Artifacts

If `TEST_KCONFIG` is set and the board provides the `highlevel_stdio` feature, the `stdio_cdc_acm` module is enabled by default in Kconfig. This setting was previously done unconditionally either by a `.config` file of the board or the bootloader `.config` file (`boards/common/samx1d-arduino-booloader.config`, `nrf52_bootloader.confg`). Changing this approach became necessary to allow enabling other `stdio_*` modules or other USB stacks like tinyUSB for these boards.

Default configuration is now realized in Kconfig files.

This file is not needed any longer with the conditional enable of ´stdio_cdc_acm` and `usbus_*` modules.

Enables the `tinyusb_device` for boards with only one USB interface, where `highlevel_stdio` uses the USBUS CDC ACM interface so far. If any `tinyusb_device` class is enabled the `highlevel_stdio` uses now `stdio_tinyusb_cdc_acm` instead of the USBUS `stdio_cdc_acm`. Therefore, the `tinyusb_device` feature can be enabled now for such boards.

gschorcht · 2022-12-09T08:35:36Z

@MrKevinWeiss With this PR (commits b362dad, bbe3a9c and 5d52077) I provided a change of the approach of how the unconditional enable of the stdio_cdc_acm and usbus* modules when TEST_KCONFIG is set and the highlevel_stdio feature is provided by the board. This change of the approach is functionally the same as using .config files but allows to use also the stdio_tinyusb_cdc_acm instead of stdio_cdc_acm when the tinyusb_device module is enabled. It also cleans up the include of the .config files in Makefiles.features. Could you take a look please, whether the changed approach would be acceptable?

MrKevinWeiss · 2022-12-12T12:24:15Z

I am in support of dropping the .config files as it does make it difficult for conditionals. @leandrolanzieri who has thought about this much longer and harder that I may have some issues with that.

Can I simplify this as "If I am using STDIO with a board supports TINYUSB and requires the HIGHLEVEL_STDIO then use the TINYUSB implementation for my USB, if there is no TINYUSB implementation then use STDIO_CDC_ACM"?

leandrolanzieri · 2022-12-12T12:33:02Z

sys/usb/usbus/Kconfig

@@ -9,6 +9,7 @@ menuconfig MODULE_USBUS
 bool "USB Unified Stack (USBUS)"
 depends on TEST_KCONFIG
 depends on HAS_PERIPH_USBDEV || MODULE_USBDEV_MOCK
+ default y if TEST_KCONFIG && HAS_HIGHLEVEL_STDIO && !MODULE_TINYUSB_DEVICE


I believe the natural way to model two mutually exclusive implementations in Kconfig would be a choice, like we're doing for stdio and others. Do you think that would be possible here? This would not scale well when adding additional backends.

I believe the natural way to model two mutually exclusive implementations in Kconfig would be a choice.

I agree with this in principle. The problem I see is that USBUS and its device classes are not configured in a separate submenu but are fully integrated into the System configuration menu. On top level the user can select the USBUS

[ ] USB Unified Stack (USBUS) ----

but if it is selected, the System configuration menu is extended to

[*] USB Unified Stack (USBUS) ---> [ ] USB CDC ACM support (NEW) [ ] USB HID support (NEW) ----

and if USB CDC ACM support is selected the System configuration menu is further extended to

[*] USB Unified Stack (USBUS) ---> [*] USB CDC ACM support [ ] USB HID support ---- [ ] Trigger a board reset via USB CDC ACM (NEW)

The tinyUSB configuration on the other hand starts in the Packages configuration menu, because it is a package. But unlike USBUS, tinyUSB configuration is done completely in separate submenus.

If we want to provide a more natural solution, there should be just an entry like

[ ] USB interface ----

in the System configuration menu, which should lead to a new submenu where the user first selects the implementation

USB implementation (USBUS) ---> [ ] ...

and all other configurations appear according to the selected implementation. The tinyUSB package would be an hidden option in that case and enabled automatically if tinyUSB implementation is selected.

Right?

That is very true, we should be able to do this with some choice overrides, in which case tinyusb would be a choice. If you don't mind I would like to give an update to the usb modelling a try?

leandrolanzieri · 2022-12-12T12:36:39Z

sys/Kconfig.stdio

@@ -11,6 +11,8 @@ menu "Standard Input/Output (STDIO)"
 choice STDIO_IMPLEMENTATION
 bool "STDIO implementation"
 default MODULE_STDIO_NATIVE if CPU_ARCH_NATIVE
+ default MODULE_STDIO_CDC_ACM if TEST_KCONFIG && HAS_HIGHLEVEL_STDIO && !MODULE_TINYUSB_DEVICE


Checking for TEST_KCONFIG is not required, as it is inherited from the menu dependency.
I'm wondering whether now, that stdio_cdc_acm is also implemented by tinyUSB, MODULE_STDIO_CDC_ACM shouldn't be moved to this file? It could even be made a menuconfig with a choice to select the backend (see my other comment on this).

gschorcht · 2022-12-12T16:53:54Z

Can I simplify this as "If I am using STDIO with a board supports TINYUSB and requires the HIGHLEVEL_STDIO then use the TINYUSB implementation for my USB, if there is no TINYUSB implementation then use STDIO_CDC_ACM"?

Unfortunatly not 😟 I think the the USBUS SDTIO implementation stdio_cdc_acm should remain the default option. The tinyUSB STDIO implementation stdio_tinyusb_cdc_acm should only be used if the tinyUSB stack is used anyway for some reason and the board provides the highlevel_stdio feature.

19086: Remodel the USB in Kconfig r=aabadie a=MrKevinWeiss ### Contribution description #### The issues with current architecture Generally there has been some confusion on how to manage KConfig with respect to the board selection of default STDIO backends, specifically for boards that require a USB based backend as there are possible stacks to use. The `<BOARD>.config` way of selecting cannot handle conditional selects. The issues is more with boards such as `esp32s2-wemos-mini`, currently some USB stack will be selected regardless of overridding the preferred STDIO. Selecting a USB stack directly with `STDIO_USB*` creates some circular dependency issues with kconfig and is hard to manage. We also have a mutually exclusive USB stacks, TINYUSB or USBUS which should probably be a choice. #### Desired behaviour 1. Ideally we want a board to default to the most obvious STDIO implementation, for example, if I have nucleo, it uses a UART, for some ESPs, USB is the default way to communicate. 2. These backends could always be overridden though, for example, I may just connect directly to a UART and want my STDIO there, or maybe use a ble based STDIO. 3. The next condition would be specifically for boards with a USB based STDIO. Since we have a TINYUSB stack and a USBUS stack we would want to use the associated STDIO depending on the stack the application selects. 4. However, if nothing is selected by the application, than bring in a USB stack (board based preference) unless there is a specific non-USB based STDIO is selected. For these boards that have this requirement, we DO NOT want to bring in the USB stack if the STDIO is specifically overridden (important for kconfig). #### Update kconfiglib package to RIOT-OS org managed one There is a problem with the upstreamed Kconfiglib implementation and the maintainer is not responsive to the fix. The issue is to do with `menuconfig`s in choices and has been fixed with the RIOT-OS based fork. This PR requires this fix. #### Changes to the USB stack A new entry point is introduced `USB_DEVICE` which indicates wanting a USB device but not caring which stack is used. This allows making a `choice` between the `TINYUSB` and `USBUS` stack allowing mutual exclusivity. Making the USB stack a `choice` means that a specific stack cannot be selected from non-board/non-cpu/non-application based symbols. Thus the `REQUIRES_` design pattern is used for a module to indicate a specific stack should be selected. This is needed for the `MODULE_TINYUSB_NETDEV` in this case. #### Changes to USB STDIO implementations The `MODULE_STDIO_CDC_ACM` and `MODULE_STDIO_TINYUSB_CDC_ACM` are both depends on now, using a `REQUIRES_USB_STDIO` to select the dependencies. This means we do not have to use `select PACKAGE_TINYUSB if TEST_KCONFIG && !MODULE_USBUS` in the board select. ##### Why not just select the USB from STDIO_USB Issue with using select for STDIO choices is that we cannot check which stack we are using to default the STDIO to that, breaking desired behaviour 3. #### The `FORCE_USB_STDIO` Desired behaviour 4 means that we do not want to bring in the USB stack if we override, say, to the UART STDIO backend. Due to the limitations of Kconfig, this is my solution to prevent the USB from being brought in if there is an STDIO that doesn't need it. It is only for the `esp32s2-wemos-mini` board and would not be used in other places and would only need to be explicitly disabled for applications requiring different STDIO backend and no USB. It is not perfect but I think the best solution and fairly understandable... <details><summary><h4>Issues with Kconfig</h4></summary> When using a `choice` and having conditional defaults, for example: ```kconfig choice IMPL default FOO if CHOOSE_FOO default BAR ``` there is a limitation of the level of the level of knowledge that can be expected from Kconfig, a limitation on circular dependencies, and a limitation that the dependencies only get resolved once. For example, if ` BAR` selects something that would eventually select `CHOOSE_FOO`, then the default should be `FOO` and which would no longer select `BAR` preventing the select `CHOOSE_FOO`... Messy stuff and we would want an error saying no no no. What Kconfig cannot handle is something like: ```kconfig choice IMPL bool "Implementation" default FOO if CHOOSE_FOO default BAR config FOO bool "Foo" config BAR bool "Bar" endchoice config CHOOSE_FOO bool config SYMBOL bool select CHOOSE_FOO if !BAR ``` `SYMBOL` causes a circular dependency in Kconfig even though the only possible outcome for the `choice` selection would be static. If we select `BAR` then `CHOOSE_FOO` would not be selected and we stay with `BAR`. If we select `FOO` than `CHOOSE_FOO` will be selected which stays with `FOO`. Everything should be fine, but isn't because Kconfig does not resolve to that degree, it simply sees that there is a dependency of the `IMPL` choice outcome (ie. `if !BAR`) that is a condition for a dependency of the `IMPL` choice selection (ie. ` if CHOOSE_FOO`). This is a limitation of the Kconfig what what makes this problem so challenging, with Make we say "select some sort of USB backend if no other stdio is specifically requested" and it will. </details> An attempt at remodelling the dependencies of the USB stack in Kconfig. Currently there are some issues, especially with the integration of TinyUSB package as a backend. This will require a kconfiglib package fix though... ### Testing procedure `TEST_KCONFIG=1 BOARD=reel make menuconfig -C examples/hello-world` ### Issues/PRs references Requires ulfalizer/Kconfiglib#123 to be merged upstream or fork for RIOT Relates maybe to #18998 and #19038 19672: pkg/micropython: model in Kconfig r=aabadie a=aabadie Co-authored-by: MrKevinWeiss <weiss.kevin604@gmail.com> Co-authored-by: Alexandre Abadie <alexandre.abadie@inria.fr>

gschorcht requested review from leandrolanzieri, jia200x, MrKevinWeiss, aabadie and haukepetersen as code owners November 30, 2022 19:55

github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: USB Area: Universal Serial Bus labels Nov 30, 2022

gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 30, 2022

gschorcht force-pushed the boards/add_tinyusb_feature branch from 7abd2ed to cbae692 Compare November 30, 2022 20:01

dylad reviewed Nov 30, 2022

View reviewed changes

This was referenced Dec 3, 2022

bootloaders/riotboot_dfu: fixes including sys/usb/usbus/dfu #18964

Merged

pkg/tinyusb: add usb_board_reset support #19006

Merged

pkg/tinyusb: add Kconfig support for stdio_tinyusb_cdc_acm #19007

Merged

benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 7, 2022

gschorcht mentioned this pull request Dec 7, 2022

pkg/tinyusb: fix Kconfig problems #19023

Merged

gschorcht added 3 commits December 8, 2022 14:09

boards: remove samdx1-arduino-bootloader.config

bbe3a9c

Default configuration is now realized in Kconfig files.

boards: remove nrf52_bootloader.config

5d52077

gschorcht force-pushed the boards/add_tinyusb_feature branch from cbae692 to c8a3b73 Compare December 8, 2022 13:59

github-actions bot removed the Area: pkg Area: External package ports label Dec 8, 2022

gschorcht added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 8, 2022

gschorcht added 5 commits December 8, 2022 18:50

boards/stm32f429i-disco: remoce .config

cb7e2a4

This file is not needed any longer with the conditional enable of ´stdio_cdc_acm` and `usbus_*` modules.

boards/common/samdx1-arduino-bootloader: support tinyUSB stdio

2018ca0

boards/common/nrf52: add tinyUSB support to bootloader

7e5cbff

sys: add tinyUSB support?????

b2c855a

gschorcht force-pushed the boards/add_tinyusb_feature branch from c8a3b73 to b2a2e05 Compare December 8, 2022 17:51

gschorcht mentioned this pull request Dec 11, 2022

sys/usbus: changed approach of default activation of stdio_cdc_acm in Kconfig #19038

Open

leandrolanzieri reviewed Dec 12, 2022

View reviewed changes

MrKevinWeiss mentioned this pull request Jan 3, 2023

Remodel the USB in Kconfig #19086

Merged

This was referenced Feb 7, 2023

boards/stm32f4discovery: default to stdio via CDC ACM #19259

Merged

boards/weact-f4x1cx: enable tinyUSB #19275

Closed

mguetschow mentioned this pull request Jul 4, 2024

boards/*nrf52*: Enable tinyusb support for all nrf52-based boards #20774

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards: enable tinyUSB for boards that use the `highlevel_stdio` feature with a single USB interface #18998

boards: enable tinyUSB for boards that use the `highlevel_stdio` feature with a single USB interface #18998

gschorcht commented Nov 30, 2022 •

edited

Loading

dylad Nov 30, 2022

gschorcht Dec 1, 2022

gschorcht Dec 1, 2022

riot-ci commented Dec 1, 2022 •

edited

Loading

gschorcht commented Dec 9, 2022

MrKevinWeiss commented Dec 12, 2022

leandrolanzieri Dec 12, 2022

gschorcht Dec 12, 2022

MrKevinWeiss Dec 15, 2022

leandrolanzieri Dec 12, 2022

gschorcht commented Dec 12, 2022

	#if !(defined(CONFIG_USB_VID) && defined(CONFIG_USB_PID))
	#ifdef USB_H_USER_IS_RIOT_INTERNAL
	#define CONFIG_USB_VID INTERNAL_PERIPHERAL_VID
	#define CONFIG_USB_PID INTERNAL_PERIPHERAL_PID
	#else
	#error Please configure your vendor and product IDs. For development, you may \
	set USB_VID=${USB_VID_TESTING} USB_PID=${USB_PID_TESTING}.
	#endif
	#endif

boards: enable tinyUSB for boards that use the highlevel_stdio feature with a single USB interface #18998

Are you sure you want to change the base?

boards: enable tinyUSB for boards that use the highlevel_stdio feature with a single USB interface #18998

Conversation

gschorcht commented Nov 30, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

dylad Nov 30, 2022

Choose a reason for hiding this comment

gschorcht Dec 1, 2022

Choose a reason for hiding this comment

gschorcht Dec 1, 2022

Choose a reason for hiding this comment

riot-ci commented Dec 1, 2022 • edited Loading

Murdock results

Artifacts

gschorcht commented Dec 9, 2022

MrKevinWeiss commented Dec 12, 2022

leandrolanzieri Dec 12, 2022

Choose a reason for hiding this comment

gschorcht Dec 12, 2022

Choose a reason for hiding this comment

MrKevinWeiss Dec 15, 2022

Choose a reason for hiding this comment

leandrolanzieri Dec 12, 2022

Choose a reason for hiding this comment

gschorcht commented Dec 12, 2022

boards: enable tinyUSB for boards that use the `highlevel_stdio` feature with a single USB interface #18998

boards: enable tinyUSB for boards that use the `highlevel_stdio` feature with a single USB interface #18998

gschorcht commented Nov 30, 2022 •

edited

Loading

riot-ci commented Dec 1, 2022 •

edited

Loading