From: Benjamin Tissoires <hidden> Date: 2017-05-16 08:18:13
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref] wrote:
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
Delay should be added to current jiffies value. Also I found out that 250 ms
is too long delay - xserver recognize such a delay as a drag gesture. Using
100 ms everything works perfectly!
Does 160 ms works too? I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
What do you think that should be changed
more in the patch to make it ready for being submitted as an official patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
I thought about some unit tests, but can't find any for hid drivers also I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.
don't know how to mock timers. One more time, thank you for your support!
From: Benjamin Tissoires <hidden> Date: 2017-05-16 09:34:46
On Tue, May 16, 2017 at 10:18 AM, Benjamin Tissoires
[off-list ref] wrote:
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref] wrote:
quoted
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
quoted
Delay should be added to current jiffies value. Also I found out that 250 ms
is too long delay - xserver recognize such a delay as a drag gesture. Using
100 ms everything works perfectly!
Does 160 ms works too? I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
quoted
What do you think that should be changed
more in the patch to make it ready for being submitted as an official patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
quoted
I thought about some unit tests, but can't find any for hid drivers also I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.
I just run this patch against the various recordings I have. There is
only one difference, but after thorough examination, it seems the
machine blocked for one second for no reasons (the recorded incoming
timestamps show 1 second delay while the scan time forwarded by the
device show 50ms between the 2 events).
So I'd call this patch safe to test and to go upstream.
Cheers,
Benjamin
quoted
don't know how to mock timers. One more time, thank you for your support!
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref] wrote:
quoted
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
quoted
Delay should be added to current jiffies value. Also I found out that 250 ms
is too long delay - xserver recognize such a delay as a drag gesture. Using
100 ms everything works perfectly!
Does 160 ms works too?
160 ms works too but I have a feeling that values closer to 200 ms gives quite a laggy experience so I'd suggest to keep value closer to 100 ms. Beside that I haven't seen situation that "normal" syn_report have been repored after longer delay than 30 ms. Mean value is 9 ms and 95 pp ~10 ms (tested by drags with evemu-record)
I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
In fact 100 ms it is exactly 6 times the maximum time between 2 reports, because 60 Hz frequency gives 16,(6) ms period
quoted
What do you think that should be changed
more in the patch to make it ready for being submitted as an official patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
quoted
I thought about some unit tests, but can't find any for hid drivers also I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.
Hi Benjamin,
On 16.05.2017 11:46, Arek Burdach wrote:
On 16.05.2017 10:18, Benjamin Tissoires wrote:
quoted
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref] wrote:
quoted
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
quoted
Delay should be added to current jiffies value. Also I found out that 250 ms
is too long delay - xserver recognize such a delay as a drag gesture. Using
100 ms everything works perfectly!
Does 160 ms works too?
160 ms works too but I have a feeling that values closer to 200 ms gives quite a laggy experience so I'd suggest to keep value closer to 100 ms. Beside that I haven't seen situation that "normal" syn_report have been repored after longer delay than 30 ms. Mean value is 9 ms and 95 pp ~10 ms (tested by drags with evemu-record)
quoted
I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
In fact 100 ms it is exactly 6 times the maximum time between 2 reports, because 60 Hz frequency gives 16,(6) ms period
quoted
quoted
What do you think that should be changed
more in the patch to make it ready for being submitted as an official patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
Do you planning to submit this change? I think that other users would be glad to take advantage of this fix :-)
Or do you want me to do it on your behalf?
quoted
quoted
I thought about some unit tests, but can't find any for hid drivers also I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.
From: Benjamin Tissoires <hidden> Date: 2017-06-07 13:13:02
Hi Arek,
On Wed, Jun 7, 2017 at 9:27 AM, Arek Burdach [off-list ref] wrote:
Hi Benjamin,
On 16.05.2017 11:46, Arek Burdach wrote:
quoted
On 16.05.2017 10:18, Benjamin Tissoires wrote:
quoted
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref]
wrote:
quoted
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
quoted
Delay should be added to current jiffies value. Also I found out that
250 ms
is too long delay - xserver recognize such a delay as a drag gesture.
Using
100 ms everything works perfectly!
Does 160 ms works too?
160 ms works too but I have a feeling that values closer to 200 ms gives
quite a laggy experience so I'd suggest to keep value closer to 100 ms.
Beside that I haven't seen situation that "normal" syn_report have been
repored after longer delay than 30 ms. Mean value is 9 ms and 95 pp ~10 ms
(tested by drags with evemu-record)
quoted
I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
In fact 100 ms it is exactly 6 times the maximum time between 2 reports,
because 60 Hz frequency gives 16,(6) ms period
quoted
quoted
What do you think that should be changed
more in the patch to make it ready for being submitted as an official
patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
Do you planning to submit this change? I think that other users would be
glad to take advantage of this fix :-)
Yes, sorry, it's still on my todo list. I am trying to fix the last
issue I can think of which is problematic:
if you happen touch again the sensor when the deferred timeout starts,
both threads will access the input node, and this can create some
nasty behaviors.
I am not 100% sure we can rely on classic solutions (spinlock,
mutexes) because in these 2 concurrent threads we are in places where
we can not stop.
So I'll need to conduct more tests and find a better/simpler solution
to not hit that. Sorry for the delay.
Or do you want me to do it on your behalf?
I'd like you to give a test when I finally get something. However,
it's not on my top priority list, sorry for that.
Cheers,
Benjamin
quoted
quoted
quoted
I thought about some unit tests, but can't find any for hid drivers also
I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.
Hi Arek,
On Wed, Jun 7, 2017 at 9:27 AM, Arek Burdach [off-list ref] wrote:
quoted
Hi Benjamin,
On 16.05.2017 11:46, Arek Burdach wrote:
quoted
On 16.05.2017 10:18, Benjamin Tissoires wrote:
quoted
On Mon, May 15, 2017 at 8:45 PM, Arek Burdach [off-list ref]
wrote:
quoted
Hi Benjamin,
I found out what was wrong in the change that you've provided:
On 12.05.2017 16:28, Benjamin Tissoires wrote:
+ if (td->mtclass.quirks & MT_QUIRK_STICKY_FINGERS)
+ mod_timer(&td->release_timer, msecs_to_jiffies(250));
}
Should be:
mod_timer(&td->release_timer, jiffies + msecs_to_jiffies(100));
Right, thanks!
quoted
Delay should be added to current jiffies value. Also I found out that
250 ms
is too long delay - xserver recognize such a delay as a drag gesture.
Using
100 ms everything works perfectly!
Does 160 ms works too?
160 ms works too but I have a feeling that values closer to 200 ms gives
quite a laggy experience so I'd suggest to keep value closer to 100 ms.
Beside that I haven't seen situation that "normal" syn_report have been
repored after longer delay than 30 ms. Mean value is 9 ms and 95 pp ~10 ms
(tested by drags with evemu-record)
quoted
I'd rather have 10 times the maximum time
between 2 reports than 6.25 times it :)
We can use 96 ms if 160 doesn't work.
In fact 100 ms it is exactly 6 times the maximum time between 2 reports,
because 60 Hz frequency gives 16,(6) ms period
quoted
quoted
What do you think that should be changed
more in the patch to make it ready for being submitted as an official
patch?
Not much actually. I need to wrote down a commit message, sign it
(your sign-off-by can also be added given that you debugged it), and
that should be it.
Do you planning to submit this change? I think that other users would be
glad to take advantage of this fix :-)
Yes, sorry, it's still on my todo list. I am trying to fix the last
issue I can think of which is problematic:
if you happen touch again the sensor when the deferred timeout starts,
both threads will access the input node, and this can create some
nasty behaviors.
Indeed this situation can produce some nasty behavior like interrupted dragging gesture but user need to click very fast to occur that.
I am not 100% sure we can rely on classic solutions (spinlock,
mutexes) because in these 2 concurrent threads we are in places where
we can not stop.
I'm very intrigued how it could be resolved without mutexes.
So I'll need to conduct more tests and find a better/simpler solution
to not hit that. Sorry for the delay.
No problem. Thank you for the notice.
quoted
Or do you want me to do it on your behalf?
I'd like you to give a test when I finally get something. However,
it's not on my top priority list, sorry for that.
I would test it with pleasure. BTW I didn't noticed any problem with current solution so far and I'm using touchscreen quite often.
One more time thank you for your help!
Cheers,
Benjamin
quoted
quoted
quoted
quoted
I thought about some unit tests, but can't find any for hid drivers also
I
I have a test suite that can emulate hid devices and inject them in
hid-multitouch. The setup is a little bit manual, so I'll try to run
it today and see if there are differences with or without the patch.