Skip to content

Fix unintended control transfers in webserial example.#645

Merged
hathach merged 2 commits intohathach:masterfrom
ipopov:webserial-fix
Feb 11, 2021
Merged

Fix unintended control transfers in webserial example.#645
hathach merged 2 commits intohathach:masterfrom
ipopov:webserial-fix

Conversation

@ipopov
Copy link
Copy Markdown
Contributor

@ipopov ipopov commented Feb 10, 2021

As discussed in #609,

The problem is that when receiving a "standard" CLEAR_FEATURE request, usbd.c calls into the class driver:

if ( invoke_class_control(rhport, driver, p_request) ) ret = true;
. (The comment there says that the class must not call tud_control_status.)

Unfortunately the "class driver", i.e. the webusb example's main function, intereprets this as a "class" request with bRequest==1 (i.e. VENDOR_REQUEST_WEBUSB), rather than as a "endpoint" request with bRequest==CLEAR_FEATURE,

case VENDOR_REQUEST_WEBUSB:
, and sends back a report.

To fix this, the class driver should ignore requests that are not clearly meant for it.

Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR, this is spot-on. I am too lazy to check for the request vendor type. There is only a couple of style feedback

  1. Please add default: return false; to the switch(bRequest)
  2. I am not against openning { in the same line with new code, however, you should not reformat the existing style.

return tud_control_xfer(rhport, request, (void*) desc_ms_os_20, total_len);
} else {
return false;
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

all switch should have default, please add one with return false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. :-)

- every switch must have a default
- revert formatting of unchanged lines
@ipopov
Copy link
Copy Markdown
Contributor Author

ipopov commented Feb 11, 2021

Thank you for the kind comments! It's a wonderful codebase, I had a great time this weekend getting to know it. I hope I can be of assistance in the future as well.

I've uploaded a new commit addressing your comments.

Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thank you very much for your PR. I am appreciate that you have spent time to troubleshoot this issue.

Comment thread examples/device/webusb_serial/src/main.c
@hathach hathach merged commit dc64d6a into hathach:master Feb 11, 2021
@hathach
Copy link
Copy Markdown
Owner

hathach commented Feb 11, 2021

Thank you for the kind comments! It's a wonderful codebase, I had a great time this weekend getting to know it. I hope I can be of assistance in the future as well.

I've uploaded a new commit addressing your comments.

Thank you very much for willing to help, I am looking forward to your future PRs. Feel free to submit any issues/pr/discussion.

PS: see also #646, while this PR fix the behavior of vendor app, 646 allow usbd to detect tud_control_status() is called from vendor and skip duplication action.

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.

2 participants