[PATCH libevdev 2/3] test: devices with abs axes need to be created properly
Peter Hutterer
peter.hutterer at who-t.net
Mon Aug 12 20:25:42 PDT 2013
On Mon, Aug 12, 2013 at 11:24:44AM +0200, Benjamin Tissoires wrote:
> On Fri, Aug 9, 2013 at 8:40 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > Simply enabling the bits is not enough, we need to provide axis
> > information too if we want to enable this properly.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>
> I'm a little bit worried about this patch. It does not looks very consistent:
> sometimes, you use "int code = 0", in other places "int code = ABS_X"
> and in some others "struct input_absinfo abs = { ABS_X, 0, 2, 0, 0,
> 0};".
>
> See my comments below:
>
> > ---
> > test/test-libevdev-has-event.c | 141 ++++++++++++++++++++++++-----------------
> > 1 file changed, 83 insertions(+), 58 deletions(-)
> >
> > diff --git a/test/test-libevdev-has-event.c b/test/test-libevdev-has-event.c
> > index f96bbc3..c1bfd0d 100644
> > --- a/test/test-libevdev-has-event.c
> > +++ b/test/test-libevdev-has-event.c
> > @@ -45,9 +45,16 @@ START_TEST(test_has_ev_bit)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int i, rc;
> > + int code = 0;
>
> Why this variable is required?
>
> >
> > - rc = test_create_device(&uidev, &dev,
> > - *evbit, 0,
> > + if (*evbit == EV_ABS) {
> > + struct input_absinfo abs = { code, 0, 2, 0, 0, 0};
>
> I would directly use: "struct input_absinfo abs = { ABS_X, 0, 2, 0, 0,
> 0};" instead.
fair enough, I'll amend it this way. I had the code variable to be more
consistent with other tests. It isn't needed here, or in the next test so
I'll use ABS_X directly.
>
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 1, &abs,
> > + -1);
> > + } else
> > + rc = test_create_device(&uidev, &dev,
> > + *evbit, code,
> > -1);
> > ck_assert_msg(rc == 0, "%s: Failed to create device with: %s",
> > libevdev_get_event_type_name(*evbit),
> > @@ -79,10 +86,17 @@ START_TEST(test_ev_bit_limits)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc;
> > + int code = 0;
>
> ditto
>
> >
> > - rc = test_create_device(&uidev, &dev,
> > - *evbit, 0,
> > - -1);
> > + if (*evbit == EV_ABS) {
> > + struct input_absinfo abs = { code, 0, 2, 0, 0, 0};
>
> ditto
>
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 1, &abs,
> > + -1);
> > + } else
> > + rc = test_create_device(&uidev, &dev,
> > + *evbit, code,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert_int_eq(libevdev_has_event_type(dev, EV_MAX + 1), 0);
> > @@ -114,9 +128,15 @@ START_TEST(test_event_codes)
> > max = libevdev_get_event_type_max(*evbit);
> >
> > for (code = 1; code < max; code += 10) {
> > - rc = test_create_device(&uidev, &dev,
> > - *evbit, code,
> > - -1);
> > + if (*evbit == EV_ABS) {
> > + struct input_absinfo abs = { code, 0, 2, 0, 0, 0};
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 1, &abs,
> > + -1);
> > + } else
> > + rc = test_create_device(&uidev, &dev,
> > + *evbit, code,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert_msg(libevdev_has_event_type(dev, *evbit), "for event type %d\n", *evbit);
> > @@ -143,6 +163,7 @@ START_TEST(test_event_code_limits)
> > struct libevdev *dev;
> > int rc;
> > int max;
> > + int code = 1;
> >
> > if (*evbit == EV_SYN) {
> > evbit++;
> > @@ -152,8 +173,14 @@ START_TEST(test_event_code_limits)
> > max = libevdev_get_event_type_max(*evbit);
> > ck_assert(max != -1);
> >
> > - rc = test_create_device(&uidev, &dev,
> > - *evbit, 1,
> > + if (*evbit == EV_ABS) {
> > + struct input_absinfo abs = { code, 0, 2, 0, 0, 0};
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 1, &abs,
> > + -1);
> > + } else
> > + rc = test_create_device(&uidev, &dev,
> > + *evbit, code,
> > -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > @@ -213,11 +240,10 @@ START_TEST(test_input_props)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc, i;
> > + struct input_absinfo abs = {0, 0, 2, 0, 0};
>
> I would rather see ABS_X there (instead of the first 0)
heh. this is actually an API difference. to keep the (internal!) uinput test
API a bit simpler, test_create_abs_device uses the abs value as code (this
way we can supply a sparse abs array).
uinput_device_set_abs() doesn't, it has a separate parameter for the axis
code and actually copies the value into the internal absinfo struct. so
while ABS_X happens to be 0 this would read as wrong api use.
> > uidev = uinput_device_new(TEST_DEVICE_NAME);
> > - rc = uinput_device_set_event_bits(uidev,
> > - EV_ABS, ABS_X,
> > - -1);
> > + rc = uinput_device_set_abs_bit(uidev, ABS_X, &abs);
> > ck_assert_int_eq(rc, 0);
> > uinput_device_set_prop(uidev, INPUT_PROP_DIRECT);
> > uinput_device_set_prop(uidev, INPUT_PROP_BUTTONPAD);
> > @@ -248,6 +274,7 @@ START_TEST(test_set_input_props)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc, fd;
> > + struct input_absinfo abs = {0, 0, 2, 0, 0};
>
> ditto
>
> >
> > dev = libevdev_new();
> > ck_assert_int_eq(libevdev_enable_property(dev, INPUT_PROP_MAX + 1), -1);
> > @@ -257,9 +284,7 @@ START_TEST(test_set_input_props)
> > ck_assert_int_eq(libevdev_has_property(dev, INPUT_PROP_BUTTONPAD), 1);
> >
> > uidev = uinput_device_new(TEST_DEVICE_NAME);
> > - rc = uinput_device_set_event_bits(uidev,
> > - EV_ABS, ABS_X,
> > - -1);
> > + rc = uinput_device_set_abs_bit(uidev,ABS_X, &abs);
>
> nitpick: missing space before ABS_X :)
amended.
> > ck_assert_int_eq(rc, 0);
> > uinput_device_set_prop(uidev, INPUT_PROP_BUTTONPAD);
> > rc = uinput_device_create(uidev);
> > @@ -288,14 +313,7 @@ START_TEST(test_slot_init_value)
> > int fd;
> >
> > uidev = uinput_device_new(TEST_DEVICE_NAME);
> > - rc = uinput_device_set_event_bits(uidev,
> > - EV_ABS, ABS_X,
> > - EV_ABS, ABS_Y,
> > - EV_ABS, ABS_MT_SLOT,
> > - EV_ABS, ABS_MT_TRACKING_ID,
> > - EV_ABS, ABS_MT_POSITION_X,
> > - EV_ABS, ABS_MT_POSITION_Y,
> > - -1);
> > + rc = uinput_device_set_event_bits(uidev, -1);
>
> Hmm... how can you test against ABS_X, ABS_Y, ABS_MT_POSITION_*,
> etc... if you disable all the event bits?
>
> ok, nevermind, I re-read the test and it is ok.
the uinput_device_set_event_bits() call is now a noop anyway, so I've
removed it.
> Maybe you should also re-write the assignments of abs with a simple
> declaration like you do later:
>
> struct input_absinfo abs[] = { { ABS_X, 0, 1000 },
> { ABS_Y, 0, 1000 },
> { ABS_MT_POSITION_X, 0, 1000 },
> { ABS_MT_POSITION_Y, 0, 1000 },
> { ABS_MT_TRACKING_ID, -1, 2 },
> { ABS_MT_SLOT, 0, 1 }};
amended.
> > ck_assert_int_eq(rc, 0);
> >
> > memset(abs, 0, sizeof(abs));
> > @@ -360,12 +378,17 @@ START_TEST(test_no_slots)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc;
> > - rc = test_create_device(&uidev, &dev,
> > - EV_ABS, ABS_X,
> > - EV_ABS, ABS_Y,
> > - EV_ABS, ABS_MT_POSITION_X,
> > - EV_ABS, ABS_MT_POSITION_Y,
> > - -1);
> > + struct input_absinfo abs[] = { { ABS_X, 0, 2 },
> > + { ABS_Y, 0, 2 },
> > + { ABS_MT_POSITION_X, 0, 2 },
> > + { ABS_MT_POSITION_Y, 0, 2 }};
> > +
> > + rc = test_create_abs_device(&uidev, &dev, 4, abs,
> > + EV_ABS, ABS_X,
> > + EV_ABS, ABS_Y,
> > + EV_ABS, ABS_MT_POSITION_X,
> > + EV_ABS, ABS_MT_POSITION_Y,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert_int_eq(libevdev_get_num_slots(dev), -1);
> > @@ -381,17 +404,23 @@ START_TEST(test_slot_number)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc;
> > + const int nslots = 4;
> > + struct input_absinfo abs[] = { { ABS_X, 0, 2 },
> > + { ABS_Y, 0, 2 },
> > + { ABS_MT_POSITION_X, 0, 2 },
> > + { ABS_MT_POSITION_Y, 0, 2 },
> > + { ABS_MT_SLOT, 0, nslots - 1 }};
> >
> > - rc = test_create_device(&uidev, &dev,
> > - EV_ABS, ABS_X,
> > - EV_ABS, ABS_Y,
> > - EV_ABS, ABS_MT_POSITION_X,
> > - EV_ABS, ABS_MT_POSITION_Y,
> > - EV_ABS, ABS_MT_SLOT,
> > - -1);
> > + rc = test_create_abs_device(&uidev, &dev, 5, abs,
> > + EV_ABS, ABS_X,
> > + EV_ABS, ABS_Y,
> > + EV_ABS, ABS_MT_POSITION_X,
> > + EV_ABS, ABS_MT_POSITION_Y,
> > + EV_ABS, ABS_MT_SLOT,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to uinput device: %s", strerror(-rc));
> >
> > - ck_assert_int_eq(libevdev_get_num_slots(dev), 1);
> > + ck_assert_int_eq(libevdev_get_num_slots(dev), nslots);
> > ck_assert_int_eq(libevdev_get_current_slot(dev), 0);
> >
> > uinput_device_free(uidev);
> > @@ -694,12 +723,12 @@ START_TEST(test_device_enable_bit)
> > {
> > struct uinput_device* uidev;
> > struct libevdev *dev, *dev2;
> > - struct input_absinfo abs;
> > + struct input_absinfo abs = {ABS_X, 0, 2};
> > int rc;
> >
> > - rc = test_create_device(&uidev, &dev,
> > - EV_ABS, ABS_X,
> > - -1);
> > + rc = test_create_abs_device(&uidev, &dev, 1, &abs,
> > + EV_ABS, ABS_X,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert(!libevdev_has_event_code(dev, EV_ABS, ABS_Y));
> > @@ -740,12 +769,12 @@ START_TEST(test_device_enable_bit_invalid)
> > {
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > - struct input_absinfo abs = {0};
> > + struct input_absinfo abs = {ABS_X, 0, 1};
> > int rc;
> >
> > - rc = test_create_device(&uidev, &dev,
> > - EV_ABS, ABS_X,
> > - -1);
> > + rc = test_create_abs_device(&uidev, &dev, 1, &abs,
> > + EV_ABS, ABS_X,
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert_int_eq(libevdev_enable_event_code(dev, EV_ABS, ABS_MAX + 1, &abs), -1);
> > @@ -766,13 +795,13 @@ START_TEST(test_device_disable_bit)
> > struct uinput_device* uidev;
> > struct libevdev *dev, *dev2;
> > int rc;
> > + struct input_absinfo abs[2] = {{ABS_X, 0, 1}, {ABS_Y, 0, 1}};
> >
> > - rc = test_create_device(&uidev, &dev,
> > - EV_ABS, ABS_X,
> > - EV_ABS, ABS_Y,
> > - EV_REL, REL_X,
> > - EV_REL, REL_Y,
> > - -1);
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 2, abs,
> > + EV_REL, REL_X,
> > + EV_REL, REL_Y,
>
> Seems like you are missing the EV_ABS bits here... :(
nope, the other way round. it's an abs device that's being created so the
absbits are enabled as they're found in the abs parameter.
I've removed this from the other test_create_abs_device() calls.
Cheers,
Peter
>
> > + -1);
> > ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> >
> > ck_assert(libevdev_has_event_code(dev, EV_ABS, ABS_X));
> > @@ -814,14 +843,10 @@ START_TEST(test_device_disable_bit_invalid)
> > struct uinput_device* uidev;
> > struct libevdev *dev;
> > int rc;
> > + struct input_absinfo abs = {ABS_X, 0, 1};
> >
> > - rc = uinput_device_new_with_events(&uidev, TEST_DEVICE_NAME, DEFAULT_IDS,
> > - EV_ABS, ABS_X,
> > - -1);
> > + rc = test_create_abs_device(&uidev, &dev, 1, &abs, -1);
> > ck_assert_msg(rc == 0, "Failed to create uinput device: %s", strerror(-rc));
> > - rc = libevdev_new_from_fd(uinput_device_get_fd(uidev), &dev);
> > - ck_assert_msg(rc == 0, "Failed to init device: %s", strerror(-rc));
> > -
> >
> > ck_assert_int_eq(libevdev_disable_event_code(dev, EV_ABS, ABS_MAX + 1), -1);
> > ck_assert_int_eq(libevdev_disable_event_code(dev, EV_MAX + 1, ABS_MAX + 1), -1);
> > --
> > 1.8.2.1
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/input-tools
>
> Cheers,
> Benjamin
More information about the Input-tools
mailing list