On Thu, Mar 3, 2011 at 6:35 PM, Peter Hutterer <peter.hutte...@who-t.net>wrote:
> On Thu, Mar 03, 2011 at 06:06:01PM -0800, Ping Cheng wrote:
> > On Thu, Mar 3, 2011 at 4:56 PM, Peter Hutterer <peter.hutte...@who-t.net
> >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 <peter.hutte...@who-t.net>
> > > ---
> > > 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.
Different fuzz/suppress for (x,y) and others is a good point. Do you mind to
add it into your commit comment or the code? At least to remind us coming
back to it when we have time.
With all three commented patches updated, the whole set is
Reviewed-by: Ping Cheng <pingli...@gmail.com>
Ping
------------------------------------------------------------------------------
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
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
