On Wed, Dec 29, 2010 at 12:38 PM, Chris Bagwell <ch...@cnpbagwell.com> wrote:
> On Wed, Dec 29, 2010 at 12:01 PM, Ping Cheng <pingli...@gmail.com> wrote:
>> Hi Chris,
>>
>> I like the concept of this patchset. Patch one looks fine. I have some
>> comments for this one inline.
>>
>> Thank you.
>>
>> Ping
>>
>> On Tue, Dec 28, 2010 at 5:10 PM, <ch...@cnpbagwell.com> wrote:
>>> From: Chris Bagwell <ch...@cnpbagwell.com>
>>>
>>> BTN_TOOL_FINGER/DOUBLETAP/TRIPLETAP have incompatible meanings
>>> between generic and protocol 4/5 devices. Add logic to give
>>> rough value of wcmProtocolLevel when probing wcmKeys (can't
>>> tell difference between 4 and 5 which is OK at this level).
>>>
>>> Use this rough value to generic set 1FGT and 2FGT features.
>>>
>>> Any new MT touchpad using ABS_MT_SLOT will now work without
>>> modification to xf86-input-wacom.
>>>
>>> Any new protocol 4 touchpad using TRIPLETAP or DOUBLETAP will
>>> also now work without modification to xf86-input-wacom.
>>>
>>> Touchscreens will work in relative mode because currently only
>>> tablet_id informs this. Generic protocols may eventually have
>>> something in kernel that can be queried for touchscreen vs.
>>> touchpad.
>>>
>>> Signed-off-by: Chris Bagwell <ch...@cnpbagwell.com>
>>> ---
>>> src/wcmISDV4.c | 3 +++
>>> src/wcmUSB.c | 16 ++++++++++++++++
>>> src/wcmValidateDevice.c | 31 +++++++++++++++++++------------
>>> 3 files changed, 38 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/wcmISDV4.c b/src/wcmISDV4.c
>>> index 2c3ca9a..6c50dfb 100644
>>> --- a/src/wcmISDV4.c
>>> +++ b/src/wcmISDV4.c
>>> @@ -942,6 +942,9 @@ static int isdv4ProbeKeys(InputInfoPtr pInfo)
>>> SETBIT(common->wcmKeys, BTN_TOOL_PEN);
>>> SETBIT(common->wcmKeys, BTN_TOOL_RUBBER);
>>>
>>> + /* wcmKeys are based on Protocol 4 meanings */
>>
>> Can you elaborate this comment a little bit? I can not link wcmKeys
>> directly to Protocol 4 somehow. I can link wcmKeys to
>> WCM_PROTOCOL_GENERIC though.
>
> I needed a boolean variable that said which of two meanings values of
> BTN_TOOL_FINGER/DOUBLETAP/TRIPLETAP represent in wcmKeys. I chose to
> re-use wcmProtocolLevel and values of WCM_PROCOTOL_GENERIC and
> WCM_PROTOCOL_4 as those boolean values since we are using it for
> similar purpose elsewhere in code.
>
> I'm open to recommendations here.
I understand why you need to introduce WCM_PROCOTOL to the discussion.
I have no question about that. I just felt it would be hard for
someone who work on the code later to understand the comment. My
comment is about the comment, not the code. Since we don't need to
have the code here anymore, the comment is gone ;).
>>> + common->wcmProtocolLevel = WCM_PROTOCOL_4;
>>
>> wcmProtocolLevel defaults to WCM_PROTOCOL_4. Why do we need to reassign it
>> here?
>
> Ah, yes, I guess its not needed. We also needlessly reinit it later
> on in wcmISDV4 I guess and why I thought it may be needed. I'll
> remove this.
>
>>
>>> +
>>> if (model->set_bits)
>>> tablet_id = model->set_bits(id, common->wcmKeys);
>>>
>>> diff --git a/src/wcmUSB.c b/src/wcmUSB.c
>>> index c25e67c..9341ee4 100644
>>> --- a/src/wcmUSB.c
>>> +++ b/src/wcmUSB.c
>>> @@ -1358,12 +1358,15 @@ static void usbDispatchEvents(InputInfoPtr pInfo)
>>> * Query the device's fd for the key bits and the tablet ID. Returns the ID
>>> * on success or 0 on failure.
>>> * For USB devices, we simply copy the information the kernel gives us.
>>> + * Since keys have different meanings between protocol 4/5 and generic,
>>> + * assign that a rough value here.
>>> */
>>> static int usbProbeKeys(InputInfoPtr pInfo)
>>> {
>>> struct input_id wacom_id;
>>> WacomDevicePtr priv = (WacomDevicePtr)pInfo->private;
>>> WacomCommonPtr common = priv->common;
>>> + unsigned long abs[NBITS(ABS_MAX)] = {0};
>>>
>>> if (ioctl(pInfo->fd, EVIOCGBIT(EV_KEY, (sizeof(unsigned long)
>>> * NBITS(KEY_MAX))),
>>> common->wcmKeys) < 0)
>>> @@ -1380,6 +1383,19 @@ static int usbProbeKeys(InputInfoPtr pInfo)
>>> return 0;
>>> }
>>>
>>> + if (ioctl(pInfo->fd, EVIOCGBIT(EV_ABS, sizeof(abs)), abs) < 0)
>>> + {
>>> + xf86Msg(X_ERROR, "%s: usbProbeKeys unable to ioctl "
>>> + "abs bits.\n", pInfo->name);
>>> + return 0;
>>> + }
>>> +
>>> + /* A generic protocol device does not report ABS_MISC event */
>>> + if (ISBITSET(abs, ABS_MISC))
>>> + common->wcmProtocolLevel = WCM_PROTOCOL_4;
>>> + else
>>> + common->wcmProtocolLevel = WCM_PROTOCOL_GENERIC;
>>> +
>>
>> Is
>>
>> if (!ISBITSET(abs, ABS_MISC))
>> common->wcmProtocolLevel = WCM_PROTOCOL_GENERIC;
>>
>> enough? wcmProtocolLevel defaults to WCM_PROTOCOL_4 anyway. If you
>> prefer your way, I am fine.
>
> Well, I wanted to do exactly that originally but "abs" is not
> available in this non-USB area of code.
This particular chunk is for usbProbeKeys in wcmUSB.c, isn't it?
> I toyed with multiple ways to
> resolve this basic issue (move tablet_type setting into ProbeKeys()
> for example) but settled on this as smallest amount of code changes.
>
> Again, I'm open to what ever approach makes sense to everyone but
> above happened to be simplest.
Are we talking about usbProbeKey specifically or ProbeKeys in general here?
>>> return wacom_id.product;
>>> }
>>>
>>> diff --git a/src/wcmValidateDevice.c b/src/wcmValidateDevice.c
>>> index b7312a3..9e0cca3 100644
>>> --- a/src/wcmValidateDevice.c
>>> +++ b/src/wcmValidateDevice.c
>>> @@ -256,28 +256,16 @@ int wcmDeviceTypeKeys(InputInfoPtr pInfo)
>>> case 0xE3: /* TPC with 2FGT */
>>> priv->common->tablet_type = WCM_TPC;
>>> priv->common->tablet_type |= WCM_LCD;
>>> - /* fall through */
>>> - case 0xD0: /* Bamboo with 2FGT */
>>> - case 0xD1: /* Bamboo with 2FGT */
>>> - case 0xD2: /* Bamboo with 2FGT */
>>> - case 0xD3: /* Bamboo with 2FGT */
>>> - case 0xD8: /* Bamboo with 2FGT */
>>> - case 0xDA: /* Bamboo with 2FGT */
>>> - case 0xDB: /* Bamboo with 2FGT */
>>> - priv->common->tablet_type |= WCM_2FGT;
>>> break;
>>>
>>> case 0x93: /* TPC with 1FGT */
>>> case 0x9A: /* TPC with 1FGT */
>>> - priv->common->tablet_type = WCM_1FGT;
>>> - /* fall through */
>>> case 0x90: /* TPC */
>>> priv->common->tablet_type |= WCM_TPC;
>>> priv->common->tablet_type |= WCM_LCD;
>>> break;
>>>
>>> case 0x9F:
>>> - priv->common->tablet_type = WCM_1FGT;
>>> priv->common->tablet_type |= WCM_LCD;
>>> break;
>>>
>>> @@ -291,6 +279,25 @@ int wcmDeviceTypeKeys(InputInfoPtr pInfo)
>>> priv->common->tablet_type |= WCM_PAD;
>>> }
>>>
>>> + /* Protocol 4 TRIPLETAP means 2 finger touch. */
>>> + if (common->wcmProtocolLevel == WCM_PROTOCOL_4 &&
>>> + ISBITSET(common->wcmKeys, BTN_TOOL_TRIPLETAP))
>>> + {
>>> + priv->common->tablet_type |= WCM_2FGT;
>>> + }
>>> + /* Protocol 4 DOUBLETAP without TRIPLETAP means 1 finger touch */
>>> + else if (common->wcmProtocolLevel == WCM_PROTOCOL_4 &&
>>> + ISBITSET(common->wcmKeys, BTN_TOOL_DOUBLETAP))
>>> + {
>>> + priv->common->tablet_type |= WCM_1FGT;
>>> + }
>>
>> Does the following work for you?
>>
>> + if (common->wcmProtocolLevel == WCM_PROTOCOL_4)
>> + {
>> + /* Protocol 4 TRIPLETAP means 2 finger touch. */
>> + if(ISBITSET(common->wcmKeys, BTN_TOOL_TRIPLETAP))
>> + priv->common->tablet_type |= WCM_2FGT;
>> + /* Protocol 4 DOUBLETAP without TRIPLETAP means 1 finger touch */
>> + else if (ISBITSET(common->wcmKeys, BTN_TOOL_DOUBLETAP))
>> + priv->common->tablet_type |= WCM_1FGT;
>> + }
>>
>
> Yes, I'm fine with that.
>
>>> +
>>> + if (common->wcmProtocolLevel == WCM_PROTOCOL_GENERIC)
>>> + {
>>> + if (ISBITSET(common->wcmKeys, BTN_TOOL_DOUBLETAP))
>>> + priv->common->tablet_type |= WCM_2FGT;
>>> + }
>>
>> Can we use MT slot number for WCM_2FGT? It would be easier to add
>> 3FGT, 4FGT, etc. when needed later. Also, should we set WCM_1FGT
>> feature for single touch devices in GENERIC protocol? Bamboo does not
>> have one finger. But others have. BTN_TOOL_FINGER (for GENERIC type,
>> this key is safe ;) can be used for this purpose.
>>
>
> On MT part first: I originally wanted to wrap above with
> ISBITSET(abs,ABS_MT_SLOT) but that info is not available here. We'd
> need to create a wcmAbs to pass that around. I believe your
> suggesting using the max value from absinfo for ABS_MT_SLOT. Thats
> possible as well but needs similar update to expose wcmMTSlots.
>
> Maybe we should expose one or the other of those two things but I
> settled on fact that above works and so did simplest in this patch.
> It may mistakenly set 2FGT if someone, for example, routed a 2 finger
> synaptics touchpad to xf86-input-wacom... but it would also be
> harmless. We'd set up to process 2 touches but would just never get
> the MT events and only process the ST ABS_X/Y/PRESSURE events instead
> (elsewhere we filter out BTN_TOOL_DOUBLETAP indications for GENERIC
> devicess).
>
> For short term, I'd rather stick to checking FINGER/DOUBLETAP for
> finger counts and optionally wrap with check for ABS_MT_SLOTS if we
> feel its needed. I think MT ioctl()'s will be in flux in this area.
> For example, 3 slots could be 2 fingers and 1 pen instead of 3
> fingers.
Ok, I agree. One step at a time.
> On the 1FGT touch: Sure, I'll add that in. I think we are very close
> to being able to handle wacom_w8001 with combined
> pen+eraser+single_touch_sent_as_BTN_TOOL_FINGER. Adding the check for
> BTN_TOOL_FINGER=WCM_1FGT is pre-requisite though.
>
> Thanks for comments,
Thank you for the patchset.
Ping
------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and,
should the need arise, upgrade to a full multi-node Oracle RAC database
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
