On Fri, Mar 18, 2011 at 4:37 PM, Chris Bagwell <ch...@cnpbagwell.com> wrote:
> On Fri, Mar 18, 2011 at 5:04 PM, Ping Cheng <pingli...@gmail.com> 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 <pingli...@gmail.com>
> > Reviewed-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> > 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.
> 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()?
> > +
> > + /* 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().
> 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
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
