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

Initial BOS/WebUSB/WinUSB support #91

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

Conversation

mon
Copy link

@mon mon commented Jan 8, 2017

This isn't merge ready, especially in regards to documentation, but I wanted to get it out there and get some discussion on if this is a good way to pull this off.

A brief note on BOS if you've not used it - a BOS descriptor is requested like any other descriptor, then the vendor specific data is pulled out using vendor control requests. This is why I call USB_Process_BOS in EVENT_USB_Device_ControlRequest.

BOS descriptors unfortunately contain a great deal of variable length data and Total Size values which require lookahead, so I've implemented this using nested macros. I hope my example code in BulkVendor shows that this isn't as messy as it sounds.

The MS OS 2.0 descriptor is particularly handy for the Bulk Vendor example, as it negates the need for a custom device driver by telling Windows to use the WinUSB driver. The downside is (AFAIK) this only works on Windows 8.1 and up. I believe it may have been backported to Windows 7 in an update but I have no machine to test.

@abcminiuser
Copy link
Owner

This looks very interesting; I'll have to give it a proper once over this weekend and see if I can see any issues with the approach, but from what I can see now it looks good. It will be great to get an example into LUFA, although it might warrant its own discrete demo project to avoid confusing those who plan on using their own driver.

@abcminiuser
Copy link
Owner

Very interesting! I've ignored the USB 3.x spec in favour of the older 2.x spec since that's all the old USB AVRs are capable (barely) of supporting, but it looks like the new BOS descriptors are actually usable on the older devices.

The approach seems good to me, with some notes:

  • Definitely needs to be in its own WebUSB vendor demo, since not everyone will need the additional features
  • The BOS and other USB3.x descriptors are fine in the StdDescriptors.h header, but the WebUSB stuff should be its own class driver I think - it would also abstract away some of the low level stuff the user doesn't need to see in their application. Any objections to it?
  • Just some some minor code style fixes (space after if, etc.) which I can easily fix before merge.

@mon
Copy link
Author

mon commented Jan 29, 2017

  • Sounds good, I might work on porting the Arduino demos to the LUFA stack. Any objections to using their test URL, since it's known to work? I'm, not great at serial/CDC descriptors so I could have some issues implementing.
  • Sounds great, but I don't think I understand the source well enough to do it myself
  • Easily done, along with more documentation from me

@JamesHagerman
Copy link

I know it's been quite a while since this PR was opened, but it would be wonderful to get this BOS descriptor support into LUFA. The QMK Mechanical Keyboard firmware uses LUFA for a collection of AVR keyboards and I would love to get them working with WebUSB if possible.

Is this PR still something that might be able to get merged in (obviously once the conflicts are resolved, of course)?

@abcminiuser
Copy link
Owner

There's two competing WebUSB PRs at the moment (#91 @mon and #103 @riggs), and I confess I've been completely lazy over the last few months trying to figure out what state each one is in and whether one is a superset of the other.

@mon/@riggs can you two please give me a quick summary of what state your respective patches are in, so I can review one/both and merge them in? As you can probably see from the CCID feature being integrated now, it's a slow process for me given the amount of time it takes to go from working patch to a fully integrated feature with correct coding style and general usability.

@mon
Copy link
Author

mon commented Jul 8, 2018

With an example and good documentation, I'd definitely call riggs' pull to be in a better state.

However, the latest comment (about broken MS OS descriptors) will need to be addressed - mine has no issues there.

Additionally, there's a little too much "userland" code for handling the MS OS stuff that could probably be baked into the library itself.

Probably best to take riggs' pull as a base and take in some of the features I've got.

@riggs
Copy link

riggs commented Jul 14, 2018

I should have a chance to figure out the problems with my PR this week. It's extrapolated from code that's working in the wild, but I clearly missed something.

I think the biggest caveat with my PR is that there are some 'magic' macros to deal with generating BOS descriptors at compile time. They aren't fixed length like the other USB descriptors, so calculating sizes and concatenating the pieces together uses some obnoxious macros. Currently those macros are defined in the project, but perhaps they should live elsewhere. These macros make creating the BOS descriptors easier & less error prone, but also makes them more opaque and reliant on 'clever' code.

@mon
Copy link
Author

mon commented Jul 15, 2018

@riggs Mine has similar "smart" macros as seen here .

I think it's ok, since once they've been tested, the benefits (static, known size) far outweigh the downsides (the code tries too hard)

@riggs
Copy link

riggs commented Jul 15, 2018

I'm specifically referring to these. _BOS_L & friends calculate the length, while the _BOS_D macros concatenate everything. In the end it works, but the inputs have to be wrapped in parens, as noted in the docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants