[PATCH libevdev 2/2] Discard incomplete sequences in case of SYN_DROPPED
David Herrmann
dh.herrmann at gmail.com
Tue Feb 25 02:01:33 PST 2014
Hi
On Tue, Feb 25, 2014 at 5:26 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> A event sequence emitted by the kernel in case of SYN_DROPPED may be:
> ABS_X
> SYN_REPORT
This SYN_REPORT should be reported right away with the ABS_X value. I
don't know why you drop it and instead report both _X and _Y together
as a sync?
> ABS_Y
> SYN_DROPPED
> ABS_Y
> SYN_REPORT
>
> Both ABS_Y events are parts of an incomplete sequence and must be
> dropped, so the event sequence we return to the caller is.
> ABS_X
> ABS_Y
> SYN_REPORT
> SYN_DROPPED
What do you mean by reporting SYN_DROPPED? Our re-sync phase? In this
case, why do you report ABS_Y at all? It's part of an invalid report
that we should not return to the caller with any cached _X values. For
ABS this is mood, but for boolean values that only make sense in
pairs, you might report wrong information here.
If the kernel sends:
ABS_X
SYN_REPORT
ABS_Y
SYN_DROPPED
ABS_Y
SYN_REPORT
My interpretation is for libevdev to return:
ABS_X
SYN_REPORT
SYN_DROPPED
The ABS_X is returned as it's a complete report. The ABS_Y reports are
all dropped as they were not completed. Therefore, the queue might
have been flushed and it should be resynced at SYN_DROPPED receival.
Thanks
David
>
> This is a lot easier for callers to handle.
>
> It requires us to keep track of incomplete event sequences, specifically
> whether there is an EV_SYN event in the pipe. The kernel usually doesn't send
> incomplete sequences but if our internal buffer is almost full, we may not
> read off all the events of a sequence.
>
> If there is no EV_SYN terminating the current sequence, we must pretend there
> are no events waiting since the events we already do have may get discarded in
> case of SYN_DROPPED.
>
> Note that this patch only handles dropping events _before_ SYN_DROPPED, events
> after SYN_DROPPED are already handled by sync_state (see the comment at the
> top of the function).
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> libevdev/libevdev.c | 73 ++++++++++++++++++++++++++++++++++++++++++---
> libevdev/libevdev.h | 6 ++++
> test/test-libevdev-events.c | 64 +++++++++++++++++++++++++++++++++++----
> 3 files changed, 134 insertions(+), 9 deletions(-)
>
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index ead0205..04913fa 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -36,6 +36,12 @@
>
> #define MAXEVENTS 64
>
> +enum read_result {
> + READ_RESULT_NO_EVENTS = 1,
> + READ_RESULT_EVENTS,
> + READ_RESULT_NO_EV_SYN,
> +};
> +
> static int sync_mt_state(struct libevdev *dev, int create_events);
>
> static int
> @@ -750,6 +756,52 @@ update_state(struct libevdev *dev, const struct input_event *e)
> }
>
> static int
> +check_for_syn_dropped(struct libevdev *dev)
> +{
> + size_t i;
> + int last_syn_report = -1,
> + syn_dropped = -1;
> +
> + if (queue_num_elements(dev) == 0)
> + return READ_RESULT_NO_EVENTS;
> +
> + /* SYN_DROPPED may come at any time, any event since the last EV_SYN
> + and up to the next EV_SYN must be dropped. see
> + https://www.kernel.org/doc/Documentation/input/event-codes.txt
> + */
> +
> + for (i = 0; i < queue_num_elements(dev); i++) {
> + struct input_event ev;
> +
> + queue_peek(dev, i, &ev);
> + if (ev.type == EV_SYN) {
> + if (ev.code == SYN_REPORT)
> + last_syn_report = i;
> + else if (ev.code == SYN_DROPPED) {
> + syn_dropped = i;
> + break;
> + }
> + }
> + }
> +
> + if (syn_dropped == -1) {
> + /* We've read an incomplete event frame due to our own buffer
> + constraints, let's pretend there are no events ATM. The
> + next read will give us the SYN_REPORT of this frame */
> + if (last_syn_report == -1)
> + return READ_RESULT_NO_EV_SYN;
> +
> + return READ_RESULT_EVENTS;
> + }
> +
> + /* slice events out, but leave SYN_DROPPED in place */
> + if (syn_dropped > 0)
> + queue_slice(dev, last_syn_report + 1, syn_dropped - last_syn_report - 1);
> +
> + return READ_RESULT_EVENTS;
> +}
> +
> +static int
> read_more_events(struct libevdev *dev)
> {
> int free_elem;
> @@ -758,7 +810,7 @@ read_more_events(struct libevdev *dev)
>
> free_elem = queue_num_free_elements(dev);
> if (free_elem <= 0)
> - return 0;
> + return READ_RESULT_NO_EVENTS;
>
> next = queue_next_element(dev);
> len = read(dev->fd, next, free_elem * sizeof(struct input_event));
> @@ -769,9 +821,11 @@ read_more_events(struct libevdev *dev)
> else if (len > 0) {
> int nev = len/sizeof(struct input_event);
> queue_set_num_elements(dev, queue_num_elements(dev) + nev);
> +
> + return check_for_syn_dropped(dev);
> }
>
> - return 0;
> + return READ_RESULT_NO_EVENTS;
> }
>
> static int
> @@ -856,6 +910,10 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> goto out;
> }
>
> + if (rc == READ_RESULT_NO_EV_SYN) {
> + rc = -EAGAIN;
> + goto out;
> + }
>
> if (queue_shift(dev, ev) != 0)
> return -EAGAIN;
> @@ -891,8 +949,15 @@ libevdev_has_event_pending(struct libevdev *dev)
> } else if (dev->fd < 0)
> return -EBADF;
>
> - if (queue_num_elements(dev) != 0)
> - return 1;
> + if (queue_num_elements(dev) != 0) {
> + size_t i;
> + for (i = 0; i < queue_num_elements(dev); i++) {
> + struct input_event ev;
> + queue_peek(dev, i, &ev);
> + if (ev.type == EV_SYN)
> + return 1;
> + }
> + }
>
> rc = poll(&fds, 1, 0);
> return (rc >= 0) ? rc : -errno;
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index 898e919..ad409dd 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -699,6 +699,12 @@ enum libevdev_read_status {
> * dropped after libevdev updates its internal state and event processing
> * continues as normal.
> *
> + * libevdev transparently drops events that are part of an incomplete event
> + * sequence caused by a SYN_DROPPED event. The caller is guaranteed that the
> + * last event before a SYN_DROPPED event is an EV_SYN, and the next event
> + * after a SYN_DROPPED is the first event of the next valid event sequence
> + * (or another SYN_DROPPED) event.
> + *
> * @param dev The evdev device, already initialized with libevdev_set_fd()
> * @param flags Set of flags to determine behaviour. If @ref LIBEVDEV_READ_FLAG_NORMAL
> * is set, the next event is read in normal mode. If @ref LIBEVDEV_READ_FLAG_SYNC is
> diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> index 666b98d..1b68469 100644
> --- a/test/test-libevdev-events.c
> +++ b/test/test-libevdev-events.c
> @@ -66,6 +66,14 @@ START_TEST(test_syn_dropped_event)
> struct libevdev *dev;
> int rc;
> struct input_event ev;
> + struct input_event syn_dropped_sequence[] = {
> + { .type = EV_REL, .code = REL_X, .value = 1 },
> + { .type = EV_REL, .code = REL_Y, .value = 2 },
> + { .type = EV_SYN, .code = SYN_DROPPED, .value = 0 },
> + { .type = EV_REL, .code = REL_X, .value = 3 },
> + { .type = EV_REL, .code = REL_Y, .value = 4 },
> + { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
> + };
> int pipefd[2];
>
> rc = test_create_device(&uidev, &dev,
> @@ -95,11 +103,9 @@ START_TEST(test_syn_dropped_event)
> ck_assert_int_eq(rc, 0);
>
> libevdev_change_fd(dev, pipefd[0]);
> - ev.type = EV_SYN;
> - ev.code = SYN_DROPPED;
> - ev.value = 0;
> - rc = write(pipefd[1], &ev, sizeof(ev));
> - ck_assert_int_eq(rc, sizeof(ev));
> + rc = write(pipefd[1], syn_dropped_sequence, sizeof(syn_dropped_sequence));
> + ck_assert_int_eq(rc, sizeof(syn_dropped_sequence));
> +
> rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
>
> libevdev_change_fd(dev, uinput_device_get_fd(uidev));
> @@ -112,6 +118,11 @@ START_TEST(test_syn_dropped_event)
> ck_assert_int_eq(ev.type, EV_SYN);
> ck_assert_int_eq(ev.code, SYN_DROPPED);
>
> + /* no delta, so expect EAGAIN since the REL_X/REL_Y we wrote after
> + * SYN_DROPPED are supposed to be dropped */
> + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_SYNC, &ev);
> + ck_assert_int_eq(rc, -EAGAIN);
> +
> /* only check for the rc, nothing actually changed on the device */
>
> libevdev_free(dev);
> @@ -345,6 +356,48 @@ START_TEST(test_has_event_pending)
>
> }
> END_TEST
> +
> +START_TEST(test_next_event_incomplete)
> +{
> + struct uinput_device* uidev;
> + struct libevdev *dev;
> + int rc;
> + struct input_event ev;
> + int pipefd[2];
> + struct input_event incomplete_sequence[] = {
> + { .type = EV_REL, .code = REL_X, .value = 1 },
> + { .type = EV_REL, .code = REL_Y, .value = 2 },
> + };
> +
> + rc = test_create_device(&uidev, &dev,
> + EV_REL, REL_X,
> + EV_REL, REL_Y,
> + EV_KEY, BTN_LEFT,
> + -1);
> + ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> +
> + /* we can't write incomplete events through uinput, so let's use a
> + * pipe instead */
> + rc = pipe2(pipefd, O_NONBLOCK);
> + ck_assert_int_eq(rc, 0);
> +
> + libevdev_change_fd(dev, pipefd[0]);
> +
> + rc = write(pipefd[1], incomplete_sequence, sizeof(incomplete_sequence));
> + ck_assert_int_eq(rc, sizeof(incomplete_sequence));
> +
> + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> + ck_assert_int_eq(rc, -EAGAIN);
> +
> + ck_assert_int_eq(libevdev_has_event_pending(dev), 0);
> +
> + libevdev_free(dev);
> + uinput_device_free(uidev);
> + close(pipefd[0]);
> + close(pipefd[1]);
> +}
> +END_TEST
> +
> START_TEST(test_syn_delta_button)
> {
> struct uinput_device* uidev;
> @@ -1317,6 +1370,7 @@ libevdev_events(void)
> tcase_add_test(tc, test_event_type_filtered);
> tcase_add_test(tc, test_event_code_filtered);
> tcase_add_test(tc, test_has_event_pending);
> + tcase_add_test(tc, test_next_event_incomplete);
> suite_add_tcase(s, tc);
>
> tc = tcase_create("SYN_DROPPED deltas");
> --
> 1.8.4.2
>
> _______________________________________________
> Input-tools mailing list
> Input-tools at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list