On Tue, Jan 19, 2016 at 7:11 PM, Peter Hutterer <peter.hutte...@who-t.net>
wrote:
> On Tue, Jan 19, 2016 at 06:35:24PM -0800, Ping Cheng wrote:
> > On Tue, Jan 19, 2016 at 6:17 PM, Peter Hutterer <
> peter.hutte...@who-t.net>
> > wrote:
> >
> > > On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> > > > On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer <
> > > peter.hutte...@who-t.net>
> > > > wrote:
> > > >
> > > > > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > > > > Group inital values by device types.
> > > > > >
> > > > > > Signed-off-by: Ping Cheng <pi...@wacom.com>
> > > > > > ---
> > > > > > v2: rely on device type instead of kernel version, as suggested
> by
> > > Peter.
> > > > > > ---
> > > > > > wacom.c | 67
> > > > > +++++++++++++----------------------------------------------------
> > > > > > 1 file changed, 13 insertions(+), 54 deletions(-)
> > > > > >
> > > > > > diff --git a/wacom.c b/wacom.c
> > > > > > index 32a98c4..3f5a725 100644
> > > > > > --- a/wacom.c
> > > > > > +++ b/wacom.c
> > > > > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct
> > > uinput_info
> > > > > *info)
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > > > > - if (features->type != INTUOS5S &&
> > > > > > - features->type != INTUOS5M &&
> > > > > > - features->type != INTUOS5L)
> > > > > > + if (features->type != INTUOS)
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> > > > >
> > > > > ok, now Im confused. in the previous version you had > INTUOS which
> > > > > would've
> > > > > excluded a couple of devices. Now you're only excluding the
> Intuos1. Is
> > > > > this
> > > > > intended?
> > > > >
> > > >
> > > > Yes, it is intentional.
> > > >
> > > > "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> > > > included, which are Intuos 3 and later, as well as all Cintitqs.
> Now, I
> > > > don't have those types in order. I can not use > INTUOS any more. In
> > > fact,
> > > > "> INTUOS" was unnecessary then. The type enum rearrangement was not
> for
> > > > this statement. It was for the if/switch statement you commented
> below.
> > >
> > > I was referring to the devices < INTUOS that would've been dropped
> with v1
> > >
> >
> > There was no < INTUOS in v1. This was a <= INTUOS3L. But, that was for
> > another loop...
>
> In the current code, the intuos 5 ones don't get the BTN_TOOL_FINGER bit
> set (at least not here).
>
That was a bug in the original code. It should be set.
> in the first version [1], you removed the intous5 condition and replaced it
> with an kernel version check and the condition
>
I see why I got confused. I didn't count the original code/version.
> + if (features->type > INTUOS)
>
> this would've added the tag to everything except PTU, PL, GRAPHIRE, P4,
> PENPARTNER and INTUOS.
>
We are inside wacom_intuos_events. Except INTUOS, all other types mentioned
above won't get into the routine. That's why we only need to consider
INTUOS here.
In the second version [2], you removed the intuos5 condition and replaced it
> with
> > + if (features->type != INTUOS)
>
> so PTU, PL, GRAPHIRE, P4 and PENPARTNER now get the bit set.
>
No, they still don't get the bit set. They are not in wacom_intuos_events
at all.
that's equivalent to the current code, so with v2 you're unsetting
> BTN_TOOL_FINGER from the Intuos, but leaving it in place for everything
> else
> (including Intuos 5 models which were previously excluded)
> [1] http://sourceforge.net/p/linuxwacom/mailman/message/34776688/
> [2] http://sourceforge.net/p/linuxwacom/mailman/message/34779774/
>
> It looks correct enough, but I'm just making sure this is indeed what you
> intended (the commit message should've mentioned this change, "Cleanup" is
> always a bit generic)
>
You are right. It is intentional. I should add more in the commit message.
But, it is just to clean up the code by properly grouping devices.
I didn't want to point out all the bugs, which were mainly caused by the
mess...
Cheers,
Ping
>
> > but included in v2. Since this is the same as the current code, I guess
> this
> > > was intentational though and maybe an oversight in v1?
> > >
> >
> > I am a bit confused ;-). This is v2 we are looking at. We only had one
> > version before this one. Do you refer to another version?
> >
> > Cheers,
> >
> > Ping
> >
> > > > > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > > > > > - if (features->type != INTUOS5S &&
> > > > > > - features->type != INTUOS5M &&
> > > > > > - features->type != INTUOS5L)
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > > > >
> > > > > > - if (features->type != INTUOS5S &&
> > > > > > - features->type != INTUOS5M &&
> > > > > > - features->type != INTUOS5L) {
> > > > > > + if (features->type == INTUOS3S ||
> > > > > > + features->type == INTUOS3 ||
> > > > > > + features->type == INTUOS3L ||
> > > > > > + features->type == CINTIQ ||
> > > > > > + features->type == BEE ||
> > > > > > + features->type == WACOM_21UX2) {
> > > > >
> > > > > I'd prefer a switch for simpler fallthrough, but I'm not going to
> > > fight too
> > > > > hard for this :)
> > > > >
> > > >
> > > > Yeah, I paid attention to your suggestion ;-). But, there are only
> two
> > > > states: with RX/Y, or without. We can definitely go with
> > > >
> > > > switch (features->type) {
> > > > case INTUOS3S:
> > > > case INTUOS3:
> > > > case INTUOS3L:
> > > > case CINTIQ:
> > > > case BEE:
> > > > case WACOM_21UX2:
> > > > set_event(info, UI_SET_ABSBIT, ABS_RX);
> > > > set_event(info, UI_SET_ABSBIT, ABS_RY);
> > > > break;
> > > > }
> > > >
> > > > Feel free to merge it in when you push the patch. You have my
> > > > signed-off-by, either way.
> > > >
> > > > Thank you for your review.
> > > >
> > > > Ping
> > > >
> > > >
> > > > > > set_event(info, UI_SET_ABSBIT, ABS_RX);
> > > > > > set_event(info, UI_SET_ABSBIT, ABS_RY);
> > > > > > }
> > > > > > @@ -352,26 +347,14 @@ static int wacom_set_events(struct
> uinput_info
> > > > > *info, struct uinput_user_dev *de
> > > > > > break;
> > > > > > case INTUOS4:
> > > > > > case INTUOS4L:
> > > > > > - set_event(info, UI_SET_ABSBIT, ABS_Z);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_8);
> > > > > > - /* fall trhu */
> > > > > > - case INTUOS4S:
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_0);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_1);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_2);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_3);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_4);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_5);
> > > > > > - set_event(info, UI_SET_KEYBIT, BTN_6);
> > > > > > - wacom_intuos_events(info);
> > > > > > - break;
> > > > > > case INTUOS5M:
> > > > > > case INTUOS5L:
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_8);
> > > > > > case INTUOS5S:
> > > > > > set_event(info, UI_SET_ABSBIT, ABS_Z);
> > > > > > + /* fall through */
> > > > > > + case INTUOS4S:
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_0);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_1);
> > > > > > set_event(info, UI_SET_KEYBIT, BTN_2);
> > > > > > @@ -416,7 +399,6 @@ static int wacom_set_initial_values(struct
> > > > > uinput_info *info,
> > > > > > dev->absmax[ABS_WHEEL] = 71;
> > > > > > /* fall through */
> > > > > > case G4:
> > > > > > - /* fall through */
> > > > > > case GRAPHIRE:
> > > > > > break;
> > > > > >
> > > > > > @@ -429,45 +411,22 @@ static int wacom_set_initial_values(struct
> > > > > uinput_info *info,
> > > > > > /* fall through */
> > > > > > case INTUOS3S:
> > > > > > dev->absmax[ABS_RX] = 4096;
> > > > > > - dev->absmax[ABS_Z] = 899;
> > > > > > - dev->absmin[ABS_Z] = -900;
> > > > > > /* fall through */
> > > > > > - case INTUOS:
> > > > > > - dev->absmax[ABS_WHEEL] = 1023;
> > > > > > - dev->absmax[ABS_TILT_X] = 127;
> > > > > > - dev->absmax[ABS_TILT_Y] = 127;
> > > > > > - dev->absmin[ABS_RZ] = -900;
> > > > > > - dev->absmax[ABS_RZ] = 899;
> > > > > > - dev->absmin[ABS_THROTTLE] = -1023;
> > > > > > - dev->absmax[ABS_THROTTLE] = 1023;
> > > > > > - break;
> > > > > > - case PL:
> > > > > > - case PTU:
> > > > > > - break;
> > > > > > - case PENPARTNER:
> > > > > > - break;
> > > > > > - case TABLETPC:
> > > > > > - dev->absmax[ABS_RX] = 1023;
> > > > > > - dev->absmax[ABS_RY] = 1023;
> > > > > > case INTUOS4S:
> > > > > > case INTUOS4:
> > > > > > case INTUOS4L:
> > > > > > - dev->absmin[ABS_Z] = -900;
> > > > > > - dev->absmax[ABS_Z] = 899;
> > > > > > - break;
> > > > > > case INTUOS5S:
> > > > > > case INTUOS5M:
> > > > > > case INTUOS5L:
> > > > > > - dev->absfuzz[ABS_X] = 4;
> > > > > > - dev->absfuzz[ABS_Y] = 4;
> > > > > > - dev->absfuzz[ABS_Y] = 4;
> > > > > > dev->absmin[ABS_Z] = -900;
> > > > > > dev->absmax[ABS_Z] = 899;
> > > > > > - dev->absmin[ABS_RZ] = -900;
> > > > > > - dev->absmax[ABS_RZ] = 899;
> > > > > > + /* fall through */
> > > > > > + case INTUOS:
> > > > > > dev->absmax[ABS_WHEEL] = 1023;
> > > > > > dev->absmax[ABS_TILT_X] = 127;
> > > > > > dev->absmax[ABS_TILT_Y] = 127;
> > > > > > + dev->absmin[ABS_RZ] = -900;
> > > > > > + dev->absmax[ABS_RZ] = 899;
> > > > > > dev->absmin[ABS_THROTTLE] = -1023;
> > > > > > dev->absmax[ABS_THROTTLE] = 1023;
> > > > > > break;
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > >
> > >
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
