Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(usb_host_uvc): support dual camera with hub (IEC-246) #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(usb_host_uvc): support dual camera with hub (IEC-246) #109

wants to merge 2 commits into from

Conversation

lijunru-hub
Copy link
Contributor

@lijunru-hub lijunru-hub commented Jan 3, 2025 •
edited
Loading

Description

Feature:

  1. Fixed the issue where connecting to the hub would automatically power it on.
  2. Added a callback: When a USB camera device is inserted, notify the user with the VID, PID, and camera parameters.
  3. Supported printing the complete USB descriptor for debugging when a new device is inserted.
  4. Supported opening a specified device using usb_handle or dev_addr.

Related

Testing

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@lijunru-hub lijunru-hub changed the title feat(usb_host_uvc): support dual camera with hub draft: feat(usb_host_uvc): support dual camera with hub Jan 3, 2025
@lijunru-hub lijunru-hub force-pushed the feat/uvc_support_dual_camera branch from 93fde33 to 7d193d8 Compare January 3, 2025 12:34
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 3, 2025
@github-actions github-actions bot changed the title draft: feat(usb_host_uvc): support dual camera with hub draft: feat(usb_host_uvc): support dual camera with hub (IEC-246) Jan 3, 2025
tore-espressif
tore-espressif reviewed Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

@lijunru-hub thank you very much for fixing this limitation.

The new open API with dev_addr looks good to me.

I left few comments for your consideration

host/class/uvc/usb_host_uvc/private_include/uvc_descriptors_priv.h
@@ -75,6 +85,10 @@ esp_err_t uvc_desc_get_frame_format_by_format(
const uvc_format_desc_t **format_desc_ret,
const uvc_frame_desc_t **frame_desc_ret);

bool uvc_desc_if_uvc_device(const usb_config_desc_t *cfg_desc);

void uvc_print_desc(const usb_standard_desc_t *_desc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that you used this function for debugging... Could we revert it and make it private again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is still private for the user (priv_include). It is called within the uvc_host_device_connected function to print the parsed UVC descriptor. This helps us debug issues with some customer cameras, such as when they cannot be opened properly. By printing the full descriptor upon insertion, it provides useful information for troubleshooting.

host/class/uvc/usb_host_uvc/private_include/uvc_descriptors_priv.h Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc/uvc_host.c Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc/uvc_descriptor_printing.c
@@ -365,7 +365,7 @@ static void print_class_specific_desc(const usb_standard_desc_t *_desc)
*
* @param[in] _desc UVC specific descriptor
*/
static void uvc_print_desc(const usb_standard_desc_t *_desc)
void uvc_print_desc(const usb_standard_desc_t *_desc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we revert this change?

host/class/uvc/usb_host_uvc/uvc_host.c Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc/private_include/uvc_descriptors_priv.h Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc/include/usb/uvc_host.h Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc/include/usb/uvc_host.h Outdated
union {
struct {
uint8_t dev_addr;
uint8_t iface_num; //!< Disconnection event
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why we need to pass iface_num to the user... Do you have any example of usage?

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 misunderstood the meaning of uvc_host_stream_config_t: uvc_stream_index. We should change iface_num to uvc_stream_index here.

If a camera supports multiple video streams, the callback will be called multiple times, with the uvc_stream_index being returned incrementally each time.

host/class/uvc/usb_host_uvc/uvc_host.c Outdated
Comment on lines 125 to 132
// Create UAC interfaces list in RAM, connected to the particular USB dev
if (is_uvc_device) {
// TODO: add config to control it
// usb_print_config_descriptor(config_desc, &uvc_print_desc);
// Create Interfaces list for a possibility to claim Interface
ESP_RETURN_ON_ERROR(uvc_host_interface_check(addr, config_desc), TAG, "uvc stream interface not found");
} else {
ESP_LOGW(TAG, "USB device with addr(%d) is not UVC device", addr);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do not pass iface_num to the user, this whole part can be removed, simplifying the code a lot. Do we really need it?

Copy link
Contributor Author

@lijunru-hub lijunru-hub Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Some devices have multiple VS interfaces. I would like to notify the user about all the VS interfaces sequentially upon insertion, so that the user can open the interface they wish to use and retrieve all the supported resolutions for that VS uvc_stream_index .

image

host/class/uvc/usb_host_uvc/private_include/uvc_types_priv.h Outdated Show resolved Hide resolved
@lijunru-hub lijunru-hub changed the title draft: feat(usb_host_uvc): support dual camera with hub (IEC-246) feat(usb_host_uvc): support dual camera with hub (IEC-246) Jan 6, 2025
@lijunru-hub lijunru-hub force-pushed the feat/uvc_support_dual_camera branch from 7d193d8 to c037c57 Compare January 6, 2025 12:55
@lijunru-hub
Copy link
Contributor Author

@tore-espressif I added an API, uvc_desc_get_frame_list, to retrieve all supported resolutions within the VS interface and pass them to the user during the connection process.

@lijunru-hub lijunru-hub force-pushed the feat/uvc_support_dual_camera branch 2 times, most recently from 0bb1f2b to 9bc9961 Compare January 8, 2025 09:35
@lijunru-hub lijunru-hub force-pushed the feat/uvc_support_dual_camera branch from 9bc9961 to 290d524 Compare January 8, 2025 09:36
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 8, 2025
View reviewed changes
host/class/uvc/usb_host_uvc/uvc_descriptor_parsing.c

num_frame = this_format->bNumFrameDescriptors;
new_frame_info = calloc(num_frame, sizeof(uvc_host_frame_info_t));
UVC_CHECK(new_frame_info != NULL, ESP_ERR_NO_MEM);

Check warning

Code scanning / clang-tidy

Potential leak of memory pointed to by 'frame_info' [clang-analyzer-unix.Malloc] Warning

Potential leak of memory pointed to by 'frame_info' [clang-analyzer-unix.Malloc]
host/class/uvc/usb_host_uvc/uvc_descriptor_parsing.c

num_frame = this_format->bNumFrameDescriptors;
new_frame_info = calloc(num_frame, sizeof(uvc_host_frame_info_t));
UVC_CHECK(new_frame_info != NULL, ESP_ERR_NO_MEM);

Check warning

Code scanning / clang-tidy

Potential leak of memory pointed to by 'new_frame_info' [clang-analyzer-unix.Malloc] Warning

Potential leak of memory pointed to by 'new_frame_info' [clang-analyzer-unix.Malloc]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tore-espressif tore-espressif tore-espressif left review comments

@roma-jam roma-jam roma-jam left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@lijunru-hub lijunru-hub

Labels
Status: Opened Issue is new
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lijunru-hub @roma-jam @tore-espressif @espressif-bot