cvs commit: src/sys/isa psm.c

[Brian Somers]

On Sat, 21 Aug 2004 14:07:44 -0600, "Justin T. Gibbs" <gibbs at scsiguy.com> wrote:
> Since the psm driver performs sync detection and out-of-sync discard,
> it seems to me that moused should not be trying to do the same thing.
> Outside of doing that, changing the PSM driver to export a zero
> syncmask anytime NEED_SYNC is set will ensure that moused doesn't
> get confused.
> 
> Index: psm.c
> ===================================================================
> RCS file: /usr/cvs/src/sys/isa/psm.c,v
> retrieving revision 1.79
> diff -d -u -r1.79 psm.c
> --- psm.c	17 Aug 2004 18:12:37 -0000	1.79
> +++ psm.c	21 Aug 2004 20:06:40 -0000
> @@ -1752,6 +1752,10 @@
>      case MOUSE_GETMODE:
>  	s = spltty();
>          *(mousemode_t *)addr = sc->mode;
> +        if ((sc->flags & PSM_NEED_SYNCBITS) != 0) {
> +            ((mousemode_t *)addr)->syncmask[0] = 0;
> +            ((mousemode_t *)addr)->syncmask[1] = 0;
> +        }
>          ((mousemode_t *)addr)->resolution = 
>  	    MOUSE_RES_LOW - sc->mode.resolution;
>  	switch (sc->mode.level) {

This won't work for the moused cases I quoted previously:

    if (pBufP == 0 && (rBuf & cur_proto[0]) != cur_proto[1])
        return 0;

    /* is there an extra data byte? */
    if (pBufP >= cur_proto[4] && (rBuf & cur_proto[0]) != cur_proto[1])
    {

This code is trying to do something (detect a forth data byte?), but
without specs for any of this stuff, I have no idea if it's doing it
correctly...

It looks like cur_proto[4] contains the length of a normal data packet.
It also looks like moused uses (rBuf & cur_proto[0]) != cur_proto[1]
to decide if this is the first byte of a new packet.

My guess is that the first ``if'' above is checking to see if we're
expecting the first byte, and if it's not a first byte we're dropping
it.

The second ``if'' above is checking to see if we've already read an
entire packet, **but** the next byte isn't a ``first byte'', therefore
we must assume it's an extra byte for this packet.

So the psm.c 1.79 changes have essentially broken moused's method of
determining the first byte.

Sooo, the only real way of fixing this is to change moused's
read(rodent.mfd, ...) so that it actually reads a packet instead
of a byte at a time.  Then it could stop mucking about with
re-packetising something that's already been done by psm.c, and
pass the whole packet in one go to r_protocol() (hey!  we can also
get rid of all those horrible static variables in r_protocol()).

How about I look at doing that?  I'll change psm so that sc->queue
keeps the concept of packets and psmread() hands back one packet at
a time (if there's room in the target), cutting the read short at the
next packet boundary.  Both old and new moused(8) binaries should
work...

> > BTW, while I was in there I noticed this:
> > 
> >     if (sc->config & PSM_CONFIG_NOCHECKSYNC)
> >         sc->dflt_mode.syncmask[0] = 0;
> >     else
> >         sc->dflt_mode.syncmask[0] = vendortype[i].syncmask;
> >     if (sc->config & PSM_CONFIG_FORCETAP)
> >         sc->mode.syncmask[0] &= ~MOUSE_PS2_TAP;
> >         ^^^^^^^^^^^^^^^^^^^^
> >     sc->dflt_mode.syncmask[1] = 0;      /* syncbits */
> >     sc->mode = sc->dflt_mode;
> >     sc->mode.packetsize = vendortype[i].packetsize;
> > 
> > which looks somewhat bogus as the whole sc->mode structure is overwritten
> > two lines later!  Perhaps it should be dflt_mode.syncmask[0] that's tweaked?
> 
> Yes, it is bogus.

I'll change it so that it updates dflt_mode instead.  I'll ask cvs who added
PSM_CONFIG_FORCETAP - it doesn't look very well tested.

> > Also, the use of sc->syncerrors seems broken.  It's never reset to zero...
> 
> Its reset to zero if a mouse packet isn't completed before a packet
> timeout fires.  It seems that the counter should be reset in other cases
> too, but I would need to study the driver further before changing that.
> is not received withing

I can do that.  It's been bothering me for some time that I've got a 3-4
second delay before my mouse recovers when I switch (windows has no delay!).
It looks as if keeping syncerrors sane will help there.

Cheers.

-- 
Brian <brian at Awfulhak.org>                        <brian@[uk.]FreeBSD.org>
      <http://www.Awfulhak.org>                   <brian@[uk.]OpenBSD.org>
Don't _EVER_ lose your sense of humour !