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@xxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> Reviewed-by: Dmitry Torokhov <dtor@xxxxxxxxxxxx>
> >> ---
> >>  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