After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 676558 - Wacom: LED do not lit up with Touchstrip Mode Switch
Wacom: LED do not lit up with Touchstrip Mode Switch
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: wacom
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Olivier Fourdan
gnome-settings-daemon-maint
Depends on: 689599
Blocks:
 
 
Reported: 2012-05-22 13:40 UTC by Olivier Fourdan
Modified: 2012-12-20 09:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch for "gsd-wacom-led-helper" (1008 bytes, patch)
2012-05-22 13:40 UTC, Olivier Fourdan
reviewed Details | Review
Updated patch (1.32 KB, patch)
2012-05-22 15:38 UTC, Olivier Fourdan
needs-work Details | Review
Updated patch (1.39 KB, patch)
2012-05-23 08:07 UTC, Olivier Fourdan
none Details | Review
Updated patch (1.47 KB, patch)
2012-05-23 08:14 UTC, Olivier Fourdan
rejected Details | Review
Updated patch (5.88 KB, patch)
2012-10-01 14:55 UTC, Olivier Fourdan
reviewed Details | Review
Updated patch for GNOME 3.6 (6.02 KB, patch)
2012-10-03 07:35 UTC, Olivier Fourdan
committed Details | Review
Proposed patch for GNOME 3.8 using libwacom 0.7 new API (10.73 KB, patch)
2012-12-05 09:55 UTC, Olivier Fourdan
accepted-commit_now Details | Review
Updated patch after review for GNOME 3.8 using libwacom 0.7 API (10.61 KB, patch)
2012-12-19 14:36 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2012-05-22 13:40:39 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
Comment 1 Bastien Nocera 2012-05-22 13:43:00 UTC
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.
Comment 2 Olivier Fourdan 2012-05-22 14:50:41 UTC
(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).
Comment 3 Bastien Nocera 2012-05-22 14:59:12 UTC
"
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)
Comment 4 Olivier Fourdan 2012-05-22 15:38:42 UTC
Created attachment 214670 [details] [review]
Updated patch

Updated patch as per comment #3
Comment 5 Bastien Nocera 2012-05-22 15:46:09 UTC
Review of attachment 214670 [details] [review]:

Good.
Comment 6 Peter Hutterer 2012-05-23 01:36:36 UTC
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.
Comment 7 Olivier Fourdan 2012-05-23 08:07:20 UTC
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)
Comment 8 Olivier Fourdan 2012-05-23 08:14:50 UTC
Created attachment 214732 [details] [review]
Updated patch

Same as comment #7 with updated comment for clarification.
Comment 9 Bastien Nocera 2012-05-23 09:49:02 UTC
(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 10 Bastien Nocera 2012-06-14 16:48:27 UTC
Comment on attachment 214732 [details] [review]
Updated patch

As per discussions on linuxwacom-devel
Comment 11 Olivier Fourdan 2012-10-01 14:55:05 UTC
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."
Comment 12 Bastien Nocera 2012-10-01 15:00:39 UTC
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.
Comment 13 Olivier Fourdan 2012-10-01 16:21:43 UTC
Agreed, that belongs to libwacom.

I'll send patches to libwacom and update the patch for 3.8 with the newly added libwacom API.
Comment 14 Olivier Fourdan 2012-10-03 07:35:39 UTC
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.
Comment 15 Bastien Nocera 2012-10-03 07:49:55 UTC
Review of attachment 225667 [details] [review]:

Looks good to commit to 3.6.
Comment 16 Olivier Fourdan 2012-10-04 08:10:35 UTC
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 17 Bastien Nocera 2012-10-09 08:54:20 UTC
Comment on attachment 225667 [details] [review]
Updated patch for GNOME 3.6

Marking as committed as it actually was.
Comment 18 Olivier Fourdan 2012-12-05 09:55:10 UTC
Adding dependency on bug 689599 to use the new API in libwacom 0.7
Comment 19 Olivier Fourdan 2012-12-05 09:55:46 UTC
Created attachment 230739 [details] [review]
Proposed patch for GNOME 3.8 using libwacom 0.7 new API
Comment 20 Bastien Nocera 2012-12-05 10:06:37 UTC
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?
Comment 21 Olivier Fourdan 2012-12-05 10:26:06 UTC
(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 :-)
Comment 22 Olivier Fourdan 2012-12-19 14:36:14 UTC
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.
Comment 23 Bastien Nocera 2012-12-19 14:41:00 UTC
Review of attachment 231886 [details] [review]:

Looks good.
Comment 24 Olivier Fourdan 2012-12-20 09:20:54 UTC
Comment on attachment 231886 [details] [review]
Updated patch after review for GNOME 3.8 using libwacom 0.7 API

Pushed in git master