Implement setInterface(alt) for usb net driver #345

hathach · 2020-04-15T11:10:53Z

mostly forward these request (recipient = interface) to class driver.
also slightly update the lwip example descriptor

follow up to #342 and #344 (merging these first make bisecting easier)
related to #320 #317

mostly forward these request (recipient = interface) to class driver.

pigrew · 2020-04-15T12:03:30Z

I believe that GET_INTERFACE must be always implemented?

I think that either it needs to be handled in usbd (as in #317 and #320), or a handler must be added to all classes' control request callback

majbthrd · 2020-04-15T14:37:00Z

examples/device/net_lwip_webserver/src/usb_descriptors.c

 // Invoked when received GET CONFIGURATION DESCRIPTOR
 // Application return pointer to descriptor
 // Descriptor contents must exist long enough for transfer to complete
 uint8_t const * tud_descriptor_configuration_cb(uint8_t index)
 {
-  return (0 == index) ? main_configuration : alt_configuration;
+  return configuration_arr[index];


Should there be bound checking? The advantage of the old approach was a valid pointer configuration descriptor would always be returned.

yeah right, I am too focused on the usbd. Just fixed it in the new commit. Also slightly changed the CONFIG_NUM to CONFIG_ID. Let's me know if you want to keep the NUM convention more.

also add configuration array bound check.

…t alt interface

don't return false with STD request get/setInterface() or targeted Data Interface (itfnum +1)

hathach · 2020-04-15T16:12:11Z

I believe that GET_INTERFACE must be always implemented?

I think that either it needs to be handled in usbd (as in #317 and #320), or a handler must be added to all classes' control request callback

Yeah, you are right, macOS does sometime send out that request. I think it will just send that request toward CDC class. Just fixed in the new commit.

majbthrd

The revised tud_descriptor_configuration_cb() in examples/device/net_lwip_webserver/src/usb_descriptors.c causes me some heartburn, but it isn't that big a deal.

One of the tests I did was to hack up usbd.c to support multiple configurations and use a DCD API that can reset non-control endpoints. That allows repeatedly switching between RNDIS and ECM, which is a good way to torture test operation.

Thanks, @hathach!

hathach · 2020-04-15T16:26:56Z

The revised tud_descriptor_configuration_cb() in examples/device/net_lwip_webserver/src/usb_descriptors.c causes me some heartburn, but it isn't that big a deal.

Why is that, I am open to all suggestion.

One of the tests I did was to hack up usbd.c to support multiple configurations and use a DCD API that can reset non-control endpoints. That allows repeatedly switching between RNDIS and ECM, which is a good way to torture test operation.

Thanks, @hathach!

Yeah, dynamic switching configuration require an close all endpoints. We will get there with dcd_edpt_close(), but I am not rush to do that. Since we solve all the problems for usb-net (multiple configuration + setInterface()). Although we don't close the Data's endpoints. Host won't actually try to read/write to those endpoints anyway (since we trick it to think so :D ) . So it is rather safe in this class driver.

When adding Audio or UVC (video) class driver, a proper edpt_close is needed, hopefully this buys us enough of time.

PS: btw, please review other pr as well. I try to break this into 3 PRs for easy reviewing (hopefully).

majbthrd · 2020-04-15T16:30:16Z

The revised tud_descriptor_configuration_cb() in examples/device/net_lwip_webserver/src/usb_descriptors.c causes me some heartburn, but it isn't that big a deal.

Why is that, I am open to all suggestion.

Sorry, I was referring to the older implementation without bounds-checking. I submitted the review about the same time you pushed the latest commits.

hathach · 2020-04-15T16:32:14Z

The revised tud_descriptor_configuration_cb() in examples/device/net_lwip_webserver/src/usb_descriptors.c causes me some heartburn, but it isn't that big a deal.

Why is that, I am open to all suggestion.

Sorry, I was referring to the older implementation without bounds-checking. I submitted the review about the same time you pushed the latest commits.

yeah, It is hard to get that though :D since host won't attempt to set an invalid config number. But right, better to be safe.

pigrew · 2020-04-15T16:44:52Z

@hathach,

I'm finding it hard to go through all these changes at once.

Could you change this PR to be against the 'enhance-itf-assoc' branch or merge 'class-driver-id' so it's more clear exactly what this PR is?

I'm trying to go through the PR in order of application to 'master' right now.

hathach · 2020-04-15T16:49:23Z

@hathach,

I'm finding it hard to go through all these changes at once.

Could you change this PR to be against the 'enhance-itf-assoc' branch or merge 'class-driver-id' so it's more clear exactly what this PR is?

I'm trying to go through the PR in order of application to 'master' right now.

Yeah, it is a bit confusing, Basically they share same commit path, I made an PR #342 then branch from that to make PR #344 then make a branch and to this PR. I those PR is merged in order, it will make it easier to follow.

hathach · 2020-04-15T17:21:34Z

gh tips: change target base to another, then back to master to get file changes updated :D

pigrew

I read the code (but have not tested it). In general, it's an improvement.

I'd suggest waiting to merge this until there is an edpt_close() function available, so that we don't worry about hacks in master.

src/class/net/net_device.c

src/device/usbd.c

pigrew

Seems good to me based on a read-through. You could try adding a "stall" then "unstall" to the bulk EP to reset DTOG in the SET_INTERFACE handler.

pigrew · 2020-04-16T14:26:16Z

src/class/net/net_device.c

+              tud_network_init_cb();
+              can_xmit = true; // we are ready to transmit a packet
+              tud_network_recv_renew(); // prepare for incoming packets
+            }


Perhaps another hack could be to do a "stall" and then an "unstall" on the two bulk EP, in order to reset their DTOGs, in the case that the EP are already open. But, not necessary, since it's hacky anyway.

that is a good hack, though let's just leave it this way, to make sure we got an actual error and have motivation to fix it :)

hathach added 3 commits April 15, 2020 17:51

implement alternate setInterface() request

caa1dce

mostly forward these request (recipient = interface) to class driver.

better demonstrate usbnet exmaple is multiple configurations

325065a

remove unused usb_descriptors.h

dc9f5cc

hathach requested review from majbthrd and pigrew April 15, 2020 11:10

majbthrd reviewed Apr 15, 2020

View reviewed changes

hathach added 3 commits April 15, 2020 22:28

rename config num to id in net webserver example

ae9f01f

also add configuration array bound check.

always response if GET_INTERFACE even if class driver does not suppor…

0ddbda0

…t alt interface

correct usbnet control complete response

86ff565

don't return false with STD request get/setInterface() or targeted Data Interface (itfnum +1)

majbthrd approved these changes Apr 15, 2020

View reviewed changes

hathach changed the base branch from master to enhance-itf-assoc April 15, 2020 17:19

hathach changed the base branch from enhance-itf-assoc to master April 15, 2020 17:19

hathach mentioned this pull request Apr 15, 2020

Enhance Interface Associate descriptor handling #344

Merged

majbthrd mentioned this pull request Apr 15, 2020

add provision for alternate interfaces #317

Closed

pigrew requested changes Apr 15, 2020

View reviewed changes

src/class/net/net_device.c Show resolved Hide resolved

src/class/net/net_device.c Outdated Show resolved Hide resolved

src/class/net/net_device.c Outdated Show resolved Hide resolved

src/class/net/net_device.c Show resolved Hide resolved

src/device/usbd.c Outdated Show resolved Hide resolved

per review

2eed58d

hathach requested a review from pigrew April 16, 2020 04:59

pigrew approved these changes Apr 16, 2020

View reviewed changes

hathach merged commit bfec3b4 into master Apr 16, 2020

hathach deleted the add-alt-itf branch April 16, 2020 14:33

majbthrd mentioned this pull request Oct 10, 2020

check usbnet example with all ports #289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement setInterface(alt) for usb net driver #345

Implement setInterface(alt) for usb net driver #345

hathach commented Apr 15, 2020

pigrew commented Apr 15, 2020 •

edited

majbthrd Apr 15, 2020

hathach Apr 15, 2020

hathach commented Apr 15, 2020

majbthrd left a comment

hathach commented Apr 15, 2020 •

edited

majbthrd commented Apr 15, 2020

hathach commented Apr 15, 2020

pigrew commented Apr 15, 2020

hathach commented Apr 15, 2020

hathach commented Apr 15, 2020

pigrew left a comment

pigrew left a comment

pigrew Apr 16, 2020

hathach Apr 16, 2020 •

edited

Implement setInterface(alt) for usb net driver #345

Implement setInterface(alt) for usb net driver #345

Conversation

hathach commented Apr 15, 2020

pigrew commented Apr 15, 2020 • edited

majbthrd Apr 15, 2020

Choose a reason for hiding this comment

hathach Apr 15, 2020

Choose a reason for hiding this comment

hathach commented Apr 15, 2020

majbthrd left a comment

Choose a reason for hiding this comment

hathach commented Apr 15, 2020 • edited

majbthrd commented Apr 15, 2020

hathach commented Apr 15, 2020

pigrew commented Apr 15, 2020

hathach commented Apr 15, 2020

hathach commented Apr 15, 2020

pigrew left a comment

Choose a reason for hiding this comment

pigrew left a comment

Choose a reason for hiding this comment

pigrew Apr 16, 2020

Choose a reason for hiding this comment

hathach Apr 16, 2020 • edited

Choose a reason for hiding this comment

pigrew commented Apr 15, 2020 •

edited

hathach commented Apr 15, 2020 •

edited

hathach Apr 16, 2020 •

edited