Re: [PATCH] iio/hid: Add mount_matrix

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/25/22 14:40, Hans de Goede wrote:
> Hi,
> 
> On 6/25/22 14:33, Hans de Goede wrote:
>> Hi,
>>
>> Jonathan, thanks for Cc-ing me on this.
>>
>> On 6/25/22 13:09, Jonathan Cameron wrote:
>>> On Fri, 24 Jun 2022 15:33:41 -0700
>>> Gwendal Grignou <gwendal@chromium.org> wrote:
>>>
>>>> ISH based sensors do not naturally return data in the W3C 'natural'
>>>> orientation.
>>>> They returns all data inverted, to match Microsoft Windows requirement:
>>>> [https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/sensors#accelerometer]
>>>> """ If the device has the SimpleOrientation of FaceUp on a table, then
>>>> the accelerometer would read -1 G on the Z axis. """
>>>
>>> Probably reference the HID Usage Tables 1.3 spec rather than the MS one.
>>> https://usb.org/sites/default/files/hut1_3_0.pdf
>>> After some waving around of my left and right hand I'm fairly sure that says the same
>>> thing as the MS spec. Section 4.4 Vector Usages 
>>>
>>>> While W3C defines [https://www.w3.org/TR/motion-sensors/#accelerometer-sensor]
>>>> """The Accelerometer sensor is an inertial-frame sensor, this means that
>>>> when the device is in free fall, the acceleration is 0 m/s2 in the
>>>> falling direction, and when a device is laying flat on a table, the
>>>> acceleration in upwards direction will be equal to the Earth gravity,
>>>> i.e. g ≡ 9.8 m/s2 as it is measuring the force of the table pushing the
>>>> device upwards."""
>>>>
>>>> Fixes all HID sensors that defines IIO_MOD_[XYZ] attributes.
>>>>
>>>> Tested on "HP Spectre x360 Convertible 13" and "Dell XPS 13 9365".
>>>>
>>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>>
>>> Ah.  Whilst this is a fix, it seems likely to break a whole bunch of existing
>>> users that are compensating for the wrong orientation in userspace.  Also, do
>>> we know how universal this is?  I have a nasty feeling it'll turn out some
>>> HID sensors do it the other way whatever the spec says.  Bastien, are you
>>> carrying a local fix for this in your userspace code?
>>>
>>> +CC a few people who are likely to know more on just how bad that will be...
>>
>> Right, so Linux userspace expects an axis system similar to the Android one,
>> which is actually the one which seems to be described here.
>>
>> The axis system expect is that when a tablet is layed flat on the table,
>> the x and y axis are as one would expect when drawing a mathematics
>> graph on the surface of the tablet.
>>
>> So X-axis goes from left to right, with left side being lower numbers and
>> right side higher numbers.
>>
>> And Y-axis goes from bottom to top, with the bottom being lower numbers and
>> the top higher numbers.
>>
>> That leaves the Z-axis which comes out of the screen at a 90° angle (to both
>> the X and Y axis) and the vector coming out of the screen towards to the user /
>> observer of the screen indicates positive numbers where as imagining the same
>> axis pointing down through the table on which the tables is lying towards
>> the floor represents negative numbers.
>>
>> This means that the accel values of a tablet resting on a table, expresses
>> in units of 1G are: [ 0, 0, -1 ] and I've seen quite a few HID sensors
>> with accel reporting on various devices and they all adhere to this
>> without applying any accel matrix. Or in other words, HID sensors behave
>> as expected by userspace when applying the norm matrix of:
>>
>> 	.rotation = {
>> 		"1", "0", "0",
>> 		"0", "1", "0",
>> 		"0", "0", "1"
>>
>> And this patch will cause the image to be upside down (no matter what the
>> rotation) when using auto-rotation with iio-sensor-proxy.
>>
>> So big NACK from me for this patch.
>>
>> I'm not sure what this patch is trying to fix but it looks to me like it
>> is a bug in the HID sensors implementation of the specific device.
>>
>> Again HID-sensors already work fine on tons of existing devices without
>> any matrix getting applied.
>>
>> Merging this patch would break existing userspace on tons of devices!
> 
> p.s.
> 
> Also I must say that the W3C specification is frankly
> quite silly. The are talking about the counter force of the
> table but that cancels out the actual  acceleration force of
> the gravity, resulting in an effective force of 0.
> 
> Take away the table and the tablet would accelerate
> at 9.8 m/s² towards the earth, so down, so -1 G .
> 
> The HID / Android (and AFAIK also Windows) definitions of the
> sensor axis are much more sensible.
> 
> If chromeos / chrome the browser wants to report accel values
> to web-apps in the silly W3C format then any conversion matrix
> for that should be applied inside chrome-the-browser.
> 
> Note that this suggested patch would like also break the
> accel values of the Android-compatibility inside Chrome OS!!

Doing some further reading it seems this patch is based
on a wrong reading of the W3C spec:

https://w3c.github.io/accelerometer/#acceleration

Has a nice figure which shows the axis exactly as I
have described them and this is what HID is providing
already.

When in rest on a flat surface we will have 1G pointing
down, so along the Z axis coming out of the bottom
of the phone, which is marked as being -Z .

Regards.

Hans




>>> One other thing inline.  The mount matrix you've provided isn't a rotation matrix.
>>>
>>> I'd forgotten the annoyance of graphics folks using the right handed sensor
>>> axis whilst nearly all other uses are left handed. It drove me mad many years
>>> ago - every code base that used sensors and rendered the result needed a
>>> flip of the z axis - it was never well documented, so half the time
>>> the code ended up with many axis flips based on people debugging local
>>> orientation problems.  *sigh*
>>>
>>>
>>>> ---
>>>>  drivers/iio/accel/hid-sensor-accel-3d.c       |  3 +++
>>>>  .../hid-sensors/hid-sensor-attributes.c       | 21 +++++++++++++++++++
>>>>  drivers/iio/gyro/hid-sensor-gyro-3d.c         |  3 +++
>>>>  drivers/iio/magnetometer/hid-sensor-magn-3d.c |  3 +++
>>>>  include/linux/hid-sensor-hub.h                |  2 ++
>>>>  5 files changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
>>>> index a2def6f9380a3..980bbd7fba502 100644
>>>> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
>>>> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
>>>> @@ -59,6 +59,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_X,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_ACCEL,
>>>>  		.modified = 1,
>>>> @@ -69,6 +70,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_Y,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_ACCEL,
>>>>  		.modified = 1,
>>>> @@ -79,6 +81,7 @@ static const struct iio_chan_spec accel_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_Z,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	},
>>>>  	IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
>>>>  };
>>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>>> index 9b279937a24e0..e367e4b482ef0 100644
>>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
>>>> @@ -585,6 +585,27 @@ int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
>>>>  }
>>>>  EXPORT_SYMBOL_NS(hid_sensor_parse_common_attributes, IIO_HID);
>>>>  
>>>> +static const struct iio_mount_matrix hid_sensor_windows_axis = {
>>>> +	.rotation = {
>>>> +		"-1", "0", "0",
>>>> +		"0", "-1", "0",
>>>> +		"0", "0", "-1"
>>>
>>> Unless my memory of rotation matrices serves me wrong, that's not a rotation matrix.
>>> (det(R) != 1)
>>>
>>> That's a an axis flip from a right handed set of axis to a left handed one.
>>> So to fix this up, you would need to invert the raw readings of at least one axis
>>> rather than rely on the mount matrix or make the scale negative.
>>>
>>> Jonathan
>>>
>>>
>>>> +	}
>>>> +};
>>>> +
>>>> +static const struct iio_mount_matrix *
>>>> +hid_sensor_get_mount_matrix(const struct iio_dev *indio_dev,
>>>> +				const struct iio_chan_spec *chan)
>>>> +{
>>>> +	return &hid_sensor_windows_axis;
>>>> +}
>>>> +
>>>> +const struct iio_chan_spec_ext_info hid_sensor_ext_info[] = {
>>>> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, hid_sensor_get_mount_matrix),
>>>> +	{ }
>>>> +};
>>>> +EXPORT_SYMBOL(hid_sensor_ext_info);
>>>> +
>>>>  MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@intel.com>");
>>>>  MODULE_DESCRIPTION("HID Sensor common attribute processing");
>>>>  MODULE_LICENSE("GPL");
>>>> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
>>>> index 8f0ad022c7f1b..b852f5166bb21 100644
>>>> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
>>>> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
>>>> @@ -58,6 +58,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_X,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_ANGL_VEL,
>>>>  		.modified = 1,
>>>> @@ -68,6 +69,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_Y,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_ANGL_VEL,
>>>>  		.modified = 1,
>>>> @@ -78,6 +80,7 @@ static const struct iio_chan_spec gyro_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>>  		.scan_index = CHANNEL_SCAN_INDEX_Z,
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	},
>>>>  	IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
>>>>  };
>>>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>>> index e85a3a8eea908..aefbdb9b0869a 100644
>>>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>>> @@ -74,6 +74,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SCALE) |
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_MAGN,
>>>>  		.modified = 1,
>>>> @@ -83,6 +84,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SCALE) |
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_MAGN,
>>>>  		.modified = 1,
>>>> @@ -92,6 +94,7 @@ static const struct iio_chan_spec magn_3d_channels[] = {
>>>>  		BIT(IIO_CHAN_INFO_SCALE) |
>>>>  		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>  		BIT(IIO_CHAN_INFO_HYSTERESIS),
>>>> +		.ext_info = hid_sensor_ext_info,
>>>>  	}, {
>>>>  		.type = IIO_ROT,
>>>>  		.modified = 1,
>>>> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
>>>> index c27329e2a5ad5..ee7d5b430a785 100644
>>>> --- a/include/linux/hid-sensor-hub.h
>>>> +++ b/include/linux/hid-sensor-hub.h
>>>> @@ -236,6 +236,8 @@ struct hid_sensor_common {
>>>>  	struct work_struct work;
>>>>  };
>>>>  
>>>> +extern const struct iio_chan_spec_ext_info hid_sensor_ext_info[];
>>>> +
>>>>  /* Convert from hid unit expo to regular exponent */
>>>>  static inline int hid_sensor_convert_exponent(int unit_expo)
>>>>  {
>>>