[PATCH libinput] libinput: add orientation and size of touch point and pressure to the API

[Peter Hutterer]

On Wed, Sep 02, 2015 at 12:50:47PM +0200, Andreas Pokorny wrote:
> Hi,
> 
> On Tue, Sep 1, 2015 at 8:30 AM, Peter Hutterer <peter.hutterer at who-t.net>
> wrote:
> 
> > [...]
> > but first up, I have to ask: how did you test this, do you have an
> > implementation higher up in the stack that makes use of this? The API looks
> > generally ok now, but once I started testing it it didn't turn out too
> > well.
> >
> 
> I have a small tool that prints one line per libevdev touch event and
> printed the additional data. Additionally I used it with mir. But most of
> the time I used that tool on the various phones.

please add this to event-debug then, that's the whole point of that tool

> what is the actual range of the major/minor your touchpad devices give you?
> > my touchscreen here gives me a measly 7-10 device units, translating to
> > less
> > than 1mm. The orientation works correctly (1 bit), I don't have pressure
> > here.
> > double-checking with a magic trackpad shows that the range there is
> > accurate, so now we need to figure out how commonplace the bogus
> > major/minor
> > coordinates are.
> >
> 
> I usually get values between 5 and 14. And yes since the resolution is
> usually reported with zero and corrected with 1 those values make not that
> much sense in mm. When scaled to pixels I believe applying an additional
> scaling factor would provide more accurate values. But that scaling differs
> from device to device. But with some time I could come up with correction
> values.

we need to figure out a way to fix this properly then. making API promises
about mm etc when we already know that the data is bogus is not a good
thing.
for the resolution, we're fixing this through the udev hwdb, for the mice
dpi we have the udev property, same for pointing sticks. something similar
is needed here.
 
> also: it's a bit disappointing that I had to fix the tools myself to
> > actually print or display this data. this should be part of this patch and
> > again brings up the question on how you tested this.
> > adding printfs to the event-debug tool also revealed a bug with the
> > has_major/has_minor/has_... implementation (see later in this email)
> >
> 
> Appologies, I didnt look into the directory and just realize what is there.
> Next patch will extend to that too.
> 
> 
> >  #define DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT ms2us(200)
> > > +#define DEFAULT_TOUCH_PRESSURE 1
> > > +#define DEFAULT_TOUCH_ORIENTATION 0
> > > +#define DEFAULT_TOUCH_MAJOR 0
> > > +#define DEFAULT_TOUCH_MINOR 0
> >
> > make this 0.0 and 1.0 since we're dealing with doubles
> >
> 
> 
> > +                     break;
> > +             case ABS_MT_TOUCH_MAJOR:
> > +                     current_slot->available_data |=
> TOUCH_SLOT_DATA_MAJOR;
> > +                     current_slot->area.major = e->value;
> 
> the handling of the has_major/minor/orientation isn't correct. if a device
> > has the axis, you can get the current per-slot value on device startup.
> > the value remains valid until overwritten by a new value on the device. so
> > libinput_event_touch_has_major() doesn't tell us whether a specific event
> > has a major axis, but rather whether a device do the axis at all, i.e. it's
> > either always true or always false.
> >
> 
> > the API could be libinput_device_has_... but I think the per-event API is
> > better. the current implementation is broken though, just add a printf to
> > event-debug and you'll see how we don't know major/minor until we get an
> > event for each.
> >
> 
> With my touchscreens I get major and minor on the first down. But after
> looking at the output of the tool again, it seems that the down event does
> not have those values. Instead it still holds stale values from the last
> owner of the slot. I am looking into that now. In previous versions of the
> patch I reset everything when the tracking_id is itself reset to -1. The
> current code (modulo that bug) assumes that with the mt protocol you will
> always have valid data for an axis available. There should not be a need to
> reset individual values. So yes if the touch screen supports a certain axis
> libinput_touch_event_has_ will indicate 1 on each down or move event even
> when not changed during that move or down action.

that is the correct behaviour. the kernel only gives you events when the
value _changes_, this event sequence is valid:

ABS_MT_SLOT 0
ABS_MT_TRACKING_ID 123
ABS_MT_POSITION_X 12
BTN_TOUCH 1
SYN_REPORT

and when this happens, the last ABS_MT_POSITION_Y of this slot is the
current one. it's unlikely for x/y on touch down, but for other values it's
not unheard of, especially when the value range is small.
since the client is supposed to have the value from the EVIOCGMTSLOTS ioctl
before handling events, the state is always known to the client.

so resetting on touch end is not correct either, the solution is to always
return true/false depending on the device, not the events.

> I have no objection to go back to the previous behavior and reset on
> ABS_MT_TRACKING_ID  = -1, since it wont make a difference to the user -
> code wise it seems more consistent.


> > orientation is the same: there's no orientation value for the first touch
> > point, though after that it keeps being printed (which ironically means
> > that
> > available_data is never reset).
> >
> > > +                     break;
> > > +             case ABS_MT_TOUCH_MINOR:
> > > +                     current_slot->available_data |=
> > TOUCH_SLOT_DATA_MINOR;
> > > +                     current_slot->area.minor = e->value;
> > > +                     break;
> > > +             case ABS_MT_ORIENTATION:
> > > +                     current_slot->available_data |=
> > > +
> > > +     struct axis_replacement down_values[] = {
> > > +             {ABS_MT_PRESSURE, input_pressure},
> > > +             {ABS_MT_TOUCH_MAJOR, input_major},
> > > +             {-1, -1}};
> > > +     struct axis_replacement move_values[] = {
> > > +             {ABS_MT_TOUCH_MAJOR, input_major_2},
> > > +             {-1, -1}};
> > > +
> > > +     const struct input_absinfo *orientation_info;
> > > +     const struct input_absinfo *pressure_info;
> > > +     const struct input_absinfo *x_info;
> > > +     const struct input_absinfo *major_info;
> > > +     double touch_major_scale;
> > > +
> > > +     double expected_major;
> > > +     double expected_minor;
> > > +     double expected_orientation = 0.0;
> > > +     double expected_pressure;
> > > +
> > > +     x_info = libevdev_get_abs_info(dev->evdev, ABS_MT_POSITION_X);
> > > +     pressure_info = libevdev_get_abs_info(dev->evdev, ABS_MT_PRESSURE);
> > > +     major_info = libevdev_get_abs_info(dev->evdev, ABS_MT_TOUCH_MAJOR);
> > > +     orientation_info = libevdev_get_abs_info(dev->evdev,
> > > +                                              ABS_MT_ORIENTATION);
> > > +
> > > +     if (orientation_info || !pressure_info || !x_info || !major_info ||
> >
> > do we have a touch screen that doesn't have x_info?
> > I had to look through the test devices here to make sure we have something
> > that actually runs the test - I wonder if there's some litest addition we
> > can do to make sure a test runs fully at least once. we've had the problem
> > once or twice in the past where a test returned early and the actual test
> > bits didn't get triggered by any device in the list.
> >
> 
> It is hard to resist pointer checking...
> 
> We could override the END_TEST macro, somehow mark the test to be executed
> next inside litest, and then use code inside the END_TEST macro to indicate
> that the test completed without bailing out.

urgh, that's going to be ugly. but yes, I don't see any other way around it.

> 
> 
> > > +         !libevdev_has_event_code(dev->evdev, EV_ABS,
> > ABS_MT_TOUCH_MAJOR) ||
> > > +         libevdev_has_event_code(dev->evdev, EV_ABS,
> > ABS_MT_TOUCH_MINOR))
> > > +             return;
> > > +
> > > +     if (exceeds_range(pressure_info, input_pressure) ||
> > > +         exceeds_range(major_info, input_major) ||
> > > +         exceeds_range(major_info, input_major_2)) {
> > > +             fprintf(stderr, "%s does not support the required value
> > ranges",
> > > +                     libinput_device_get_name(dev->libinput_device));
> >
> 
> Wrt ck_abort_msg the intention here and above it so to provide some info
> that the device was not used for this test instead of failing the test. A
> ck_warn_msg instead of failure. Then again I could also add another layer
> of calculation and scale the test input values to the ranges supported by
> the device. But it already starts to feel wrong that I have put
> calculations mirroring libinputs behavior inside the test. Doing that would
> mean adapting the test itself to make it fit to the growing set of devices
> in the suite. The idea behind those two tests was that the added properties
> are passed through libinput and exposed to the user of the API, and that if
> the device indicates changes those will be indicated too. The second test
> just adds the bevior for default values.

if you add such a warning and we have a test device that triggers it, you'll
get the warning every time you run the test. this isn't any better than
aborting and fixing the test.

besides, you're picking magic values and then trusting that the device that
has the values somehow passes the test. That's not much better than
calculating what the values _should_ be in the test, and then checking for
what's coming out of libinput to be that value.

> The point I am trying to get at is that this problem seems to be an
> artifact of the test strategy. I believe tests should be as self contained
> as possible, test one thing, be simple to read, not have their own logic
> the reader has to interpret, otherwise they themselves might contain
> failures or errors in the logic might hide errors in the test code. 

I agree, but reality gets in the way of that :)
the majority of test cases are short and simple, but there are some that are
more complicated.

> With
> the current approach there is a part of the input parameters hard coded or
> configureable inside the device description. A central entity selecting
> which tests to run on which simulated device (litest and the test
> registry), and then the test itself. That being said I dont have an easy
> solution. So for most of the logic I added smaller unit tests that run only
> internal parts libinput without a simulated device would have been
> sufficient. Then a simple acceptance test that declares a simulated device
> for the test and has hard coded input and output values.. To get the unit
> tests working without a uinput device requires creating interfaces that can
> be overriden, adding that would bubble upwards through the internals of
> libinput.

yep, there's no easy solution. The current test framework is designed for
one priority: we don't want to break known features on new devices. So most
of how it works is geared towards adding a new device and then making sure
nothing breaks. because for evdev/synaptics/... that is the biggest problem
- you'd fix something for one device and another one breaks.

for some tests, this is not ideal because you start getting blurry at the
details, e.g. pointer acceleration is almost impossible to test this way.

unit-testing libinput itself would be interesting, but the changes required
are, as you said, intrusive. and I don't know how much you'd get out of it,
libinput requires a lot of state, so I don't put my hopes up that
unit-testing is easy to add.

Cheers,
   Peter