Created attachment 49709 [details] [review] Jitter filter patch against current master Some devices (e.g. IRTouch touchscreens) are too sensitive and report many touch events, resulting in a multitude of click and movement events for a single tap, rendering the device completely unusable (accidental drag&drop events, cursor jumping to 0/0, both at the same time, ...). I wrote a small patch that adds an (optional, configureable) filter to discard these events before processing them. This seems to duplicate the Windows driver's behaviour and makes tapping working (at the cost of disabling usable motion events). I propose to include it into evdev – it'll make irtouch screens a lot more useable and will probably be helpful for other kinds of touchscreens as well.
*** Bug 39665 has been marked as a duplicate of this bug. ***
Created attachment 49716 [details] [review] Updated patch
Created attachment 50367 [details] [review] Now using kernel fuzz too Updated patch according to feedback in #xorg-devel. Patch now uses fuzz for coordinate-based filtering as well (either kernel- or user-supplied) and is cleaned up a bit.
Review of attachment 50367 [details] [review]: Thanks. A few comments: * JitterFilterUseFuzz is not needed. If the kernel gives us fuzz of non-zero, we just use that, alternatively we use whatever the user configured. If the user happens to have a device that has a fuzz and it's not what they want, they need to configure it anyway. This also makes the code a bit simpler since you then only need to check one fuzz value, not absinfo as well. * JitterFilter: "performs basic sanity checks" is a bit too ambiguous for what the filter does, this should have a description of what the filter really does + int fuzz; /* Minimum allowed value for coordinate deltas. */ should be localised, move into the matching block if (abs (value - pEvdev->jitter_filter.values[map]) < abs(fuzz)) no space in abs(value ... + xf86IDrvMsg(pInfo, X_PROBED, "Jitter filter enabled.\n"); X_CONFIG, probed is for values obtained from the kernel I'm not a fan of stuffing too much into functions. In this case your code is all nicely contained within one block anyway - why not just move that into a separate function so that the caller's code is simpler. (map == 1 && time_delta > 50)) a hardcoded time delta? this seems odd + /* Don't update the same axis twice in a row. This would spawn events with one axis zero'd, resulting in + the cursor jumping to the screen borders. */ this would be a bug in the code. the valuator masks can unset a value, so even if you update twice you just need to take care to unset the matching valuator. + if (map == pEvdev->jitter_filter.map) this condition confuses me a bit. If I have a device that just sends ABS_X, even if I get past fuzz it won't take the values because it never resets until the time_delta = timeout * 3 is hit. shouldn't this update + if ((map == 0 && time_delta < pEvdev->jitter_filter.timeout) || /* Don't accept multiple events for one click */ why is this only on the X axis... + (map == 1 && time_delta > 50)) { /* Stray axis update not belonging to the last event, discard */ ...and why is this only on the Y axis? Is this some device quirk? The description seems like this device is completely out of whack, I'd love to see the evtest logs for it. I'm pretty sure we'll need some kernel handling for this to skip invalid packets. I'm also wondering what the need for the FilterTimeout is. Presumably, if you configure the fuzz correctly, the timeout shouldn't be needed.
> * JitterFilterUseFuzz is not needed. If the kernel gives us fuzz of non-zero, > we just use that, alternatively we use whatever the user configured. Okay. > * JitterFilter: "performs basic sanity checks" is a bit too ambiguous for what > the filter does, this should have a description of what the filter really does Will do. > should be localised, move into the matching block Which block would that be? > I'm not a fan of stuffing too much into functions. In this case your code is > all nicely contained within one block anyway - why not just move that into a > separate function so that the caller's code is simpler. I thought of that, but initially didn't do it because this is only useful for absolute events anyway. But can do. > a hardcoded time delta? this seems odd Whoops. Should be .timeout / 10000. > this would be a bug in the code. the valuator masks can unset a value, so even > if you update twice you just need to take care to unset the matching valuator. So… do a unset() first and then let the original code do the set()? > this condition confuses me a bit. If I have a device that just sends ABS_X, > even if I get past fuzz it won't take the values because it never resets until > the time_delta = timeout * 3 is hit. shouldn't this update To my experience this would create a lot of click events with Y zeroed. Would that be filtered by the valuator unset? > why is this only on the X axis... > ...and why is this only on the Y axis? Is this some device quirk? The > description seems like this device is completely out of whack, "completely out of whack" is a nice understatement. The device is completely unusable without this patch. > I'd love to see the evtest logs for it. Can do. > I'm pretty sure we'll need some kernel handling for > this to skip invalid packets. Oh joy. > I'm also wondering what the need for the FilterTimeout is. Presumably, if you > configure the fuzz correctly, the timeout shouldn't be needed. If the valuator unset cancels out all of the click events, it might, I need to test that. But if I'm not mistaken, our whacky touchscreen would then still produce double and/or triple clicks for every single tap.
Created attachment 50491 [details] evtest log for sample device
(In reply to comment #6) > Created an attachment (id=50491) [details] > evtest log for sample device This looks like a normal log, nothing overly crazy in there. I understand the need for jitter, but other than that it seems normal. Even tried emulating some of these events with a virtual device and saw no unexpected jumping. What version of evdev/server are you using again? maybe this is a bug in your specific version?
> What version of evdev/server are you using again? maybe this is a bug in your > specific version? Maybe, so far I've been testing with Ubuntu 11.04, which has a rather thoroughly patched evdev. Will test with Arch if I find the time.
Works actually fine for vanilla Xorg, the problem was Ubuntu's patches all along. https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bug/832624
Ok, thanks for testing. FTR, i'm not opposed to a jitter filter in evdev but let's integrate this when we see a real need for it.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.