Hi Jason,
On Fri, Mar 30, 2012 at 4:21 AM, Jason Gerecke <killert...@gmail.com> wrote:
> 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...
No problem, thanks for reviewing it so extensively!
> On Thu, Mar 22, 2012 at 12:37 PM, Nikolai Kondrashov <spbn...@gmail.com>
> wrote:
>> --- 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.
I wasn't sure that these are initialized properly in all the cases in
wcmISDV4.c and wcmUSB.c and it was there before me, after all. I'll read the
code more closely and will remove it if I don't find anything suspicious.
>> --- 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.
I was wondering about it too, and maybe it wasn't needed before, but now it
is needed to clamp the angles to 60 degrees for devices which have wider
range, like Waltop Sirius.
>> 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 :)
For now it seems there is nothing to change, considering the comments below.
>> 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.
I think you're right. At least I can't remember why I did it this way. Will
fix.
>> +#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.
I'm assuming applications don't actually expect reported values to be in
degrees, but rather expect fractions of the maximum 60 degree angle.
I've tried looking at the GIMP and mypaint code and it looked like it, but
I'm not 100% sure. At least both of them worked fine with the maximum of
105, which Waltop Sirius kernel driver currently produces. However, I don't
have a Wacom or, in fact, any other tilt-reporting tablet to compare to.
I'm OK with the fixed maximum angle, because there is only so much tilt a
human hand is comfortable to do. Although I'm not sure it is exactly 60
degrees, I'm inclined to trust Wacom's judgement and expertise in this
matter.
BTW, has anyone actually tried to measure the angles corresponding to
reported values with a Wacom tablet?
> 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.
Yes, because the applications expect fixed maximum angle. Thus we calculate
the maximum values a device would report with its resolution if its range
was 60 degrees, so the applications could scale the reported values
correctly.
> 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;
The absinfo.resolution is in units/radian, as agreed to by the kernel folks,
for rotary controls. It could have been units/degree, but this would reduce
the precision. However, it makes no sense introducing an artificial unit of
units/Pi*2, especially since the HID spec doesn't use it.
> This will work if we increase the range of angles our tablets can
> measure or how fine we can measure those angles.
This will work with units/radian as well.
> 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.
Since we only care about fractions of 60 degrees, we only need to store the
range, but not the resolution.
>> + /*
>> + * 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.
I'm not assuming they're identical. I'm only clamping/extending the range to
the one expected by applications. This would cover the case when they're not
equal.
>> + /*
>> + * 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).
It would be untrue. I'm only expecting the physical range to match.
>> + 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.
We don't need that for the reasons stated above.
>> 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;
This comment also doesn't apply, considering the applications expect fixed
physical angle range.
>> +/* The maximum tilt angle reported to applications - 60 degrees in radians
>> */
>> +#define MAX_TILT_ANGLE 1.04719755
>> +
>
> This isn't necessary :)
This is needed to calculate the clamping/extending range, if the
applications are indeed expecting the fixed range.
>> @@ -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.
We don't need it, as explained above.
Thanks!
Sincerely,
Nick
------------------------------------------------------------------------------
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
