espressif / idf-extra-components

Additional components for ESP-IDF, maintained by Espressif
147 stars 89 forks source link

usb: add ECM device class #133

Closed Jacques-Zhao closed 1 year ago

Jacques-Zhao commented 1 year ago

Added support for USB NCM class. This MR relies on a tinyusb MR. The USB NCM example will be placed in the examples/peripherals/usb/device/tusb_ncm directory under esp-idf repository.

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

roma-jam commented 1 year ago

@Jacques-zhao Hi, well done and you have done a great job!

The one thing I would like to ask. If there is an additional string descriptor logic has been added, probably it is better to do that with a specific function. For example, "tusb_set_extra_str_descritpor" or "tusb_set_str_descriptor", which should have been added as an independent function from "tusb_set_descriptor".

That can help us to achieve following things:

What do you think?

Anyway, thanks for the changes. Probably, @tore-espressif can suggest more things here.

Always ready to a discuss, so feel free to do that. RJ

tore-espressif commented 1 year ago

@roma-jam @Jacques-zhao I started refactoring the string descriptors handling too :)

I'll review this (or push my code for comparison) soon

tore-espressif commented 1 year ago

Hi @Jacques-zhao @roma-jam

I wanted to refactor ithe interanl string descriptor handling for a while so now it is ready in https://github.com/espressif/idf-extra-components/pull/134 PTAL

Looking at this PR, I have a couple of questions:

  1. For some classes (MSC, CDC) we provide tinyusb 'additions', that help users with integrating different ESP APIs. For some classes we don't provide the additions (MIDI, HID). IMO, for network class we should provide the addition, that would integrate our existing esp_netif with TinyUSB-NET, would do you think?
  2. In case you plan to add the NET addition, it might be better to merge it to esp-idf first, in form of an example. This way it gets properly reviewed and we can make some more changes, before we merge it to esp_tinyusb as stable
Jacques-Zhao commented 1 year ago

I will resubmit a new version based on Tomas PR(https://github.com/espressif/idf-extra-components/pull/134)