On Mon, Mar 17, 2014 at 12:11:15PM -0700, Jason Gerecke wrote:
> On Tue, Mar 11, 2014 at 2:10 PM, Egbert Eich <e...@freedesktop.org> wrote:
> > From: Egbert Eich <e...@suse.com>
> >
> > Worn out devices send a non-zero pressure even when not in contact
> > with the tablet. To compensate for this the driver detects if the
> > pressure sent by the device immediately after indicating proximity
> > is non-zero. It substracts this value from any pressure value sent
> > later and rescales the pressure range to the full device range.
> > If it later on sees the pressure value fall below this inital value
> > it will readjust it this lower value.
> > The downside of this is that when the pen is pushed onto the tablet
> > really fast the initial pressure reading may be non-zero also the
> > pen isn't worn. This can lead to lost events.
> > This patch tries to address this:
> > If the first pressure reading is != 0 it is recorded. If the recorded
> > maximum value is >0 but a later pressure reading is higher the maximum
> > value is replaced . If no button press event is generated the 'normal'
> > way it is checked of the recorded maximum would trigger one when
> > minPressure decreases.
> > Once a 'normal' button event is generated or minPressure doesn't
> > decrease any more the recorded maximum is set to 0 which will disable
> > the checks until the next prox in.
> >
> > Signed-off-by: Egbert Eich <e...@suse.com>
> > ---
>
> Looking good -- the logic seems sound, though there are two questions
> I have (and one nitpick)
>
> > v2: After a 'regular' button event occured don't consider MaxRawPressure
> > until next prox in.
> > Don't consider MaxRawPressure any more when it has triggered a button
> > event but minPressure
> > doesn't drop any more.
> >
> > src/wcmCommon.c | 68
> > +++++++++++++++++++++++++++++++++++++++++++++--------
> > src/xf86Wacom.h | 2 +-
> > src/xf86WacomDefs.h | 2 ++
> > test/wacom-tests.c | 11 ++++-----
> > 4 files changed, 65 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/wcmCommon.c b/src/wcmCommon.c
> > index afc09f9..b39be08 100644
> > --- a/src/wcmCommon.c
> > +++ b/src/wcmCommon.c
> > @@ -1066,11 +1066,11 @@ rebasePressure(const WacomDevicePtr priv, const
> > WacomDeviceState *ds)
> > * @see rebasePressure
> > */
> > TEST_NON_STATIC int
> > -normalizePressure(const WacomDevicePtr priv, const WacomDeviceState *ds)
> > +normalizePressure(const WacomDevicePtr priv, const int raw_pressure)
> > {
> > WacomCommonPtr common = priv->common;
> > double pressure;
> > - int p = ds->pressure;
> > + int p = raw_pressure;
> > int range_left;
> >
> > /* normalize pressure to 0..FILTER_PRESSURE_RES */
> > @@ -1094,16 +1094,16 @@ normalizePressure(const WacomDevicePtr priv, const
> > WacomDeviceState *ds)
> > * Returns the state of all buttons, but buttons other than button 1 are
> > * unmodified.
> > */
> > +#define PRESSURE_BUTTON 1
> > static int
> > -setPressureButton(const WacomDevicePtr priv, const WacomDeviceState *ds)
> > +setPressureButton(const WacomDevicePtr priv, int buttons, const int
> > pressure)
> > {
> > WacomCommonPtr common = priv->common;
> > - int button = 1;
> > - int buttons = ds->buttons;
> > + int button = PRESSURE_BUTTON;
> >
> > /* button 1 Threshold test */
> > /* set button1 (left click) on/off */
> > - if (ds->pressure < common->wcmThreshold)
> > + if (pressure < common->wcmThreshold)
> > {
> > buttons &= ~button;
> > if (priv->oldButtons & button) /* left click was on */
> > @@ -1111,7 +1111,7 @@ setPressureButton(const WacomDevicePtr priv, const
> > WacomDeviceState *ds)
> > /* don't set it off if it is within the tolerance
> > and threshold is larger than the tolerance */
> > if ((common->wcmThreshold > THRESHOLD_TOLERANCE) &&
> > - (ds->pressure > common->wcmThreshold -
> > THRESHOLD_TOLERANCE))
> > + (pressure > common->wcmThreshold -
> > THRESHOLD_TOLERANCE))
> > buttons |= button;
> > }
> > }
> > @@ -1169,6 +1169,7 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> > WacomDeviceState* ds = &pChannel->valid.states[0];
> > WacomDevicePtr priv = NULL;
> > WacomDeviceState filtered;
> > + int raw_pressure = 0;
> >
> > /* device_type should have been retrieved and set in the respective
> > * models, wcmISDV4.c or wcmUSB.c. Once it comes here, something
> > @@ -1242,11 +1243,58 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> >
> > if ((IsPen(priv) || IsTouch(priv)) && common->wcmMaxZ)
> > {
> > + int prev_min_pressure = priv->oldProximity ?
> > priv->minPressure : 0;
> > +
> > detectPressureIssue(priv, common, &filtered);
> > +
> > + raw_pressure = filtered.pressure;
> > + if (!priv->oldProximity)
> > + priv->maxRawPressure = raw_pressure;
> > +
> > priv->minPressure = rebasePressure(priv, &filtered);
> > - filtered.pressure = normalizePressure(priv, &filtered);
> > - if (IsPen(priv))
> > - filtered.buttons = setPressureButton(priv,
> > &filtered);
> > +
> > + filtered.pressure = normalizePressure(priv,
> > filtered.pressure);
> > + if (IsPen(priv)) {
> > + filtered.buttons = setPressureButton(priv,
> > +
> > filtered.buttons,
> > +
> > filtered.pressure);
> > +
> > + /* Here we run a heustic to avoid the loss of
> > button events if the
> Nit: s/heustic/heuristic/
>
> > + * pen gets pushed onto the tablet so quickly that
> > the first pressure
> > + * event read is non-zero and is thus interpreted
> > as a pressure bias */
> > + if (filtered.buttons & PRESSURE_BUTTON) {
> > + /* If we triggered 'normally' reset max
> > pressure to
> > + * avoid to trigger again while this device
> > is in proximity */
> > + priv->maxRawPressure = 0;
> > + } else if (priv->maxRawPressure) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> see below.
> > + /* If we haven't triggered normally we
> > record the maximal pressure
> > + * and see if this would have triggered
> > with a lowered bias. */
> > + if (priv->maxRawPressure < raw_pressure &&
> > + priv->prevRawPressure <= raw_pressure )
> > + priv->maxRawPressure = raw_pressure;
> > + if (priv->maxRawPressure) {
>
> This check appears to be unnecessary? priv->maxRawPressure was
> non-zero upon entering this block, and the 'if' statement immediately
> above will never decrease its value.
Yes, it was a left over from a previous version.
This is basically because we now only test for one condition in the line
marked above.
[..]
> > + }
> > + /* We need to record the pressure for the next time
> > we come here to make
> > + * sure we only record maxRawPressure on an
> > inclining slope. We cannot use
> > + * maxRawPressure as this may have been reset */
> > + priv->prevRawPressure = raw_pressure;
>
> If maxRawPressure is zero for any reason, the block of code that bumps
> up its value will never be called. There should be no need for
> prevRawPressure from what I understand.
Same as above.
Cheers,
Egbert.
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
