Skip to content

Conversation

@josuah
Copy link
Contributor

@josuah josuah commented Jul 16, 2025

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).

  • In the class, add a uvc_add_format() to allow the application to push the exact format it wants
  • In the class, remove all format-related video API calls away from the UVC stack, to let the application handle this
  • In the sample, filter out formats that are not supported by the host whenever standard formats are also supported
  • In the sample, filter out formats that are too large for the memory

What 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.

@josuah josuah marked this pull request as draft July 16, 2025 10:22
@github-actions github-actions bot added area: USB Universal Serial Bus area: Samples Samples labels Jul 16, 2025
@github-actions github-actions bot requested a review from nashif July 16, 2025 10:23
@josuah
Copy link
Contributor Author

josuah commented Jul 16, 2025

This is not ready for review, will be properly split in individual commits, and "un-drafted" when ready...

@josuah josuah force-pushed the pr_uvc_no_policy branch from 70ab814 to 4833717 Compare July 18, 2025 21:48
@josuah josuah changed the title [WIP] UVC: remove the policies from the class UVC: remove the policies from the class Jul 18, 2025
@josuah
Copy link
Contributor Author

josuah commented Jul 18, 2025

Force-push:

  • Rebase on top of the MacOSX-related bugfix

Windows, Linux, MacOSX, Android are now expected to all work.

Tested with nRF52840-DK:

west build --board=nrf52840dk/nrf52840 --snippet=video-sw-generator samples/subsys/usb/uvc/

Tested with Arduino Nicla Vision:

west build --board=arduino_nicla_vision/stm32h747xx/m7 samples/subsys/usb/uvc/

@josuah josuah added area: Video Video subsystem Release Notes Required Release notes required for this change labels Jul 18, 2025
@josuah josuah changed the title UVC: remove the policies from the class UVC: move application decision to the application Jul 19, 2025
@josuah josuah force-pushed the pr_uvc_no_policy branch 3 times, most recently from f1cb80e to f5328dd Compare July 24, 2025 16:23
@josuah
Copy link
Contributor Author

josuah commented Jul 24, 2025

Force-push:

  • Rebase on main to fix conflict.
  • Reword the documentation and commit messages.
  • Add a migration guide entry for the semantic change (but no direct API breakage).

@sonarqubecloud
Copy link

@josuah josuah marked this pull request as ready for review July 24, 2025 16:56
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Jul 24, 2025
@zephyrbot zephyrbot requested review from cfriedt and jhedberg July 24, 2025 16:57
tmon-nordic
tmon-nordic previously approved these changes Sep 15, 2025
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs rebase.

@josuah
Copy link
Contributor Author

josuah commented Oct 21, 2025

Force-push: applying review suggestions

  • Return the error code even though the code continues to run
  • Fix doc typos

@josuah josuah requested a review from jfischer-no October 21, 2025 17:22
avolmat-st
avolmat-st previously approved these changes Oct 22, 2025
Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

tmon-nordic
tmon-nordic previously approved these changes Oct 22, 2025
@josuah josuah dismissed stale reviews from tmon-nordic and avolmat-st via bc63264 October 22, 2025 15:18
@josuah
Copy link
Contributor Author

josuah commented Oct 23, 2025

nRF52840 with -DCONFIG_USBD_VIDEO_MAX_FORMATS=1 (completely invalid, should not work):

*** Booting Zephyr OS build v4.2.0-6382-g042a6e7b3bb7 ***
[00:00:00.250,183] <wrn> usbd_uvc: Not enough format descriptors to add descriptors for 'YUYV' and 64x64
[00:00:00.250,213] <wrn> uvc_sample: Skipping format 1920x1080
[00:00:00.250,366] <err> usbd_uvc: Invalid format ID (1) and/or frame ID (1)
[00:00:00.250,366] <err> usbd_uvc: init: failed to query the default probe
[00:00:00.250,366] <err> usbd_init: Failed to initialize class instance
[00:00:00.250,396] <err> usbd_init: Failed to init FS configuration 1
[00:00:00.250,457] <err> usbd_sample_config: Failed to initialize device support

nRF52840 with -DCONFIG_USBD_VIDEO_MAX_FORMATS=2 (just enough for video-sw-generator):

*** Booting Zephyr OS build v4.2.0-6382-g042a6e7b3bb7 ***
[00:00:00.250,213] <wrn> uvc_sample: Skipping format 1920x1080
[00:00:00.250,457] <inf> uvc_sample: Waiting the host to select the video format

Here on FRDM-MCXN947 with -DCONFIG_USBD_VIDEO_MAX_FORMATS=2:

mpv-shot0006

@josuah
Copy link
Contributor Author

josuah commented Oct 23, 2025

Force-push:

  • Use a much more basic way to report failure: check if it will fit in uvc_add_format() and exit early if it would not.
  • Remove first_err as a consequence, use normal if (ret != 0) { return ret; }

[EDIT: sonarcubecloud says that variables used only in LOG_INF() are never used, sounds safe to ignore]

When running out of descriptor, return an error instead of ignoring it.
The application need to make sure to adjust the Kconfig macros to have
enough descriptors for all formats to add.

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]>
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]>
@sonarqubecloud
Copy link

@avolmat-st
Copy link

LGTM. Tested ok with PR#94562 on top of it via the STM32N6.

Copy link

@avolmat-st avolmat-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested ok with PR#94562 on top of it via the STM32N6.

@josuah josuah removed the Release Notes Required Release notes required for this change label Oct 23, 2025
desc->wWidth = sys_cpu_to_le16(w);
desc->wHeight = sys_cpu_to_le16(h);
desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(max_size);
desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(fmt.size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry nitpicking: for commit "usb: device_next: uvc: use fmt->size instead of computing it every time",
fmt.size is not already set. This gets fixed in a later commit but I would suggest that this commit still uses

	desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(MAX(p, w) * h);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have verified the final version of the file, but this fmt struct is local in this commit. Good catch for bissectability!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this got merged. Since this got raised past-review and is in git history by now. One commit will have a runtime (not compile time) error unfortunately 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was too slow to react. Now it's merged. My bad.

Copy link
Contributor Author

@josuah josuah Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's also very useful as a reminder of what I need to check the next time, so still useful :)

@kartben kartben merged commit ef3e8cc into zephyrproject-rtos:main Oct 23, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Samples Samples area: USB Universal Serial Bus area: Video Video subsystem Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants