USB Multi-instance support

[finikorg]

This is RFC for proposed way of multi - instance USB class support.

Multi - instance means that the same instances of some class might be included to the code, for example 2 CDC ACM (Serial ports). Before you could include only one instance of the same class.
I have created sample illustrating this possibility with 2 USB serial ports.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[codecov-io]

Codecov Report

Merging #12682 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #12682      +/-   ##
==========================================
- Coverage   48.62%    48.6%   -0.03%     
==========================================
  Files         313      313              
  Lines       46441    46468      +27     
  Branches    10712    10722      +10     
==========================================
  Hits        22584    22584              
- Misses      19398    19425      +27     
  Partials     4459     4459

Impacted Files	Coverage Δ
subsys/usb/usb_device.c	7.83% <ø> (ø)	⬆️
subsys/usb/usb_descriptor.c	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 128d2e2...e3d0afb. Read the comment docs.

[pfalcon]

This is RFC for proposed way of multi - instance support.

Multi-instance what?

[finikorg]

This is RFC for proposed way of multi - instance support.

Multi-instance what?

Multiple USB classes (several CDC ACM devices, etc)

[MarkWangChinese]

HID class driver APIs have no instance parameter like cdc_acm's "struct device *dev", how HID class driver support multiple HID instances? For example: int hid_int_ep_write(const u8_t *data, u32_t data_len, u32_t *bytes_ret). From my viewpoint, it may be necessary to add the device instance parameter to the HID API, and implement one way to define configurable number of the HID interface/endpoint descriptors because the application determine the instances count. Other classes drivers have the same problem.

How to support the multiple instances of controller and devce stack? for example: RT1050 has two USB controllers, Zephyr application cannot initialize two device stack instances to use the two controllers at the same time. From my viewpoint, it may be necessary to add the device instance parameter to the device stack API and controller API.

If I am wrong, please let me know.

[finikorg]

@MarkWangChinese I will check also all other classes and push more commits for HID, etc. For the multi controller case, I need to think more, probably we need controller device model and change stack to use this specific device functions.

[pawelzadrozniak]

Yep, I'd like all implementations of classes to be unified - i.e. defined as a device, all local data (control block) grouped in one structure which could be retrieved with device structure pointer and no other static variables (unless it's a common variable for all instances).

A couple of ideas.
Macro for retrieving specific class instance count + sum of all instances of any class would be useful. With such macros we could get rid of composite device definition in kconfig.

Each instance will also need a configurable endpoint numbers (kconfig?) and I'd add a test routine executed during usb initialization (included only in debug build) which would go through all instances and check if user has not overlapped EPs in class configurations.

[masz-nordic]

In my opinion something like:

usb_hid_register_device(hid_dev, hid_report_desc, sizeof(hid_report_desc), NULL);
usb_cdc_register_device(cdc_dev, ...);
/* other instances or classes registered */
usb_init(); 

would work better with applications that use multiple classes and multiple instances. Initialization done currently in usb_hid_init() can be moved to hid_status_cb() with new event, for example USB_DC_INITIALIZED.

[finikorg]

ACM CDC is different from the other classes since it has (serial) device already, all other classes should have device similar to HID Device.

About usb_hid_init(): if we move general init to usb_init() then the remaining part we can move to usb_hid_register_device()

[carlescufi]

@masz-nordic @pawelzadrozniak @jfischer-phytec-iot Can you please revisit this PR?

addressed

[jfischer-no]

It is not virtual, where does it come from? Also "class device driver port name" sounds strange, why not "CDC ACM class name" or "CDC ACM interface name"?

[finikorg]

@jfischer-phytec-iot interface and class might be misunderstood in USB. What about 'CDC_ACM_DEVICE_1`
Also another question: Is this OK to have number of instances defined in menuconfig?

[finikorg]

Just wondering if we specify only number of instances, can we create number of devices with macros in include/misc/util.h

[MarkWangChinese]

From my understanding, it need calls cfg->cb_usb_status too as follow if cb_usb_status_composite is NULL and cb_usb_status is not NULL.
if (cfg->cb_usb_status_composite) {
cfg->cb_usb_status_composite(cfg, status, param);
} else if (cfg->cb_usb_status) {
cfg->cb_usb_status(status, param);
}
For example: mass1 + cdc_acm1 + cdc_acm2, the mass storage class driver use cb_usb_status and cdc class driver use cb_usb_status_composite.
Or we can remove cb_usb_status and replace cb_usb_status with cb_usb_status_composite, and rename cb_usb_status_composite as another name.

Please let me know if I am wrong.

[finikorg]

For composite configuration we need to add cb_usb_status_composite to MSC. Even if now only one instance of MSC can be enabled.

Probably renaming callback would be the right approach. So we might have common callback with NULL parameter for non composite configuration in place of cfg.

[MarkWangChinese]

Yes, it is better to rename the callback.
Do you plan to add device instance (struct device *dev ) to all USB class drivers? for example: MSC.
If you plan add it, is it better to add one (struct device *dev) parameter to the follow callback functions?
usb_request_handler class_handler;
usb_request_handler vendor_handler;
usb_request_handler custom_handler;
usb_dc_status_callback cb_usb_status;
void (*cb_usb_status_composite)(struct usb_cfg_data *cfg,
enum usb_dc_status_code cb_status,
const u8_t *param);
And add the device_name (const char *dev_name) to struct usb_cfg_data.
After these changes, the usb_get_dev_data_by_cfg, usb_get_dev_data_by_iface and usb_get_dev_data_by_ep are not needed any more.

[finikorg]

@MarkWangChinese fc34152a3bc7cfa7e668d2bd4524d9727725fe99 renames callbacks. There is other PR #13310. You can comment there, since this PR is merged already.

[MarkWangChinese]

#ifdef CONFIG_USB_COMPOSITE_DEVICE may be needed for the above added lines.

[finikorg]

Will fix it in the follow up patches