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.

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

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

Conversation

lijunru-hub
Copy link
Contributor

@lijunru-hub lijunru-hub commented Jan 3, 2025

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
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/uvc_host.c 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/include/usb/uvc_host.h Outdated Show resolved Hide resolved
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.

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

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

@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

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]

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]
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 this PR!

The API changes LGTM and the features are very useful. I have only one concern about memory allocation. Could you please fix it? You can contact me if anything is unclear

Thanks again!

*
* @param[in] config_desc Pointer to the USB configuration descriptor.
* @param[in] bInterfaceNumber The interface number to search for frame descriptors.
* @param[out] frame_info_list Pointer to a list of frame info structures (allocated dynamically).
Copy link
Collaborator

Choose a reason for hiding this comment

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

My main concern aligns with the issues flagged by the static analyzer: the use of dynamic memory allocation.

This violates the principle of ownership in dynamic memory management—if you allocate memory, it's your responsibility to ensure it is properly freed. Relying on others to manage memory you allocate can lead to resource leaks or undefined behavior. Consider revising this to avoid dynamic allocation or clearly define ownership and the lifecycle of the allocated memory.

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 API needs to be designed to be called twice: the first call retrieves the list length, and after the user allocates sufficient memory, the second call fills the data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this:

  1. the new device callback (uvc_host_driver_event_data_t) gives user number of frame formats.
  2. In the callback, the user allocates the memory (array of uvc_host_frame_info_t) and passes pointer and size of this array to uvc_desc_get_frame_list, which fills the array
  3. The user is responsible for freeing the memory when he is done

So it could look like this

typedef struct {
    enum uvc_host_driver_event type;      /**< Event type */
    union {
        struct {
            uint8_t dev_addr;             /**< Device address */
            uint8_t uvc_stream_index;     /**< Index of UVC function you want to use. Set to 0 to use first available UVC function */
            size_t frame_info_num;        /**< Number of frame information list */
        } device_connected;               /**< UVC_HOST_DEVICE_CONNECTED event */
    };
} uvc_host_driver_event_data_t;
// This is public API
esp_err_t uvc_desc_get_frame_list(
    uint8_t dev_addr;                  /**< Pass value from the callback */
    uint8_t uvc_stream_index;     /**< Pass value from the callback */ // TODO:Update these comments. This function can be also called from other places
    uvc_host_frame_info_t (*frame_info_list)[], // Pointer to array of uvc_host_frame_info_t
    size_t list_size // Size of the list - to avoid out-of-boundaries array access
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Opened Issue is new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants