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

[Andreas Pokorny]

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.

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.

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.

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.


> > +         !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.

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. 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.

Fixed all the other findings locally. Thanks for the review!

regards
Andreas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150902/e94a0012/attachment.html>