by Chase Douglas

[permalink] [raw]

Subject: Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

On Wed, 2010-09-01 at 00:00 +0200, Henrik Rydberg wrote:
> On 08/31/2010 08:41 PM, Chase Douglas wrote:
>
> > The trackpad speaks a similar, but different, protocol from the magic
> > mouse. However, only small code tweaks here and there are needed to make
> > basic multitouch work.
> >
> > Extra logic is required for single-touch emulation of the touchpad. The
> > changes made here take the approach that only one finger may emulate the
> > single pointer when multiple fingers have touched the screen. Once that
> > finger is raised, all touches must be raised before any further single
> > touch events can be sent.
> >
> > Sometimes the magic trackpad sends two distinct touch reports as one big
> > report. Simply splitting the packet in two and resending them through
> > magicmouse_raw_event ensures they are handled properly.
> >
> > I also added myself to the copyright statement.
> >
> > Signed-off-by: Chase Douglas <[email protected]>
>
>
> Thanks for this driver. Comments inline.
>
> [...]
>
> > @@ -166,15 +170,29 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
> > static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
> > {
> > struct input_dev *input = msc->input;
> > - int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> > - int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> > - int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> > - int size = tdata[5] & 0x3f;
> > - int orientation = (tdata[6] >> 2) - 32;
> > - int touch_major = tdata[3];
> > - int touch_minor = tdata[4];
> > - int state = tdata[7] & TOUCH_STATE_MASK;
> > - int down = state != TOUCH_STATE_NONE;
> > + int id, x, y, size, orientation, touch_major, touch_minor, state, down;
> > +
> > + if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > + id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> > + x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> > + y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> > + size = tdata[5] & 0x3f;
> > + orientation = (tdata[6] >> 2) - 32;
> > + touch_major = tdata[3];
> > + touch_minor = tdata[4];
> > + state = tdata[7] & TOUCH_STATE_MASK;
> > + down = state != TOUCH_STATE_NONE;
> > + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > + id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> > + x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> > + y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> > + size = tdata[6] & 0x3f;
> > + orientation = (tdata[7] >> 2) - 32;
> > + touch_major = tdata[4];
> > + touch_minor = tdata[5];
> > + state = tdata[8] & TOUCH_STATE_MASK;
> > + down = state != TOUCH_STATE_NONE;
> > + }
> >
> > /* Store tracking ID and other fields. */
> > msc->tracking_ids[raw_id] = id;
> > @@ -225,10 +243,15 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
> > }
> > }
> >
> > - /* Generate the input events for this touch. */
> > - if (report_touches && down) {
> > + if (down) {
> > msc->touches[id].down = 1;
> > + if (msc->single_touch_id == -1)
> > + msc->single_touch_id = id;
> > + } else if (msc->single_touch_id == id)
> > + msc->single_touch_id = -2;
>
>
> The logic using single_touch_id seems complicated. Any chance you could get by
> with less code here?

The overall code for single touch handling is ~10 lines spread around
the driver. I've tried to condense the code as much as possible. Perhaps
someone could come up with something more elegant, but all this code
does is keep track of the touch that is tied to single touch emulation.

In my next submission I've added macros to make the -1 and -2 values
more obvious on inspection.

> > + /* Generate the input events for this touch. */
> > + if (report_touches && down) {
> > input_report_abs(input, ABS_MT_TRACKING_ID, id);
>
>
> The tracking id reported by the macpad is not ideal; it works more like a slot
> that a MT protocol tracking id. Perhaps it is a slot? I would recommend looking
> at the recent submissions for bamboo touch, 3m-pct and w8001 to see how the
> tracking id and slots could be handled.

Yes, I think it could be better handled. However, that handling probably
belongs in another patch when slots are implemented in this driver. I
just haven't had a chance to get to it yet :).

> > input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
> > input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
> > @@ -236,8 +259,12 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
> > input_report_abs(input, ABS_MT_POSITION_X, x);
> > input_report_abs(input, ABS_MT_POSITION_Y, y);
> >
> > - if (report_undeciphered)
> > - input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> > + if (report_undeciphered) {
> > + if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
> > + input_event(input, EV_MSC, MSC_RAW, tdata[7]);
> > + else /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > + input_event(input, EV_MSC, MSC_RAW, tdata[8]);
> > + }
> >
> > input_mt_sync(input);
> > }
> > @@ -248,7 +275,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> > {
> > struct magicmouse_sc *msc = hid_get_drvdata(hdev);
> > struct input_dev *input = msc->input;
> > - int x, y, ts, ii, clicks, last_up;
> > + int x = 0, y = 0, ts, ii, clicks = 0, last_up;
> >
> > switch (data[0]) {
> > case 0x10:
> > @@ -258,7 +285,19 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> > y = (__s16)(data[4] | data[5] << 8);
> > clicks = data[1];
> > break;
> > - case TOUCH_REPORT_ID:
> > + case TRACKPAD_REPORT_ID:
> > + /* Expect four bytes of prefix, and N*9 bytes of touch data. */
> > + if (size < 4 || ((size - 4) % 9) != 0)
> > + return 0;
> > + ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
> > + msc->delta_time = (ts - msc->last_timestamp) & 0x3ffff;
> > + msc->last_timestamp = ts;
>
>
> The delta_time and last_timestamp do not seem to be used anywhere?

Good point. I've removed them in my next submission.

> > + msc->ntouches = (size - 4) / 9;
> > + for (ii = 0; ii < msc->ntouches; ii++)
> > + magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
> > + clicks = data[1];
> > + break;
> > + case MOUSE_REPORT_ID:
> > /* Expect six bytes of prefix, and N*8 bytes of touch data. */
> > if (size < 6 || ((size - 6) % 8) != 0)
> > return 0;
> > @@ -269,19 +308,6 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> > for (ii = 0; ii < msc->ntouches; ii++)
> > magicmouse_emit_touch(msc, ii, data + ii * 8 + 6);
> >
> > - if (report_touches) {
> > - last_up = 1;
> > - for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> > - if (msc->touches[ii].down) {
> > - last_up = 0;
> > - msc->touches[ii].down = 0;
> > - }
> > - }
> > - if (last_up) {
> > - input_mt_sync(input);
> > - }
> > - }
> > -
> > /* When emulating three-button mode, it is important
> > * to have the current touch information before
> > * generating a click event.
> > @@ -290,6 +316,14 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> > y = (int)(((data[3] & 0x30) << 26) | (data[2] << 22)) >> 22;
> > clicks = data[3];
> > break;
> > + case DOUBLE_REPORT_ID:
> > + /* Sometimes the trackpad sends two touch reports in one
> > + * packet.
> > + */
> > + magicmouse_raw_event(hdev, report, data + 2, data[1]);
> > + magicmouse_raw_event(hdev, report, data + 2 + data[1],
> > + size - 2 - data[1]);
> > + break;
>
>
> Nice find.
>
> > case 0x20: /* Theoretically battery status (0-100), but I have
> > * never seen it -- maybe it is only upon request.
> > */
> > @@ -301,9 +335,41 @@ static int magicmouse_raw_event(struct hid_device *hdev,
> > return 0;
> > }
> >
> > - magicmouse_emit_buttons(msc, clicks & 3);
> > - input_report_rel(input, REL_X, x);
> > - input_report_rel(input, REL_Y, y);
> > + if ((data[0] == MOUSE_REPORT_ID || data[0] == TRACKPAD_REPORT_ID)) {
> > + last_up = 1;
> > + for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> > + if (msc->touches[ii].down) {
> > + last_up = 0;
> > + msc->touches[ii].down = 0;
> > + }
> > + }
> > + if (last_up) {
> > + msc->single_touch_id = -1;
> > + msc->ntouches = 0;
> > + if (report_touches)
> > + input_mt_sync(input);
> > + }
>
>
> If the pointer emulation was handled differently, and the above was replaced
> with something like
>
> if (!msc->ntouches)
> input_mt_sync(input);
>
> it would be short enough not to need to be broken out of the switch... Besides,
> BTN_TOUCH is emitted for the trackpad case, so the above code is not strictly
> needed.

I've rewritten the touch down logic, and this does get simple enough to
keep inside the switch scope.

> > + }
> > +
> > + if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > + magicmouse_emit_buttons(msc, clicks & 3);
> > + input_report_rel(input, REL_X, x);
> > + input_report_rel(input, REL_Y, y);
> > + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > + input_report_key(input, BTN_MOUSE, clicks & 1);
> > + input_report_key(input, BTN_TOUCH, msc->ntouches > 0);
> > + input_report_key(input, BTN_TOOL_FINGER, msc->ntouches == 1);
> > + input_report_key(input, BTN_TOOL_DOUBLETAP, msc->ntouches == 2);
> > + input_report_key(input, BTN_TOOL_TRIPLETAP, msc->ntouches == 3);
> > + input_report_key(input, BTN_TOOL_QUADTAP, msc->ntouches == 4);
> > + if (msc->single_touch_id >= 0) {
> > + input_report_abs(input, ABS_X,
> > + msc->touches[msc->single_touch_id].x);
> > + input_report_abs(input, ABS_Y,
> > + msc->touches[msc->single_touch_id].y);
> > + }
> > + }
> > +
> > input_sync(input);
> > return 1;
> > }
> > @@ -339,18 +405,27 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> > input->dev.parent = hdev->dev.parent;
> >
> > __set_bit(EV_KEY, input->evbit);
> > - __set_bit(BTN_LEFT, input->keybit);
> > - __set_bit(BTN_RIGHT, input->keybit);
> > - if (emulate_3button)
> > - __set_bit(BTN_MIDDLE, input->keybit);
> > - __set_bit(BTN_TOOL_FINGER, input->keybit);
> > -
> > - __set_bit(EV_REL, input->evbit);
> > - __set_bit(REL_X, input->relbit);
> > - __set_bit(REL_Y, input->relbit);
> > - if (emulate_scroll_wheel) {
> > - __set_bit(REL_WHEEL, input->relbit);
> > - __set_bit(REL_HWHEEL, input->relbit);
> > +
> > + if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > + __set_bit(BTN_LEFT, input->keybit);
> > + __set_bit(BTN_RIGHT, input->keybit);
> > + if (emulate_3button)
> > + __set_bit(BTN_MIDDLE, input->keybit);
> > +
> > + __set_bit(EV_REL, input->evbit);
> > + __set_bit(REL_X, input->relbit);
> > + __set_bit(REL_Y, input->relbit);
> > + if (emulate_scroll_wheel) {
> > + __set_bit(REL_WHEEL, input->relbit);
> > + __set_bit(REL_HWHEEL, input->relbit);
> > + }
> > + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > + __set_bit(BTN_MOUSE, input->keybit);
> > + __set_bit(BTN_TOOL_FINGER, input->keybit);
> > + __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
> > + __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
> > + __set_bit(BTN_TOOL_QUADTAP, input->keybit);
> > + __set_bit(BTN_TOUCH, input->keybit);
> > }
> >
> > if (report_touches) {
> > @@ -360,16 +435,26 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
> > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> > input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
> > - input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
> > - 0, 0);
> > +
> > /* Note: Touch Y position from the device is inverted relative
> > * to how pointer motion is reported (and relative to how USB
> > * HID recommends the coordinates work). This driver keeps
> > * the origin at the same position, and just uses the additive
> > * inverse of the reported Y.
> > */
> > - input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
> > - 0, 0);
> > + if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> > + input_set_abs_params(input, ABS_MT_POSITION_X, -1100,
> > + 1358, 0, 0);
> > + input_set_abs_params(input, ABS_MT_POSITION_Y, -1589,
> > + 2047, 0, 0);
> > + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> > + input_set_abs_params(input, ABS_X, -2909, 3167, 0, 0);
> > + input_set_abs_params(input, ABS_Y, -2456, 2565, 0, 0);
> > + input_set_abs_params(input, ABS_MT_POSITION_X, -2909,
> > + 3167, 0, 0);
> > + input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
> > + 2565, 0, 0);
> > + }
> > }
> >
> > if (report_undeciphered) {
> > @@ -388,12 +473,29 @@ static struct feature mouse_features[] = {
> > { { 0xf8, 0x01, 0x32 }, 3 }
> > };
> >
> > +static struct feature trackpad_features[] = {
> > + { { 0xf1, 0xdb }, 2 },
> > + { { 0xf1, 0xdc }, 2 },
> > + { { 0xf0, 0x00 }, 2 },
> > + { { 0xf1, 0xdd }, 2 },
> > + { { 0xf0, 0x02 }, 2 },
> > + { { 0xf1, 0xc8 }, 2 },
> > + { { 0xf0, 0x09 }, 2 },
> > + { { 0xf1, 0xdc }, 2 },
> > + { { 0xf0, 0x00 }, 2 },
> > + { { 0xf1, 0xdd }, 2 },
> > + { { 0xf0, 0x02 }, 2 },
> > + { { 0xd7, 0x01 }, 2 },
> > +};
>
>
> As noted by Michael, this could likely be simplified. the "0x01" on the last
> line could be the same coding as wellspring mode for bcm5974.

I've simplified this for the next submission.

-- Chase

2010-09-01 01:55:23

by Chase Douglas

[permalink] [raw]

Subject: Re: [PATCH 5/6 v2] HID: magicmouse: enable Magic Trackpad support

On Tue, 2010-08-31 at 20:08 -0400, Michael Poole wrote:
> One behavior that slightly surprised me -- which I believe is a quirk
> due to userspace not expecting touchpads to have button switches -- is
> that touches on the trackpad that do not close the switch can still be
> interpreted by X as clicks.

This is actually done by the X synaptics input module. It's the defacto
X touchpad driver and enables niceties like two finger scrolling and
whatnot. You can either use it as is, use xinput/xorg.conf to manipulate
its configuration, or switch to the X evdev input module which is more
"bare" support.

FYI, our gesture support in Maverick is based on a modified version of
the X evdev input module. However, because the whole stack of desktop
libs and toolkits won't support gestures in Maverick, you lose things
like two finger scroll. Thus, we're going to leave the default X input
module for the magic trackpad to synaptics in Ubuntu.

> Once the discussions about if/how to tweak this code settle down, I'll
> put together a patch to change the "down" and "last_up" logic as I
> suggested earlier.

I actually decided to tackle this to make the patches I'm writing
easier. I'll be sending a new batch shortly.

Thanks,

-- Chase