[9front] USB isochronous endpoint with feedback

[Michael Forney <mforney@mforney.org>]

[-- Attachment #1: Type: text/plain, Size: 2826 bytes --]

Hi,

I'm working on getting my USB audio DAC to work in 9front.  The device
implements USB audio 2.0 which is a bit different than the 1.0
supported by nusb/audio, but I made some local changes to deal with
that (still WIP though).  The main difference is that sampling rate is
controlled through a clock source entity connected to the terminal
associated with the AS interface rather than being selected through an
alternate setting of the AS interface and sampling rate endpoint
control.

The issue I'm encountering is that the output iso endpoint is
asynchronous, and this is not supported by usb(3) or nusb(2).  An
async iso endpoint is not synchronized to USB (μ)frames, but to some
other clock.  Instead, sinks use an associated feedback endpoint which
provides a fixed-point number of samples to write each frame.  This is
updated by the device to maintain an optimal buffer level.  Luckily,
it seems that if I just ignore the feedback endpoint and use the
existing pollival/hz/samplesz mechanism, it works well enough.

However, the existence of the feedback endpoint breaks several
assumptions made in nusb(2).  The feedback endpoint has the same
number as the data endpoint, which causes usbd to create an unusable
rw iso endpoint.  Attached patch 1 prevents usbd from creating rw iso
endpoints (the feedback endpoint gets mapped to the 0x10|* range, just
as for endpoints with different types).

The other problem is that nusb(2) assumes that there is a single
endpoint per Altc; the settings for the last endpoint in the alt
setting take precedence.  There is a bit of overlap between fields in
Ep and Altc.  Both have maxpkt and ntds, but only Altc has pollival.
Altc contains the settings of the last endpoint in the alternate
setting, and Ep contains the last alt setting of the endpoint, but
neither of these is what I want (which is the alt settings of the data
endpoint).  nusb/audio uses the Altc to prepare the endpoint, so it
ends up using the settings for the feedback endpoint to configure the
data endpoint.

It seems to me that the hierarchy of structures in nusb(2) is not
quite right.  My best idea about how to fix it is to move Altc from
Iface to Ep, since it only seems to be used for endpoint settings.
That way, alternate settings for *each* endpoint in the interface are
available.  We could then drop the maxpkt and ntds fields from Ep,
using the ones from altc[0] instead.  This would probably be a fairly
involved change, and isn't something I want to tackle right now.  My
current solution is attached patch 2 to keep track of the iso usage in
the Ep, and patch 3 to only update the fields in the Altc for *data*
endpoints.  I believe this should be sufficient for USB audio devices.

Anyway, sorry for the long email.  Any advice or comments on my
patches would be appreciated!

-Michael

[-- Attachment #2.1: Type: text/plain, Size: 360 bytes --]

from postmaster@1ess:
The following attachment had content that we can't
prove to be harmless.  To avoid possible automatic
execution, we changed the content headers.
The original header was:

	Content-Disposition: attachment; filename=0001-nusb-don-t-create-rw-iso-endpoints.patch
	Content-Type: text/plain; charset="US-ASCII"
	Content-Transfer-Encoding: 7bit

[-- Attachment #2.2: 0001-nusb-don-t-create-rw-iso-endpoints.patch.suspect --]
[-- Type: application/octet-stream, Size: 785 bytes --]

 From da941b80e930c50c1d9fa0f358d2320d3a826b32
From: Michael Forney <mforney@mforney.org>
Date: Sun, 24 Jan 2021 11:27:09 +0000
Subject: nusb: don't create rw iso endpoints

There may be two iso endpoints with the same ID if it is asynchronous
or adaptive (one for data, one for feedback), and rw iso endpoints are
unusable (error out with "iso i/o is half-duplex").

diff 4834fade31f7f2e1845993c1ad873e396e5d03f9 da941b80e930c50c1d9fa0f358d2320d3a826b32
--- a/sys/src/cmd/nusb/lib/parse.c	Wed Feb  3 16:19:57 2021
+++ b/sys/src/cmd/nusb/lib/parse.c	Sun Jan 24 03:27:09 2021
@@ -127,7 +127,7 @@
 		ep = mkep(d, epid);
 		ep->dir = dir;
 	}else if((ep->addr & 0x80) != (addr & 0x80)){
-		if(ep->type == type)
+		if(ep->type == type && type != Eiso)
 			ep->dir = Eboth;
 		else {
 			/*

[-- Attachment #3.1: Type: text/plain, Size: 375 bytes --]

from postmaster@1ess:
The following attachment had content that we can't
prove to be harmless.  To avoid possible automatic
execution, we changed the content headers.
The original header was:

	Content-Disposition: attachment; filename=0003-nusb-only-save-settings-for-data-endpoint-in-Altc.patch
	Content-Type: text/plain; charset="US-ASCII"
	Content-Transfer-Encoding: 7bit

[-- Attachment #3.2: 0003-nusb-only-save-settings-for-data-endpoint-in-Altc.patch.suspect --]
[-- Type: application/octet-stream, Size: 1417 bytes --]

 From 343d4964099a80a628b5aa6e5606bafc771240d4
From: Michael Forney <mforney@mforney.org>
Date: Sun, 07 Feb 2021 00:37:26 +0000
Subject: nusb: only save settings for data endpoint in Altc

Otherwise, the last endpoint in the alternate setting takes
precedence, which may be a feedback endpoint for an async iso data
endpoint, which is not the one we're interested in.

diff cbf7e75d538fde63d151c00a715ce89bd396dd5e 343d4964099a80a628b5aa6e5606bafc771240d4
--- a/sys/src/cmd/nusb/lib/parse.c	Sun Jan 24 03:21:10 2021
+++ b/sys/src/cmd/nusb/lib/parse.c	Sat Feb  6 16:37:26 2021
@@ -111,8 +111,6 @@
 		return -1;
 	}
 	dep = (DEp *)b;
-	altc->attrib = dep->bmAttributes;	/* here? */
-	altc->interval = dep->bInterval;
 
 	type = dep->bmAttributes & 0x03;
 	addr = dep->bEndpointAddress;
@@ -145,14 +143,18 @@
 	ep->maxpkt = GET2(dep->wMaxPacketSize);
 	ep->ntds = 1 + ((ep->maxpkt >> 11) & 3);
 	ep->maxpkt &= 0x7FF;
-	altc->maxpkt = ep->maxpkt;
-	altc->ntds = ep->ntds;
 	ep->addr = addr;
 	ep->type = type;
 	ep->isotype = (dep->bmAttributes>>2) & 0x03;
 	ep->isousage = (dep->bmAttributes>>4) & 0x03;
 	ep->conf = c;
 	ep->iface = ip;
+	if(ep->type != Eiso || ep->isousage == Edata || ep->isousage == Eimplicit){
+		altc->attrib = dep->bmAttributes;
+		altc->interval = dep->bInterval;
+		altc->maxpkt = ep->maxpkt;
+		altc->ntds = ep->ntds;
+	}
 	for(i = 0; i < nelem(ip->ep); i++)
 		if(ip->ep[i] == nil)
 			break;