[PATCH 2/7] libinput: add orientation and size of touch point and pressure to the API
Peter Hutterer
peter.hutterer at who-t.net
Wed Jun 10 23:04:58 PDT 2015
On Wed, Jun 10, 2015 at 04:09:12PM +0200, Andreas Pokorny wrote:
> This change adds four new properties to touch events. These properties are
> directly forwarded from the evdev interface.
I assume you have a (prospective) user for this already? If so, how are you
using these?
first comment here is: no device-specific values, a major point of libinput
is to take the guesswork out of the input stack. forwarding device-specific
values doesn't help with that.
so all of the new get_touch_* need to be normalized from [0, 1.0], and
orientation needs to be defined in degrees.
A device that can't do this needs to be handled accordingly within libinput.
> ---
> src/evdev.c | 81 ++++++++++++++++++++++++++++++++++++--------------
> src/evdev.h | 4 +++
> src/libinput-private.h | 12 ++++++--
> src/libinput.c | 72 ++++++++++++++++++++++++++++++++++++++++++--
> src/libinput.h | 73 +++++++++++++++++++++++++++++++++++++++++++++
> src/libinput.sym | 8 +++++
> test/touch.c | 51 +++++++++++++++++++++++++++++++
> 7 files changed, 275 insertions(+), 26 deletions(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index d6a2fff..af507d6 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -43,6 +43,10 @@
>
> #define DEFAULT_WHEEL_CLICK_ANGLE 15
> #define DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT 200
> +#define DEFAULT_TOUCH_PRESSURE 1
> +#define DEFAULT_TOUCH_ORIENTATION 0
> +#define DEFAULT_TOUCH_MAJOR 1
> +#define DEFAULT_TOUCH_MINOR 1
>
> enum evdev_key_type {
> EVDEV_KEY_TYPE_NONE,
> @@ -262,6 +266,7 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
> struct libinput_seat *seat = base->seat;
> struct normalized_coords accel, unaccel;
> struct device_coords point;
> + struct mt_slot *slot_data;
>
> slot = device->mt.slot;
>
> @@ -313,25 +318,32 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
> break;
>
> seat->slot_map |= 1 << seat_slot;
> - point = device->mt.slots[slot].point;
> + slot_data = &device->mt.slots[slot];
> + point = slot_data->point;
> transform_absolute(device, &point);
>
> touch_notify_touch_down(base, time, slot, seat_slot,
> - &point);
> + &point, slot_data->touch_major,
> + slot_data->touch_minor, slot_data->orientation,
> + slot_data->pressure);
> break;
> case EVDEV_ABSOLUTE_MT_MOTION:
> if (!(device->seat_caps & EVDEV_DEVICE_TOUCH))
> break;
>
> seat_slot = device->mt.slots[slot].seat_slot;
> - point = device->mt.slots[slot].point;
> +
> + slot_data = &device->mt.slots[slot];
> + point = slot_data->point;
>
> if (seat_slot == -1)
> break;
>
> transform_absolute(device, &point);
> touch_notify_touch_motion(base, time, slot, seat_slot,
> - &point);
> + &point, slot_data->touch_major,
> + slot_data->touch_minor, slot_data->orientation,
> + slot_data->pressure);
> break;
> case EVDEV_ABSOLUTE_MT_UP:
> if (!(device->seat_caps & EVDEV_DEVICE_TOUCH))
> @@ -370,7 +382,8 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
> point = device->abs.point;
> transform_absolute(device, &point);
>
> - touch_notify_touch_down(base, time, -1, seat_slot, &point);
> + touch_notify_touch_down(base, time, -1, seat_slot, &point, DEFAULT_TOUCH_MAJOR,
> + DEFAULT_TOUCH_MINOR, DEFAULT_TOUCH_ORIENTATION, DEFAULT_TOUCH_PRESSURE);
> break;
> case EVDEV_ABSOLUTE_MOTION:
> point = device->abs.point;
> @@ -382,8 +395,9 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
> if (seat_slot == -1)
> break;
>
> - touch_notify_touch_motion(base, time, -1, seat_slot,
> - &point);
> + touch_notify_touch_motion(base, time, -1, seat_slot, &point,
> + DEFAULT_TOUCH_MAJOR, DEFAULT_TOUCH_MINOR, DEFAULT_TOUCH_ORIENTATION,
> + DEFAULT_TOUCH_PRESSURE);
> } else if (device->seat_caps & EVDEV_DEVICE_POINTER) {
> pointer_notify_motion_absolute(base, time, &point);
> }
> @@ -538,13 +552,19 @@ evdev_process_key(struct evdev_device *device,
> }
> }
>
> +static inline void
> +wake_device_on_mt_motion(struct evdev_device* device) {
{ on separate line please, and below: no {} for single-line conditions
> + if (device->pending_event == EVDEV_NONE) {
> + device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> + }
> +}
the function name and what it does seem to be at odds.
device_set_pending_motion_flag() or something would be better here.
I wonder if the best solution to this would be to make the pending_event
flags actual flags, then you don't have to mess with the EVDEV_NONE check.
it would make evdev_flush_pending_event() into an if/else chain but I'm
tempted to have that split out into helper functions per type anyway.
> +
> static void
> evdev_process_touch(struct evdev_device *device,
> struct input_event *e,
> uint64_t time)
> {
> - switch (e->code) {
> - case ABS_MT_SLOT:
> + if (e->code == ABS_MT_SLOT) {
> if ((size_t)e->value >= device->mt.slots_len) {
> log_bug_libinput(device->base.seat->libinput,
> "%s exceeds slots (%d of %d)\n",
> @@ -555,8 +575,7 @@ evdev_process_touch(struct evdev_device *device,
> }
> evdev_flush_pending_event(device, time);
> device->mt.slot = e->value;
> - break;
> - case ABS_MT_TRACKING_ID:
> + } else if(e->code == ABS_MT_TRACKING_ID) {
> if (device->pending_event != EVDEV_NONE &&
> device->pending_event != EVDEV_ABSOLUTE_MT_MOTION)
> evdev_flush_pending_event(device, time);
> @@ -564,17 +583,35 @@ evdev_process_touch(struct evdev_device *device,
> device->pending_event = EVDEV_ABSOLUTE_MT_DOWN;
> else
> device->pending_event = EVDEV_ABSOLUTE_MT_UP;
> - break;
> - case ABS_MT_POSITION_X:
> - device->mt.slots[device->mt.slot].point.x = e->value;
> - if (device->pending_event == EVDEV_NONE)
> - device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> - break;
> - case ABS_MT_POSITION_Y:
> - device->mt.slots[device->mt.slot].point.y = e->value;
> - if (device->pending_event == EVDEV_NONE)
> - device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> - break;
> + } else {
> + struct mt_slot *current_slot = &device->mt.slots[device->mt.slot];
I assume this line is the reason for the if condition replacing the switch?
device->mt.slot is always defined, we get it from the ioctl on startup. so
you can leave the switch in place and do the current_slot assignment
before the switch.
> +
> + switch(e->code) {
> + case ABS_MT_POSITION_X:
> + current_slot->point.x = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + case ABS_MT_POSITION_Y:
> + current_slot->point.y = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + case ABS_MT_TOUCH_MAJOR:
> + current_slot->touch_major = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + case ABS_MT_TOUCH_MINOR:
> + current_slot->touch_minor = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + case ABS_MT_ORIENTATION:
> + current_slot->orientation = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + case ABS_MT_PRESSURE:
> + current_slot->pressure = e->value;
> + wake_device_on_mt_motion(device);
> + break;
> + }
looking at the wake_device_on_mt_motion again: you're saving one line here
per call, is it really worth it? how about a motion_update = true; in the
switch cases and then have the == EVDEV_NONE condition at the end of
evdev_process_touch.
> }
> }
>
> diff --git a/src/evdev.h b/src/evdev.h
> index a875663..b8ef599 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -108,6 +108,10 @@ enum evdev_device_model {
> struct mt_slot {
> int32_t seat_slot;
> struct device_coords point;
> + int32_t touch_major;
> + int32_t touch_minor;
> + int32_t orientation;
> + int32_t pressure;
> };
>
> struct evdev_device {
> diff --git a/src/libinput-private.h b/src/libinput-private.h
> index 889ede9..c34f79a 100644
> --- a/src/libinput-private.h
> +++ b/src/libinput-private.h
> @@ -344,14 +344,22 @@ touch_notify_touch_down(struct libinput_device *device,
> uint64_t time,
> int32_t slot,
> int32_t seat_slot,
> - const struct device_coords *point);
> + const struct device_coords *point,
> + int32_t touch_major,
> + int32_t touch_minor,
> + int32_t orientation,
> + int32_t pressure);
>
> void
> touch_notify_touch_motion(struct libinput_device *device,
> uint64_t time,
> int32_t slot,
> int32_t seat_slot,
> - const struct device_coords *point);
> + const struct device_coords *point,
> + int32_t touch_major,
> + int32_t touch_minor,
> + int32_t orientation,
> + int32_t pressure);
>
> void
> touch_notify_touch_up(struct libinput_device *device,
> diff --git a/src/libinput.c b/src/libinput.c
> index 72d27e4..92cc790 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -111,6 +111,10 @@ struct libinput_event_touch {
> int32_t slot;
> int32_t seat_slot;
> struct device_coords point;
> + int32_t touch_major;
> + int32_t touch_minor;
> + int32_t orientation;
> + int32_t pressure;
> };
>
> static void
> @@ -636,6 +640,54 @@ libinput_event_touch_get_y(struct libinput_event_touch *event)
> return evdev_convert_to_mm(device->abs.absinfo_y, event->point.y);
> }
>
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_touch_major(struct libinput_event_touch *event)
> +{
> + require_event_type(libinput_event_get_context(&event->base),
> + event->base.type,
> + 0,
> + LIBINPUT_EVENT_TOUCH_DOWN,
> + LIBINPUT_EVENT_TOUCH_MOTION);
> +
> + return event->touch_major;
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_touch_minor(struct libinput_event_touch *event)
> +{
> + require_event_type(libinput_event_get_context(&event->base),
> + event->base.type,
> + 0,
> + LIBINPUT_EVENT_TOUCH_DOWN,
> + LIBINPUT_EVENT_TOUCH_MOTION);
> +
> + return event->touch_minor;
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_orientation(struct libinput_event_touch *event)
> +{
> + require_event_type(libinput_event_get_context(&event->base),
> + event->base.type,
> + 0,
> + LIBINPUT_EVENT_TOUCH_DOWN,
> + LIBINPUT_EVENT_TOUCH_MOTION);
> +
> + return event->orientation;
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_pressure(struct libinput_event_touch *event)
> +{
> + require_event_type(libinput_event_get_context(&event->base),
> + event->base.type,
> + 0,
> + LIBINPUT_EVENT_TOUCH_DOWN,
> + LIBINPUT_EVENT_TOUCH_MOTION);
> +
> + return event->pressure;
> +}
> +
> struct libinput_source *
> libinput_add_fd(struct libinput *libinput,
> int fd,
> @@ -1242,7 +1294,11 @@ touch_notify_touch_down(struct libinput_device *device,
> uint64_t time,
> int32_t slot,
> int32_t seat_slot,
> - const struct device_coords *point)
> + const struct device_coords *point,
> + int32_t touch_major,
> + int32_t touch_minor,
> + int32_t orientation,
> + int32_t pressure)
> {
> struct libinput_event_touch *touch_event;
>
> @@ -1258,6 +1314,10 @@ touch_notify_touch_down(struct libinput_device *device,
> .slot = slot,
> .seat_slot = seat_slot,
> .point = *point,
> + .touch_major = touch_major,
> + .touch_minor = touch_minor,
> + .orientation = orientation,
> + .pressure = pressure,
> };
>
> post_device_event(device, time,
> @@ -1270,7 +1330,11 @@ touch_notify_touch_motion(struct libinput_device *device,
> uint64_t time,
> int32_t slot,
> int32_t seat_slot,
> - const struct device_coords *point)
> + const struct device_coords *point,
> + int32_t touch_major,
> + int32_t touch_minor,
> + int32_t orientation,
> + int32_t pressure)
> {
> struct libinput_event_touch *touch_event;
>
> @@ -1286,6 +1350,10 @@ touch_notify_touch_motion(struct libinput_device *device,
> .slot = slot,
> .seat_slot = seat_slot,
> .point = *point,
> + .touch_major = touch_major,
> + .touch_minor = touch_minor,
> + .orientation = orientation,
> + .pressure = pressure,
> };
>
> post_device_event(device, time,
> diff --git a/src/libinput.h b/src/libinput.h
> index 845e1c5..deecdda 100644
> --- a/src/libinput.h
> +++ b/src/libinput.h
> @@ -914,6 +914,79 @@ libinput_event_touch_get_y_transformed(struct libinput_event_touch *event,
> /**
> * @ingroup event_touch
> *
> + * Return the current minor diameter of the touch ellipse in device specific
> + * coordinates. This value might not be provided by the device.
> + *
how does a caller know whether a value is "not provided" or a valid value?
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current touch minor value
> + */
> +double
> +libinput_event_touch_get_touch_minor(struct libinput_event_touch *event);
'touch_get_touch' seems superfluous, drop the second _touch.
> +
> +/**
> + * @ingroup event_touch
> + *
> + * Return the current major diameter of the touch ellipse in device
> + * specific coordinates. This value might not be provided by the device.
> + *
brownie points for an ascii art here explaining the major/minor :)
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current touch major value
> + */
> +double
> +libinput_event_touch_get_touch_major(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * Return the current pressure value touch point as a device specific value.
> + * This value might not be measured by the device.
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current pressure value
> + */
> +double
> +libinput_event_touch_get_pressure(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * Return the orientation of the touch ellipse in device specific values.
> + * If the ellipse is aligned with the Y Axis of the screen 0 should be
> + * returned.
hint: "should" does not a good API :)
but even then it's very blurry. if I get 1 - what does this mean? is it
better than 10? without that specification, the api is boolean because
anything non-zero is meaningless. so let's define this in degrees, clockwise
off the north-facing y axis and do the rest ourselves where needed.
in addition: how does the orientation affect the major/minor values? this
needs to be specified.
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current orientation value
> + */
> +double
> +libinput_event_touch_get_orientation(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
> * @return The generic libinput_event of this event
> */
> struct libinput_event *
> diff --git a/src/libinput.sym b/src/libinput.sym
> index 9c11174..9be5a9a 100644
> --- a/src/libinput.sym
> +++ b/src/libinput.sym
> @@ -140,3 +140,11 @@ LIBINPUT_0.15.0 {
> global:
> libinput_device_keyboard_has_key;
> } LIBINPUT_0.14.0;
> +
> +LIBINPUT_0.18.0 {
> +global:
> + libinput_event_touch_get_touch_minor;
> + libinput_event_touch_get_touch_major;
> + libinput_event_touch_get_orientation;
> + libinput_event_touch_get_pressure;
> +} LIBINPUT_0.15.0;
> diff --git a/test/touch.c b/test/touch.c
> index be2bea9..7a69bd7 100644
> --- a/test/touch.c
> +++ b/test/touch.c
> @@ -651,6 +651,55 @@ START_TEST(touch_initial_state)
> }
> END_TEST
>
> +START_TEST(touch_point_properties)
> +{
> + struct litest_device *dev = litest_current_device();
> + struct libinput *li = dev->libinput;
> + struct libinput_event *ev;
> + struct libinput_event_touch *tev;
> +
> + litest_drain_events(li);
> +
> + litest_push_event_frame(dev);
> + litest_touch_down(dev, 0, 5, 95);
> + litest_touch_pressure(dev, 0, 128);
> + litest_touch_orientation(dev, 0, 64);
> + litest_touch_major(dev, 0, 14);
> + litest_touch_minor(dev, 0, 8);
> + litest_pop_event_frame(dev);
ok, now having seen this I'm convinced that the approach with passing an
axis array in is better than the push/pop requirement here.
or maybe a litest_touch_down_v() that takes an event code + value pair but I
suspect that will be too inflexible.
> +
> + litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_DOWN, -1);
> +
> + ev = libinput_get_event(li);
> + tev = libinput_event_get_touch_event(ev);
> +
> + ck_assert_int_eq(libinput_event_touch_get_slot(tev), 0);
> + ck_assert_int_eq(libinput_event_touch_get_pressure(tev), 128);
> + ck_assert_int_eq(libinput_event_touch_get_orientation(tev), 64);
> + ck_assert_int_eq(libinput_event_touch_get_touch_major(tev), 14);
> + ck_assert_int_eq(libinput_event_touch_get_touch_minor(tev), 8);
> +
> + libinput_event_destroy(ev);
> +
> + litest_push_event_frame(dev);
> + litest_touch_orientation(dev, 0, 128);
> + litest_touch_major(dev, 0, 30);
> + litest_pop_event_frame(dev);
> + litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_MOTION, -1);
> +
> +
> + ev = libinput_get_event(li);
> + tev = libinput_event_get_touch_event(ev);
> +
> + ck_assert_int_eq(libinput_event_touch_get_pressure(tev), 128);
> + ck_assert_int_eq(libinput_event_touch_get_orientation(tev), 128);
> + ck_assert_int_eq(libinput_event_touch_get_touch_major(tev), 30);
> + ck_assert_int_eq(libinput_event_touch_get_touch_minor(tev), 8);
> +
> + libinput_event_destroy(ev);
> +}
> +END_TEST
> +
> void
> litest_setup_tests(void)
> {
> @@ -675,6 +724,8 @@ litest_setup_tests(void)
> litest_add("touch:protocol a", touch_protocol_a_init, LITEST_PROTOCOL_A, LITEST_ANY);
> litest_add("touch:protocol a", touch_protocol_a_touch, LITEST_PROTOCOL_A, LITEST_ANY);
> litest_add("touch:protocol a", touch_protocol_a_2fg_touch, LITEST_PROTOCOL_A, LITEST_ANY);
> + litest_add("touch:touch point properties", touch_point_properties, LITEST_TOUCH | LITEST_ELLIPSE_SIZE |
> + LITEST_ELLIPSE_ORIENTATION | LITEST_PRESSURE, LITEST_ANY);
no spaces around | for these flags, all on one line please (the only
exception to the 80 char rule)
Cheers,
Peter
>
> litest_add_ranged("touch:state", touch_initial_state, LITEST_TOUCH, LITEST_PROTOCOL_A, &axes);
> }
> --
> 2.1.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list