[PATCH synaptics 2/2] Force SLOTSTATE_EMPTY on DeviceOff

[Chase Douglas]

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>

-- Chase