[PATCH synaptics 2/2] Force SLOTSTATE_EMPTY on DeviceOff

[Peter Hutterer]

On Thu, May 03, 2012 at 12:47:20PM +0200, Nicola Soranzo wrote:
> On Tue, May 01, 2012 at 08:25:25AM -0700, Chase Douglas wrote:
> > On 04/30/2012 06:19 PM, Peter Hutterer wrote:
> > > On Mon, Apr 30, 2012 at 10:45:30AM -0700, Chase Douglas wrote:
> > >> On 04/29/2012 08:21 PM, Peter Hutterer wrote:
> > >>> SLOTSTATE_OPEN_EMPTY on resume leads to erroneously detected
> touches.
> > >>>
> > >>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > >>> ---
> > >>>  src/eventcomm.c |    2 +-
> > >>>  src/synaptics.c |    6 +++---
> > >>>  src/synproto.c  |    6 +++---
> > >>>  src/synproto.h  |    2 +-
> > >>>  4 files changed, 8 insertions(+), 8 deletions(-)
> > >>>
> > >>> diff --git a/src/eventcomm.c b/src/eventcomm.c
> > >>> index 741f988..4ef8ad6 100644
> > >>> --- a/src/eventcomm.c
> > >>> +++ b/src/eventcomm.c
> > >>> @@ -662,7 +662,7 @@ EventReadHwState(InputInfoPtr pInfo,
> > >>>      SynapticsParameters *para = &priv->synpara;
> > >>>      struct eventcomm_proto_data *proto_data = priv->proto_data;
> > >>>  
> > >>> -    SynapticsResetTouchHwState(hw);
> > >>> +    SynapticsResetTouchHwState(hw, FALSE);
> > >>>  
> > >>>      /* Reset cumulative values if buttons were not previously
> pressed */
> > >>>      if (!hw->left && !hw->right && !hw->middle)
> > >>> diff --git a/src/synaptics.c b/src/synaptics.c
> > >>> index 935650d..6dc8004 100644
> > >>> --- a/src/synaptics.c
> > >>> +++ b/src/synaptics.c
> > >>> @@ -1619,7 +1619,7 @@ timerFunc(OsTimerPtr timer, CARD32 now,
> pointer arg)
> > >>>  
> > >>>      priv->hwState->millis += now - priv->timer_time;
> > >>>      SynapticsCopyHwState(hw, priv->hwState);
> > >>> -    SynapticsResetTouchHwState(hw);
> > >>> +    SynapticsResetTouchHwState(hw, FALSE);
> > >>>      delay = HandleState(pInfo, hw, hw->millis, TRUE);
> > >>>  
> > >>>      priv->timer_time = now;
> > >>> @@ -1659,7 +1659,7 @@ ReadInput(InputInfoPtr pInfo)
> > >>>      int delay = 0;
> > >>>      Bool newDelay = FALSE;
> > >>>  
> > >>> -    SynapticsResetTouchHwState(hw);
> > >>> +    SynapticsResetTouchHwState(hw, FALSE);
> > >>>  
> > >>>      while (SynapticsGetHwState(pInfo, priv, hw)) {
> > >>>  /* Semi-mt device touch slots do not track touches. When there is
> a
> > >>> @@ -3017,7 +3017,7 @@ UpdateTouchState(InputInfoPtr pInfo, struct
> SynapticsHwState *hw)
> > >>>          }
> > >>>      }
> > >>>  
> > >>> -    SynapticsResetTouchHwState(hw);
> > >>> +    SynapticsResetTouchHwState(hw, FALSE);
> > >>>  #endif
> > >>>  }
> > >>>  
> > >>> diff --git a/src/synproto.c b/src/synproto.c
> > >>> index cf54c4d..e7534e2 100644
> > >>> --- a/src/synproto.c
> > >>> +++ b/src/synproto.c
> > >>> @@ -153,11 +153,11 @@ SynapticsResetHwState(struct
> SynapticsHwState *hw)
> > >>>      hw->middle = 0;
> > >>>      memset(hw->multi, 0, sizeof(hw->multi));
> > >>>  
> > >>> -    SynapticsResetTouchHwState(hw);
> > >>> +    SynapticsResetTouchHwState(hw, TRUE);
> > >>>  }
> > >>>  
> > >>>  void
> > >>> -SynapticsResetTouchHwState(struct SynapticsHwState *hw)
> > >>> +SynapticsResetTouchHwState(struct SynapticsHwState *hw, Bool
> force_empty)
> > >>>  {
> > >>>  #ifdef HAVE_MULTITOUCH
> > >>>      int i;
> > >>> @@ -175,7 +175,7 @@ SynapticsResetTouchHwState(struct
> SynapticsHwState *hw)
> > >>>              case SLOTSTATE_OPEN:
> > >>>              case SLOTSTATE_OPEN_EMPTY:
> > >>>              case SLOTSTATE_UPDATE:
> > >>> -                hw->slot_state[i] = SLOTSTATE_OPEN_EMPTY;
> > >>> +                hw->slot_state[i] = force_empty ?
> SLOTSTATE_EMPTY : SLOTSTATE_OPEN_EMPTY;
> > >>>                  break;
> > >>>  
> > >>>              default:
> > >>> diff --git a/src/synproto.h b/src/synproto.h
> > >>> index 7f80bcd..413579d 100644
> > >>> --- a/src/synproto.h
> > >>> +++ b/src/synproto.h
> > >>> @@ -120,7 +120,7 @@ extern void SynapticsHwStateFree(struct
> SynapticsHwState **hw);
> > >>>  extern void SynapticsCopyHwState(struct SynapticsHwState *dst,
> > >>>                                   const struct SynapticsHwState
> *src);
> > >>>  extern void SynapticsResetHwState(struct SynapticsHwState *hw);
> > >>> -extern void SynapticsResetTouchHwState(struct SynapticsHwState
> *hw);
> > >>> +extern void SynapticsResetTouchHwState(struct SynapticsHwState
> *hw, Bool force_empty);
> > >>>  
> > >>>  extern Bool SynapticsIsSoftButtonAreasValid(int *values);
> > >>>
> > >>
> > >> This looks functionally correct, but I don't love the extra
> force_empty
> > >> parameter. There's only one case where we want to force empty
> slots.
> > >> What if instead we make this a one-line change in
> SynapticsResetHwState
> > >> after the call to SynapticsResetTouchHwState:
> > >>
> > >> /* Force all touches to the empty, closed state */
> > >> memset(hw->slot_state, 0, hw->num_mt_mask * sizeof(enum
> SynapticsSlotState);
> > > 
> > > I'm not a fan of this. If you reset, you shouldn't need code outside
> the
> > > caller to re-set some arrays again. We already have a
> reset_hw_state() that
> > > does something else again. (eventually) having one function that
> resets the
> > > struct is a good thing, even if the type of reset is controlled by
> some
> > > parameters.
> > 
> > That's a fair point. I'm still not crazy about "force_empty" as it
> > sounds like a hack due to "force" and it's not clear what are we
> > emptying. Maybe rename it to "close_touches" or "end_touches"?
> > 
> > It's bike shedding though, so either way:
> > 
> > Reviewed-by: Chase Douglas <chase.douglas at canonical.com>
> 
> renamed to set_slot_empty. that's descriptive enough, I suppose :)
> 
> Hi Peter,
> I think that in the patch committed to 
> 
> http://cgit.freedesktop.org/xorg/driver/xf86-input-synaptics/commit/?id=d13e83b921a398b9472b07874cf5061c8a0ea6a6
> 
> it would be better to rename "force_empty" to "sel_slot_empty" also in
> src/synproto.h .

oops, thanks. fixed locally, will be upstream with the next push.

Cheers,
  Peter