-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
@tore-espressif @roma-jam @Dazza0 PTAL! |
Hey @leeebo, The main problem there - is that if device has at least one interface, driver opens the device with In that case, it is better to "touch" the new device, checking the uac interface presence and add it to the list. But I explained the idea in espressif/idf-extra-components#214 (comment) Please, read the document and share your opinion. |
@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. |
@leeebo regarding the test app: I took a look at the failing CI logs, could you please try renaming the |
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? |
@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 |
8bdcfcd
to
94cf090
Compare
d196ffc
to
86d46de
Compare
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 Full descriptor |
There was a problem hiding this 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
There was a problem hiding this 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.
3dab838
to
bc4cebf
Compare
There was a problem hiding this 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
Hi @leeebo,
Which seems OK, as this device has two audio interfaces, where mps for EP is 200 B. Another one fails with:
Anyway, it seems that I am not sure how to verify it on a hardware. |
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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? |
Hi @peter-marcisovsky would you please point out how to just build the test app without run it? 388bad1 seems not work |
Sure,
|
Hi @leeebo, to reply to your comment:
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. |
Hi @roma-jam, the logic blocks in your UAC device is like the below: However the current feature unit parsing logic can not work with |
Hi @roma-jam , the latest commit should fixed the BTW, would you please try with ESP32-P4 for the |
There was a problem hiding this 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
That's the output with Jabra Evolve headset on S3:
ESP32-P4 for the
|
There was a problem hiding this 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! 👍
Hi @leeebo
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 Could you please release the PHY, once a test fails? |
Co-authored-by: Peter Marčišovský <[email protected]>
Co-authored-by: Peter Marčišovský <[email protected]>
Fixed now, you will get |
There was a problem hiding this 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 👍
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. |
The PR aims to add UAC Host Driver (Based on ESP-IDF USB Host Driver), featuring:
Checklist:
(No mock test, can only be test locally) Speaker and Mic works as normal
- [x] example: Play MP3 as normalI have removed the examples to esp-iot-solution/examples, reducing the work of review here.- [ ] example: Record voice to SDcardthread-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