I've been meaning to get around to reviewing your patch (I'm glad to see you taking it on!), but have been pretty busy lately. Sorry for the delay...
On Thu, Mar 22, 2012 at 12:37 PM, Nikolai Kondrashov <spbn...@gmail.com> wrote: > Use tilt range and, optionally, resolution reported by the kernel for event > devices. > > Temporarily center the range on zero until kernel is fixed to do this. > > Use resolution, if present, to determine the physical range and clamp/extend > it to 60 degrees either way as expected by applications, otherwise assume it > corresponds to this range. > > This is needed to handle non-Wacom tablets reporting zero-centered tilt and > having physical range greater than 60 degrees. > --- > > This is in continuation of the "Tilt value meaning" thread: > http://sf.net/mailarchive/forum.php?thread_name=4F5F6C72.8080906%40gmail.com&forum_name=linuxwacom-devel > > This is needed to support Waltop Sirius Battery Free Tablet patches: > http://thread.gmane.org/gmane.linux.kernel.input/24153 > > src/wcmCommon.c | 8 ++++- > src/wcmFilter.c | 16 +++++----- > src/wcmISDV4.c | 12 +++++++- > src/wcmUSB.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++- > src/xf86Wacom.c | 8 +++--- > src/xf86WacomDefs.h | 11 ++++++- > 6 files changed, 109 insertions(+), 20 deletions(-) > > diff --git a/src/wcmCommon.c b/src/wcmCommon.c > index 7199105..1d66209 100644 > --- a/src/wcmCommon.c > +++ b/src/wcmCommon.c > @@ -1466,8 +1466,12 @@ WacomCommonPtr wcmNewCommon(void) > common->wcmMaxTouchY = 1024; /* max touch Y value */ > common->wcmMaxStripX = 4096; /* Max fingerstrip X */ > common->wcmMaxStripY = 4096; /* Max fingerstrip Y */ > - common->wcmMaxtiltX = 128; /* Max tilt in X directory */ > - common->wcmMaxtiltY = 128; /* Max tilt in Y directory */ > + common->wcmTiltXOff = -64; /* Tilt offset in X direction */ > + common->wcmTiltXMin = -64; /* Min tilt in X direction */ > + common->wcmTiltXMax = 64; /* Max tilt in X direction */ > + common->wcmTiltYOff = -64; /* Tilt offset in Y direction */ > + common->wcmTiltYMin = -64; /* Min tilt in Y direction */ > + common->wcmTiltYMax = 64; /* Max tilt in Y direction */ There's no need to initialize any of these variables here. Instead, they should be initialized in usbWcmGetRanges. > common->wcmCursorProxoutDistDefault = PROXOUT_INTUOS_DISTANCE; > /* default to Intuos */ > common->wcmSuppress = DEFAULT_SUPPRESS; > diff --git a/src/wcmFilter.c b/src/wcmFilter.c > index 47e958a..3802857 100644 > --- a/src/wcmFilter.c > +++ b/src/wcmFilter.c > @@ -298,16 +298,16 @@ int wcmFilterCoord(WacomCommonPtr common, > WacomChannelPtr pChannel, > ds->device_type == ERASER_ID)) > { > ds->tiltx = tx / common->wcmRawSample; > - if (ds->tiltx > common->wcmMaxtiltX/2-1) > - ds->tiltx = common->wcmMaxtiltX/2-1; > - else if (ds->tiltx < -common->wcmMaxtiltX/2) > - ds->tiltx = -common->wcmMaxtiltX/2; > + if (ds->tiltx > common->wcmTiltXMax) > + ds->tiltx = common->wcmTiltXMax; > + else if (ds->tiltx < common->wcmTiltXMin) > + ds->tiltx = common->wcmTiltXMin; > > ds->tilty = ty / common->wcmRawSample; > - if (ds->tilty > common->wcmMaxtiltY/2-1) > - ds->tilty = common->wcmMaxtiltY/2-1; > - else if (ds->tilty < -common->wcmMaxtiltY/2) > - ds->tilty = -common->wcmMaxtiltY/2; > + if (ds->tilty > common->wcmTiltYMax) > + ds->tilty = common->wcmTiltYMax; > + else if (ds->tilty < common->wcmTiltYMin) > + ds->tilty = common->wcmTiltYMin; > } > > return 0; /* lookin' good */ Why do we bother doing clamping in this function? git-blame indicates that the behavior is as old as the driver, but I can't find any compelling reason for keeping it around. > diff --git a/src/wcmISDV4.c b/src/wcmISDV4.c > index 892fbff..5aa68c5 100644 > --- a/src/wcmISDV4.c > +++ b/src/wcmISDV4.c > @@ -405,8 +405,16 @@ static int isdv4GetRanges(InputInfoPtr pInfo) > common->wcmMaxY = reply.y_max; > if (reply.tilt_x_max && reply.tilt_y_max) > { > - common->wcmMaxtiltX = reply.tilt_x_max; > - common->wcmMaxtiltY = reply.tilt_y_max; > + common->wcmTiltXOff = -reply.tilt_x_max/2; > + common->wcmTiltXMin = 0 + common->wcmTiltXOff; > + common->wcmTiltXMax = reply.tilt_x_max + > + common->wcmTiltXOff; > + > + common->wcmTiltYOff = -reply.tilt_y_max/2; > + common->wcmTiltYMin = 0 + common->wcmTiltYOff; > + common->wcmTiltYMax = reply.tilt_y_max + > + common->wcmTiltYOff; > + > common->wcmFlags |= TILT_ENABLED_FLAG; > } > See my comments on USB below, and apply as necessary up here :) > diff --git a/src/wcmUSB.c b/src/wcmUSB.c > index d41ced6..1d2e58a 100644 > --- a/src/wcmUSB.c > +++ b/src/wcmUSB.c > @@ -573,6 +573,76 @@ int usbWcmGetRanges(InputInfoPtr pInfo) > common->wcmMaxStripY = absinfo.maximum; > } > > + /* X tilt range */ > + if (ISBITSET(abs, ABS_TILT_X) && > + !ioctl(pInfo->fd, EVIOCGABS(ABS_TILT_X), &absinfo)) > + { > + /* Center the reported range on zero */ > + /* FIXME remove once kernel is fixed */ > + common->wcmTiltXOff = - (absinfo.minimum + absinfo.maximum) / > 2; I would move this line into the 'else' block below as a reasonable fallback. For the 'if' block, it should be set to '0' and assumed that the hardware is serious about the logical<->physical reslationship implied by the resolution. > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,30) > + /* If resolution is specified */ > + if (absinfo.resolution > 0) > + { > + int max_angle = round(MAX_TILT_ANGLE * > + absinfo.resolution); > + Two things: 1) MAX_TILT_ANGLE is in radians. To get a max_angle in degrees (which applications expect) absinfo.resolution *must* be in degrees/radian. This is a completely nonsensical unit for resolution. 2) The maximum angle does not take into account the value of absinfo.maximum. Instead, it appears to depend entirely on the value of absinfo.resolution. What you really want is to have absinfo.resolution relate the physical and logical units, and then to translate absinfo.maximum (a logical unit) into max_angle (a physical unit). Resolution would be best expressed IMO as "units/revolution". The calculation above would thus become: int max_angle = (absinfo.maximum * 360) / absinfo.resolution; This will work if we increase the range of angles our tablets can measure or how fine we can measure those angles. Also, to make it possible to recover the angle from a device with arbitrary resolution, you'll want to store absinfo.resolution to common->wcmTiltXRes (see bottom-most comment) here. > + /* > + * Clamp/extend the kernel range to the range > + * expected by applications > + */ > + common->wcmTiltXMin = -max_angle; > + common->wcmTiltXMax = max_angle; Rather than assuming that the minimum and maximum tilt angles are identical (they may not be in the corner case where the hardware itself is not physically centered about zero!), it's easy enough to calculate min_angle from absinfo.minimum. > + } > + else > +#endif > + { > + /* > + * Assume the angles reported by the kernel cover > + * exactly the range expected by applications > + */ It'd be more correct to say that you're assuming the angles reported by the kernel driver are in the *units* expected by applications (i.e. degrees). > + common->wcmTiltXMin = absinfo.minimum + > + common->wcmTiltXOff; > + common->wcmTiltXMax = absinfo.maximum + > + common->wcmTiltXOff; Similar to the 'if' case above, you'll want to set common->wcmTiltXRes (see bottom-most comment) to 1 here. > + } > + } > + > + /* Y tilt range */ > + if (ISBITSET(abs, ABS_TILT_Y) && > + !ioctl(pInfo->fd, EVIOCGABS(ABS_TILT_Y), &absinfo)) > + { > + /* Center the reported range on zero */ > + /* FIXME remove once kernel is fixed */ > + common->wcmTiltYOff = - (absinfo.minimum + absinfo.maximum) / > 2; > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,30) > + /* If resolution is specified */ > + if (absinfo.resolution > 0) > + { > + int max_angle = round(MAX_TILT_ANGLE * > + absinfo.resolution); > + > + /* > + * Clamp/extend the kernel range to the range > + * expected by applications > + */ > + common->wcmTiltYMin = -max_angle; > + common->wcmTiltYMax = max_angle; > + } > + else > +#endif > + { > + /* > + * Assume the angles reported by the kernel cover > + * exactly the range expected by applications > + */ > + common->wcmTiltYMin = absinfo.minimum + > + common->wcmTiltYOff; > + common->wcmTiltYMax = absinfo.maximum + > + common->wcmTiltYOff; > + } > + } > + > /* max z cannot be configured */ > if (ISBITSET(abs, ABS_PRESSURE) && > !ioctl(pInfo->fd, EVIOCGABS(ABS_PRESSURE), &absinfo)) > @@ -1032,10 +1102,10 @@ static int usbParseAbsEvent(WacomCommonPtr common, > ds->rotation = event->value; > break; > case ABS_TILT_X: > - ds->tiltx = event->value - common->wcmMaxtiltX/2; > + ds->tiltx = event->value + common->wcmTiltXOff; Since its possible that the tilt axis has a different resolution than the 1 unit/degree expected by applications, you should use common->wcmTiltXRes (see bottom-most comment) in the calculation: ds->tiltx = (event->value + wcmTiltXOff) * 360 / common->wcmTiltXRes; > break; > case ABS_TILT_Y: > - ds->tilty = event->value - common->wcmMaxtiltY/2; > + ds->tilty = event->value + common->wcmTiltYOff; > break; > case ABS_PRESSURE: > ds->pressure = event->value; > diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c > index b04e06d..50c58d9 100644 > --- a/src/xf86Wacom.c > +++ b/src/xf86Wacom.c > @@ -240,8 +240,8 @@ wcmInitAxes(DeviceIntPtr pWcm) > } else > { > label = XIGetKnownProperty(AXIS_LABEL_PROP_ABS_TILT_X), > - min = -64; > - max = 63; > + min = common->wcmTiltXMin; > + max = common->wcmTiltXMax; > min_res = max_res = res = 1; > mode = Absolute; > } > @@ -277,8 +277,8 @@ wcmInitAxes(DeviceIntPtr pWcm) > } else > { > label = XIGetKnownProperty(AXIS_LABEL_PROP_ABS_TILT_Y); > - min = -64; > - max = 63; > + min = common->wcmTiltYMin; > + max = common->wcmTiltYMax; > min_res = max_res = res = 1; > mode = Absolute; > } > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h > index f662131..39cf179 100644 > --- a/src/xf86WacomDefs.h > +++ b/src/xf86WacomDefs.h > @@ -43,6 +43,9 @@ > #define MAX_ROTATION_RANGE 1800 /* the maximum range of the marker pen > rotation */ > #define MAX_ABS_WHEEL 1023 /* the maximum value of absolute wheel */ > > +/* The maximum tilt angle reported to applications - 60 degrees in radians */ > +#define MAX_TILT_ANGLE 1.04719755 > + This isn't necessary :) > #define MIN_PAD_RING 0 /* I4 absolute scroll ring min value */ > #define MAX_PAD_RING 71 /* I4 absolute scroll ring max value > */ > > @@ -438,8 +441,12 @@ struct _WacomCommonRec > /* tablet Z resolution is equivalent > * to wcmMaxZ which is equal to 100% > pressure */ > int wcmMaxDist; /* tablet max distance value */ > - int wcmMaxtiltX; /* styli max tilt in X directory */ > - int wcmMaxtiltY; /* styli max tilt in Y directory */ > + int wcmTiltXOff; /* styli tilt offset in X direction */ > + int wcmTiltXMin; /* styli min tilt in X direction */ > + int wcmTiltXMax; /* styli max tilt in X direction */ > + int wcmTiltYOff; /* styli tilt offset in Y direction */ > + int wcmTiltYMin; /* styli min tilt in Y direction */ > + int wcmTiltYMax; /* styli max tilt in Y direction */ > In addition to the min/max/offset, you should store the per-axis resolution to properly handle devices which do not match the 1 unit/degree resolution expected by applications. > int wcmMaxStripX; /* Maximum fingerstrip X */ > int wcmMaxStripY; /* Maximum fingerstrip Y */ > -- > 1.7.9.1 > > Jason --- Day xee-nee-svsh duu-'ushtlh-ts'it; nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it. Huu-chan xuu naa~-gha. ------------------------------------------------------------------------------ This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
