usb: device_next: uvc: Initial ISO EP Descriptors#104619
usb: device_next: uvc: Initial ISO EP Descriptors#104619zacck wants to merge 2 commits intozephyrproject-rtos:mainfrom
Conversation
zacck
force-pushed
the
pr_uvc_iso_ep
branch
2 times, most recently
from
72e50fe to
f237315
Compare
Additional UVC descriptors for catering to the Isochronous Video Streaming Endpoint Signed-off-by: Zacck Osiemo <coderv63@gmail.com>
To save bandwidth the UVC class has an alternate setting (0) with no endpoints, switching to which frees all the allocated bandwidth on the bus, This enables it to switch into and out of this alternate setting Signed-off-by: Zacck Osiemo <coderv63@gmail.com>
|
Ok I think we can start actually considering this code now. |
|
There was a problem hiding this comment.
Thanks for this first round!
Given the amount of notifications the USB area has (i.e. as opposed to Video), I am converting it as a draft until it works, I hope that's ok.
Feel free to ping me individually on i.e. Discord.
Edit: we also need to update the way buffers are chopped down and sent to the USB stack.
There is a header on every ISO frame whereas BULK on the first packet of the transfer only
| const uint8_t alternate) | ||
| { | ||
| LOG_DBG("Select alternate %u for interface %u", alternate, iface); | ||
| struct uvc_data *data = usbd_class_get_private(c_data); |
There was a problem hiding this comment.
For USB in particular, you would need to add a const after pointers, to guarantee that data will not be assigned to another pointer in the body, just to get a compiler warning if this happens.
| struct uvc_data *data = usbd_class_get_private(c_data); | |
| struct uvc_data *const data = usbd_class_get_private(c_data); |
There was a problem hiding this comment.
You might also want to keep all variables declaration at the top of the block with an empty line after them.
I do not see that in https://kernel.org/doc/html/latest/process/coding-style.html but there is a checkpatch.pl check IIRC, and otherwise recommended during reviews on USB I receive.
There was a problem hiding this comment.
Maybe possible to reduce even more by not re-defining the endpoint, but having macros for the content of the fields that change, i.e. wMaxPacketSize = UVC_EP_MPS, bmAttributes = UVC_EP_TYPE.
| config USBD_VIDEO_ISO | ||
| bool "USB Video Class ISO EP Implementation" | ||
| help | ||
| This allows the USB Video class to use an ISO Endpoint Implementation |
There was a problem hiding this comment.
How about a choice instead of boolean?
Also, making isochronous the default is not bad idea.
| LOG_DBG("Instance %p, interface %u alternate %u changed", | ||
| c_data, iface, alternate); |
| LOG_DBG("Select alternate %u for interface %u", alternate, iface); | ||
| struct uvc_data *data = usbd_class_get_private(c_data); | ||
|
|
||
| data->alternate = alternate; |
There was a problem hiding this comment.
This is just updated but not used?
Likely this is all what can be done for now, as there is no API for sending events to the application.
This needs either a custom API or just dropping that variable for now as not in use.
| } | ||
|
|
||
| #if CONFIG_USBD_VIDEO_ISO | ||
| #define USBD_VIDEO_ENDPOINT_DESC \ |
There was a problem hiding this comment.
This is not just endpoints there?
It could be helping if you de-duplicate everything that could be: if1, if1_hdr...
I think you would only need to customize the endpoint descriptor and add add a define either empty either AltSettings 1?
| struct usb_desc_header *uvc_fs_desc_##n[UVC_MAX_FS_DESC] = { \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.iad, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_hdr, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_ct, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_su, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_pu, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_xu, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_ot, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1_hdr, \ | ||
| /* More pointers are generated here at runtime */ \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1_ep_fs, \ | ||
| (struct usb_desc_header *) NULL, \ | ||
| }; \ | ||
| \ | ||
| struct usb_desc_header *uvc_hs_desc_##n[UVC_MAX_HS_DESC] = { \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.iad, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_hdr, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_ct, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_su, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_pu, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_xu, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if0_ot, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1, \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1_hdr, \ | ||
| /* More pointers are generated here at runtime */ \ | ||
| (struct usb_desc_header *) &uvc_desc_##n.if1_ep_hs, \ | ||
| (struct usb_desc_header *) NULL, \ | ||
| }; |
There was a problem hiding this comment.
This is the place where the selection of descriptors actually used is happening, where it points to struct fields defined above.
Here there would need a UVC_DESC_ISO_ALT_SETTINGS_PTRS (or some other nice name) that is either empty or contains the extra AltSettings.
| /* Currently selected interface alternate setting */ | ||
| uint8_t alternate; |
There was a problem hiding this comment.
I think it is possible to make it all work without this variable, not used at the moment, and nothing requires knowing if things are on/off in the current API AFAIU.
4 participants
Additional UVC descriptors for catering to the Isochronous Video Streaming Endpoint