sorry about the delay, as usual too many things on my slate
On Fri, Dec 31, 2010 at 10:48:47PM -0600, ch...@cnpbagwell.com wrote:
> From: Chris Bagwell <ch...@cnpbagwell.com>
>
> Kernel side input event filtering forces user land to track previous
> tools values when switching to new tools. If its not accounted
> for, sending new-but-duplicate values for new tool would cause confusion.
> Most cases of cursor jumps when entering proximity can be traced
> to how its (not) being handled in todays code.
>
> For generic and protocol 4 devices, its just a matter of removing
> existing memset(). For 2FGT protocol 4 devices, its slightly
> harder because it switches fingers between channels.
>
> Protocol 5 DUALINPUT devices are harder. Take example of 2
> stylus in proximity at same time and one has tilt support but
> the other one does not. Current kernel drivers will always
> report tilt values upon query and require user land to filter
> out as needed. So we can't just copy previous values between
> tools. For Protocol 5, this patch contiues to use old approach;
> at least until kernel side changes can be made.
>
> In this change, moved a strange force to in proximity to special case
> of no tool found. See deleted comments in patch for background on that.
>
> Signed-off-by: Chris Bagwell <ch...@cnpbagwell.com>
> ---
> src/wcmUSB.c | 109
> ++++++++++++++++++++++-----------------------------
> src/xf86WacomDefs.h | 1 +
> 2 files changed, 48 insertions(+), 62 deletions(-)
>
> diff --git a/src/wcmUSB.c b/src/wcmUSB.c
> index c25e67c..faeb3c4 100644
> --- a/src/wcmUSB.c
> +++ b/src/wcmUSB.c
> @@ -35,6 +35,7 @@ typedef struct {
> int wcmBTNChannel;
> Bool wcmUseMT;
> int wcmMTChannel;
> + int wcmPrevChannel;
> int wcmEventCnt;
> struct input_event wcmEvents[MAX_USB_EVENTS];
> } wcmUSBData;
> @@ -582,7 +583,7 @@ static int usbChooseChannel(WacomCommonPtr common)
> * and all other button presses to PAD. Hardcode PAD
> * channel here.
> */
> - private->wcmBTNChannel = MAX_CHANNELS-1;
> + private->wcmBTNChannel = PAD_CHANNEL;
> }
> else if (common->wcmProtocolLevel == WCM_PROTOCOL_4)
> {
> @@ -610,7 +611,7 @@ static int usbChooseChannel(WacomCommonPtr common)
> * pad devices.
> */
> if (serial == 0xf0)
> - channel = MAX_CHANNELS-1;
> + channel = PAD_CHANNEL;
> else if (serial)
> channel = serial-1;
> else
> @@ -912,6 +913,12 @@ static int usbParseAbsMTEvent(WacomCommonPtr common,
> struct input_event *event)
> dsnew = &common->
> wcmChannel[private->wcmMTChannel].work;
>
> + /* Set tool specific data here. Since
> + * MT slots have fixed mapping to channel,
> + * the channel will always have previous
> + * tools values. Since MT event filtering
> + * is also per slot, this works as expected.
> + */
> dsnew->device_type = TOUCH_ID;
> dsnew->device_id = TOUCH_DEVICE_ID;
> dsnew->serial_num = event->value+1;
> @@ -975,15 +982,9 @@ static int usbParseKeyEvent(WacomCommonPtr common,
> int change = 1;
>
> /* BTN_TOOL_* are sent to indicate when a specific tool is going
> - * in our out of proximity. When going out of proximity, ds
> - * is initialized to zeros elsewere. When going in proximity,
> - * here we initialize tool specific values.
> - *
> - * This requires tools that map to same channel of an input
> - * device and that share events (ABS_X of PEN and ERASER for
> - * example) not to be in proximity at the same time. Tools
> - * that map to different channels can be in proximity at same
> - * time with no confusion.
> + * in our out of proximity. When going in proximity, here we
> + * initialize tool specific values. Making sure shared values
> + * are correct values during tool change is done elsewhere.
> */
> switch (event->code)
> {
> @@ -1170,7 +1171,6 @@ static void usbDispatchEvents(InputInfoPtr pInfo)
> WacomCommonPtr common = priv->common;
> int channel;
> int channel_change = 0, btn_channel_change = 0, mt_channel_change = 0;
> - WacomChannelPtr pChannel;
> WacomDeviceState dslast;
> wcmUSBData* private = common->private;
>
> @@ -1184,65 +1184,49 @@ static void usbDispatchEvents(InputInfoPtr pInfo)
> return;
> }
>
> - pChannel = common->wcmChannel + channel;
> - dslast = pChannel->valid.state;
> -
> - /* Because of linux input filtering, the kernel driver can not
> - * always force total set of event data when new tool comes into
> - * proximity. This includes simple case of flipping stylus
> - * from pen to eraser tool. Therefore, when new tool is in-prox
> - * we must initialize all shared event values to same as previous
> - * tool to account for filtered events.
> - *
> - * For Generic and Protocol 4 devices that have fixed channel
> - * mappings, this is no problem. Protocol 5 devices are difficult
> - * because they dynamically assign channel #'s and even simple
> - * case above can switch from channel 1 to channel 0.
> - *
> - * To simplify things, we take advantage of fact wacom kernel
> - * drivers force all values to zero when going out of proximity so
> - * we take a short cut and memset() to align when going in-prox
> - * instead of a memcpy().
> - *
> - * TODO: Some non-wacom tablets send X/Y data right before coming
> - * in proximity. The following discards that data.
> - * Adding "&& dslast.proximimty" to check would probably help
> - * this case.
> - * Some non-wacom tablets may also never reset their values
> - * to zero when out-of-prox. The memset() can loss this data.
> - * Adding a !WCM_PROTOCOL_GENERIC check would probably help this case.
> - */
> - if (!common->wcmChannel[channel].work.proximity)
> + if (common->wcmProtocolLevel != WCM_PROTOCOL_5)
tbh, I prefer if you'd reverse this conditions. especially for complex
conditions that require more brain-space, having
if (foo)
bar;
else
foobar;
seems easier to read, as "everything else" falls into the "else" else
branch. this condition here is the semantic equivalent of
if (everything else)
foobar;
else if (specific case)
bar;
> {
> - memset(&common->wcmChannel[channel],0,sizeof(WacomChannel));
> -
> - /* in case the in-prox event was missing */
> - /* TODO: There are not valid times when in-prox
> - * events are not sent by a driver except:
> - *
> - * 1) Starting X while tool is already in prox.
> - * 2) Non-wacom tablet sends only BTN_TOUCH without
> - * BTN_TOOL_PEN since it only support 1 tool.
> + /* Because of linux input filtering, each switch to a new
> + * tool is required to have its initial values match values
> + * of previous tool.
> *
> - * Case 1) should be handled in same location as
> - * below check of (ds->device_type == 0) since its
> - * same reason. It is better to query for real
> - * value instead of assuming in-prox.
> - * Case 2) should be handled in case statement that
> - * processes BTN_TOUCH for WCM_PROTOCOL_GENERIC devices.
> + * For normal case, all tools are in channel 0 and so
> + * no issue. Protocol 4 2FGT devices split between
> + * two channels though and so need to copy data between
> + * channels to prevent loss of events; which could
> + * lead to cursor jumps.
> *
> - * So we should not be forcing to in-prox here because
> - * it could cause cursor jump from (X,Y)=(0,0) if events
> - * are sent while out-of-prox; which can happen only
> - * with WCM_PROTOCOL_GENERIC devices. Hint: see TODO above.
> + * PAD device is special. It shares no events
> + * with other channels and is always in proximity.
> + * So it requires no copying of data from other
> + * channels.
> */
> - common->wcmChannel[channel].work.proximity = 1;
> + if (private->wcmPrevChannel != channel &&
> + channel != PAD_CHANNEL &&
> + private->wcmPrevChannel != PAD_CHANNEL)
> + {
> + common->wcmChannel[channel].work =
> +
> common->wcmChannel[private->wcmPrevChannel].work;
> + private->wcmPrevChannel = channel;
> + }
> }
> + /* Protocol 5 devices have some complications related to DUALINPUT
> + * support and can not use above logic to recover from input
> + * event filtering. Instead, just deal with occasional dropped
> + * event. Since tools are dynamically assigned a channel #, the
> + * structure must be initialiized to known starting values
^ typo
> + * when first entering proximity to discard invalid data.
> + */
> + else if (!common->wcmChannel[channel].work.proximity)
> + memset(&common->wcmChannel[channel],0,sizeof(WacomChannel));
>
> - /* all USB data operates from previous context except relative values*/
> ds = &common->wcmChannel[channel].work;
> + dslast = common->wcmChannel[channel].valid.state;
> +
> + /* all USB data operates from previous context except relative values*/
> ds->relwheel = 0;
> ds->serial_num = private->wcmLastToolSerial;
> +
> /* For protocol 4 and 5 devices, ds == btn_ds. */
> btn_ds = &common->wcmChannel[private->wcmBTNChannel].work;
>
> @@ -1306,6 +1290,7 @@ static void usbDispatchEvents(InputInfoPtr pInfo)
> if (ISBITSET(keys, wcmTypeToKey[i].tool_key))
> {
> ds->device_type = wcmTypeToKey[i].device_type;
> + ds->proximity = 1;
> break;
> }
> }
> diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> index e621f78..4e6b5f9 100644
> --- a/src/xf86WacomDefs.h
> +++ b/src/xf86WacomDefs.h
> @@ -392,6 +392,7 @@ extern WacomDeviceClass gWacomISDV4Device;
> #define RAW_FILTERING_FLAG 4
>
> #define MAX_CHANNELS 3
> +#define PAD_CHANNEL (MAX_CHANNELS-1)
> #define MAX_FINGERS 2
please split this into a separate patch.
>
> typedef struct {
> --
> 1.7.3.4
as for the rest of the code, channels are still a grey area for me so if
Ping is happy with the patch (once split up), I'll merge it.
Cheers,
Peter
------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web. Learn how to
best implement a security strategy that keeps consumers' information secure
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
