usbdev: Add dedicated stall functions #17086

bergzand · 2021-10-30T10:11:07Z

Contribution description

This PR adds dedicated stall functions for usbdev peripherals. Two
functions are added. The first function (usbdev_ep_stall) to enable and
disable the stall condition on generic endpoints. The second function is
a dedicated function to set the stall condition on endpoint zero in both
directions. This status can only be set and should automatically be
cleared by the usbdev implementation (or hardware) after a new setup
request is received from the host.

Testing procedure

examples/usbus_minimal should still enumerate correctly on the host side.
usbus/cdc_acm: Return stall on line coding not supported #17085 can be used to demonstrate the ep0_stall function with the tests/usbus_cdc_acm_stdio/ test

Issues/PRs references

None

bergzand · 2021-11-01T08:53:46Z

I have no idea how to solve the cppcheck issue, I can't reproduce it locally

kaspar030 · 2021-11-04T11:50:21Z

Code looks fine.
Can you provide some test output?

bergzand · 2021-11-04T12:20:14Z

Can you provide some test output?

Is it okay if I add test output for all platforms to #17090, the code for this PR doesn't have real usages yet except for what is in that PR.

kaspar030 · 2021-11-04T12:35:49Z

sure!

dylad

Just one comment and a question, otherwise I'm fine with the changes.

dylad · 2021-11-04T19:23:58Z

drivers/include/periph/usbdev.h

+     * @note The stall condition is automatically cleared after receiving a
+     * setup packet.
+     */


Do you mean by hardware or software ?
IIRC, the ST USB IP that I am working on, will rely on software to do this action.

Do you mind rewording this a bit ?
Something like
@note The stall condition should be cleared automatically either by hardware or by the usbdev implementation after receiving a setup packet.

sys/usb/usbus/usbus_control.c

bergzand · 2021-11-05T13:00:52Z

While the code here mostly works for the platforms, #17090 is a bit flawed. I suggest we wait with this PR until we've tested #17090 as that one really needs this PR to be sound and this PR needs that PR to test. (Should I merge them into one PR? :)

dylad · 2021-11-05T14:42:22Z

(Should I merge them into one PR? :)

To be honest, I would prefer one single PR for this one.
Unless the fix it needs become too big to handle in a single PR.

bergzand · 2021-11-05T14:43:07Z

To be honest, I would prefer one single PR for this one.

👍 Then lets stick with two PRs for this.

gschorcht · 2023-03-09T16:23:14Z

@bergzand I provided the changes for STM32 FS dev as bergzand#9.

cpu/stm32: Add stall functions to usbdev_fs

benpicco · 2023-03-09T18:48:22Z

please squash

benpicco · 2023-03-09T20:00:58Z

bors merge

17086: usbdev: Add dedicated stall functions r=benpicco a=bergzand ### Contribution description This PR adds dedicated stall functions for usbdev peripherals. Two functions are added. The first function (usbdev_ep_stall) to enable and disable the stall condition on generic endpoints. The second function is a dedicated function to set the stall condition on endpoint zero in both directions. This status can only be set and should automatically be cleared by the usbdev implementation (or hardware) after a new setup request is received from the host. ### Testing procedure - examples/usbus_minimal should still enumerate correctly on the host side. - #17085 can be used to demonstrate the ep0_stall function with the `tests/usbus_cdc_acm_stdio/` test ### Issues/PRs references None Co-authored-by: Koen Zandberg <koen@bergzand.net> Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

bors · 2023-03-10T00:10:51Z

Build failed:

Murdock

benpicco · 2023-03-10T00:12:24Z

bors merge

bors · 2023-03-10T02:22:13Z

Build succeeded:

19362: sys/usbus: handle exceeding number of endpoints r=dylad a=gschorcht ### 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 #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 #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 #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. Co-authored-by: Gunar Schorcht <gunar@schorcht.net>

bergzand · 2023-03-10T09:51:35Z

Thanks all!

dylad · 2023-03-11T21:47:16Z

commit 10578c7 broke USB on SAM0.

gschorcht · 2023-03-12T08:12:00Z

commit 10578c7 broke USB on SAM0.

I can confirm this but since I added only the STM32 FS device changes, I didn't test it for SAM0, sorry 😟

gschorcht · 2023-03-12T13:19:26Z

cpu/sam0_common/periph/usbdev.c

@@ -584,12 +584,23 @@ static inline void _enable_ep_stall_in(UsbDeviceEndpoint *ep_reg)

 static inline void _disable_ep_stall_out(UsbDeviceEndpoint *ep_reg)
 {
-    ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_STALLRQ0;
+    ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_STALLRQ0 |
+                              USB_DEVICE_EPSTATUSCLR_DTGLOUT;


This change and the same below cause the problem. When I change it back, it works again.

dylad · 2023-03-12T20:20:53Z

I didn't test it for SAM0, sorry 😟

No need to apologize, this can happen and we will fix it :)
I can confirm that reverting the DTGLOUT/DTGLIN bit seems to fix the issue.
If you want to create a PR for that you can go ahead. Otherwise I can create it myself tomorrow during the day.

gschorcht · 2023-03-13T08:35:30Z

If you want to create a PR for that you can go ahead. Otherwise I can create it myself tomorrow during the day.

Since @bergzand as the author of this driver has deeper knowledge about the hardware and added the setting of these bits, we should first ask him what was the reason for setting these bits.

bergzand · 2023-03-13T09:30:20Z

Since @bergzand as the author of this driver has deeper knowledge about the hardware and added the setting of these bits, we should first ask him what was the reason for setting these bits.

Well, according to the USB specification (section 5.7.5 and 5.8.5) recovery after a stall should set the data toggle bit to DATA0. That was my intention here, but apparently it's causing PID mismatches here. I'm fine with reverting that specific bit if it is causing issues.

gschorcht · 2023-03-13T10:56:17Z

If the EPSTATUS.DTGLIN/DTGLOUT bits are reset in _disable_ep_stall_in/out functions, it seems that the device already answers the first GET_DESCRIPTOR(DEVICE) with a wrong PID in IN transaction. SETUP DATA0 is answered with IN DATA0. This causes the host not to accept the data and the enumeration fails. The reason is probably that _usbdev_ep_xmit always calls _disable_ep_stall_in/out so that the next IN/OUT transaction will always use PID 0.

I don't know but I guess that the core handles the PID internally.

bergzand · 2023-03-13T11:45:17Z

The reason is probably that _usbdev_ep_xmit always calls _disable_ep_stall_in/out so that the next IN/OUT transaction will always use PID 0.

This might be the actual bug here in the implementation.

Reverting theses changes from RIOT-OS#17086 because it breaks SAM0 usbdev in master Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

dylad · 2023-03-13T12:55:27Z

#19384 was created to revert these bits for SAM0.
Let's fix usbdev on SAM0 for master first. We can still improve the rest later.

19384: sam0/usbdev: partial revert of #17086 r=gschorcht a=dylad ### Contribution description Reverting theses changes from #17086 because it breaks SAM0 `usbdev` in master. The idea was to following the USB spec but for not obvious reason yet, it doesn't work as intended. So revert these changes so `usbdev` works again on master. We will try to figured out what's wrong later. ### Testing procedure Test any `usbus` related test application w/ a SAM0-based board. ### Issues/PRs references See discussion in #17086 Co-authored-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

17090: usbus: Add support for endpoint halt condition r=gschorcht a=bergzand ### Contribution description This PR adds support for the endpoint halt condition on the USBUS side. Instead of directly stalling an endpoint, handlers should enable the halt condition on an usbus endpoint to signal error condition. This can then be cleared via a CLEAR FEATURE request from the host. This PR also extends support for the GET STATUS requests to support endpoints and interfaces as recipient. It also adds the SET and CLEAR FEATURE requests for the endpoints with support to set and clear the halt condition on an endpoint. ### Testing procedure The feature can be shown when adding the following patch to the CDC_ECM code: ```Patch diff --git a/sys/usb/usbus/cdc/ecm/cdc_ecm.c b/sys/usb/usbus/cdc/ecm/cdc_ecm.c index f580209..3d87a67087 100644 --- a/sys/usb/usbus/cdc/ecm/cdc_ecm.c +++ b/sys/usb/usbus/cdc/ecm/cdc_ecm.c `@@` -31,7 +31,7 `@@` #include <string.h> -#define ENABLE_DEBUG 0 +#define ENABLE_DEBUG 1 #include "debug.h" static void _event_handler(usbus_t *usbus, usbus_handler_t *handler, `@@` -335,7 +335,12 `@@` static void _transfer_handler(usbus_t *usbus, usbus_handler_t *handler, size_t len = 0; usbdev_ep_get(ep, USBOPT_EP_AVAILABLE, &len, sizeof(size_t)); _store_frame_chunk(cdcecm); - if (len == USBUS_CDCECM_EP_DATA_SIZE) { + DEBUG("Length: %u\n", cdcecm->len); + if (cdcecm->len > 1000) { + usbus_endpoint_halt(cdcecm->ep_out); + _handle_rx_flush(cdcecm); + } + else if (len == USBUS_CDCECM_EP_DATA_SIZE) { usbdev_ep_ready(ep, 0); } } diff --git a/sys/usb/usbus/usbus.c b/sys/usb/usbus/usbus.c index 826966c..1bed707e2d 100644 --- a/sys/usb/usbus/usbus.c +++ b/sys/usb/usbus/usbus.c `@@` -40,7 +40,7 `@@` #include <string.h> #include <errno.h> -#define ENABLE_DEBUG 0 +#define ENABLE_DEBUG 1 #include "debug.h" #define _USBUS_MSG_QUEUE_SIZE (16) ``` With this patch, the CDC ECM code will trigger the halt condition after receiving more than 1000 bytes for a single network packet. For example with a `ping 2001:db8::2 -s 1200` (assuming `2001:db8::2` is the RIOT device). On a nRF52840dk this looks like the following: <details><summary>nRF52840dk</summary> ``` 2021-10-31 21:24:26,271 # usbus: starting thread 3 2021-10-31 21:24:26,275 # usbus: Adding string descriptor number 1 for: "USB config" 2021-10-31 21:24:26,281 # usbus: Adding string descriptor number 2 for: "USB device" 2021-10-31 21:24:26,286 # usbus: Adding string descriptor number 3 for: "RIOT-os.org" 2021-10-31 21:24:26,292 # usbus: Adding string descriptor number 4 for: "AC1B17D1BE432280" 2021-10-31 21:24:26,294 # CDC ECM: initialization 2021-10-31 21:24:26,299 # usbus: Adding string descriptor number 5 for: "AAEC5F69468B" 2021-10-31 21:24:26,305 # NETOPT_RX_END_IRQ not implemented byusbus: USB suspend detected 2021-10-31 21:24:26,312 # main(): This is RIOT! (Version: 2022.01-devel-281-g7da001-pr/usbus/halt_endpoint) 2021-10-31 21:24:26,316 # Test application for the USBUS CDC ECM interface 2021-10-31 21:24:26,317 # 2021-10-31 21:24:26,321 # This test pulls in parts of the GNRC network stack, use the 2021-10-31 21:24:26,327 # provided shell commands (i.e. ifconfig, ping6) to interact with 2021-10-31 21:24:26,330 # the CDC ECM based network interface. 2021-10-31 21:24:26,330 # 2021-10-31 21:24:26,332 # Starting the shell now... > 2021-10-31 21:24:26,481 # CDC ECM: Reset 2021-10-31 21:24:26,545 # CDC ECM: Reset 2021-10-31 21:24:26,631 # CDC ECM: Request: 0xb 2021-10-31 21:24:26,635 # CDC ECM: Changing active interface to alt 1 2021-10-31 21:24:26,638 # CDC ECM: sending link up indication 2021-10-31 21:24:26,645 # CDC ECM: Request: 0x43 2021-10-31 21:24:26,649 # CDC ECM: Not modifying filter to 0xc 2021-10-31 21:24:29,889 # CDC ECM: Request: 0x43 2021-10-31 21:24:29,893 # CDC ECM: Not modifying filter to 0xc 2021-10-31 21:24:29,901 # CDC ECM: Request: 0x43 2021-10-31 21:24:29,904 # CDC ECM: Not modifying filter to 0xe 2021-10-31 21:24:29,907 # CDC ECM: sending link speed indication > ifconfig 4 add 2001:db8::2/64 2021-10-31 21:24:35,296 # ifconfig 4 add 2001:db8::2/64 2021-10-31 21:24:35,300 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:35,305 # success: added 2001:db8::2/64 to interface 4 2021-10-31 21:24:35,307 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:51,075 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:24:51,078 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:06,851 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:06,855 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:13,271 # CDC ECM: Request: 0x43 2021-10-31 21:25:13,274 # CDC ECM: Not modifying filter to 0xe 2021-10-31 21:25:13,275 # Length: 64 2021-10-31 21:25:13,276 # Length: 90 2021-10-31 21:25:13,521 # Length: 64 2021-10-31 21:25:13,522 # Length: 86 2021-10-31 21:25:13,569 # Length: 64 2021-10-31 21:25:13,570 # Length: 90 2021-10-31 21:25:14,927 # Length: 64 2021-10-31 21:25:14,927 # Length: 86 2021-10-31 21:25:14,931 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:14,935 # CDC_ECM: Handling TX xmit from netdev 2021-10-31 21:25:14,936 # Length: 64 2021-10-31 21:25:14,937 # Length: 128 2021-10-31 21:25:14,938 # Length: 192 2021-10-31 21:25:14,939 # Length: 256 2021-10-31 21:25:14,940 # Length: 320 2021-10-31 21:25:14,941 # Length: 384 2021-10-31 21:25:14,942 # Length: 448 2021-10-31 21:25:14,943 # Length: 512 2021-10-31 21:25:14,945 # Length: 576 2021-10-31 21:25:14,946 # Length: 640 2021-10-31 21:25:14,947 # Length: 704 2021-10-31 21:25:14,948 # Length: 768 2021-10-31 21:25:14,949 # Length: 832 2021-10-31 21:25:14,950 # Length: 896 2021-10-31 21:25:14,951 # Length: 960 2021-10-31 21:25:14,952 # Length: 1024 2021-10-31 21:25:14,954 # Endpoint 1 halted 2021-10-31 21:25:14,955 # Length: 64 2021-10-31 21:25:14,956 # Endpoint 1 unhalted 2021-10-31 21:25:15,961 # Length: 128 2021-10-31 21:25:15,962 # Length: 192 2021-10-31 21:25:15,963 # Length: 256 2021-10-31 21:25:15,964 # Length: 320 2021-10-31 21:25:15,966 # Length: 384 2021-10-31 21:25:15,967 # Length: 448 2021-10-31 21:25:15,968 # Length: 512 2021-10-31 21:25:15,969 # Length: 576 2021-10-31 21:25:15,970 # Length: 640 2021-10-31 21:25:15,971 # Length: 704 2021-10-31 21:25:15,972 # Length: 768 2021-10-31 21:25:15,973 # Length: 832 2021-10-31 21:25:15,974 # Length: 896 2021-10-31 21:25:15,975 # Length: 960 2021-10-31 21:25:15,977 # Length: 1024 2021-10-31 21:25:15,978 # Endpoint 1 halted 2021-10-31 21:25:15,979 # Length: 64 2021-10-31 21:25:15,981 # Endpoint 1 unhalted ``` </details> Of interest in that excerpt are the two "Endpoint 1 halted" and "Endpoint 1 unhalted". In Wireshark, dumping the USB traffic, this looks like: ![image](https://user-images.githubusercontent.com/5160052/139600179-df34d8a2-80b5-4485-a6f6-f3615090126f.png) With the CLEAR FEATURE request visible to clear the stall condition on the endpoint After the stall condition traffic to the device continues again normally without requiring a restart of the device or the USB connection. ### Issues/PRs references Needs #17086 Co-authored-by: Koen Zandberg <koen@bergzand.net>

Reverting theses changes from RIOT-OS#17086 because it breaks SAM0 usbdev in master Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

bergzand requested review from aabadie, benpicco, DipSwitch, dylad, fjmolinas, haukepetersen, keestux, MrKevinWeiss and vincent-d as code owners October 30, 2021 10:11

github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: sys Area: System Area: USB Area: Universal Serial Bus Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 30, 2021

bergzand force-pushed the pr/usbdev/stall_function branch from 7da0018 to 3ee63da Compare October 31, 2021 11:22

bergzand mentioned this pull request Oct 31, 2021

usbus: Add support for endpoint halt condition #17090

Merged

dylad reviewed Nov 4, 2021

View reviewed changes

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

dylad added the State: waiting for author State: Action by the author of the PR is required label Nov 28, 2021

dylad self-assigned this Nov 28, 2021

Merge pull request #9 from gschorcht/pr/usbdev/stall_function

559f39d

cpu/stm32: Add stall functions to usbdev_fs

bors bot merged commit 513676f into RIOT-OS:master Mar 10, 2023

gschorcht mentioned this pull request Mar 10, 2023

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

Merged

bergzand deleted the pr/usbdev/stall_function branch March 10, 2023 09:51

gschorcht mentioned this pull request Mar 11, 2023

sys/usbus: usbus_cdc_ecm does not work together with usbus_cdc_acm for STM32 #19359

Closed

gschorcht reviewed Mar 12, 2023

View reviewed changes

dylad added a commit to dylad/RIOT that referenced this pull request Mar 13, 2023

sam0/usbdev: partial revert of RIOT-OS#17086

b4b41bc

Reverting theses changes from RIOT-OS#17086 because it breaks SAM0 usbdev in master Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

dylad mentioned this pull request Mar 13, 2023

sam0/usbdev: partial revert of #17086 #19384

Merged

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

zhaolanhuang pushed a commit to zhaolanhuang/RIOT that referenced this pull request Dec 6, 2023

sam0/usbdev: partial revert of RIOT-OS#17086

134ae6a

Reverting theses changes from RIOT-OS#17086 because it breaks SAM0 usbdev in master Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usbdev: Add dedicated stall functions #17086

usbdev: Add dedicated stall functions #17086

usbdev: Add dedicated stall functions #17086

usbdev: Add dedicated stall functions #17086

Conversation

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment