[PATCH libinput 7/8] evdev: Release still pressed keys/buttons when removing device
Peter Hutterer
peter.hutterer at who-t.net
Wed Jul 16 21:25:25 PDT 2014
On Wed, Jul 16, 2014 at 10:39:12PM +0200, Jonas Ådahl wrote:
> Keep track of pressed keys and buttons in a bitmask array and iterate
> through it on device removal releasing every still pressed key.
fwiw, the kernel should release all keys on disconnect these days, but for
the manual removal it's necessary.
> This commit enables _GNU_SOURCE features in evdev.c, more specifically
> static_assert(). This is supported by gcc 4.6 and above, but is not part
> of the C standard until C11. configure will fail if static_assert()
> doesn't work whith _GNU_SOURCE defined.
>
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> configure.ac | 11 +++++
> src/evdev.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/evdev.h | 14 ++++++
> test/keyboard.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++
> test/pointer.c | 91 ++++++++++++++++++++++++++++++++++++++
> 5 files changed, 362 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index fd402e2..e35fff6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -67,6 +67,17 @@ fi
> AC_SUBST(GCC_CFLAGS)
> AC_SUBST(GCC_CXXFLAGS)
>
> +AC_MSG_CHECKING([whether static_assert() is supported])
> +AC_COMPILE_IFELSE(
> + [AC_LANG_SOURCE([[#define _GNU_SOURCE
> + #include <assert.h>
> + static_assert(1, "Test");
> + ]])],
> + [have_static_assert=1
> + AC_MSG_RESULT([yes])],
> + [have_static_assert=0
> + AC_MSG_ERROR([no built-in static_assert])])
we don't seem to be using have_static_assert anywhere.
> +
> AC_PATH_PROG(DOXYGEN, [doxygen])
> if test "x$DOXYGEN" = "x"; then
> AC_MSG_WARN([doxygen not found - required for documentation])
> diff --git a/src/evdev.c b/src/evdev.c
> index c031258..3fb5ab4 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -21,6 +21,8 @@
> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> */
>
> +#define _GNU_SOURCE
> +
> #include "config.h"
>
> #include <errno.h>
> @@ -47,6 +49,87 @@ enum evdev_key_type {
> EVDEV_KEY_TYPE_BUTTON,
> };
>
> +static void
> +get_key_index_and_bit(int code, int *index, int *bit)
> +{
> + static_assert((KEY_CNT % 64) == 0, "KEY_CNT not aligned to 64");
we could just use NLONGS and LONG_BITS from libevdev. not that I'm against
static_assert but using the same macros across the board makes things a bit
easier.
> +
> + *index = code / 64;
> + *bit = code % 64;
> +}
> +
> +static int
> +get_key_state(struct evdev_device *device, int code)
> +{
> + int index = 0;
> + int bit = 0;
> +
> + get_key_index_and_bit(code, &index, &bit);
> + return !!(device->key_mask[index] & (1ULL << bit));
> +}
> +
> +static void
> +set_key_state(struct evdev_device *device, int code, int pressed)
> +{
> + int index = 0;
> + int bit = 0;
> +
> + get_key_index_and_bit(code, &index, &bit);
> +
> + if (pressed)
> + device->key_mask[index] |= (1ULL << bit);
> + else
> + device->key_mask[index] &= ~(1ULL << bit);
> +}
I think it'd be easier to use bit_is_set/set_bit/clear_bit helper functions
like in 812ea542e7023c4a16c02a0c2264fb873d34abe4 on the tablet-support
branch? those came from libevdev which handles longs, so just copying the
ones from libevdev-util.h and renaming them to longbit_is_set, etc. would be
the best approach here IMO.
> +
> +void
> +evdev_keyboard_notify_key(struct evdev_device *device,
> + uint32_t time,
> + int key,
> + enum libinput_key_state state)
> +{
> + struct libinput *libinput = device->base.seat->libinput;
> +
> + if ((state == LIBINPUT_KEY_STATE_PRESSED &&
> + get_key_state(device, key) != 0) ||
> + (state == LIBINPUT_KEY_STATE_RELEASED &&
> + get_key_state(device, key) != 1)) {
I'd prefer a boolean key_is_down() instead of get_state, especially since we're
not the LIBINPUT_KEY_STATE enums here. but given get_key_state is only used
here it'd be even easier to return the enum from get_key_state and
check for state != get_key_state directly.
> + log_bug_kernel(
> + libinput,
> + "%s: Driver sent multiple (0x%x) pressed/released",
> + device->devnode, key);
use libevdev_event_code_get_name(device->evdev, EV_KEY, key)
> + return;
> + }
> +
> + set_key_state(device, key, state == LIBINPUT_KEY_STATE_PRESSED);
just pass the enum through and let set_key_state handle it, much easier to
read
> +
> + keyboard_notify_key(&device->base, time, key, state);
> +}
> +
> +void
> +evdev_pointer_notify_button(struct evdev_device *device,
> + uint32_t time,
> + int button,
> + enum libinput_button_state state)
> +{
> + struct libinput *libinput = device->base.seat->libinput;
> +
> + if ((state == LIBINPUT_BUTTON_STATE_PRESSED &&
> + get_key_state(device, button) != 0) ||
> + (state == LIBINPUT_BUTTON_STATE_RELEASED &&
> + get_key_state(device, button) != 1)) {
> + log_bug_kernel(
> + libinput,
> + "%s: Driver sent multiple (0%x) pressed/released",
> + device->devnode, button);
> + return;
> + }
> +
> + set_key_state(device, button, state == LIBINPUT_BUTTON_STATE_PRESSED);
> +
> + pointer_notify_button(&device->base, time, button, state);
> +}
> +
> void
> evdev_device_led_update(struct evdev_device *device, enum libinput_led leds)
> {
> @@ -278,6 +361,45 @@ get_key_type(uint16_t code)
> }
>
> static void
> +release_pressed_keys(struct evdev_device *device)
> +{
> + struct libinput *libinput = device->base.seat->libinput;
> + struct timespec ts;
> + uint64_t time;
> + int code;
> +
> + if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) {
> + log_error(libinput, "clock_gettime: %s\n", strerror(errno));
> + return;
We already require CLOCK_MONOTONIC in configure, so the only time this can
fail is if ts is outside the address space which is hard when it's on the
stack. So I think either the error handling is superfluous here or we should
log this as log_bug_libinput().
> + }
> +
> + time = ts.tv_sec * 1000ULL + ts.tv_nsec / 1000000;
maybe as a follow-up: we should really have timeval_to_ms() and
timespec_to_ms() helper functions...
> +
> + for (code = 0; code < KEY_CNT; code++) {
> + if (get_key_state(device, code) == 1) {
> + switch (get_key_type(code)) {
> + case EVDEV_KEY_TYPE_NONE:
> + break;
> + case EVDEV_KEY_TYPE_KEY:
> + evdev_keyboard_notify_key(
> + device,
> + time,
> + code,
> + LIBINPUT_KEY_STATE_RELEASED);
> + break;
> + case EVDEV_KEY_TYPE_BUTTON:
> + evdev_pointer_notify_button(
> + device,
> + time,
> + code,
> + LIBINPUT_BUTTON_STATE_RELEASED);
> + break;
> + }
> + }
> + }
> +}
> +
> +static void
> evdev_process_touch_button(struct evdev_device *device,
> uint64_t time, int value)
> {
> @@ -310,16 +432,16 @@ evdev_process_key(struct evdev_device *device,
> case EVDEV_KEY_TYPE_NONE:
> break;
> case EVDEV_KEY_TYPE_KEY:
> - keyboard_notify_key(
> - &device->base,
> + evdev_keyboard_notify_key(
> + device,
> time,
> e->code,
> e->value ? LIBINPUT_KEY_STATE_PRESSED :
> LIBINPUT_KEY_STATE_RELEASED);
> break;
> case EVDEV_KEY_TYPE_BUTTON:
> - pointer_notify_button(
> - &device->base,
> + evdev_pointer_notify_button(
> + device,
> time,
> e->code,
> e->value ? LIBINPUT_BUTTON_STATE_PRESSED :
> @@ -937,6 +1059,8 @@ evdev_device_remove(struct evdev_device *device)
> libinput_remove_source(device->base.seat->libinput,
> device->source);
>
> + release_pressed_keys(device);
> +
> if (device->mtdev)
> mtdev_close_delete(device->mtdev);
> close_restricted(device->base.seat->libinput, device->fd);
> diff --git a/src/evdev.h b/src/evdev.h
> index fad1f84..9c3f3d8 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -94,6 +94,8 @@ struct evdev_device {
> struct {
> struct motion_filter *filter;
> } pointer;
> +
> + uint64_t key_mask[KEY_CNT / 64];
see the NLONGS comments above.
> };
>
> #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
> @@ -173,6 +175,18 @@ evdev_device_transform_y(struct evdev_device *device,
> uint32_t height);
>
> void
> +evdev_keyboard_notify_key(struct evdev_device *device,
> + uint32_t time,
> + int key,
> + enum libinput_key_state state);
> +
> +void
> +evdev_pointer_notify_button(struct evdev_device *device,
> + uint32_t time,
> + int button,
> + enum libinput_button_state state);
> +
> +void
> evdev_device_remove(struct evdev_device *device);
>
> void
> diff --git a/test/keyboard.c b/test/keyboard.c
> index a55405c..1a21e49 100644
> --- a/test/keyboard.c
> +++ b/test/keyboard.c
> @@ -25,6 +25,7 @@
> #include <check.h>
> #include <stdio.h>
>
> +#include "libinput-util.h"
> #include "litest.h"
>
> START_TEST(keyboard_seat_key_count)
> @@ -112,10 +113,127 @@ START_TEST(keyboard_seat_key_count)
> }
> END_TEST
>
> +static void
> +test_key_event(struct litest_device *dev, unsigned int key, int state)
> +{
> + struct libinput *li = dev->libinput;
> + struct libinput_event *event;
> + struct libinput_event_keyboard *kevent;
> +
> + litest_event(dev, EV_KEY, key, state);
> + litest_event(dev, EV_SYN, SYN_REPORT, 0);
> +
> + libinput_dispatch(li);
> +
> + event = libinput_get_event(li);
> + ck_assert(event != NULL);
> + ck_assert_int_eq(libinput_event_get_type(event), LIBINPUT_EVENT_KEYBOARD_KEY);
> +
> + kevent = libinput_event_get_keyboard_event(event);
> + ck_assert(kevent != NULL);
> + ck_assert_int_eq(libinput_event_keyboard_get_key(kevent), key);
> + ck_assert_int_eq(libinput_event_keyboard_get_key_state(kevent),
> + state ?
> + LIBINPUT_KEY_STATE_PRESSED :
> + LIBINPUT_KEY_STATE_RELEASED);
> + libinput_event_destroy(event);
> +}
> +
> +START_TEST(keyboard_key_auto_release)
> +{
> + struct libinput *libinput;
> + struct litest_device *dev;
> + struct libinput_event *event;
> + struct libinput_event_keyboard *kevent;
> + struct {
> + int code;
> + int released;
> + } keys[] = {
> + { .code = KEY_A, },
> + { .code = KEY_S, },
> + { .code = KEY_D, },
> + { .code = KEY_G, },
> + { .code = KEY_Z, },
> + { .code = KEY_DELETE, },
> + { .code = KEY_F24, },
> + };
hehe. for extra fun you should've snuck KEY_LEFTCTRL, KEY_LEFTALT and
KEY_BACKSPACE in here ;)
> + static int events[2 * (ARRAY_LENGTH(keys) + 1)];
static?
> + unsigned i;
> + int key;
> + int valid_code;
> +
> + /* Enable all tested keys on the device */
> + for (i = 0; i < 2 * ARRAY_LENGTH(keys);) {
> + key = keys[i / 2].code;
> + events[i++] = EV_KEY;
> + events[i++] = key;
> + }
> + events[i++] = -1;
> + events[i++] = -1;
> +
> + libinput = litest_create_context();
> + dev = litest_add_device_with_overrides(libinput,
> + LITEST_KEYBOARD,
> + "Generic keyboard",
> + NULL, NULL, events);
> +
> + litest_drain_events(libinput);
> +
> + /* Send pressed events, without releasing */
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + test_key_event(dev, keys[i].code, 1);
> + }
> +
> + litest_drain_events(libinput);
> +
> + /* "Disconnect" device */
> + litest_delete_device(dev);
> +
> + /* Mark all released keys until device is removed */
> + while (1) {
> + event = libinput_get_event(libinput);
> + ck_assert_notnull(event);
> +
> + if (libinput_event_get_type(event) ==
> + LIBINPUT_EVENT_DEVICE_REMOVED) {
> + libinput_event_destroy(event);
> + break;
> + }
> +
> + ck_assert_int_eq(libinput_event_get_type(event),
> + LIBINPUT_EVENT_KEYBOARD_KEY);
can we use a temporary "type" variable? calling get_type() over two lines of
code once is enough, that 80 limit is a bit annoying given the verbosity of
our API.
> + kevent = libinput_event_get_keyboard_event(event);
> + ck_assert_notnull(kevent);
this line is superfluous, IMO, we have a different test IIRC that checks for
this to work, so we don't need to worry about it here.
> + ck_assert_int_eq(libinput_event_keyboard_get_key_state(kevent),
> + LIBINPUT_KEY_STATE_RELEASED);
> + key = libinput_event_keyboard_get_key(kevent);
> +
> + valid_code = 0;
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + if (keys[i].code == key) {
> + ck_assert_int_eq(keys[i].released, 0);
> + keys[i].released = 1;
> + valid_code = 1;
> + }
> + }
> + ck_assert_int_eq(valid_code, 1);
> + libinput_event_destroy(event);
> + }
> +
> + /* Check that all pressed keys has been released. */
> + for (i = 0; i < ARRAY_LENGTH(keys); ++i) {
> + ck_assert_int_eq(keys[i].released, 1);
> + }
> +
> + libinput_unref(libinput);
> +}
> +END_TEST
> +
> int
> main(int argc, char **argv)
> {
> litest_add_no_device("keyboard:seat key count", keyboard_seat_key_count);
> + litest_add_no_device("keyboard:key_auto_release", keyboard_key_auto_release);
>
> return litest_run(argc, argv);
> }
> diff --git a/test/pointer.c b/test/pointer.c
> index aa75274..57aae8a 100644
> --- a/test/pointer.c
> +++ b/test/pointer.c
> @@ -152,6 +152,96 @@ START_TEST(pointer_button)
> }
> END_TEST
>
> +START_TEST(pointer_button_auto_release)
> +{
> + struct libinput *libinput;
> + struct litest_device *dev;
> + struct libinput_event *event;
> + struct libinput_event_pointer *pevent;
> + struct {
> + int code;
> + int released;
> + } buttons[] = {
> + { .code = BTN_LEFT, },
> + { .code = BTN_MIDDLE, },
> + { .code = BTN_EXTRA, },
> + { .code = BTN_SIDE, },
> + { .code = BTN_BACK, },
> + { .code = BTN_FORWARD, },
> + { .code = BTN_4, },
> + };
> + static int events[2 * (ARRAY_LENGTH(buttons) + 1)];
same comments as above for this function.
> + unsigned i;
> + int button;
> + int valid_code;
> +
> + /* Enable all tested buttons on the device */
> + for (i = 0; i < 2 * ARRAY_LENGTH(buttons);) {
> + button = buttons[i / 2].code;
> + events[i++] = EV_KEY;
> + events[i++] = button;
> + }
> + events[i++] = -1;
> + events[i++] = -1;
> +
> + libinput = litest_create_context();
> + dev = litest_add_device_with_overrides(libinput,
> + LITEST_MOUSE,
> + "Generic mouse",
> + NULL, NULL, events);
> +
> + litest_drain_events(libinput);
> +
> + /* Send pressed events, without releasing */
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + test_button_event(dev, buttons[i].code, 1);
> + }
> +
> + litest_drain_events(libinput);
> +
> + /* "Disconnect" device */
> + litest_delete_device(dev);
> +
> + /* Mark all released buttons until device is removed */
> + while (1) {
> + event = libinput_get_event(libinput);
> + ck_assert_notnull(event);
> +
> + if (libinput_event_get_type(event) ==
> + LIBINPUT_EVENT_DEVICE_REMOVED) {
> + libinput_event_destroy(event);
> + break;
> + }
> +
> + ck_assert_int_eq(libinput_event_get_type(event),
> + LIBINPUT_EVENT_POINTER_BUTTON);
> + pevent = libinput_event_get_pointer_event(event);
> + ck_assert_notnull(pevent);
> + ck_assert_int_eq(libinput_event_pointer_get_button_state(pevent),
> + LIBINPUT_BUTTON_STATE_RELEASED);
> + button = libinput_event_pointer_get_button(pevent);
> +
> + valid_code = 0;
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + if (buttons[i].code == button) {
> + ck_assert_int_eq(buttons[i].released, 0);
> + buttons[i].released = 1;
> + valid_code = 1;
> + }
> + }
> + ck_assert_int_eq(valid_code, 1);
> + libinput_event_destroy(event);
> + }
> +
> + /* Check that all pressed buttons has been released. */
> + for (i = 0; i < ARRAY_LENGTH(buttons); ++i) {
> + ck_assert_int_eq(buttons[i].released, 1);
> + }
> +
> + libinput_unref(libinput);
> +}
> +END_TEST
> +
> static void
> test_wheel_event(struct litest_device *dev, int which, int amount)
> {
> @@ -300,6 +390,7 @@ int main (int argc, char **argv) {
>
> litest_add("pointer:motion", pointer_motion_relative, LITEST_POINTER, LITEST_ANY);
> litest_add("pointer:button", pointer_button, LITEST_BUTTON, LITEST_CLICKPAD);
> + litest_add_no_device("pointer:button_auto_release", pointer_button_auto_release);
> litest_add("pointer:scroll", pointer_scroll_wheel, LITEST_WHEEL, LITEST_ANY);
> litest_add_no_device("pointer:seat button count", pointer_seat_button_count);
>
> --
> 1.8.5.1
>
> _______________________________________________
> 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