Re: your mail

Re: your mail

[Henrik Rydberg]

Hi Maurus,

> Subject: [RFC] HID: add Microsoft Touch Mouse driver
> 
> This patch adds support for the proprietary Microsoft Touch Mouse
> multitouch protocol.

Exciting stuff, nice job on the patch too. Please find some initial
comments inline.

> @@ -0,0 +1,371 @@
> +/*
> + *  HID driver for the Microsoft Touch Mouse
> + *
> + *  Copyright (c) 2011 Maurus Cuelenaere <mcuelenaere@gmail.com>
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +
> +#define MSTM_RAW_DATA_HID_ID		0x27
> +#define MSTM_RAW_DATA_FOOTER		0x51
> +
> +#define MSTM_DATA_WIDTH			15
> +#define MSTM_DATA_HEIGHT		13
> +
> +struct mstm_raw_data_packet {
> +	__u8 hid_id;
> +	__u8 data_length;
> +	__u16 unk1;
> +	__u8 unk2;
> +	__u8 footer;
> +	__u8 timestamp;
> +	__u8 data[25]; /* milliseconds */
> +};

I take it this means you get at most 50 nibbles per transfer?

> +
> +struct mstm_state {
> +	bool advance_flag;
> +	int x;
> +	int y;
> +	unsigned char last_timestamp;
> +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
> +};

The ability to simply send an "input screen" would be perfect
here. This device may be on the border of what can/should be handled
via input events. A memory mapped driver, uio-based or something
similar, could be an option.

> +
> +struct mstm_device {
> +	struct input_dev *input;
> +
> +	struct mstm_state state;
> +};
> +
> +#define MSTM_INTERFACE_KEYBOARD		0
> +#define MSTM_INTERFACE_MOUSE		1
> +#define MSTM_INTERFACE_CONTROL		2
> +
> +static inline int hid_get_interface_number(struct hid_device *hdev) {
> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +	return intf->cur_altsetting->desc.bInterfaceNumber;
> +}
> +
> +/*
> + * The mouse sensor only yields data for a specific part of its surface.
> + * Because of this, we can't just increase x and y uniformly; so there's
> + * a need for this simple algorithm.
> + *
> + * Visual representation of available data:
> + *    0 1 2 3 4 5 6 7 8 9 A B C D E F
> + *  0       * * * * * * * * * *
> + *  1     * * * * * * * * * * * *
> + *  2   * * * * * * * * * * * * * *
> + *  3   * * * * * * * * * * * * * *
> + *  4 * * * * * * * * * * * * * * * *
> + *  5 * * * * * * * * * * * * * * * *
> + *  6 * * * * * * * * * * * * * * * *
> + *  7 * * * * * * * * * * * * * * * *
> + *  8 * * * * * * * * * * * * * * * *
> + *  9 * * * * * * * * * * * * * * * *
> + *  A * * * * * * * * * * * * * * * *
> + *  B * * * * * * * * * * * * * * * *
> + *  C * * * * * * * * * * * * * * * *
> + *  D * * * * * * * * * * * * * * * *
> + */
> +static void mstm_advance_pointer(struct mstm_state *state)
> +{
> +	int max, nextMin;
> +
> +	state->x++;
> +
> +	switch (state->y) {
> +	case 0:
> +		max = 11;
> +		nextMin = 2;
> +		break;
> +	case 1:
> +		max = 12;
> +		nextMin = 1;
> +		break;
> +	case 2:
> +		max = 13;
> +		nextMin = 1;
> +		break;
> +	case 3:
> +		max = 13;
> +		nextMin = 0;
> +		break;
> +	default:
> +		max = 14;
> +		nextMin = 0;
> +		break;
> +	}
> +
> +	if (state->x > max) {
> +		state->y++;
> +		state->x = nextMin;
> +	}
> +}
> +
> +static void mstm_parse_nibble(struct mstm_state *state, __u8 nibble)
> +{
> +	int i;
> +
> +	if (state->advance_flag) {
> +		state->advance_flag = false;
> +		for (i = -3; i < nibble; i++)
> +			mstm_advance_pointer(state);
> +	} else {
> +		if (nibble == 0xF) {
> +			/* The next nibble will indicate how many
> +			* positions to advance, so set a flag */
> +			state->advance_flag = true;
> +		} else {
> +			state->data[state->x][state->y] = nibble;
> +			mstm_advance_pointer(state);
> +		}
> +	}
> +}

Looking good.

> +static void mstm_push_data(struct hid_device *hdev)
> +{
> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
> +	struct input_dev *input = mstm_dev->input;
> +	int x, y;
> +
> +	for (x = 0; x < MSTM_DATA_WIDTH; x++) {
> +		for (y = 0; y < MSTM_DATA_HEIGHT; y++) {
> +			unsigned char pressure = mstm_dev->state.data[x][y];
> +			if (pressure > 0) {
> +				//input_report_abs(input, ABS_MT_BLOB_ID, 1);
> +				//input_report_abs(input, ABS_MT_TOUCH_MAJOR, pressure/3);
> +				input_report_abs(input, ABS_MT_POSITION_X, x);
> +				input_report_abs(input, ABS_MT_POSITION_Y, y);
> +				input_mt_sync(input);
> +			}
> +		}
> +	}

True, the blob id has not yet been used, but its definition is
different from how it is used here. Also, since you do not attempt any
kind of clustering (single-linkage or similar), the blob as stated
might not even be connected. One possible option could be to use the
slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
else. The device would (rightfully) not be recognized as MT since the
position is missing, all data would be available for processing in
userspace, and bandwidth would be minimized since there could only be
so many changes coming in per millisecond.

> +static int mstm_input_mapping(struct hid_device *hdev,
> +		struct hid_input *hi, struct hid_field *field,
> +		struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
> +
> +	//printk("mstm_input_mapping(%p)\n", hdev);
> +
> +	/* bail early if not the correct interface */
> +	if (mstm_dev == NULL)
> +		return 0;
> +
> +	/* HACK: get rid of ABS_MT_* params */
> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_GENDESK)
> +		return -1;

I am not sure about the hack here, nor its explanation. ;-)

> +
> +	/* map input device to hid device */
> +	mstm_dev->input = hi->input;
> +
> +	return 0;
> +}
> +
> +static void mstm_setup_input(struct mstm_device *mstm)
> +{
> +	__set_bit(EV_ABS, mstm->input->evbit);
> +
> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_X, 0, MSTM_DATA_WIDTH, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_Y, 0, MSTM_DATA_HEIGHT, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_TOUCH_MAJOR, 0, 0xF, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_BLOB_ID, 0, 10, 0, 0);
> +
> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_X, 6); /* 83mm */
> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_Y, 5); /* 70mm */
> +
> +	input_set_events_per_packet(mstm->input, 60);
> +}

Regarding the resolution of touch major, it is generally assumed (in
userspace) that the units are the same as the position, so scaling in
the driver is reasonable. Otherwise, why not specify resolution for
touch major as well?

Thanks,
Henrik

Re: your mail

[Maurus Cuelenaere]

Hi Henrik,

Op 25-01-12 14:26, Henrik Rydberg schreef:
> Hi Maurus,
>
>> Subject: [RFC] HID: add Microsoft Touch Mouse driver
>>
>> This patch adds support for the proprietary Microsoft Touch Mouse
>> multitouch protocol.
> Exciting stuff, nice job on the patch too. Please find some initial
> comments inline.
>
>> @@ -0,0 +1,371 @@
>> +/*
>> + *  HID driver for the Microsoft Touch Mouse
>> + *
>> + *  Copyright (c) 2011 Maurus Cuelenaere<mcuelenaere@gmail.com>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + *
>> + */
>> +
>> +#include<linux/device.h>
>> +#include<linux/input.h>
>> +#include<linux/hid.h>
>> +#include<linux/module.h>
>> +#include<linux/usb.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +#define MSTM_RAW_DATA_HID_ID		0x27
>> +#define MSTM_RAW_DATA_FOOTER		0x51
>> +
>> +#define MSTM_DATA_WIDTH			15
>> +#define MSTM_DATA_HEIGHT		13
>> +
>> +struct mstm_raw_data_packet {
>> +	__u8 hid_id;
>> +	__u8 data_length;
>> +	__u16 unk1;
>> +	__u8 unk2;
>> +	__u8 footer;
>> +	__u8 timestamp;
>> +	__u8 data[25]; /* milliseconds */
>> +};
> I take it this means you get at most 50 nibbles per transfer?

Correct.

Hmm, looks like that milliseconds comment needs to be on the line above it.

>> +
>> +struct mstm_state {
>> +	bool advance_flag;
>> +	int x;
>> +	int y;
>> +	unsigned char last_timestamp;
>> +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
>> +};
> The ability to simply send an "input screen" would be perfect
> here. This device may be on the border of what can/should be handled
> via input events. A memory mapped driver, uio-based or something
> similar, could be an option.

In my first tests, I was doing readouts in userspace using hidraw, which 
performed quite well.

Bandwidth could be an issue, but I'd like to use the current APIs as 
much as possible so I don't need to go modifying mtdev, X, ...

>> +
>> +struct mstm_device {
>> +	struct input_dev *input;
>> +
>> +	struct mstm_state state;
>> +};
>> +
>> +#define MSTM_INTERFACE_KEYBOARD		0
>> +#define MSTM_INTERFACE_MOUSE		1
>> +#define MSTM_INTERFACE_CONTROL		2
>> +
>> +static inline int hid_get_interface_number(struct hid_device *hdev) {
>> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>> +	return intf->cur_altsetting->desc.bInterfaceNumber;
>> +}
>> +
>> +/*
>> + * The mouse sensor only yields data for a specific part of its surface.
>> + * Because of this, we can't just increase x and y uniformly; so there's
>> + * a need for this simple algorithm.
>> + *
>> + * Visual representation of available data:
>> + *    0 1 2 3 4 5 6 7 8 9 A B C D E F
>> + *  0       * * * * * * * * * *
>> + *  1     * * * * * * * * * * * *
>> + *  2   * * * * * * * * * * * * * *
>> + *  3   * * * * * * * * * * * * * *
>> + *  4 * * * * * * * * * * * * * * * *
>> + *  5 * * * * * * * * * * * * * * * *
>> + *  6 * * * * * * * * * * * * * * * *
>> + *  7 * * * * * * * * * * * * * * * *
>> + *  8 * * * * * * * * * * * * * * * *
>> + *  9 * * * * * * * * * * * * * * * *
>> + *  A * * * * * * * * * * * * * * * *
>> + *  B * * * * * * * * * * * * * * * *
>> + *  C * * * * * * * * * * * * * * * *
>> + *  D * * * * * * * * * * * * * * * *
>> + */
>> +static void mstm_advance_pointer(struct mstm_state *state)
>> +{
>> +	int max, nextMin;
>> +
>> +	state->x++;
>> +
>> +	switch (state->y) {
>> +	case 0:
>> +		max = 11;
>> +		nextMin = 2;
>> +		break;
>> +	case 1:
>> +		max = 12;
>> +		nextMin = 1;
>> +		break;
>> +	case 2:
>> +		max = 13;
>> +		nextMin = 1;
>> +		break;
>> +	case 3:
>> +		max = 13;
>> +		nextMin = 0;
>> +		break;
>> +	default:
>> +		max = 14;
>> +		nextMin = 0;
>> +		break;
>> +	}
>> +
>> +	if (state->x>  max) {
>> +		state->y++;
>> +		state->x = nextMin;
>> +	}
>> +}
>> +
>> +static void mstm_parse_nibble(struct mstm_state *state, __u8 nibble)
>> +{
>> +	int i;
>> +
>> +	if (state->advance_flag) {
>> +		state->advance_flag = false;
>> +		for (i = -3; i<  nibble; i++)
>> +			mstm_advance_pointer(state);
>> +	} else {
>> +		if (nibble == 0xF) {
>> +			/* The next nibble will indicate how many
>> +			* positions to advance, so set a flag */
>> +			state->advance_flag = true;
>> +		} else {
>> +			state->data[state->x][state->y] = nibble;
>> +			mstm_advance_pointer(state);
>> +		}
>> +	}
>> +}
> Looking good.
>
>> +static void mstm_push_data(struct hid_device *hdev)
>> +{
>> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>> +	struct input_dev *input = mstm_dev->input;
>> +	int x, y;
>> +
>> +	for (x = 0; x<  MSTM_DATA_WIDTH; x++) {
>> +		for (y = 0; y<  MSTM_DATA_HEIGHT; y++) {
>> +			unsigned char pressure = mstm_dev->state.data[x][y];
>> +			if (pressure>  0) {
>> +				//input_report_abs(input, ABS_MT_BLOB_ID, 1);
>> +				//input_report_abs(input, ABS_MT_TOUCH_MAJOR, pressure/3);
>> +				input_report_abs(input, ABS_MT_POSITION_X, x);
>> +				input_report_abs(input, ABS_MT_POSITION_Y, y);
>> +				input_mt_sync(input);
>> +			}
>> +		}
>> +	}
> True, the blob id has not yet been used, but its definition is
> different from how it is used here. Also, since you do not attempt any
> kind of clustering (single-linkage or similar), the blob as stated
> might not even be connected.

Indeed, without clustering the BLOB_ID would be useless but that line 
was only for testing with one finger.

> One possible option could be to use the
> slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
> else. The device would (rightfully) not be recognized as MT since the
> position is missing, all data would be available for processing in
> userspace, and bandwidth would be minimized since there could only be
> so many changes coming in per millisecond.

So how does userspace then finds out where these pressure points are 
located?
Or do you mean to just dump all data to user space (15 * 13 * 
sizeof(ABS_MT_PRESSURE value) + overhead)?

>> +static int mstm_input_mapping(struct hid_device *hdev,
>> +		struct hid_input *hi, struct hid_field *field,
>> +		struct hid_usage *usage, unsigned long **bit, int *max)
>> +{
>> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>> +
>> +	//printk("mstm_input_mapping(%p)\n", hdev);
>> +
>> +	/* bail early if not the correct interface */
>> +	if (mstm_dev == NULL)
>> +		return 0;
>> +
>> +	/* HACK: get rid of ABS_MT_* params */
>> +	if ((usage->hid&  HID_USAGE_PAGE) == HID_UP_GENDESK)
>> +		return -1;
> I am not sure about the hack here, nor its explanation. ;-)

The HID tables specify some ABS_MT_* values and I didn't know whether 
these were going to conflict with the ones I report above, so I just 
discard all GenericDesktop fields.

root@hp4530s:~# grep MT /sys/kernel/debug/hid/0003\:045E\:0773.0006/rdesc
GenericDesktop.MultiAxis ---> Absolute.MTMajor
GenericDesktop.0009 ---> Absolute.MTMinor
GenericDesktop.000a ---> Absolute.MTMajorW
GenericDesktop.000b ---> Absolute.MTMinorW
GenericDesktop.000c ---> Absolute.MTOrientation
GenericDesktop.000d ---> Absolute.MTPositionX
GenericDesktop.000e ---> Absolute.MTPositionY
GenericDesktop.000f ---> Absolute.MTToolType
GenericDesktop.0010 ---> Absolute.MTBlobID

>> +
>> +	/* map input device to hid device */
>> +	mstm_dev->input = hi->input;
>> +
>> +	return 0;
>> +}
>> +
>> +static void mstm_setup_input(struct mstm_device *mstm)
>> +{
>> +	__set_bit(EV_ABS, mstm->input->evbit);
>> +
>> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_X, 0, MSTM_DATA_WIDTH, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_Y, 0, MSTM_DATA_HEIGHT, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_TOUCH_MAJOR, 0, 0xF, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_BLOB_ID, 0, 10, 0, 0);
>> +
>> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_X, 6); /* 83mm */
>> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_Y, 5); /* 70mm */
>> +
>> +	input_set_events_per_packet(mstm->input, 60);
>> +}
> Regarding the resolution of touch major, it is generally assumed (in
> userspace) that the units are the same as the position, so scaling in
> the driver is reasonable. Otherwise, why not specify resolution for
> touch major as well?

I didn't specify it because other HID drivers only specified resolutions 
for POSITION_{X,Y}, I'll try it and see how mtview reacts.

Thanks for the review!

-- 
Maurus Cuelenaere

Fwd: Re: your mail

[Maurus Cuelenaere]

[Retry sending to linux-input]

Hi Henrik,

Op 25-01-12 14:26, Henrik Rydberg schreef:
>  Hi Maurus,
>
>>  Subject: [RFC] HID: add Microsoft Touch Mouse driver
>>
>>  This patch adds support for the proprietary Microsoft Touch Mouse
>>  multitouch protocol.
>  Exciting stuff, nice job on the patch too. Please find some initial
>  comments inline.
>
>>  @@ -0,0 +1,371 @@
>>  +/*
>>  + *  HID driver for the Microsoft Touch Mouse
>>  + *
>>  + *  Copyright (c) 2011 Maurus Cuelenaere<mcuelenaere@gmail.com>
>>  + *
>>  + *
>>  + * This program is free software; you can redistribute it and/or modify
>>  + * it under the terms of the GNU General Public License version 2 as
>>  + * published by the Free Software Foundation.
>>  +
>>  + *
>>  + * This program is distributed in the hope that it will be useful,
>>  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>  + * GNU General Public License for more details.
>>  + *
>>  + * You should have received a copy of the GNU General Public License
>>  + * along with this program; if not, write to the Free Software
>>  + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>>  + *
>>  + */
>>  +
>>  +#include<linux/device.h>
>>  +#include<linux/input.h>
>>  +#include<linux/hid.h>
>>  +#include<linux/module.h>
>>  +#include<linux/usb.h>
>>  +
>>  +#include "hid-ids.h"
>>  +
>>  +#define MSTM_RAW_DATA_HID_ID		0x27
>>  +#define MSTM_RAW_DATA_FOOTER		0x51
>>  +
>>  +#define MSTM_DATA_WIDTH			15
>>  +#define MSTM_DATA_HEIGHT		13
>>  +
>>  +struct mstm_raw_data_packet {
>>  +	__u8 hid_id;
>>  +	__u8 data_length;
>>  +	__u16 unk1;
>>  +	__u8 unk2;
>>  +	__u8 footer;
>>  +	__u8 timestamp;
>>  +	__u8 data[25]; /* milliseconds */
>>  +};
>  I take it this means you get at most 50 nibbles per transfer?

Correct.

Hmm, looks like that milliseconds comment needs to be on the line above it.

>>  +
>>  +struct mstm_state {
>>  +	bool advance_flag;
>>  +	int x;
>>  +	int y;
>>  +	unsigned char last_timestamp;
>>  +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
>>  +};
>  The ability to simply send an "input screen" would be perfect
>  here. This device may be on the border of what can/should be handled
>  via input events. A memory mapped driver, uio-based or something
>  similar, could be an option.

In my first tests, I was doing readouts in userspace using hidraw, which
performed quite well.

Bandwidth could be an issue, but I'd like to use the current APIs as
much as possible so I don't need to go modifying mtdev, X, ...

>>  +
>>  +struct mstm_device {
>>  +	struct input_dev *input;
>>  +
>>  +	struct mstm_state state;
>>  +};
>>  +
>>  +#define MSTM_INTERFACE_KEYBOARD		0
>>  +#define MSTM_INTERFACE_MOUSE		1
>>  +#define MSTM_INTERFACE_CONTROL		2
>>  +
>>  +static inline int hid_get_interface_number(struct hid_device *hdev) {
>>  +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>>  +	return intf->cur_altsetting->desc.bInterfaceNumber;
>>  +}
>>  +
>>  +/*
>>  + * The mouse sensor only yields data for a specific part of its surface.
>>  + * Because of this, we can't just increase x and y uniformly; so there's
>>  + * a need for this simple algorithm.
>>  + *
>>  + * Visual representation of available data:
>>  + *    0 1 2 3 4 5 6 7 8 9 A B C D E F
>>  + *  0       * * * * * * * * * *
>>  + *  1     * * * * * * * * * * * *
>>  + *  2   * * * * * * * * * * * * * *
>>  + *  3   * * * * * * * * * * * * * *
>>  + *  4 * * * * * * * * * * * * * * * *
>>  + *  5 * * * * * * * * * * * * * * * *
>>  + *  6 * * * * * * * * * * * * * * * *
>>  + *  7 * * * * * * * * * * * * * * * *
>>  + *  8 * * * * * * * * * * * * * * * *
>>  + *  9 * * * * * * * * * * * * * * * *
>>  + *  A * * * * * * * * * * * * * * * *
>>  + *  B * * * * * * * * * * * * * * * *
>>  + *  C * * * * * * * * * * * * * * * *
>>  + *  D * * * * * * * * * * * * * * * *
>>  + */
>>  +static void mstm_advance_pointer(struct mstm_state *state)
>>  +{
>>  +	int max, nextMin;
>>  +
>>  +	state->x++;
>>  +
>>  +	switch (state->y) {
>>  +	case 0:
>>  +		max = 11;
>>  +		nextMin = 2;
>>  +		break;
>>  +	case 1:
>>  +		max = 12;
>>  +		nextMin = 1;
>>  +		break;
>>  +	case 2:
>>  +		max = 13;
>>  +		nextMin = 1;
>>  +		break;
>>  +	case 3:
>>  +		max = 13;
>>  +		nextMin = 0;
>>  +		break;
>>  +	default:
>>  +		max = 14;
>>  +		nextMin = 0;
>>  +		break;
>>  +	}
>>  +
>>  +	if (state->x>   max) {
>>  +		state->y++;
>>  +		state->x = nextMin;
>>  +	}
>>  +}
>>  +
>>  +static void mstm_parse_nibble(struct mstm_state *state, __u8 nibble)
>>  +{
>>  +	int i;
>>  +
>>  +	if (state->advance_flag) {
>>  +		state->advance_flag = false;
>>  +		for (i = -3; i<   nibble; i++)
>>  +			mstm_advance_pointer(state);
>>  +	} else {
>>  +		if (nibble == 0xF) {
>>  +			/* The next nibble will indicate how many
>>  +			* positions to advance, so set a flag */
>>  +			state->advance_flag = true;
>>  +		} else {
>>  +			state->data[state->x][state->y] = nibble;
>>  +			mstm_advance_pointer(state);
>>  +		}
>>  +	}
>>  +}
>  Looking good.
>
>>  +static void mstm_push_data(struct hid_device *hdev)
>>  +{
>>  +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>>  +	struct input_dev *input = mstm_dev->input;
>>  +	int x, y;
>>  +
>>  +	for (x = 0; x<   MSTM_DATA_WIDTH; x++) {
>>  +		for (y = 0; y<   MSTM_DATA_HEIGHT; y++) {
>>  +			unsigned char pressure = mstm_dev->state.data[x][y];
>>  +			if (pressure>   0) {
>>  +				//input_report_abs(input, ABS_MT_BLOB_ID, 1);
>>  +				//input_report_abs(input, ABS_MT_TOUCH_MAJOR, pressure/3);
>>  +				input_report_abs(input, ABS_MT_POSITION_X, x);
>>  +				input_report_abs(input, ABS_MT_POSITION_Y, y);
>>  +				input_mt_sync(input);
>>  +			}
>>  +		}
>>  +	}
>  True, the blob id has not yet been used, but its definition is
>  different from how it is used here. Also, since you do not attempt any
>  kind of clustering (single-linkage or similar), the blob as stated
>  might not even be connected.

Indeed, without clustering the BLOB_ID would be useless but that line
was only for testing with one finger.

>  One possible option could be to use the
>  slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
>  else. The device would (rightfully) not be recognized as MT since the
>  position is missing, all data would be available for processing in
>  userspace, and bandwidth would be minimized since there could only be
>  so many changes coming in per millisecond.

So how does userspace then finds out where these pressure points are
located?
Or do you mean to just dump all data to user space (15 * 13 *
sizeof(ABS_MT_PRESSURE value) + overhead)?

>>  +static int mstm_input_mapping(struct hid_device *hdev,
>>  +		struct hid_input *hi, struct hid_field *field,
>>  +		struct hid_usage *usage, unsigned long **bit, int *max)
>>  +{
>>  +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>>  +
>>  +	//printk("mstm_input_mapping(%p)\n", hdev);
>>  +
>>  +	/* bail early if not the correct interface */
>>  +	if (mstm_dev == NULL)
>>  +		return 0;
>>  +
>>  +	/* HACK: get rid of ABS_MT_* params */
>>  +	if ((usage->hid&   HID_USAGE_PAGE) == HID_UP_GENDESK)
>>  +		return -1;
>  I am not sure about the hack here, nor its explanation. ;-)

The HID tables specify some ABS_MT_* values and I didn't know whether
these were going to conflict with the ones I report above, so I just
discard all GenericDesktop fields.

root@hp4530s:~# grep MT /sys/kernel/debug/hid/0003\:045E\:0773.0006/rdesc
GenericDesktop.MultiAxis --->  Absolute.MTMajor
GenericDesktop.0009 --->  Absolute.MTMinor
GenericDesktop.000a --->  Absolute.MTMajorW
GenericDesktop.000b --->  Absolute.MTMinorW
GenericDesktop.000c --->  Absolute.MTOrientation
GenericDesktop.000d --->  Absolute.MTPositionX
GenericDesktop.000e --->  Absolute.MTPositionY
GenericDesktop.000f --->  Absolute.MTToolType
GenericDesktop.0010 --->  Absolute.MTBlobID

>>  +
>>  +	/* map input device to hid device */
>>  +	mstm_dev->input = hi->input;
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void mstm_setup_input(struct mstm_device *mstm)
>>  +{
>>  +	__set_bit(EV_ABS, mstm->input->evbit);
>>  +
>>  +	input_set_abs_params(mstm->input, ABS_MT_POSITION_X, 0, MSTM_DATA_WIDTH, 0, 0);
>>  +	input_set_abs_params(mstm->input, ABS_MT_POSITION_Y, 0, MSTM_DATA_HEIGHT, 0, 0);
>>  +	input_set_abs_params(mstm->input, ABS_MT_TOUCH_MAJOR, 0, 0xF, 0, 0);
>>  +	input_set_abs_params(mstm->input, ABS_MT_BLOB_ID, 0, 10, 0, 0);
>>  +
>>  +	input_abs_set_res(mstm->input, ABS_MT_POSITION_X, 6); /* 83mm */
>>  +	input_abs_set_res(mstm->input, ABS_MT_POSITION_Y, 5); /* 70mm */
>>  +
>>  +	input_set_events_per_packet(mstm->input, 60);
>>  +}
>  Regarding the resolution of touch major, it is generally assumed (in
>  userspace) that the units are the same as the position, so scaling in
>  the driver is reasonable. Otherwise, why not specify resolution for
>  touch major as well?

I didn't specify it because other HID drivers only specified resolutions
for POSITION_{X,Y}, I'll try it and see how mtview reacts.

Thanks for the review!

-- 
Maurus Cuelenaere

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Henrik Rydberg]

> In my first tests, I was doing readouts in userspace using hidraw,
> which performed quite well.
> 
> Bandwidth could be an issue, but I'd like to use the current APIs as
> much as possible so I don't need to go modifying mtdev, X, ...

Unless you do clustering, filtering and what not in-kernel, something
will have to change for sure.

> >One possible option could be to use the
> >slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
> >else. The device would (rightfully) not be recognized as MT since the
> >position is missing, all data would be available for processing in
> >userspace, and bandwidth would be minimized since there could only be
> >so many changes coming in per millisecond.
> 
> So how does userspace then finds out where these pressure points are
> located?
> Or do you mean to just dump all data to user space (15 * 13 *
> sizeof(ABS_MT_PRESSURE value) + overhead)?

Having each pressure point represented by one slot id was the idea.
Userspace would have to know how the points are mapped, of
course. Still not overly happy about the general fit, though. Dmitry?

> >>+	/* HACK: get rid of ABS_MT_* params */
> >>+	if ((usage->hid&  HID_USAGE_PAGE) == HID_UP_GENDESK)
> >>+		return -1;
> >I am not sure about the hack here, nor its explanation. ;-)
> 
> The HID tables specify some ABS_MT_* values and I didn't know
> whether these were going to conflict with the ones I report above,
> so I just discard all GenericDesktop fields.
> 
> root@hp4530s:~# grep MT /sys/kernel/debug/hid/0003\:045E\:0773.0006/rdesc
> GenericDesktop.MultiAxis ---> Absolute.MTMajor
> GenericDesktop.0009 ---> Absolute.MTMinor
> GenericDesktop.000a ---> Absolute.MTMajorW
> GenericDesktop.000b ---> Absolute.MTMinorW
> GenericDesktop.000c ---> Absolute.MTOrientation
> GenericDesktop.000d ---> Absolute.MTPositionX
> GenericDesktop.000e ---> Absolute.MTPositionY
> GenericDesktop.000f ---> Absolute.MTToolType
> GenericDesktop.0010 ---> Absolute.MTBlobID

Right, they would conflict. You can also use the input_mapped() hook.

Thanks,
Henrik

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Maurus Cuelenaere]

Op 25-01-12 16:00, Henrik Rydberg schreef:
>> In my first tests, I was doing readouts in userspace using hidraw,
>> which performed quite well.
>>
>> Bandwidth could be an issue, but I'd like to use the current APIs as
>> much as possible so I don't need to go modifying mtdev, X, ...
> Unless you do clustering, filtering and what not in-kernel, something
> will have to change for sure.
>
>>> One possible option could be to use the
>>> slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
>>> else. The device would (rightfully) not be recognized as MT since the
>>> position is missing, all data would be available for processing in
>>> userspace, and bandwidth would be minimized since there could only be
>>> so many changes coming in per millisecond.
>> So how does userspace then finds out where these pressure points are
>> located?
>> Or do you mean to just dump all data to user space (15 * 13 *
>> sizeof(ABS_MT_PRESSURE value) + overhead)?
> Having each pressure point represented by one slot id was the idea.
> Userspace would have to know how the points are mapped, of
> course. Still not overly happy about the general fit, though. Dmitry?

Ping.

Or do you want me to resend the original mails? It didn't seem to get 
through on the mailing list.

-- 
Maurus Cuelenaere

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Dmitry Torokhov]

On Wed, Jan 25, 2012 at 04:00:35PM +0100, Henrik Rydberg wrote:
> > In my first tests, I was doing readouts in userspace using hidraw,
> > which performed quite well.
> > 
> > Bandwidth could be an issue, but I'd like to use the current APIs as
> > much as possible so I don't need to go modifying mtdev, X, ...
> 
> Unless you do clustering, filtering and what not in-kernel, something
> will have to change for sure.
> 
> > >One possible option could be to use the
> > >slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
> > >else. The device would (rightfully) not be recognized as MT since the
> > >position is missing, all data would be available for processing in
> > >userspace, and bandwidth would be minimized since there could only be
> > >so many changes coming in per millisecond.
> > 
> > So how does userspace then finds out where these pressure points are
> > located?
> > Or do you mean to just dump all data to user space (15 * 13 *
> > sizeof(ABS_MT_PRESSURE value) + overhead)?
> 
> Having each pressure point represented by one slot id was the idea.
> Userspace would have to know how the points are mapped, of
> course. Still not overly happy about the general fit, though. Dmitry?

I am having doubts that this device, as it is, is suitable for input
interface; I really do not think that bastardizing slot IDs is good
idea. Unless we move all computation necessary to identify individual
contacts into the kernel (and then use standard MT protocol), I'd
recommend looking into hidraw + uinput solution.

Thanks.

-- 
Dmitry

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Maurus Cuelenaere]

Op 25-01-12 14:26, Henrik Rydberg schreef:
>> +
>> +struct mstm_state {
>> +	bool advance_flag;
>> +	int x;
>> +	int y;
>> +	unsigned char last_timestamp;
>> +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
>> +};
> The ability to simply send an "input screen" would be perfect
> here. This device may be on the border of what can/should be handled
> via input events. A memory mapped driver, uio-based or something
> similar, could be an option.

Op 30-01-12 08:27, Dmitry Torokhov schreef:
> On Wed, Jan 25, 2012 at 04:00:35PM +0100, Henrik Rydberg wrote:
>>
>>>> One possible option could be to use the
>>>> slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
>>>> else. The device would (rightfully) not be recognized as MT since the
>>>> position is missing, all data would be available for processing in
>>>> userspace, and bandwidth would be minimized since there could only be
>>>> so many changes coming in per millisecond.
>>> So how does userspace then finds out where these pressure points are
>>> located?
>>> Or do you mean to just dump all data to user space (15 * 13 *
>>> sizeof(ABS_MT_PRESSURE value) + overhead)?
>> Having each pressure point represented by one slot id was the idea.
>> Userspace would have to know how the points are mapped, of
>> course. Still not overly happy about the general fit, though. Dmitry?
> I am having doubts that this device, as it is, is suitable for input
> interface; I really do not think that bastardizing slot IDs is good
> idea. Unless we move all computation necessary to identify individual
> contacts into the kernel (and then use standard MT protocol), I'd
> recommend looking into hidraw + uinput solution.

Ok, so it's pretty clear the consensus is that there should be a 
different interface to push the data to user space.
My initial reaction is V4L, as the data is basically a stream of 
monochrome frames, this kind of fits the video description. Also, this 
could be of some benefit to all those emulate-multi-touch-using-webcam 
projects.
Other options I see are mmap on the input device, creating a separate 
(char?) device, UIO or even having the whole thing in userspace.

However, there's one tiny hack I'd like to have and I'm unsure whether 
this can be done from userspace: as the mouse has only one physical push 
button, it can't distinguish between left, middle or right click.
Obviously, it can do this based on the information it gathers from its 
touch surface, however the firmware has only the following dumb 
implementation: when only one finger is present on the right part of the 
surface, assume right click; otherwise assume left click. This means 
that you need to lift your left finger when performing a right click.

Now, the receiver is represented as 3 HID devices: one keyboard 
descriptor, one mouse descriptor and one miscellaneous/control 
descriptor (containing touch surface data). My plan was to intercept 
mouse clicks on the mouse descriptor and look at what part of touch 
surface (left/right) has the most pressure and, if needed, swap the 
click to a right click.

Any hints on how to do this in userspace? Of course I could hack up 
xf86-input-evdev, but I'd like to do this at the highest possible layer, 
ie the input subsystem.

-- 
Maurus Cuelenaere

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Henrik Rydberg]

> Ok, so it's pretty clear the consensus is that there should be a
> different interface to push the data to user space.
> My initial reaction is V4L, as the data is basically a stream of
> monochrome frames, this kind of fits the video description. Also,
> this could be of some benefit to all those
> emulate-multi-touch-using-webcam projects.

On the other hand, your device, and the ones to come, are likely not
normal ccd cameras, and the interface is bound to be different; you
would most likely prefer the stream to be silent when there are no
touches, for instance, and only generate interrupts or new-data events
as there is something to react to.

> Other options I see are mmap on the input device, creating a
> separate (char?) device, UIO or even having the whole thing in
> userspace.

Personally, I think mmapped input devices would be a great extension
to the input subsystem. I am thinking event types distributed in a 2D
plane, maybe one node per type, and signalled by normal input events
of the data-to-read type.

> However, there's one tiny hack I'd like to have and I'm unsure
> whether this can be done from userspace: as the mouse has only one
> physical push button, it can't distinguish between left, middle or
> right click.
> Obviously, it can do this based on the information it gathers from
> its touch surface, however the firmware has only the following dumb
> implementation: when only one finger is present on the right part of
> the surface, assume right click; otherwise assume left click. This
> means that you need to lift your left finger when performing a right
> click.
> 
> Now, the receiver is represented as 3 HID devices: one keyboard
> descriptor, one mouse descriptor and one miscellaneous/control
> descriptor (containing touch surface data). My plan was to intercept
> mouse clicks on the mouse descriptor and look at what part of touch
> surface (left/right) has the most pressure and, if needed, swap the
> click to a right click.
> 
> Any hints on how to do this in userspace? Of course I could hack up
> xf86-input-evdev, but I'd like to do this at the highest possible
> layer, ie the input subsystem.

The application you describe could well be implemented in the driver
you submitted; a simple handling of the MT data resulting in normal
input events.

Thanks,
Henrik

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Maurus Cuelenaere]

Op 07-02-12 17:12, Henrik Rydberg schreef:
>> Ok, so it's pretty clear the consensus is that there should be a
>> different interface to push the data to user space.
>> My initial reaction is V4L, as the data is basically a stream of
>> monochrome frames, this kind of fits the video description. Also,
>> this could be of some benefit to all those
>> emulate-multi-touch-using-webcam projects.
> On the other hand, your device, and the ones to come, are likely not
> normal ccd cameras, and the interface is bound to be different; you
> would most likely prefer the stream to be silent when there are no
> touches, for instance, and only generate interrupts or new-data events
> as there is something to react to.

True, I had forgotten about that. There are no HID packets sent when 
nothing touches the surface and the CPU shouldn't needlessly spent 
cycles on parsing an empty stream.

>> Other options I see are mmap on the input device, creating a
>> separate (char?) device, UIO or even having the whole thing in
>> userspace.
> Personally, I think mmapped input devices would be a great extension
> to the input subsystem. I am thinking event types distributed in a 2D
> plane, maybe one node per type, and signalled by normal input events
> of the data-to-read type.

Ok, I'll look into that!

>> However, there's one tiny hack I'd like to have and I'm unsure
>> whether this can be done from userspace: as the mouse has only one
>> physical push button, it can't distinguish between left, middle or
>> right click.
>> Obviously, it can do this based on the information it gathers from
>> its touch surface, however the firmware has only the following dumb
>> implementation: when only one finger is present on the right part of
>> the surface, assume right click; otherwise assume left click. This
>> means that you need to lift your left finger when performing a right
>> click.
>>
>> Now, the receiver is represented as 3 HID devices: one keyboard
>> descriptor, one mouse descriptor and one miscellaneous/control
>> descriptor (containing touch surface data). My plan was to intercept
>> mouse clicks on the mouse descriptor and look at what part of touch
>> surface (left/right) has the most pressure and, if needed, swap the
>> click to a right click.
>>
>> Any hints on how to do this in userspace? Of course I could hack up
>> xf86-input-evdev, but I'd like to do this at the highest possible
>> layer, ie the input subsystem.
> The application you describe could well be implemented in the driver
> you submitted; a simple handling of the MT data resulting in normal
> input events.

Yes, that was my initial idea. But as I had the impression that an 
userspace solution was preferred, I wondered how to solve that.

-- 
Maurus Cuelenaere

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Henrik Rydberg]

> >>Any hints on how to do this in userspace? Of course I could hack up
> >>xf86-input-evdev, but I'd like to do this at the highest possible
> >>layer, ie the input subsystem.
> >The application you describe could well be implemented in the driver
> >you submitted; a simple handling of the MT data resulting in normal
> >input events.
> 
> Yes, that was my initial idea. But as I had the impression that an
> userspace solution was preferred, I wondered how to solve that.

The conclusion was that as is, the device is unsuitable as an MT
device, needing either further in-kernel conversion or some other,
non-MT way of getting the data to userspace. Converting the data as
you describe, outputting normal input events, falls into the first
category.

Thanks,
Henrik

Re: [RFC] Microsoft Touch Mouse driver [was: Re: your mail]

[Dmitry Torokhov]

On Tue, Feb 07, 2012 at 11:38:24PM +0100, Henrik Rydberg wrote:
> > >>Any hints on how to do this in userspace? Of course I could hack up
> > >>xf86-input-evdev, but I'd like to do this at the highest possible
> > >>layer, ie the input subsystem.
> > >The application you describe could well be implemented in the driver
> > >you submitted; a simple handling of the MT data resulting in normal
> > >input events.
> > 
> > Yes, that was my initial idea. But as I had the impression that an
> > userspace solution was preferred, I wondered how to solve that.
> 
> The conclusion was that as is, the device is unsuitable as an MT
> device, needing either further in-kernel conversion or some other,
> non-MT way of getting the data to userspace. Converting the data as
> you describe, outputting normal input events, falls into the first
> category.

It should be possible to grab both interfaces using hidraw, perform
necessary decoding in userspace and route proper MT stream (including
button data) back into the kernel via uinput. This way the rest of the
stack doe not need to be touched.

Thanks.

-- 
Dmitry