[PATCH libinput 6/6] tablet: add touch arbitration

[Peter Hutterer]

On Thu, Sep 01, 2016 at 03:48:39PM -0700, Jason Gerecke wrote:
> The first 5 patches look fine, but I have a few questions about this
> one, especially in the unit tests:
> 
[...]
> > @@ -1466,6 +1500,16 @@ tablet_process(struct evdev_dispatch *dispatch,
> >  }
> >  
> >  static void
> > +tablet_suspend(struct evdev_dispatch *dispatch,
> > +	       struct evdev_device *device)
> > +{
> > +	struct tablet_dispatch *tablet =
> > +		(struct tablet_dispatch *)dispatch;
> > +
> > +	tablet_set_touch_device_enabled(tablet->touch_device, true);
> 
> If the touch device was already suspended for some reason, doesn't this
> mean that suspending the pen will suddenly un-suspend it? I suppose if
> the only way touch can be suspended is via arbitration then this will
> work, but it could change down the road...

yes, good catch, this is indeed broken. I have  a test case for this now
and will fix this. We don't have an API to suspend a device but we do have
the send-events configuration that maps to suspend (currently anyway).

[...]

> > +START_TEST(intuos_touch_arbitration)
> > +{
> > +	struct litest_device *dev = litest_current_device();
> > +	struct litest_device *finger;
> > +	struct libinput *li = dev->libinput;
> > +	struct axis_replacement axes[] = {
> > +		{ ABS_DISTANCE, 10 },
> > +		{ ABS_PRESSURE, 0 },
> > +		{ -1, -1 }
> > +	};
> > +
> > +	litest_tablet_proximity_in(dev, 10, 10, axes);
> > +	litest_drain_events(li);
> > +
> > +	finger = litest_add_device(li, LITEST_WACOM_FINGER);
> > +	litest_drain_events(li);
> > +
> > +	litest_tablet_proximity_in(dev, 10, 10, axes);
> 
> Isn't this proximity event (or the first prox/drain sequence above)
> redundant? Similar "redundant" events occur multiple times below, so its
> hard to tell if they are or if its just copy/paste :)

yep, they're all copy/pasto, fixed locally.

[...]

> > +START_TEST(intuos_touch_arbitration_stop_touch)
> > +{
> > +	struct litest_device *dev = litest_current_device();
> > +	struct litest_device *finger;
> > +	struct libinput *li = dev->libinput;
> > +	struct axis_replacement axes[] = {
> > +		{ ABS_DISTANCE, 10 },
> > +		{ ABS_PRESSURE, 0 },
> > +		{ -1, -1 }
> > +	};
> > +
> > +	finger = litest_add_device(li, LITEST_WACOM_FINGER);
> > +	litest_touch_down(finger, 0, 30, 30);
> > +	litest_touch_move_to(finger, 0, 30, 30, 80, 80, 10, 1);
> > +
> > +	litest_tablet_proximity_in(dev, 10, 10, axes);
> > +	litest_drain_events(li);
> > +
> > +	litest_tablet_proximity_in(dev, 10, 10, axes);
> > +	litest_tablet_motion(dev, 10, 10, axes);
> > +	litest_tablet_motion(dev, 20, 40, axes);
> > +	litest_drain_events(li);
> > +
> > +	litest_touch_move_to(finger, 0, 80, 80, 30, 30, 10, 1);
> > +	litest_assert_empty_queue(li);
> 
> For what its worth, this might not always be the ideal form of
> arbitration. It matches what the kernel does (and we haven't heard any
> complaints), but another option would be to allow the currently-in-prox
> device to suspend its sibling even if that happens to mean touch
> suspends pen. Its may be more useful on devices which can sense the pen
> at greater distances (where you don't want to stop a deliberate touch
> due to a pen held in the same hand at the edge of proximity) and is the
> approach taken by xf86-input-wacom.
> 
> Just something to consider if you feel like experimenting with the
> arbitration's "feel".

the exact behaviour is internal only so we could have different behaviour
for different pens. but let's get *something* working first, then we can
fine-tune where needed.

Cheers,
   Peter