[9front] improving nusb/audio coverage

[9front] improving nusb/audio coverage

[ashley]

hey folks. I'm trying to get my Scarlett 2i4 to work with nusb/audio,
and I have run into two separate issues. I am going to explain them both
in this message, and I would be extremely grateful for any form of advice.

I would like to make it clear that I have not read the USB spec, and
only partially read the audio class spec. I find its writing style to be
overly verbose and unnatural, and the naming choices made confusing and
prone to ambiguity. the relevant information is always spread out over
several unrelated sections and has to be manually cross-referenced. I
apologize in advance if I've made any obvious mistakes or glaringly
misinterpreted the document at any point.

to start off, the misleadingly named bCSourceId field of both input and
output terminals points not to a clock source specifically, but to a
clock entity, which can be one of clock source, clock selector, or clock
multiplier (3.13.11 clock entities, page 36). the code at this point only
supports the former, and silently fails if another clock entity is in use.

my 2i4 appears to have a single clock selector, which acts as the
clock source for the terminals, and it is fed by a single clock
source.

I suggest something like the following:

Desc*
findclock(int id)
{
	Desc *dd;
	int i;
	for(i = 0; i < nelem(adev->usb->ddesc); i++){
		dd = adev->usb->ddesc[i];
		if(dd == nil || dd->iface != ac)
			continue;
		if(dd->data.bDescriptorType != 0x24 || dd->data.bLength != 8)
			continue;
		if(dd->data.bbytes[1] == id)
			return dd;
	}
	return nil;
}

int
isclocksuitable(Desc *dd)
{
	/* perhaps getclockrange should get called here as well? */

	/* check that clock has rw frequency control */
	return (dd->data.bbytes[3] & 3) != 3;
}

Desc*
findclocksource(Aconf* c)
{
	Desc *dd;
	uchar *b;
	int i;

	dd = findclock(c->clock);
	b = dd->data.bbytes;
	switch(b[0]) { /* bDescriptorSubtype */
	case 0x0A: /* CLOCK_SOURCE */
		if(isclocksuitable(dd))
			return dd;
		else
			return nil;
	case 0x0B: /* CLOCK_SELECTOR */
		c->selector = c->clock;
		for(i = 3; i < b[2]+3; i++) { /* bNrInPins */
			c->clock = b[i];
			if(isclocksuitable(findclock(c->clock)))
				break;
		}
		return dd;
	case 0x0C: /* CLOCK_MULTIPLIER */
		/* inform the user that this clock entity is not supported in some way */
		return nil;
	}
	return nil;
}

AConf should probably keep c->selector around, in case we need it later.

after this it may be necessary to send a CX_CLOCK_SELECTOR_CONTROL clock
selector control request to the selector to ensure that it selects the
right clock. an intuitive place to do that would be setclock, but since
a clock selector can have any clock entity connected to its input pins,
including other selectors, it may be necessary to call findclocksource
recursively, sending a clock selector control request with each recursion
level (and one way or another I am a bit daunted by usbcmd -- I am very
unsure in what way its parameters correspond to the request formats
prescribed by the spec, whose structs otherwise map in a fairly straight
forward manner (modulo the offsets and the magic number names)).



this brings us to the second problem: once we have successfully configured
our terminals and stuff, we have to set up an endpoint with the right
format so that we can write plan9-style audio to it.  nusb/audio currently
only supports audio formats where c->bits == 8*c->bps, which the spec
explicitly states might not hold (2.2.5 type I format type descriptor,
usb device class definition for audio data formats, p. 10. yes, you have
to hunt down a *separate* companion document to find this info.) the
spec calls c->bps bSubrfameSize and c->bits bBitResolution.  my 2i4
inconveniently lacks any formats with that property; in fact it only
has one format: 24-bits, 4 bytes per subframe, which entails one byte
of padding per subframe. why anyone ever thought that this was a sane
way to format audio is simply beyond me.

this problem is less straightforward to solve. as far as I can tell
nusb/audio directly exposes the endpoint as a file so that audio
data may be written to it, but this highlights a fundamental
incongruence in the different ways that plan9 and usb format audio.
in my uninformed opinion it seems that the nusb/audio driver is the
right place to perform any conversions between the two, so that the
audioU* files would go through fswrite and fsread first to add and
remove padding respectively, before forwarding the audio data to/from
the actual USB endpoint.



as a nitpick, this check in parsestream leaks c:

		if(Proto(ac->csp) == Paudio1)
			continue;

it should goto Skip instead of continue.

lastly, it seems counterintuitive to me that if the first AS endpoint
we check that doesn't meet the preconditions fails any of what follows
we immediately give up. is there some sort of order guaranteed by the
spec that makes this a necessity? instead of giving up we could continue
checking the remaining candidates:

	for(; as != nil; as = as->next){
		c = emallocz(sizeof(*c), 1);
		as->aux = c;

		/* find AS endpoint */
		for(j = 0; j < nelem(as->ep); j++){
			e = as->ep[j];
			if(e != nil && e->type == Eiso && (e->attrib>>4 & 3) == Edata){
				c->ep = e;
				goto Try;
			}
		Skip:
			continue;
		}

		free(c);
		as->aux = nil;
		continue;

	Try:
		c->zb = zb;
		...

(purely illustrative example. the control flow is quite convoluted and there's probably a better
way to write it.)

to be clear this is not a problem I am experiencing in actuality,
rather simply a thought that occurred to me while reading the code.

cheers