On Fri, Oct 27, 2017 at 01:26:40PM -0700, Jason Gerecke wrote:
> On Wed, Oct 25, 2017 at 10:24 PM, Peter Hutterer
> <peter.hutte...@who-t.net> wrote:
> > Some of wacom's tablets, notably the Bamboo series, have a non-predictable
> > scheme of mapping the buttons to numeric button numbers in libwacom. Since
> > we
> > promise sequential button numbers, we need to have those identical to
> > libwacom, otherwise it's impossible to map the two together.
> >
> > Most tablets have a predictable mapping, so this does not affect the
> > majority
> > of devices.
> >
> > For the old-style bamboos, this swaps the buttons around with the buttons
> > being ordered in a vertical order.
>
> This language gives me pause because I know under the covers the order
> ultimately comes down to whatever the libwacom SVGs specify. Still, it
> looks like all the Bamboo SVGs do indeed use a vertical order so you
> can leave it as-is if you'd like.
I guess the alternative here is that we swap the libwacom data files and
SVGs around to have the order follow the evdev code sort order. Either way,
the end-user breakage is roughly the same. These tablets are older enough
that I don't really care either way so I'm happy to take your preference
here. I've appended 'in libwacom' to the sentence in the commit message
to be less ambiguous here.
> > Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> > meson.build | 9 +++
> > src/evdev-tablet-pad.c | 74 +++++++++++++++++++--
> > test/test-pad.c | 171
> > ++++++++++++++++++++++++++++++++++++++++++++-----
> > 3 files changed, 233 insertions(+), 21 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index f93cde70..72bd90b0 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -66,6 +66,15 @@ if have_libwacom
> > name : 'libwacom_get_paired_device check',
> > dependencies : dep_libwacom)
> > config_h.set10('HAVE_LIBWACOM_GET_PAIRED_DEVICE', result)
> > +
> > + code = '''
> > + #include <libwacom/libwacom.h>
> > + int main(void) { libwacom_get_button_evdev_code(NULL, 'A'); }
> > + '''
> > + result = cc.links(code,
> > + name : 'libwacom_get_button_evdev_code check',
> > + dependencies : dep_libwacom)
> > + config_h.set10('HAVE_LIBWACOM_GET_BUTTON_EVDEV_CODE', result)
> > else
> > dep_libwacom = declare_dependency()
> > endif
> > diff --git a/src/evdev-tablet-pad.c b/src/evdev-tablet-pad.c
> > index 82604d70..2e85f7ac 100644
> > --- a/src/evdev-tablet-pad.c
> > +++ b/src/evdev-tablet-pad.c
> > @@ -27,6 +27,10 @@
> > #include <stdbool.h>
> > #include <string.h>
> >
> > +#if HAVE_LIBWACOM
> > +#include <libwacom/libwacom.h>
> > +#endif
> > +
> > #define pad_set_status(pad_,s_) (pad_)->status |= (s_)
> > #define pad_unset_status(pad_,s_) (pad_)->status &= ~(s_)
> > #define pad_has_status(pad_,s_) (!!((pad_)->status & (s_)))
> > @@ -517,17 +521,61 @@ static struct evdev_dispatch_interface pad_interface
> > = {
> > .get_switch_state = NULL,
> > };
> >
> > +static bool
> > +pad_init_buttons_from_libwacom(struct pad_dispatch *pad,
> > + struct evdev_device *device)
> > +{
> > + bool rc = false;
> > +#if HAVE_LIBWACOM_GET_BUTTON_EVDEV_CODE
> > + WacomDeviceDatabase *db = NULL;
> > + WacomDevice *tablet = NULL;
> > + int num_buttons;
> > + int map = 0;
> > +
> > + db = libwacom_database_new();
> > + if (!db) {
> > + evdev_log_info(device,
> > + "Failed to initialize libwacom context.\n");
> > + goto out;
> > + }
> > +
> > + tablet = libwacom_new_from_usbid(db,
> > + evdev_device_get_id_vendor(device),
> > +
> > evdev_device_get_id_product(device),
> > + NULL);
> > + if (!tablet)
> > + goto out;
> > +
> > + num_buttons = libwacom_get_num_buttons(tablet);
> > + for (int i = 0; i < num_buttons; i++) {
> > + unsigned int code;
> > +
> > + code = libwacom_get_button_evdev_code(tablet, 'A' + i);
> > + if (code == 0)
> > + continue;
> > +
> > + pad->button_map[code] = map++;
> > + }
> > +
> > + pad->nbuttons = map;
> > +
> > + rc = true;
> > +out:
> > + if (tablet)
> > + libwacom_destroy(tablet);
> > + if (db)
> > + libwacom_database_destroy(db);
> > +#endif
> > + return rc;
> > +}
> > +
> > static void
> > -pad_init_buttons(struct pad_dispatch *pad,
> > - struct evdev_device *device)
> > +pad_init_buttons_from_kernel(struct pad_dispatch *pad,
> > + struct evdev_device *device)
> > {
> > unsigned int code;
> > - size_t i;
> > int map = 0;
> >
> > - for (i = 0; i < ARRAY_LENGTH(pad->button_map); i++)
> > - pad->button_map[i] = -1;
> > -
> > /* we match wacom_report_numbered_buttons() from the kernel */
> > for (code = BTN_0; code < BTN_0 + 10; code++) {
> > if (libevdev_has_event_code(device->evdev, EV_KEY, code))
> > @@ -553,6 +601,20 @@ pad_init_buttons(struct pad_dispatch *pad,
> > }
> >
> > static void
> > +pad_init_buttons(struct pad_dispatch *pad,
> > + struct evdev_device *device)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < ARRAY_LENGTH(pad->button_map); i++)
> > + pad->button_map[i] = -1;
> > +
> > + if (!pad_init_buttons_from_libwacom(pad, device))
> > + pad_init_buttons_from_kernel(pad, device);
> > +
> > +}
> > +
> > +static void
> > pad_init_left_handed(struct evdev_device *device)
> > {
> > if (evdev_tablet_has_left_handed(device))
> > diff --git a/test/test-pad.c b/test/test-pad.c
> > index 085d6c58..3383c60f 100644
> > --- a/test/test-pad.c
> > +++ b/test/test-pad.c
> > @@ -29,6 +29,10 @@
> > #include <unistd.h>
> > #include <stdbool.h>
> >
> > +#if HAVE_LIBWACOM
> > +#include <libwacom/libwacom.h>
> > +#endif
> > +
> > #include "libinput-util.h"
> > #include "litest.h"
> >
> > @@ -119,6 +123,35 @@ START_TEST(pad_time)
> > }
> > END_TEST
> >
> > +START_TEST(pad_num_buttons_libwacom)
> > +{
> > +#if HAVE_LIBWACOM
> > + struct litest_device *dev = litest_current_device();
> > + struct libinput_device *device = dev->libinput_device;
> > + WacomDeviceDatabase *db = NULL;
> > + WacomDevice *wacom = NULL;
> > + unsigned int nb_lw, nb;
> > +
> > + db = libwacom_database_new();
> > + ck_assert_notnull(db);
> > +
> > + wacom = libwacom_new_from_usbid(db,
> > + libevdev_get_id_vendor(dev->evdev),
> > + libevdev_get_id_product(dev->evdev),
> > + NULL);
> > + ck_assert_notnull(wacom);
> > +
> > + nb_lw = libwacom_get_num_buttons(wacom);
> > + nb = libinput_device_tablet_pad_get_num_buttons(device);
> > +
> > + ck_assert_int_eq(nb, nb_lw);
> > +
> > + libwacom_destroy(wacom);
> > + libwacom_database_destroy(db);
> > +#endif
> > +}
> > +END_TEST
> > +
> > START_TEST(pad_num_buttons)
> > {
> > struct litest_device *dev = litest_current_device();
> > @@ -141,33 +174,42 @@ START_TEST(pad_num_buttons)
> > }
> > END_TEST
> >
> > -START_TEST(pad_button)
> > +START_TEST(pad_button_intuos)
> > {
> > +#if !HAVE_LIBWACOM_GET_BUTTON_EVDEV_CODE
> > struct litest_device *dev = litest_current_device();
> > struct libinput *li = dev->libinput;
> > unsigned int code;
> > unsigned int expected_number = 0;
> > struct libinput_event *ev;
> > struct libinput_event_tablet_pad *pev;
> > + unsigned int count;
> > +
> > + /* Intuos button mapping is sequential up from BTN_0 and continues
> > + * with BTN_A */
> > +
> > + if (!libevdev_has_event_code(dev->evdev, EV_KEY, BTN_0))
> > + return;
> >
> > litest_drain_events(li);
> >
> > - for (code = BTN_0; code < KEY_MAX; code++) {
> > - if (!libevdev_has_event_code(dev->evdev, EV_KEY, code))
> > + for (code = BTN_0; code < BTN_DIGI; code++) {
> > + /* SKip over the BTN_MOUSE and BTN_JOYSTICK range */
> > + if (code >= BTN_MOUSE && code < BTN_A) {
> > + ck_assert(!libevdev_has_event_code(dev->evdev,
> > + EV_KEY, code));
>
> Will this get tripped up by the ExpressKey remote? It uses BTN_BASE
> and BTN_BASE2 for its two highest-numbered buttons (following
> BTN_0..BTN_9, BTN_A..BTN_Z). No other device uses those codes from
> what I can tell, but a hypothetical future tablet with enough buttons
> would.
indeed it does - an excessive return (below) skipped the rest of the test,
thus never triggering the assertion. I'll fix this, thanks.
> > continue;
> > -
> > - litest_button_click(dev, code, 1);
> > - litest_button_click(dev, code, 0);
> > - libinput_dispatch(li);
> > -
> > - switch (code) {
> > - case BTN_STYLUS:
> > - litest_assert_empty_queue(li);
> > - continue;
> > - default:
> > - break;
> > }
> >
> > + if (!libevdev_has_event_code(dev->evdev, EV_KEY, code))
> > + return;
> > +
> > + litest_button_click(dev, code, 1);
> > + litest_button_click(dev, code, 0);
> > + libinput_dispatch(li);
> > +
> > + count++;
> > +
> > ev = libinput_get_event(li);
> > pev = litest_is_pad_button_event(ev,
> > expected_number,
> > @@ -186,6 +228,102 @@ START_TEST(pad_button)
> > }
> >
> > litest_assert_empty_queue(li);
> > +
> > + ck_assert_int_gt(count, 3);
> > +#endif
> > +}
> > +END_TEST
> > +
> > +START_TEST(pad_button_bamboo)
> > +{
> > +#if !HAVE_LIBWACOM_GET_BUTTON_EVDEV_CODE
> > + struct litest_device *dev = litest_current_device();
> > + struct libinput *li = dev->libinput;
> > + unsigned int code;
> > + unsigned int expected_number = 0;
> > + struct libinput_event *ev;
> > + struct libinput_event_tablet_pad *pev;
> > + unsigned int count;
> > +
> > + if (!libevdev_has_event_code(dev->evdev, EV_KEY, BTN_LEFT))
> > + return;
>
> Pre-3.17 kernels combine the pen and pad on the same device. A device
> like the Intuos4 will declare the BTN_0 range for its ExpressKeys, as
> well as the BTN_LEFT range for its puck. Not sure if that'll be
> problematic...
not for the test here, we expect something more recent. The whole libinput
tablet pad support was basically written with post 3.17 as expectation.
Speaking of which, I should set up a test device that has pre-3.17 behaviour
and see what happens :)
Cheers,
Peter
> > +
> > + litest_drain_events(li);
> > +
> > + for (code = BTN_LEFT; code < BTN_JOYSTICK; code++) {
> > + if (!libevdev_has_event_code(dev->evdev, EV_KEY, code))
> > + return;
> > +
> > + litest_button_click(dev, code, 1);
> > + litest_button_click(dev, code, 0);
> > + libinput_dispatch(li);
> > +
> > + count++;
> > +
> > + ev = libinput_get_event(li);
> > + pev = litest_is_pad_button_event(ev,
> > + expected_number,
> > +
> > LIBINPUT_BUTTON_STATE_PRESSED);
> > + ev = libinput_event_tablet_pad_get_base_event(pev);
> > + libinput_event_destroy(ev);
> > +
> > + ev = libinput_get_event(li);
> > + pev = litest_is_pad_button_event(ev,
> > + expected_number,
> > +
> > LIBINPUT_BUTTON_STATE_RELEASED);
> > + ev = libinput_event_tablet_pad_get_base_event(pev);
> > + libinput_event_destroy(ev);
> > +
> > + expected_number++;
> > + }
> > +
> > + litest_assert_empty_queue(li);
> > +
> > + ck_assert_int_gt(count, 3);
> > +#endif
> > +}
> > +END_TEST
> > +
> > +START_TEST(pad_button_libwacom)
> > +{
> > +#if HAVE_LIBWACOM_GET_BUTTON_EVDEV_CODE
> > + struct litest_device *dev = litest_current_device();
> > + struct libinput *li = dev->libinput;
> > + WacomDeviceDatabase *db = NULL;
> > + WacomDevice *wacom = NULL;
> > +
> > + db = libwacom_database_new();
> > + assert(db);
> > +
> > + wacom = libwacom_new_from_usbid(db,
> > + libevdev_get_id_vendor(dev->evdev),
> > + libevdev_get_id_product(dev->evdev),
> > + NULL);
> > + assert(wacom);
> > +
> > + litest_drain_events(li);
> > +
> > + for (int i = 0; i < libwacom_get_num_buttons(wacom); i++) {
> > + unsigned int code;
> > +
> > + code = libwacom_get_button_evdev_code(wacom, 'A' + i);
> > +
> > + litest_button_click(dev, code, 1);
> > + litest_button_click(dev, code, 0);
> > + libinput_dispatch(li);
> > +
> > + litest_assert_pad_button_event(li,
> > + i,
> > +
> > LIBINPUT_BUTTON_STATE_PRESSED);
> > + litest_assert_pad_button_event(li,
> > + i,
> > +
> > LIBINPUT_BUTTON_STATE_RELEASED);
> > + }
> > +
> > +
> > + libwacom_destroy(wacom);
> > + libwacom_database_destroy(db);
> > +#endif
> > }
> > END_TEST
> >
> > @@ -788,7 +926,10 @@ litest_setup_tests_pad(void)
> > litest_add("pad:time", pad_time, LITEST_TABLET_PAD, LITEST_ANY);
> >
> > litest_add("pad:button", pad_num_buttons, LITEST_TABLET_PAD,
> > LITEST_ANY);
> > - litest_add("pad:button", pad_button, LITEST_TABLET_PAD, LITEST_ANY);
> > + litest_add("pad:button", pad_num_buttons_libwacom,
> > LITEST_TABLET_PAD, LITEST_ANY);
> > + litest_add("pad:button", pad_button_intuos, LITEST_TABLET_PAD,
> > LITEST_ANY);
> > + litest_add("pad:button", pad_button_bamboo, LITEST_TABLET_PAD,
> > LITEST_ANY);
> > + litest_add("pad:button", pad_button_libwacom, LITEST_TABLET_PAD,
> > LITEST_ANY);
> > litest_add("pad:button", pad_button_mode_groups, LITEST_TABLET_PAD,
> > LITEST_ANY);
> >
> > litest_add("pad:ring", pad_has_ring, LITEST_RING, LITEST_ANY);
> > --
> > 2.13.6
> >
>
> Those concerns aside, for the set:
>
> Acked-by: Jason Gerecke <jason.gere...@wacom.com>
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
