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

feat: add usb host uac driver and examples (IEC-99) #27

Merged
merged 6 commits into from
May 23, 2024

Conversation

leeebo
Copy link
Collaborator

@leeebo leeebo commented Mar 22, 2024

The PR aims to add UAC Host Driver (Based on ESP-IDF USB Host Driver), featuring:

  • Compatibility with UAC 1.0 specification
  • Independent IN/OUT streaming control
  • Independent Volume/Mute control
  • Multiple devices/instances for future HUB support

Checklist:

  • (No mock test, can only be test locally) Speaker and Mic works as normal
    - [x] example: Play MP3 as normal I have removed the examples to esp-iot-solution/examples, reducing the work of review here.
    - [ ] example: Record voice to SDcard

  • thread-safe protect (ringbuffer safe delete, critical section protect)

  • refactor open/close device as @roma-jam suggest

  • Support Parse USB Audio device with MIXER_UNIT see

uac_host_refactor2

@leeebo
Copy link
Collaborator Author

leeebo commented Mar 22, 2024

@tore-espressif @roma-jam @Dazza0 PTAL!

@github-actions github-actions bot changed the title feat: add usb host uac driver and examples feat: add usb host uac driver and examples (IEC-99) Mar 22, 2024
@roma-jam
Copy link
Collaborator

Hey @leeebo,
Wow, a lot of code, great job!
I briefly checked the logic, and it seems that it is the same as we have right now in hid_driver v1.0.2 (correct me if I am wrong).

The main problem there - is that if device has at least one interface, driver opens the device with usb_host_device_open() and hold the device opened, even if the interface is not claimed (opened) by user. This happened during the client_event_cb which we should fulfill as fast as we can.

In that case, it is better to "touch" the new device, checking the uac interface presence and add it to the list.
I made the changes in hid driver here (but they are not ready to be merge yet): espressif/idf-extra-components#214

But I explained the idea in espressif/idf-extra-components#214 (comment)

Please, read the document and share your opinion.

@leeebo
Copy link
Collaborator Author

leeebo commented Mar 23, 2024

@roma-jam Yes, you are right, the device handling logic and data structure is same as HID v1.0.2. Thanks for yor advice, I will check it.

@peter-marcisovsky
Copy link
Collaborator

@leeebo regarding the test app:

I took a look at the failing CI logs, could you please try renaming the pytest_usb_host_hid.py to pytest_usb_host_uac.py in your test_app folder? So we will have passing CI for this PR.

@leeebo
Copy link
Collaborator Author

leeebo commented Mar 26, 2024

@leeebo regarding the test app:

I took a look at the failing CI logs, could you please try renaming the pytest_usb_host_hid.py to pytest_usb_host_uac.py in your test_app folder? So we will have passing CI for this PR.

Hi @peter-marcisovsky , as we currently have no UAC 1.0 device for the mock test (tinyusb supports UAC2.0 only) , I just test it using my local UAC device, any advice for the test?

@peter-marcisovsky
Copy link
Collaborator

@leeebo sorry I was not aware that the tinyusb supports only UAC2.0.

In this case, please enable at least build in CI by adding a path to your new component to .idf_build_apps.toml file in the root dir.

@leeebo leeebo changed the title feat: add usb host uac driver and examples (IEC-99) (Draft)feat: add usb host uac driver and examples (IEC-99) Mar 26, 2024
@leeebo leeebo force-pushed the feat/usb_host_uac branch 2 times, most recently from 8bdcfcd to 94cf090 Compare March 28, 2024 12:35
@leeebo leeebo changed the title (Draft)feat: add usb host uac driver and examples (IEC-99) feat: add usb host uac driver and examples (IEC-99) Mar 31, 2024
@leeebo leeebo force-pushed the feat/usb_host_uac branch 4 times, most recently from d196ffc to 86d46de Compare April 1, 2024 11:35
@leeebo
Copy link
Collaborator Author

leeebo commented Apr 1, 2024

Another topic is whether we can adjust ep's MPS before claiming the interface, which, although not standard, makes it easier to provide workaround when the FIFO size is limited.

In this Headset case, the speaker supports sampling rate 96KHz 16bit 2ch at maximum, so the OUT MPS is 96*2*2 = 384. It is used when the user selects 96KHz, and will not be touched in most time. It is sufficient to declare an interface with MPS=192~193 when selecting 48KHz.

1713428200488_d

Full descriptor
honor_headset_pcapng.zip

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.

@leeebo I'm sorry for the late review.

This is 1st round review focused mostly on API + some nitpicks.

I'll continue with implementation review soon

host/class/uac/usb_host_uac/CMakeLists.txt Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/Kconfig Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_descriptors.c Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_descriptors.c Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_host.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

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

Left some minor comments and typos fixes.

host/class/uac/usb_host_uac/include/usb/uac.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Show resolved Hide resolved
host/class/uac/usb_host_uac/include/usb/uac.h Outdated Show resolved Hide resolved
@Dazza0 Dazza0 self-requested a review April 18, 2024 07:39
@roma-jam roma-jam self-requested a review April 18, 2024 11:39
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.

@leeebo Output path works great with my UAC 1.0 device!

Is this version supposed to work in input devices (microphones) too?

EDIT: I run a quick test with a microphone and it looks OK too

host/class/uac/usb_host_uac/uac_host.c Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_host.c Outdated Show resolved Hide resolved
@roma-jam
Copy link
Collaborator

Hi @leeebo,
I have only two UAC devices I could use.
One of them fails with:

I (3253) UAC_TEST: USB Host installed
I (3253) UAC_TEST: UAC Class Driver installed
E (128523) HCD DWC: EP MPS (200) exceeds supported limit (128)
E (128523) uac-host: uac_host_interface_claim_and_prepare_transfer(897): Unable to claim Interface
E (128528) uac-host: uac_host_device_start(1930): Unable to claim Interface

Which seems OK, as this device has two audio interfaces, where mps for EP is 200 B.

Another one fails with:

 (13845) UAC_TEST: USB Host installed
I (13845) UAC_TEST: UAC Class Driver installed
W (19301) uac-host: Feature Unit ID 2 not corresponding to the input terminal
W (19301) uac-host: Feature Unit ID 7 not corresponding to the input terminal

assert failed: block_trim_free tlsf.c:496 (block_is_free(block) && "block must be free")
HINT: CORRUPT HEAP: heap metadata corrupted resulting in TLSF malfunction.
Make sure you are not making out of bound writing on the memory you allocate in your application.
Make sure you are not writing on freed memory.
For more information run 'idf.py docs -sp api-reference/system/heap_debug.html'.

Anyway, it seems that I am not sure how to verify it on a hardware.
I am launching test_app, test 5 and then connect the device to the port. Am I doing something wrong?

Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

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

@leeebo Thanks for the such a huge addition.
In general, the idea is clear. Apparently, I was not able to verify it on a hardware, but probably you will help me and suggest how I can do that.

I have left couple of thing, that came to me during the review.

The main idea is: we have three big logical blocks (ringbuffer, USB device handling with its ctrl_transfer handling, interface handling (uac_device part)). How do you think, would it make sense to distinguish them to several separate files?

host/class/uac/usb_host_uac/uac_host.c Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_host.c Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_host.c Show resolved Hide resolved
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.

LGTM!

host/class/uac/usb_host_uac/include/usb/uac_host.h Outdated Show resolved Hide resolved
host/class/uac/usb_host_uac/uac_host.c Outdated Show resolved Hide resolved
@leeebo
Copy link
Collaborator Author

leeebo commented Apr 23, 2024

W (19301) uac-host: Feature Unit ID 2 not corresponding to the input terminal
W (19301) uac-host: Feature Unit ID 7 not corresponding to the input terminal
assert failed: block_trim_free tlsf.c:496 (block_is_free(block) && "block must be free")

Hi @roma-jam thanks for your test, for the first device, it related to the topic "should we support a configurable fifo size?",

For the second device, it must be something wrong in the descriptor's parsing logic, would you please share the full descriptors?

@leeebo
Copy link
Collaborator Author

leeebo commented Apr 28, 2024

@leeebo sorry I was not aware that the tinyusb supports only UAC2.0.

In this case, please enable at least build in CI by adding a path to your new component to .idf_build_apps.toml file in the root dir.

Hi @peter-marcisovsky would you please point out how to just build the test app without run it? 388bad1 seems not work

@roma-jam
Copy link
Collaborator

Sure,

For the second device, it must be something wrong in the descriptor's parsing logic, would you please share the full descriptors?

jabra_ev_hs.txt

@peter-marcisovsky
Copy link
Collaborator

peter-marcisovsky commented Apr 29, 2024

Hi @leeebo, to reply to your comment:

Hi @peter-marcisovsky would you please point out how to just build the test app without run it? 388bad1 seems not work

To build a test app, without running in in the CI, you could mark pytest as temporarily disabled in CI

We are using this approach, for example in usb examples in esp-idf

Edit: Sorry, I forgot we are not in esp-idf CI. So this tag unfortunately will not work.

@leeebo
Copy link
Collaborator Author

leeebo commented May 8, 2024

Sure,

For the second device, it must be something wrong in the descriptor's parsing logic, would you please share the full descriptors?

jabra_ev_hs.txt

Hi @roma-jam, the logic blocks in your UAC device is like the below:

image

However the current feature unit parsing logic can not work with MIXER_UNIT , I will refactor the parsing logic in this PR to make it more flexible (Replace the one sequential descriptor query with multiple times )

@leeebo
Copy link
Collaborator Author

leeebo commented May 8, 2024

Hi @roma-jam , the latest commit should fixed the MIXER_UNIT handle bug, please try again if you have time.

BTW, would you please try with ESP32-P4 for the EP MPS (200) exceeds supported limit (128) issue, it should work.

Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

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

Some observations from running the test app

@roma-jam
Copy link
Collaborator

@leeebo,

That's the output with Jabra Evolve headset on S3:

Running test uac tx rx loopback with disconnect...
I (2393) UAC_TEST: USB Host installed
I (2393) UAC_TEST: UAC Class Driver installed
W (8311) uac-host: UAC Interface 2->1, Frequency Number 5 exceed the maximum 4
W (8313) UAC_TEST: Speaker channels 2 and microphone channels 1 are not the same
I (8351) uac-host: Set Interface 1-1
I (8351) uac-host: Set EP 82 frequency 8000
I (8426) uac-host: Set Interface 2-1
I (8426) uac-host: Set EP 02 frequency 8000
I (9593) uac-host: Set Interface 1-0
I (9599) uac-host: Set Interface 2-0
I (9736) uac-host: Set Interface 1-1
I (9736) uac-host: Set EP 82 frequency 8000
I (9816) uac-host: Set Interface 2-1
I (9816) uac-host: Set EP 02 frequency 8000
E (10977) USBH: Device 1 gone
E (10977) uac-host: uac_control_transfer(1473): Unable to submit control transfer
W (10978) uac-host: Set Interface 1-0 Failed
I (10983) UAC_TEST: UAC Device disconnected
E (10997) uac-host: uac_control_transfer(1473): Unable to submit control transfer
W (10997) uac-host: Set Interface 2-0 Failed
I (11001) UAC_TEST: UAC Device disconnected
All devices freed
I (11987) UAC_TEST: UAC Driver uninstall
No more clients
I (11987) UAC_TEST: USB Host shutdown
MALLOC_CAP_8BIT: Before 383072 bytes free, After 383056 bytes free (delta -16)
MALLOC_CAP_32BIT: Before 383072 bytes free, After 383056 bytes free (delta -16)
./main/test_host_uac.c:777:test uac tx rx loopback with disconnect:PASS
Test ran in 9759ms

-----------------------
1 Tests 0 Failures 0 Ignored 
OK
Enter next test, or 'enter' to see menu

ESP32-P4 for the EP MPS (200) exceeds supported limit (128) output (BTW, I don't have test number 5 for P4, hope that how is was planned):

Running test uac rx reading...
I (70774) UAC_TEST: USB Host installed
I (70774) UAC_TEST: UAC Class Driver installed
I (79664) uac-host: Set Interface 2-1
I (79665) uac-host: Set EP 82 frequency 48000
I (79665) UAC_TEST: Start reading data from MIC
I (84687) UAC_TEST: Stop reading data from MIC
I (84687) uac-host: Set Interface 2-0
I (85198) UAC_TEST: UAC Driver uninstall
No more clients
All devices freed
I (85198) UAC_TEST: USB Host shutdown
MALLOC_CAP_8BIT: Before 604376 bytes free, After 604360 bytes free (delta -16)
MALLOC_CAP_32BIT: Before 604376 bytes free, After 604360 bytes free (delta -16)
./main/test_host_uac.c:381:test uac rx reading:PASS
Test ran in 2656ms

-----------------------
1 Tests 0 Failures 0 Ignored 
OK

@roma-jam roma-jam self-requested a review May 22, 2024 07:53
Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

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

Thanks a lot @leeebo,

LGTM! 👍

@peter-marcisovsky
Copy link
Collaborator

peter-marcisovsky commented May 22, 2024

Hi @leeebo
Just tired the test app again. This time I tried running pytest. There is a following issue:

2024-05-22 11:19:41 |    |   \/   _____/\______   \ _/  |_  ____   _______/  |_ 
2024-05-22 11:19:41 |    |   /\_____  \  |    |  _/ \   __\/ __ \ /  ___/\   __\
2024-05-22 11:19:41 |    |  / /        \ |    |   \  |  | \  ___/ \___ \  |  |  
2024-05-22 11:19:41 |______/ /_______  / |______  /  |__|  \___  >____  > |__|  
2024-05-22 11:19:41                  \/         \/             \/     \/        
2024-05-22 11:19:41 
2024-05-22 11:19:41 
2024-05-22 11:19:41 Press ENTER to see the list of tests.
2024-05-22 11:19:41 [uac_host]
2024-05-22 11:19:41 Running tests matching '[uac_host]'...
2024-05-22 11:19:41 Running test uac device handling...
2024-05-22 11:19:41 I (591) UAC_TEST: USB Host installed
2024-05-22 11:19:41 I (591) UAC_TEST: UAC Class Driver installed
2024-05-22 11:19:42 ./main/test_host_uac.c:308:test uac device handling:FAIL: Expected 3 Was 1. Function [uac_host][known_device]MALLOC_CAP_8BIT: Before 383072 bytes free, After 368080 bytes free (delta -14992)
2024-05-22 11:19:42 
2024-05-22 11:19:42 Running test uac rx reading...
2024-05-22 11:19:42 E (998) usb_phy: usb_new_phy(296): selected PHY is in use
2024-05-22 11:19:42 ./main/test_host_uac.c:151:test uac rx reading:FAIL: Expected 0 Was 259. Function [uac_host][rx]MALLOC_CAP_8BIT: Before 368080 bytes free, After 363252 bytes free (delta -4828)
2024-05-22 11:19:42 
2024-05-22 11:19:42 Running test uac tx writing...
2024-05-22 11:19:42 E (1023) usb_phy: usb_new_phy(296): selected PHY is in use
2024-05-22 11:19:42 ./main/test_host_uac.c:151:test uac tx writing:FAIL: Expected 0 Was 259. Function [uac_host][tx]MALLOC_CAP_8BIT: Before 363252 bytes free, After 358424 bytes free (delta -4828)

If a test fails, in my case its the very first test (the known devices error which I already pointed out, and you already explained.. so it's not an issue here), all the following tests collected by pytest will fail as well because of
E (998) usb_phy: usb_new_phy(296): selected PHY is in use

Could you please release the PHY, once a test fails?
In the esp-idf usb tests for example, we are calling usb_del_phy() during the test teardown.

@leeebo
Copy link
Collaborator Author

leeebo commented May 23, 2024

@peter-marcisovsky

Could you please release the PHY, once a test fails? In the esp-idf usb tests for example, we are calling usb_del_phy() during the test teardown.

Fixed now, you will get 5 Tests 1 Failures with your device now.

Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

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

Thanks for the additions.
LGTM 👍

@leeebo
Copy link
Collaborator Author

leeebo commented May 23, 2024

Hi @tore-espressif @roma-jam @peter-marcisovsky Thanks very much for the review work. I will squash the commits and release the first usb_host_uac v1.0.0.

@leeebo leeebo merged commit 4e895b4 into espressif:master May 23, 2024
17 checks passed
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.

4 participants