Skip to content

usbus: add descriptor prefix support#12429

Merged
dylad merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/usbus/pre_descriptor
Oct 20, 2019
Merged

usbus: add descriptor prefix support#12429
dylad merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/usbus/pre_descriptor

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Oct 11, 2019 •
edited
Loading

Contribution description

This PR adds support to the descriptor generators to prepend descriptors to the current element. This is required to add an interface association descriptor before the CDC ACM control interface descriptor. With this descriptor, Windows should correctly detect a RIOT-based CDC ACM peripheral and use the USBser.sys driver out of the box.

Testing procedure

Test together with #12430, the interface association should always be right before the Communications Abstract modem interface descriptor.

Issues/PRs references

Required for #12430

Another PR is required to straighten out the terminology s/header/descriptor/g to increase alignment with the USB spec.

@bergzand bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: USB Area: Universal Serial Bus labels Oct 11, 2019
@bergzand bergzand requested review from chrysn and dylad October 11, 2019 18:30
@bergzand bergzand mentioned this pull request Oct 11, 2019
Merged
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 11, 2019
@dylad dylad added this to the Release 2020.01 milestone Oct 17, 2019
@dylad dylad self-assigned this Oct 17, 2019
dylad
dylad reviewed Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Quick overview, just typos or styles issues.

sys/include/usb/usbus.h Outdated
*
* Must return the length of the header that will be generated by
* @ref get_header
* Must return the length total length of the descriptors that will be
Copy link
Copy Markdown
Member

@dylad dylad Oct 17, 2019

Choose a reason for hiding this comment

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

Suggested change
* Must return the length total length of the descriptors that will be
* Must return the total length of the descriptor that will be

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 18, 2019

Choose a reason for hiding this comment

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

Same as below, multiple descriptors can be generated

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 18, 2019

Choose a reason for hiding this comment

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

Clarified this a bit

Copy link
Copy Markdown
Member

@dylad dylad Oct 18, 2019

Choose a reason for hiding this comment

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

Alright

sys/include/usb/usbus.h Outdated
size_t (*get_header_len)(usbus_t *usbus, void *arg);
size_t fixed_len; /**< length of the header if it is a fixed length */
size_t fixed_len; /**< total length of the generated
descripors if they are fixed length */
Copy link
Copy Markdown
Member

@dylad dylad Oct 17, 2019

Choose a reason for hiding this comment

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

Suggested change
descripors if they are fixed length */
descriptors if they are fixed length */

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 18, 2019

Choose a reason for hiding this comment

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

Wait, shouldn't have applied your suggestion, the fixed length must be the sum of the prefixed descriptor and the appended descriptor, so "descriptors" is correct here.

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 18, 2019

Choose a reason for hiding this comment

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

Also clarified this one a bit

Copy link
Copy Markdown
Member

@dylad dylad Oct 18, 2019

Choose a reason for hiding this comment

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

what is bothering me here is the missing T

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 18, 2019

Choose a reason for hiding this comment

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

what is bothering me here is the missing T

I might have accidentally fixed that with my fixups

sys/usb/usbus/usbus_fmt.c Outdated
static inline size_t _get_pre_header(usbus_t *usbus, usbus_hdr_gen_t *hdr)
{
return hdr->funcs->get_header(usbus, hdr->arg);
return hdr->funcs->fmt_pre_descriptor != NULL
Copy link
Copy Markdown
Member

@dylad dylad Oct 17, 2019

Choose a reason for hiding this comment

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

Suggested change
return hdr->funcs->fmt_pre_descriptor != NULL
return (hdr->funcs->fmt_pre_descriptor != NULL)

sys/usb/usbus/usbus_fmt.c Outdated

static inline size_t _get_additional_header(usbus_t *usbus, usbus_hdr_gen_t *hdr)
{
return hdr->funcs->get_header != NULL
Copy link
Copy Markdown
Member

@dylad dylad Oct 17, 2019

Choose a reason for hiding this comment

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

Suggested change
return hdr->funcs->get_header != NULL
(return hdr->funcs->get_header != NULL)

dylad
dylad reviewed Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Changes look good, I'll test on hardware just to ensure nothing is broken during the weekend.
You can squash if you want.

@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 18, 2019
@bergzand bergzand force-pushed the pr/usbus/pre_descriptor branch from d5d881d to bce70bf Compare October 18, 2019 20:39
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2019

Squashed

@bergzand bergzand mentioned this pull request Oct 18, 2019
Merged
dylad
dylad approved these changes Oct 20, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Tested on SAME54 with cdc_acm, and lsusb. Everything looks sane.

@dylad dylad added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 20, 2019
@dylad dylad merged commit d9c240a into RIOT-OS:master Oct 20, 2019
@bergzand bergzand deleted the pr/usbus/pre_descriptor branch October 20, 2019 13:56
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 20, 2019

Tested on SAME54 with cdc_acm, and lsusb. Everything looks sane.

Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dylad dylad dylad approved these changes

@chrysn chrysn Awaiting requested review from chrysn

Assignees

@dylad dylad

Labels

Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@bergzand @dylad @benpicco