[PATCH libinput] Fix premature flushing of evdev event on mx4 touchscreen

Peter Hutterer peter.hutterer at who-t.net
Sun Sep 6 18:51:23 PDT 2015


On Sun, Sep 06, 2015 at 02:59:07PM +0200, Andreas Pokorny wrote:
> The mx4 touchscreen driver emits BTN_TOUCH and BTN_TOOL_FINGER key events
> on a new contact. Prior to this patch only the BTN_TOUCH event was filtered
> out. Now the whole range of BTN_ events for tool types and certain builtin
> multi finger gestures are marked as non-key type.
> 
> This patch is based on v5 of the touch property extenion patches.
> 
> Signed-off-by: Andreas Pokorny <andreas.pokorny at canonical.com>
> ---
>  src/evdev.c  |  10 ++---
>  test/touch.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 04df275..3e22aa9 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -557,7 +557,7 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  static enum evdev_key_type
>  get_key_type(uint16_t code)
>  {
> -	if (code == BTN_TOUCH)
> +	if (code >= BTN_DIGI && code <= BTN_TOOL_QUADTAP)
>  		return EVDEV_KEY_TYPE_NONE;
>  
>  	if (code >= KEY_ESC && code <= KEY_MICMUTE)
> @@ -630,16 +630,16 @@ evdev_process_key(struct evdev_device *device,
>  	if (e->value == 2)
>  		return;
>  
> -	if (e->code == BTN_TOUCH) {
> -		if (!device->is_mt)
> +	type = get_key_type(e->code);
> +
> +	if (type == EVDEV_KEY_TYPE_NONE) {
> +		if (e->code == BTN_TOUCH && !device->is_mt)
>  			evdev_process_touch_button(device, time, e->value);
>  		return;
>  	}
>  
>  	evdev_flush_pending_event(device, time);
>  
> -	type = get_key_type(e->code);
> -
>  	/* Ignore key release events from the kernel for keys that libinput
>  	 * never got a pressed event for. */
>  	if (e->value == 0) {

this part looks good

> diff --git a/test/touch.c b/test/touch.c
> index 875d94d..05d1ff3 100644
> --- a/test/touch.c
> +++ b/test/touch.c
> @@ -912,6 +912,129 @@ START_TEST(touch_point_no_minor_or_orientation)
>  }
>  END_TEST
>  
> +START_TEST(touchscreen_with_btn_tool_finger_on_down)
> +{
> +	struct libevdev_uinput *uinput;
> +	struct libinput *li;
> +	struct libinput_device *device;
> +	struct libinput_event *event;
> +	struct libinput_event_touch *tevent;

quick grep shows all the other test use 'tev', let's keep it consistent.

> +	const struct input_absinfo abs[] = {
> +		{ ABS_X, 0, 1152, 0, 0, 0 },
> +		{ ABS_Y, 0, 1920, 0, 0, 0 },
> +		{ ABS_MT_SLOT, 0, 9, 0, 0, 0 },
> +		{ ABS_MT_TRACKING_ID, 0, 65535, 0, 0, 0 },
> +		{ ABS_MT_POSITION_X, 0, 1152, 0, 0, 0 },
> +		{ ABS_MT_POSITION_Y, 0, 1920, 0, 0, 0 },
> +		{ ABS_MT_TOUCH_MAJOR, 0, 23, 0, 0, 0 },
> +		{ ABS_MT_TOUCH_MINOR, 0, 23, 0, 0, 0 },
> +		{ -1, -1, -1, -1, -1, -1 }
> +	};
> +	const struct input_event input_sequence[] = {
> +		{ {0}, EV_ABS, ABS_MT_SLOT, 0},
> +		{ {0}, EV_ABS, ABS_MT_TRACKING_ID, 9},
> +		{ {0}, EV_KEY, BTN_TOUCH, 1},
> +		{ {0}, EV_KEY, BTN_TOOL_FINGER, 1},
> +		{ {0}, EV_ABS, ABS_MT_POSITION_X, 128},
> +		{ {0}, EV_ABS, ABS_MT_POSITION_Y, 240},
> +		{ {0}, EV_ABS, ABS_MT_TOUCH_MAJOR, 5},
> +		{ {0}, EV_ABS, ABS_MT_TOUCH_MINOR, 4},
> +		{ {0}, EV_SYN, SYN_REPORT, 0},
> +		{ {0}, EV_ABS, ABS_MT_TOUCH_MAJOR, 6},
> +		{ {0}, EV_SYN, SYN_REPORT, 0},
> +		{ {0}, EV_ABS, ABS_MT_TRACKING_ID, -1},
> +		{ {0}, EV_KEY, BTN_TOUCH, 0},
> +		{ {0}, EV_KEY, BTN_TOOL_FINGER, 0},
> +		{ {0}, EV_SYN, SYN_REPORT, 0}
> +	};

I'm wondering - wouldn't it be worth to add this device as a separate litest
device and add the BTN_TOOL_FINGER in the down events for that device?
hand-crafted devices and event sequences should be for custom devices and
quirky sequences, this one appears to be pretty stock-standard and thus we
expect everything else to work fine as well.

plus, I suspect if you add this as a normal test device, the other tests
will fail so you may not even need a  separate test.

> +	const int num_events = sizeof input_sequence / sizeof input_sequence[0];

ARRAY_LENGTH

> +	const int width = 1152;
> +	const int height = 1920;
> +
> +	uinput = litest_create_uinput_abs_device(
> +		"test device",
> +		NULL, abs,
> +		EV_KEY, BTN_TOUCH,
> +		EV_KEY, BTN_TOOL_FINGER,
> +		INPUT_PROP_MAX, INPUT_PROP_DIRECT,
> +		-1, -1);
> +
> +	li = litest_create_context();
> +	device = libinput_path_add_device(li,
> +					  libevdev_uinput_get_devnode(uinput));
> +	ck_assert(device != NULL);
> +	device = libinput_device_ref(device);
> +
> +	for (int i = 0; i!=num_events; ++i)
> +		libevdev_uinput_write_event(uinput,
> +					    input_sequence[i].type,
> +					    input_sequence[i].code,
> +					    input_sequence[i].value);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	ck_assert_int_eq(libinput_event_get_type(event), LIBINPUT_EVENT_DEVICE_ADDED);
> +	libinput_event_destroy(event);

drop this group, use a litest_drain_events() instead after the add device
and before you start sending touch events.

> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_DOWN);
> +	ck_assert_int_eq(
> +		round(libinput_event_touch_get_x_transformed(tevent, width)),
> +		128);
> +	ck_assert_int_eq(
> +		round(libinput_event_touch_get_y_transformed(tevent, height)),
> +		240);
> +	ck_assert_int_eq(
> +		round(libinput_event_touch_get_major_transformed(tevent,
> +								 width,
> +								 height)),
> +		5);
> +	ck_assert_int_eq(
> +		round(libinput_event_touch_get_minor_transformed(tevent,
> +								 width,
> +								 height)),
> +		4);

this is pretty unreadable, a tmp variable "value" would make this a lot
better.

> +	libinput_event_destroy(event);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_FRAME);
> +	libinput_event_destroy(event);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_MOTION);
> +	ck_assert_int_eq(
> +		round(libinput_event_touch_get_major_transformed(tevent,
> +								 width,
> +								 height)),
> +		6);
> +	libinput_event_destroy(event);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_FRAME);
> +	libinput_event_destroy(event);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_UP);
> +	libinput_event_destroy(event);
> +
> +	litest_wait_for_event(li);
> +	event = libinput_get_event(li);
> +	tevent = litest_is_touch_event(event, LIBINPUT_EVENT_TOUCH_FRAME);

you only need litest_wait_for_event() once after libinput_dispatch() or if
you're expecting a timer to expire. both isn't true here, so simply skip
them.

> +	libinput_event_destroy(event);
> +
> +	ck_assert_int_eq(libinput_next_event_type(li), LIBINPUT_EVENT_NONE);

litest_assert_empty_queue()

> +
> +	libinput_device_unref(device);
> +	libinput_unref(li);
> +	libevdev_uinput_destroy(uinput);
> +}
> +END_TEST
> +
>  void
>  litest_setup_tests(void)
>  {
> @@ -942,4 +1065,6 @@ litest_setup_tests(void)
>  	litest_add_ranged("touch:state", touch_initial_state, LITEST_TOUCH, LITEST_PROTOCOL_A, &axes);
>  
>  	litest_add("touch:time", touch_time_usec, LITEST_TOUCH, LITEST_TOUCHPAD);
> +
> +	litest_add_no_device("touch:filter EV_KEY BTN_TOOL_FINGER", touchscreen_with_btn_tool_finger_on_down);

the string in the test is a suite name (comes from the check framework) and
serves to limit tests to a category of tests. e.g. you can run the
"touch:calibration" suite to check all calibration tests. 
it's not to label a specific test, that's what the function name is for.
Please come up with something more generic, "touch:special events" or so
maybe.

Cheers,
   Peter


>  }
> -- 
> 2.5.0
> 


More information about the wayland-devel mailing list