... and this is the complete link: https://marc.info/?l=openbsd-tech&m=165006255922269&w=2
On 5/3/22 10:03, Ulf Brosziewski wrote: > The implementation of the tapping mechanism in wsmouse(4) has a bug > concerning the transitions into the hold/drag state, see > https://marc.info/... > for details. The patch proposed in that message is obsolete. I've > been made aware that there is another problem, the transition only > works with left-button taps. > > This patch merges tap detection and the handling of hold/drag states, > which is a cleaner and more generic approach. It fixes the problems > mentioned above, and it permits a better synchronization of button > states when tap inputs and the use of hardware buttons are mixed. > > OK? > > > Index: dev/wscons/wstpad.c > =================================================================== > RCS file: /cvs/src/sys/dev/wscons/wstpad.c,v > retrieving revision 1.30 > diff -u -p -r1.30 wstpad.c > --- dev/wscons/wstpad.c 24 Mar 2021 18:28:24 -0000 1.30 > +++ dev/wscons/wstpad.c 2 May 2022 22:17:57 -0000 > @@ -82,18 +82,17 @@ enum tap_state { > TAP_DETECT, > TAP_IGNORE, > TAP_LIFTED, > - TAP_2ND_TOUCH, > TAP_LOCKED, > - TAP_NTH_TOUCH, > + TAP_LOCKED_DRAG, > }; > > enum tpad_cmd { > CLEAR_MOTION_DELTAS, > SOFTBUTTON_DOWN, > SOFTBUTTON_UP, > + TAPBUTTON_SYNC, > TAPBUTTON_DOWN, > TAPBUTTON_UP, > - TAPBUTTON_DOUBLECLK, > VSCROLL, > HSCROLL, > }; > @@ -212,8 +211,10 @@ struct wstpad { > struct { > enum tap_state state; > int contacts; > - int centered; > + int valid; > + u_int pending; > u_int button; > + int masked; > int maxdist; > struct timeout to; > /* parameters: */ > @@ -651,6 +652,7 @@ wstpad_softbuttons(struct wsmouseinput * > } > } > > +/* Check whether the duration of t is within the tap limit. */ > int > wstpad_is_tap(struct wstpad *tp, struct tpad_touch *t) > { > @@ -675,7 +677,7 @@ wstpad_tap_filter(struct wstpad *tp, str > dy = abs(t->y - t->orig.y) * tp->ratio; > dist = (dx >= dy ? dx + 3 * dy / 8 : dy + 3 * dx / 8); > } > - tp->tap.centered = (CENTERED(t) && dist <= (tp->tap.maxdist << 12)); > + tp->tap.valid = (CENTERED(t) && dist <= (tp->tap.maxdist << 12)); > } > > > @@ -694,7 +696,7 @@ wstpad_tap_touch(struct wsmouseinput *in > lifted = (input->mt.sync[MTS_TOUCH] & ~input->mt.touches); > FOREACHBIT(lifted, slot) { > s = &tp->tpad_touches[slot]; > - if (tp->tap.state == TAP_DETECT && !tp->tap.centered) > + if (tp->tap.state == TAP_DETECT && !tp->tap.valid) > wstpad_tap_filter(tp, s); > if (t == NULL || timespeccmp(&t->orig.time, > &s->orig.time, >)) > @@ -703,7 +705,7 @@ wstpad_tap_touch(struct wsmouseinput *in > } else { > if (tp->t->state == TOUCH_END) { > t = tp->t; > - if (tp->tap.state == TAP_DETECT && !tp->tap.centered) > + if (tp->tap.state == TAP_DETECT && !tp->tap.valid) > wstpad_tap_filter(tp, t); > } > } > @@ -711,30 +713,31 @@ wstpad_tap_touch(struct wsmouseinput *in > return (t); > } > > -/* > - * If each contact in a sequence of contacts that overlap in time > - * is a tap, a button event may be generated when the number of > - * contacts drops to zero, or to one if there is a masked touch. > - */ > -static inline int > -tap_finished(struct wstpad *tp, int nmasked) > +/* Determine the "tap button", keep track of whether a touch is masked. */ > +static inline u_int > +tap_button(struct wstpad *tp) > { > - return (tp->contacts == nmasked > - && (nmasked == 0 || !wstpad_is_tap(tp, tp->t))); > + int n = tp->tap.contacts - tp->contacts - 1; > + > + tp->tap.masked = tp->contacts; > + > + return (n >= 0 && n < TAP_BTNMAP_SIZE ? tp->tap.btnmap[n] : 0); > } > > -static inline u_int > -tap_btn(struct wstpad *tp, int nmasked) > +/* > + * In the hold/drag state, do not mask touches if no masking was involved > + * in the preceding tap gesture. > + */ > +static inline int > +tap_unmask(struct wstpad *tp) > { > - int n = tp->tap.contacts - nmasked; > - > - return (n <= TAP_BTNMAP_SIZE ? tp->tap.btnmap[n - 1] : 0); > + return ((tp->tap.button || tp->tap.pending) && tp->tap.masked == 0); > } > > /* > - * This handler supports one-, two-, and three-finger-taps, which > - * are mapped to left-button, right-button and middle-button events, > - * respectively; moreover, it supports tap-and-drag operations with > + * In the default configuration, this handler maps one-, two-, and > + * three-finger taps to left-button, right-button, and middle-button > + * events, respectively. Setting the LOCKTIME parameter enables > * "locked drags", which are finished by a timeout or a tap-to-end > * gesture. > */ > @@ -743,140 +746,111 @@ wstpad_tap(struct wsmouseinput *input, u > { > struct wstpad *tp = input->tp; > struct tpad_touch *t; > - int nmasked, err = 0; > + int contacts, is_tap, slot, err = 0; > > - if (tp->btns) { > - /* > - * Don't process tapping while hardware buttons are being > - * pressed. If the handler is not in its initial state, > - * release the "tap button". > - */ > - if (tp->tap.state > TAP_IGNORE) { > - timeout_del(&tp->tap.to); > - *cmds |= 1 << TAPBUTTON_UP; > - } > - /* > - * It might be possible to produce a click within the tap > - * timeout; ignore the current touch. > - */ > - tp->tap.state = TAP_IGNORE; > - tp->tap.contacts = 0; > - tp->tap.centered = 0; > - } > + /* Synchronize the button states, if necessary. */ > + if (input->btn.sync) > + *cmds |= 1 << TAPBUTTON_SYNC; > > /* > - * If a touch from the bottom area is masked, reduce the > - * contact counts and ignore it. > + * It is possible to produce a click within the tap timeout. > + * Wait for a new touch before generating new button events. > */ > - nmasked = (input->mt.ptr_mask ? 1 : 0); > + if (PRIMARYBTN_RELEASED(tp)) > + tp->tap.contacts = 0; > + > + /* Reset the detection state whenever a new touch starts. */ > + if (tp->contacts > tp->prev_contacts || (IS_MT(tp) && > + (input->mt.touches & input->mt.sync[MTS_TOUCH]))) { > + tp->tap.contacts = tp->contacts; > + tp->tap.valid = 0; > + } > > /* > - * Only touches in the TOUCH_END state are relevant here. > - * t is NULL if no touch has been lifted. > + * The filtered number of active touches excludes a masked > + * touch if its duration exceeds the tap limit. > */ > - t = wstpad_tap_touch(input); > + contacts = tp->contacts; > + if ((slot = ffs(input->mt.ptr_mask) - 1) >= 0 > + && !wstpad_is_tap(tp, &tp->tpad_touches[slot]) > + && !tap_unmask(tp)) { > + contacts--; > + } > > switch (tp->tap.state) { > case TAP_DETECT: > - if (tp->contacts > tp->tap.contacts) > - tp->tap.contacts = tp->contacts; > - > + /* Find the oldest touch in the TOUCH_END state. */ > + t = wstpad_tap_touch(input); > if (t) { > - if (wstpad_is_tap(tp, t)) { > - if (tap_finished(tp, nmasked)) { > - if (tp->tap.centered) { > - tp->tap.state = TAP_LIFTED; > - tp->tap.button = > - tap_btn(tp, nmasked); > - } > - tp->tap.contacts = 0; > - tp->tap.centered = 0; > + is_tap = wstpad_is_tap(tp, t); > + if (is_tap && contacts == 0) { > + if (tp->tap.button) > + *cmds |= 1 << TAPBUTTON_UP; > + tp->tap.pending = (tp->tap.valid > + ? tap_button(tp) : 0); > + if (tp->tap.pending) { > + tp->tap.state = TAP_LIFTED; > + err = !timeout_add_msec(&tp->tap.to, > + CLICKDELAY_MS); > } > - } else { > - if (tp->contacts > nmasked) > + } else if (!is_tap && tp->tap.locktime == 0) { > + if (contacts == 0 && tp->tap.button) > + *cmds |= 1 << TAPBUTTON_UP; > + else if (contacts) > tp->tap.state = TAP_IGNORE; > - tp->tap.contacts = 0; > - tp->tap.centered = 0; > - } > - if (tp->tap.state == TAP_LIFTED) { > - if (tp->tap.button != 0) { > - *cmds |= 1 << TAPBUTTON_DOWN; > + } else if (!is_tap && tp->tap.button) { > + if (contacts == 0) { > + tp->tap.state = TAP_LOCKED; > err = !timeout_add_msec(&tp->tap.to, > - tp->tap.clicktime); > + tp->tap.locktime); > } else { > - tp->tap.state = TAP_DETECT; > + tp->tap.state = TAP_LOCKED_DRAG; > } > } > } > break; > - > case TAP_IGNORE: > - if (tp->contacts == nmasked) > + if (contacts == 0) { > tp->tap.state = TAP_DETECT; > - break; > - case TAP_LIFTED: > - if (tp->contacts > nmasked) { > - timeout_del(&tp->tap.to); > - if (tp->tap.button == LEFTBTN) { > - tp->tap.state = TAP_2ND_TOUCH; > - } else { > + if (tp->tap.button) > *cmds |= 1 << TAPBUTTON_UP; > - tp->tap.state = TAP_DETECT; > - } > } > break; > - case TAP_2ND_TOUCH: > - if (t) { > - if (wstpad_is_tap(tp, t)) { > - *cmds |= 1 << TAPBUTTON_DOUBLECLK; > - tp->tap.state = TAP_LIFTED; > - err = !timeout_add_msec(&tp->tap.to, > - CLICKDELAY_MS); > - } else if (tp->contacts == nmasked) { > - if (tp->tap.locktime == 0) { > - *cmds |= 1 << TAPBUTTON_UP; > - tp->tap.state = TAP_DETECT; > - } else { > - tp->tap.state = TAP_LOCKED; > - err = !timeout_add_msec(&tp->tap.to, > - tp->tap.locktime); > - } > - } > - } else if (tp->contacts != nmasked + 1) { > - *cmds |= 1 << TAPBUTTON_UP; > + case TAP_LIFTED: > + if (contacts) { > + timeout_del(&tp->tap.to); > tp->tap.state = TAP_DETECT; > + if (tp->tap.pending) > + *cmds |= 1 << TAPBUTTON_DOWN; > } > break; > case TAP_LOCKED: > - if (tp->contacts > nmasked) { > + if (contacts) { > timeout_del(&tp->tap.to); > - tp->tap.state = TAP_NTH_TOUCH; > + tp->tap.state = TAP_LOCKED_DRAG; > } > break; > - case TAP_NTH_TOUCH: > - if (t) { > - if (wstpad_is_tap(tp, t)) { > + case TAP_LOCKED_DRAG: > + if (contacts == 0) { > + t = wstpad_tap_touch(input); > + if (t && wstpad_is_tap(tp, t)) { > /* "tap-to-end" */ > *cmds |= 1 << TAPBUTTON_UP; > tp->tap.state = TAP_DETECT; > - } else if (tp->contacts == nmasked) { > + } else { > tp->tap.state = TAP_LOCKED; > err = !timeout_add_msec(&tp->tap.to, > tp->tap.locktime); > } > - } else if (tp->contacts != nmasked + 1) { > - *cmds |= 1 << TAPBUTTON_UP; > - tp->tap.state = TAP_DETECT; > } > break; > } > > if (err) { /* Did timeout_add fail? */ > - if (tp->tap.state == TAP_LIFTED) > - *cmds &= ~(1 << TAPBUTTON_DOWN); > - else > - *cmds |= 1 << TAPBUTTON_UP; > - > + input->sbtn.buttons &= ~tp->tap.button; > + input->sbtn.sync |= tp->tap.button; > + tp->tap.pending = 0; > + tp->tap.button = 0; > tp->tap.state = TAP_DETECT; > } > } > @@ -888,19 +862,31 @@ wstpad_tap_timeout(void *p) > struct wstpad *tp = input->tp; > struct evq_access evq; > u_int btn; > - int s; > + int s, ev; > > s = spltty(); > evq.evar = *input->evar; > if (evq.evar != NULL && tp != NULL && > (tp->tap.state == TAP_LIFTED || tp->tap.state == TAP_LOCKED)) { > - tp->tap.state = TAP_DETECT; > - input->sbtn.buttons &= ~tp->tap.button; > - btn = ffs(tp->tap.button) - 1; > + > + if (tp->tap.pending) { > + tp->tap.button = tp->tap.pending; > + tp->tap.pending = 0; > + ev = BTN_DOWN_EV; > + btn = ffs(tp->tap.button) - 1; > + input->sbtn.buttons |= tp->tap.button; > + timeout_add_msec(&tp->tap.to, tp->tap.clicktime); > + } else { > + ev = BTN_UP_EV; > + btn = ffs(tp->tap.button) - 1; > + input->sbtn.buttons &= ~tp->tap.button; > + tp->tap.button = 0; > + tp->tap.state = TAP_DETECT; > + } > evq.put = evq.evar->put; > evq.result = EVQ_RESULT_NONE; > getnanotime(&evq.ts); > - wsmouse_evq_put(&evq, BTN_UP_EV, btn); > + wsmouse_evq_put(&evq, ev, btn); > wsmouse_evq_put(&evq, SYNC_EV, 0); > if (evq.result == EVQ_RESULT_SUCCESS) { > if (input->flags & LOG_EVENTS) { > @@ -928,15 +914,12 @@ wstpad_click(struct wsmouseinput *input) > set_freeze_ts(tp, 0, FREEZE_MS); > } > > -/* > - * Translate the "command" bits into the sync-state of wsmouse, or into > - * wscons events. > - */ > +/* Translate the "command" bits into the sync-state of wsmouse. */ > void > -wstpad_cmds(struct wsmouseinput *input, struct evq_access *evq, u_int cmds) > +wstpad_cmds(struct wsmouseinput *input, u_int cmds) > { > struct wstpad *tp = input->tp; > - u_int btn, sbtns_dn = 0, sbtns_up = 0; > + u_int tapbtn, sbtns_dn = 0, sbtns_up = 0; > int n; > > FOREACHBIT(cmds, n) { > @@ -955,24 +938,24 @@ wstpad_cmds(struct wsmouseinput *input, > sbtns_up |= tp->softbutton; > tp->softbutton = 0; > continue; > + case TAPBUTTON_SYNC: > + if ((tapbtn = (tp->tap.button | tp->tap.pending))) { > + input->btn.sync &= ~tapbtn; > + sbtns_up &= ~tapbtn; > + sbtns_dn &= ~tapbtn; > + } > + continue; > case TAPBUTTON_DOWN: > + tp->tap.button = tp->tap.pending; > + tp->tap.pending = 0; > sbtns_dn |= tp->tap.button; > continue; > case TAPBUTTON_UP: > - sbtns_up |= tp->tap.button; > - continue; > - case TAPBUTTON_DOUBLECLK: > - /* > - * We cannot add the final BTN_UP event here, a > - * delay is required. This is the reason why the > - * tap handler returns from the 2ND_TOUCH state > - * into the LIFTED state with a short timeout > - * (CLICKDELAY_MS). > - */ > - btn = ffs(PRIMARYBTN) - 1; > - wsmouse_evq_put(evq, BTN_UP_EV, btn); > - wsmouse_evq_put(evq, SYNC_EV, 0); > - wsmouse_evq_put(evq, BTN_DOWN_EV, btn); > + if ((tp->tap.button & input->btn.buttons) == 0 > + && tp->tap.button != tp->softbutton) { > + sbtns_up |= tp->tap.button; > + } > + tp->tap.button = 0; > continue; > case HSCROLL: > input->motion.dw = tp->scroll.dw; > @@ -1276,7 +1259,7 @@ wstpad_process_input(struct wsmouseinput > } > } > > - wstpad_cmds(input, evq, cmds); > + wstpad_cmds(input, cmds); > > if (IS_MT(tp)) > clear_touchstates(input, TOUCH_NONE); >
