UAC2 supports interrupt-endpoint for providing control-change notifications to the host #1702

battlesnake · 2022-10-23T10:02:32Z

Relates to issue #1697

Describe the PR

UAC2 supports an interrupt endpoint to provide notifications to the host about control changes (e.g. mute button pressed on the device).

The interrupt endpoint lives in the control interface, and transmits 6-byte messages to notify the host about control values changing.
The host may then decide to issue a get-entity request via the control endpoint in order to read the new control value.

These interrupts can notify about changes to endpoints, interfaces, entities/controls, and also provide vendor-specific notifications.

The implementation of this functionality in TinyUSB was incomplete.

I have tested this on a custom board using STM32H7 and no RTOS.
On Linux, I see ALSAMIXER's UI controls on the host PC change, when I interact with the physical controls on the device.

I'm not too familiar with the coding style or architecture for TinyUSB, so please give suggestions on making this PR conform with the existing style.

This PR also includes the fix from #1698, where compilation was failing (when interrupt-endpoint enabled) due to a "section" attribute being applied to a member within a struct.

Additional context

…ations to the host

HiFiPhile · 2022-10-24T09:14:36Z

src/class/audio/audio_device.h

@@ -531,7 +541,33 @@ TU_ATTR_WEAK TU_ATTR_FAST_FUNC void tud_audio_feedback_interval_isr(uint8_t func

 #endif // CFG_TUD_AUDIO_ENABLE_EP_OUT && CFG_TUD_AUDIO_ENABLE_FEEDBACK_EP

-#if CFG_TUD_AUDIO_INT_CTR_EPSIZE_IN
+#if CFG_TUD_AUDIO_ENABLE_INTERRUPT_EP
+// UAC2 §9.6


Thanks for your PR !

Which UAC2 document are you using ? In my side Interrupt Data Message is section 6.1 p129.

That seems to be a mis-paste from my other work on isochronous feedback (§9.6 in USB20 document).
I'll submit another PR soon with improvements for audio feedback which should reduce code size, and add (verified) support for FIFO-based feedback.

Reference fixed

battlesnake · 2022-10-24T20:42:54Z

Also, to get this (and feedback) working on my STM32H7 board, I also had to use this patch (for enabling SOF interrupt):
#1684

This is with the st/synopsys port.
I recently and briefly tried the synopsis/dwc2 port, but it didn't work out-of-the-box.
So I switch back to st/synopsys for now, and will look into the dwc2 issue sometime in the future.

HiFiPhile · 2022-10-26T10:09:53Z

src/class/audio/audio_device.c

+  while (p_desc < p_desc_end)
+  {
+    // Find correct interface
+    if (tu_desc_type(p_desc) == TUSB_DESC_INTERFACE && ((tusb_desc_interface_t const * )p_desc)->bInterfaceNumber == 0 && ((tusb_desc_interface_t const * )p_desc)->bAlternateSetting == 0)


bInterfaceNumber could be other than 0, it's defined by _intfnum

tinyusb/src/device/usbd.h

Line 442 in 28f49c0

#define TUD_AUDIO_MIC_ONE_CH_DESCRIPTOR(_itfnum, _stridx, _nBytesPerSample, _nBitsUsedPerSample, _epin, _epsize) \

.

@PanRe could you take a look ? I havn't work on UAC for a moment 🤣

Ah sorry, this is leftover from when I initially hacked it together for my specific project. I'll generalise that soon.

Also, since there are several places where we sequentially search the descriptors for some specific descriptor, should I create a separate function for doing that, which takes a predicate as an argument?
It should reduce code-size a bit while not burning a significant amount of extra cycles as a result.
And the CPU overhead shouldn't matter much anyway since this is set-up stuff rather than "in the loop" stuff.

Also, since there are several places where we sequentially search the descriptors for some specific descriptor, should I create a separate function for doing that, which takes a predicate as an argument?

I think there is no need to do it now, later we can generalize them with a function like tu_desc_search() if needed.

True, this number can be any interface number!

You may also skip that check since you already work on the descriptor corresponding to the right interrupt EP. Do we need to remember which INT EP corresponds to which interface? For sending messages via INT EP?

PanRe · 2022-10-26T14:55:41Z

Thanks a lot! Indeed the interrupt EP was not implemented correctly! Since you are the first one who needs it, i am glad you fix the issues!
One thing, the naming convention CFG_TUD_AUDIO_INT_CTR_EPSIZE_IN was wrongly introduced and is misleading - CTR has nothing to do with the interrupt EP, so it should be skipped. Could you fix that if you have the time?

battlesnake · 2022-10-26T18:57:12Z

INT_CTR to INT refactor done

PanRe · 2022-11-01T16:36:32Z

Thanks a lot and sorry for the late reply! I have but a few questions on this:

do we need a separate buffer for the interrupt messages to be sent?
where is the interrupt EP communication handled? I guess its outside the UAC2 driver currently. Does it make sense to incorporate the INT EP comunications somehow into the UAC2 driver?
i know this could be a lot of work but i wanted to ask: could you come up with some minimum working example such that other users could start on INT EPs within UAC2 right away? That would be awsome! :)

battlesnake · 2022-11-01T18:43:54Z

The buffer within the tinyusb audio device structure was there already when I started this work - I have no idea if it's required or if we can bypass it.
It might actually be ideal if the buffer was larger (or 6*n bytes, where n is specified by project-specific tinyusb config).
That way, multiple control updates could be buffered in tinyusb, compared to my current solution of managing the queue of updates on project-side.

My board + build system are quite custom so I can't easily extract an example out or test an example project.

Here's the relevant code though for using this feature. I have a buffer containing three pre-determined messages, which I send incrementally when the audio controls have changed (which can happen by user using physical controls, or me testing via serial interface, or by the USB host setting parameters via existing tinyusb functionality).

usb.c (implementations of tinyusb event handlers and tinyusb basics)

...

/* Called as part of main loop */
void usb_update()
{
#if CFG_TUD_AUDIO_ENABLE_INTERRUPT_EP
	usb_control_status_update();
#endif
#if CFG_TUD_AUDIO_ENABLE_FEEDBACK_EP
	usb_feedback_config_update();
#endif
	tud_task_ext(0, false);
}

...

#if CFG_TUD_AUDIO_ENABLE_INTERRUPT_EP
bool tud_audio_int_done_cb(_unused uint8_t rhport, _unused uint16_t n_bytes_copied)
{
	return usb_control_status_done();
}
#endif

usb_control_status.c (manages notifying host about control changes)

#include ...

#include <tusb.h>

#if CFG_TUD_AUDIO_ENABLE_INTERRUPT_EP

#define MAX_STATUS_MESSAGES_QUEUED 3
#define AUDIO_ATTR_CUR AUDIO_CS_REQ_CUR
#define STATUS_INTERRUPT_INFO_VENDOR_SPECIFIC 0x01
#define STATUS_INTERRUPT_INFO_ORIGIN_ENDPOINT 0x02
#define STATUS_INTERRUPT_INFO_ORIGIN_INTERFACE 0x00
#define STATUS_INTERRUPT_CONTROL_MASTER 0x00

static const audio_status_update_t status_message_queue[MAX_STATUS_MESSAGES_QUEUED] = {
	{
		.bInfo = STATUS_INTERRUPT_INFO_ORIGIN_INTERFACE,
		.bAttribute = AUDIO_CS_REQ_CUR,
		.wValue_cn_or_mcn = STATUS_INTERRUPT_CONTROL_MASTER,
		.wValue_cs = AUDIO_FU_CTRL_VOLUME,
		.wIndex_ep_or_int = ITF_NUM_AUDIO_CONTROL,
		.wIndex_entity_id = TERM_FEATURE_ID,
	},
	{
		.bInfo = STATUS_INTERRUPT_INFO_ORIGIN_INTERFACE,
		.bAttribute = AUDIO_CS_REQ_CUR,
		.wValue_cn_or_mcn = STATUS_INTERRUPT_CONTROL_MASTER,
		.wValue_cs = AUDIO_FU_CTRL_MUTE,
		.wIndex_ep_or_int = ITF_NUM_AUDIO_CONTROL,
		.wIndex_entity_id = TERM_FEATURE_ID,
	},
	{
		.bInfo = STATUS_INTERRUPT_INFO_ORIGIN_INTERFACE,
		.bAttribute = AUDIO_CS_REQ_CUR,
		.wValue_cn_or_mcn = STATUS_INTERRUPT_CONTROL_MASTER,
		.wValue_cs = AUDIO_FU_CTRL_GRAPHIC_EQUALIZER,
		.wIndex_ep_or_int = ITF_NUM_AUDIO_CONTROL,
		.wIndex_entity_id = TERM_FEATURE_ID,
	},
};

static uint32_t status_message_next = MAX_STATUS_MESSAGES_QUEUED;

/* Sends the next message and increments the message counter */
static void status_message_send_next()
{
	if (status_message_next >= MAX_STATUS_MESSAGES_QUEUED) {
		return;
	}
	usb_control_status_log("msg#=%d", status_message_next);
	const void *msg = &status_message_queue[status_message_next];
	status_message_next++;
	tud_audio_int_write(msg, 6);
}

void usb_control_status_update()
{
	/* If any control values have changed since our observer last polled, notify host */
	if (control_update(CONTROL_OBSERVER_USB)) {
		status_message_next = 0;
		status_message_send_next();
	}
}

bool usb_control_status_done()
{
	status_message_send_next();
	return true;
}

#endif

battlesnake · 2022-11-01T18:47:35Z

Almost forgot, relevant part of descriptor. Goes in control-interface part of descriptor, i.e. before any of the streaming interfaces/alternates.

descriptor

#if USB_ENABLE_STATUS_EP
#define STATUS_INT_EP_DESC_LEN TUD_DESC_EP_LEN
#define STATUS_INT_EP_DESC() \
		TUD_DESC_EP( \
			ENDPOINT_AUDIO_INTERRUPT, \
			HOSTIN_AUDIO_INTERRUPT_TYPE, \
			6 /* Buffer size as per standard */, \
			CONTROL_UPDATE_INTERVAL_LOG2P1 \
		),
#else
#define STATUS_INT_EP_DESC_LEN 0
#define STATUS_INT_EP_DESC()
#endif

battlesnake · 2022-11-01T18:55:30Z

Project-specific implementation details in my case:

Basically - each thing that cares about the audio controls has a value in a CONTROL_OBSERVER_* enum to represent it.
These are "observers", example:

The audio processor unit is an observer of the controls.
The USB control/status unit is an observer of the controls.
The serial user-interface is also an observer of the controls.

There's an array of observer flags (bool), one for each observer.

Whenever a control value changes (i.e. new value != old value, not just a "*_set method called") then all observer flags are set to true.
When an observer is polled (e.g. if (control_update(CONTROL_OBSERVER_USB)) in the above code), then we reset the flag for that specific observer to false while returning its previous value.

This way, whenever anything changes audio parameters, all observers get to know about it whenever they next poll during their update, and they can handle it each their own way.

Probably over-complicated for a basic demo, where you could probably just have a single boolean flag.

nathaniel-brough · 2022-11-20T16:37:43Z

Looks like your CI failure for the RP2040 build could be fixed in this PR #1716

battlesnake · 2022-11-29T20:46:30Z

@maxsimmonds1337 Info is here if you fancy building an example/demo.

maxsimmonds1337 · 2022-11-29T20:56:56Z

@maxsimmonds1337 Info is here if you fancy building an example/demo.

Perfect, thanks @battlesnake

Gadgetoid · 2024-03-12T21:45:34Z

Has anyone got this working?

I've attempted to reconcile these changes with the version of tinyusb used by Pico SDK, but at best nothing happens and at worst I can reliably crash my board. This happens when I attempt to adjust the volume from the host after calling tud_audio_int_ctr_n_write .

There's an awful work in progress mess here:

Gadgetoid/pico-tinyusb-i2s-speaker@84c127f

This doesn't show my futile modifications to tinyusb, which mostly involve trying to set ep_int_ctr to the correct value, since afaict everything else is covered.

HiFiPhile · 2024-03-13T15:21:00Z

I can take a look later, but there are too much pending changes in UAC class not merged yet...

Gadgetoid · 2024-03-13T16:45:07Z

I can take a look later

Thank you!

I can take a look later, but there are too much pending changes in UAC class not merged yet...

😬 I guess I'm going to have some fixes to do in future!

HiFiPhile · 2024-04-01T18:47:46Z

@battlesnake @Gadgetoid Sorry for the extra long delay, finally I reviewed the changes and it's ready to merge:

Some code refactor, now the only option needed is CFG_TUD_AUDIO_ENABLE_INTERRUPT_EP
Added interrupt endpoint descriptor in usbd.h.
Added support to uac2_headset example, now volume can be changed by on-board button press (Tested with Nucleo-G0B1RE on Windows 10 and Linux)

Requires upstream changes to tinyusb, see here: hathach/tinyusb#1702

Gadgetoid · 2024-04-02T17:24:29Z

Sorry for the extra long delay

Looks like three weeks since I bumped it, that's a fast turnaround IMO and it's hugely appreciated! (We'll pretend the PR isn't years old because something something glass houses and stones 😬)

I have updated my code and replaced the tinyusb submodule in my local clone of pico-sdk. Everything works as I'd expect with our RP2040-based design. Including mute/unmute. Full changeset on my not-actually-a-headset-but-two-speakers codebase here: Gadgetoid/pico-tinyusb-i2s-speaker@6032314

Tested on RP2040, macOS (Ventura) and Linux (Raspberry Pi OS bookworm, 6.6.20+rpt-rpi-2712). 🥳

Thanks, both, for making audio on the Pico just that little bit slicker.

battlesnake · 2024-05-15T19:42:10Z

Hooray!

I can switch back to mainline for my audio projects, instead of using my fork :D

This PR should hopefully bring in 2-way sync between device and host for control values.

Sometime I'll try to add support to the Linux UAC2 driver for multi-band equaliser controls too, which it currently seems to totally lack.

battlesnake mentioned this pull request Oct 23, 2022

Interrupt-endpoint for audio class #1697

Closed

1 task

UAC2 supports interrupt-endpoint for providing control-change notific…

f931983

…ations to the host

battlesnake force-pushed the uac2-interrupt-endpoint branch from 345ad9c to f931983 Compare October 23, 2022 10:38

HiFiPhile reviewed Oct 24, 2022

View reviewed changes

fixed spec reference

025d347

HiFiPhile reviewed Oct 26, 2022

View reviewed changes

battlesnake added 3 commits October 26, 2022 21:40

clean up descriptor search for interrupt endpoint

15ed45e

PanRe refactor applied: s{_int_ctr_}{_int_}g; s{_INT_CTR_}{_INT_}g;

9673d20

Merge branch 'master' into uac2-interrupt-endpoint

dd0350f

HiFiPhile added 3 commits April 1, 2024 12:35

Merge remote-tracking branch 'upstream/master' into pr/1702

a1f01fc

UAC2: refactor interrupt endpoint support.

05f9cab

UAC2: fix mounted condition.

7ca9880

HiFiPhile force-pushed the uac2-interrupt-endpoint branch 2 times, most recently from 2c8bc03 to 2defcfd Compare April 1, 2024 18:38

UAC2: add interrupt volume control to uac2_headset example.

6cf2798

HiFiPhile force-pushed the uac2-interrupt-endpoint branch from 2defcfd to 6cf2798 Compare April 1, 2024 18:39

HiFiPhile approved these changes Apr 1, 2024

View reviewed changes

Gadgetoid added a commit to Gadgetoid/pico-tinyusb-i2s-speaker that referenced this pull request Apr 1, 2024

Experimental: volume sync to host.

7779b8e

Requires upstream changes to tinyusb, see here: hathach/tinyusb#1702

Gadgetoid added a commit to Gadgetoid/pico-tinyusb-i2s-speaker that referenced this pull request Apr 2, 2024

Experimental: volume sync to host.

6032314

Requires upstream changes to tinyusb, see here: hathach/tinyusb#1702

HiFiPhile merged commit 60acb99 into hathach:master Apr 2, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UAC2 supports interrupt-endpoint for providing control-change notifications to the host #1702

UAC2 supports interrupt-endpoint for providing control-change notifications to the host #1702

battlesnake commented Oct 23, 2022

HiFiPhile Oct 24, 2022

battlesnake Oct 24, 2022 •

edited

Loading

battlesnake Oct 24, 2022

battlesnake commented Oct 24, 2022

HiFiPhile Oct 26, 2022

battlesnake Oct 26, 2022

HiFiPhile Oct 26, 2022

PanRe Oct 26, 2022

PanRe Oct 26, 2022

battlesnake Oct 26, 2022

PanRe commented Oct 26, 2022

battlesnake commented Oct 26, 2022

PanRe commented Nov 1, 2022

battlesnake commented Nov 1, 2022 •

edited

Loading

battlesnake commented Nov 1, 2022

battlesnake commented Nov 1, 2022 •

edited

Loading

nathaniel-brough commented Nov 20, 2022

battlesnake commented Nov 29, 2022

maxsimmonds1337 commented Nov 29, 2022

Gadgetoid commented Mar 12, 2024

HiFiPhile commented Mar 13, 2024

Gadgetoid commented Mar 13, 2024

HiFiPhile commented Apr 1, 2024

Gadgetoid commented Apr 2, 2024

battlesnake commented May 15, 2024

UAC2 supports interrupt-endpoint for providing control-change notifications to the host #1702

UAC2 supports interrupt-endpoint for providing control-change notifications to the host #1702

Conversation

battlesnake commented Oct 23, 2022

Choose a reason for hiding this comment

battlesnake Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

battlesnake commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanRe commented Oct 26, 2022

battlesnake commented Oct 26, 2022

PanRe commented Nov 1, 2022

battlesnake commented Nov 1, 2022 • edited Loading

usb.c (implementations of tinyusb event handlers and tinyusb basics)

usb_control_status.c (manages notifying host about control changes)

battlesnake commented Nov 1, 2022

descriptor

battlesnake commented Nov 1, 2022 • edited Loading

nathaniel-brough commented Nov 20, 2022

battlesnake commented Nov 29, 2022

maxsimmonds1337 commented Nov 29, 2022

Gadgetoid commented Mar 12, 2024

HiFiPhile commented Mar 13, 2024

Gadgetoid commented Mar 13, 2024

HiFiPhile commented Apr 1, 2024

Gadgetoid commented Apr 2, 2024

battlesnake commented May 15, 2024

battlesnake Oct 24, 2022 •

edited

Loading

battlesnake commented Nov 1, 2022 •

edited

Loading

battlesnake commented Nov 1, 2022 •

edited

Loading