[9front] [patch] nusb/audio /dev entries fix v2

[9front] [patch] nusb/audio /dev entries fix v2

[Arne Meyer]

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

Hello,

the nusb/audio driver changes the namespace at 2 positions. It adds entries 
for audio and audioin in #u and an entry for volume in #σ/usb. So depending on
where #A, #u and #σ are bound in the namespace parts of nusb/audio will shadow or be shadowed
by #A.

Consensus on the mailing was to not mess with the bind order and let the user decide
what to do with nusb/audio. To make this easier and avoid any ambiguities I propose
to add a suffix U<N> to all of nusb/audio's device entries.

This is how the device entries look for me with the attached patch:

/dev/audioU83862
/dev/volumeU83862

I can now bind them on /dev/audio and /dev/volume in termrc or use the attached script to automatically 
bind any usb audio devices when they are plugged in. We also get as many usb audio devices as we want.

Greetings,
Arne

[-- Attachment #2: nusbaudio.patch --]
[-- Type: application/octet-stream, Size: 1250 bytes --]

diff 34e7b54c139826b55713702910a1280e6651df60 uncommitted
--- a/sys/man/4/nusb
+++ b/sys/man/4/nusb
@@ -274,6 +274,7 @@
 .BR /dev ),
 compatible with
 .IR audio (3).
+The files have a suffix UN, with N being the USB device identifier.
 .SS Camera devices
 .I Cam
 configures and exposes a USB camera device's capabilities,
--- a/sys/src/cmd/nusb/audio/audio.c
+++ b/sys/src/cmd/nusb/audio/audio.c
@@ -365,6 +365,7 @@
 	int dir = e->dir;
 	Aconf *c;
 	Range *f;
+	char buf[32];
 
 	for(;e != nil; e = e->next){
 		c = e->iface->aux;
@@ -395,9 +396,12 @@
 	devctl(d, "sampledelay %d", audiodelay);
 	devctl(d, "hz %d", speed);
 	if(e->dir==Ein)
-		devctl(d, "name audioin");
+		snprint(buf, sizeof buf, "name audioinU%s", audiodev->hname); 
 	else
-		devctl(d, "name audio");
+		snprint(buf, sizeof buf, "name audioU%s", audiodev->hname);
+	
+	devctl(d, buf);
+
 	return d;
 }
 
@@ -549,7 +553,8 @@
 		sysfatal("no output stream found");
 
 	fs.tree = alloctree(user, "usb", DMDIR|0555, nil);
-	createfile(fs.tree->root, "volume", user, 0666, nil);
+	snprint(buf, sizeof buf, "volumeU%s", audiodev->hname);
+	createfile(fs.tree->root, buf, user, 0666, nil);
 
 	snprint(buf, sizeof buf, "%d.audio", audiodev->id);
 	postsharesrv(&fs, nil, "usb", buf);

[-- Attachment #3: audiobind.rc --]
[-- Type: application/octet-stream, Size: 363 bytes --]

#!/bin/rc

if(! test -e /dev/audio)
	aux/stub /dev/audio
if(! test -e /dev/volume)
	aux/stub /dev/volume

@{
	rfork e
	fn attach {
		switch($4){
		case *01
			sleep 2
			bind /dev/audioU$5 /dev/audio
			bind /dev/volumeU$5 /dev/volume
		}
	}

	fn detach {
		switch($4){
		case *01
			unmount /dev/audio
			unmount /dev/volume
		}
	}

	rc < '#σ/usb/usbevent' &
}

Re: [9front] [patch] nusb/audio /dev entries fix v2

[cinap_lenrek]

>	int dir = e->dir;
> 	Aconf *c;
> 	Range *f;
>+	char buf[32];
> 
> 	for(;e != nil; e = e->next){
> 		c = e->iface->aux;
>@@ -395,9 +396,12 @@
> 	devctl(d, "sampledelay %d", audiodelay);
> 	devctl(d, "hz %d", speed);
> 	if(e->dir==Ein)
>-		devctl(d, "name audioin");
>+		snprint(buf, sizeof buf, "name audioinU%s", audiodev->hname); 
> 	else
>-		devctl(d, "name audio");
>+		snprint(buf, sizeof buf, "name audioU%s", audiodev->hname);
>+	
>+	devctl(d, buf);
>+

devctl() takes a format string, so the temporary buffer is not needed.
(also avoiding format string injection from hname).

the idea is good tho.

one catch i can see is what happens when you dont have a built-in
soundcard (#A) at all?

then you have no place to bind it to :(

maybe a solution could be to have this handled by mixfs?

like we could add an argument to mixfs to pass the name of
the audio device (defaulting to /dev/audio).

or even have mixfs being able to switch output devices with
a volue message by reading /dev/audio*.

if you don't want to use mixfs, you can use aux/stub
to the rescue.

let me look into mixfs solution...

--
cinap

Re: [9front] [patch] nusb/audio /dev entries fix v2

[Arne Meyer]

> cinap_lenrek@felloff.net hat am 29.12.2023 18:10 CET geschrieben:

> one catch i can see is what happens when you dont have a built-in
> soundcard (#A) at all?
> 
> then you have no place to bind it to :(
> 
This is up to the user. Either use aux/stub and bind on that or use the script I attached which takes care of that.

> maybe a solution could be to have this handled by mixfs?
> 
I liked Sigrid's solution: There should be a new program that sits between the devices like #A, nusb and mixfs and supplies a file system compatible with a #A. Something like this:

provide a file system like #A
try to access #A and use that as the default audio device. If not, discard all data.
Listen on usbevent and add all found devices to a  list.
Change the default audio device via some ctl command.

Re: [9front] [patch] nusb/audio /dev entries fix v2

[cinap_lenrek]

> I liked Sigrid's solution: There should be a new program that sits between the devices
> like #A, nusb and mixfs and supplies a file system compatible with a #A. Something like this:

isnt that basically what mixfs does?

anyway, my concern about this change is that it will
break an existing usecase where when you plug in
usb soundcard that it becomes the default,
without any other action needed.

i agree that the way we do it right now makes
it impossible to have multiple usb soundcards.

but i really would like to preserve the property
that stuff will just freaking work.

so here you go:

http://felloff.net/usr/cinap_lenrek/mixfs.diff

on startup, it scans /dev/audio* which will pick
up usb soundcards as well. (here, now it just
freaking works!).

the active audio device is available in /dev/volume:

term% cat /dev/volume
dev /dev/audio
mix 100 100

you can then change it like:

echo dev /dev/audioUxxxx > /dev/volume

even while audio is currently playing.

when the device errors on write
(like when you pull the usb soundcard),
it will rescan /dev/audio* and switch to
whatever it finds first.

--
cinap

Re: [9front] [patch] nusb/audio /dev entries fix v2

[hiro]

> anyway, my concern about this change is that it will
> break an existing usecase where when you plug in
> usb soundcard that it becomes the default,
> without any other action needed.

IIUC this is what audiobind.rc supplied in his original mail provides.

> but i really would like to preserve the property
> that stuff will just freaking work.

sure. sane defaults are important. but they can still be implemented
in rc (in a user configurable file).

> you can then change it like:
>
> echo dev /dev/audioUxxxx > /dev/volume
>
> even while audio is currently playing.

yeah that is super nice.

Re: [9front] [patch] nusb/audio /dev entries fix v2

[Arne Meyer]

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


> so here you go:
> 
> http://felloff.net/usr/cinap_lenrek/mixfs.diff
> 

Very nice. Works for me.

Here is the new patch that uses format strings in devctl.

Greetings,
Arne

[-- Attachment #2: nusbaudio.patch --]
[-- Type: application/octet-stream, Size: 796 bytes --]

diff 34e7b54c139826b55713702910a1280e6651df60 uncommitted
--- a/sys/src/cmd/nusb/audio/audio.c
+++ b/sys/src/cmd/nusb/audio/audio.c
@@ -395,9 +395,9 @@
 	devctl(d, "sampledelay %d", audiodelay);
 	devctl(d, "hz %d", speed);
 	if(e->dir==Ein)
-		devctl(d, "name audioin");
+		devctl(d, "name audioinU%s", audiodev->hname);
 	else
-		devctl(d, "name audio");
+		devctl(d, "name audioU%s", audiodev->hname);	
 	return d;
 }
 
@@ -549,7 +549,8 @@
 		sysfatal("no output stream found");
 
 	fs.tree = alloctree(user, "usb", DMDIR|0555, nil);
-	createfile(fs.tree->root, "volume", user, 0666, nil);
+	snprint(buf, sizeof buf, "volumeU%s", audiodev->hname);
+	createfile(fs.tree->root, buf, user, 0666, nil);
 
 	snprint(buf, sizeof buf, "%d.audio", audiodev->id);
 	postsharesrv(&fs, nil, "usb", buf);

Re: [9front] [patch] nusb/audio /dev entries fix v2

[Arne Meyer]

So,

I've been running the mixfs patch with my updated patch. It works really well, at least for me.
Can both be committed or are there any concerns about this?

> Arne Meyer <meyer.arne83@netcologne.de> hat am 30.12.2023 14:25 CET geschrieben:
> 
>  
> > so here you go:
> > 
> > http://felloff.net/usr/cinap_lenrek/mixfs.diff
> > 
> 
> Very nice. Works for me.
> 
> Here is the new patch that uses format strings in devctl.
> 
> Greetings,
> Arne

Re: [9front] [patch] nusb/audio /dev entries fix v2

[cinap_lenrek]

sorry, i was busy debigging kernel crashes in the scheduler.

for mixfs, i was waiting because i was not happy with the
fact that you can specify arbitrary files for the new
audio device which could be a security issue.

for now, i'll restrict the audio device name to /dev/audio*,
#u/audio* and #A/audio*...

the new mixfs and updated manpage has been pushed now.

i'll apply your nusb/audio patch next.

--
cinap

Re: [9front] [patch] nusb/audio /dev entries fix v2

[cinap_lenrek]

pushed.

--
cinap

Re: [9front] [patch] nusb/audio /dev entries fix v2

[Arne Meyer]

> cinap_lenrek@felloff.net hat am 06.01.2024 20:43 CET geschrieben:
> 
>  
> sorry, i was busy debigging kernel crashes in the scheduler.
> 
no worries, I appreciate your help. cheers.
> --
> cinap