Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp_tinyusb v1.4.4: Adding FS configuration descriptor for HS device #11

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

roma-jam
Copy link
Collaborator

@roma-jam roma-jam commented Feb 6, 2024

esp_tinyusb v1.4.4 (Release)

General description

  • USB Device now able to work with both Hosts: High Speed (HS) or Full Speed (FS)

Changes

  • Moved all the descriptor preparation logic inside descriptor_control.c file, tinyusb_set_descriptors(..). This function is responsible for descriptors preparation based on the tinyusb configuration, provided during tinyusb driver install.
  • Renamed default descriptors from *_kconfig to *_default.
  • Added descriptor_hs_cfg_default[] for HighSpeed device and descriptor_hs_cfg_default[] for FullSpeed device.
  • Added .hs_cfg_desc field to tinyusb configuration (only when TUD_OPT_HIGH_SPEED is set)
  • Added .qualifier_desc field to tinyusb configuration (only when TUD_OPT_HIGH_SPEED is set)

Details

Configuration descriptor definitions:

  1. FS only
    1.1 Configuration descriptor NOT present (.configuration_descriptor = NULL). Default FS compatible configuration descriptor is using.
    1.2 Configuration descriptor PRESENT (.configuration_descriptor != NULL). Provided descriptor is using.
  2. HS/FS
    2.1 Configuration descriptor NOT present (.configuration_descriptor = NULL). Default FS compatible configuration descriptor is using.
    2.2 HS configuration descriptor NOT present (.hs_cfg_desc = NULL). Default HS compatible configuration descriptor is using.
    2.3 Configuration descriptor PRESENT (.configuration_descriptor != NULL). Provided FS descriptor is using.
    2.4 HS configuration descriptor PRESENT (.hs_cfg_desc != NULL). Provided HS descriptor is using.

IMPORTANT: In any cases for HS/FS, configuration descriptors must be equal size.

Hints

  • For HS/FS field .fs_cfg_desc might be used instead of .configuration_descriptor
  • In HS all combinations are possible (2.1/2.2, 2.1/2.4, 2.3/2.2, 2.3/2.4), but HS and FS configuration descriptors must be the same size.
  • Device qualifier descriptor must be present if device descriptor is not default. Otherwise, default qualifier descriptor is using.

@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch from 73788f7 to 681ddf1 Compare February 6, 2024 12:47
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_bvalid_signal_test branch 2 times, most recently from 91da76e to 5dede2c Compare February 6, 2024 15:55
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 77cac3e to 672da25 Compare February 6, 2024 18:54
@roma-jam roma-jam changed the title esp_tinyusb: Adding HS/FS configuration descriptor definition possibility esp_tinyusb v1.4.4: Adding HS/FS configuration descriptor definition possibility Feb 6, 2024
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 4 times, most recently from 55b1332 to e379750 Compare February 7, 2024 10:13
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_bvalid_signal_test branch 2 times, most recently from 1325870 to d0b5b2e Compare February 13, 2024 08:23
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 7416511 to 4168b9e Compare February 13, 2024 08:38
Base automatically changed from feature/esp_tinyusb_bvalid_signal_test to master February 13, 2024 08:40
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 281383a to 2e21557 Compare February 13, 2024 09:11
@roma-jam roma-jam changed the title esp_tinyusb v1.4.4: Adding HS/FS configuration descriptor definition possibility esp_tinyusb v1.4.4: Adding FS configuration descriptor for HS device Feb 14, 2024
@roma-jam roma-jam marked this pull request as ready for review February 14, 2024 14:15
@roma-jam roma-jam self-assigned this Feb 14, 2024
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

@roma-jam Thanks for the updates.

My biggest concern is about memory allocation for the descriptors. Previously, if user did not provide his own descriptors, all Kconfig generated descs were statically allocated in esp_tinyusb.
If user provided his descs, he was responsible for making sure that their lifetime is the same as the lifetime of the USB device (usually made static).

This is surely not the best solutions, but it is consistent across the whole esp_tinyusb component and also consistent with TinyUSB upstream examples.

device/esp_tinyusb/CHANGELOG.md Outdated Show resolved Hide resolved
device/esp_tinyusb/include/tinyusb.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include_private/descriptors_control.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include_private/descriptors_control.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include_private/descriptors_control.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include_private/descriptors_control.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include/tinyusb.h Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
@roma-jam
Copy link
Collaborator Author

@tore-espressif Thanks for the review!
Will follow-up with all suggestions.

@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 80d5db2 to b0945a4 Compare February 15, 2024 18:34
@tore-espressif
Copy link
Collaborator

tore-espressif commented Feb 16, 2024

@roma-jam Do you think we could increase default debug level to 1 = error in this release?
https://github.com/espressif/esp-usb/blob/master/device/esp_tinyusb/Kconfig#L4

// CFG_TUSB_DEBUG for debugging
// 0 : no debug
// 1 : print error
// 2 : print warning
// 3 : print info

cc @peter-marcisovsky

@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from caa144a to a23f959 Compare February 16, 2024 13:23
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Thanks @roma-jam , looks better now!

device/esp_tinyusb/test/local/CMakeLists.txt Show resolved Hide resolved
device/esp_tinyusb/include_private/descriptors_control.h Outdated Show resolved Hide resolved
device/esp_tinyusb/include/tinyusb.h Outdated Show resolved Hide resolved
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 76e84c6 to 024da71 Compare March 1, 2024 13:03
device/esp_tinyusb/test_app/main/test_hot_disconnect.c Outdated Show resolved Hide resolved
device/esp_tinyusb/include/tinyusb.h Outdated Show resolved Hide resolved
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from a8e14d1 to 7a6e7a3 Compare March 5, 2024 08:58
@roma-jam roma-jam requested a review from tore-espressif March 5, 2024 09:34
Copy link
Collaborator

@tore-espressif tore-espressif 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 @roma-jam LGTM.

@Dazza0 PTAL too

@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch from 7a6e7a3 to 0265367 Compare March 6, 2024 11:39
Copy link

@Dazza0 Dazza0 left a comment

Choose a reason for hiding this comment

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

@roma-jam Left a bunch of nitpicks, but changes LGTM overall

device/esp_tinyusb/CHANGELOG.md Show resolved Hide resolved
device/esp_tinyusb/test_app/main/CMakeLists.txt Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/descriptors_control.c Outdated Show resolved Hide resolved
device/esp_tinyusb/CHANGELOG.md Show resolved Hide resolved
device/esp_tinyusb/include_private/usb_descriptors.h Outdated Show resolved Hide resolved
device/esp_tinyusb/usb_descriptors.c Outdated Show resolved Hide resolved
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 2 times, most recently from 9d3df2d to 3c51edf Compare March 9, 2024 12:12
@roma-jam
Copy link
Collaborator Author

roma-jam commented Mar 11, 2024

USB example verification list (ESP-IDF v5.3-dev-2495-g0bbee51829-dirty, esp32p4)

  • tusb_composite_msc_serialdevice
  • tusb_console
  • tusb_hid (HS configuration descriptor must be provided by user for HID, requires example update)
  • tusb_midi (HS configuration descriptor must be provided by user for MIDI, requires example update)
  • tusb_msc (HS and Qualifier descriptors must be present (Device Descriptor not default), requires example update)
  • tusb_ncm (Unable to build, because there is no WiFi support in esp32p4, exclude from esp32p4 support list?)
  • tusb_serial_device

@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch 6 times, most recently from 0a53965 to 40a009d Compare March 15, 2024 10:22
Added union in tinyusb configuration for configuration descriptor pointer
Moved selecting user-defined or default descriptor based on tinyusb configuration to descriptors_control
Changed default Kconfig level to 1
@roma-jam roma-jam force-pushed the feature/esp_tinyusb_hs_fs_configurations branch from 40a009d to 8ea55b6 Compare March 15, 2024 10:25
@roma-jam roma-jam merged commit 4b6a798 into master Mar 15, 2024
12 checks passed
@roma-jam roma-jam deleted the feature/esp_tinyusb_hs_fs_configurations branch December 12, 2024 11:49
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.

3 participants