On Sun, Mar 27, 2011 at 12:51:32PM -0700, Ping Cheng wrote:
> On Sat, Mar 26, 2011 at 6:26 PM, Chris Bagwell <ch...@cnpbagwell.com> wrote:
>
> > On Thu, Mar 24, 2011 at 5:35 PM, Ping Cheng <pingli...@gmail.com> wrote:
> > > Change oldHwProx to oldCursorHwProx to better refect its use.
> > >
> > > Protocol 5 distance starts from the MaxCursorDist (256) when
> > > getting in the prox while protocol 4 distance is 0 when
> > > getting in the prox.
> > >
> > > oldCursorHwProx keeps the hardware in/out prox state so we can
> > > we set the MaxCursorDist for the next round of relative cursor
> > > movement when tool first comes in prox.
> >
> > I do appreciate the variable clarification. It wasn't obvious to me
> > in past its scope was so limited.
> >
> > >
> > > Signed-off-by: Ping Cheng <pingli...@gmail.com>
> > > ---
> > > src/wcmCommon.c | 24 ++++++++++--------------
> > > src/wcmConfig.c | 2 +-
> > > src/wcmUSB.c | 4 ----
> > > src/xf86Wacom.c | 6 ------
> > > src/xf86WacomDefs.h | 2 +-
> > > 5 files changed, 12 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/src/wcmCommon.c b/src/wcmCommon.c
> > > index 0f858eb..845cf6e 100644
> > > --- a/src/wcmCommon.c
> > > +++ b/src/wcmCommon.c
> > > @@ -31,6 +31,8 @@
> > > #define THRESHOLD_TOLERANCE (FILTER_PRESSURE_RES / 125)
> > > #define DEFAULT_THRESHOLD (FILTER_PRESSURE_RES / 75)
> > >
> > > +#define MAX_CURSOR_DISTANCE 256
> > > +
> > > /* X servers pre 1.9 didn't copy data passed into xf86Post*Event.
> > > * Data passed in would be modified, requiring the driver to copy the
> > > * data beforehand.
> > > @@ -1138,14 +1140,6 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> > > /* Device transformations come first */
> > > priv = pInfo->private;
> > >
> > > - if (IsUSBDevice(common))
> > > - {
> > > - if (IsTouch(priv) && !ds->proximity)
> > > - priv->oldHwProx = 0;
> > > - else if (IsStylus(priv) || IsEraser(priv))
> > > - priv->oldHwProx = 1;
> > > - }
> > > -
> > > /* send a touch out for USB Tablet PCs */
> > > if (IsUSBDevice(common) && !IsTouch(priv)
> > > && common->wcmTouchDefault && !priv->oldProximity)
> > > @@ -1180,19 +1174,18 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> > > filtered.pressure = applyPressureCurve(priv,&filtered);
> > > }
> > >
> > > - else if (IsCursor(priv) && !priv->oldHwProx)
> > > + else if (IsCursor(priv) && !priv->oldCursorHwProx)
> > > {
> > > /* initial current max distance for Intuos series */
> > > if ((TabletHasFeature(common, WCM_ROTATION)) ||
> > > (TabletHasFeature(common, WCM_DUALINPUT)))
> > > - common->wcmMaxCursorDist = 256;
> > > + common->wcmMaxCursorDist = MAX_CURSOR_DISTANCE;
> > > else
> > > common->wcmMaxCursorDist = 0;
> > > }
> > >
> > > - /* Store current hard prox for next use */
> > > - if (!IsTouch(priv))
> > > - priv->oldHwProx = ds->proximity;
> > > + /* Store cursor hardware prox for next use */
> > > + priv->oldCursorHwProx = ds->proximity;
> >
> > Should it be wrapped with "if (IsCursor(priv))" to prevent a pen from
> > updating it? I can't tell from diff alone if its an issue.
> >
>
> Good point. It normally should not be an issue. But better safe than sorry.
>
>
> > What ever the answer:
> >
> > Reviewed-by: Chris Bagwell <ch...@cnpbagwell.com>
> >
> > The reset are questions for my understanding.
> >
>
> Thank you for the review. I'll wait a few days for more comments. V2 will be
> sent sometime next week. My answers to your other questions are below.
>
> >
> > > /* User-requested filtering comes next */
> > >
> > > @@ -1227,14 +1220,17 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> > > /* force out-prox when distance is outside wcmCursorProxoutDist
> > for pucks */
> > > if (IsCursor(priv))
> > > {
> > > - /* force out-prox when distance is outside
> > wcmCursorProxoutDist. */
> > > if (common->wcmProtocolLevel == WCM_PROTOCOL_5)
> > > {
> > > + /* protocol 5 distance starts from the
> > MaxCursorDist
> > > + * when getting in the prox.
> > > + */
> > > if (common->wcmMaxCursorDist > filtered.distance)
> > > common->wcmMaxCursorDist =
> > filtered.distance;
> > > }
> > > else
> > > {
> > > + /* protocol 4 distance is 0 when getting in the
> > prox */
> > > if (common->wcmMaxCursorDist < filtered.distance)
> > > common->wcmMaxCursorDist =
> > filtered.distance;
> >
> > These if()'s always confused me and the comments help but what is
> > behavior of 4 vs. 5 when out of prox?
> >
>
> The difference lies in the distance we get. For P5 devices, distance is
> reported as the maximum when tool is first detect then gradually reduced to
> 0 when getting closer to the tablet. The P4 is the opposite: distance is 0
> when tool is first in prox. The values increases as the tool gets closer to
> the tablet.
can you add this to the commit message please? it was quite informative
>
> Maybe we should normalize it in wcmUSB so they all have some behavior
> > here? Not for this patch of course.
> >
>
> Yeah, that can be considered in wcmUSB.c.
yes please. ideally, all devices should behave the same when they come out
of the backend-specific code. (I know that's not always possible, by we
should aim for it).
Acked-by: Peter Hutterer <peter.hutte...@who-t.net> otherwise. hard for me
to refuse a patch that removes twice as many lines as it adds ;)
Cheers,
Peter
> > > }
> > > diff --git a/src/wcmConfig.c b/src/wcmConfig.c
> > > index 2bf0ed3..6235d3c 100644
> > > --- a/src/wcmConfig.c
> > > +++ b/src/wcmConfig.c
> > > @@ -62,7 +62,7 @@ static int wcmAllocate(InputInfoPtr pInfo)
> > > priv->next = NULL;
> > > priv->pInfo = pInfo;
> > > priv->common = common; /* common info pointer */
> > > - priv->oldHwProx = 1; /* previous hardware proximity */
> > > + priv->oldCursorHwProx = 0; /* previous cursor hardware
> > proximity */
> > > priv->nPressCtrl [0] = 0; /* pressure curve x0 */
> > > priv->nPressCtrl [1] = 0; /* pressure curve y0 */
> > > priv->nPressCtrl [2] = 100; /* pressure curve x1 */
> > > diff --git a/src/wcmUSB.c b/src/wcmUSB.c
> > > index 5f0dbe2..e65f24f 100644
> > > --- a/src/wcmUSB.c
> > > +++ b/src/wcmUSB.c
> > > @@ -394,10 +394,6 @@ static void usbInitProtocol5(WacomCommonPtr common,
> > const char* id,
> > >
> > > /* tilt enabled */
> > > common->wcmFlags |= TILT_ENABLED_FLAG;
> > > -
> > > - /* reinitialize max here since 0 is for Graphire series */
> > > - common->wcmMaxCursorDist = 256;
> > > -
> > > }
> > >
> > > static void usbInitProtocol4(WacomCommonPtr common, const char* id,
> > > diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
> > > index 6762a3d..a124ed3 100644
> > > --- a/src/xf86Wacom.c
> > > +++ b/src/xf86Wacom.c
> > > @@ -442,12 +442,6 @@ static int wcmDevInit(DeviceIntPtr pWcm)
> > >
> > > wcmRotateTablet(pInfo, common->wcmRotate);
> > >
> > > - if (IsTouch(priv))
> > > - {
> > > - /* hard prox out */
> > > - priv->oldHwProx = 0;
> > > - }
> > > -
> > > InitWcmDeviceProperties(pInfo);
> > > XIRegisterPropertyHandler(pInfo->dev, wcmSetProperty, NULL,
> > wcmDeleteProperty);
> > >
> > > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> > > index 5f5d969..0e57087 100644
> > > --- a/src/xf86WacomDefs.h
> > > +++ b/src/xf86WacomDefs.h
> > > @@ -276,7 +276,7 @@ struct _WacomDeviceRec
> > > int oldThrottle; /* previous throttle value */
> > > int oldButtons; /* previous buttons state */
> > > int oldProximity; /* previous proximity */
> > > - int oldHwProx; /* previous hardware proximity */
> > > + int oldCursorHwProx; /* previous cursor hardware proximity */
> > > int old_device_id; /* last in prox device id */
> > > int old_serial; /* last in prox tool serial number */
> > > int devReverseCount; /* Relative ReverseConvert called twice
> > each movement*/
> > > --
> > > 1.7.4
> > >
> > >
> > >
> > ------------------------------------------------------------------------------
> > > Enable your software for Intel(R) Active Management Technology to meet
> > the
> > > growing manageability and security demands of your customers. Businesses
> > > are taking advantage of Intel(R) vPro (TM) technology - will your
> > software
> > > be a part of the solution? Download the Intel(R) Manageability Checker
> > > today! http://p.sf.net/sfu/intel-dev2devmar
> > > _______________________________________________
> > > Linuxwacom-devel mailing list
> > > Linuxwacom-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
> > >
> >
> ------------------------------------------------------------------------------
> Enable your software for Intel(R) Active Management Technology to meet the
> growing manageability and security demands of your customers. Businesses
> are taking advantage of Intel(R) vPro (TM) technology - will your software
> be a part of the solution? Download the Intel(R) Manageability Checker
> today! http://p.sf.net/sfu/intel-dev2devmar
> _______________________________________________
> Linuxwacom-devel mailing list
> Linuxwacom-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
------------------------------------------------------------------------------
Enable your software for Intel(R) Active Management Technology to meet the
growing manageability and security demands of your customers. Businesses
are taking advantage of Intel(R) vPro (TM) technology - will your software
be a part of the solution? Download the Intel(R) Manageability Checker
today! http://p.sf.net/sfu/intel-dev2devmar
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
