GNOME Bugzilla – Bug 704167
Led indicator handling over bluetooth
Last modified: 2014-02-07 11:53:56 UTC
Created attachment 249072 [details] [review] Led indicator handling over bluetooth v1 Add status led handling over bluetooth without breaking the same functionality over USB. I have some problems with full testing in jhbuild environment, but the helper works fine from command line.
Review of attachment 249072 [details] [review]: ::: plugins/wacom/gsd-wacom-led-helper.c @@ +49,2 @@ /* convert to text */ + if (brightness == -1) /* brightness == -1 indicates USB */ I don't understand this. What's the difference in handling USB vs. Bluetooth for this device? @@ +130,3 @@ + element = g_list_next(element); + } + g_list_free(hid_list); You will leak all the elements after the matching one. Don't unref in the loop, and use g_list_free_full() here instead. @@ +197,3 @@ + filename = get_bt_led_filename (client, device); + brightness = 127; /* maximum brightness accepted by wacom Intuos4 */ + } else { Should you check whether it's bluetooth here, for sanity checking? @@ +211,3 @@ + /* brightness is ignored for USB, but it's required for bluetooth + * led_num is ignored by bluetooth, but required for USB due to + * differences in kernel API. Wouldn't it be better to adapt the Bluetooth API to match the USB one? This is a problem that kernels are supposed to fix for user-space (providing a uniform access to hardware).
(In reply to comment #1) > Review of attachment 249072 [details] [review]: > > ::: plugins/wacom/gsd-wacom-led-helper.c > @@ +49,2 @@ > /* convert to text */ > + if (brightness == -1) /* brightness == -1 indicates USB */ > > I don't understand this. What's the difference in handling USB vs. Bluetooth > for this device? usb: one file for brightness (value: 0-127) and one file to set active led (value: 0-3). Someone decided to make is wacom specific leds, so they don't show in sysfs in "led class". bt: 4 files for brightness (value: 0-127), and no file for setting the active led. Setting brightness of any led automaticaly makes it the active one. The leds are accessible through /sys/class/leds/ as they are "led class" devices. > @@ +130,3 @@ > + element = g_list_next(element); > + } > + g_list_free(hid_list); > > You will leak all the elements after the matching one. Don't unref in the loop, > and use g_list_free_full() here instead. OK, thanks! > @@ +197,3 @@ > + filename = get_bt_led_filename (client, device); > + brightness = 127; /* maximum brightness accepted by wacom Intuos4 */ > + } else { > > Should you check whether it's bluetooth here, for sanity checking? I think it will be hid - I'll make the check. > @@ +211,3 @@ > + /* brightness is ignored for USB, but it's required for bluetooth > + * led_num is ignored by bluetooth, but required for USB due to > + * differences in kernel API. > > Wouldn't it be better to adapt the Bluetooth API to match the USB one? This is > a problem that kernels are supposed to fix for user-space (providing a uniform > access to hardware). I don't think changing user-space interface in kernel is an option: https://lkml.org/lkml/2012/12/23/75 I had decided to use "led class" as defined here: https://www.kernel.org/doc/Documentation/leds/leds-class.txt ,so I'd rather say that the usb driver should match the bluetooth driver. The biggest problem with implementing that in kernel is that I have only one model of tablet and those changes (to use led class in usb driver) would potentially break led support on whole range on wacom tablets. I could try to extend the kernel API by adding led class for usb driver, but I don't think the usb maintainer would like to see 4 leds handled in 2 different ways just because it would look nice for user-space :-) That's about OLED displays: The access to the hardware won't be uniform on intuos4 WL anyway because the tablet accepts 4-bit per pixel over usb and only 1-bit per pixel over bluetooth - patch for OLEDs over bluetooth is in progress. P.S. I have an idea how to code it in a cleaner way...
Created attachment 249322 [details] [review] Led indicator handling over bluetooth v2 Slightly changed flow of the code (probably easier to read + less duplicated code) and led brightness for bluetooth connection is now openly defined.
Review of attachment 249322 [details] [review]: ::: plugins/wacom/gsd-wacom-led-helper.c @@ +35,2 @@ static gboolean +gsd_wacom_led_helper_write (const gchar *filename, gint value, gboolean usb, GError **error) If this function is just a helper, throwing a int into a sysfs file, I'd rather not have any wacom specifics in there, especially not per-connection type changes. @@ +101,3 @@ + char *filename; + + status = g_strdup_printf ("/leds/%s:selector:%i/brightness", g_udev_device_get_name (device), led_num); You're using led_num here, I'd rather globals weren't used so you'll need to pass it in to the function. I've made changes to master so that this doesn't work anymore :) @@ +121,3 @@ + hid_list = g_udev_client_query_by_subsystem (client, "hid"); + element = g_list_first(hid_list); + while (element) { for (l = hid_list; hid_list != NULL; l = l->next) { } @@ +186,3 @@ } if (g_strcmp0 (g_udev_device_get_property (device, "ID_BUS"), "usb") != 0) { At this point, I would create a new function: char * get_led_sys_path (GUdevDevice *device, int group_num, int led_num, int &value); And hide the USB/Bluetooth differences in there.
Created attachment 253057 [details] [review] Led indicator handling over bluetooth v3 The patch has been tested in 90% - works fine for usb connection from jhbuild session and works from command line for bluetooth connection. The problem with jhbuild session is not related to gsd-wacom-led-helper as the helper is not even called, so I'm quite sure it just my local problem. Please review.
Created attachment 256462 [details] [review] wacom: Add OLED handling over Bluetooth For the Intuos4 Wireless tablets.
I'll leave this up to you to debug, and will commit once you've root-caused the problem. I've added a commit to the list utility to show whether there's LEDs or OLEDs detected for a particular button. The output of that on your device should show whether it's supposed to poke the OLEDs. commit d8096ba76ef12f357c5cd5a58e9e7175edf668c9 Author: Bastien Nocera <hadess@hadess.net> Date: Fri Oct 4 13:38:17 2013 +0200 wacom: Show whether buttons have (O)LEDs in debug tool
I just tested the patch and it works fine. The problem I had was caused by access rights to brightness files. Please apply.
Pushed to gnome-3-10 and master. Attachment 256462 [details] pushed as d456727 - wacom: Add OLED handling over Bluetooth
Review of attachment 256462 [details] [review]: For future reference: the content of the patch is correct, but the title is not. It's _LED_ only patch.