On Wed, Mar 26, 2014 at 8:38 PM, Paul A. Tessier <phern...@gmail.com> wrote:
> I'm pretty much set myself, now that I have it working the way I like. I
> just figured I'd try to wrap it up and send it where it might be useful. I'd
> like to avoid rolling my on driver forever :) I think I'm going to skip on
Glad to have a new face contributing :) I hope its not asking too much
for you to finish polishing things up here in the home stretch.
> adding more X buttons for the touch detection on the tablet buttons,
> xf86-input-wacom looks like it would take a while for me to figure out, and
> then I'd have to write a separate program to do pop-ups like the window's
> driver does, i.e. too much work.
>
The X driver is a little hard to grok, but once you know where to look
adding extra properties isn't too bad. And the userspace pop-up
overlays sounds like something right up GNOME's alley given all the
interesting work they've done in writing a control panel. The
trickiest part I think is simply getting the data out of the kernel in
a clean way. I had patches to get the touch-sensitivity working with
the Intuos5, but they were rejected by the kernel maintainers. If you
have any thoughts on how to accomplish it, I'd be glad to hear them :)
> In all things I defer to your expertise on this, as I haven't done much
> system programming or contributions to open source projects.
>
> I agree about using the uniq field was poorly thought out, but that was the
> very first thing I did to get the tablet to be recognized in ubuntu
> properly. Then I realized that the leds weren't working so I spent some time
> learning about sysfs, and using wireshark to do some USB sniffing.
> Everything I got was figured out by sniffing the USB going to a windows VM.
> So I really have no idea about what the protocols are supposed to be.
>
The protocol docs aren't generally available (I've got access to them,
but only under NDA...), so I was scratching my head quite a bit when I
saw you adding support for the sleep timers! You can learn quite a bit
from doing traces and twiddling bits in the driver if you have the
will and patience.
>
>
> On 03/26/2014 07:31 PM, Jason Gerecke wrote:
>>
>> Thanks for the patch! I've added my comments in-line below :)
>>
>> Firstly though, it looks like your patch was mangled along the way, so
>> I haven't been able to give it a whirl... In the future I'd suggest
>> either attaching the patches as files, or using `git send-email`.
>
> I've attached the original two patches as you suggested.
>
>> You should introduce an e.g. WAC_CMD_WIRELESS_CONTROL macro rather
>> than use a magic constant.
>
> Agreed.
>
>> Way more complex than it needs to be. `int crop_lum =
>> wacom->led.crop_lum & 0x03` should be all you need.
>
> I gave crop_lum the same range as ring_lum since they are controlled by the
> same slider in windows, which seems to have a large range even though the
> Intuos Pro only support 4 values. I wasn't sure if limiting the range would
> be too premature, plus I liked keeping it consistent with the other ranges
> of values for led brightness.
>
Ah, I remember now. We specify sysfs will take a brightness in range
0..127 for the ring, even though the hardware can't support all those
levels. For the sake of consistency, you're probably right in copying
the range.
>> This doesn't quite jibe with the protocol docs that I have. buf[2]
>> contains a "powersave" (EMR low-power mode) timer. The "sleep"
>> (EMR/MFT/wireless minimum power) timer is controlled by buf[1]. If
>> you'd like to export both timers through sysfs, that'd be awesome :)
>
> The value in buf[2] was exactly the same as the number of minutes that the
> windows driver displayed. The windows driver is also probably wrong if what
> you say is correct. Which is most likely as setting it in windows didn't
> really seem to have the correct effect. Setting it to 1 minute, it didn't go
> to sleep for 30 minutes some times, regardless of being left alone. Other
> times is slept quite quickly.
>
Full "sleep" mode is admittedly somewhat more annoying to leave since
you have to press a button to wake the tablet. The "powersave" mode by
contrast should be exited within a fraction of a second of using the
tablet. The Windows driver might only be changing the powersave for
user-experience reasons I suppose. I'd definitely see what difference
you can find between buf[1] and buf[2].
>>> - return snprintf(buf, 2, "%d\n", wacom->led.select[SET_ID]); \
>>> + return snprintf(buf, 3, "%d\n", wacom->led.select[SET_ID]); \
>>
>> Keen eye.
>
> For the sysfs show functions I would have prefered to do snprintf(buf,
> PAGE_SIZE, ... ) like sysfs.txt suggests, but when in rome ...
>
In that case, I'm tempted to say "even Rome eventually fell" and start
using PAGE_SIZE if that's what sysfs.txt suggests.
>>> + wacom->sleep_timer = clamp_val(time, 1, 20);
>>
>> Valid values are from 5 to 60 (below the tablet defaults to 15 minute
>> sleep; above it disables sleep). For the powersave timer, they're 1 to
>> 60 (below, 1 minute default; above disabled)
>>
>> We might want to have seperate attributes that allow the user to
>> explicitely disable/default the timers, or possibly allow these to
>> take special values (0 = disabled, -1 = default?). Whatever the
>> decision, our sysfs documentaiton should be updated to reflect it.
>
> The values for the sleep timer only goes from 1 to 20 in windows, so I
> copied that. I do like your suggestions much better.
>
Good to know what the range used on Windows is as well; control panel
developers might be interested in knowing what Windows and Mac have by
default.
>>> + return snprintf(buf, 5, "%d\n", wacom->sleep_timer);
>>
>> Only need 4, not 5.
>
> Yes, bad math ... should have just used PAGE_SIZE.
>
>>> + wacom->led.crop_lum = 32;
>>
>> Should be `wacom->led.crop_lum = 2`
>
> Staying consistent with llv and hlv.
>
>>> + if (wacom_wac->uniq[0] == 0) {
>>> + if (wacom_wac->features.quirks & WACOM_QUIRK_WIRELESS_KIT)
>>> + snprintf(wacom_wac->uniq, WACOM_UNIQ_MAX, "056a:%04x",
>>> + wacom_wac->pid);
>>
>> Instead of hardcoding "056a" into the string, use USB_VENDOR_ID_WACOM.
>
> Doh!
>
>>> + else
>>> + snprintf(wacom_wac->uniq, WACOM_UNIQ_MAX, "%04x:%04x",
>>> + dev->descriptor.idVendor, dev->descriptor.idProduct);
>>> + }
>>> +
>>> input_dev->name = wacom_wac->name;
>>> + input_dev->uniq = wacom_wac->uniq;
>>
>> The tablet VID:PID isn't a unique enough identifier to be stored in
>> the `uniq` field. You should add a sysfs attribute to the wireless
>> group which exports the information.
>
> Agree completely, but after going through the trouble of adding the
> wacom_wireless sysfs group, I realized I should move the wireless tablet pid
> there, but the sleep timer was the last thing I did, and redoing previous
> work when I finally got everything working was too much.
>
I know exactly how you feel :D Unfortunately, the upstream kernel
would never accept a patch which did that :(
>>> +
>>> + switch (wacom_features->type) {
>>> + case INTUOSPS:
>>> + case INTUOSPM:
>>> + case INTUOSPL:
>>
>> All wireless tablets should have this particular group, IIRC. Bamboo
>> PIDs 0xDC-0xDF, Intuos, Intuos5, and Intuos Pro.
>
> Wasn't sure if they worked the same.
>
>>> + if (wacom_features->device_type == BTN_TOOL_PEN) {
>>> + wacom->sleep_timer = 2;
>>
>> The default value for this and the powersave timer are 15 minutes and
>> 1 minute respectively. You should set them to zero to use the hardware
>> default.
>>
> The windows default was 2. I wasn't sure if 0 meant never sleep and I don't
> like killing my batteries.
>
>> 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....
>
>
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....
------------------------------------------------------------------------------
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
