On Sat, Mar 19, 2011 at 6:55 PM, Ping Cheng <[email protected]> wrote:
> On Fri, Mar 18, 2011 at 4:37 PM, Chris Bagwell <[email protected]> wrote:
>>
>> On Fri, Mar 18, 2011 at 5:04 PM, Ping Cheng <[email protected]> wrote:
>> > Serial ISDv4 kernel driver, wacom_w8001.ko, provides both pen and
>> > touch events on the same logical port. Filtering touch events when
>> > pen is in proximity while allowing pen events (ABS_X/Y, etc) pass.
>> >
>> > Making this action configurable would make sense when XInput 2.1 is
>> > ready. At that point, we can post MT valuators while pen events are
>> > posted. Some code refactoring is needed to add a new channel to
>> > store the pen data as well as the 2FGT data.
>> >
>> > Defering the code refactoring until we support XInput 2.1.
>> >
>> > Signed-off-by: Ping Cheng <[email protected]>
>> > Reviewed-by: Peter Hutterer <[email protected]>
>> > ---
>> > v2 changes: Merged patch 2 into 1. Incorporated Peter's comments.
>> > Added Reviewed-by.
>> >
>> > src/wcmUSB.c | 98
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> > 1 files changed, 92 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/src/wcmUSB.c b/src/wcmUSB.c
>> > index 9e0f310..79d9ae7 100644
>> > --- a/src/wcmUSB.c
>> > +++ b/src/wcmUSB.c
>> > @@ -33,6 +33,8 @@
>> > typedef struct {
>> > int wcmLastToolSerial;
>> > int wcmBTNChannel;
>> > + int wcmDeviceType;
>> > + Bool wcmPenTouch;
>> > Bool wcmUseMT;
>> > int wcmMTChannel;
>> > int wcmPrevChannel;
>> > @@ -571,8 +573,14 @@ int usbWcmGetRanges(InputInfoPtr pInfo)
>> > common->wcmMaxDist = absinfo.maximum;
>> >
>> > if (ISBITSET(abs, ABS_MT_SLOT))
>> > + {
>> > private->wcmUseMT = 1;
>> >
>> > + /* pen and MT on the same logical port */
>> > + if (ISBITSET(common->wcmKeys, BTN_TOOL_PEN))
>> > + private->wcmPenTouch = TRUE;
>> > + }
>> > +
>> > /* A generic protocol device does not report ABS_MISC event */
>> > if (!ISBITSET(abs, ABS_MISC))
>> > common->wcmProtocolLevel = WCM_PROTOCOL_GENERIC;
>> > @@ -865,12 +873,33 @@ static int usbFilterEvent(WacomCommonPtr common,
>> > struct input_event *event)
>> > }
>> > else if (event->type == EV_ABS)
>> > {
>> > - switch(event->code)
>> > + /* When pen and touch are on the same port, we
>> > need
>> > + * to leave pen data alone.
>> > + */
>> > + if ((private->wcmDeviceType != STYLUS_ID) &&
>> > + (private->wcmDeviceType !=
>> > ERASER_ID))
>>
>> Can you make this an affirmative check? The intent is filter
>> X/Y/Pressure if device support MT and current tool overlaps MT events
>> (pointer emulation).
>
> Right.
>
>> So I think it can be simplified for now to 'if
>> (TOUCH_ID)".
>
> How come I didn't think about it then ;). It will be in v3.
>
>>
>> > {
>> > - case ABS_X:
>> > - case ABS_Y:
>> > - case ABS_PRESSURE:
>> > - return 1;
>> > + /* filter ST for MT */
>> > + switch(event->code)
>> > + {
>> > + case ABS_X:
>> > + case ABS_Y:
>> > + case ABS_PRESSURE:
>> > + return 1;
>> > + }
>> > + }
>> > + else
>> > + {
>> > + /* filter MT for pen */
>> > + switch(event->code)
>> > + {
>> > + case ABS_MT_SLOT:
>> > + case ABS_MT_TRACKING_ID:
>> > + case ABS_MT_POSITION_X:
>> > + case ABS_MT_POSITION_Y:
>> > + case ABS_MT_PRESSURE:
>> > + return 1;
>> > + }
>> > }
>> > }
>> > }
>> > @@ -1219,6 +1248,46 @@ static int usbParseBTNEvent(WacomCommonPtr
>> > common,
>> > return change;
>> > }
>> >
>> > +/***
>> > + * Retrieve the tool type from an USB data packet by looking at the
>> > event
>> > + * codes. Refer to linux/input.h for event codes that define tool
>> > types.
>> > + *
>> > + * @param data A pointer to struct wcmUSBData to access device specific
>> > data.
>> > + */
>> > +static void usbGetToolType(wcmUSBData *data)
>> > +{
>>
>> This function is much more readable then v1. Thanks!
>
> Thank you for your review and comments.
>
>>
>> I'll study it more and may have a follow up patch that moves the other
>> BTN_TOOL_* logic to this function so its always done up front. I
>> believe this process-tool-event-up-front aligns with recent
>> xf86-input-evdev changes as well.
>
> We'll need to move findDeviceType() from wcmCommon.c to wcmUSB.c to fully
> support the "tool type up front" logic. I agree that is the way to go.
>
>> > + int i;
>> > + struct input_event* event;
>> > +
>> > + /* default to 0 if no pen/touch/eraser event code is in the
>> > packet */
>> > + data->wcmDeviceType = 0;
>> > +
>> > + for (i = 0; (i < data->wcmEventCnt) && !data->wcmDeviceType;
>> > ++i)
>> > + {
>> > + event = data->wcmEvents + i;
>> > +
>> > + switch (event->code)
>> > + {
>> > + case BTN_TOOL_PEN:
>> > + case BTN_TOOL_PENCIL:
>> > + case BTN_TOOL_BRUSH:
>> > + case BTN_TOOL_AIRBRUSH:
>> > + data->wcmDeviceType = STYLUS_ID;
>> > + break;
>> > +
>> > + case BTN_TOOL_FINGER:
>> > + case ABS_MT_SLOT:
>> > + case ABS_MT_TRACKING_ID:
>> > + data->wcmDeviceType = TOUCH_ID;
>> > + break;
>> > +
>> > + case BTN_TOOL_RUBBER:
>> > + data->wcmDeviceType = ERASER_ID;
>> > + break;
>> > + }
>> > + }
>> > +}
>> > +
>> > static void usbDispatchEvents(InputInfoPtr pInfo)
>> > {
>> > int i;
>> > @@ -1228,11 +1297,28 @@ static void usbDispatchEvents(InputInfoPtr
>> > pInfo)
>> > WacomCommonPtr common = priv->common;
>> > int channel;
>> > int channel_change = 0, btn_channel_change = 0, mt_channel_change
>> > = 0;
>> > - WacomDeviceState dslast;
>> > + WacomDeviceState dslast = common->wcmChannel[0].valid.state;
>> > + Bool pen_in_prox = ((dslast.device_type == STYLUS_ID) ||
>> > + (dslast.device_type == ERASER_ID)) &&
>> > dslast.proximity;
>>
>> I'm very confused by why dslast is needed. I'm sure if I thought
>> about it harder, I'd understand. Can you add comment to explain for
>> future reference?
>
> We get both pen and touch data from the kernel when they both are in/down.
> So, if we were processing pen events (pen is in prox), we should ignore ST
> events.
>
> But, we would want to post MT events to the userland so gestures may be
> performed while pen moves the cursor. That requires a new channel to store
> the extra set of packets. Later...
>
>>
>> Offhand, I'm thinking it should be a function called
>> usbIsStylusToolInProx().
>
> How about isPenInProx()? That would work for all devices, USB or not.
Sounds good to me.
>
>> > wcmUSBData* private = common->private;
>> >
>> > DBG(6, common, "%d events received\n", private->wcmEventCnt);
>> >
>> > + if (private->wcmPenTouch)
>> > + {
>> > + usbGetToolType(private);
>>
>> This doesn't seem to be getting anything. Maybe call Setup or Init or
>> similar.
>
> usbInitToolType()?
Sounds good to me.
>
>>
>> > +
>> > + /* don't process touch event when pen is in-prox.
>> > + * Touch events will be posted to the userland when
>> > XInput 2.1
>> > + * is ready.
>> > + */
>> > + if ((private->wcmDeviceType == TOUCH_ID) && pen_in_prox)
>> > + {
>> > + private->wcmEventCnt = 0;
>> > + return;
>>
>> This is to aggressive. There could technically be two sync windows in
>> 1 buffer and this will discard both.
>
> We only process one sync'ed packet at a time, don't we? Refer to
> usbParseSynEvent().
Oh yeah. Opps. I somehow had ISDv4 logs on my mind and thinking this
count mirrored the extra data it shows. So this part looks good then.
>
>>
>> I think you can add the if()
>> part to lower down were it optionally calls wcmEvent().
>
> Let me know if you still think so after revisiting usbParseSynEvent.
>
>> Overall, I can tell these patches gets these ISDv4 touch+pen screens
>> working.
>
> Yeah, it has to ;).
>
>>
>> Wish I had one to play with. :-)
>
> Wait until we can move more fingers on the screen.
>
> Ping
>
------------------------------------------------------------------------------
Colocation vs. Managed Hosting
A question and answer guide to determining the best fit
for your organization - today and in the future.
http://p.sf.net/sfu/internap-sfd2d
_______________________________________________
Linuxwacom-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
