On Tue, Jun 28, 2016 at 02:22:16PM -0700, Jason Gerecke wrote:
> On 06/23/2016 05:21 PM, Peter Hutterer wrote:
> > On Wed, Jun 22, 2016 at 09:20:36AM -0700, Jason Gerecke wrote:
> >> On 06/21/2016 06:47 PM, Peter Hutterer wrote:
> >>> On Tue, Jun 21, 2016 at 03:52:53PM -0700, Jason Gerecke wrote:
> >>>> Providing the name of a tablet to show-svg-image can be difficult at
> >>>> times, since the name may not be exactly what you expect or there may
> >>>> be more than one device which shares the same name. To make its use a
> >>>> little easier, allow it to take the device ID of the device to display
> >>>> as a command line argument.
> >>>>
> >>>> Signed-off-by: Jason Gerecke <jason.gere...@wacom.com>
> >>>> ---
> >>>> tools/show-svg-image.c | 74
> >>>> ++++++++++++++++++++++++++++++++++++++++++++------
> >>>> 1 file changed, 66 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/tools/show-svg-image.c b/tools/show-svg-image.c
> >>>> index 674d27e..152f71f 100644
> >>>> --- a/tools/show-svg-image.c
> >>>> +++ b/tools/show-svg-image.c
> >>>> @@ -355,16 +355,20 @@ main (int argc, char **argv)
> >>>> GdkWindow *gdk_win;
> >>>> GdkColor black;
> >>>> char *tabletname;
> >>>> + char *tabletid;
> >>>> const char *filename;
> >>>> GOptionEntry
> >>>> options[] = {
> >>>> - {"tablet", 't', 0, G_OPTION_ARG_STRING,
> >>>> &tabletname, "Name of the tablet to show", "<string>"},
> >>>> + {"tablet", 't', 0, G_OPTION_ARG_STRING,
> >>>> &tabletname, "Name of the tablet to show", "[string]"},
> >>>> + {"id", 'i', 0, G_OPTION_ARG_STRING, &tabletid,
> >>>> "ID of the tablet to show", "[string]"},
> >>>> {NULL}
> >>>> };
> >>>>
> >>>> handle = NULL;
> >>>> error = NULL;
> >>>> + device = NULL;
> >>>> tabletname = NULL;
> >>>> + tabletid = NULL;
> >>>>
> >>>> context = g_option_context_new ("- libwacom tablet viewer");
> >>>> g_option_context_add_main_entries (context, options, NULL);
> >>>> @@ -374,8 +378,12 @@ main (int argc, char **argv)
> >>>> g_option_context_free (context);
> >>>>
> >>>> gtk_init (&argc, &argv);
> >>>> - if (tabletname == NULL) {
> >>>> - g_warning ("No tablet name provided, exiting");
> >>>> + if (tabletname == NULL && tabletid == NULL) {
> >>>> + g_warning ("Tablet name or ID must be provided,
> >>>> exiting");
> >>>> + return 1;
> >>>> + }
> >>>> + if (tabletname != NULL && tabletid != NULL) {
> >>>> + g_warning ("Tablet name or ID must be provided,
> >>>> exiting");
> >>>> return 1;
> >>>> }
> >>>> db = libwacom_database_new_for_path(TOPSRCDIR"/data");
> >>>> @@ -383,15 +391,65 @@ main (int argc, char **argv)
> >>>> g_warning ("Failed to load libwacom database, exiting");
> >>>> return 1;
> >>>> }
> >>>> - device = libwacom_new_from_name(db, tabletname, NULL);
> >>>> - if (!device) {
> >>>> - g_warning ("Device '%s' not found in libwacom database,
> >>>> exiting", tabletname);
> >>>> - return 1;
> >>>> + if (tabletname != NULL) {
> >>>> + device = libwacom_new_from_name (db, tabletname, NULL);
> >>>> + if (!device) {
> >>>> + g_warning ("Device '%s' not found in libwacom
> >>>> database, exiting", tabletname);
> >>>> + return 1;
> >>>> + }
> >>>> + }
> >>>> + else if (tabletid != NULL) {
> >>>> + WacomDevice **devices;
> >>>> + WacomDevice **d;
> >>>> + WacomBusType bustype;
> >>>> + char **tabletid_token;
> >>>> + int vid;
> >>>> + int pid;
> >>>> +
> >>>> + tabletid_token = g_strsplit (tabletid, ":", -1);
> >>>
> >>> sscanf(tabletid, "%s:%x:%x") should work here too? seems easier than
> >>> tokenising
> >>>
> >>
> >> Changed locally.
> >>
> >>>> + if (g_strv_length (tabletid_token) != 3)
> >>>> + g_warning ("Incorrectly formatted tablet id,
> >>>> exiting.");
> >>>> +
> >>>> + if (g_ascii_strcasecmp ("", tabletid_token[0]) == 0)
> >>>> + bustype = WBUSTYPE_UNKNOWN;
> >>>> + else if (g_ascii_strcasecmp ("usb", tabletid_token[0])
> >>>> == 0)
> >>>> + bustype = WBUSTYPE_USB;
> >>>> + else if (g_ascii_strcasecmp ("serial",
> >>>> tabletid_token[0]) == 0)
> >>>> + bustype = WBUSTYPE_SERIAL;
> >>>> + else if (g_ascii_strcasecmp ("bluetooth",
> >>>> tabletid_token[0]) == 0)
> >>>> + bustype = WBUSTYPE_BLUETOOTH;
> >>>> + else if (g_ascii_strcasecmp ("i2c", tabletid_token[0])
> >>>> == 0)
> >>>> + bustype = WBUSTYPE_I2C;
> >>>> + else {
> >>>> + g_warning ("Unknown bus type, exiting.");
> >>>> + return 1;
> >>>> + }
> >>>> +
> >>>> + vid = (int)g_ascii_strtoll (tabletid_token[1], NULL,
> >>>> 16);
> >>>> + pid = (int)g_ascii_strtoll (tabletid_token[2], NULL,
> >>>> 16);
> >>>> +
> >>>> + g_strfreev (tabletid_token);
> >>>> +
> >>>> + devices = libwacom_list_devices_from_database (db,
> >>>> NULL);
> >>>> + d = devices;
> >>>> + while (*d && !device) {
> >>>> + if (libwacom_get_vendor_id (*d) == vid &&
> >>>> + libwacom_get_product_id (*d) == pid &&
> >>>> + (libwacom_get_bustype (*d) == bustype ||
> >>>> bustype == WBUSTYPE_UNKNOWN))
> >>>> + device = *d;
> >>>> + d++;
> >>>> + }
> >>>> + free (devices);
> >>>
> >>> tbh, that looks like something we should have a libwacom_new_from_id()
> >>> for.
> >>> we have from_usbid which has the obvious drawback but either way that
> >>> seems
> >>> like something useful to have around.
> >>>
> >>
> >> I was a bit hesitant to update the API since a thin wrapper around
> >> 'libwacom_new' (which already takes vid/pid/bus arguments) seemed so
> >> obvious that I suspected there may be a reason it didn't exist. I could
> >> certainly either introduce one or corresponding
> >> serialid/i2cid/bluetoothid functions.
> >
> > I *think* the reason it doesn't exist is because we had the new_by_usbid and
> > then the bluetooth/i2c tablets came later and we never needed the matching
> > call. I'd say rather than adding more bus-specific ones we should deprecate
> > the usbid one and add one that takes the bus ID as argument.
> >
> > Cheers,
> > Peter
> >
>
> I finished writing the patches, but...
>
> <musing aloud>
>
> It occurred to me that even if a device isn't actually connected over
> USB, its still a USB ID that we're referring to. Serial and I2C devices
> natively use the PNP/ACPI ID namespace but the database currently refers
> to the USB ID that is extracted from the device (e.g. the i2c sensor in
> the Lenovo ThinkPad Tablet 10 has an ACPI hardware ID of WCOM0008 but
> its HID descriptor has the ID 056a:0114). Bluetooth also uses its own ID
> namespace, with us again getting a USB ID from the descriptor.
>
> Given that, is there any reason 'libwacom_new_from_usbid' should
> necessarily be limited to devices on the USB bus?
>
> I can still imagine that a caller may want to limit the device match to
> a specific bus, but in a world of multiple ID namespaces,
> 'libwacom_new_from_id' might not be the best name choice in hindsight.
hmm, good point. but two things here:
we have a couple of devices listed with the same vid/pid but
different bustypes (ISDv4 90, 93 and e3). That should be fine though, a
problem would be *different* devices with the same vid/pid.
is there any current use-case where the bus filter is
required for this call? it doesn't seem to be used outside the libwacom
tests. CC-ing Bastien, it was introduced in ad122e70b9c445d, maybe he
remembers. given the above, removing that filter should be fine.
Cheers,
Peter
>
> </musing aloud>
>
> 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....
>
> >> Let me know which you would prefer and I'll send some patches.
> >>
> >>> and the last nit: can we please split this block into a helper function.
> >>> main() is already quite unwieldly
> >>>
> >>
> >> Changed locally.
> >>
> >>> I've pushed the other patches, thanks.
> >>>
> >>> Cheers,
> >>> Peter
> >>>
> >>
> >> Thanks.
> >>
> >> 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....
> >>
> >>>> +
> >>>> + if (!device) {
> >>>> + g_warning ("Device ID '%s' not found in
> >>>> libwacom database, exiting", tabletid);
> >>>> + return 1;
> >>>> + }
> >>>> }
> >>>>
> >>>> filename = libwacom_get_layout_filename(device);
> >>>> if (filename == NULL) {
> >>>> - g_warning ("Device '%s' has no layout available,
> >>>> exiting", tabletname);
> >>>> + g_warning ("Device '%s' has no layout available,
> >>>> exiting", libwacom_get_name(device));
> >>>> return 1;
> >>>> }
> >>>> handle = rsvg_handle_new_from_file (filename, &error);
> >>>> --
> >>>> 2.8.3
> >>>>
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
