[PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Wei-Ning Huang]

From: Wei-Ning Huang <wnhuang@chromium.org>

The current hid-multitouch driver only allow the report of two
orientations, vertical and horizontal. We use the Azimuth orientation
usage 0x3F under the Digitizer usage page to report orientation if the
device supports it.

Changelog:
  v1 -> v2:
   - Fix commit message.
   - Remove resolution reporting for ABS_MT_ORIENTATION.
  v2 -> v3:
   - Fix commit message.
  v3 -> v4:
   - Fix ABS_MT_ORIENTATION ABS param range.
   - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
     set by ABS_DG_AZIMUTH.

Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/hid/hid-multitouch.c | 52 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/hid.h          |  1 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 440b999304a5..3317dae64ef7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
 #define MT_IO_FLAGS_PENDING_SLOTS	2
 
 struct mt_slot {
-	__s32 x, y, cx, cy, p, w, h;
+	__s32 x, y, cx, cy, p, w, h, a;
 	__s32 contactid;	/* the device ContactID assigned to this slot */
 	bool touch_state;	/* is the touch valid? */
 	bool inrange_state;	/* is the finger in proximity of the sensor? */
 	bool confidence_state;  /* is the touch made by a finger? */
+	bool has_azimuth;       /* the contact reports azimuth */
 };
 
 struct mt_class {
@@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
 				set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
 					cls->sn_height);
-				input_set_abs_params(hi->input,
-					ABS_MT_ORIENTATION, 0, 1, 0, 0);
+
+				/*
+				 * Only set ABS_MT_ORIENTATION if it is not
+				 * already set by the HID_DG_AZIMUTH usage.
+				 */
+				if (!test_bit(ABS_MT_ORIENTATION,
+						hi->input->absbit))
+					input_set_abs_params(hi->input,
+						ABS_MT_ORIENTATION, 0, 1, 0, 0);
 			}
 			mt_store_field(usage, td, hi);
 			return 1;
@@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			td->cc_index = field->index;
 			td->cc_value_index = usage->usage_index;
 			return 1;
+		case HID_DG_AZIMUTH:
+			hid_map_usage(hi, usage, bit, max,
+				EV_ABS, ABS_MT_ORIENTATION);
+			/*
+			 * Azimuth has the range of [0, MAX) representing a full
+			 * revolution. Set ABS_MT_ORIENTATION to a quarter of
+			 * MAX according the definition of ABS_MT_ORIENTATION
+			 */
+			input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
+				-field->logical_maximum / 4,
+				field->logical_maximum / 4,
+				cls->sn_move ?
+				field->logical_maximum / cls->sn_move : 0, 0);
+			mt_store_field(usage, td, hi);
+			return 1;
 		case HID_DG_CONTACTMAX:
 			/* we don't set td->last_slot_field as contactcount and
 			 * contact max are global to the report */
@@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 			int wide = (s->w > s->h);
 			int major = max(s->w, s->h);
 			int minor = min(s->w, s->h);
+			int orientation = wide;
+
+			if (s->has_azimuth)
+				orientation = s->a;
 
 			/*
 			 * divided by two to match visual scale of touch
@@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 			input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
 			input_event(input, EV_ABS, ABS_MT_DISTANCE,
 				!s->touch_state);
-			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
+			input_event(input, EV_ABS, ABS_MT_ORIENTATION,
+				orientation);
 			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
@@ -789,6 +817,22 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
 			break;
 		case HID_DG_CONTACTCOUNT:
 			break;
+		case HID_DG_AZIMUTH:
+			/*
+			 * Azimuth is counter-clockwise and ranges from [0, MAX)
+			 * (a full revolution). Convert it to clockwise ranging
+			 * [-MAX/2, MAX/2].
+			 *
+			 * Note that ABS_MT_ORIENTATION require us to report
+			 * the limit of [-MAX/4, MAX/4], but the value can go
+			 * out of range to [-MAX/2, MAX/2] to report an upside
+			 * down ellipsis.
+			 */
+			if (value > field->logical_maximum / 2)
+				value -= field->logical_maximum;
+			td->curdata.a = -value;
+			td->curdata.has_azimuth = true;
+			break;
 		case HID_DG_TOUCH:
 			/* do nothing */
 			break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ab05a86269dc..d81b9b6fd83a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -281,6 +281,7 @@ struct hid_item {
 
 #define HID_DG_DEVICECONFIG	0x000d000e
 #define HID_DG_DEVICESETTINGS	0x000d0023
+#define HID_DG_AZIMUTH		0x000d003f
 #define HID_DG_CONFIDENCE	0x000d0047
 #define HID_DG_WIDTH		0x000d0048
 #define HID_DG_HEIGHT		0x000d0049
-- 
2.12.2

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Henrik Rydberg]

Hi Wei-Ning,

> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>    - Fix commit message.
>    - Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>    - Fix commit message.
>   v3 -> v4:
>    - Fix ABS_MT_ORIENTATION ABS param range.
>    - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>      set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> ---
>  drivers/hid/hid-multitouch.c | 52 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/hid.h          |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS	2
>  
>  struct mt_slot {
> -	__s32 x, y, cx, cy, p, w, h;
> +	__s32 x, y, cx, cy, p, w, h, a;
>  	__s32 contactid;	/* the device ContactID assigned to this slot */
>  	bool touch_state;	/* is the touch valid? */
>  	bool inrange_state;	/* is the finger in proximity of the sensor? */
>  	bool confidence_state;  /* is the touch made by a finger? */
> +	bool has_azimuth;       /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>  				set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>  					cls->sn_height);
> -				input_set_abs_params(hi->input,
> -					ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> +				/*
> +				 * Only set ABS_MT_ORIENTATION if it is not
> +				 * already set by the HID_DG_AZIMUTH usage.
> +				 */
> +				if (!test_bit(ABS_MT_ORIENTATION,
> +						hi->input->absbit))
> +					input_set_abs_params(hi->input,
> +						ABS_MT_ORIENTATION, 0, 1, 0, 0);
>  			}
>  			mt_store_field(usage, td, hi);
>  			return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			td->cc_index = field->index;
>  			td->cc_value_index = usage->usage_index;
>  			return 1;
> +		case HID_DG_AZIMUTH:
> +			hid_map_usage(hi, usage, bit, max,
> +				EV_ABS, ABS_MT_ORIENTATION);
> +			/*
> +			 * Azimuth has the range of [0, MAX) representing a full
> +			 * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +			 * MAX according the definition of ABS_MT_ORIENTATION
> +			 */
> +			input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> +				-field->logical_maximum / 4,
> +				field->logical_maximum / 4,
> +				cls->sn_move ?
> +				field->logical_maximum / cls->sn_move : 0, 0);
> +			mt_store_field(usage, td, hi);
> +			return 1;
>  		case HID_DG_CONTACTMAX:
>  			/* we don't set td->last_slot_field as contactcount and
>  			 * contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  			int wide = (s->w > s->h);
>  			int major = max(s->w, s->h);
>  			int minor = min(s->w, s->h);
> +			int orientation = wide;
> +
> +			if (s->has_azimuth)
> +				orientation = s->a;
>  
>  			/*
>  			 * divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  			input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>  			input_event(input, EV_ABS, ABS_MT_DISTANCE,
>  				!s->touch_state);
> -			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> +			input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> +				orientation);
>  			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> @@ -789,6 +817,22 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>  			break;
>  		case HID_DG_CONTACTCOUNT:
>  			break;
> +		case HID_DG_AZIMUTH:
> +			/*
> +			 * Azimuth is counter-clockwise and ranges from [0, MAX)
> +			 * (a full revolution). Convert it to clockwise ranging
> +			 * [-MAX/2, MAX/2].
> +			 *
> +			 * Note that ABS_MT_ORIENTATION require us to report
> +			 * the limit of [-MAX/4, MAX/4], but the value can go
> +			 * out of range to [-MAX/2, MAX/2] to report an upside
> +			 * down ellipsis.
> +			 */
> +			if (value > field->logical_maximum / 2)
> +				value -= field->logical_maximum;
> +			td->curdata.a = -value;
> +			td->curdata.has_azimuth = true;
> +			break;
>  		case HID_DG_TOUCH:
>  			/* do nothing */
>  			break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86269dc..d81b9b6fd83a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -281,6 +281,7 @@ struct hid_item {
>  
>  #define HID_DG_DEVICECONFIG	0x000d000e
>  #define HID_DG_DEVICESETTINGS	0x000d0023
> +#define HID_DG_AZIMUTH		0x000d003f
>  #define HID_DG_CONFIDENCE	0x000d0047
>  #define HID_DG_WIDTH		0x000d0048
>  #define HID_DG_HEIGHT		0x000d0049
> -- 
> 2.12.2
> 

Acked-by: Henrik Rydberg <rydberg@bitmath.org>

This version looks good - thank you Wei-Ning, thank you Benjamin.

Henrik

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Benjamin Tissoires]

On Oct 10 2017 or thereabouts, Wei-Ning Huang wrote:
> From: Wei-Ning Huang <wnhuang@chromium.org>
> 
> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>    - Fix commit message.
>    - Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>    - Fix commit message.
>   v3 -> v4:
>    - Fix ABS_MT_ORIENTATION ABS param range.
>    - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>      set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>

Looks good now:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> ---
>  drivers/hid/hid-multitouch.c | 52 ++++++++++++++++++++++++++++++++++++++++----
>  include/linux/hid.h          |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS	2
>  
>  struct mt_slot {
> -	__s32 x, y, cx, cy, p, w, h;
> +	__s32 x, y, cx, cy, p, w, h, a;
>  	__s32 contactid;	/* the device ContactID assigned to this slot */
>  	bool touch_state;	/* is the touch valid? */
>  	bool inrange_state;	/* is the finger in proximity of the sensor? */
>  	bool confidence_state;  /* is the touch made by a finger? */
> +	bool has_azimuth;       /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>  				set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>  					cls->sn_height);
> -				input_set_abs_params(hi->input,
> -					ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> +				/*
> +				 * Only set ABS_MT_ORIENTATION if it is not
> +				 * already set by the HID_DG_AZIMUTH usage.
> +				 */
> +				if (!test_bit(ABS_MT_ORIENTATION,
> +						hi->input->absbit))
> +					input_set_abs_params(hi->input,
> +						ABS_MT_ORIENTATION, 0, 1, 0, 0);
>  			}
>  			mt_store_field(usage, td, hi);
>  			return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			td->cc_index = field->index;
>  			td->cc_value_index = usage->usage_index;
>  			return 1;
> +		case HID_DG_AZIMUTH:
> +			hid_map_usage(hi, usage, bit, max,
> +				EV_ABS, ABS_MT_ORIENTATION);
> +			/*
> +			 * Azimuth has the range of [0, MAX) representing a full
> +			 * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +			 * MAX according the definition of ABS_MT_ORIENTATION
> +			 */
> +			input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> +				-field->logical_maximum / 4,
> +				field->logical_maximum / 4,
> +				cls->sn_move ?
> +				field->logical_maximum / cls->sn_move : 0, 0);
> +			mt_store_field(usage, td, hi);
> +			return 1;
>  		case HID_DG_CONTACTMAX:
>  			/* we don't set td->last_slot_field as contactcount and
>  			 * contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  			int wide = (s->w > s->h);
>  			int major = max(s->w, s->h);
>  			int minor = min(s->w, s->h);
> +			int orientation = wide;
> +
> +			if (s->has_azimuth)
> +				orientation = s->a;
>  
>  			/*
>  			 * divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  			input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>  			input_event(input, EV_ABS, ABS_MT_DISTANCE,
>  				!s->touch_state);
> -			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> +			input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> +				orientation);
>  			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> @@ -789,6 +817,22 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>  			break;
>  		case HID_DG_CONTACTCOUNT:
>  			break;
> +		case HID_DG_AZIMUTH:
> +			/*
> +			 * Azimuth is counter-clockwise and ranges from [0, MAX)
> +			 * (a full revolution). Convert it to clockwise ranging
> +			 * [-MAX/2, MAX/2].
> +			 *
> +			 * Note that ABS_MT_ORIENTATION require us to report
> +			 * the limit of [-MAX/4, MAX/4], but the value can go
> +			 * out of range to [-MAX/2, MAX/2] to report an upside
> +			 * down ellipsis.
> +			 */
> +			if (value > field->logical_maximum / 2)
> +				value -= field->logical_maximum;
> +			td->curdata.a = -value;
> +			td->curdata.has_azimuth = true;
> +			break;
>  		case HID_DG_TOUCH:
>  			/* do nothing */
>  			break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86269dc..d81b9b6fd83a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -281,6 +281,7 @@ struct hid_item {
>  
>  #define HID_DG_DEVICECONFIG	0x000d000e
>  #define HID_DG_DEVICESETTINGS	0x000d0023
> +#define HID_DG_AZIMUTH		0x000d003f
>  #define HID_DG_CONFIDENCE	0x000d0047
>  #define HID_DG_WIDTH		0x000d0048
>  #define HID_DG_HEIGHT		0x000d0049
> -- 
> 2.12.2
> 

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Dmitry Torokhov]

On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Wei-Ning,
>
>> The current hid-multitouch driver only allow the report of two
>> orientations, vertical and horizontal. We use the Azimuth orientation
>> usage 0x3F under the Digitizer usage page to report orientation if the
>> device supports it.
>>
>> Changelog:
>>   v1 -> v2:
>>    - Fix commit message.
>>    - Remove resolution reporting for ABS_MT_ORIENTATION.
>>   v2 -> v3:
>>    - Fix commit message.
>>   v3 -> v4:
>>    - Fix ABS_MT_ORIENTATION ABS param range.
>>    - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>>      set by ABS_DG_AZIMUTH.
>>
>> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
>> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
>> ---
>>  drivers/hid/hid-multitouch.c | 52 ++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/hid.h          |  1 +
>>  2 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 440b999304a5..3317dae64ef7 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>>  #define MT_IO_FLAGS_PENDING_SLOTS    2
>>
>>  struct mt_slot {
>> -     __s32 x, y, cx, cy, p, w, h;
>> +     __s32 x, y, cx, cy, p, w, h, a;
>>       __s32 contactid;        /* the device ContactID assigned to this slot */
>>       bool touch_state;       /* is the touch valid? */
>>       bool inrange_state;     /* is the finger in proximity of the sensor? */
>>       bool confidence_state;  /* is the touch made by a finger? */
>> +     bool has_azimuth;       /* the contact reports azimuth */
>>  };
>>
>>  struct mt_class {
>> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                       if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>>                               set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>>                                       cls->sn_height);
>> -                             input_set_abs_params(hi->input,
>> -                                     ABS_MT_ORIENTATION, 0, 1, 0, 0);
>> +
>> +                             /*
>> +                              * Only set ABS_MT_ORIENTATION if it is not
>> +                              * already set by the HID_DG_AZIMUTH usage.
>> +                              */
>> +                             if (!test_bit(ABS_MT_ORIENTATION,
>> +                                             hi->input->absbit))
>> +                                     input_set_abs_params(hi->input,
>> +                                             ABS_MT_ORIENTATION, 0, 1, 0, 0);
>>                       }
>>                       mt_store_field(usage, td, hi);
>>                       return 1;
>> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                       td->cc_index = field->index;
>>                       td->cc_value_index = usage->usage_index;
>>                       return 1;
>> +             case HID_DG_AZIMUTH:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                             EV_ABS, ABS_MT_ORIENTATION);
>> +                     /*
>> +                      * Azimuth has the range of [0, MAX) representing a full
>> +                      * revolution. Set ABS_MT_ORIENTATION to a quarter of
>> +                      * MAX according the definition of ABS_MT_ORIENTATION
>> +                      */
>> +                     input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
>> +                             -field->logical_maximum / 4,
>> +                             field->logical_maximum / 4,


So do we expect userspace to have special handling for the range when
"min" is negative? I.e. when range is [0,1] it is understood that we
are reporting the first quadrant only, with 0 reported when finger is
roughly vertical and 1 is when it is horizontal. Similarly, the range
[0-90] would describe the 1st quadrant as well. This matches the
in-kernel documentation. However here we have [-90, 90] that describes
2 quadrants. Do we want to keep it as is and have userspace adapt (we
probably will need a patch to multi-touch-protocol.txt), or should
this be:

input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0,
field->logical_maximum / 4, ...);

and userspace should be able to handle reported negative events and
have them being understood as going counter-clockwise into the 4th and
then 3rd quadrant?

Thanks,
Dmitry

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Benjamin Tissoires]

On Oct 10 2017 or thereabouts, Dmitry Torokhov wrote:
> On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> > Hi Wei-Ning,
> >
> >> The current hid-multitouch driver only allow the report of two
> >> orientations, vertical and horizontal. We use the Azimuth orientation
> >> usage 0x3F under the Digitizer usage page to report orientation if the
> >> device supports it.
> >>
> >> Changelog:
> >>   v1 -> v2:
> >>    - Fix commit message.
> >>    - Remove resolution reporting for ABS_MT_ORIENTATION.
> >>   v2 -> v3:
> >>    - Fix commit message.
> >>   v3 -> v4:
> >>    - Fix ABS_MT_ORIENTATION ABS param range.
> >>    - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
> >>      set by ABS_DG_AZIMUTH.
> >>
> >> Signed-off-by: Wei-Ning Huang <wnhuang@chromium.org>
> >> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> >> ---
> >>  drivers/hid/hid-multitouch.c | 52 ++++++++++++++++++++++++++++++++++++++++----
> >>  include/linux/hid.h          |  1 +
> >>  2 files changed, 49 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index 440b999304a5..3317dae64ef7 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
> >>  #define MT_IO_FLAGS_PENDING_SLOTS    2
> >>
> >>  struct mt_slot {
> >> -     __s32 x, y, cx, cy, p, w, h;
> >> +     __s32 x, y, cx, cy, p, w, h, a;
> >>       __s32 contactid;        /* the device ContactID assigned to this slot */
> >>       bool touch_state;       /* is the touch valid? */
> >>       bool inrange_state;     /* is the finger in proximity of the sensor? */
> >>       bool confidence_state;  /* is the touch made by a finger? */
> >> +     bool has_azimuth;       /* the contact reports azimuth */
> >>  };
> >>
> >>  struct mt_class {
> >> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> >>                       if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
> >>                               set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
> >>                                       cls->sn_height);
> >> -                             input_set_abs_params(hi->input,
> >> -                                     ABS_MT_ORIENTATION, 0, 1, 0, 0);
> >> +
> >> +                             /*
> >> +                              * Only set ABS_MT_ORIENTATION if it is not
> >> +                              * already set by the HID_DG_AZIMUTH usage.
> >> +                              */
> >> +                             if (!test_bit(ABS_MT_ORIENTATION,
> >> +                                             hi->input->absbit))
> >> +                                     input_set_abs_params(hi->input,
> >> +                                             ABS_MT_ORIENTATION, 0, 1, 0, 0);
> >>                       }
> >>                       mt_store_field(usage, td, hi);
> >>                       return 1;
> >> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> >>                       td->cc_index = field->index;
> >>                       td->cc_value_index = usage->usage_index;
> >>                       return 1;
> >> +             case HID_DG_AZIMUTH:
> >> +                     hid_map_usage(hi, usage, bit, max,
> >> +                             EV_ABS, ABS_MT_ORIENTATION);
> >> +                     /*
> >> +                      * Azimuth has the range of [0, MAX) representing a full
> >> +                      * revolution. Set ABS_MT_ORIENTATION to a quarter of
> >> +                      * MAX according the definition of ABS_MT_ORIENTATION
> >> +                      */
> >> +                     input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> >> +                             -field->logical_maximum / 4,
> >> +                             field->logical_maximum / 4,
> 
> 
> So do we expect userspace to have special handling for the range when
> "min" is negative? I.e. when range is [0,1] it is understood that we
> are reporting the first quadrant only, with 0 reported when finger is
> roughly vertical and 1 is when it is horizontal. Similarly, the range
> [0-90] would describe the 1st quadrant as well. This matches the
> in-kernel documentation. However here we have [-90, 90] that describes
> 2 quadrants. Do we want to keep it as is and have userspace adapt (we
> probably will need a patch to multi-touch-protocol.txt), or should

Actually, I requested the [-90, 90] change because the only other user
in the kernel I could find that support ABS_MT_ORIENTATION with a
different range than [0,1] was hid-magicmouse and it was not following
the documentation to the letter:
https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git/tree/drivers/hid/hid-magicmouse.c?h=for-next#n411


After a deeper search, we have (reporting ABS_MT_ORIENTATION different
from [0,1]):
drivers/hid/hid-magicmouse.c -> [-32,32]
drivers/input/touchscreen/stmfts.c -> [0,255]
drivers/input/touchscreen/atmel_mxt_ts.c -> [0,255]
drivers/input/mouse/bcm5974.c -> [-16384, 16384]
drivers/input/mouse/cyapa.c -> [-127, 127]
drivers/input/misc/xen-kbdfront.c -> ???? (set_abs_params is not even
                                     called)

So we clearly already have messed up everywhere. I suspect the [0,255]
ranges to be the min/max already reported by the touchscreen.

I am not sure if libinput even uses ABS_MT_ORIENTATION, but I'd go for
fixing the documentation. And re-reading it, it's not clear that the doc
tells us to have [0,90]. It mentions negative values and out of ranges
too, so we might just as well simply clarify that we rather have [-90,90],
with 0 being "north".

Cheers,
Benjamin

> this be:
> 
> input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0,
> field->logical_maximum / 4, ...);
> 
> and userspace should be able to handle reported negative events and
> have them being understood as going counter-clockwise into the 4th and
> then 3rd quadrant?
> 
> Thanks,
> Dmitry

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Jiri Kosina]

On Wed, 11 Oct 2017, Benjamin Tissoires wrote:

> I am not sure if libinput even uses ABS_MT_ORIENTATION

I don't think it does, so that should be okay. However ...

> , but I'd go for fixing the documentation. And re-reading it, it's not 
> clear that the doc tells us to have [0,90]. It mentions negative values 
> and out of ranges too, so we might just as well simply clarify that we 
> rather have [-90,90], with 0 being "north".

... I'd like the documentation fix to go in together in one go with this 
patch if possible.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Benjamin Tissoires]

On Oct 11 2017 or thereabouts, Jiri Kosina wrote:
> On Wed, 11 Oct 2017, Benjamin Tissoires wrote:
> 
> > I am not sure if libinput even uses ABS_MT_ORIENTATION
> 
> I don't think it does, so that should be okay. However ...

I had a meeting this Peter at noon today. The summary is that libinput
doesn't uses ABS_MT_ORIENTATION, and that the documentation requires
actually 3 things:
- 0 is Y-aligned, up ("north")
- maximum should be aligned with X, pointing toward the right ("east")
- negative and out of range values are allowed

>From this, we can conclude that the minimum doesn't matter, as long as
it is 0 or -max, it is the same from the user space point of view.

One thing that the documentation suggests is that if we report [0, max],
this would indicate that out of ranges values won't be triggered, and
[-max, max] would seem to indicate that the data might be negative and
so out of range values would be acceptable.

Anyway, no changes in any cases from userspace.

> 
> > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > clear that the doc tells us to have [0,90]. It mentions negative values 
> > and out of ranges too, so we might just as well simply clarify that we 
> > rather have [-90,90], with 0 being "north".
> 
> ... I'd like the documentation fix to go in together in one go with this 
> patch if possible.
> 

Sounds like a plan.

Cheers,
Benjamin

> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Henrik Rydberg]

> > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > and out of ranges too, so we might just as well simply clarify that we 
> > > rather have [-90,90], with 0 being "north".
> > 
> > ... I'd like the documentation fix to go in together in one go with this 
> > patch if possible.
> > 
> 
> Sounds like a plan.

How about this patch?

Henrik

---

>From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@bitmath.org>
Date: Wed, 11 Oct 2017 22:41:39 +0200
Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION

As more drivers start to support touch orientation, clarify how the
value range should be set to match the expected behavior in
userland.

Signed-off-by: Henrik Rydberg <rydberg@bitmath.org>
---
 Documentation/input/multi-touch-protocol.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/input/multi-touch-protocol.rst b/Documentation/input/multi-touch-protocol.rst
index 8035868..a0c5c03 100644
--- a/Documentation/input/multi-touch-protocol.rst
+++ b/Documentation/input/multi-touch-protocol.rst
@@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
     The orientation of the touching ellipse. The value should describe a signed
     quarter of a revolution clockwise around the touch center. The signed value
     range is arbitrary, but zero should be returned for an ellipse aligned with
-    the Y axis of the surface, a negative value when the ellipse is turned to
-    the left, and a positive value when the ellipse is turned to the
-    right. When completely aligned with the X axis, the range max should be
-    returned.
+    the Y axis of the surface (north). A negative value should be returned when
+    the ellipse is turned to the left (west), with the smallest value reported
+    when aligned with the negative X axis. The largest value should be returned
+    when aligned with the positive X axis.
+
+    The value range should be specified as [-range_max, range_max].
 
     Touch ellipsis are symmetrical by default. For devices capable of true 360
-    degree orientation, the reported orientation must exceed the range max to
+    degree orientation, the reported orientation will exceed range_max, in order to
     indicate more than a quarter of a revolution. For an upside-down finger,
-    range max * 2 should be returned.
+    +- 2 * range_max should be returned.
 
     Orientation can be omitted if the touch area is circular, or if the
     information is not available in the kernel driver. Partial orientation
-- 
2.14.1

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

[Peter Hutterer]

On Wed, Oct 11, 2017 at 10:43:24PM +0200, Henrik Rydberg wrote:
> > > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > > and out of ranges too, so we might just as well simply clarify that we 
> > > > rather have [-90,90], with 0 being "north".
> > > 
> > > ... I'd like the documentation fix to go in together in one go with this 
> > > patch if possible.
> > > 
> > 
> > Sounds like a plan.
> 
> How about this patch?
> 
> Henrik
> 
> ---
> 
> From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@bitmath.org>
> Date: Wed, 11 Oct 2017 22:41:39 +0200
> Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION
> 
> As more drivers start to support touch orientation, clarify how the
> value range should be set to match the expected behavior in
> userland.
> 
> Signed-off-by: Henrik Rydberg <rydberg@bitmath.org>
> ---
>  Documentation/input/multi-touch-protocol.rst | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/input/multi-touch-protocol.rst b/Documentation/input/multi-touch-protocol.rst
> index 8035868..a0c5c03 100644
> --- a/Documentation/input/multi-touch-protocol.rst
> +++ b/Documentation/input/multi-touch-protocol.rst
> @@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
>      The orientation of the touching ellipse. The value should describe a signed
>      quarter of a revolution clockwise around the touch center. The signed value
>      range is arbitrary, but zero should be returned for an ellipse aligned with
> -    the Y axis of the surface, a negative value when the ellipse is turned to
> -    the left, and a positive value when the ellipse is turned to the
> -    right. When completely aligned with the X axis, the range max should be
> -    returned.
> +    the Y axis of the surface (north). A negative value should be returned when
> +    the ellipse is turned to the left (west), 

this bit is good.


> +    with the smallest value reported
> +    when aligned with the negative X axis. The largest value should be returned
> +    when aligned with the positive X axis.

this bit is less precise than before, because 'smallest' doesn't mean
'minimum' but smallest value. so a device announcing -90,90 could think that
returning -120 is a good idea for X alignment. I liked the previous "When
completely aligned with the X axis, the range max should be returned.", it
was less ambiguous.

replacing 'smallest value'/'largest value' with -range_max/range_max should
be sufficient here.


> +
> +    The value range should be specified as [-range_max, range_max].

I'd really like this to be [0, max] == can only detect one quarter/half and
[-max, +max] == can detect both directions. It's one extra bit of
information that may come in useful at some point.

>      Touch ellipsis are symmetrical by default. For devices capable of true 360
> -    degree orientation, the reported orientation must exceed the range max to
> +    degree orientation, the reported orientation will exceed range_max, in order to
>      indicate more than a quarter of a revolution. For an upside-down finger,
> -    range max * 2 should be returned.
> +    +- 2 * range_max should be returned.

ack to this bit

Cheers,
   Peter

>  
>      Orientation can be omitted if the touch area is circular, or if the
>      information is not available in the kernel driver. Partial orientation
> -- 
> 2.14.1
>