Allow passing feedback value unchanged through tud_audio_n_fb_set

[vmilea]

The Windows driver for USB Audio 2.0 seems to disregard the spec and expects 16.16 format instead of 10.14 on my full-speed device. Interestingly, on Linux the ALSA driver supports either format. So for now I'm calling tud_audio_n_fb_set(feedback << 2) to force 16.16 format everywhere. This works but is a bit hacky.

Could we have a config flag to disable the format conversion in tud_audio_n_fb_set()?

[vmilea]

For future reference: this blog describes how to get logs from the Windows driver. The driver complains after the feedback value being out of range when in 10.14 format.

[HiFiPhile]

That's interesting, though I don't remember I had issue testing full-speed.

[vmilea]

That's interesting, though I don't remember I had issue testing full-speed.

It happens on the Raspberry Pi Pico. I'd be interested to learn if it's specific to this device.

[HiFiPhile]

@PanRe haven't see you for awhile, do you remember something about this ?

[PanRe]

@HiFiPhile yeah it has been a while true! How are you all good?

@vmilea The driver can not know in advance if it is operated in full or high speed mode. Hence, the driver was designed such that the feedback value is always saved in 16.16 format and in case of full speed mode, the driver takes care of reforamting the value to the specified 10.14 format for sending. This way, the user does not need to care about the format with respect to the speed setting, just always set the value in 16.16 format. Until now, there were no issues with this design.
Does this answer your question?

[vmilea]

@PanRe The API design is fine, it's just that the reformatting to 10.14 sometimes gets in the way. I'm proposing something like CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION so it can be disabled if needed.

This is to work around a bug in the actual Windows driver. When passing 10.14 format on my full-speed device it refuses the value:

[USBAudio2]feedback value 0x000b0651 is out of range [0x002c0000,0x002d0000], ignoring value

The driver is expecting instead a 16.16 value between 0x2c.0-0x2d.0 (44.0-45.0).

[PanRe]

@vmilea Ah yeah now i see. Would you file a PR therefore? However, it is up to @hathach to decide if the PR is accepted. I would say it would be reasonable to do as you proposed. But i would prefer CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION since this renders to enable a non-default setting in the driver. And please explicitly describe in the comments what it is good for (as you did above).
By the way, do you have a working minimal speaker example which can be used to test for this bug? If yes, it would be really nice if you could file a PR with such. I think other people would be glad for support!

[HiFiPhile]

I've a guess but doesn't have a board to test with for the moment.

In XMOS's lib they use 3 bytes feedback for FS and 4 bytes for HS.

if (busSpeed == XUD_SPEED_HS)
  {
      XUD_SetReady_In(ep_aud_fb, fb_clocks, 4);
  }
  else
  {
      XUD_SetReady_In(ep_aud_fb, fb_clocks, 3);
  }

Maybe Windows use the byte length to decide coding format.

@vmilea Could you try to change 4 to 3 to see what happens ?

tinyusb/src/class/audio/audio_device.c

Line 1042 in f8288be

return usbd_edpt_xfer(rhport, audio->ep_fb, (uint8_t *) &audio->fb_val, 4);

[vmilea]

@HiFiPhile Good guess. Unfortunately it doesn't work:

[USBAudio2](0x01): buf 1 feedback packet 0 has invalid packet length 3, ignoring packet

I also tried setting wMaxPacketSize=3 on the feedback endpoint which the driver didn't accept:

[USBAudio2](0x0101): feedback endpoint MaxPacketSize=3 is invalid

[vmilea]

Would you file a PR therefore?

Done.

By the way, do you have a working minimal speaker example which can be used to test for this bug? If yes, it would be really nice if you could file a PR with such. I think other people would be glad for support!

I don't right now. It's not too difficult to create one with dummy output and fixed feedback value, but without real audio output the bug can only be checked through Windows driver logs.

[hathach]

thank you @vmilea for the issue and @HiFiPhile and @PanRe for helpful input. PR #1235 look good, though I would like to get more info with your example and set up for reproducing purpose, specially with descriptor. Which could potentially cause windows driver confusion. Would you mind provide

• your windows build version
• Your minimal code that cause the issue (best to be on github for other to comment on line of code). It is best to modify one of stock audio examples if possible, otherwise, please make it similar to stock one. So that we could test it on different MCUs.

I will try to pull out my old windows machine to test with.

[vmilea]

Ok, I'll come up with a minimal example tomorrow.

[vmilea]

your windows build version

Windows 10 Version 21H2 (OS Build 19044.1348).

[vmilea]

Your minimal code that cause the issue

Here is the test code. It's based on uac2_headset, tested on RP2040 and should work on other MCUs. The audio sink runs at 44100Hz, it discards incoming samples and sends a slightly skewed feedback rate of 44.125.

Follow these instructions to capture driver logs: https://matthewvaneerde.wordpress.com/2017/10/23/how-to-gather-and-read-logs-for-microsofts-usb-audio-2-0-class-driver/

I've left CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION=0 by default. With the FS device connected and playing something, the driver should print:

[USBAudio2]feedback value 0x000b0800 is out of range [0x002c0000,0x002d0000], ignoring value

With CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION=1, feedback should be accepted and instead you'll see:

[USBAudio2]EP 0x81 FB  stream running

[hathach]

thank you for the update, I will test it out as soon as I could, please be patient.

[geekbozu]

I feel like we have been working on a similar project...I fought with this a week or 2 ago.
I use an stm32f411 core.

The issue is windows actually ignores the speed of the device and checks the bcdUSB field.
When the bcdUsb field says we are a USB2.X the UAC2 driver assumes we are a high speed device and uses a 16.16 format.
When setting bcdUSB to 1.X, it uses the 10.14 format expected on a Full Speed device.

My solution was to just make a separate function that took in boolean to select the speed. However this approach is probably cleaner.

This also causes problems with using the device between Windows and Linux, Linux kernel driver properly uses the format based on device enumeration speed. NOT the bcdUSB field.

My very hacky workaround to this was to count sample data for 1000 frames(1 second) and see if we got any deviation from the expected sample rate, If we did we know feedback is working.

        usbAudioSampleStatus += n_bytes_received/2;

	frameCountForFeedback++;
	if(frameCountForFeedback >= 999 && !feedbackModeLocked){  //1 second...I hope this long enough
		if(usbAudioSampleStatus == sampFreq){
			usbAudioFeedBackFormat = !usbAudioFeedBackFormat; //Toggle format
		}else{
			feedbackModeLocked = true;
		}
		frameCountForFeedback = 0;
	}

[PanRe]

@vmilea Thanks for the effort!
@geekbozu Thanks for this idea! I actually like it as it would fix the Microsoft bug and other users would not get bothered by it. Once the Microsoft-bug was fixed, we could revert this patch. What would do you think @hathach ?

[geekbozu]

It's by no means "perfect" but in my test case of every PC/device in my home it seems to get the job done and select the appropriate format. ...except my android phone which just crashes the usb stack heh.
I should mention thats being called in the tud_audio_rx_done_pre_read_cb function.

[hathach]

@PanRe @geekbozu @vmilea thank you for more update, I am testing it out real soon, have some issue with Tracefmt last time (resolved now) and have to do lots of house cleaning lately.

[hathach]

It is definitely Windows UAC2 driver issue, despite of fullspeed detection, it still mis-interprets as highspeed endpoint

[USBAudio2]VID=0xcafe PID=0x4010 REV=0x0100
[USBAudio2]UsbSpeed = 2 FullSpeed
[USBAudio2]language ID = 0x0409
[USBAudio2]Product='TinyUSB speaker'

[USBAudio2]  AS OUT IfNb=0x01 AltSetting=0x01 '<NULL>'
[USBAudio2]     Format Type I PCM (0x00000001) SubSlotSize=2 BitsPerSample=16 ChannelCount=2
[USBAudio2]     Data Endpoint 0x08 Isochronous Asynchronous Data ActualMaxPacketSize=184 ActualPacketFrequency=1000/s (HS Endpoint)
[USBAudio2]     Feedback Endpoint 0x88 Isochronous None Feedback ActualMaxPacketSize=4 ActualPacketFrequency=1000/s (HS Endpoint)
[USBAudio2]     Supported sampling frequencies:
[USBAudio2]       min=44100 max=44100 res=0

I have tried to alter descriptor here and there, and change the feedback endpoint packet size to 3 following linux uac2 driver https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L387 .
then windows driver complains that packet size is invalid and refuse to Inspecting audio function topology of 'TinyUSB speaker'

[USBAudio2]reported IN channel mask 0x00000000 corrected to 0x00000003
[USBAudio2](0x0101): feedback endpoint MaxPacketSize=3 is invalid

I will try a bit more to see which parameter windows uac2sys used to determine device is highspeed endpoint (regardless of actual link speed). If it does not work out, we could use one of above walkaround.

attached is full log

iso-fb-3bytes.txt
iso-fb-4bytes.txt

PS: Sorry for late response, I am still trying to catch up.

[hathach]

I agree with @PanRe that @geekbozu walkaround is a better solution since it is more portable and allow device to work with both windows and Linux/macOS. @geekbozu do you have time to make an PR to get this upstream. We could add an comment note to remind us that we could remove or off it out when later windows update e.g win11 fix this issue etc..

[geekbozu]

I should be able to find some time in the next week or so. I need to think of how I would want to bake this into the driver properly. If at at all.

I actually wonder if just extending tud_audio_fb_set to take a parameter on which format to convert to. Or adding a function that does no conversion and letting the user handle it is the better choice, With documentation of course
Thoughts?

[hathach]

I should be able to find some time in the next week or so. I need to think of how I would want to bake this into the driver properly. If at at all.

That would be great

I actually wonder if just extending tud_audio_fb_set to take a parameter on which format to convert to. Or adding a function that does no conversion and letting the user handle it is the better choice, With documentation of course Thoughts?

It is also a good approach as well, I think we should also take this chance to name the function more explicitly e.g tud_audio_n_feedback_set(func_id, fb, format) where format is an enum for auto/16.16/10.14. Auto will do the conversion based on bus speed as it currently does.

[PanRe]

Sorry for the late reply. I am not sure if it is a good idea to make the feedback function so flexible:

• First of all, this sould not be a concern of the user as the value format was defined within the USB specs, so nobody would expect to set it on its own. The reason we would like to do that now is because we need a patch for Windows driver bug
• There is no chance the driver knows if a corretly working Linux driver or a buggy Windows driver is hosting the audio driver. Hence, the flexibility is not of use
• The user may switch between Linux and Windows hosts so we need a solution working in both cases

How about changing the driver such that there are four compile options:

• CFG_TUD_AUDIO_FEEDBACK_FORMAT_AUTO (mode accoring to USB specs as done right now)
• CFG_TUD_AUDIO_FEEDBACK_FORMAT_FIX_16_16 (fix to 16.16 format)
• CFG_TUD_AUDIO_FEEDBACK_FORMAT_FIX_10_14 (fix to 10.14 format)
• CFG_TUD_AUDIO_FEEDBACK_FORMAT_PATCH (patch of @geekbozu)

Incorporate it using common #if, #elif, #else, #endif commands and set the current default value to CFG_TUD_AUDIO_FEEDBACK_FORMAT_PATCH. Once the bug in Windows is fixed, we can change the default mode to CFG_TUD_AUDIO_FEEDBACK_FORMAT_AUTO.
What do you think?

[vmilea]

I think tud_audio_n_feedback_set(func_id, fb, format) sounds reasonable.

But to clarify a couple things about original proposal vmilea@ef879e8:

• It doesn't require changing the API.
• With CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION=1, we let the user handle the format. So they can change format at runtime like in @geekbozu's hack. In my opinion this kind of hack is better left to the user than integrating into tinyusb.

[vmilea]

Regarding cross-platform: Fixed 16.16 format is currently viable on Windows (10 / 11) and Linux (ALSA driver detects if the feedback rate is out of bounds and switches to the other format automatically). Unfortunately I don't have a Mac to check there as well.

[vmilea]

There is no chance the driver knows if a corretly working Linux driver or a buggy Windows driver is hosting the audio driver. Hence, the flexibility is not of use

I originally thought so as well, but as @geekbozu showed we may guess: if the incoming data rate doesn't correlate with feedback rate, than likely the feedback format is wrong.

[PanRe]

@vmilea Well, i would strongly vote for not letting the user (i mean the poor developer) handle this case. IMHO the tinyUSB driver should take care of this matter. This saves the users a lot of time and us a lot of bug reports.

Nevertheless, the ALSA driver you mentioned kept me thinking. Against the specifications they check and allow different formats too. I guess to robustify their driver against other UAC drivers which also do not respect the specs. Maybe we should do that too? Since not even Microsoft was capable of implementing the specs corretly, there may be more wrongly implented drivers floating around?!

[vmilea]

I guess to robustify their driver against other UAC drivers which also do not respect the specs. Maybe we should do that too?

Supporting both formats on host driver is trivial (shift feedback value by 2 if it's closer to the nominal rate).

On the other side, the client driver can't detect format mismatch directly. So tinyusb would just guesstimate after a number of frames based on data rate. In my experience, these heuristics are tricky to implement and tend to fail in surprising ways. This is why I proposed leaving it out of the library.

[vmilea]

@hathach Have you reported the Windows driver bug? I think there was a Feedback Hub app for that. If not, I'll do it once I'm back from vacation.

[hathach]

Thank you @PanRe and @vmilea for the feedback

Regarding cross-platform: Fixed 16.16 format is currently viable on Windows (10 / 11) and Linux (ALSA driver detects if the feedback rate is out of bounds and switches to the other format automatically). Unfortunately I don't have a Mac to check there as well.

I did a quick check with CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION=1 on an macOS Big Sur and it works well.

Have you reported the Windows driver bug? I think there was a Feedback Hub app for that. If not, I'll do it once I'm back from vacation.

Please proceed to file issue on windows feedback hub when you have time, I only run windows on VM when needed to test and not familiar with their driver development.

[hathach]

Sorry for the late reply. I am not sure if it is a good idea to make the feedback function so flexible:

• First of all, this should not be a concern of the user as the value format was defined within the USB specs, so nobody would expect to set it on its own. The reason we would like to do that now is because we need a patch for Windows driver bug
• There is no chance the driver knows if a correctly working Linux driver or a buggy Windows driver is hosting the audio driver. Hence, the flexibility is not of use
• The user may switch between Linux and Windows hosts so we need a solution working in both cases

yeah I agree with all of your points, we need a dynamic solution that works with all OS windows/linux/macos. I have tested both 16.16 and 10.14 on Linux/MacOS, and it works great on both platforms with both format on Fullspeed device. Apparently they do some feedback correction in the host driver.

• Linux and macOS Big Sur support both 16.16 and 10.14 on FS device
• Windows does not support FS 10.14 hence violate USB specs.

To sum up FS 16.16 is universally supported by major OS.

How about changing the driver such that there are four compile options:

CFG_TUD_AUDIO_FEEDBACK_FORMAT_AUTO (mode accoring to USB specs as done right now)
CFG_TUD_AUDIO_FEEDBACK_FORMAT_FIX_16_16 (fix to 16.16 format)
CFG_TUD_AUDIO_FEEDBACK_FORMAT_FIX_10_14 (fix to 10.14 format)
CFG_TUD_AUDIO_FEEDBACK_FORMAT_PATCH (patch of @geekbozu)

I think these are pretty much the same as @vmilea with correction on/off. Where the correct = off will leave it up to user to pick the format and apply the patch if needed. Since FS 16.16 is supported by all major OS, I even think we should have default value to force 16.16 format (hence rename to ENABLE_FEEDBACK_FORMAT_CORRECTION). Since it will work with all existing OS, waiting for windows patch will take months or more, and user may or may not even upgrade their PCs due to various reasons (mass upgrading can be a headache).

SUM UP: we will merge 1235 with macro rename from CFG_TUD_AUDIO_DISABLE_FEEDBACK_FORMAT_CORRECTION to CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION and default to force 16.16 on both FS and HS device. These are purely based on little testing with my only macOS I have (Linux is probably more consistent), more tests would be welcome. @PanRe @vmilea @geekbozu please let me know if this makes sense to you, I am open to all suggestion,

• Support disabling feedback format correction #1234 #1235

[vmilea]

@hathach Thanks for the additional testing. I agree, in practice 16.16 is a better default.

[geekbozu]

Let em start with I am all for this. I think its a saner default.
However,
#1235 Documentation is not accurate,
The issue is the windows driver is assuming the format based on the bcdDevice field.
if bcdDevice = 0x01XX aka a 1.0 device windows assumes a 10.14 format.
if bcdDevice = 0x02XX aka a 2.0 Device makes windows expect 16.16.

All of our examples default to a usb2.0 device even though we are full speed.

If linux/mac support FullSpeed USB 2.0 16.16 out of the box. I agree this is a better default as well.

[hathach]

if bcdDevice = 0x01XX aka a 1.0 device windows assumes a 10.14 format.
if bcdDevice = 0x02XX aka a 2.0 Device makes windows expect 16.16.
All of our examples default to a usb2.0 device even though we are full speed.

Yeah, you are right. Since UAC 2.0 (2006) came after USB 2.0 (2000), it is absolutely normal to have fullspeed only to have bcd = 2.0. Since the new default does not correct format, application could just pass 10.14 when using with bcd = 1.1. We surely need to update the comment/document a bit to better explain these scenario.

[geekbozu]

So do we want to make a separate PR to allow modifications at runtime to the Feedback value? Or is getting the documentation updated in #1235 enough for everyone?

[hathach]

So do we want to make a separate PR to allow modifications at runtime to the Feedback value? Or is getting the documentation updated in #1235 enough for everyone?

your format mismatched detection snippet is awesome, I think we should also put it into the example with explanation (either enabled or commented out). If possible, it is probably better to be a separated PR after #1235.

@PanRe do you think having 16.16 as default is reasonable. If yes, we will wrap up #1235 . Of course, this change could be reverted/changed later on if there is issue in practice.

[PanRe]

@hathach yes I think 16.16. as default is the most elegant approach! Thx for testing it on Mac! :)Happy new year everybody btw! ;)

[geekbozu]

So do we want to make a separate PR to allow modifications at runtime to the Feedback value? Or is getting the documentation updated in #1235 enough for everyone?

your format mismatched detection snippet is awesome, I think we should also put it into the example with explanation (either enabled or commented out). If possible, it is probably better to be a separated PR after #1235.

@PanRe do you think having 16.16 as default is reasonable. If yes, we will wrap up #1235 . Of course, this change could be reverted/changed later on if there is issue in practice.

Merge #1235 and I can add update one of the examples with a PR adding that snippet above! As it requires more then just copying that code somewhere. The good news is using 16.16 format by default actually removes any custom formatting. So the above snipped can be modified to work properly. (just by sending 10.14 format to the routine)

Thanks everyone for the sleuthing as well. This was a tricky one to debug out on my end so its nice getting some help :D

[geekbozu]

FYI I opened a feedback request on the hub a when this started for the Windows driver. But no traction yet
https://aka.ms/AAeugzn

[vmilea]

FYI I opened a feedback request on the hub a when this started for the Windows driver.

Thanks, I've upvoted and added a few details.

[vmilea]

@hathach I've updated #1235 as suggested. Can we merge it?

("Build ESP" is failing, seems like a CI issue rather than something wrong in PR).

[hathach]

thanks @vmilea for the update, I was a bit busy, will check that out asap.

[hathach]

#1235 is merged, I think we are good to close this. Thank you every one for joining the discussion and helping to resolve this.