Skip to content

Conversation

@jfischer-no
Copy link
Contributor

Support BOS descriptor with vendor request code and add WebUSB sample for the new USB device stack.

Add a function similar to sample_usbd_init_device(), but one that does
not initialize the device. It allows the application to set additional
features, such as additional descriptors.

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no added area: USB Universal Serial Bus Experimental Experimental features not enabled by default labels Jul 26, 2024
@jfischer-no jfischer-no added this to the v4.0.0 milestone Jul 26, 2024
@jfischer-no jfischer-no self-assigned this Jul 26, 2024
@zephyrbot zephyrbot added the area: Samples Samples label Jul 26, 2024
@zephyrbot zephyrbot requested review from kartben and nashif July 26, 2024 12:08
Copy link
Contributor

Choose a reason for hiding this comment

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

Why abuse the descriptors list? I think it would be better to just have a separate list for the vendor requests. The vendor requests should not be limited to BOS-only, but generally any vendor (here: application developer) should be able to provide their own vendor commands in application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why abuse the descriptors list?

It is a descriptor and it does not abusing anything.

I think it would be better to just have a separate list for the vendor requests. The vendor requests should not be limited to BOS-only, but generally any vendor (here: application developer) should be able to provide their own vendor commands in application.

There are no requirements in the specification to allow arbitrary vendor request with recipient device. Using the BOS platform capability is the most portable.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is this a descriptor? This is not descriptor in any way, just a way for application to register vendor commands. USB specification doesn't require the devices to implement any optional functionality (vendor commands are optional functionality). In this context the specification states "A device vendor may also define requests supported by the device.".

What do you mean by "most portable"? What does portability have to do with a stack functionality to allow application developer to support vendor commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please familiarize yourself with how MSOS2 and WebUSB use the BOS descriptor, I am pretty sure you will understand it.

Comment on lines 64 to 114
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is defined in a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? Or what is the problem? This is limited to a specific part and can be used as a template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything compilation unit that includes this header will have the variables in the corresponding object file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only that include it. So where is the problem?

Comment on lines 29 to 88
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is defined in a header?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to somehow take the advantage of the new stack to enable automatic struct msosv2_function_subset_header insertion when there is more than 1 interface?

The old stack did have #66449 as a solution, but that's fragile approach.

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 do not think we can handle it. IMO it is better to write a comprehensive documentation about it, with some examples. I think the average user has no idea where and how to use MSOSv2 descriptors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comprehensive documentation is not a real solution but a way to avoid having to come up with a real solution. While the average user might not have idea about individual MSOSv2 descriptors, it would be good to provide "just-working" experience (if possible).

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

I would prefer a generic way to register vendor requests targeting the device (not a class nor an interface) from the application code. This could easily be used for handling MSOSv2 vendor requests, but wouldn't be limited to that (not all vendor requests would require a BOS).

@github-actions github-actions bot added the Stale label Oct 13, 2024
@zephyrproject-rtos zephyrproject-rtos deleted a comment from github-actions bot Oct 17, 2024
@jfischer-no jfischer-no force-pushed the pr-device_next-sample-webusb branch 3 times, most recently from 5291081 to 6f692c4 Compare October 21, 2024 10:51
@carlescufi
Copy link
Member

@tmon-nordic @henrikbrixandersen can you please take another look?

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.

The new bRequest registration seems fine, just few things to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer some prefix on the node, e.g.

Suggested change
static struct usbd_vreq_node name = { \
static struct usbd_vreq_node usbd_vreq_node_##name = { \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the user does not know the real structure name. This makes sense for more abstract macros, but not here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If not implement it right now, at least add a TODO: Add subset header when there is more than one interface.

See #66449 for more information.

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 have aligned it with the descriptor in the legacy sample and added a comment.

Allow the user to register a vendor request node identified by the
vendor code (bRequest) and containing two callbacks to handle the vendor
request. The device stack uses the vendor request node to call the
vendor request callbacks when it receives a request of type Vendor,
recipient Device, and bRequest value equal to the vendor code.

Signed-off-by: Johann Fischer <[email protected]>
Platform capability descriptors such as MSOSv2 or WebUSB BOS have a
vendor request code that is used by the host to perform vendor-specific
requests. Add a convenient way to define and register a platform
capability descriptor with a vendor request node.

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no force-pushed the pr-device_next-sample-webusb branch from 6f692c4 to eb96943 Compare October 25, 2024 16:24
@jfischer-no jfischer-no modified the milestones: v4.0.0, 4.1 Oct 25, 2024
@jfischer-no jfischer-no added this to the v4.1.0 milestone Oct 25, 2024
Add a WebUSB sample that uses the new USB device support.

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no force-pushed the pr-device_next-sample-webusb branch from eb96943 to 210cda7 Compare October 25, 2024 16:39
@carlescufi
Copy link
Member

@henrikbrixandersen PTAL

@nashif nashif merged commit 1687e19 into zephyrproject-rtos:main Nov 19, 2024
23 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 Experimental Experimental features not enabled by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

usb: device_next: unable to handle vendor requests in address state

6 participants