On Fri, Nov 14, 2014 at 1:09 AM, Robert Miller <yocto...@gmail.com> wrote: > Hi! I've never submitted a patch to anything before so I hope I'm not > wasting your time, but I found a bug while using my cintiq companion hybrid > and I traced it to linux_wacom. > > I'm on Ubuntu 14.04, and the device I'm testing with is a Cintiq Companion > Hybrid. > > The bug I noticed is that sometimes (maybe one time in 100), when I try to > switch from eraser to pen tip or back, the device would be misdetected. > After some tracing through the code, I note that it's possible for a device > with an invalid/unknown ID to set the proximity to 'In' in a range event. > When this happens, xinput query-state on the device will show 'In', even > when it is out of range. If this state is on the eraser, and then you place > the pen tip in range, then xinput shows both devices as 'In', but > applications detect the wrong one (the eraser in this case). > > It's a simple fix: After the ID is potentially set in wacom_intuos_inout, I > check to see if the ID is valid, and if not I return 0 before the range or > exit events can be executed. I've included a patch below that does exactly > that. I've tested on the 3.7 tree, but I believe all of the different trees > should compile with this patch. I have been unable to cause the bug to occur > after applying the patch even after several hours of use. > > Please let me know if I'm doing anything wrong in the submission of this > patch, because I'd like to know 'the right way' in the future. > > Thanks! > - Robert Miller > > ------------------------------------------------------------------ > > From 1ab1b2884abb75690b653a7f8961d8dfebd87d09 Mon Sep 17 00:00:00 2001 > From: Robert Miller <yocto...@gmail.com> > Date: Fri, 14 Nov 2014 01:01:18 -0800 > Subject: [PATCH] Prevent occasional Pen/Eraser misdetection > > I note that it's possible for a device with an invalid/unknown ID > to set the proximity to 'In' in a range event. > It's a simple fix: After the ID is potentially set in > wacom_intuos_inout, I check to see if the ID is valid, and > if not I return 0 before the range or exit events can be executed. > --- > 3.7/wacom_wac.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/3.7/wacom_wac.c b/3.7/wacom_wac.c > index 6caf6e6..db05815 100644 > --- a/3.7/wacom_wac.c > +++ b/3.7/wacom_wac.c > @@ -477,6 +477,13 @@ static int wacom_intuos_inout(struct wacom_wac *wacom) > } > return 1; > } > + > + /* > + * If we don't know the ID at this point, > + * then there's not much we can do with the event. > + */ > + if(!wacom->id[idx]) > + return 0; > > /* older I4 styli don't work with new Cintiqs */ > if (!((wacom->id[idx] >> 20) & 0x01) && > -- > 1.9.1 > > Robert,
Firstly, thanks for the patch :) There's not too much I can nitpick with regards to procedure or style -- it seems to largely follow the kernel guidelines[1] that we prefer people to follow. The primary thing that stands out is a missing "Signed-off-by" line on the patch (see the guide for more information). We also usually like to have a patch to send upstream before worrying about input-wacom, though its understandable creating the input-wacom patch first since development is a lot easier :D Once the patch is accepted upstream we'll try to get the patch into input-wacom, with it applied to as many of the different trees as possible (in this case it'd be good for the patch to apply to all of the trees). It took me a while to figure out exactly how this bug would occur and why your patch would work, but I've finally been able to reproduce the issue and largely agree with your assessment. I would suggest the commit message be slightly re-worded though, since from what I understand* the proximity is never actually set "in" for the kernel driver (that's purely an effect of the X driver). Perhaps something like "The tablet may generate 'range' (and even 'exit') reports prior to getting close enough to trigger proximity; these events should be ignored until we get an 'enter' report which has properly initialized the tool.". *If the pen comes merely "in range" of the tablet and then leaves before coming fully "in prox", the kernel will generate an event when it arguably shouldn't (since all the BTN_TOOL_* remain zero). This tickles a corner-case handler in the X driver (at src/wcmUSB.c:1710) which winds up forcing the no-longer-in-prox device to be marked as in-prox and also consequently jump to (0,0). Prior to xf86-input-wacom 0.26.0, switching between the stylus and eraser tool while in this state will cause both tools to be marked in-prox (in 0.26.0 and later, the arbitration logic will force the bogus device out when the new one enters). [1]: https://www.kernel.org/doc/Documentation/SubmittingPatches Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
