feature(esp_tinyusb): Added tusb_teardown() call while tinyusb_driver_uninstall() #39

roma-jam · 2024-06-11T11:36:27Z

esp_tinyusb (Not a Release)

Requirements

To uninstall tinyusb driver, a teardown tinyusb stack should be called. (feature(usbd): Added tud_teardown() tinyusb#27)

Description

Added tusb_teardown() call while tinyusb_driver_uninstall()
Added test case to verify reconfiguration procedure

Changes

Added tusb_teardown()
Added test case device_class_reconfiguration which:

Configure tinyusb driver with HID class
Install tinyusb driver
Wait connection event (which means the enumeration completed successufully)
Uninstall tinyusb driver
Repeat steps 1-4 for next classes [CDC, MSC]

Notes

Test case only covers the enumeration and configuration for a class. There is no communication in between tinyusb driver tinyusb_driver_install() and tinyusb_driver_uninstall() calls.

Resolve espressif/esp-idf#13788

device/esp_tinyusb/tinyusb.c

roma-jam · 2024-06-11T11:39:39Z

device/esp_tinyusb/tinyusb.c

@@ -69,5 +69,6 @@ esp_err_t tinyusb_driver_install(const tinyusb_config_t *config)
 esp_err_t tinyusb_driver_uninstall()
 {
 tinyusb_free_descriptors();
+ tusb_teardown();


@tore-espressif ,

when you will have time

Is there any good way how I can make this change backward compatible with previous espressif/tinyusb components (previous versions, which doesn't have this implementation)?
Will appreciate any suggestions.

Here are some ideas, that came to me:

Tie it to the configuration and use idf version to determine the necessity and implementation of tusb_teardown()

Define it somewhere in esp_tinyusb with WEAK attribute. Which let us redefine in with new version and don't panic with old versions, when there is no implementation of it

Enable it with menuconfig (which seems really strange for me, but anyway maybe it is possible).

I like option No.2, but maybe there are other simple and better solution.

roma-jam · 2024-06-11T11:43:15Z

device/esp_tinyusb/test_app/main/test_descriptors_config.c

+};
+
+
+TEST_CASE("device_class_reconfiguration", "[usb_device]")


@peter-marcisovsky

After adding the feature from here: espressif/tinyusb#27, we will be able to run esp_tinyusb device tests, because they could be run one by one in a one test launch.

Currently, I have no idea how to add them to our CI and where, but if you have some ideas, please feel free to share.
Thanks.

To add usb device test, you just need to create a new pytest marker in your pytest*.py file, like so:

@pytest.mark.usb_device

Then, pytest command being run from build_and_run_test_app_usb.yml file, must include this new marker.
Right now, we are using only -m usb_host marker. Thus there will be one more marker -m usb_device
So the new pytest command would look like:
pytest --target=${{ matrix.idf_target }} -m usb_host -m usb_device --build-dir=build_${{ matrix.idf_target }}

This should be all.

roma-jam · 2024-06-11T11:50:07Z

@tore-espressif , @peter-marcisovsky
This changes unlock the possibility to run usb device tests one by one (espressif/tinyusb#27 should be merged first, though).

I posted two questions for both of you, because it seems that we can make it better and we can use it.

I need that for enumeration driver testing (but maybe I will eliminate esp_tinyusb component from the chain, but anyway), so feel free to check it out when possible.

Meanwhile, I will go and cover the Enum Driver as much as I can.

peter-marcisovsky

Left some observations from using the test app

peter-marcisovsky · 2024-06-13T11:58:00Z

device/esp_tinyusb/test_app/sdkconfig.defaults

 CONFIG_TINYUSB_CDC_ENABLED=y
 CONFIG_TINYUSB_CDC_COUNT=2
-CONFIG_TINYUSB_HID_COUNT=0
+CONFIG_TINYUSB_HID_COUNT=1


After enabling HID in the menuconfig, the
tinyusb_set_descriptors() would fail
because of
// Default configuration descriptor is provided only for CDC, MSC and NCM classes
ESP_GOTO_ON_FALSE(config->configuration_descriptor, ESP_ERR_INVALID_ARG, fail, TAG, "Configuration descriptor must be provided for this device");

Thus making all the descriptors test cases failing.

peter-marcisovsky · 2024-06-13T13:00:37Z

device/esp_tinyusb/tinyusb.c

@@ -66,8 +78,18 @@ esp_err_t tinyusb_driver_install(const tinyusb_config_t *config)
 return ESP_OK;
 }

-esp_err_t tinyusb_driver_uninstall()
+esp_err_t tinyusb_driver_uninstall(void)


Could we call this function from tearDown()?
Once a test case fails, the PHY does not get released, thus all the following test cases would fail because of the:

E (23549) usb_phy: usb_new_phy(296): selected PHY is in use E (23559) TinyUSB: tinyusb_driver_install(65): Install USB PHY failed

leeebo · 2024-06-21T06:59:14Z

device/esp_tinyusb/tinyusb.c

+ if (!tusb_teardown()) {
+ return ESP_ERR_NOT_FINISHED;
+ }
+


@roma-jam Should we restore the USB to the default function (eg. USB-Serial-JTAG) at the end of uninstall?

@leeebo I don't think that would be a good idea, as for user it is transparent in that case.
Even, IDK, whether we have such feature before installing tinyUSB driver?

Just in case: do we have such call (maybe in rom) to enable this feature? Or you thinking about configure the PHY during teardown in the way, to enable USB-Serial-JTAG?

The USB-Serial-JTAG is a hardware circuit that can respond to the host's requests and is enabled by default. Users can use the USB-Serial-JTAG for USB download without any code in APP.

But after tinyUSB install then uninstalled, the USB phy not be switched back to USB-Serial-JTAG

@roma-jam @leeebo as a user of the library in esp32s3 I think teardown/uninstall should not auto-reinstall jtag but there should be a convenience function that allows to restore it, if necessary it could be called automatically and be driven by Kconfig to provide best defaults. In my use case i want to be able to switch from usb cdc guest to msc usb host and viceversa and that is currently not possible without a device restart

finger563 · 2024-09-23T14:27:29Z

Is there a timeline for this feature to be finalized?

roma-jam · 2024-09-25T08:45:20Z

Hi @finger563 ,

This feature allows the re-configure the driver with these changes: espressif/tinyusb#27
But as far as the same changes are already in the upstream, so we will rebase these changes after updating version to v0.17.0.
For current feature there is no available timeline for now, but the one thing for sure: the update to v0.17.0 will be done first.

Meanwhile, this feature should be available in the upstream of TinyUSB, if anything it is possible to use it.

Sorry for the inconvenience.

roma-jam self-assigned this Jun 11, 2024

github-advanced-security bot found potential problems Jun 11, 2024

View reviewed changes

device/esp_tinyusb/tinyusb.c Fixed Show fixed Hide fixed

roma-jam commented Jun 11, 2024

View reviewed changes

roma-jam force-pushed the feature/esp_tinyusb_stack_teardown branch 3 times, most recently from 8b60d62 to ce33baf Compare June 11, 2024 12:45

ref(esp_tinyusb): Added tusb_teardown() call

9fbdf0d

roma-jam force-pushed the feature/esp_tinyusb_stack_teardown branch from ce33baf to d7cb17d Compare June 11, 2024 12:48

feature(esp_tinyusb): Added test for usb device class reconfiguration

c14eebb

roma-jam force-pushed the feature/esp_tinyusb_stack_teardown branch from d7cb17d to c14eebb Compare June 11, 2024 12:57

roma-jam marked this pull request as ready for review June 11, 2024 13:14

roma-jam requested review from peter-marcisovsky, tore-espressif, Dazza0 and leeebo June 11, 2024 13:14

peter-marcisovsky reviewed Jun 13, 2024

View reviewed changes

leeebo reviewed Jun 21, 2024

View reviewed changes

ref(esp_tinyusb): Updated CHANGELOG, returned pytest file

87ddae1

peter-marcisovsky mentioned this pull request Oct 15, 2024

ci(esp_tinyusb): Run esp_tinyusb test apps in CI #75

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(esp_tinyusb): Added tusb_teardown() call while tinyusb_driver_uninstall() #39

feature(esp_tinyusb): Added tusb_teardown() call while tinyusb_driver_uninstall() #39

roma-jam commented Jun 11, 2024

roma-jam Jun 11, 2024

roma-jam Jun 11, 2024 •

edited

Loading

roma-jam Jun 11, 2024

peter-marcisovsky Jun 11, 2024 •

edited

Loading

roma-jam commented Jun 11, 2024 •

edited

Loading

peter-marcisovsky left a comment

peter-marcisovsky Jun 13, 2024

peter-marcisovsky Jun 13, 2024

leeebo Jun 21, 2024 •

edited

Loading

roma-jam Jun 21, 2024

leeebo Jun 24, 2024

greenaddress Sep 17, 2024

finger563 commented Sep 23, 2024

roma-jam commented Sep 25, 2024

		};


		TEST_CASE("device_class_reconfiguration", "[usb_device]")

feature(esp_tinyusb): Added tusb_teardown() call while tinyusb_driver_uninstall() #39

Are you sure you want to change the base?

feature(esp_tinyusb): Added tusb_teardown() call while tinyusb_driver_uninstall() #39

Conversation

roma-jam commented Jun 11, 2024

esp_tinyusb (Not a Release)

Requirements

Description

Changes

Notes

roma-jam Jun 11, 2024

Choose a reason for hiding this comment

roma-jam Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

roma-jam Jun 11, 2024

Choose a reason for hiding this comment

peter-marcisovsky Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

roma-jam commented Jun 11, 2024 • edited Loading

peter-marcisovsky left a comment

Choose a reason for hiding this comment

peter-marcisovsky Jun 13, 2024

Choose a reason for hiding this comment

peter-marcisovsky Jun 13, 2024

Choose a reason for hiding this comment

leeebo Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

roma-jam Jun 21, 2024

Choose a reason for hiding this comment

leeebo Jun 24, 2024

Choose a reason for hiding this comment

greenaddress Sep 17, 2024

Choose a reason for hiding this comment

finger563 commented Sep 23, 2024

roma-jam commented Sep 25, 2024

roma-jam Jun 11, 2024 •

edited

Loading

peter-marcisovsky Jun 11, 2024 •

edited

Loading

roma-jam commented Jun 11, 2024 •

edited

Loading

leeebo Jun 21, 2024 •

edited

Loading