On Fri, Mar 30, 2012 at 3:11 AM, Nikolai Kondrashov <spbn...@gmail.com> wrote:
> 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.
>
It looks like the truth was somewhere between both of our assumptions.
We should report in integer degrees, and need to be clamped between
-60 and +60.
> 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.
>
GIMP, Inkscape, and MyPaint all treat the values they get from GTK as
the normalized length of the tool's projection along the corresponding
axis (they take the arctangent of the values to get an azimuth). While
GTK doesn't explicitly state that this is what applications receive,
its a reasonable reading of their documentation: "tilt attributes
range from -1.0 to 1.0 ... representing the maximum tilt to the left
or up ... [and] to the right or down". It looks like in practice GTK
actually only scales the tilt values it gets from X based on the
minimum and maximum. Its not what applications expect, but appears to
work well enough that nobody's bothered to file a bug.
Krita, however, does appear to depend on the magic number 60 when
dealing with tilt. Diving deeper, it appears that the Qt API actually
states applications will receive "the angle between the device (a pen,
for example) and the perpendicular" and that "the angle is in the
range -60 to +60 degrees". Qt itself doesn't enforce this at all, and
just stuffs whatever data it receives from X into the tilt variables.
That means that for now we've got to be careful to do the clamping
ourselves.
> 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.
>
I'm of the opinion that if we should keep the maximum fidelity
possible when passing data up the stack. If a tablet comes out that
can sense a full 90 degrees of tilt, I figure its the driver's job to
provide applications with the full range. Of course, if things are
written assuming a limited range of motion (like Qt apparently was)
then we need to let them know the assumption was invalid and be
mindful of compatibility in the meantime.
> BTW, has anyone actually tried to measure the angles corresponding to
> reported values with a Wacom tablet?
>
The reported values are correct to within a few degrees. While
marketing indicates +-60 degrees, the tablets will report all the way
out to +-64 degrees since they've got a 7-bit field to fill. It
doesn't appear to change with the sine of the angle (unlike the
Sirius), but my tests were admittedly rough.
>> 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.
>
Didn't know that :) I couldn't find any kernel docs to indicate an
appropriate unit, so I made one up on the spot.
>> 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.
>
With Qt wanting integer degrees, I think we still need this to scale
high- or low-resolution devices to 1 unit/degree.
>>> + /*
>>> + * 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.
>
I see now what you were intending. Since I was coming at the problem
from the standpoint of "fixed resolution, free range", it didn't make
much sense when I was reading over it. It probably doesn't matter
either way since I don't know when we'd ever encounter a device which
did this. Just me worrying about corner cases that are way out in left
field :D
>>> + /*
>>> + * 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.
>
And now we're expecting both the units and 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
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
