-
Notifications
You must be signed in to change notification settings - Fork 8.3k
usb: device_next: use slist to store completed transfer requests #99672
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.
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
usb: device_next: use slist to store completed transfer requests #99672
Conversation
USBD_MAX_UDC_MSG configures the number of events coming from the UDC driver that the stack can keep. This can be filled very quickly if there would be multiple bus events for some reason, or function handlers of those events are blocked for long time. As a consequence, subsequent events could be dropped, and completed transfers not handled. To avoid it, we can store completed transfer requests in a slist and check it on every event. Signed-off-by: Johann Fischer <[email protected]>
|
| /** Notification message recipient callback */ | ||
| usbd_msg_cb_t msg_cb; | ||
| /** slist to keep endpoint events */ | ||
| sys_slist_t ep_events; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it was probably considered, but if not, another option is to use sys_dlist_t. There is no additional cost in storage, and list elements can be removed efficiently from any point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The events are processed in FIFO manner, so the advantage of "list elements can be removed efficiently from any point" doesn't seem necessary.
| if (event->type == UDC_EVT_EP_REQUEST) { | ||
| /* It has already been handled and cannot be another event type. */ | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep the case UDC_EVT_EP_REQUEST under switch below, i.e.
case UDC_EVT_EP_REQUEST:
/* It has already been handled */
break;
Functionality wise this is equivalent to the if above, but it makes it clear that there is no omission in the switch.
| { | ||
| int err = 0; | ||
|
|
||
| /* Always check if there is a completed transfer request. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add note that this check is really a workaround for usbd_event_carrier() dropping messages on full queue, because otherwise the reason for unconditionally calling event_handler_ep_request() is not really obvious.



USBD_MAX_UDC_MSG configures the number of events coming from the UDC driver that the stack can keep. This can be filled very quickly if there would be multiple bus events for some reason, or function handlers of those events are blocked for long time. As a consequence, subsequent events could be dropped, and completed transfers not handled. To avoid it, we can store completed transfer requests in a slist and check it on every event.
Resolves: #74058