new nusb/serial driver

new nusb/serial driver

[Eli Cohen]

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

I have written an extremely limited and flaky CDC ACM driver for
nusb/serial. it works well enough to communicate with my Arduino Mega
2560. it would be nice if someone smarter than me with similar
hardware could look at this.

[-- Attachment #2: serial.patch --]
[-- Type: text/x-patch, Size: 1756 bytes --]

Only in /usr/glenda/nusb/serial: cdc.c
diff -ur /sys/src/cmd/nusb/serial/mkfile /usr/glenda/nusb/serial/mkfile
--- /sys/src/cmd/nusb/serial/mkfile	Fri Dec 13 00:31:34 2019
+++ /usr/glenda/nusb/serial/mkfile	Mon Jun  8 18:00:33 2020
@@ -1,7 +1,7 @@
 </$objtype/mkfile
 
 TARG=serial
-OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O
+OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O cdc.$O
 HFILES=\
 	../lib/usb.h\
 	serial.h\
diff -ur /sys/src/cmd/nusb/serial/serial.c /usr/glenda/nusb/serial/serial.c
--- /sys/src/cmd/nusb/serial/serial.c	Fri Dec 13 00:31:34 2019
+++ /usr/glenda/nusb/serial/serial.c	Wed Jun 10 21:36:54 2020
@@ -621,9 +621,12 @@
 {
 	int i, epin, epout, epintr;
 	Ep *ep, **eps;
+	int oifc;
 
+	oifc = ifc;
 	epintr = epin = epout = -1;
 
+TOPFINDEPS:
 	/*
 	 * interfc 0 means start from the start which is equiv to
 	 * iterate through endpoints probably, could be done better
@@ -644,8 +647,13 @@
 		}
 	}
 	dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", ifc, epin, epout, epintr);
-	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1))
-		return -1;
+	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1)){
+		ifc++;
+		if(ifc > 1)
+			return -1;
+		goto TOPFINDEPS;
+	}
+	ifc = oifc;
 
 	if(openeps(&ser->p[ifc], epin, epout, epintr) < 0)
 		return -1;
@@ -714,6 +722,7 @@
 extern int slprobe(Serial *ser);
 extern int chprobe(Serial *ser);
 extern int uconsprobe(Serial *ser);
+extern int cdcprobe(Serial *ser);
 
 void
 threadmain(int argc, char* argv[])
@@ -750,7 +759,8 @@
 	&& ftprobe(ser)
 	&& slprobe(ser)
 	&& plprobe(ser)
-	&& chprobe(ser))
+	&& chprobe(ser)
+	&& cdcprobe(ser))
 		sysfatal("no serial devices found");
 
 	for(i = 0; i < ser->nifcs; i++){

[-- Attachment #3: cdc.c --]
[-- Type: text/x-csrc, Size: 2398 bytes --]

#include <u.h>
#include <libc.h>
#include <thread.h>
#include <fcall.h>
#include <9p.h>
#include "usb.h"
#include "serial.h"

enum {
	ScACM = 2,
	AT	= 1,
};

static int
cdcsetparam(Serialport *p)
{
	int res;
	uchar vals[7];
	Serial *ser;

	ser = p->s;

	PUT4(vals, p->baud);

	vals[4] = 0;
	if(p->stop == 1)
		vals[4] = 0;
	if(p->stop == 15)
		vals[4] = 1;
	if(p->stop == 2)
		vals[4] = 2;

	vals[5] = p->parity;

	vals[6] = p->bits;

	res = usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x20, 0, 0, vals, 7);

	return res;
}

static int
cdcinit(Serialport *p)
{
	Serial *ser;

	p->baud = 115200;
	p->bits = 8;
	p->parity = 0;
	p->stop = 1;

	ser = p->s;

	cdcsetparam(p);
	incref(ser->dev);
	return 0;
}

static int
cdcsetbreak(Serialport *p, int val)
{
	Serial *ser;

	ser = p->s;
	return usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x23, (val != 0? 0xffff: 0x0000), 0, nil, 0);
}

static int
cdcseteps(Serialport *p)
{
	devctl(p->epin, "maxpkt 256");
	devctl(p->epout, "maxpkt 256");
	return 0;
}

static int
cdcsendlines(Serialport *p)
{
	if(p->rts)
		p->ctlstate |= 0x0002;
	else
		p->ctlstate &= ~0x0002;
	if(p->dtr)
		p->ctlstate |= 0x0001;
	else
		p->ctlstate &= ~0x0001;

	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, 0x22, p->ctlstate, 0, nil, 0);
}

static int
cdcclearpipes(Serialport *p)
{
	Serial *ser;

	ser = p->s;

	if(unstall(ser->dev, p->epout, Eout) < 0)
		dprint(2, "disk: unstall epout: %r\n");
	if(unstall(ser->dev, p->epin, Ein) < 0)
		dprint(2, "disk: unstall epin: %r\n");
	if(unstall(ser->dev, p->epintr, Ein) < 0)
		dprint(2, "disk: unstall epintr: %r\n");

	return 0;
}

static int
cdcwait4data(Serialport *p, uchar *data, int count)
{
	int n;

	qunlock(p->s);
	while ((n = read(p->epin->dfd, data, count)) == 0)
		;
	qlock(p->s);

	return n;
}

static Serialops cdcops = {
	.init		= cdcinit,
	.setparam	= cdcsetparam,
	.setbreak	= cdcsetbreak,
	.seteps		= cdcseteps,
	.sendlines	= cdcsendlines,
	.clearpipes	= cdcclearpipes,
	.wait4data	= cdcwait4data,
};

int
cdcprobe(Serial *ser)
{
	int i, c;
	ulong csp;
	Usbdev *ud = ser->dev->usb;
	Ep *ep;

	c = 0;
	for (i = 0; i < nelem(ud->ep); i++) {
		if ((ep = ud->ep[i]) == nil)
			continue;
		csp = ep->iface->csp;
		if (Class(csp) == Clcomms && Subclass(csp) == ScACM && Proto(csp) == AT)
			c++;
	}

	if (c == 0)
		return -1;

	ser->nifcs = 1;
	ser->outhdrsz = 0;
	ser->hasepintr = 1;
	ser->Serialops = cdcops;
	return 0;
}

Re: [9front] new nusb/serial driver

[Alex Musolino]

You didn't include cdc.c in your patch.

Re: [9front] new nusb/serial driver

[Eli Cohen]

I wasn't sure how to do that with ape/diff, I just attached the file to my email

On Thu, Jun 11, 2020 at 12:18 AM Alex Musolino <alex@musolino.id.au> wrote:
>
> You didn't include cdc.c in your patch.



-- 
http://echoline.org

Re: [9front] new nusb/serial driver

[Alex Musolino]

> I just attached the file to my email

Did you?  I don't see it.

Re: [9front] new nusb/serial driver

[Eli Cohen]

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

I'm sending it again just in case

On Thu, Jun 11, 2020 at 12:30 AM Alex Musolino <alex@musolino.id.au> wrote:
>
> > I just attached the file to my email
>
> Did you?  I don't see it.

[-- Attachment #2: cdc.c --]
[-- Type: text/x-csrc, Size: 2398 bytes --]

#include <u.h>
#include <libc.h>
#include <thread.h>
#include <fcall.h>
#include <9p.h>
#include "usb.h"
#include "serial.h"

enum {
	ScACM = 2,
	AT	= 1,
};

static int
cdcsetparam(Serialport *p)
{
	int res;
	uchar vals[7];
	Serial *ser;

	ser = p->s;

	PUT4(vals, p->baud);

	vals[4] = 0;
	if(p->stop == 1)
		vals[4] = 0;
	if(p->stop == 15)
		vals[4] = 1;
	if(p->stop == 2)
		vals[4] = 2;

	vals[5] = p->parity;

	vals[6] = p->bits;

	res = usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x20, 0, 0, vals, 7);

	return res;
}

static int
cdcinit(Serialport *p)
{
	Serial *ser;

	p->baud = 115200;
	p->bits = 8;
	p->parity = 0;
	p->stop = 1;

	ser = p->s;

	cdcsetparam(p);
	incref(ser->dev);
	return 0;
}

static int
cdcsetbreak(Serialport *p, int val)
{
	Serial *ser;

	ser = p->s;
	return usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x23, (val != 0? 0xffff: 0x0000), 0, nil, 0);
}

static int
cdcseteps(Serialport *p)
{
	devctl(p->epin, "maxpkt 256");
	devctl(p->epout, "maxpkt 256");
	return 0;
}

static int
cdcsendlines(Serialport *p)
{
	if(p->rts)
		p->ctlstate |= 0x0002;
	else
		p->ctlstate &= ~0x0002;
	if(p->dtr)
		p->ctlstate |= 0x0001;
	else
		p->ctlstate &= ~0x0001;

	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, 0x22, p->ctlstate, 0, nil, 0);
}

static int
cdcclearpipes(Serialport *p)
{
	Serial *ser;

	ser = p->s;

	if(unstall(ser->dev, p->epout, Eout) < 0)
		dprint(2, "disk: unstall epout: %r\n");
	if(unstall(ser->dev, p->epin, Ein) < 0)
		dprint(2, "disk: unstall epin: %r\n");
	if(unstall(ser->dev, p->epintr, Ein) < 0)
		dprint(2, "disk: unstall epintr: %r\n");

	return 0;
}

static int
cdcwait4data(Serialport *p, uchar *data, int count)
{
	int n;

	qunlock(p->s);
	while ((n = read(p->epin->dfd, data, count)) == 0)
		;
	qlock(p->s);

	return n;
}

static Serialops cdcops = {
	.init		= cdcinit,
	.setparam	= cdcsetparam,
	.setbreak	= cdcsetbreak,
	.seteps		= cdcseteps,
	.sendlines	= cdcsendlines,
	.clearpipes	= cdcclearpipes,
	.wait4data	= cdcwait4data,
};

int
cdcprobe(Serial *ser)
{
	int i, c;
	ulong csp;
	Usbdev *ud = ser->dev->usb;
	Ep *ep;

	c = 0;
	for (i = 0; i < nelem(ud->ep); i++) {
		if ((ep = ud->ep[i]) == nil)
			continue;
		csp = ep->iface->csp;
		if (Class(csp) == Clcomms && Subclass(csp) == ScACM && Proto(csp) == AT)
			c++;
	}

	if (c == 0)
		return -1;

	ser->nifcs = 1;
	ser->outhdrsz = 0;
	ser->hasepintr = 1;
	ser->Serialops = cdcops;
	return 0;
}

Re: [9front] new nusb/serial driver

[Eli Cohen]

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

here's a patch made with ape/diff -N

On Thu, Jun 11, 2020 at 12:36 AM Eli Cohen <echoline@gmail.com> wrote:
>
> I'm sending it again just in case
>
> On Thu, Jun 11, 2020 at 12:30 AM Alex Musolino <alex@musolino.id.au> wrote:
> >
> > > I just attached the file to my email
> >
> > Did you?  I don't see it.

[-- Attachment #2: serial.patch --]
[-- Type: text/x-patch, Size: 3527 bytes --]

diff -N /sys/src/cmd/nusb/serial/cdc.c /usr/glenda/nusb/serial/cdc.c
0a1,156
> #include <u.h>
> #include <libc.h>
> #include <thread.h>
> #include <fcall.h>
> #include <9p.h>
> #include "usb.h"
> #include "serial.h"
> 
> enum {
> 	ScACM = 2,
> 	AT	= 1,
> };
> 
> static int
> cdcsetparam(Serialport *p)
> {
> 	int res;
> 	uchar vals[7];
> 	Serial *ser;
> 
> 	ser = p->s;
> 
> 	PUT4(vals, p->baud);
> 
> 	vals[4] = 0;
> 	if(p->stop == 1)
> 		vals[4] = 0;
> 	if(p->stop == 15)
> 		vals[4] = 1;
> 	if(p->stop == 2)
> 		vals[4] = 2;
> 
> 	vals[5] = p->parity;
> 
> 	vals[6] = p->bits;
> 
> 	res = usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x20, 0, 0, vals, 7);
> 
> 	return res;
> }
> 
> static int
> cdcinit(Serialport *p)
> {
> 	Serial *ser;
> 
> 	p->baud = 115200;
> 	p->bits = 8;
> 	p->parity = 0;
> 	p->stop = 1;
> 
> 	ser = p->s;
> 
> 	cdcsetparam(p);
> 	incref(ser->dev);
> 	return 0;
> }
> 
> static int
> cdcsetbreak(Serialport *p, int val)
> {
> 	Serial *ser;
> 
> 	ser = p->s;
> 	return usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x23, (val != 0? 0xffff: 0x0000), 0, nil, 0);
> }
> 
> static int
> cdcseteps(Serialport *p)
> {
> 	devctl(p->epin, "maxpkt 256");
> 	devctl(p->epout, "maxpkt 256");
> 	return 0;
> }
> 
> static int
> cdcsendlines(Serialport *p)
> {
> 	if(p->rts)
> 		p->ctlstate |= 0x0002;
> 	else
> 		p->ctlstate &= ~0x0002;
> 	if(p->dtr)
> 		p->ctlstate |= 0x0001;
> 	else
> 		p->ctlstate &= ~0x0001;
> 
> 	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, 0x22, p->ctlstate, 0, nil, 0);
> }
> 
> static int
> cdcclearpipes(Serialport *p)
> {
> 	Serial *ser;
> 
> 	ser = p->s;
> 
> 	if(unstall(ser->dev, p->epout, Eout) < 0)
> 		dprint(2, "disk: unstall epout: %r\n");
> 	if(unstall(ser->dev, p->epin, Ein) < 0)
> 		dprint(2, "disk: unstall epin: %r\n");
> 	if(unstall(ser->dev, p->epintr, Ein) < 0)
> 		dprint(2, "disk: unstall epintr: %r\n");
> 
> 	return 0;
> }
> 
> static int
> cdcwait4data(Serialport *p, uchar *data, int count)
> {
> 	int n;
> 
> 	qunlock(p->s);
> 	while ((n = read(p->epin->dfd, data, count)) == 0)
> 		;
> 	qlock(p->s);
> 
> 	return n;
> }
> 
> static Serialops cdcops = {
> 	.init		= cdcinit,
> 	.setparam	= cdcsetparam,
> 	.setbreak	= cdcsetbreak,
> 	.seteps		= cdcseteps,
> 	.sendlines	= cdcsendlines,
> 	.clearpipes	= cdcclearpipes,
> 	.wait4data	= cdcwait4data,
> };
> 
> int
> cdcprobe(Serial *ser)
> {
> 	int i, c;
> 	ulong csp;
> 	Usbdev *ud = ser->dev->usb;
> 	Ep *ep;
> 
> 	c = 0;
> 	for (i = 0; i < nelem(ud->ep); i++) {
> 		if ((ep = ud->ep[i]) == nil)
> 			continue;
> 		csp = ep->iface->csp;
> 		if (Class(csp) == Clcomms && Subclass(csp) == ScACM && Proto(csp) == AT)
> 			c++;
> 	}
> 
> 	if (c == 0)
> 		return -1;
> 
> 	ser->nifcs = 1;
> 	ser->outhdrsz = 0;
> 	ser->hasepintr = 1;
> 	ser->Serialops = cdcops;
> 	return 0;
> }
diff -N /sys/src/cmd/nusb/serial/mkfile /usr/glenda/nusb/serial/mkfile
4c4
< OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O
---
> OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O cdc.$O
diff -N /sys/src/cmd/nusb/serial/serial.c /usr/glenda/nusb/serial/serial.c
623a624
> 	int oifc;
624a626
> 	oifc = ifc;
626a629
> TOPFINDEPS:
647,648c650,656
< 	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1))
< 		return -1;
---
> 	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1)){
> 		ifc++;
> 		if(ifc > 1)
> 			return -1;
> 		goto TOPFINDEPS;
> 	}
> 	ifc = oifc;
716a725
> extern int cdcprobe(Serial *ser);
753c762,763
< 	&& chprobe(ser))
---
> 	&& chprobe(ser)
> 	&& cdcprobe(ser))

Re: [9front] new nusb/serial driver

[cinap_lenrek]

why are you setting the maxpkt size? we (should have) already have setup the packet size
from the endpoint descriptor when we created the endpoint. why are we overriding
it here? is this in the CDC standard?

> static int
> cdcseteps(Serialport *p)
> {
> 	devctl(p->epin, "maxpkt 256");
> 	devctl(p->epout, "maxpkt 256");
> 	return 0;
> }

--
cinap

Re: [9front] new nusb/serial driver

[Eli Cohen]

I only did that because the other drivers did it. It seems to work
just as well without that

On Thu, Jun 11, 2020 at 5:15 AM <cinap_lenrek@felloff.net> wrote:
>
> why are you setting the maxpkt size? we (should have) already have setup the packet size
> from the endpoint descriptor when we created the endpoint. why are we overriding
> it here? is this in the CDC standard?
>
> > static int
> > cdcseteps(Serialport *p)
> > {
> >       devctl(p->epin, "maxpkt 256");
> >       devctl(p->epout, "maxpkt 256");
> >       return 0;
> > }
>
> --
> cinap

Re: [9front] new nusb/serial driver

[cinap_lenrek]

> I only did that because the other drivers did it.

yeah, it could be that they'r also wrong. or the devices they support
have broken descriptors... or they did the same as you and just copied
it from another driver. :)

> It seems to work just as well without that

good. then i'd suggest we leave it out. given that cdc is a generic
standard, we should use the value the device advertises as we'r not
targeting specific chips where we know which packet sizes will work
or not.

another issue:

> 	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1)){
> 		ifc++;
> 		if(ifc > 1)
> 			return -1;
> 		goto TOPFINDEPS;
> 	}

i think this should not be done here. rather, we should do that in
the caller of findendendpoints(), and have it continue if it cant
open a interface. and only fail in the end when all interfaces
failed to be usable.

the reason is the caller already iterates over the interfaces,
and the task of this function is to bring the interface up that
was passed to it.

otherwise, everything looks good. you are almost there! :-)

--
cinap

Re: [9front] new nusb/serial driver

[Eli Cohen]

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

This driver still needs work. it's flaky, but I noticed the ftdi
driver is too...

> another issue:
>
> >       if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1)){
> >               ifc++;
> >               if(ifc > 1)
> >                       return -1;
> >               goto TOPFINDEPS;
> >       }
>
> i think this should not be done here. rather, we should do that in
> the caller of findendendpoints(), and have it continue if it cant
> open a interface. and only fail in the end when all interfaces
> failed to be usable.
>
> the reason is the caller already iterates over the interfaces,
> and the task of this function is to bring the interface up that
> was passed to it.
>

The problem I was having was that with all the other drivers there was
a 1:1 mapping of USB interfaces to serial port interfaces. see what I
mean? ifc is the index of the serial port interface used directly to
index the USB interface. I have changed the code to a loop at least.

There are some other things that need work still. the openbsd umodem
driver does some things this doesn't yet

[-- Attachment #2: serial.patch --]
[-- Type: text/x-patch, Size: 4094 bytes --]

diff -N /sys/src/cmd/nusb/serial/cdc.c nusb/serial/cdc.c
0a1,148
> #include <u.h>
> #include <libc.h>
> #include <thread.h>
> #include <fcall.h>
> #include <9p.h>
> #include "usb.h"
> #include "serial.h"
> 
> enum {
> 	ScACM	= 2,
> 	AT		= 1,
> 	None	= 0,
> };
> 
> static int
> cdcsetparam(Serialport *p)
> {
> 	int res;
> 	uchar vals[7];
> 	Serial *ser;
> 
> 	ser = p->s;
> 
> 	PUT4(vals, p->baud);
> 
> 	vals[4] = 0;
> 	if(p->stop == 1)
> 		vals[4] = 0;
> 	if(p->stop == 15)
> 		vals[4] = 1;
> 	if(p->stop == 2)
> 		vals[4] = 2;
> 
> 	vals[5] = p->parity;
> 
> 	vals[6] = p->bits;
> 
> 	res = usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x20, 0, 0, vals, 7);
> 
> 	return res;
> }
> 
> static int
> cdcinit(Serialport *p)
> {
> 	Serial *ser;
> 
> 	p->baud = 115200;
> 	p->bits = 8;
> 	p->parity = 0;
> 	p->stop = 1;
> 
> 	ser = p->s;
> 
> 	cdcsetparam(p);
> 	incref(ser->dev);
> 	return 0;
> }
> 
> static int
> cdcsetbreak(Serialport *p, int val)
> {
> 	Serial *ser;
> 
> 	ser = p->s;
> 	return usbcmd(ser->dev, Rh2d | Rclass | Riface, 0x23, (val != 0? 0xffff: 0x0000), 0, nil, 0);
> }
> 
> static int
> cdcsendlines(Serialport *p)
> {
> 	if(p->rts)
> 		p->ctlstate |= 0x0002;
> 	else
> 		p->ctlstate &= ~0x0002;
> 	if(p->dtr)
> 		p->ctlstate |= 0x0001;
> 	else
> 		p->ctlstate &= ~0x0001;
> 
> 	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, 0x22, p->ctlstate, 0, nil, 0);
> }
> 
> static int
> cdcclearpipes(Serialport *p)
> {
> 	Serial *ser;
> 
> 	ser = p->s;
> 
> 	if(unstall(ser->dev, p->epout, Eout) < 0)
> 		dprint(2, "serial: unstall epout: %r\n");
> 	if(unstall(ser->dev, p->epin, Ein) < 0)
> 		dprint(2, "serial: unstall epin: %r\n");
> 	if(unstall(ser->dev, p->epintr, Ein) < 0)
> 		dprint(2, "serial: unstall epintr: %r\n");
> 
> 	return 0;
> }
> 
> static int
> cdcwait4data(Serialport *p, uchar *data, int count)
> {
> 	int n;
> 
> 	qunlock(p->s);
> 	while ((n = read(p->epin->dfd, data, count)) == 0)
> 		;
> 	qlock(p->s);
> 
> 	return n;
> }
> 
> static Serialops cdcops = {
> 	.init		= cdcinit,
> 	.setparam	= cdcsetparam,
> 	.setbreak	= cdcsetbreak,
> 	.sendlines	= cdcsendlines,
> 	.clearpipes	= cdcclearpipes,
> 	.wait4data	= cdcwait4data,
> };
> 
> int
> cdcprobe(Serial *ser)
> {
> 	int i, c;
> 	ulong csp;
> 	Usbdev *ud = ser->dev->usb;
> 	Ep *ep;
> 
> 	c = 0;
> 	for (i = 0; i < nelem(ud->ep); i++) {
> 		if ((ep = ud->ep[i]) == nil)
> 			continue;
> 		csp = ep->iface->csp;
> 		if (Class(csp) == Clcomms && Subclass(csp) == ScACM && Proto(csp) == AT)
> 			c++;
> 	}
> 
> 	if (c == 0)
> 		return -1;
> 
> 	ser->nifcs = 1;
> 	ser->outhdrsz = 0;
> 	ser->hasepintr = 1;
> 	ser->Serialops = cdcops;
> 	return 0;
> }
diff -N /sys/src/cmd/nusb/serial/mkfile nusb/serial/mkfile
4c4
< OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O
---
> OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O cdc.$O
diff -N /sys/src/cmd/nusb/serial/serial.c nusb/serial/serial.c
623a624
> 	int oifc;
624a626
> 	oifc = ifc;
631c633,634
< 	eps = ser->dev->usb->conf[0]->iface[ifc]->ep;
---
> 	do {
> 		eps = ser->dev->usb->conf[0]->iface[oifc]->ep;
633,643c636,647
< 	for(i = 0; i < Nep; i++){
< 		if((ep = eps[i]) == nil)
< 			continue;
< 		if(ser->hasepintr && ep->type == Eintr &&
< 		    ep->dir == Ein && epintr == -1)
< 			epintr = ep->id;
< 		if(ep->type == Ebulk){
< 			if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
< 				epin = ep->id;
< 			if((ep->dir == Eout || ep->dir == Eboth) && epout == -1)
< 				epout = ep->id;
---
> 		for(i = 0; i < Nep; i++){
> 			if((ep = eps[i]) == nil)
> 				continue;
> 			if(ser->hasepintr && ep->type == Eintr &&
> 			    ep->dir == Ein && epintr == -1)
> 				epintr = ep->id;
> 			if(ep->type == Ebulk){
> 				if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
> 					epin = ep->id;
> 				if((ep->dir == Eout || ep->dir == Eboth) && epout == -1)
> 					epout = ep->id;
> 			}
645c649
< 	}
---
> 	} while((epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1)) && ++oifc < Niface);
716a721
> extern int cdcprobe(Serial *ser);
753c758,759
< 	&& chprobe(ser))
---
> 	&& chprobe(ser)
> 	&& cdcprobe(ser))

Re: [9front] new nusb/serial driver

[cinap_lenrek]

i see. we need to stop assuming that the port index is equivalent
to the usb interface index. This appears to be consistent with
the code in ftgettype():

	ser->nifcs = 0;
	for(i = 0; i < Niface; i++)
		if(cnf->iface[i] != nil)
			ser->nifcs++;

how about this?

diff -r 307fbb6fd10a sys/src/cmd/nusb/serial/serial.c
--- a/sys/src/cmd/nusb/serial/serial.c	Fri Jun 12 01:36:50 2020 +0200
+++ b/sys/src/cmd/nusb/serial/serial.c	Fri Jun 12 19:58:17 2020 +0200
@@ -617,19 +617,17 @@
 }
 
 static int
-findendpoints(Serial *ser, int ifc)
+findendpoints(Serialport *p)
 {
 	int i, epin, epout, epintr;
 	Ep *ep, **eps;
+	Serial *ser;
 
+	ser = p->s;
+	if(ser->dev->usb->conf[0]->iface[p->interfc] == nil)
+		return -1;
+	eps = ser->dev->usb->conf[0]->iface[p->interfc]->ep;
 	epintr = epin = epout = -1;
-
-	/*
-	 * interfc 0 means start from the start which is equiv to
-	 * iterate through endpoints probably, could be done better
-	 */
-	eps = ser->dev->usb->conf[0]->iface[ifc]->ep;
-
 	for(i = 0; i < Nep; i++){
 		if((ep = eps[i]) == nil)
 			continue;
@@ -643,22 +641,22 @@
 				epout = ep->id;
 		}
 	}
-	dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", ifc, epin, epout, epintr);
+	dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", p->interfc, epin, epout, epintr);
 	if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1))
 		return -1;
 
-	if(openeps(&ser->p[ifc], epin, epout, epintr) < 0)
+	if(openeps(p, epin, epout, epintr) < 0)
 		return -1;
 
-	dprint(2, "serial: ep in %s out %s\n", ser->p[ifc].epin->dir, ser->p[ifc].epout->dir);
+	dprint(2, "serial: ep in %s out %s\n", p->epin->dir, p->epout->dir);
 	if(ser->hasepintr)
-		dprint(2, "serial: ep intr %s\n", ser->p[ifc].epintr->dir);
+		dprint(2, "serial: ep intr %s\n", p->epintr->dir);
 
 	if(usbdebug > 1 || serialdebug > 2){
-		devctl(ser->p[ifc].epin,  "debug 1");
-		devctl(ser->p[ifc].epout, "debug 1");
+		devctl(p->epin,  "debug 1");
+		devctl(p->epout, "debug 1");
 		if(ser->hasepintr)
-			devctl(ser->p[ifc].epintr, "debug 1");
+			devctl(p->epintr, "debug 1");
 		devctl(ser->dev, "debug 1");
 	}
 	return 0;
@@ -722,7 +720,7 @@
 	Dev *dev;
 	char buf[50];
 	Serialport *p;
-	int i;
+	int i, ifc;
 
 	ARGBEGIN{
 	case 'd':
@@ -753,18 +751,23 @@
 	&& chprobe(ser))
 		sysfatal("no serial devices found");
 
-	for(i = 0; i < ser->nifcs; i++){
+	i = 0;
+	for(ifc = 0; ifc < Niface; ifc++) {
 		p = &ser->p[i];
+		p->s = ser;
 		p->baud = ~0;
-		p->interfc = i;
-		p->s = ser;
-		if(i == ser->jtag)
-			p->isjtag++;
-		if(findendpoints(ser, i) < 0)
-			sysfatal("no endpoints found for ifc %d", i);
+		p->isjtag = (i == ser->jtag);
+		p->interfc = ifc;
+		if(findendpoints(p) < 0)
+			continue;
 		p->w4data  = chancreate(sizeof(ulong), 0);
 		p->gotdata = chancreate(sizeof(ulong), 0);
+		if(++i >= ser->nifcs)
+			break;
 	}
+	if(i == 0)
+		sysfatal("no endpoints found");
+	ser->nifcs = i;
 
 	qlock(ser);
 	serialreset(ser);

--
cinap

Re: [9front] new nusb/serial driver

[Eli Cohen]

I think you need to preserve the indices epintr, epin, and epout in
findendpoints. The way it was working, on the first call it would find
epintr, then epin and epout were on a difference USB iface. between
multiple calls epintr would get reset to -1.

On Fri, Jun 12, 2020 at 11:03 AM <cinap_lenrek@felloff.net> wrote:
>
> i see. we need to stop assuming that the port index is equivalent
> to the usb interface index. This appears to be consistent with
> the code in ftgettype():
>
>         ser->nifcs = 0;
>         for(i = 0; i < Niface; i++)
>                 if(cnf->iface[i] != nil)
>                         ser->nifcs++;
>
> how about this?
>
> diff -r 307fbb6fd10a sys/src/cmd/nusb/serial/serial.c
> --- a/sys/src/cmd/nusb/serial/serial.c  Fri Jun 12 01:36:50 2020 +0200
> +++ b/sys/src/cmd/nusb/serial/serial.c  Fri Jun 12 19:58:17 2020 +0200
> @@ -617,19 +617,17 @@
>  }
>
>  static int
> -findendpoints(Serial *ser, int ifc)
> +findendpoints(Serialport *p)
>  {
>         int i, epin, epout, epintr;
>         Ep *ep, **eps;
> +       Serial *ser;
>
> +       ser = p->s;
> +       if(ser->dev->usb->conf[0]->iface[p->interfc] == nil)
> +               return -1;
> +       eps = ser->dev->usb->conf[0]->iface[p->interfc]->ep;
>         epintr = epin = epout = -1;
> -
> -       /*
> -        * interfc 0 means start from the start which is equiv to
> -        * iterate through endpoints probably, could be done better
> -        */
> -       eps = ser->dev->usb->conf[0]->iface[ifc]->ep;
> -
>         for(i = 0; i < Nep; i++){
>                 if((ep = eps[i]) == nil)
>                         continue;
> @@ -643,22 +641,22 @@
>                                 epout = ep->id;
>                 }
>         }
> -       dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", ifc, epin, epout, epintr);
> +       dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", p->interfc, epin, epout, epintr);
>         if(epin == -1 || epout == -1 || (ser->hasepintr && epintr == -1))
>                 return -1;
>
> -       if(openeps(&ser->p[ifc], epin, epout, epintr) < 0)
> +       if(openeps(p, epin, epout, epintr) < 0)
>                 return -1;
>
> -       dprint(2, "serial: ep in %s out %s\n", ser->p[ifc].epin->dir, ser->p[ifc].epout->dir);
> +       dprint(2, "serial: ep in %s out %s\n", p->epin->dir, p->epout->dir);
>         if(ser->hasepintr)
> -               dprint(2, "serial: ep intr %s\n", ser->p[ifc].epintr->dir);
> +               dprint(2, "serial: ep intr %s\n", p->epintr->dir);
>
>         if(usbdebug > 1 || serialdebug > 2){
> -               devctl(ser->p[ifc].epin,  "debug 1");
> -               devctl(ser->p[ifc].epout, "debug 1");
> +               devctl(p->epin,  "debug 1");
> +               devctl(p->epout, "debug 1");
>                 if(ser->hasepintr)
> -                       devctl(ser->p[ifc].epintr, "debug 1");
> +                       devctl(p->epintr, "debug 1");
>                 devctl(ser->dev, "debug 1");
>         }
>         return 0;
> @@ -722,7 +720,7 @@
>         Dev *dev;
>         char buf[50];
>         Serialport *p;
> -       int i;
> +       int i, ifc;
>
>         ARGBEGIN{
>         case 'd':
> @@ -753,18 +751,23 @@
>         && chprobe(ser))
>                 sysfatal("no serial devices found");
>
> -       for(i = 0; i < ser->nifcs; i++){
> +       i = 0;
> +       for(ifc = 0; ifc < Niface; ifc++) {
>                 p = &ser->p[i];
> +               p->s = ser;
>                 p->baud = ~0;
> -               p->interfc = i;
> -               p->s = ser;
> -               if(i == ser->jtag)
> -                       p->isjtag++;
> -               if(findendpoints(ser, i) < 0)
> -                       sysfatal("no endpoints found for ifc %d", i);
> +               p->isjtag = (i == ser->jtag);
> +               p->interfc = ifc;
> +               if(findendpoints(p) < 0)
> +                       continue;
>                 p->w4data  = chancreate(sizeof(ulong), 0);
>                 p->gotdata = chancreate(sizeof(ulong), 0);
> +               if(++i >= ser->nifcs)
> +                       break;
>         }
> +       if(i == 0)
> +               sysfatal("no endpoints found");
> +       ser->nifcs = i;
>
>         qlock(ser);
>         serialreset(ser);
>
> --
> cinap

Re: [9front] new nusb/serial driver

[cinap_lenrek]

what makes you think that?

the only driver using interrupt endpoints is the prolific driver, and it is
used for getting serial line status for a port. if you have multiple ports,
on different interfaces how are you going to know which port the status
is for?

prolific driver only supports one iface. the previous code would *NOT* do
multiple calls to findendpoints() because nifc == 1. and it would sysfatal()
if findendpoints() failed..

so my assumption is that the interrupt endpoint is indeed per interface.

does this make sense?

--
cinap

Re: [9front] new nusb/serial driver

[Eli Cohen]

hi cinap,

I need to work on this driver more still, and look at the openbsd
driver closer. I'm not even using the interrupt yet but it seems to
work with this device.

what I meant was, epintr is found on the first USB interface but epin
and epout are on the second USB interface, but I can't just set nifcs
= 2 because it's only one serial port. there is one serial port using
*two* USB interfaces, one for the (unused) interrupt endpoint, and the
next one for in/out

- echoline

On Sat, Jun 13, 2020 at 7:04 AM <cinap_lenrek@felloff.net> wrote:
>
> what makes you think that?
>
> the only driver using interrupt endpoints is the prolific driver, and it is
> used for getting serial line status for a port. if you have multiple ports,
> on different interfaces how are you going to know which port the status
> is for?
>
> prolific driver only supports one iface. the previous code would *NOT* do
> multiple calls to findendpoints() because nifc == 1. and it would sysfatal()
> if findendpoints() failed..
>
> so my assumption is that the interrupt endpoint is indeed per interface.
>
> does this make sense?
>
> --
> cinap

Re: [9front] new nusb/serial driver

[cinap_lenrek]

ok, but this is completely new behaviour. i took a look at the usbcdc11.pdf
and it describes a mechanism how different interfaces are linked together
to one functional unit in section 5.2.3.5 (Union Functional Descriptor).

instead of randomly picking a intr endpoint from some other random interface,
you need to figure out how the standard mechanism works.

this will also help you making sense of the openbsd driver.

remember, cdc is a generic standard. there could indeed be devices having
multiple ports.

--
cinap