Add USBCDC_ECM class #9443

juhaylinen · 2019-01-21T13:09:43Z

Description

Add initial Ethernet over USB communication support using
the ECM (Ethernet Control Model) subclass of USB CDC.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@c1728p9

ciarmcom · 2019-01-21T14:00:32Z

@juhaylinen, thank you for your changes.
@c1728p9 @ARMmbed/mbed-os-maintainers please review.

juhaylinen · 2019-01-22T07:12:32Z

FYI @mikter

c1728p9

If read and write are implemented can linux then use this class to provide an internet connection over USB?

c1728p9 · 2019-01-23T17:27:07Z

usb/device/USBCDC_ECM/USBCDC_ECM.cpp

+
+    _bulk_buf_size = read_finish(_bulk_out);
+
+    read_start(_bulk_out, _bulk_buf, MAX_PACKET_SIZE_BULK);


Without being able to receive data the functionality of this device is quite limited.

c1728p9 · 2019-01-23T17:29:25Z

usb/device/USBCDC_ECM/USBCDC_ECM.cpp

+    if (setup->bmRequestType.Type == CLASS_TYPE) {
+        //printf("In USBCallback_request: CLASS specific Request: %02x\n", setup->bRequest);
+        switch (setup->bRequest) {
+            case SET_ETHERNET_MULTICAST_FILTERS:


Should these be forwarded to the user of this call as a callback? Alternatively, if this class took in a an EMAC phy then you could call the appropriate functions. @kjbracey-arm what are your thoughts on this?

c1728p9 · 2019-01-23T17:33:45Z

usb/device/USBCDC_ECM/USBCDC_ECM.h

+    void _bulk_in_callback();
+    void _bulk_out_callback();
+    bool _notify_network_connection(uint8_t value);
+    bool _notify_connection_speed_change(uint32_t up, uint32_t down);


_notify_network_connection and _notify_connection_speed_change are private and I don't see them being used anywhere in this class. Is this dead code?

c1728p9 · 2019-01-23T17:37:39Z

usb/device/USBCDC_ECM/USBCDC_ECM.cpp

+
+const uint8_t *USBCDC_ECM::string_iconfiguration_desc()
+{
+    static uint8_t string_imac_addr[26] = {0};


This array should probably be a member of the class rather than static.

c1728p9 · 2019-01-23T17:44:14Z

usb/device/USBCDC_ECM/USBCDC_ECM.cpp

+        return NULL;
+    }
+
+    static const uint8_t configDescriptor[] = {


This array should probably be a member of the class rather than static. Also the local static initialization will cause mutexes to be used in interrupt context.

cmonr · 2019-01-28T17:11:22Z

@c1728p9 Mind taking another look? New commit looks like it resolved old comments.

juhaylinen · 2019-02-05T14:24:21Z

@c1728p9 I have addressed the changes requested. Please review.

cmonr · 2019-02-14T15:21:08Z

@c1728p9 Please rereview.

c1728p9 · 2019-02-20T20:20:08Z

Hi @juhaylinen, to progress this further you'll need to create documentation on how to use this containing an API reference along with one or more examples on how to use this. For reference the documentation for all the existing USB classes can be found at https://github.com/ARMmbed/mbed-os-5-docs/tree/development/docs/api/usb.

Also from our email conversations it sounds like this only works on Linux. Is that correct? To be accepted as part of mbed-os this needs to work on Windows, Linux and OSX.

Once these two points are addressed I'll mark this as approved.

CC @cmonr

juhaylinen · 2019-02-21T14:24:48Z

Linux and OSX have native support for USBCDC_ECM. Windows seem to require 3rd party driver (Belcarra, Thesycon, etc.) for the CDC-ECM support. Is this OK?

c1728p9 · 2019-02-21T15:17:30Z

Hi @juhaylinen, if you provide instructions in the documentation on how to setup and use USBCDC_ECM on Windows with freely available 3rd party software it will be OK.

juhaylinen · 2019-02-22T08:53:58Z

@c1728p9 Any change getting this in 5.12 since only documentation is needed?

c1728p9 · 2019-02-22T17:06:09Z

Hi @juhaylinen I don't feel comfortable merging this without at least some preliminary docs/example. Without those I don't know how to test this on Windows. I'm also having a hard time seeing how users could easily make use of this in its current form, which docs would help to clarify.

cmonr · 2019-02-22T17:07:03Z

I also share @c1728p9 concern about lack of documentation.

cmonr · 2019-02-28T01:05:12Z

@c1728p9 Should this be retargeted to master since the feature branch was merged?

c1728p9 · 2019-02-28T02:22:38Z

@cmonr yeah, this PR should be against master now.

cmonr · 2019-02-28T02:45:30Z

@juhaylinen Please rebase against master when possible.

Add initial Ethernet over USB communication support using the ECM (Ethernet Control Model) subclass of USB CDC.

Replace static arrays used for iMAC and config descriptor with class members Minor coding style fixes

Fix build issues after rebase

Handle SET_ETHERNET_PACKET_FILTER request

adbridge · 2019-03-27T14:26:17Z

@juhaylinen is this still waiting for docs and examples ? As per @c1728p9 and @cmonr comments ?

cmonr · 2019-03-27T20:40:33Z

LGTM, but this still needs docs, and replies to @c1728p9 comments.
Not sure an example is required, but would be nice to have.

juhaylinen · 2019-03-28T12:20:46Z

@adbridge Docs and examples are still missing. I have been busy with more urgent tasks and I'm not sure when I can continue with this.
Adding Windows support is a bit tricky since I was not able to find free driver for USB ECM. Belcarra offers demo version of the commercial driver https://usblan.belcarra.com/2015/12/usblan-for-windows-10.html but it is not part of the company's Windows update. The demo version can be installed manually but I was not able to capture Ethernet packets from USB device. Maybe firewall or antivirus SW is blocking the traffic.

mikter · 2019-03-28T13:59:03Z

We will continue this work when our plans go forward. remaining part of work is planned to next release.

For widows support it is quite clear it does not support it and we should not do anything more to add support for windows for this feature. perhaps link in documentation to places where to get windows support could be added.
This is a valid feature for platforms that do support it (macOS and Linux)

c1728p9 · 2019-03-28T22:39:21Z

For widows support it is quite clear it does not support it and we should not do anything more to add support for windows for this feature.

Windows support is an important aspect of this, as the a large number of mbed users are on Windows. It may be more appropriate to push USBCDC_ECM into its own repository rather than merge it into mbed-os. That way it doesn't set the expectation this has the support/compatibility of the existing USB classes. @bulislaw what do you think?

mikter · 2019-03-29T06:58:18Z

@c1728p9 I don't understand your approach Windows does not support it we can't add support for windows as what ever we do it does not support it. Windows supports proprietary RNDIS protocol which this is not. Here is a link for different protocols https://usblan.belcarra.com/p/protocols.html

Windows support can be added by external companies and you can find driver for windows that will support ECM. https://usblan.belcarra.com/2015/12/usblan-for-windows-10.html

@paspik @AnttiKauppila any idea how to continue this as requirement as we can't make all the different USB ethernet protocols

bulislaw · 2019-04-04T14:44:30Z

I don't like the idea of not adding support for all 3 platforms. If there's a way of making it work on Windows, external drivers, we should be exploring it. If there are issues with our corporate setup we need to either figure out why is that (and work with IT to remove it) or request non-IT managed windows (VM?) to complete setup and testing.

Also we'll need the requested docs before bringing it to master.

0xc0170 · 2019-05-13T13:04:26Z

@juhaylinen Besides one platform lacking, can we at least get the rest completed? @c1728p9 will soon move from USB, it would be nice to have this completed (even if parked on feature branch). Please talk to him this week.

TuomoHautamaki · 2019-05-16T09:28:07Z

we at least get the rest completed

No more work has been planned for this. What do you think is missing if we exclude windows support?

TuomoHautamaki · 2019-05-16T09:28:34Z

@bulislaw do you have an update to the windows support topic?

bulislaw

Good arguments were raised during offline discussions. Specifically:

Feature branch maintenance is expensive
By pushing it to master we will add value
Mbed OS promise is around development on 3 OSs, not feature support
Feature can be used on OSs with standard ECM support, proprietary drivers can be explored when Windows support will be required

adbridge · 2019-05-17T08:42:08Z

CI started

mbed-ci · 2019-05-17T12:25:58Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

0xc0170 · 2019-05-20T09:29:49Z

Docs PRs are there, I reviewed them. This should be ready for integration now.

juhaylinen force-pushed the usbcdc_ecm branch from af98aa8 to 84b6c2c Compare January 21, 2019 13:32

ciarmcom requested review from c1728p9 and a team January 21, 2019 14:00

ciarmcom added the needs: review label Jan 21, 2019

c1728p9 suggested changes Jan 23, 2019

View reviewed changes

0xc0170 added the release-version: feature branch label Jan 29, 2019

c1728p9 force-pushed the feature-hal-spec-usb-device branch from 091b0e7 to e23e57d Compare January 29, 2019 18:42

cmonr added needs: preceding PR needs: work and removed needs: review labels Jan 29, 2019

juhaylinen force-pushed the usbcdc_ecm branch from e583636 to 1c11290 Compare February 4, 2019 13:48

juhaylinen mentioned this pull request Feb 20, 2019

Bring USB Feature branch into master #9768

Merged

cmonr removed the needs: preceding PR label Feb 28, 2019

cmonr changed the base branch from feature-hal-spec-usb-device to master February 28, 2019 02:45

cmonr added release-version: 5.13.0-rc1 and removed release-version: feature branch labels Feb 28, 2019

juhaylinen added 5 commits March 13, 2019 12:17

Add USBCDC_ECM class

4e4653b

Add initial Ethernet over USB communication support using the ECM (Ethernet Control Model) subclass of USB CDC.

Fix USBCDC_ECM descriptors

e7416db

Replace static arrays used for iMAC and config descriptor with class members Minor coding style fixes

USBCDC_ECM: send connection notifications to the host

fc00718

Fix build issues after rebase

USBCDC_ECM: Add receive functionality

87f2f7f

Handle SET_ETHERNET_PACKET_FILTER request

Fix mutex issue

4b71459

juhaylinen force-pushed the usbcdc_ecm branch from 4f1c1a7 to 4b71459 Compare March 13, 2019 10:23

cmonr approved these changes Mar 27, 2019

View reviewed changes

juhaylinen mentioned this pull request Apr 16, 2019

Add API reference and example for USBCDC_ECM class ARMmbed/mbed-os-5-docs#1044

Merged

bulislaw approved these changes May 16, 2019

View reviewed changes

adbridge added needs: CI and removed needs: work labels May 17, 2019

0xc0170 added ready for merge and removed needs: CI labels May 20, 2019

0xc0170 merged commit d299b3f into ARMmbed:master May 20, 2019

0xc0170 removed the ready for merge label May 20, 2019


		_bulk_buf_size = read_finish(_bulk_out);

		read_start(_bulk_out, _bulk_buf, MAX_PACKET_SIZE_BULK);

Add USBCDC_ECM class #9443

Add USBCDC_ECM class #9443

Uh oh!

Conversation

juhaylinen commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jan 21, 2019

Uh oh!

juhaylinen commented Jan 22, 2019

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

c1728p9 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

c1728p9 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

c1728p9 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

c1728p9 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

c1728p9 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jan 28, 2019

Uh oh!

juhaylinen commented Feb 5, 2019

Uh oh!

cmonr commented Feb 14, 2019

Uh oh!

c1728p9 commented Feb 20, 2019

Uh oh!

juhaylinen commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 commented Feb 21, 2019

Uh oh!

juhaylinen commented Feb 22, 2019

Uh oh!

c1728p9 commented Feb 22, 2019

Uh oh!

cmonr commented Feb 22, 2019

Uh oh!

cmonr commented Feb 28, 2019

Uh oh!

c1728p9 commented Feb 28, 2019

Uh oh!

cmonr commented Feb 28, 2019

Uh oh!

adbridge commented Mar 27, 2019

Uh oh!

cmonr commented Mar 27, 2019

Uh oh!

juhaylinen commented Mar 28, 2019

Uh oh!

mikter commented Mar 28, 2019

Uh oh!

c1728p9 commented Mar 28, 2019

Uh oh!

mikter commented Mar 29, 2019

Uh oh!

bulislaw commented Apr 4, 2019

Uh oh!

0xc0170 commented May 13, 2019

Uh oh!

TuomoHautamaki commented May 16, 2019

Uh oh!

TuomoHautamaki commented May 16, 2019

Uh oh!

bulislaw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

juhaylinen commented Jan 21, 2019 •

edited

Loading

juhaylinen commented Feb 21, 2019 •

edited

Loading

bulislaw left a comment •

edited

Loading

0xc0170 commented May 20, 2019 •

edited

Loading