On Thu, Mar 03, 2011 at 06:06:01PM -0800, Ping Cheng wrote: > On Thu, Mar 3, 2011 at 4:56 PM, Peter Hutterer > <[email protected]>wrote: > > > Instead of returning rather ambiguous 0, 1 and 2, return enums with > > readable > > descriptions. > > > > No functional changes, other than that we now skip a few tests in > > wcmCheckSuppress if we already have the result. > > > > Signed-off-by: Peter Hutterer <[email protected]> > > --- > > src/wcmCommon.c | 101 > > ++++++++++++++++++++++++++++++---------------------- > > src/xf86Wacom.h | 6 +++ > > test/wacom-tests.c | 14 +++++++ > > 3 files changed, 78 insertions(+), 43 deletions(-) > > > > diff --git a/src/wcmCommon.c b/src/wcmCommon.c > > index 0a5a8ac..16d850e 100644 > > --- a/src/wcmCommon.c > > +++ b/src/wcmCommon.c > > @@ -53,8 +53,10 @@ static int *VCOPY(const int *valuators, int nvals) > > > > > > ****************************************************************************/ > > > > static int applyPressureCurve(WacomDevicePtr pDev, const > > WacomDeviceStatePtr pState); > > -static void commonDispatchDevice(WacomCommonPtr common, unsigned int > > channel, > > - const WacomChannelPtr pChannel, int suppress); > > +static void commonDispatchDevice(WacomCommonPtr common, > > + unsigned int channel, > > + const WacomChannelPtr pChannel, > > + enum WacomSuppressMode suppress); > > static void sendAButton(InputInfoPtr pInfo, int button, int mask, > > int first_val, int num_vals, int *valuators); > > > > @@ -766,50 +768,65 @@ void wcmSendEvents(InputInfoPtr pInfo, const > > WacomDeviceState* ds) > > } > > } > > > > > > -/***************************************************************************** > > - * wcmCheckSuppress -- > > - * Determine whether device state has changed enough - return 0 > > - * if not. > > - > > ****************************************************************************/ > > - > > -static int wcmCheckSuppress(WacomCommonPtr common, > > - const WacomDeviceState* dsOrig, > > - WacomDeviceState* dsNew) > > +/** > > + * Determine whether device state has changed enough to warrant further > > + * processing. The driver's "suppress" setting decides how much > > + * movement/state change must occur before we process events to avoid > > + * overloading the server with minimal changes (and getting fuzzy events). > > + * wcmCheckSuppress ensures that events meet this standard. > > + * > > + * @param dsOrig Previous device state > > + * @param dsNew Current device state > > + * > > + * @retval SUPPRESS_ALL Ignore this event completely. > > + * @retval SUPPRESS_NONE Process event normally. > > + * @retval SUPPRESS_NON_MOTION Suppress all data but motion data. > > + */ > > +static enum WacomSuppressMode > > +wcmCheckSuppress(WacomCommonPtr common, > > + const WacomDeviceState* dsOrig, > > + WacomDeviceState* dsNew) > > { > > int suppress = common->wcmSuppress; > > - /* NOTE: Suppression value of zero disables suppression. */ > > - int returnV = 0; > > + enum WacomSuppressMode returnV = SUPPRESS_NONE; > > > > /* Ignore all other changes that occur after initial out-of-prox. */ > > if (!dsNew->proximity && !dsOrig->proximity) > > - return 0; > > + return SUPPRESS_ALL; > > > > /* Never ignore proximity changes. */ > > - if (dsOrig->proximity != dsNew->proximity) returnV = 1; > > - > > - if (dsOrig->buttons != dsNew->buttons) returnV = 1; > > - if (dsOrig->stripx != dsNew->stripx) returnV = 1; > > - if (dsOrig->stripy != dsNew->stripy) returnV = 1; > > - if (abs(dsOrig->tiltx - dsNew->tiltx) > suppress) returnV = 1; > > - if (abs(dsOrig->tilty - dsNew->tilty) > suppress) returnV = 1; > > - if (abs(dsOrig->pressure - dsNew->pressure) > suppress) returnV = > > 1; > > - if (abs(dsOrig->capacity - dsNew->capacity) > suppress) returnV = > > 1; > > - if (abs(dsOrig->throttle - dsNew->throttle) > suppress) returnV = > > 1; > > + if (dsOrig->proximity != dsNew->proximity) goto out; > > + > > + if (dsOrig->buttons != dsNew->buttons) goto out; > > + if (dsOrig->stripx != dsNew->stripx) goto out; > > + if (dsOrig->stripy != dsNew->stripy) goto out; > > + if (abs(dsOrig->tiltx - dsNew->tiltx) > suppress) goto out; > > + if (abs(dsOrig->tilty - dsNew->tilty) > suppress) goto out; > > + if (abs(dsOrig->pressure - dsNew->pressure) > suppress) goto out; > > + if (abs(dsOrig->capacity - dsNew->capacity) > suppress) goto out; > > + if (abs(dsOrig->throttle - dsNew->throttle) > suppress) goto out; > > if (abs(dsOrig->rotation - dsNew->rotation) > suppress && > > - (1800 - abs(dsOrig->rotation - dsNew->rotation)) > > > suppress) returnV = 1; > > + (1800 - abs(dsOrig->rotation - dsNew->rotation)) > suppress) > > goto out; > > > > /* look for change in absolute wheel position > > * or any relative wheel movement > > */ > > if ((abs(dsOrig->abswheel - dsNew->abswheel) > suppress) > > - || (dsNew->relwheel != 0)) returnV = 1; > > + || (dsNew->relwheel != 0)) > > + goto out; > > > > - /* cursor moves or not? */ > > + returnV = SUPPRESS_ALL; > > + > > +out: > > + /* Special handling for cursor: if nothing else changed but the > > + * pointer x/y, suppress all but cursor movement. This return value > > + * is used in commonDispatchDevice to short-cut event processing. > > + */ > > if ((abs(dsOrig->x - dsNew->x) > suppress) || > > (abs(dsOrig->y - dsNew->y) > suppress)) > > { > > - if (!returnV) /* need to check if cursor moves or not */ > > - returnV = 2; > > + if (returnV == SUPPRESS_ALL) > > + returnV = SUPPRESS_NON_MOTION; > > } > > else /* don't move cursor */ > > { > > @@ -833,7 +850,7 @@ void wcmEvent(WacomCommonPtr common, unsigned int > > channel, > > WacomDeviceState* pLast; > > WacomDeviceState ds; > > WacomChannelPtr pChannel; > > - int suppress = 0; > > + enum WacomSuppressMode suppress; > > WacomDevicePtr priv = common->wcmDevices; > > pChannel = common->wcmChannel + channel; > > pLast = &pChannel->valid.state; > > @@ -895,12 +912,10 @@ void wcmEvent(WacomCommonPtr common, unsigned int > > channel, > > common->wcmModel->FilterRaw(common,pChannel,&ds); > > } > > > > - /* Discard unwanted data */ > > + /* skip event if we don't have enough movement */ > > > > Maybe you want to add this info to the comments: Suppress is not aimed at > skipping the event due to no movement. The skipping part was a minor > feature. The main purpose was to discard small (x,y) movement (less than > wcmSuppress) to avoid jitter when user holding the tool on the tablet. > > > > suppress = wcmCheckSuppress(common, pLast, &ds); > > - if (!suppress) > > - { > > + if (suppress == SUPPRESS_ALL) > > return; > > - } > > > > /* JEJ - Do not move this code without discussing it with me. > > * The device state is invariant of any filtering performed below. > > @@ -1148,7 +1163,8 @@ setPressureButton(const WacomDevicePtr priv, const > > WacomDeviceState *ds) > > } > > > > static void commonDispatchDevice(WacomCommonPtr common, unsigned int > > channel, > > - const WacomChannelPtr pChannel, Bool > > suppress) > > + const WacomChannelPtr pChannel, > > + enum WacomSuppressMode suppress) > > { > > InputInfoPtr pInfo = NULL; > > WacomToolPtr tool = NULL; > > @@ -1260,20 +1276,19 @@ static void commonDispatchDevice(WacomCommonPtr > > common, unsigned int channel, > > deltx *= priv->factorX; > > delty *= priv->factorY; > > > > + /* less than one device coordinate movement? */ > > > > Sorry to bother you again. I am still trying to figure out how this check > works. I must have missed the concept. What is a device coordinate? Tool > (x,y), I guess. How do we get a less than one device coordinate movement? I > thought it is the screen movement that we cares about, not the tool > movement. A device coordinate > change that causes a deltx/y >=1 may not lead to a cursor movement. > > If the if-statement doesn't avoid discarding non-cursor movement events due > to the factor that we do not have enough information to make the decision > (lack of screen size/resolution, etc), removing this block of code might be > a better option. > > I need to understand the concept first though. Please shed light on me. >
the coordinates we deal with in input drivers are always device coordinates, i.e. the range the device announces. in wacom's case that's 0 - 50000-ish. ignore screen coordinates, they are the realm of the server only. so if the device gives you x = 30000 and then x = 30001, you've had movement by 1 device coordinate. that doesn't necessarily translate into a movement of 1 pixel on the screen though (unlikely, unless your screen res is _really_ high*). we can't have less than one device coordinate from the driver, but in this block the factor comes into play. if the factor is less than 1, you may get less than one device coordinate movement. (Note that I don't think factorX/Y is currently working anyway, it was hooked to KeepShape, but remainders are still there.) having said all that, the current suppress default of 2 is rather low anyway. given wacom's resolutions, that's what, a fraction of a mm? so this code path is IMO quite unnecssary anyway but in this patch i just focused on making it easier to understand, not fixing/removing it. there's still a valid reason for having suppress (in fact we just added a similar feature to evdev) but mixing all coordinate ranges into a single value seems suboptimal. e.g. fuzz for x/y should be greater than fuzz for pressure, since the range is 10 times larger. Cheers, Peter * coincidentally, because X is limited to 16 bit signed coordinates, well, you can't. ------------------------------------------------------------------------------ What You Don't Know About Data Connectivity CAN Hurt You This paper provides an overview of data connectivity, details its effect on application quality, and explores various alternative solutions. http://p.sf.net/sfu/progress-d2d _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
