usb: device_next: new USB Audio 2 implementation #67614
Conversation
|
@jzipperer-fb @larsgk @koffes @alexsven |
| /* This sample does not send audio data so this won't be called */ | ||
| } | ||
|
|
||
| #if IS_ENABLED(CONFIG_SOC_COMPATIBLE_NRF5340_CPUAPP) |
There was a problem hiding this comment.
Looks strange to have this kind of check in the middle of a sample.
There was a problem hiding this comment.
I agree. I just didn't have an idea how to cleanly separate it, i.e. how the "interface" between this target specific code and rest of application should look like. I can try to think of something but not sure if I'll come up with anything better than just inline ifdef.
There was a problem hiding this comment.
can it be solved without using anything nrfx specific?
There was a problem hiding this comment.
I don't think so. The only non-specific solution would be to gather tick timestamps in both I2S and USB interrupts. The problematic part is to expose such timestamp information to application and to actually capture the tick timestamp inside the intterupt handler (in all I2S and all USB drivers). This would still most likely need additional logic to filter out for software scheduling overhead (to function properly this needs sub-sample precision, here the PI controller gets sample estimate with 1/32 sample resolution, i.e. value 16 means half-sample, from nRF specific code).
There was a problem hiding this comment.
Of course, I would like to be proven wrong. But to prove me wrong someone would have to actually come up with actual implementation that works. I agree that is is not ideal to have target-specific code in samples, but here for the time being it seems to be the only sensible approach. The sample can be compiled for other targets (with a compile time warning) and it will operate but will suffer underruns/overruns because the lack of proper feedback (which must be computed at run-time, because it essentially is ratio between actual I2S master clock and USB host SOF clock, i.e. nominally both are the same, but the actual deviations from nominal value is the key here).
For the USB Audio 2 class implementation however, the explicit feedback value is just something that application must provide (unless it either uses implicit feedback in which case it has to send audio samples at appropriate rate to host; or unless it claims that the clock is sof-synchronized in which case there'll always be nominal number of samples). The sample is in my opinion mandatory for such non-obvious functionality. While it would be best to have it target independent, it is always helpful to have some reference implementation that is known to work.
There was a problem hiding this comment.
I don't think we would want a sample living among the generic samples to be specific to Nordic? I had a quick look and I would say that so far we seem to have done a pretty good job at avoiding that (not sure about this guy, but that seems to be pretty much it) .
Could this be moved to boards/nrf? Is the "common" (and I understand maybe degraded?) version of this sample still something thart could live in an agnostic way in the locaiton currently proposed?
Note: did a quick check this way: grep "include <" samples/subsys -R | sort -u
There was a problem hiding this comment.
@kartben We could remove the sample altogether then because without target specific code it does not make sense to have it. We could then just have class implementation alone without any sample. Would doing so be any better?
There was a problem hiding this comment.
@kartben Not sure if you noticed but the sample is not really specific to Nordic as it is now. It does have the #else with warning and empty implementation of the necessary feedback functionality. This shows how to do the feedback, with two different methods, on Nordic which is IMHO better than removing the Nordic specific part and leaving all targets essentially not doing what this sample is supposed to show. Because the targeet specific code is also extremely application (i.e. sample) specific it doesn't make slightest sense to put it in boards/nrf. Having target specific part for at least one target is in my opinion the proper way to have sample with unavoidable target specific functionality in it.
|
This is a very interesting PR, well done and well documented - and I'd like to try it out if I get some time this weekend. That said, I think it would be great if it was somehow possible to not have too much vendor/board specific code in generic samples. BTW: I do realize that bot I2S, PWM and probably more higher level abstractions in Zephyr may need to be revisited (e.g. the PWM API can do 'beep', while the nrfx pwm can play complex sample sequences). |
|
|
||
| #include "../app.overlay" | ||
|
|
||
| &pinctrl { |
There was a problem hiding this comment.
Someone might try to use an Arduino shield with this sample code or copy&paste it to own application, for whatever reason. Should arduino_spi node be deleted? @gmarull
There was a problem hiding this comment.
The same would then have to apply to https://github.com/zephyrproject-rtos/zephyr/blob/5273af6ba80a5919fd56e69d05b820beadb3ec0e/samples/drivers/i2s/echo/boards/nrf5340dk_nrf5340_cpuapp.overlay that uses the same pins for I2S as this sample.
| #define CONTROL_SELECTOR(setup) ((setup->wValue & 0xFF00) >> 8) | ||
| #define CONTROL_CHANNEL_NUMBER(setup) (setup->wValue & 0x00FF) | ||
|
|
||
| typedef enum __packed { |
There was a problem hiding this comment.
curious, what is the reason to have __packed here?
There was a problem hiding this comment.
To instruct compiler that that the enum should use smallest type available. There is a constant array of all entity types placed in code memory (flash) for every UAC2 instance (necessary for e.g. handling control transfers). Such construct (enum __packed) is used in internal Bluetooth API as well.
There was a problem hiding this comment.
I mean, do you really need it here? It does not use more memory without. Not sure fshort-enums flag is set for GCC in Zephyr.
git grep "fshort-enums"
boards/posix/nrf_bsim/CMakeLists.txt: -fshort-enums
boards/posix/nrf_bsim/CMakeLists.txt:target_compile_options(native_simulator INTERFACE -fshort-enums)
cmake/compiler/armclang/target.cmake: -fshort-enums
cmake/compiler/clang/target.cmake: -fshort-enums
cmake/compiler/clang/target.cmake: -fshort-enums
There was a problem hiding this comment.
Removed the __packed from enum typedef.
| return -ENOTSUP; | ||
| } | ||
|
|
||
| if (setup->bmRequestType == GET_CLASS_REQUEST_TYPE) { |
There was a problem hiding this comment.
This is probably okay, but you only need to differentiate the request type class to the recipient interface and endpoint, otherwise the stack would not call this callback.
There was a problem hiding this comment.
Yes, Audio class does support interface and endpoint requests. Here, currently the absolute minimum of control requests are handled to satisfy the most basic example.
Start of Frame is not relevant for most classes, but it is crucial for isochronous operation. The most prominent example where SOF is necessary is USB Audio class. Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
Introduce new USB Audio 2 implementation written from scratch. Main goal behind new implementation was to perform entity configuration with devicetree bindings, hiding the descriptor complexity from application. Initial implementation is working at Full-Speed only. High-Speed support will come later, but even at Full-Speed only this is viable replacement for old stack USB Audio 1 class (USB Audio 1 is limited to Full-Speed by specification, i.e. it is explicitly forbidden for USB Audio 1 device to work at High-Speed). Implemented is only absolute minimum set of features required for basic implicit and explicit feedback samples. Only one sample frequency is currently supported. Optional interrupt notifications are not supported. Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
The sample illustrates how to calculate feedback value based on I2S sample clock measurement or indirect I2S FRAMESTART and USBD SOF. The sample is currently only supported on nrf5340dk_nrf5340_cpuapp target because the feedback measurement uses target specific peripherals. While it should be possible to perform feedback value calculation entirely in software (possibly with some additional filtering due to software scheduling jitter), the I2S API does not provide necessary timestamps. Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
|
Very nice to see this. Thanks for the work here! |
MIDI is completely separate interface that is linked with USB Audio class only because of its original bundling with USB Audio 1. When USB Audio 2 devices implement MIDI, they really have separate interface (not bundled in the interface association descriptor with the USB Audio 2 interfaces) for the MIDIStreaming in USB Audio 1 interface. In fact, inside USB MIDI 2.0 Specification there is:
I currently do not plan on adding MIDI support myself as I have no use case for it. If someone comes up with the implementation, I can review it. |
|
Fair enough. Thanks for the quick reply. |
Introduce new USB Audio 2 implementation written from scratch. Main goal behind new implementation was to perform entity configuration with devicetree bindings, hiding the descriptor complexity from application.
Initial implementation is working at Full-Speed only. High-Speed support will come later, but even at Full-Speed only this is viable replacement for old stack USB Audio 1 class (USB Audio 1 is limited to Full-Speed by specification, i.e. it is explicitly forbidden for USB Audio 1 device to work at High-Speed).
Implemented is only absolute minimum set of features required for basic implicit and explicit feedback samples. Only one sample frequency is currently supported. Optional interrupt notifications are not supported.
The implementation does achieve zero-copy for audio samples in the provided explicit feedback sample. Only explicit feedback sample is provided for the time being, but implicit feedback is known to work with this class implementation. The only reason why implicit feedback is not yet published is code quality - and that I want to have a chance to get the class merged before feature freeze. The implicit feedback will come in later.