GNOME Bugzilla – Bug 676558
Wacom: LED do not lit up with Touchstrip Mode Switch
Last modified: 2012-12-20 09:21:12 UTC
Created attachment 214654 [details] [review] Proposed patch for "gsd-wacom-led-helper" g-s-d uses 4 groups for Mode switch: WACOM_BUTTON_RING_MODESWITCH => group 1 WACOM_BUTTON_RING2_MODESWITCH => group 2 WACOM_BUTTON_TOUCHSTRIP_MODESWITCH => group 3 WACOM_BUTTON_TOUCHSTRIP2_MODESWITCH => group 4 And this mode (minus 1) is passed to the "gsd-wacom-led-helper" to determine the SYSFS file status_led<n>_select to write to. But the kernel provides only 2 groups of LED [1]: What: /sys/bus/usb/devices/<busnum>-<devnum>:<cfg>.<intf>/wacom_led/status_led0_select Date: August 2011 Contact: linux-input@vger.kernel.org Description: Writing to this file sets which one of the four (for Intuos 4) or of the right four (for Cintiq 21UX2 and Cintiq 24HD) status LEDs is active (0..3). The other three LEDs on the same side are always inactive. What: /sys/bus/usb/devices/<busnum>-<devnum>:<cfg>.<intf>/wacom_led/status_led1_select Date: September 2011 Contact: linux-input@vger.kernel.org Description: Writing to this file sets which one of the left four (for Cintiq 21UX2 and Cintiq 24HD) status LEDs is active (0..3). The other three LEDs on the left are always inactive. So the LED do not work with devices which use Touchstrip Mode Switch such as the Cintiq 21UX2 The patch attached here write to status_led0_select/status_led1_select only as this is all the Kernel provides. [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Documentation/ABI/testing/sysfs-driver-wacom;hb=HEAD
Review of attachment 214654 [details] [review]: ::: plugins/wacom/gsd-wacom-led-helper.c @@ +72,3 @@ char *filename; + /* Only "status_led0_select" and "status_led1_select" are available */ I'd expect something a bit more verbose here. The bug description is too long, but this is certainly too short.
(In reply to comment #1) > [...] > I'd expect something a bit more verbose here. > > The bug description is too long, but this is certainly too short. Not sure what's wrong with a detailed bug description, but I'll leave it to you then to update the comment in the code, as I am not not sure what you expect exactly (other than we use 4 groups internally and there's only 2 control files provided by the kernel interface).
" There's a maximum of 2 LED groups handled by the kernel, as you can't have strip LEDs and ring button LEDs on the same device First ring or touchstrip uses LED 0 Second ring or touchstrip uses LED 1 " (and add a link to the description in git)
Created attachment 214670 [details] [review] Updated patch Updated patch as per comment #3
Review of attachment 214670 [details] [review]: Good.
Review of attachment 214670 [details] [review]: Not correct on the Cintiq 21UX2, the status LEDs are swapped with the button that triggers them (left button changes right LEDs, etc) I don't know if that is true for the 24HD as well.
Created attachment 214730 [details] [review] Updated patch Updated patch as per comment #6 Note, this patch assumes the following: - First ring uses LED 0 - Second ring uses LED 1 - First touchstrip uses LED 1 - Second touchstrip uses LED 0 If that's not true for all devices and the actual mapping is device dependent, then there's no (clean) way I see to fix this in the helper program as it has no knowledge of the actual device (libwacom would need to provide us with that info in that case)
Created attachment 214732 [details] [review] Updated patch Same as comment #7 with updated comment for clarification.
(In reply to comment #6) > Review of attachment 214670 [details] [review]: > > Not correct on the Cintiq 21UX2, the status LEDs are swapped with the button > that triggers them (left button changes right LEDs, etc) I don't know if that > is true for the 24HD as well. What's the reason for the kernel driver not hiding that problem? We shouldn't need to know how the device is wired electrically...
Comment on attachment 214732 [details] [review] Updated patch As per discussions on linuxwacom-devel
Created attachment 225500 [details] [review] Updated patch Updated patch following latest discussions on linuxwacom-devel http://sourceforge.net/mailarchive/message.php?msg_id=29898591 Excerpts follow: "As noted in both the kernel's ABI docs and the libwacom datafiles, it should be as follows: Intuos4 (only [left] ring): => status_led0_select Intuos5 (only [left] ring): => status_led0_select Cintiq 21UX2 (left strip): => status_led1_select Cintiq 21UX2 (right strip): => status_led0_select Cintiq 22HD (left strip) => NO LEDs Cintiq 22HD (right strip) => NO LEDs Cintiq 24HD (left ring): => status_led1_select Cintiq 24HD (right ring): => status_led0_select Cintiq 24HD touch (left ring): => status_led1_select Cintiq 24HD touch (right ring): => status_led0_select Though its not a /documented/ rule, the behavior can be summed up as follows: if a device has only one ring/strip, use status_led0_select; otherwise the left ring/strip is controlled by status_led1_select and the right ring/strip by status_led0_select." [...] "I'm not sure if its something that can really be fixed. The odd behavior is documented, which means its not a bug. A fix then would fall under the category of kernel API change, which can only be done for "grave errors or security problems" (i.e., not bad API choice). Because the interface has been upstream for a year now (almost to the day) which means there's likely non-GNOME code out there using it -- "fixing" the kernel would thus break existing applications."
Review of attachment 225500 [details] [review]: Code looks fine for 3.6, but I really want to see those functions moved to libwacom for 3.8. ::: plugins/wacom/gsd-wacom-device.c @@ +1187,3 @@ + *num_rings = 1; + else + *num_rings = 0; We could also add the necessary API to libwacom. Can you do that for GNOME 3.8 please? (File the bug about using that new API in the meanwhile) ::: plugins/wacom/gsd-wacom-manager.c @@ +551,3 @@ + */ +static int +get_led_group_id(GsdWacomDevice *device, Could we not move this ugly code to libwacom as well? It seems that this is exactly the sort of thing that we really want in libwacom instead of having magic numbers in g-s-d.
Agreed, that belongs to libwacom. I'll send patches to libwacom and update the patch for 3.8 with the newly added libwacom API.
Created attachment 225667 [details] [review] Updated patch for GNOME 3.6 Instead of issuing a warning when trying to access a non-existing touchring/touchstrip and still try to write to the corresponding LED and thus issuing another warning in gsd-wacom-led-helper, use the result of the helper function get_led_group_id() and ignore the request if not valid.
Review of attachment 225667 [details] [review]: Looks good to commit to 3.6.
Thanks! Pushed to git master! Leaving BZ open for 3.8 once the logic has landed in libwacom, current relevant thread on linuxwacom-devel is http://sourceforge.net/mailarchive/message.php?msg_id=29913781
Comment on attachment 225667 [details] [review] Updated patch for GNOME 3.6 Marking as committed as it actually was.
Adding dependency on bug 689599 to use the new API in libwacom 0.7
Created attachment 230739 [details] [review] Proposed patch for GNOME 3.8 using libwacom 0.7 new API
Review of attachment 230739 [details] [review]: ::: plugins/wacom/gsd-wacom-device.h @@ +101,3 @@ #define MAX_GROUP_ID 4 +#define GSD_WACOM_LED_UNAVAILABLE -1 unavailable seems to me that it would mean the LED could be available at some point on that button. GDS_WACOM_NO_LED?
(In reply to comment #20) > +#define GSD_WACOM_LED_UNAVAILABLE -1 > > unavailable seems to me that it would mean the LED could be available at some > point on that button. > GDS_WACOM_NO_LED? Yeap, NO_LED is better :-)
Created attachment 231886 [details] [review] Updated patch after review for GNOME 3.8 using libwacom 0.7 API (In reply to comment #20) > Review of attachment 230739 [details] [review]: > +#define GSD_WACOM_LED_UNAVAILABLE -1 > > unavailable seems to me that it would mean the LED could be available at some > point on that button. > GDS_WACOM_NO_LED? Updated.
Review of attachment 231886 [details] [review]: Looks good.
Comment on attachment 231886 [details] [review] Updated patch after review for GNOME 3.8 using libwacom 0.7 API Pushed in git master