-
Notifications
You must be signed in to change notification settings - Fork 8.1k
UVC: move application decision to the application #93192
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
base: main
Are you sure you want to change the base?
UVC: move application decision to the application #93192
Conversation
This is not ready for review, will be properly split in individual commits, and "un-drafted" when ready... |
Force-push:
Windows, Linux, MacOSX, Android are now expected to all work. Tested with nRF52840-DK:
Tested with Arduino Nicla Vision:
|
f1cb80e
to
f5328dd
Compare
Force-push:
|
|
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.
Needs rebase.
9180180
to
ee542de
Compare
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.
LGTM, thanks a lot !!
@jfischer-no, @tmon-nordic, could you have a look at this PR which is a dependency for some others ? Thanks. |
include/zephyr/usb/class/usbd_uvc.h
Outdated
* At runtime, it will forward all USB controls from the host to this device. | ||
* It will query its supported video controls and frame intervals and use this information to | ||
* generate the USB descriptors presented to the host. At runtime, it will forward all USB controls | ||
* from the host to this device. |
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.
At runtime, it will forward all USB controls from the host to this device.
How would this function "forward all USB controls" and why? I doubt this documentation is correct. Perhaps it needs different wording, also not clear how the changes are relevant to the commit message.
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.
What I meant was that the class would turn USB control commands for the UVC class into "video controls" at the Zephyr Video API level.
Good opportunity to improve the inaccurate documentation.
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 realized that I did integrate @avolmat-st suggestions into existing commits, I just split them out like I should have.
ee542de
to
915e75b
Compare
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.
Just some nits, otherwise LGTM.
For supporting situations where UVC formats are dynamically generated at runtime, do not return an error in case descriptors were not added, but instead verbosely log the error and allow enumeration to happen. Signed-off-by: Josuah Demangeon <[email protected]>
Make use of the recently merged fmt->size to know the maximum size of the frame to be allocated, which works for both compressed and uncompressed video. Signed-off-by: Josuah Demangeon <[email protected]>
915e75b
to
104a3f6
Compare
The UVC class was deciding itself which formats were sent to the host. Remove this logic out of the UVC class and introduce uvc_add_format() to give the application the freedom of which format to list. Signed-off-by: Josuah Demangeon <[email protected]>
The UVC class now lets the application select the format list sent to the host. Leverage this in the sample to filter out any format that is not expected to work (buffer too big, rarely supported formats). Signed-off-by: Josuah Demangeon <[email protected]>
Add USB UVC device's new uvc_add_format() function to the release note, and document the semantic changes of UVC APIs in the migration guide. Signed-off-by: Josuah Demangeon <[email protected]>
104a3f6
to
95cc035
Compare
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.
Force-push:
- Rebased on
main
- Applied @ngphibang suggestions
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.
.
|
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.
LGTM. Thanks
Dependency:
The USB class was doing arbitrary "best guess" choices about what pixel formats or resolutions to include (all of them, and min/max only when format are ranges).
uvc_add_format()
to allow the application to push the exact format it wantsWhat is added in the sample is a matter of application choices, and trapping these choices in the UVC class prevented the samples to work without manual configuration on diverse platform.
MacOS support still not confirmed.
Linux, Windows, Android tested functional with the hardware I have.