samples: usb: add UAC2 implicit feedback sample #70029
Conversation
tmon-nordic
added
area: USB
tmon-nordic
requested review from
koffes,
larsgk,
jfischer-no,
carlescufi and
alexsven
| CONFIG_UDC_DRIVER_LOG_LEVEL_WRN=y | ||
|
|
||
| #Enable timer for asynchronous feedback | ||
| CONFIG_NRFX_TIMER2=y |
There was a problem hiding this comment.
Do we need to use a NRFX type timer here, or can a generic Zephyr timer be used?
There was a problem hiding this comment.
feedback_nrf53.c implementation uses timer and connects USB SOF and I2S FRAMESTART to it with DPPI. DPPI is so specific to Nordic that I don't really see how to achieve this without resorting to low-level hardware interface.
I don't know how to achieve proper feedback without relying on the low-level functionality. This however is something that application has to somehow figure out and I do provide a working sample implementation for nRF53.
| return NULL; | ||
| } | ||
|
|
||
| void feedback_process(struct feedback_ctx *ctx) |
There was a problem hiding this comment.
With should normally be executed in these functions? Can some comments be added to guide customers?
There was a problem hiding this comment.
Read the comment in feedback.h:
https://github.com/zephyrproject-rtos/zephyr/blob/602054402313f885c2a722616ceee87c735668b6/samples/subsys/usb/uac2_implicit_feedback/src/feedback.h#L22-L33
The comment refers to what has to be reported. How this is achieved is entirely up to the implementer.
I only know how to achieve it using DPPI subsystem and TIMER on nRF53. I hope that eventually someone will come up with platform independent solution, but this is not a trivial task at all and I think it would require some sort of interrupt/event timestamping API that is simply not available yet (and some clever filtering because the control goal is to have I2S FRAMESTART and USB SOF happen at the same time, where obviously one interrupt handler will execute before the other and the two will keep "swapping" with each other which one executes first).
|
|
||
| struct usb_i2s_ctx { | ||
| const struct device *i2s_dev; | ||
| bool headphones_enabled; |
There was a problem hiding this comment.
Would it be more descriptive to name this playback and recording? Or out and in?
There was a problem hiding this comment.
In order to understand the sample I believe it is crucial to understand the audio device devicetree description. In devicetree description there are headphones and microphone terminals. headphones_enabled and microphone_enabled therefore is probably the least confusing name possible - headphones_enabled refers to headphones output terminal and microphone_enabled refers to microphone input terminal.
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
6020544 to
d8414a8
Compare
desowin
force-pushed
the
implicit-feedback
branch
2 times, most recently
from
e960c03 to
a0cb4b1
Compare
|
@koffes please take another look |
| /* Counter used to determine when to start I2S and then when to start | ||
| * sending RX packets to host. Overflows are not a problem because this | ||
| * variable is not necessary after both I2S and RX is started. | ||
| */ | ||
| uint8_t i2s_counter; |
There was a problem hiding this comment.
If you move it down to the other uint8_t you could probably reduce the size of the struct.
There was a problem hiding this comment.
There are 5 bool variables before this uint8_t, so moving this uint8_t won't really save anything.
On nRF5340DK the sizeof(struct usb_i2s_ctx) is 24 bytes. When i2s_counter is moved to the end of structure the size remains unchanged at 24 bytes. Moving all pointers to the beginning also makes the size remain intact.
Because there are 9 single-byte members (5 bool and 4 uint8_t) separated into two "blocks", shuffling them around won't give any structure size reduction regardless if the pointer size is 4 or 8 bytes.
There was a problem hiding this comment.
Indeed, bool, "the default is CHAR_TYPE_SIZE"
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
a0cb4b1 to
59e9ac5
Compare
| tests: | ||
| sample.subsys.usb.uac2_implicit_feedback: | ||
| depends_on: | ||
| - usb_device |
There was a problem hiding this comment.
Must be - usbd (#73269 merged), sorry thought of it too late
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
59e9ac5 to
27d09a9
Compare
|
Bulk changing the milestone from everything which did not make it to the v3.7.0 freeze deadline Please note that until rc2 only PRs containing bug fixes, docs and tests can be merged. Exceptions require approval by the release team and a vote by the TSC. |
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
27d09a9 to
4e10305
Compare
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
4e10305 to
45ae5a5
Compare
The sample illustates how to achieve bidirectional asynchronous audio. Implicit feedback is the only way to achieve asynchronous USB headset on devices that have only one isochronous IN and one isochronous OUT endpoint. The sample implements stereo playback and mono recording. While it would be possible to have stereo recording, the commonly available headsets are mono only. The number of channels can differ between audio sink and source because the only requirement is that the two run on same clock. Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
tmon-nordic
force-pushed
the
implicit-feedback
branch
from
45ae5a5 to
63d6305
Compare
The sample illustates how to achieve bidirectional asynchronous audio. Implicit feedback is the only way to achieve asynchronous USB headset on devices that have only one isochronous IN and one isochronous OUT endpoint.
The sample implements stereo playback and mono recording. While it would be possible to have stereo recording, the commonly available headsets are mono only. The number of channels can differ between audio sink and source because the only requirement is that the two run on same clock.