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.

Sign up for GitHub

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

Jump to bottom

sys/usbus: handle exceeding number of endpoints #19362

Merged
merged 1 commit into from
Mar 10, 2023
Merged

sys/usbus: handle exceeding number of endpoints #19362

merged 1 commit into from
Mar 10, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Mar 7, 2023 •
edited
Loading

Contribution description

This PR fixes issue #19359 for STM32 USB OTG cores partially:

  1. It must not be silently ignored if the number of endpoints is not sufficient for an application. Instead of producing a non-working application, the application stops now with kernel_panic if the number of EPs is exhausted. This fixes the problem described in issue sys/usbus: usbus_cdc_ecm does not work together with usbus_cdc_acm for STM32 #19359 for USB cores with CID version 1.x, e.g. for STM32F439ZI FS interface (CID 1200) since they only have 4 IN and 4 OUT endpoints including the control endpoint EP0.
  2. [Update: this part was fixed by PR usbdev: Add dedicated stall functions #17086] If a feature is not supported, the device has to signal a STALL on the endpoint that should be used for the data phase in a control transaction. This means that for control read transactions the IN endpoint must signal a STALL and for control write transactions the OUT endpoint must signal a STALL. In former implementation, only the IN endpoint signaled a STALL independent on whether it was a control read or control write transaction. The change also fixes the problem that the enumeration stopped for about 5 seconds if module usb_reset_board isn't used. The reason is that the host sends a SET LINE CODING request to the CDC ACM interface and the device must signal a STALL on the OUT endpoint if it is not supported.

Testing procedure

  1. Use a STM32 board with USB OTG version 1.x, for example a nucleo-f439zi: 
    USEMODULE='stdio_cdc_acm' BOARD=nucleo-f439zi make -j8 -C tests/usbus_cdc_ecm flash
    Without this PR, the application seems to run but the CDC ECM interface is not working. The ping command can't be executed. With this PR, the application stops with kernel_panic. Because stdio_cdc_acm is used which doesn't work in this case, the kernel_panic has to be observed in debugger. 
    USEMODULE='stdio_cdc_acm' BOARD=nucleo-f439zi make -j8 -C tests/usbus_cdc_ecm debug
  2. [Update: this part was fixed by PR usbdev: Add dedicated stall functions #17086] ~Use a STM32 board with USB OTG version 2.x and USB FS connector, for example a nucleo-f767zi: 
    USEMODULE='stdio_cdc_acm' BOARD=nucleo-f767zi make -j8 -C tests/usbus_cdc_ecm flash
    Without this PR a delay of 5 seconds in enumeration of the CDC ACM interface can be observed before the CDC ECM interface is enumerated. With this PR there is no delay anymore.~

Issues/PRs references

Fixes issue #19359 patially.

@gschorcht gschorcht changed the title sys/usbus: fix handling of limited number of EP and STALL for control writes sys/usbus: fix handling of limited number of EPs and STALL for control writes Mar 7, 2023
@github-actions github-actions bot added Area: sys Area: System Area: USB Area: Universal Serial Bus labels Mar 7, 2023
@gschorcht gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 7, 2023
@riot-ci
Copy link

riot-ci commented Mar 7, 2023 •
edited
Loading

Murdock results

✔️ PASSED

1fa988d sys/usbus: handle exceeding of number of endpoints

Success Failures Total Runtime
6882 0 6882 11m:07s

Artifacts

@gschorcht gschorcht force-pushed the sys/usbus/fix_ep_num_set_line_code branch 2 times, most recently from 204855e to b61513a Compare March 7, 2023 19:19
@benpicco
Copy link
Contributor

benpicco commented Mar 8, 2023

Please squash

@gschorcht
Copy link
Contributor Author

gschorcht commented Mar 9, 2023 •
edited
Loading

This PR #17086 will make this PR partially obsolete. Which one should be merged first?

@gschorcht
Copy link
Contributor Author

I am still not really convinced if an assert is really the best approach to handle the situation where the number of EPs is exceeded. If stdio_cdc_acm is used for the console, you can't not connect a terminal and have to connect a debugger to detect the kernel_panic. It's easy for example for STM32 boards with ST-LINK but can a problem for boards that share the USB OTG interface with USB Serial/JTAG bridge like ESP32-S3.

Ideally, the number of EPs needed should be known at compile time and a _static_assert should be used to detect misconfigurations at compile time. It actually makes no sense to allocate EPs dynamically.

@gschorcht
Copy link
Contributor Author

I am still not really convinced if an assert is really the best approach to handle the situation where the number of EPs is exceeded. If stdio_cdc_acm is used for the console

If, on the other hand, we do not use assert but just print an error message, the problem is that it takes a few seconds for a terminal program to connect to the CDC-ACM interface, so the error message would not be seen.

@gschorcht
Copy link
Contributor Author

gschorcht commented Mar 9, 2023 •
edited
Loading

What if we introduce for each interface macros that define the number of EPs required? For example USBUS_CDC_ACM_EP_IN_NUMOF, USBUS_CDC_ECM_EP_OUT_NUMOF, ... which are all zero by default but have a certain value if the according module is used. Each USB device driver would then have to define the number of EPs supported, e.g. USB_EP_IN_NUMOF and USB_EP_OUT_NUMOF. Then a simple check could be used at compile time.

@dylad
Copy link
Member

dylad commented Mar 9, 2023

What if we introduce for each interface macros that define the number of EPs required?

Sounds good to me.

@benpicco
Copy link
Contributor

benpicco commented Mar 9, 2023

Which one should be merged first?

If you can provide the missing functions for cpu/stm32/periph/usbdev_fs.c we can merge that first if you prefer. I think you can push directly to @bergzand's branch.

@bergzand
Copy link
Member

bergzand commented Mar 9, 2023

Heya, thanks all for pushing this. I really don't have the time at this point to provide the missing implementation. I'd really appreciate it if somebody could add the missing bits.

Thanks!

@gschorcht
Copy link
Contributor Author

PR #19371 provides compile time check for the static configuration in auto_init_usb.

@gschorcht gschorcht force-pushed the sys/usbus/fix_ep_num_set_line_code branch from b61513a to 30d409a Compare March 10, 2023 06:56
@gschorcht
sys/usbus: handle exceeding of number of endpoints
1fa988d 
If the number of endpoints is not sufficient for an application, it should not be silently ignored and cause a non-working application. Rather, should cause an assertion as it is a configuration issue.
@gschorcht gschorcht force-pushed the sys/usbus/fix_ep_num_set_line_code branch from 30d409a to 1fa988d Compare March 10, 2023 06:59
@gschorcht gschorcht changed the title sys/usbus: fix handling of limited number of EPs and STALL for control writes sys/usbus: handle exceeding number of endpoints Mar 10, 2023
@gschorcht
Copy link
Contributor Author

Now where PR #17086 is merged, this PR is reduced to the dynamic check of exceeding number of EPs.

@gschorcht gschorcht mentioned this pull request Mar 10, 2023
Merged
dylad
dylad approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

ACK.

@dylad
Copy link
Member

dylad commented Mar 10, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 10, 2023

🕐 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.

@bors
Copy link
Contributor

bors bot commented Mar 10, 2023

Build succeeded:

@bors bors bot merged commit 13d0895 into RIOT-OS:master Mar 10, 2023
@gschorcht gschorcht deleted the sys/usbus/fix_ep_num_set_line_code branch March 11, 2023 16:02
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@dylad dylad dylad approved these changes

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

Assignees
No one assigned
Labels
Area: sys Area: System Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.04
Development

Successfully merging this pull request may close these issues.
6 participants
@gschorcht @riot-ci @benpicco @dylad @bergzand @MrKevinWeiss