Bug 105963

in 

tp_need_motion_history_reset(struct tp_dispatch *tp)

I added this 



static inline bool
tp_need_motion_history_reset(struct tp_dispatch *tp)
{
	bool rc = false;
	evdev_log_debug(tp->device, "Checking for consecutive non-motion events\n");
			}





but I am not getting any such log messages at all.

ok, so I changed to printf and now I get something

the quirk is not active, the AND of the flag is returning 0

printf("Flag check: %d %d and then AND: %d\n",tp->device->model_flags,EVDEV_MODEL_LENOVO_T450_TOUCHPAD,tp->device->model_flags & EVDEV_MODEL_LENOVO_T450_TOUCHPAD);

gives


Flag check: 512 131072 and then AND: 0

I have a system file in /lib/udev/hwdb.d which is out of date. It has not P50 rule

the ninja install is updating /usr/lib/udev/hwdb.d
this has the P50 rule but it looks like it is not being used

is it in the wrong place?

http://manpages.ubuntu.com/manpages/xenial/man7/udev.7.html

OK, it was in the wrong place. After copying the correct quirk rules file to the Ubuntu location, and rebooting the quirk is active. It definitely has an effect, I'm not sure it is a good effect but I can play now.

/lib isn't a symlink to /usr/lib? interesting... Anyway, you can pass -Dudev-dir=/lib/udev/ at configure time to install to the right directory.

Created attachment 138720 [details]
email garbage

thanks. I'm having fun working it all out. There is no problem with
scrolling and the sudden movements are better. The acceleration factor
needs to be much slower at slow movements, and the pressure sensitivity may
need tweaking. I'll keep fiddling until it works like Windows. Then we can
see how the configuration can work.

On 10 April 2018 at 17:57, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 5 <https://bugs.freedesktop.org/show_bug.cgi?id=105963#c5> on
> bug 105963 <https://bugs.freedesktop.org/show_bug.cgi?id=105963> from Peter
> Hutterer <peter.hutterer@who-t.net> *
>
> /lib isn't a symlink to /usr/lib? interesting... Anyway, you can pass
> -Dudev-dir=/lib/udev/ at configure time to install to the right directory.
>
> ------------------------------
> You are receiving this mail because:
>
>    - You reported the bug.
>
>

Peter, don't deploy the P50 changes. After a reboot, the touchpad was unusable.
I think sharing a flag with other quirks causes problems. In my branch I have created a separate P50 flag.

the best of fixing this will be different acceleration behaviour, to mimic windows. I can't get smooth movement any other way, I think. 
when I do 

 xinput --set-prop "$DEVICE" "libinput Accel Speed"  -.9

I get very slow pointer movement, as expected .But I can't see where in libinput this setting influences behaviour. 
printf diagnostics in 
touchpad_accel_profile_linear
don't seem to show any change between a setting of -0.9 and 0 although the pointer behaviour is really different.

Created attachment 138744 [details] [review]
patch for P50

With a setting such as 
 xinput --set-prop "$DEVICE" "libinput Accel Speed"  1

this patch works pretty well for the P50

Comment on attachment 138744 [details] [review]
patch for P50

Review of attachment 138744 [details] [review]:
-----------------------------------------------------------------

Thanks, overall - I don't know why this patch works but the previous one didn't. Functionally they are (or should be) exactly the same on your touchpad.

The (1 << 28) is a definite bug that needs to be fixed, please submit a separate patch for that.

::: src/evdev-mt-touchpad.c
@@ +106,4 @@
>  	 * is reset whenever a new finger is down, so we'd be resetting the
>  	 * speed and failing.
>  	 */
> +	if (t->history.count < HISTORY_COUNT_THRESHOLD) {

fwiw, I haven't used a const here because we only use it in one place. So having the number right here makes it more obvious what the value is. There's no strict rule for this, there is some gut feeling involved :)

Same goes for the NON_MOTION_EVENT_COUNT

@@ +435,4 @@
>  
>  	if (t->history.count <= 1)
>  		return zero;
> +		

this and a whole bunch of others look like detritus and erroneous whitespace being introduced. I recommend looking into git add -p, this saves you from adding spurious hunks.

@@ +1411,4 @@
>  	   reset that touch to non-dirty effectively swallowing that event
>  	   and restarting with the next event again.
>  	 */
> +	if (tp->device->model_flags & EVDEV_MODEL_LENOVO_P50_TOUCHPAD) {

this would break all the touchpads that currently rely on this setting. But I don't quite get why this is even needed, the flag name itself just doesn't matter.

::: src/evdev.h
@@ -126,4 @@
>  	EVDEV_MODEL_LOGITECH_MARBLE_MOUSE = (1 << 26),
>  	EVDEV_MODEL_TABLET_NO_PROXIMITY_OUT = (1 << 27),
>  	EVDEV_MODEL_MS_NANO_TRANSCEIVER = (1 << 28),
> -	EVDEV_MODEL_TABLET_NO_TILT = (1 << 28),

oh crap, that's a real bug (both flags having 1<<28). Can you split this into a separate commit please and I'll get it merged.

::: udev/90-libinput-model-quirks.hwdb
@@ +220,5 @@
> + 
> +# Lenovo P50
> +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadP50*
> + LIBINPUT_MODEL_LENOVO_P50_TOUCHPAD=1
> + LIBINPUT_ATTR_PALM_PRESSURE_THRESHOLD=150

I really don't understand why this could be needed. The udev properties you set here are the same as the ones with the previous patch (compare udevadm info /sys/class/input/eventX output to be sure). Even the palm threshold is the same, so all that happens here is a renaming of flags but the functional effect should be the same before and after and this patch is effectively a noop (for your touchpad, it breaks all the ones below :)

::: .gitignore
@@ +16,5 @@
> +/help/
> +/.cproject
> +/.project
> +/tim_notes.txt
> +.externalToolBuilders

can't merge those, too specific. Please remove from the patch, thanks.

Created attachment 138745 [details]
email garbage

Thanks, I will take those actions, including learning a bit more about git.
I will also see if I over-reacted by making a separate udev rule for the
P50. Before I split it, the touchpad became unusable after a reboot; I
assumed that other quirks somewhere were being applied which were causing
the trouble (so my hypothesis is not that the udev properties need to be
different, but that somewhere other quirks were being activated because a
bunch of different ThinkPads were grouped together). However, these
problems may have been due to some of my other exploratory changes (since
backed out since none of them helped). You're right: the only active change
is the rejection of non-touch events which you already coded, plus the use
of a dedicated P50 rule (I thought you created that for the P50 bug report
which is why I wasn't concerned about a specific P50 rule excluding other
hardware from the non-touch event rejection).

The touchpad is still a poor experience at slow acceleration settings
(<-0.7) but it is considerably improved at xinput acceleration settings
>0.5 (previously close to unusable, but not any more, it's actually
pleasant to use at last).


On 11 April 2018 at 10:42, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 10 <https://bugs.freedesktop.org/show_bug.cgi?id=105963#c10> on
> bug 105963 <https://bugs.freedesktop.org/show_bug.cgi?id=105963> from Peter
> Hutterer <peter.hutterer@who-t.net> *
>
> Comment on attachment 138744 [details] [review] <https://bugs.freedesktop.org/attachment.cgi?id=138744> [details] <https://bugs.freedesktop.org/attachment.cgi?id=138744&action=edit> [review] <https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=105963&attachment=138744>
> patch for P50
>
> Review of attachment 138744 [details] [review] <https://bugs.freedesktop.org/attachment.cgi?id=138744> [details] <https://bugs.freedesktop.org/attachment.cgi?id=138744&action=edit> [review] <https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=105963&attachment=138744>:
> -----------------------------------------------------------------
>
> Thanks, overall - I don't know why this patch works but the previous one
> didn't. Functionally they are (or should be) exactly the same on your touchpad.
>
> The (1 << 28) is a definite bug that needs to be fixed, please submit a
> separate patch for that.
>
> ::: src/evdev-mt-touchpad.c
> @@ +106,4 @@>  	 * is reset whenever a new finger is down, so we'd be resetting the
> >  	 * speed and failing.
> >  	 */
> > +	if (t->history.count < HISTORY_COUNT_THRESHOLD) {
>
> fwiw, I haven't used a const here because we only use it in one place. So
> having the number right here makes it more obvious what the value is. There's
> no strict rule for this, there is some gut feeling involved :)
>
> Same goes for the NON_MOTION_EVENT_COUNT
>
> @@ +435,4 @@>
> >  	if (t->history.count <= 1)
> >  		return zero;
> > +		
>
> this and a whole bunch of others look like detritus and erroneous whitespace
> being introduced. I recommend looking into git add -p, this saves you from
> adding spurious hunks.
>
> @@ +1411,4 @@>  	   reset that touch to non-dirty effectively swallowing that event
> >  	   and restarting with the next event again.
> >  	 */
> > +	if (tp->device->model_flags & EVDEV_MODEL_LENOVO_P50_TOUCHPAD) {
>
> this would break all the touchpads that currently rely on this setting. But I
> don't quite get why this is even needed, the flag name itself just doesn't
> matter.
>
> ::: src/evdev.h
> @@ -126,4 @@>  	EVDEV_MODEL_LOGITECH_MARBLE_MOUSE = (1 << 26),
> >  	EVDEV_MODEL_TABLET_NO_PROXIMITY_OUT = (1 << 27),
> >  	EVDEV_MODEL_MS_NANO_TRANSCEIVER = (1 << 28),
> > -	EVDEV_MODEL_TABLET_NO_TILT = (1 << 28),
>
> oh crap, that's a real bug (both flags having 1<<28). Can you split this into a
> separate commit please and I'll get it merged.
>
> ::: udev/90-libinput-model-quirks.hwdb
> @@ +220,5 @@> +
> > +# Lenovo P50
> > +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadP50*
> > + LIBINPUT_MODEL_LENOVO_P50_TOUCHPAD=1
> > + LIBINPUT_ATTR_PALM_PRESSURE_THRESHOLD=150
>
> I really don't understand why this could be needed. The udev properties you set
> here are the same as the ones with the previous patch (compare udevadm info
> /sys/class/input/eventX output to be sure). Even the palm threshold is the
> same, so all that happens here is a renaming of flags but the functional effect
> should be the same before and after and this patch is effectively a noop (for
> your touchpad, it breaks all the ones below :)
>
> ::: .gitignore
> @@ +16,5 @@> +/help/
> > +/.cproject
> > +/.project
> > +/tim_notes.txt
> > +.externalToolBuilders
>
> can't merge those, too specific. Please remove from the patch, thanks.
>
> ------------------------------
> You are receiving this mail because:
>
>    - You reported the bug.
>
>

*please* trim your emails. Have a look at the URL of this bug report to see what the bug has become...

[This was typed in response to comment #8, but bugzilla didn't take it first time round]

Also look into bug 101139, that is the current bug for the "way too fast" behaviour.

As for debugging this in general: touchpad_accel_profile_linear() is the acceleration profile and the returned factor is applied to the deltas. The threshold and incline are affected by the configured speed, see touchpad_accelerator_set_speed() which is called when you change the property.

Note that if you're using libinput debug-events or any other tools, the context is *not* the one your X server has, it's always a new context and changes don't go across. Think of it as two different processes reading the same byte stream, you can change how you process data in one process but the other one still receives the unmodified byte stream and is thus unaffected. So any changes in X don't affect libinput debug-events and vice versa.

libinput debug-gui has a hidden feature, cursor keys up/down change the pointer acceleration by ±0.1 with each press. That's going to be helpful for testing because you see the immediate effects on the cursor.

Also note that in calculate_acceleration() we're using simpson's rule to smooth things a bit. So that may affect things a bit as well.

> I think sharing a flag with other quirks causes problems

It shouldn't, the flags are just that and libinput doesn't care about them beyond the usual handling. That particular T450 flag is only used in one place, so re-using it is safe.

Created attachment 138751 [details] [review]
fix duplicate flag value is evdev.h

fix duplicate flag value is evdev.h

Created attachment 138755 [details] [review]
Patch to reapply the p50 rule & fix syntax error in existing rules

"Note that if you're using libinput debug-events or any other tools, the context is *not* the one your X server has, it's always a new context and changes don't go across. "

ah, that explains a lot, I was ready to sign up to a flat earth.

in touchpad_accel_profile_linear the value of
const double threshold = accel_filter->threshold
is crucial.

For me, it is defaulting to 254
This speed is hardly reached; it represents very rapid motion on the P50. 

the threshold code is basically not of any practical use because the condition 

 if (speed_in < threshold) {
		factor = 1;

rarely happens. 

I've attached an experimental idea which is working quite well on my P50.
the threshold now depends on filter->speed_adjustment

and the accel function is simplified

Created attachment 138757 [details] [review]
experimental changes to touchpad_accel_profile

this is full of constants which probably only make sense on the P50, but it makes a nice difference, plus it seems a bit simpler.

(In reply to Tim Richardson from comment #17)

> 
>  if (speed_in < threshold) {
> 		factor = 1;
> 
> rarely happens. 
> 
Sorry, tired. I mean the condition is nearly always true.

Comment on attachment 138755 [details] [review]
Patch to reapply the p50 rule & fix syntax error in existing rules

Review of attachment 138755 [details] [review]:
-----------------------------------------------------------------

As a general rule, please always separate logical steps within patches. Adding a new device should not be combined with fixing another device - makes it hard to separate the patches out for e.g. stable branches or regression testing. So this patch would have to be two patches, one for the P50, one for the alleged syntax error.

Alas, that 'syntax error' is intentional, these Lenovo series have an optional suffix as well (T450, T450s, T450p, ...). They all have the same hardware, so this rule catches all. It should arguably be a ? instead as there is only one letter but the current rule works. The trailing : doesn't matter because all modalias have a trailing ':'. So it doesn't quite match as you'd expect but it does match :)

As for the P50 patch - this is the same as in bug 105022 now, right? That one apparently made scrolling unworkable so I had to revert it upstream. Does this patch have any effect on scrolling for you?

Hi Peter, I confirm that I have no adverse side effects from the P50 rule triggering the ignore non-touch events logic with the code I am using. Locally I have gone back to exactly your udev rules including your p50 rule.

You will probably conclude to ignore my patches and re-apply the one you reverted, so assuming that, I won't resubmit patches for this. That is, I assume this is less work for you (and me). Thanks for your patience, I feel that my future contributions will be much better thanks to your help. This is the first C I have looked at in years. The barrier to entry is really low, you have done awesome job making someone like me a contributor, even if not a very good one just yet.


I have continued to do work on better P50 touchpad, see experimental patch. It is dramatically better, building on the non-touch event rejection. 
One reason is the threshold for the application of part 3 of the acceleration profile is about 250 mm/s movement, which is way too fast ... this is probably a nominal figure, and it presumably makes sense of a lot of hardware I will never see, yet is it terribly wrong for the P50. My changes go beyond this, I have simplified the algorithm.

I'm really happy to have a much better laptop thanks to the past few days.
However, the big question for me is how do I submit hardware-specific changes? There is a a separate acceleration profile and default values for the ThinkPad X230 ... should I replicate this? It seems to me based on this experience that we should actually expect quirks to include idiosyncratic magic values and acceleration algorithms, but maybe the P50 is just an outlier.

Comment on attachment 138751 [details] [review]
fix duplicate flag value is evdev.h

Review of attachment 138751 [details] [review]:
-----------------------------------------------------------------

Pushed as dd6059aefc80cd540a133390a9068b4b890494bf, thanks.

> You will probably conclude to ignore my patches and re-apply the one you reverted

yep, sorry. Much easier this way :)

for reference: commit 0dccaa8a424022b7a4ed6b7022dd849e8d813225


For the touchpad profile, I think moving to bug 101139 is the best idea here. Which means that this bug can be closed, I think.