usb: device_next: allow to initialize multiple CDC ACM instances#105141
usb: device_next: allow to initialize multiple CDC ACM instances#105141jfischer-no wants to merge 6 commits intozephyrproject-rtos:mainfrom
Conversation
| * instance. If multiple chosen node properties reference the same device, | ||
| * usbd_register_class() will fail with -EBUSY. | ||
| */ | ||
| static int register_multiple(struct usbd_context *const uds_ctx, |
There was a problem hiding this comment.
nit: not sure why this function is called register_multiple if it only registers on class matching the given device (?)
There was a problem hiding this comment.
Mostly because or the Kconfig option name.
5aef515 to
4d68431
Compare
| STRUCT_SECTION_FOREACH_ALTERNATE(usbd_class_hs, usbd_class_node, c_nd) { | ||
| struct usbd_class_data *c_data = c_nd->c_data; | ||
|
|
||
| if (c_data->priv == dev) { |
There was a problem hiding this comment.
Isn't the whole idea behind private data structures, to actually be private to class implementations? Why comparing it up here against dev is appropriate?
| if (c_data->priv == dev) { | ||
| err = usbd_register_class(&cdc_acm_serial, | ||
| c_data->name, speed, 1); | ||
| if (err != 0 && err != -EBUSY) { |
There was a problem hiding this comment.
Multiple chosen nodes pointing to the same CDC ACM instance effectively rely on -EBUSY error being returned when instance is already registered. Class already being registered is not the only case when -EBUSY is returned. Why some errors can be treated differently from others?
4d68431 to
da1d6ad
Compare
|
| STRUCT_SECTION_FOREACH_ALTERNATE(usbd_class_fs, usbd_class_node, c_nd) { | ||
| struct usbd_class_data *c_data = c_nd->c_data; | ||
|
|
||
| if (usbd_class_get_private(c_data) == dev) { |
There was a problem hiding this comment.
Isn't the whole idea behind private data structures, to actually be private to class implementations? Calling usbd_class_get_private() to get access to private data structure is not changing the underlying problem with the fact that outside code should be not be allowed to look into private data structures.
There was a problem hiding this comment.
So in other words, are you asking that CDC ACM class implementation expose an intermediate utility, say in usb_cdc.h, for getting the associated device?
There was a problem hiding this comment.
If there is a need to access some information that is only accessible via class internals, then there is a need to expose the information using a proper API.
There was a problem hiding this comment.
Isn't the whole idea behind private data structures, to actually be private to class implementations? Calling
usbd_class_get_private()to get access to private data structure is not changing the underlying problem with the fact that outside code should be not be allowed to look into private data structures.
private void pointers are usually used to store references to a mapped device, context in Zephyr, or some user data, not only in USB. usbd_class_get_private() is a public USBD API to get the reference. This code is located in USB device support subsystem directory. I absolutely do not see any reason to introduce yet another API to get UART device mapped to a specific CDC ACM instance.
There was a problem hiding this comment.
I absolutely do not see any reason to introduce yet another API to get UART device mapped to a specific CDC ACM instance.
While I think there is no need to get UART device mapped to a specific CDC ACM instance, there is a real need to get CDC ACM instance identified based on UART device.
Exactly the same root cause (no possibility to match zephyr,cdc-acm-uart with CDC ACM class instance) was the reason behind #103844. There you were quick to reject it and state:
You do not have to use "register all functions"!
While it is true that using usbd_register_all_classes is not mandatory, there is currently no interface that would allow the user to know which class to register if they want to have some particular USB functionality that is instantiated using devicetree. Exactly the same issue applies to zephyr,cdc-acm-uart, zephyr,hid-device, zephyr,uac2, zephyr,midi2-device. All these classes currently rely on usbd_register_all_classes and the fact that in most applications there is just a single UDC controller and a single USB device context.
To solve the problem, I believe we would need to have something like USBD_DEFINE_CLASS_DT() that would do what USBD_DEFINE_CLASS() does and create a const symbol that can be used at link time to match the USB class identifier (e.g. "cdc_acm_0") that should be possible to be obtained using some macro like USBD_CLASS_GET_DT(). Having such macro, there would be no need for violating encapsulation principle and the whole iteration you have here looking for the class instance would become completely unnecessary.
| if (usbd_class_get_private(c_data) == dev) { | ||
| err = usbd_register_class(&cdc_acm_serial, | ||
| c_data->name, speed, 1); | ||
| if (err != 0 && err != -EALREADY) { |
There was a problem hiding this comment.
Why not determine if there are duplicates in the uart_devs list instead of handling -EALREADY here?
There was a problem hiding this comment.
Why make it more complicated and "determine if there are duplicates in the uart_devs list" for no reason?
There was a problem hiding this comment.
Because sanitizing the list is effectively simpler than relying on specific errors to happen.
| if (err) { | ||
| LOG_ERR("Failed to register classes"); | ||
| return err; | ||
| if (IS_ENABLED(CONFIG_CDC_ACM_SERIAL_MULTIPLE_INSTANCES)) { |
There was a problem hiding this comment.
Is it possible to deduce the same from ARRAY_SIZE(uart_devs) > 1? Or maybe it was preferred to use Kconfig for other reasons.
If so, I wonder if only using the for() loop only would work.
I might be missing a detail of backward compatibility with the legacy stack.
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
da1d6ad to
203c325
Compare
Add common mapping to sample.yaml. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
…tered Return -EALREADY if the class instance already registered, which, in some circumstances, may be considered a non-critical error. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
The new macro has an additional parameter that takes a string literal that can be used as a reference. For example, to have a reference between device name that a class instance is implementing and class name, which is used to register a class. The device name can be passed using the DEVICE_DT_NAME() macro. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Use USBD_DEFINE_CLASS_WITH_NAME() in CDC ACM implementation. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Application can use new option CDC_ACM_SERIAL_MULTIPLE_INSTANCES if different CDC ACM serial backends are required for common use cases such as logging, the shell, and specific protocols. This option also guarantees the order in which the instances will be registered and appear in the configuration descriptor. The option uses the chosen node properties to identify UART devices. The following are currently supported, in this order: "zephyr,console", "zephyr,shell-uart", "zephyr,uart-mcumgr", "zephyr,bt-c2h-uart", "zephyr-ot-uart", "zephyr-bt_mon-uart". A supported property may be missing, and properties may reference the same device. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
203c325 to
e7e0d1e
Compare
Remove Kconfig option CDC_ACM_SERIAL_MULTIPLE_INSTANCES. Instead initialize multiple instances when there are multiple nodes with compatible "zephyr,cdc-acm-usrt". Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
e7e0d1e to
740cebd
Compare
|



Application can use new option CDC_ACM_SERIAL_MULTIPLE_INSTANCES if different CDC ACM serial backends are required for common use cases such as logging, the shell, and specific protocols. This option also guarantees the order in which the instances will be registered and appear in the configuration descriptor. The option uses the chosen node properties to identify UART devices.
The following are currently supported, in this order: "zephyr,console", "zephyr,shell-uart", "zephyr,uart-mcumgr". A supported property may be missing, and properties may reference the same device.