On Fri, Sep 28, 2012 at 12:49 AM, Olivier Fourdan <ofour...@redhat.com> wrote: > Hi all, > > [renaming the thread for LED related discussions] > > Ping Cheng said the following on 09/28/2012 03:54 AM: >> >> On Thu, Sep 27, 2012 at 3:20 PM, Jason Gerecke<killert...@gmail.com> >> wrote: >>> >>> > On Thu, Sep 27, 2012 at 1:56 PM, Ping Cheng<pingli...@gmail.com> >>> > wrote: >>> > >>>> >>>> >> BTW, this code from [1] does not fit what I understood >>>> >> >>>> >> + * - First ring (group 0) uses LED 0 >>>> >> + * - Second ring (group 1) uses LED 1 >>>> >> + * - First touchstrip (group 2) uses LED 1 >>>> >> + * - Second touchstrip (group 3) uses LED 0 >>>> >> >>>> >> There are two cases, tablet has one set of expresskeys; and tablet >>>> >> has two sets of expresskeys. >>>> >> >>>> >> If there is only one set of expresskeys, ring or touchstrip will and >>>> >> can only use LED 0; >>>> >> If there are two sets of expresskeys, the left set (i.e., your first >>>> >> ring/touchstrip) uses LED 1; the right set, your second >>>> >> ring/touchstrip, uses LED 0. >>>> >> >>>> >> [1]https://bugzilla.gnome.org/attachment.cgi?id=214732&action=edit >>> >>> > >>> > Not to get off-topic, but do we want to consider revising this >>> > interface to make it more logical? Having the "first" ring/strip being >>> > controlled by the "second" LED control (and vice versa) rubs me the >>> > wrong way. I'd consider it a bug, except that its the documented >>> > behavior. I'm not sure it qualifies as a "grave error" that Dmitry >>> > would allow to be changed, but it'd make more sense to fix it in the >>> > kernel than to rely on libwacom to describe the quirk. >> >> I think we are not talking the same thing. Maybe I didn't make myself >> clear. Let me try it again. >> >> I meant the four lines should read something like: >> >> + * - First ring (group 0) uses LED 1 >> + * - Second ring (group 1) uses LED 0 >> + * - First touchstrip (group 2) uses LED 1 >> + * - Second touchstrip (group 3) uses LED 0 >> >> That is, no matter it is ring or touchstrip, the one in the first >> group should use LED 1, the one in the second group uses LED 0. The >> kernel driver is consistent in this sense. > > > No, unfortunately not on Cintiq, that's precisely the problem, see [3] > below. > >> And this is for tablets >> with two sets of LEDs. If there is only one set of LEDs, such as >> Intuos 4 and 5, only LED 0 can be used. > > > What GNOME does right now, is to write to the following sysfs entries: > > - First ring => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second ring => /sys/bus/usb/devices/.../wacom_led/status_led1_select > - First touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led2_select > - Second touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led3_select > > This is not what the kernel expects as there is no > status_led2_select/status_led3_select in the SysFS documentation for LED > support [1] so LED support won't work in many cases > > That's bug 676558 [2] and the initial fix was to use something like: > > - First ring => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second ring => /sys/bus/usb/devices/.../wacom_led/status_led1_select > - First touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led1_select > > [That's what Ping said intially] > > But trying to fix like that raised another issue, as touchstrips LED are > inverted [3] on Cintiq, so what works in reality is: > > - First ring => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second ring => /sys/bus/usb/devices/.../wacom_led/status_led1_select > - First touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led1_select > - Second touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led0_select > Even that patch is wrong, unfortunately. 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. > But that patch was denied and the consensus reached was that such hardware > implementation details should be hidden from userland apps > > So we need to fix the kernel to get: > > - First ring => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second ring => /sys/bus/usb/devices/.../wacom_led/status_led1_select > - First touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led0_select > - Second touchstrip => /sys/bus/usb/devices/.../wacom_led/status_led1_select > > independently of the hardware. > I talked more with Ping yesterday, and 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. The only real solution I see is to create a new API that makes more sense, like what Przemo is trying to do for the OLEDs. Even that's not a perfect solution (more maintenance work in the kernel), but its not the quagmire that tweaking what already exists is. > Cheers, > Olivier. > > -- > [1]http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Documentation/ABI/testing/sysfs-driver-wacom;hb=HEAD > [2] https://bugzilla.gnome.org/show_bug.cgi?id=676558 > [3] https://bugzilla.gnome.org/show_bug.cgi?id=676558#c6 > Jason --- When you're rife with devastation / There's a simple explanation: You're a toymaker's creation / Trapped inside a crystal ball. And whichever way he tilts it / Know that we must be resilient We won't let them break our spirits / As we sing our silly song. ------------------------------------------------------------------------------ Got visibility? Most devs has no idea what their production app looks like. Find out how fast your code is with AppDynamics Lite. http://ad.doubleclick.net/clk;262219671;13503038;y? http://info.appdynamics.com/FreeJavaPerformanceDownload.html _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
