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

UAC2: Implement feedback by fifo counting. #2328

Merged
merged 29 commits into from
Aug 5, 2024
Merged

Conversation

HiFiPhile
Copy link
Collaborator

@HiFiPhile HiFiPhile commented Nov 19, 2023

Describe the PR

  • Implement feedback by fifo counting
  • Implement host OS guessing to resolve compatibility issues

Compared to other methods the only thing needed is add a callback to set sample rate, all calculations are done inside the class.

void tud_audio_feedback_params_cb(uint8_t func_id, uint8_t alt_itf, audio_feedback_params_t* feedback_param)
{
  // Set feedback methode to fifo counting
  feedback_param->method = AUDIO_FEEDBACK_METHOD_FIFO_COUNT;
  feedback_param->sample_freq = current_sample_rate;
}

A new example uac2_speaker_fb has been added to show how the mechanism work with a stereo speaker. Both FS (STM32F429) and HS (LPC55S69) configs are tested and FIFO level is well maintained half filled.


A HID debug interface can be enabled by setting CFG_AUDIO_DEBUG=1 which export UAC class information and can be read by audio_debug.py.

image

@HiFiPhile
Copy link
Collaborator Author

Test welcomed @Protoxy22 @Lurcy38 @kf6gpe @PanRe

@Protoxy22
Copy link

Protoxy22 commented Nov 20, 2023

Wow great work, I will have time to test this next week on an STM32H743 Eval Board with USB HS

Thank you

@Hal9000Space
Copy link

This is interesting. I'm curious where can I find audio_debug.py. Any ideas? Thanks a lot!

@HiFiPhile
Copy link
Collaborator Author

This is interesting. I'm curious where can I find audio_debug.py. Any ideas? Thanks a lot!

Just in the PR examples/device/uac2_speaker_fb/src/audio_debug.py

@Hal9000Space
Copy link

This is interesting. I'm curious where can I find audio_debug.py. Any ideas? Thanks a lot!

Just in the PR examples/device/uac2_speaker_fb/src/audio_debug.py

Ah, so not in main yet. Ok, thanks!

@Lurcy38
Copy link
Contributor

Lurcy38 commented Dec 1, 2023

great :) , I will be very happy to test this implementation in my app, but i cannot reach the branch rx_fb , how to do ?

@HiFiPhile
Copy link
Collaborator Author

@Lurcy38 it's under my user space
https://github.com/HiFiPhile/tinyusb/tree/rx_fb
Or you can download PR by adding .patch
https://github.com/hathach/tinyusb/pull/2328.patch

@HiFiPhile
Copy link
Collaborator Author

@hathach After more tests this PR is ready.

@mastupristi
Copy link

Hi, what is the status of this PR? what is missing for it to be approved?

@Marcus2060
Copy link

Hi, great work,
very nice feedback method improvement on Tinyusb class Audio !
I Have implemented the feedback method FIFO_COUNTER (stereo 16 bit, 48KHz) on a my RP2040 hardware with I2S DAC with success .. but ...
I observe a good audio USB reproduction on Windows but the system crashes on MAC computer...???
If, instead, I remove only the feedback EP, the system It's starting to work on MAC too ! But obviously not sync ...

Could this be due to the fact that macOS does not accept this feedback method or some other reason such as incorrect descriptors for the MAC?

Thanks!

@mastupristi

This comment was marked as off-topic.

@mastupristi

This comment was marked as off-topic.

@mastupristi

This comment was marked as off-topic.

@Protoxy22

This comment was marked as off-topic.

@mastupristi

This comment was marked as off-topic.

@HiFiPhile
Copy link
Collaborator Author

Back from holiday...

I would add TUSB_ISO_EP_ATT_EXPLICIT_FB on the data endpoints, just to be sure that all OS are suggested to use the available feedback endpoint @HiFiPhile ?

No it's not how it works...

@HiFiPhile
Copy link
Collaborator Author

HiFiPhile commented Feb 27, 2024

I observe a good audio USB reproduction on Windows but the system crashes on MAC computer...???

@Marcus2060 What do you mean crash, does the kernel panic or having popping sound or something else ?

You can try to change

#define CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION 0 // 0 or 1

the choice of format is left to the caller and feedback argument is sent as-is. If CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION is set, then tinyusb
expects 16.16 format and handles the conversion to 10.14 on FS.
Note that due to a bug in its USB Audio 2.0 driver, Windows currently requires 16.16 format for _all_ USB 2.0 devices. 

From other people's experience it seems this format incompatibility issue exist.

On a FS interface, is there a good way to switch between the feedback formats for explicit feedback?

Windows expects 16.16. I get that. It looks very much like macOS and related Apple products adhere closer to the USB spec, and want 14.10 for the clock feedback.

I see there are 3 solutions:

  • Identify host OS on enumeration and change format correction on the fly : TODO, could be tricky to implement
  • Change the MCU and use high-speed
  • Ask Microsoft to fix their code

@Marcus2060
Copy link

Hi,
I continued working with a Full-Speed USB audio system RP2040 48KHz 16 bit by using feedback explicit endpoint.
(TinyUSB with FIFO counter feedback mechanism).

I've done other experiments and I get the impression that the usbd_edpt_xfer() function on low-level stack doesn't work well when we try to send 3 bytes.
These are my two scenarios:

MAC:

  1. The system works fine with 10.14 feedback (CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION=1) but only if I set MaxPacketBytes=3 in the feedback descriptor.
  2. The system doesn't work if I try to send 3 Bytes (10.14) via usbd_edpt_xfer() but with MaxPacketBytes=4 ??? (MaxPacketBytes means Max number of bytes transfer on the endpoint ??...)
  3. The system doesn't work with 16.16 feedback (4 bytes sent) because seems that macOS does not accept 16.16 for full speed devices.

Windows:

  1. The system works well with 16.16 feedback (4 bytes sent).
  2. The system doesn't work with 10.14 format (CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION=1) when I try to send 3 bytes by usbd_edpt_xfer() (MaxPacketBytes = 4 in the descriptor) ... I listen some click audio due, as we know, with more or less bytes on Audio buffer respect to the DAC buffer...
  3. If I use, instead, the same code of MAC (as above MaxPacketBytes = 3 in the descriptor) unfortunately the system stops working because the Audio class it is not enumerate on WIndows and I no longer see the card ... ???

Also:
I tested the pico-playground usb_sound_card project (Pico board + DAC board) and it works well both Windows and MAC. If we see the code (usb_sound_card.c) seems that feedback routine uses 10.14 format putted in 3 bytes.

So, could the problem be on the low level of the Tinyusb stack when we try to send 3 bytes feedback via usbd_edpt_xfer() to Host ???

Thanks!

@HiFiPhile
Copy link
Collaborator Author

So, could the problem be on the low level of the Tinyusb stack when we try to send 3 bytes feedback via usbd_edpt_xfer() to Host ???

No it can't be, you also tested 3 bytes works on MAC.

If I use, instead, the same code of MAC (as above MaxPacketBytes = 3 in the descriptor) unfortunately the system stops working because the Audio class it is not enumerate on WIndows and I no longer see the card ... ???

It's true.

I tested the pico-playground usb_sound_card project (Pico board + DAC board) and it works well both Windows and MAC. If we see the code (usb_sound_card.c) seems that feedback routine uses 10.14 format putted in 3 bytes.

Pico use UAC 1.0 while TUSB uses UAC 2.0, you can see their descriptors are quite different.

@Marcus2060
Copy link

Hi,
thank you for reply and sorry if I try to clarify better:

No it can't be, you also tested 3 bytes works on MAC.

No, my previous comment is precisely because I tested that the 3 bytes don't seem to work (see my pont 1, 2 about MAC):

  1. The system works fine with 10.14 feedback (CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION=1) but only if I set MaxPacketBytes=3 in the feedback descriptor.
  2. The system doesn't work if I try to send 3 Bytes (10.14) via usbd_edpt_xfer() but with MaxPacketBytes=4 (MaxPacketBytes means Max number of bytes transfer on the endpoint ??...)

I highlighted the 3 bytes sent problem because:
Point 1 could it be a mechanism where the low-level USB stack forces anyway the 3 bytes ??
Point 2 why doesnt'work while point 1 works ??

Thanks anyway for your attention.

@HiFiPhile
Copy link
Collaborator Author

HiFiPhile commented Mar 4, 2024

@Marcus2060

The system doesn't work if I try to send 3 Bytes (10.14) via usbd_edpt_xfer() but with MaxPacketBytes=4 (MaxPacketBytes means Max number of bytes transfer on the endpoint ??...)

Sorry I've some problem understanding, you mean 3 Bytes && MaxPacketBytes=4 doesn't work ? How about 3 Bytes && MaxPacketBytes=3 ?

@Marcus2060
Copy link

Sorry I've some problem understanding, you mean 3 Bytes && MaxPacketBytes=4 doesn't work ? How about 3 Bytes && MaxPacketBytes=3 ?

Below 4 different tests on MAC using 10.14 format:

  1. When (4 Bytes sent) && (MaxPacketBytes=4) -> Wrong Audio (I think the macOS recognize 16.16).
  2. When (3 Bytes sent) && (MaxPacketBytes=4) -> No Sync, some clicks.
  3. When (3 Bytes sent) && (MaxPacketBytes=3) -> No Sync, some clicks.
  4. When (4 Bytes sent) && (MaxPacketBytes=3) -> Works fine, no click!

Below the function to send feedback (feedback value is an uint32_t) :
usbd_edpt_xfer(audio->rhport, audio->ep_fb, (uint8_t *) &value, 4);

@HiFiPhile
Copy link
Collaborator Author

HiFiPhile commented May 12, 2024

Are you using the tusb_config.h and descriptors from the example

Yes they are from stack example, only difference is enable correction and set wMaxPacketSize=3:

diff --git a/examples/device/uac2_speaker_fb/src/tusb_config.h b/examples/device/uac2_speaker_fb/src/tusb_config.h
index 1cc1031c1..1a1cebca8 100644
--- a/examples/device/uac2_speaker_fb/src/tusb_config.h
+++ b/examples/device/uac2_speaker_fb/src/tusb_config.h
@@ -123,7 +123,7 @@ extern "C" {
 #define CFG_TUD_AUDIO_FUNC_1_DESC_LEN                                TUD_AUDIO_SPEAKER_STEREO_FB_DESC_LEN

 // Enable if Full-Speed on OSX, also set feedback EP size to 3
-#define CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION              0
+#define CFG_TUD_AUDIO_ENABLE_FEEDBACK_FORMAT_CORRECTION              1

 // Audio format type I specifications
 #if defined(__RX__)
diff --git a/examples/device/uac2_speaker_fb/src/usb_descriptors.c b/examples/device/uac2_speaker_fb/src/usb_descriptors.c
index fa307674d..5d84024ea 100644
--- a/examples/device/uac2_speaker_fb/src/usb_descriptors.c
+++ b/examples/device/uac2_speaker_fb/src/usb_descriptors.c
@@ -150,7 +150,7 @@ uint8_t const desc_configuration[] =
     TUD_CONFIG_DESCRIPTOR(1, ITF_NUM_TOTAL, 0, CONFIG_TOTAL_LEN, 0x00, 100),

     // Interface number, string index, byte per sample, bit per sample, EP Out, EP size, EP feedback, feedback EP size,
-    TUD_AUDIO_SPEAKER_STEREO_FB_DESCRIPTOR(0, 4, CFG_TUD_AUDIO_FUNC_1_N_BYTES_PER_SAMPLE_RX, CFG_TUD_AUDIO_FUNC_1_RESOLUTION_RX, EPNUM_AUDIO_OUT, CFG_TUD_AUDIO_FUNC_1_EP_OUT_SZ_MAX, EPNUM_AUDIO_FB | 0x80, 4),
+    TUD_AUDIO_SPEAKER_STEREO_FB_DESCRIPTOR(0, 4, CFG_TUD_AUDIO_FUNC_1_N_BYTES_PER_SAMPLE_RX, CFG_TUD_AUDIO_FUNC_1_RESOLUTION_RX, EPNUM_AUDIO_OUT, CFG_TUD_AUDIO_FUNC_1_EP_OUT_SZ_MAX, EPNUM_AUDIO_FB | 0x80, 3),

 #if CFG_AUDIO_DEBUG
     // Interface number, string index, protocol, report descriptor len, EP In address, size & polling interval

Also did you run into any difficulties getting the python script running on OSX?

There are some dependencies needs install:

sudo pip3 install hid
sudo pip3 install matplotlib

@HiFiPhile
Copy link
Collaborator Author

Last commit optimized debug interface fifo count, it's not the real count inside UAC class but a replication, it was less accurate.

@ra1nb0w
Copy link
Contributor

ra1nb0w commented Jul 25, 2024

maybe someone is interested in a rebased version to the latest master
https://github.com/ra1nb0w/tinyusb/tree/rx_fb

@HiFiPhile
Copy link
Collaborator Author

maybe someone is interested in a rebased version to the latest master https://github.com/ra1nb0w/tinyusb/tree/rx_fb

Thank you, will rebase when free, also move #2628 into example.

@HiFiPhile
Copy link
Collaborator Author

@hathach This PR has been tested my multiple users and works well, since @PanRe is not here could you take a look and merge?

Copy link
Collaborator

@PanRe PanRe left a comment

Choose a reason for hiding this comment

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

Impressive work! Thank you all for your effort in improving the UAC2 driver!!

@HiFiPhile HiFiPhile merged commit a7d1888 into hathach:master Aug 5, 2024
78 checks passed
@ra1nb0w
Copy link
Contributor

ra1nb0w commented Aug 5, 2024

thank you to everyone!

@geekbozu
Copy link

geekbozu commented Aug 5, 2024

Woo love to see this finally get in! Thanks for keeping it going :D

@mstaack
Copy link

mstaack commented Aug 5, 2024

great! hello proper uac:)

@skuep
Copy link
Contributor

skuep commented Aug 5, 2024

Nice work @HiFiPhile !
Regarding the OS guessing, I have done some work regarding Windows WCID in the past which could theoretically be used for reliably detecting windows (7 and up I Believe) by detecting string Descriptor 0xEE. See
https://github.com/pbatard/libwdi/wiki/WCID-Devices#user-content-Microsoft_OS_String_Descriptor

But I am not sure if the Descriptor is requested before the configuration Descriptor. Because I think it needs to change the maxPackstSize for the feedback endpoint, correct?

Other than that we could force renumeration by disconnecting and connecting the USB device virtually 🤣 EDIT: LOL of course that is patented. Insane. https://patents.google.com/patent/US20120054372A1/en

@HiFiPhile
Copy link
Collaborator Author

But I am not sure if the Descriptor is requested before the configuration Descriptor. Because I think it needs to change the maxPackstSize for the feedback endpoint, correct?

Yes the detection should be done before configuration descriptor request, that's why I put Windows' descriptor as default. I think 0xEE is requested in the same order as MsOS 2.0 descriptor.

Other than that we could force renumeration by disconnecting and connecting the USB device virtually 🤣 EDIT: LOL of course that is patented. Insane. https://patents.google.com/patent/US20120054372A1/en

I think I read it, many patents are just some nonsense...

@skuep
Copy link
Contributor

skuep commented Sep 18, 2024

@HiFiPhile thanks a lot for your effort. I looked through this and was wondering if it's wise to use the BOS Descriptor for the quirk detection because apparently the BOS Descriptor is mandatory only for USB versions > 2.0 if I read correctly.

In addition, device, config and string Descriptor order is already enough to distinguish between MacOS and not-MacOS.

What do you think?

@HiFiPhile
Copy link
Collaborator Author

@HiFiPhile thanks a lot for your effort. I looked through this and was wondering if it's wise to use the BOS Descriptor for the quirk detection because apparently the BOS Descriptor is mandatory only for USB versions > 2.0 if I read correctly.

In addition, device, config and string Descriptor order is already enough to distinguish between MacOS and not-MacOS.

What do you think?

You are right, config and string Descriptor is already enough to distinguish between MacOS and not-MacOS.

It's a full detection method from #2628 which allows each OS to be treated differently. In my testing BCD 2.01 is well supported, normally it shouldn't be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.