sys/usbus: check for the number of required and provided EPs in static configurations #19371

gschorcht · 2023-03-09T14:31:42Z

Contribution description

This PR provides a static check at compile time whether the number of EPs required in a static configuration does not exceed the number of EPs provided by the USB device.

Background

In issue #19359 the problem was reported that usbus_cdc_ecm didn't work together with stdio_cdc_acm on some STM32 boards. The reason for some of the boards was simply that the application tried to allocate more EPs than available and simply ignored this and just didn't work.

Solution

Since auto_init_usb uses a static configuration with exactly one USBUS stack instance and one USB device, at least in case auto_init is used a static check can be carried out to make sure that the number of EPs required by the application doesn't exceed the number of EPs provided by the USB device. For this purpose, each usbus_* module defines the number of IN and OUT EPs required by that module. Each USB device driver defines the number of EPs provided by USB device if it differs from the default of 8 EPs. During the auto initialization the total number of required IN and OUT EPs is then compared with the number of EPs provided by the USB device using a static assert.

Testing procedure

Green CI

Compilation of

USEMODULE='stdio_cdc_acm' BOARD=nucleo-f439zi make -j8 -C tests/usbus_cdc_ecm

should lead to compilation error

sys/auto_init/usb/auto_init_usb.c:81:1: error: static assertion failed: "Number of required IN endpoints exceeded"
 _Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,
 ^~~~~~~~~~~~~~
Makefile.base:146: recipe for target 'tests/usbus_cdc_ecm/bin/nucleo-f439zi/auto_init_usbus/auto_init_usb.o' failed

while compilation of

USEMODULE='stdio_cdc_acm' BOARD=nucleo-f767zi make -j8 -C tests/usbus_cdc_ecm

should work.

Issues/PRs references

Fixes issue #19359 partially.

riot-ci · 2023-03-09T14:39:58Z

Murdock results

✔️ PASSED

cc2fedb tests/usbus_cdc_ecm: add blacklist info

Success	Failures	Total	Runtime
6882	0	6882	12m:52s

Artifacts

Documentation preview

dylad

one nitpick.
Please squash directly.

Otherwise works as advertised !

dylad · 2023-03-09T20:19:04Z

sys/auto_init/usb/auto_init_usb.c

+                                     USBUS_HID_EP_OUT_REQUIRED_NUMOF + \
+                                     USBUS_MSC_EP_OUT_REQUIRED_NUMOF)
+
+_Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,


Suggested change

_Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,

assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,

For consistency across the file.

It is intentionally a _Static_assert to check it already during compile time. The problem with assert is that it is checked at runtime, which can't be observed if USBUS isn't working and stdio_cdc_acm is used. If assert were applicable, it would be sufficient to check if usbdev_new_ep returns something other than NULL (PR #19362 does the check for dynamic configurations during runtime by assert).

Or did you mean that C11 macro static_assert should be used instead of _Static_assert?

Or did you mean that C11 macro static_assert should be used instead of _Static_assert?

Yes that's what I was thinking.
Thanks !

dylad · 2023-03-09T20:19:18Z

sys/auto_init/usb/auto_init_usb.c

+_Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,
+               "Number of required IN endpoints exceeded");
+
+_Static_assert(USBUS_EP_OUT_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,


gschorcht · 2023-03-10T07:25:47Z

I had to rebase to resolve conflicts after PR #17086 was merged. I also changed _Static_assert to static_assert.

dylad

ACK.
Works as advertised.

dylad · 2023-03-10T08:12:36Z

bors merge

bors · 2023-03-10T08:12:39Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

dylad · 2023-03-10T10:41:10Z

bors cancel
bors merge

19371: sys/usbus: check for the number of required and provided EPs in static configurations r=dylad a=gschorcht ### Contribution description This PR provides a static check at compile time whether the number of EPs required in a static configuration does not exceed the number of EPs provided by the USB device. #### Background In issue #19359 the problem was reported that `usbus_cdc_ecm` didn't work together with `stdio_cdc_acm` on some STM32 boards. The reason for some of the boards was simply that the application tried to allocate more EPs than available and simply ignored this and just didn't work. #### Solution Since `auto_init_usb` uses a static configuration with exactly one USBUS stack instance and one USB device, at least in case `auto_init` is used a static check can be carried out to make sure that the number of EPs required by the application doesn't exceed the number of EPs provided by the USB device. For this purpose, each `usbus_*` module defines the number of IN and OUT EPs required by that module. Each USB device driver defines the number of EPs provided by USB device if it differs from the default of 8 EPs. During the auto initialization the total number of required IN and OUT EPs is then compared with the number of EPs provided by the USB device using a static assert. ### Testing procedure 1. Green CI 2. Compilation of ```python USEMODULE='stdio_cdc_acm' BOARD=nucleo-f439zi make -j8 -C tests/usbus_cdc_ecm ``` should lead to compilation error ```python sys/auto_init/usb/auto_init_usb.c:81:1: error: static assertion failed: "Number of required IN endpoints exceeded" _Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS, ^~~~~~~~~~~~~~ Makefile.base:146: recipe for target 'tests/usbus_cdc_ecm/bin/nucleo-f439zi/auto_init_usbus/auto_init_usb.o' failed ``` while compilation of ``` USEMODULE='stdio_cdc_acm' BOARD=nucleo-f767zi make -j8 -C tests/usbus_cdc_ecm ``` should work. ### Issues/PRs references Fixes issue #19359 partially. Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

benpicco · 2023-03-10T11:58:09Z

bors merge

bors · 2023-03-10T11:58:11Z

Already running a review

benpicco · 2023-03-10T12:06:50Z

bors cancel
bors merge

gschorcht · 2023-03-12T12:17:19Z

Rebased onto master after PR #19380 has been merged.

gschorcht · 2023-03-12T12:35:33Z

@dylad Which do you think is better, to blacklist the boards with insufficient number of EPs in tests/usbus_cdc_ecm or to use stdio_uart in tests/usbus_cdc_ecm instead?

Blacklist boards with insufficient number of EPs
- Pro: problems similar to Issue sys/usbus: usbus_cdc_ecm does not work together with usbus_cdc_acm for STM32 #19359 can also be detected for new boards.
- Con: tests/usbus_cdc_ecm can't be used out of the box for blacklisted boards.
Using stdio_uart
- Pro: tests/usbus_cdc_ecm can be used out of the to test CDC ECM for these boards.
- Con: a external UART2USB adapter is required

On the other, the user has always the option to use for example

USEMODULE=stdio_uart BOARD=weact-f401cc make -C tests/usbus-cdc-ecm

if really necessary. Therefore, I personally prefer blacklisting.

dylad · 2023-03-12T20:25:08Z

I also tend to prefer blacklisting.
Maybe there is a way to blacklist a board when a specific module is used ?

maribu · 2023-03-13T11:36:18Z

sys/auto_init/usb/auto_init_usb.c

+
+

Suggested change

A last minute style nit-pick :)

Good catch. I felt free to squash this small fix directly.

Since `auto_init_usb` provides a static auto configuration for a single USBUS stack instance using a single USB device, a static assert can be used here to check whether the number of EPs required by the configuration does not exceed the number of EPs provided by the USB device.

gschorcht · 2023-03-13T12:16:11Z

I also tend to prefer blacklisting.
Maybe there is a way to blacklist a board when a specific module is used ?

Since blacklisting a board depending on used modules doesn't work, I suggest to solve it as following (12a3d3f).

If no alternative stdio_* module or module stdio_cdc_acm is enabled in the make command, the a board is blacklisted and user gets a short text why and how to use the application:

BOARD=stm32f4discovery make -C tests/usbus_cdc_ecm

or

USEMODULE=stdio_cdc_acm BOARD=stm32f4discovery make -C tests/usbus_cdc_ecm

give

make: Verzeichnis „/home/gs/src/RIOT-Xtensa-ESP.esp-idf-4.4/tests/usbus_cdc_ecm“ wird betreten
Warning:
    The board is blacklisted because it uses `stdio_cdc_acm` as STDIO, which needs
    a CDC ACM interface in addition to the CDC ECM interface, but the number of
    available endpoints is not sufficient for this. To use this application you
    have to use `stdio_uart` or any other `stdio*` module, for example:

    USEMODULE=stdio_uart BOARD=stm32f4discovery make -C tests/usbus_cdc_ecm

The selected BOARD=stm32f4discovery is blacklisted: stm32f4discovery weact-f401cc weact-f401ce weact-f411ce

Using

USEMODULE=stdio_uart BOARD=stm32f4discovery make -C tests/usbus_cdc_ecm

works.

tests/usbus_cdc_ecm/Makefile

benpicco · 2023-03-14T19:36:04Z

bors merge

kaspar030 · 2023-03-14T20:29:04Z

bors merge

19371: sys/usbus: check for the number of required and provided EPs in static configurations r=kaspar030 a=gschorcht ### Contribution description This PR provides a static check at compile time whether the number of EPs required in a static configuration does not exceed the number of EPs provided by the USB device. #### Background In issue #19359 the problem was reported that `usbus_cdc_ecm` didn't work together with `stdio_cdc_acm` on some STM32 boards. The reason for some of the boards was simply that the application tried to allocate more EPs than available and simply ignored this and just didn't work. #### Solution Since `auto_init_usb` uses a static configuration with exactly one USBUS stack instance and one USB device, at least in case `auto_init` is used a static check can be carried out to make sure that the number of EPs required by the application doesn't exceed the number of EPs provided by the USB device. For this purpose, each `usbus_*` module defines the number of IN and OUT EPs required by that module. Each USB device driver defines the number of EPs provided by USB device if it differs from the default of 8 EPs. During the auto initialization the total number of required IN and OUT EPs is then compared with the number of EPs provided by the USB device using a static assert. ### Testing procedure 1. Green CI 2. Compilation of ```python USEMODULE='stdio_cdc_acm' BOARD=nucleo-f439zi make -j8 -C tests/usbus_cdc_ecm ``` should lead to compilation error ```python sys/auto_init/usb/auto_init_usb.c:81:1: error: static assertion failed: "Number of required IN endpoints exceeded" _Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS, ^~~~~~~~~~~~~~ Makefile.base:146: recipe for target 'tests/usbus_cdc_ecm/bin/nucleo-f439zi/auto_init_usbus/auto_init_usb.o' failed ``` while compilation of ``` USEMODULE='stdio_cdc_acm' BOARD=nucleo-f767zi make -j8 -C tests/usbus_cdc_ecm ``` should work. ### Issues/PRs references Fixes issue #19359 partially. 19382: tests/pkg_nanors: use static allocation r=kaspar030 a=benpicco 19388: drivers/usbdev_synopsys_dwc2: disable DMA mode r=kaspar030 a=gschorcht ### Contribution description This PR disables the DMA mode for HS cores due to several problems found: - The STALL bit of the OUT control endpoint does not seem to be cleared automatically on the next SETUP received. At least the USB OTG HS core does not generate an interrupt on the next SETUP received. This happens, for example, when CDC ACM is used and the host sends the `SET_LINE_CODING` request which is answered with setting the STALL bit of the OUT endpoint. In this case the enumeration of further interfaces, for example CDC ECM, is stopped. This problem was described in #17085 (comment) - The Enumeration fails for CDC ECM interface which uses URB support (PR #17091) ### Testing procedure Use a STM32 board with USH OTG HS interface: ```python USEMODULE='stdio_cdc_acm periph_usbdev_hs_utmi' BOARD=stm32f723e-disco make -j8 -C tests/usbus_cdc_ecm flash USEMODULE='stdio_cdc_acm periph_usbdev_hs_ulpi' BOARD=stm32f746g-disco make -j8 -C tests/usbus_cdc_ecm flash ``` Without this PR, either the enumeration completely fails (mostly for `stm32f723e-disco`) ```python [377629.753895] usb 1-2.3: new high-speed USB device number 76 using xhci_hcd [377629.854349] usb 1-2.3: device descriptor read/all, error -71 [377629.937990] usb 1-2.3: new high-speed USB device number 77 using xhci_hcd [377630.038261] usb 1-2.3: device descriptor read/all, error -71 [377630.038711] usb 1-2-port3: attempt power cycle [377630.641970] usb 1-2.3: new high-speed USB device number 78 using xhci_hcd [377630.666066] usb 1-2.3: device descriptor read/8, error -71 [377630.794076] usb 1-2.3: device descriptor read/8, error -71 [377630.981806] usb 1-2.3: new high-speed USB device number 79 using xhci_hcd [377631.002092] usb 1-2.3: device descriptor read/8, error -71 [377631.130091] usb 1-2.3: device descriptor read/8, error -71 [377631.238344] usb 1-2-port3: unable to enumerate USB device ``` or the enumeration of the CDC ECM interface stops with error. ```python [377972.828168] usb 1-2.3: new high-speed USB device number 100 using xhci_hcd [377972.928762] usb 1-2.3: config 1 interface 0 altsetting 0 endpoint 0x81 has an invalid bInterval 255, changing to 11 [377972.928765] usb 1-2.3: config 1 interface 1 altsetting 0 bulk endpoint 0x1 has invalid maxpacket 64 [377972.928767] usb 1-2.3: config 1 interface 1 altsetting 0 bulk endpoint 0x82 has invalid maxpacket 64 [377972.929225] usb 1-2.3: New USB device found, idVendor=1209, idProduct=7d00, bcdDevice= 1.00 [377972.929228] usb 1-2.3: New USB device strings: Mfr=3, Product=2, SerialNumber=4 [377972.929230] usb 1-2.3: Product: stm32f723e-disco [377972.929232] usb 1-2.3: Manufacturer: RIOT-os.org [377972.929233] usb 1-2.3: SerialNumber: A6BAC4E1B1E0806B [377972.932399] cdc_acm 1-2.3:1.0: ttyACM1: USB ACM device [377972.933905] cdc_ether: probe of 1-2.3:1.2 failed with error -32 [377973.184377] usb 1-4.3.4: reset high-speed USB device number 32 using xhci_hcd ``` With this PR the enumeration should work as it should: ```python [378480.097974] usb 1-4.3.4: reset high-speed USB device number 32 using xhci_hcd [378484.289762] usb 1-2.3: new high-speed USB device number 16 using xhci_hcd [378484.394638] usb 1-2.3: config 1 interface 0 altsetting 0 endpoint 0x81 has an invalid bInterval 255, changing to 11 [378484.394642] usb 1-2.3: config 1 interface 1 altsetting 0 bulk endpoint 0x1 has invalid maxpacket 64 [378484.394644] usb 1-2.3: config 1 interface 1 altsetting 0 bulk endpoint 0x82 has invalid maxpacket 64 [378484.395296] usb 1-2.3: New USB device found, idVendor=1209, idProduct=7d00, bcdDevice= 1.00 [378484.395299] usb 1-2.3: New USB device strings: Mfr=3, Product=2, SerialNumber=4 [378484.395301] usb 1-2.3: Product: stm32f723e-disco [378484.395303] usb 1-2.3: Manufacturer: RIOT-os.org [378484.395304] usb 1-2.3: SerialNumber: A6BAC4E1B1E0806B [378484.398547] cdc_acm 1-2.3:1.0: ttyACM1: USB ACM device [378484.401007] cdc_ether 1-2.3:1.2 usb0: register 'cdc_ether' at usb-0000:00:14.0-2.3, CDC Ethernet Device, e6:75:97:3a:74:ba [378484.449870] cdc_ether 1-2.3:1.2 enp0s20f0u2u3i2: renamed from usb0 ``` ### Issues/PRs references Co-authored-by: Gunar Schorcht <gunar@schorcht.net> Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>

gschorcht · 2023-03-15T05:47:43Z

Thanks for reviewing and testing 😄

gschorcht · 2023-03-15T06:05:48Z

bors merge

Oh shit, I forgot one board on the blacklist. I squashed it directly. Can we merge it?

dylad · 2023-03-15T07:42:57Z

bors merge

bors · 2023-03-15T10:48:34Z

Build succeeded:

gschorcht requested review from aabadie, fjmolinas, DipSwitch, vincent-d and basilfx as code owners March 9, 2023 14:31

github-actions bot added Area: cpu Area: CPU/MCU ports Area: sys Area: System Area: USB Area: Universal Serial Bus Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Mar 9, 2023

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 Mar 9, 2023

gschorcht mentioned this pull request Mar 9, 2023

sys/usbus: handle exceeding number of endpoints #19362

Merged

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch from 0c3212c to 2bc51f3 Compare March 9, 2023 14:44

benpicco requested review from dylad and bergzand March 9, 2023 15:16

dylad reviewed Mar 9, 2023

View reviewed changes

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch 2 times, most recently from fae1b39 to 83f37aa Compare March 10, 2023 07:20

dylad approved these changes Mar 10, 2023

View reviewed changes

gschorcht added 2 commits March 12, 2023 13:15

cpu/gd32v: add number of EPs provided by USB OTG peripheral

170ec09

cpu/stm32: move number of EP definition to periph_cpu.h

760b80f

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch from 3396aee to decd052 Compare March 12, 2023 12:16

gschorcht removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 12, 2023

maribu reviewed Mar 13, 2023

View reviewed changes

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch 2 times, most recently from 8615cd8 to 12a3d3f Compare March 13, 2023 12:10

benpicco reviewed Mar 13, 2023

View reviewed changes

tests/usbus_cdc_ecm/Makefile Outdated Show resolved Hide resolved

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch from 12a3d3f to 82a520e Compare March 14, 2023 10:22

gschorcht mentioned this pull request Mar 14, 2023

drivers/usbdev_synopsys_dwc2: add GD32V support #19389

Merged

gschorcht added 2 commits March 15, 2023 06:58

tests/usbus_cdc_ecm: blacklist boards with insufficient number of EPs

b4d39db

tests/usbus_cdc_ecm: add blacklist info

cc2fedb

gschorcht force-pushed the sys/usbus/static_ep_numof_check branch from 82a520e to cc2fedb Compare March 15, 2023 05:59

bors bot merged commit fd38db6 into RIOT-OS:master Mar 15, 2023
23 checks passed

gschorcht deleted the sys/usbus/static_ep_numof_check branch April 12, 2023 14:11

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/usbus: check for the number of required and provided EPs in static configurations #19371

sys/usbus: check for the number of required and provided EPs in static configurations #19371

gschorcht commented Mar 9, 2023 •

edited

riot-ci commented Mar 9, 2023 •

edited

dylad left a comment

dylad Mar 9, 2023

gschorcht Mar 10, 2023

gschorcht Mar 10, 2023

dylad Mar 10, 2023

dylad Mar 9, 2023

gschorcht commented Mar 10, 2023

dylad left a comment

dylad commented Mar 10, 2023

bors bot commented Mar 10, 2023

dylad commented Mar 10, 2023

benpicco commented Mar 10, 2023

bors bot commented Mar 10, 2023

benpicco commented Mar 10, 2023

gschorcht commented Mar 12, 2023

gschorcht commented Mar 12, 2023

dylad commented Mar 12, 2023

maribu Mar 13, 2023

gschorcht Mar 13, 2023

gschorcht commented Mar 13, 2023 •

edited

benpicco commented Mar 14, 2023

kaspar030 commented Mar 14, 2023

gschorcht commented Mar 15, 2023

gschorcht commented Mar 15, 2023

dylad commented Mar 15, 2023

bors bot commented Mar 15, 2023

	_Static_assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,
	assert(USBUS_EP_IN_REQUIRED_NUMOF <= USBDEV_NUM_ENDPOINTS,

sys/usbus: check for the number of required and provided EPs in static configurations #19371

sys/usbus: check for the number of required and provided EPs in static configurations #19371

Conversation

gschorcht commented Mar 9, 2023 • edited

Contribution description

Background

Solution

Testing procedure

Issues/PRs references

riot-ci commented Mar 9, 2023 • edited

Murdock results

Artifacts

dylad left a comment

Choose a reason for hiding this comment

dylad Mar 9, 2023

Choose a reason for hiding this comment

gschorcht Mar 10, 2023

Choose a reason for hiding this comment

gschorcht Mar 10, 2023

Choose a reason for hiding this comment

dylad Mar 10, 2023

Choose a reason for hiding this comment

dylad Mar 9, 2023

Choose a reason for hiding this comment

gschorcht commented Mar 10, 2023

dylad left a comment

Choose a reason for hiding this comment

dylad commented Mar 10, 2023

bors bot commented Mar 10, 2023

dylad commented Mar 10, 2023

benpicco commented Mar 10, 2023

bors bot commented Mar 10, 2023

benpicco commented Mar 10, 2023

gschorcht commented Mar 12, 2023

gschorcht commented Mar 12, 2023

dylad commented Mar 12, 2023

maribu Mar 13, 2023

Choose a reason for hiding this comment

gschorcht Mar 13, 2023

Choose a reason for hiding this comment

gschorcht commented Mar 13, 2023 • edited

benpicco commented Mar 14, 2023

kaspar030 commented Mar 14, 2023

gschorcht commented Mar 15, 2023

gschorcht commented Mar 15, 2023

dylad commented Mar 15, 2023

bors bot commented Mar 15, 2023

gschorcht commented Mar 9, 2023 •

edited

riot-ci commented Mar 9, 2023 •

edited

gschorcht commented Mar 13, 2023 •

edited