Skip to content

Commit

Permalink
Merge #17090
Browse files Browse the repository at this point in the history
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 f580209e03..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 826966c929..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>
  • Loading branch information
bors[bot] and bergzand authored Apr 11, 2023
2 parents bbc5fc9 + 7b2db7b commit 4b9ed26
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 3 deletions.
9 changes: 9 additions & 0 deletions sys/include/usb/descriptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ extern "C" {
#define USB_TYPE_DESCRIPTOR_INTERFACE_ASSOC 0x0b /**< Interface association */
/** @} */

/**
* @name USB standard feature selectors
* @{
*/
#define USB_FEATURE_ENDPOINT_HALT 0x00 /**< Endpoint halt */
#define USB_FEATURE_DEVICE_REMOTE_WAKEUP 0x01 /**< Device remote wakeup */
#define USB_FEATURE_TEST_MODE 0x02 /**< Test mode feature */
/** @} */

/**
* @name USB configuration attributes
* @anchor USB_CONF_ATTR
Expand Down
17 changes: 17 additions & 0 deletions sys/include/usb/usbus.h
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ typedef struct usbus_endpoint {
uint8_t interval; /**< Poll interval for interrupt endpoints */
bool active; /**< If the endpoint should be activated after
reset */
bool halted; /**< Endpoint is halted */
} usbus_endpoint_t;

/**
Expand Down Expand Up @@ -676,6 +677,22 @@ void usbus_urb_submit(usbus_t *usbus, usbus_endpoint_t *endpoint, usbus_urb_t *u
*/
int usbus_urb_cancel(usbus_t *usbus, usbus_endpoint_t *endpoint, usbus_urb_t *urb);

/**
* @brief Set the halt condition on an endpoint.
*
* The endpoint will respond with stall to all packets and must explicitly be
* cleared by the host by clearing the halt condition or switching interfaces
*/
void usbus_endpoint_halt(usbus_endpoint_t *ep);

/**
* @brief Clear the halt condition on an endpoint
*
* @note Must only be used when the endpoint is halted and when the host issues
* a SetInterface request on the interface containing the endpoint
*/
void usbus_endpoint_clear_halt(usbus_endpoint_t *ep);

/**
* @brief Enable an endpoint
*
Expand Down
29 changes: 29 additions & 0 deletions sys/usb/usbus/usbus.c
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,34 @@ static void *_usbus_thread(void *args)
return NULL;
}

void usbus_endpoint_halt(usbus_endpoint_t *ep)
{
assert(ep->ep->num != 0); /* Not valid for endpoint 0 */
DEBUG("Endpoint %u halted\n", ep->ep->num);
ep->halted = 1;
usbdev_ep_stall(ep->ep, true);
}

void usbus_endpoint_clear_halt(usbus_endpoint_t *ep)
{
assert(ep->ep->num != 0); /* Not valid for endpoint 0 */
DEBUG("Endpoint %u unhalted\n", ep->ep->num);
ep->halted = 0;
usbdev_ep_stall(ep->ep, false);
}

/**
* @brief Reset the halted status on USB reset condition
*/
static void _usbus_endpoint_reset_halt(usbus_t *usbus)
{
/* Clear halted state. No need to notify usbdev, USB reset already resets those */
for (size_t i = 0; i < USBDEV_NUM_ENDPOINTS; i++) {
usbus->ep_out[i].halted = 0;
usbus->ep_in[i].halted = 0;
}
}

/* USB event callback */
static void _event_cb(usbdev_t *usbdev, usbdev_event_t event)
{
Expand All @@ -487,6 +515,7 @@ static void _event_cb(usbdev_t *usbdev, usbdev_event_t event)
usbus->addr = 0;
usbdev_set(usbus->dev, USBOPT_ADDRESS, &usbus->addr,
sizeof(uint8_t));
_usbus_endpoint_reset_halt(usbus);
flag = USBUS_HANDLER_FLAG_RESET;
msg = USBUS_EVENT_USB_RESET;
DEBUG("usbus: USB reset detected\n");
Expand Down
60 changes: 57 additions & 3 deletions sys/usb/usbus/usbus_control.c
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,39 @@ static usbus_string_t *_get_descriptor(usbus_t *usbus, uint16_t idx)

static int _req_status(usbus_t *usbus)
{
uint8_t status[2];
/* Signal self powered */
uint16_t status = (CONFIG_USB_SELF_POWERED) ? 1 : 0;
usbus_control_slicer_put_bytes(usbus, (uint8_t*)&status, sizeof(status));
return sizeof(status);
}

static int _req_iface_status(usbus_t *usbus)
{
uint16_t status = 0; /* always zero */
usbus_control_slicer_put_bytes(usbus, (uint8_t*)&status, sizeof(status));
return sizeof(status);
}

memset(status, 0, sizeof(status));
usbus_control_slicer_put_bytes(usbus, status, sizeof(status));
static int _req_endpoint_status(usbus_t *usbus, usbus_endpoint_t *ep)
{
uint16_t status = ep->halted ? 1 : 0;
usbus_control_slicer_put_bytes(usbus, (uint8_t*)&status, sizeof(status));
return sizeof(status);
}

static int _req_endpoint_feature(usbus_endpoint_t *ep, uint16_t feature, bool enable)
{
switch (feature) {
case USB_FEATURE_ENDPOINT_HALT:
enable ? usbus_endpoint_halt(ep) : usbus_endpoint_clear_halt(ep);
break;
default:
DEBUG("usbus: unknown endpoint feature request: %u\n", feature);
return -1;
}
return 1;
}

static int _req_str(usbus_t *usbus, uint16_t idx)
{
/* Return an error condition by default */
Expand Down Expand Up @@ -238,6 +264,11 @@ static int _recv_interface_setup(usbus_t *usbus, usb_setup_t *pkt)
(usbus_control_handler_t *)usbus->control;
uint16_t destination = pkt->index & 0x0f;

/* Globally handle the iface get status request */
if (pkt->request == USB_SETUP_REQ_GET_STATUS) {
return _req_iface_status(usbus);
}

/* Find interface handler */
for (usbus_interface_t *iface = usbus->iface; iface; iface = iface->next) {
if (destination == iface->idx &&
Expand All @@ -251,6 +282,26 @@ static int _recv_interface_setup(usbus_t *usbus, usb_setup_t *pkt)
return -1;
}

static int _recv_endpoint_setup(usbus_t *usbus, usb_setup_t *pkt)
{
uint8_t destination = pkt->index & 0x0f;
bool in = pkt->index & (1 << 7); /* Bit seven is 1 for IN, 0 for OUT */
usbus_endpoint_t *ep = in ? &usbus->ep_in[destination] :
&usbus->ep_out[destination];

switch (pkt->request) {
case USB_SETUP_REQ_GET_STATUS:
return _req_endpoint_status(usbus, ep);
case USB_SETUP_REQ_SET_FEATURE:
return _req_endpoint_feature(ep, pkt->value, true);
case USB_SETUP_REQ_CLEAR_FEATURE:
return _req_endpoint_feature(ep, pkt->value, false);
default:
DEBUG("usbus: Unknown endpoint request %u\n", pkt->request);
return -1;
}
}

static void _recv_setup(usbus_t *usbus, usbus_control_handler_t *handler)
{
usb_setup_t *pkt = &handler->setup;
Expand All @@ -272,6 +323,9 @@ static void _recv_setup(usbus_t *usbus, usbus_control_handler_t *handler)
case USB_SETUP_REQUEST_RECIPIENT_INTERFACE:
res = _recv_interface_setup(usbus, pkt);
break;
case USB_SETUP_REQUEST_RECIPIENT_ENDPOINT:
res = _recv_endpoint_setup(usbus, pkt);
break;
default:
DEBUG("usbus_control: Unhandled setup request\n");
}
Expand Down

0 comments on commit 4b9ed26

Please sign in to comment.