* [PATCH] iio/hid: Add mount_matrix
@ 2022-06-24 22:33 Gwendal Grignou
2022-06-25 11:09 ` Jonathan Cameron
0 siblings, 1 reply; 17+ messages in thread
From: Gwendal Grignou @ 2022-06-24 22:33 UTC (permalink / raw)
To: jikos, jic23, srinivas.pandruvada, wpsmith; +Cc: linux-iio, Gwendal Grignou
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. """
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>
---
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"
+ }
+};
+
+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)
{
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-24 22:33 [PATCH] iio/hid: Add mount_matrix Gwendal Grignou
@ 2022-06-25 11:09 ` Jonathan Cameron
2022-06-25 12:33 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2022-06-25 11:09 UTC (permalink / raw)
To: Gwendal Grignou
Cc: jikos, srinivas.pandruvada, wpsmith, linux-iio, Bastien Nocera,
Hans de Goede
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...
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)
> {
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 11:09 ` Jonathan Cameron
@ 2022-06-25 12:33 ` Hans de Goede
2022-06-25 12:40 ` Hans de Goede
2022-06-25 18:26 ` srinivas pandruvada
0 siblings, 2 replies; 17+ messages in thread
From: Hans de Goede @ 2022-06-25 12:33 UTC (permalink / raw)
To: Jonathan Cameron, Gwendal Grignou
Cc: jikos, srinivas.pandruvada, wpsmith, linux-iio, Bastien Nocera
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!
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)
>> {
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 12:33 ` Hans de Goede
@ 2022-06-25 12:40 ` Hans de Goede
2022-06-25 12:52 ` Hans de Goede
2022-06-25 18:26 ` srinivas pandruvada
1 sibling, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2022-06-25 12:40 UTC (permalink / raw)
To: Jonathan Cameron, Gwendal Grignou
Cc: jikos, srinivas.pandruvada, wpsmith, linux-iio, Bastien Nocera
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!!
Regards,
Hans
>
> 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)
>>> {
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 12:40 ` Hans de Goede
@ 2022-06-25 12:52 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2022-06-25 12:52 UTC (permalink / raw)
To: Jonathan Cameron, Gwendal Grignou
Cc: jikos, srinivas.pandruvada, wpsmith, linux-iio, Bastien Nocera
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)
>>>> {
>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 12:33 ` Hans de Goede
2022-06-25 12:40 ` Hans de Goede
@ 2022-06-25 18:26 ` srinivas pandruvada
2022-06-25 20:52 ` Gwendal Grignou
1 sibling, 1 reply; 17+ messages in thread
From: srinivas pandruvada @ 2022-06-25 18:26 UTC (permalink / raw)
To: Hans de Goede, Jonathan Cameron, Gwendal Grignou
Cc: jikos, wpsmith, linux-iio, Bastien Nocera
On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> > > rs#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!
>
Yes, it will. We have devices from 5+ years with this feature. So not
practical to test every device.
Thanks,
Srinivas
> 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)
> > > {
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 18:26 ` srinivas pandruvada
@ 2022-06-25 20:52 ` Gwendal Grignou
2022-06-26 14:42 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Gwendal Grignou @ 2022-06-25 20:52 UTC (permalink / raw)
To: srinivas pandruvada
Cc: Hans de Goede, Jonathan Cameron, jikos, wpsmith, linux-iio,
Bastien Nocera
On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> > > > rs#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
That document does not clearly defined what the accelerometer should return.
> > > 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
But W3C and Android expect [ 0, 0, 1g ] when resting on the table. See
commit message for W3C and for Android, see
https://source.android.com/devices/sensors/sensor-types#accelerometer
"""When the device lies flat on a table, the acceleration value along
z is +9.81 alo, which corresponds to the acceleration of the device (0
m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why we
need to change the sign to be consistent with these standards.
> > 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.
This is a not a bug since HID sensors from 2 laptops from different
manufacturers (HP and Dell) report the same values.
> >
> > Again HID-sensors already work fine on tons of existing devices
> > without
> > any matrix getting applied.
Note this patch does not change the value reported by the sensor, just
define a matrix to make sure the sensors values are consistent with
Android/W3C.
> >
> > Merging this patch would break existing userspace on tons of devices!
Since the user space application may not use the defined matrix, it
would not affect existing them. Inverting the scale definitely will.
> >
> Yes, it will. We have devices from 5+ years with this feature. So not
> practical to test every device.
>
> Thanks,
> Srinivas
>
> > 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)
This is indeed not a rotation matrix.
What I observed is when laying flat on the table (open at 180 degree),
using the sysfs interface the value reported by the laptops along the
Z axis is -1g, while using an android phone, the reported value is 1g.
Same thing when the device is lying on this left side (reported value
along the X axis -1g instead of 1g for an android phone), and when
resting on the front lip (reported value along the Y axis is -1g
instead of 1g).
> > >
> > > 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.
Negative scale is a good option, but it will definitely break user
space applications, when the mount matrix may not be used today.
Gwendal.
> > >
> > > 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)
> > > > {
> > >
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-25 20:52 ` Gwendal Grignou
@ 2022-06-26 14:42 ` Hans de Goede
2022-06-27 9:05 ` Gwendal Grignou
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2022-06-26 14:42 UTC (permalink / raw)
To: Gwendal Grignou, srinivas pandruvada
Cc: Jonathan Cameron, jikos, wpsmith, linux-iio, Bastien Nocera
Hi,
On 6/25/22 22:52, Gwendal Grignou wrote:
> On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
>>
>> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
>>>>> rs#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
> That document does not clearly defined what the accelerometer should return.
>>>> 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
> But W3C and Android expect [ 0, 0, 1g ] when resting on the table. See
> commit message for W3C and for Android, see
> https://source.android.com/devices/sensors/sensor-types#accelerometer
> """When the device lies flat on a table, the acceleration value along
> z is +9.81 alo, which corresponds to the acceleration of the device (0
> m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why we
> need to change the sign to be consistent with these standards.
Windows and HID devices use [ 0, 0, -1 ] for device face up on
a flat table and this is what Linux has been reporting to userspace
for many many years now. Auto-display-rotation based in accel
reading was first supported in GNOME through iio-sensor-proxy
in 2014 !
And iio-sensor-proxy expects the Windows/HID sign of the axis
you can NOT just go and change this, that would break 8 years
of precedent of using the current Windows/HID sign (+/-) for
the axis!
Also besides the kernel reporting a matrix it is also possible
for userspace to provide a matrix using udev's hwdb:
https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
This has been in place since 2016 and this sets a matrix adjusting
readings of non HID sensors to match the *HID/windows* definition
of the axis.
>>> 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.
> This is a not a bug since HID sensors from 2 laptops from different
> manufacturers (HP and Dell) report the same values.
Right, I wrongly assumed the Android definition would match the
IMHO much saner / much more intuitive HID/Windows definitions,
since the arrows in the phone image in the Android docs point
in the same direction as the Windows docs.
But I have tested on an Android phone now and I see that
Android indeed has all the axis inverted.
>>> Again HID-sensors already work fine on tons of existing devices
>>> without
>>> any matrix getting applied.
> Note this patch does not change the value reported by the sensor, just
> define a matrix to make sure the sensors values are consistent with
> Android/W3C.
And iio-sensor-proxy will read the kernel provided matrix, apply it
and then put the image upside down since you've just mirrored both
the X and Y axis, breaking rotation for all existing GNOME and KDE
users.
So still a big NACK from me.
If Android needs the axis to be flipped, then it should do so
in the iio-sensor HAL inside android, not add a matrix which
breaks 8 years of userspace API convention.
Breaking userspace is simply not acceptible, we have a very clear
no regressions policy. So this patch simply cannot be merged: NACK.
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)
> This is indeed not a rotation matrix.
> What I observed is when laying flat on the table (open at 180 degree),
> using the sysfs interface the value reported by the laptops along the
> Z axis is -1g, while using an android phone, the reported value is 1g.
> Same thing when the device is lying on this left side (reported value
> along the X axis -1g instead of 1g for an android phone), and when
> resting on the front lip (reported value along the Y axis is -1g
> instead of 1g).
>
>>>>
>>>> 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.
> Negative scale is a good option, but it will definitely break user
> space applications, when the mount matrix may not be used today.
>
> Gwendal.
>>>>
>>>> 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)
>>>>> {
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-26 14:42 ` Hans de Goede
@ 2022-06-27 9:05 ` Gwendal Grignou
2022-06-27 9:55 ` Bastien Nocera
0 siblings, 1 reply; 17+ messages in thread
From: Gwendal Grignou @ 2022-06-27 9:05 UTC (permalink / raw)
To: Hans de Goede
Cc: srinivas pandruvada, Jonathan Cameron, jikos, wpsmith, linux-iio,
Bastien Nocera
of
On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 6/25/22 22:52, Gwendal Grignou wrote:
> > On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> >>
> >> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> >>>>> rs#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
> > That document does not clearly defined what the accelerometer should return.
> >>>> 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
> > But W3C and Android expect [ 0, 0, 1g ] when resting on the table. See
> > commit message for W3C and for Android, see
> > https://source.android.com/devices/sensors/sensor-types#accelerometer
> > """When the device lies flat on a table, the acceleration value along
> > z is +9.81 alo, which corresponds to the acceleration of the device (0
> > m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why we
> > need to change the sign to be consistent with these standards.
>
> Windows and HID devices use [ 0, 0, -1 ] for device face up on
> a flat table and
> this is what Linux has been reporting to userspace for many many years now.
This is not true. Most drivers for standalone sensor report [ 0, 0, 1]
when the accelerometer sits face up on a table. For instance:
- in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
raw accelerometer for sensor KX023 (see datasheet
https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
page 17), as the scale is a positive value.
- same for sensor BMI160 in driver
drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
and Android requirement (see
https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
page 107).
> Auto-display-rotation based in accel
> reading was first supported in GNOME through iio-sensor-proxy
> in 2014 !
>
> And iio-sensor-proxy expects the Windows/HID sign of the axis
> you can NOT just go and change this, that would break 8 years
> of precedent of using the current Windows/HID sign (+/-) for
> the axis!
>
> Also besides the kernel reporting a matrix it is also possible
> for userspace to provide a matrix using udev's hwdb:
>
> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
Note that the determinant of most of the matrices in this file equals
-1. It means these matrices do not just describe a rotation, they
first invert the axis (see
https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
136 out of 160 matrices invert the axes. In particular, entries with
acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out 31
are, with acpi:SMO8500* 25 out 27 are.
The matrices for directly connected accelerometers/IMU where axes are
not inverted may be errors and confirm that some IIO drivers do report
[ 0, 0, 1 ] for accelerometer ICs face up. I will send a more detailed
message with my findings on the systemd mailing list.
>
> This has been in place since 2016 and this sets a matrix adjusting
> readings of non HID sensors to match the *HID/windows* definition
> of the axis.
>
> >>> 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.
> > This is a not a bug since HID sensors from 2 laptops from different
> > manufacturers (HP and Dell) report the same values.
>
> Right, I wrongly assumed the Android definition would match the
> IMHO much saner / much more intuitive HID/Windows definitions,
> since the arrows in the phone image in the Android docs point
> in the same direction as the Windows docs.
>
> But I have tested on an Android phone now and I see that
> Android indeed has all the axis inverted.
>
> >>> Again HID-sensors already work fine on tons of existing devices
> >>> without
> >>> any matrix getting applied.
> > Note this patch does not change the value reported by the sensor, just
> > define a matrix to make sure the sensors values are consistent with
> > Android/W3C.
>
> And iio-sensor-proxy will read the kernel provided matrix, apply it
> and then put the image upside down since you've just mirrored both
> the X and Y axis, breaking rotation for all existing GNOME and KDE
> users.
>
> So still a big NACK from me.
>
> If Android needs the axis to be flipped, then it should do so
> in the iio-sensor HAL inside android, not add a matrix which
> breaks 8 years of userspace API convention.
>
> Breaking userspace is simply not acceptible, we have a very clear
> no regressions policy. So this patch simply cannot be merged: NACK.
I agree this patch is not right, but it has at least the merit to
reveal an inconsistency among IIO drivers. As Jonathan suggested, some
drivers should return a negative scale. We need to choose which ones
should: either the group of drivers that reports unaltered data from
the hardware sensors and conform to W3C standard and android
specification, or the ones that conform to the HID specification.
Regards,
Gwendal.
>
> 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)
> > This is indeed not a rotation matrix.
> > What I observed is when laying flat on the table (open at 180 degree),
> > using the sysfs interface the value reported by the laptops along the
> > Z axis is -1g, while using an android phone, the reported value is 1g.
> > Same thing when the device is lying on this left side (reported value
> > along the X axis -1g instead of 1g for an android phone), and when
> > resting on the front lip (reported value along the Y axis is -1g
> > instead of 1g).
> >
> >>>>
> >>>> 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.
> > Negative scale is a good option, but it will definitely break user
> > space applications, when the mount matrix may not be used today.
> >
> > Gwendal.
> >>>>
> >>>> 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)
> >>>>> {
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-27 9:05 ` Gwendal Grignou
@ 2022-06-27 9:55 ` Bastien Nocera
2022-06-27 20:23 ` Gwendal Grignou
2022-06-28 12:33 ` Hans de Goede
0 siblings, 2 replies; 17+ messages in thread
From: Bastien Nocera @ 2022-06-27 9:55 UTC (permalink / raw)
To: Gwendal Grignou, Hans de Goede
Cc: srinivas pandruvada, Jonathan Cameron, jikos, wpsmith, linux-iio
On Mon, 2022-06-27 at 02:05 -0700, Gwendal Grignou wrote:
> of
>
> On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com>
> wrote:
> >
> > Hi,
> >
> > On 6/25/22 22:52, Gwendal Grignou wrote:
> > > On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > >
> > > > On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> > > > > > > rs#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
> > > That document does not clearly defined what the accelerometer
> > > should return.
> > > > > > 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
> > > But W3C and Android expect [ 0, 0, 1g ] when resting on the
> > > table. See
> > > commit message for W3C and for Android, see
> > > https://source.android.com/devices/sensors/sensor-types#accelerometer
> > > """When the device lies flat on a table, the acceleration value
> > > along
> > > z is +9.81 alo, which corresponds to the acceleration of the
> > > device (0
> > > m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why
> > > we
> > > need to change the sign to be consistent with these standards.
> >
> > Windows and HID devices use [ 0, 0, -1 ] for device face up on
> > a flat table and
> > this is what Linux has been reporting to userspace for many many
> > years now.
> This is not true. Most drivers for standalone sensor report [ 0, 0,
> 1]
> when the accelerometer sits face up on a table. For instance:
> - in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
> raw accelerometer for sensor KX023 (see datasheet
> https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
> page 17), as the scale is a positive value.
> - same for sensor BMI160 in driver
> drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
> and Android requirement (see
> https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
> page 107).
Accelerometers can be installed in any orientation inside the shell of
the device, which means that raw data reported from the device needs to
be checked against how it's implemented.
> > Auto-display-rotation based in accel
> > reading was first supported in GNOME through iio-sensor-proxy
> > in 2014 !
> >
> > And iio-sensor-proxy expects the Windows/HID sign of the axis
> > you can NOT just go and change this, that would break 8 years
> > of precedent of using the current Windows/HID sign (+/-) for
> > the axis!
> >
> > Also besides the kernel reporting a matrix it is also possible
> > for userspace to provide a matrix using udev's hwdb:
> >
> > https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
> Note that the determinant of most of the matrices in this file equals
> -1. It means these matrices do not just describe a rotation, they
> first invert the axis (see
> https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
> 136 out of 160 matrices invert the axes. In particular, entries with
> acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out
> 31
> are, with acpi:SMO8500* 25 out 27 are.
>
> The matrices for directly connected accelerometers/IMU where axes are
> not inverted may be errors and confirm that some IIO drivers do
> report
> [ 0, 0, 1 ] for accelerometer ICs face up. I will send a more
> detailed
> message with my findings on the systemd mailing list.
You're already in touch with the people who have worked on that code
here (that'd be Hans and myself).
>
> >
> > This has been in place since 2016 and this sets a matrix adjusting
> > readings of non HID sensors to match the *HID/windows* definition
> > of the axis.
> >
> > > > > 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.
> > > This is a not a bug since HID sensors from 2 laptops from
> > > different
> > > manufacturers (HP and Dell) report the same values.
> >
> > Right, I wrongly assumed the Android definition would match the
> > IMHO much saner / much more intuitive HID/Windows definitions,
> > since the arrows in the phone image in the Android docs point
> > in the same direction as the Windows docs.
> >
> > But I have tested on an Android phone now and I see that
> > Android indeed has all the axis inverted.
> >
> > > > > Again HID-sensors already work fine on tons of existing
> > > > > devices
> > > > > without
> > > > > any matrix getting applied.
> > > Note this patch does not change the value reported by the sensor,
> > > just
> > > define a matrix to make sure the sensors values are consistent
> > > with
> > > Android/W3C.
> >
> > And iio-sensor-proxy will read the kernel provided matrix, apply it
> > and then put the image upside down since you've just mirrored both
> > the X and Y axis, breaking rotation for all existing GNOME and KDE
> > users.
> >
> > So still a big NACK from me.
> >
> > If Android needs the axis to be flipped, then it should do so
> > in the iio-sensor HAL inside android, not add a matrix which
> > breaks 8 years of userspace API convention.
> >
> > Breaking userspace is simply not acceptible, we have a very clear
> > no regressions policy. So this patch simply cannot be merged: NACK.
> I agree this patch is not right, but it has at least the merit to
> reveal an inconsistency among IIO drivers. As Jonathan suggested,
> some
> drivers should return a negative scale. We need to choose which ones
> should: either the group of drivers that reports unaltered data from
> the hardware sensors and conform to W3C standard and android
> specification, or the ones that conform to the HID specification.
You need to realise that the drivers aren't created equal.
Whether a driver returns:
- the raw readings from the sensor (which might or might not be aligned
with the Android, Windows 8 or IIO docs),
- adjusted readings (meant to be used as-is if the sensor is mounted
with the right orientation inside the right component of the device),
- or completely ludicrous readings because the sensor is in the wrong
part of the system, or tracks something else (all the HP "disk fall"
sensors)
all those are essentially API. You can't "fix" this after the fact. I,
and others who contributed to the systemd quirks file, expected the
kernel readings to not change with the kernel version.
The right approach would be to define which is the expected orientation
in the IIO docs, although that already seems to be done[1], and
actually make sure this is respected in *new* implementations.
Finally, I'll also add that the implementation in iio-sensor-proxy
expects a single authoritative mount matrix to be provided for a
device, as a mount matrix is supposed to correct the readings for the
way a sensor was mounted inside the chassis/display of a particular
device.
Cheers
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
An implementor might consider that for a hand-held device, a
'natural' orientation would be 'front facing camera at the top'.
The main hardware reference frame could then be described as :
* Y is in the plane of the screen and is positive towards the
top of the screen ;
* X is in the plane of the screen, perpendicular to Y axis, and
positive towards the right hand side of the screen ;
* Z is perpendicular to the screen plane and positive out of the
screen.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-27 9:55 ` Bastien Nocera
@ 2022-06-27 20:23 ` Gwendal Grignou
2022-06-27 20:48 ` srinivas pandruvada
2022-06-28 12:44 ` Hans de Goede
2022-06-28 12:33 ` Hans de Goede
1 sibling, 2 replies; 17+ messages in thread
From: Gwendal Grignou @ 2022-06-27 20:23 UTC (permalink / raw)
To: Bastien Nocera
Cc: Hans de Goede, srinivas pandruvada, Jonathan Cameron, jikos,
wpsmith, linux-iio
On Mon, Jun 27, 2022 at 2:55 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Mon, 2022-06-27 at 02:05 -0700, Gwendal Grignou wrote:
> > of
> >
> > On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com>
> > wrote:
> > >
> > > Hi,
> > >
> > > On 6/25/22 22:52, Gwendal Grignou wrote:
> > > > On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > >
> > > > > On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> > > > > > > > rs#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
> > > > That document does not clearly defined what the accelerometer
> > > > should return.
> > > > > > > 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
> > > > But W3C and Android expect [ 0, 0, 1g ] when resting on the
> > > > table. See
> > > > commit message for W3C and for Android, see
> > > > https://source.android.com/devices/sensors/sensor-types#accelerometer
> > > > """When the device lies flat on a table, the acceleration value
> > > > along
> > > > z is +9.81 alo, which corresponds to the acceleration of the
> > > > device (0
> > > > m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why
> > > > we
> > > > need to change the sign to be consistent with these standards.
> > >
> > > Windows and HID devices use [ 0, 0, -1 ] for device face up on
> > > a flat table and
> > > this is what Linux has been reporting to userspace for many many
> > > years now.
> > This is not true. Most drivers for standalone sensor report [ 0, 0,
> > 1]
> > when the accelerometer sits face up on a table. For instance:
> > - in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
> > raw accelerometer for sensor KX023 (see datasheet
> > https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
> > page 17), as the scale is a positive value.
> > - same for sensor BMI160 in driver
> > drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
> > and Android requirement (see
> > https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
> > page 107).
>
> Accelerometers can be installed in any orientation inside the shell of
> the device, which means that raw data reported from the device needs to
> be checked against how it's implemented.
>
> > > Auto-display-rotation based in accel
> > > reading was first supported in GNOME through iio-sensor-proxy
> > > in 2014 !
> > >
> > > And iio-sensor-proxy expects the Windows/HID sign of the axis
> > > you can NOT just go and change this, that would break 8 years
> > > of precedent of using the current Windows/HID sign (+/-) for
> > > the axis!
> > >
> > > Also besides the kernel reporting a matrix it is also possible
> > > for userspace to provide a matrix using udev's hwdb:
> > >
> > > https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
> > Note that the determinant of most of the matrices in this file equals
> > -1. It means these matrices do not just describe a rotation, they
> > first invert the axis (see
> > https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
> > 136 out of 160 matrices invert the axes. In particular, entries with
> > acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out
> > 31
> > are, with acpi:SMO8500* 25 out 27 are.
> >
> > The matrices for directly connected accelerometers/IMU where axes are
> > not inverted may be errors and confirm that some IIO drivers do
> > report
> > [ 0, 0, 1 ] for accelerometer ICs face up. I will send a more
> > detailed
> > message with my findings on the systemd mailing list.
>
> You're already in touch with the people who have worked on that code
> here (that'd be Hans and myself).
>
> >
> > >
> > > This has been in place since 2016 and this sets a matrix adjusting
> > > readings of non HID sensors to match the *HID/windows* definition
> > > of the axis.
> > >
> > > > > > 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.
> > > > This is a not a bug since HID sensors from 2 laptops from
> > > > different
> > > > manufacturers (HP and Dell) report the same values.
> > >
> > > Right, I wrongly assumed the Android definition would match the
> > > IMHO much saner / much more intuitive HID/Windows definitions,
> > > since the arrows in the phone image in the Android docs point
> > > in the same direction as the Windows docs.
> > >
> > > But I have tested on an Android phone now and I see that
> > > Android indeed has all the axis inverted.
> > >
> > > > > > Again HID-sensors already work fine on tons of existing
> > > > > > devices
> > > > > > without
> > > > > > any matrix getting applied.
> > > > Note this patch does not change the value reported by the sensor,
> > > > just
> > > > define a matrix to make sure the sensors values are consistent
> > > > with
> > > > Android/W3C.
> > >
> > > And iio-sensor-proxy will read the kernel provided matrix, apply it
> > > and then put the image upside down since you've just mirrored both
> > > the X and Y axis, breaking rotation for all existing GNOME and KDE
> > > users.
> > >
> > > So still a big NACK from me.
> > >
> > > If Android needs the axis to be flipped, then it should do so
> > > in the iio-sensor HAL inside android, not add a matrix which
> > > breaks 8 years of userspace API convention.
> > >
> > > Breaking userspace is simply not acceptible, we have a very clear
> > > no regressions policy. So this patch simply cannot be merged: NACK.
> > I agree this patch is not right, but it has at least the merit to
> > reveal an inconsistency among IIO drivers. As Jonathan suggested,
> > some
> > drivers should return a negative scale. We need to choose which ones
> > should: either the group of drivers that reports unaltered data from
> > the hardware sensors and conform to W3C standard and android
> > specification, or the ones that conform to the HID specification.
>
> You need to realise that the drivers aren't created equal.
>
> Whether a driver returns:
> - the raw readings from the sensor (which might or might not be aligned
> with the Android, Windows 8 or IIO docs),
> - adjusted readings (meant to be used as-is if the sensor is mounted
> with the right orientation inside the right component of the device),
> - or completely ludicrous readings because the sensor is in the wrong
> part of the system, or tracks something else (all the HP "disk fall"
> sensors)
>
> all those are essentially API. You can't "fix" this after the fact. I,
> and others who contributed to the systemd quirks file, expected the
> kernel readings to not change with the kernel version.
Relying on a quirk file for all standalone sensors is not sustainable
IMHO. I agree changing existing output in newer kernels is also very
difficult.
Should we add a new iio sysfs attribute to indicate the sensors report
data according to either the W3C specification or HID specification?
If present, and set to W3C, iio-sensor-proxy would invert the axis
first before applying [new] rotation matrices which will never change
the basis.
>
> The right approach would be to define which is the expected orientation
> in the IIO docs, although that already seems to be done[1], and
> actually make sure this is respected in *new* implementations.
All standalone sensors respect the 'natural' orientation. But when
flat on a table they return the force exercised by the table to
prevent further fall, as defined by W3C, while the HID sensor hub
reports the gravity vector itself.
This discrepancy should be fixed at the kernel level.
>
> Finally, I'll also add that the implementation in iio-sensor-proxy
> expects a single authoritative mount matrix to be provided for a
> device, as a mount matrix is supposed to correct the readings for the
> way a sensor was mounted inside the chassis/display of a particular
> device.
The definition of the mount matrix in iio sysfs [1] is """[...] is a
rotation matrix which
informs userspace about sensor chip's placement relative to the main
hardware it is mounted on."""
Per definition, a "rotation matrix" does not invert the axis: """they
leave "handedness" unchanged.""" [2]
Most of the matrices in 60-sensor.hwdb do change the handedness.
[1]: https://www.spinics.net/lists/linux-iio/msg24305.html
[2]: https://en.wikipedia.org/wiki/Rotation_matrix#Geometry
Regards,
Gwendal.
>
> Cheers
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
> An implementor might consider that for a hand-held device, a
> 'natural' orientation would be 'front facing camera at the top'.
> The main hardware reference frame could then be described as :
> * Y is in the plane of the screen and is positive towards the
> top of the screen ;
> * X is in the plane of the screen, perpendicular to Y axis, and
> positive towards the right hand side of the screen ;
> * Z is perpendicular to the screen plane and positive out of the
> screen.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-27 20:23 ` Gwendal Grignou
@ 2022-06-27 20:48 ` srinivas pandruvada
2022-06-28 12:44 ` Hans de Goede
1 sibling, 0 replies; 17+ messages in thread
From: srinivas pandruvada @ 2022-06-27 20:48 UTC (permalink / raw)
To: Gwendal Grignou, Bastien Nocera
Cc: Hans de Goede, Jonathan Cameron, jikos, wpsmith, linux-iio
On Mon, 2022-06-27 at 13:23 -0700, Gwendal Grignou wrote:
> On Mon, Jun 27, 2022 at 2:55 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> >
>
[...]
> Relying on a quirk file for all standalone sensors is not sustainable
> IMHO. I agree changing existing output in newer kernels is also very
> difficult.
> Should we add a new iio sysfs attribute to indicate the sensors
> report
> data according to either the W3C specification or HID specification?
> If present, and set to W3C, iio-sensor-proxy would invert the axis
> first before applying [new] rotation matrices which will never change
> the basis.
When we added support for HID sensor hub, it was primarily driven for
supporting Linux on systems sold with Windows. This started from USB
external hubs to i2c external hubs to now ISH. Still they all are
designed for Windows primarily as OEMs will test on Windows and will
have to pass Windows compliance tests.
We can export the following fields to user space to do any model
specific quirks:
From HID spec:
Property: Sensor Manufacturer
Property: Sensor Model
Property: Sensor Serial Number
Property: Sensor Description
Property: Sensor Connection Type
Connection Type: PC Integrated
Connection Type: PC Attached
Connection Type: PC External
Property: Sensor Device Path
Property: Hardware Revision
Property: Firmware Version
Property: Release Date
Thanks,
Srinivas
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-27 9:55 ` Bastien Nocera
2022-06-27 20:23 ` Gwendal Grignou
@ 2022-06-28 12:33 ` Hans de Goede
2022-07-13 15:58 ` Jonathan Cameron
2022-07-19 1:57 ` Jakob Hauser
1 sibling, 2 replies; 17+ messages in thread
From: Hans de Goede @ 2022-06-28 12:33 UTC (permalink / raw)
To: Bastien Nocera, Gwendal Grignou
Cc: srinivas pandruvada, Jonathan Cameron, jikos, wpsmith, linux-iio
Hi,
On 6/27/22 11:55, Bastien Nocera wrote:
> On Mon, 2022-06-27 at 02:05 -0700, Gwendal Grignou wrote:
>> of
>>
>> On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 6/25/22 22:52, Gwendal Grignou wrote:
>>>> On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
>>>> <srinivas.pandruvada@linux.intel.com> wrote:
>>>>>
>>>>> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
>>>>>>>> rs#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
>>>> That document does not clearly defined what the accelerometer
>>>> should return.
>>>>>>> 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
>>>> But W3C and Android expect [ 0, 0, 1g ] when resting on the
>>>> table. See
>>>> commit message for W3C and for Android, see
>>>> https://source.android.com/devices/sensors/sensor-types#accelerometer
>>>> """When the device lies flat on a table, the acceleration value
>>>> along
>>>> z is +9.81 alo, which corresponds to the acceleration of the
>>>> device (0
>>>> m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why
>>>> we
>>>> need to change the sign to be consistent with these standards.
>>>
>>> Windows and HID devices use [ 0, 0, -1 ] for device face up on
>>> a flat table and
>>> this is what Linux has been reporting to userspace for many many
>>> years now.
>> This is not true. Most drivers for standalone sensor report [ 0, 0,
>> 1]
>> when the accelerometer sits face up on a table. For instance:
>> - in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
>> raw accelerometer for sensor KX023 (see datasheet
>> https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
>> page 17), as the scale is a positive value.
>> - same for sensor BMI160 in driver
>> drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
>> and Android requirement (see
>> https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
>> page 107).
>
> Accelerometers can be installed in any orientation inside the shell of
> the device, which means that raw data reported from the device needs to
> be checked against how it's implemented.
>
>>> Auto-display-rotation based in accel
>>> reading was first supported in GNOME through iio-sensor-proxy
>>> in 2014 !
>>>
>>> And iio-sensor-proxy expects the Windows/HID sign of the axis
>>> you can NOT just go and change this, that would break 8 years
>>> of precedent of using the current Windows/HID sign (+/-) for
>>> the axis!
>>>
>>> Also besides the kernel reporting a matrix it is also possible
>>> for userspace to provide a matrix using udev's hwdb:
>>>
>>> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
>> Note that the determinant of most of the matrices in this file equals
>> -1. It means these matrices do not just describe a rotation, they
>> first invert the axis (see
>> https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
>> 136 out of 160 matrices invert the axes. In particular, entries with
>> acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out
>> 31
>> are, with acpi:SMO8500* 25 out 27 are.
>>
>> The matrices for directly connected accelerometers/IMU where axes are
>> not inverted may be errors and confirm that some IIO drivers do
>> report
>> [ 0, 0, 1 ] for accelerometer ICs face up. I will send a more
>> detailed
>> message with my findings on the systemd mailing list.
>
> You're already in touch with the people who have worked on that code
> here (that'd be Hans and myself).
>
>>
>>>
>>> This has been in place since 2016 and this sets a matrix adjusting
>>> readings of non HID sensors to match the *HID/windows* definition
>>> of the axis.
>>>
>>>>>> 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.
>>>> This is a not a bug since HID sensors from 2 laptops from
>>>> different
>>>> manufacturers (HP and Dell) report the same values.
>>>
>>> Right, I wrongly assumed the Android definition would match the
>>> IMHO much saner / much more intuitive HID/Windows definitions,
>>> since the arrows in the phone image in the Android docs point
>>> in the same direction as the Windows docs.
>>>
>>> But I have tested on an Android phone now and I see that
>>> Android indeed has all the axis inverted.
>>>
>>>>>> Again HID-sensors already work fine on tons of existing
>>>>>> devices
>>>>>> without
>>>>>> any matrix getting applied.
>>>> Note this patch does not change the value reported by the sensor,
>>>> just
>>>> define a matrix to make sure the sensors values are consistent
>>>> with
>>>> Android/W3C.
>>>
>>> And iio-sensor-proxy will read the kernel provided matrix, apply it
>>> and then put the image upside down since you've just mirrored both
>>> the X and Y axis, breaking rotation for all existing GNOME and KDE
>>> users.
>>>
>>> So still a big NACK from me.
>>>
>>> If Android needs the axis to be flipped, then it should do so
>>> in the iio-sensor HAL inside android, not add a matrix which
>>> breaks 8 years of userspace API convention.
>>>
>>> Breaking userspace is simply not acceptible, we have a very clear
>>> no regressions policy. So this patch simply cannot be merged: NACK.
>> I agree this patch is not right, but it has at least the merit to
>> reveal an inconsistency among IIO drivers. As Jonathan suggested,
>> some
>> drivers should return a negative scale. We need to choose which ones
>> should: either the group of drivers that reports unaltered data from
>> the hardware sensors and conform to W3C standard and android
>> specification, or the ones that conform to the HID specification.
>
> You need to realise that the drivers aren't created equal.
>
> Whether a driver returns:
> - the raw readings from the sensor (which might or might not be aligned
> with the Android, Windows 8 or IIO docs),
> - adjusted readings (meant to be used as-is if the sensor is mounted
> with the right orientation inside the right component of the device),
> - or completely ludicrous readings because the sensor is in the wrong
> part of the system, or tracks something else (all the HP "disk fall"
> sensors)
>
> all those are essentially API. You can't "fix" this after the fact. I,
> and others who contributed to the systemd quirks file, expected the
> kernel readings to not change with the kernel version.
>
> The right approach would be to define which is the expected orientation
> in the IIO docs, although that already seems to be done[1], and
> actually make sure this is respected in *new* implementations.
>
> Finally, I'll also add that the implementation in iio-sensor-proxy
> expects a single authoritative mount matrix to be provided for a
> device, as a mount matrix is supposed to correct the readings for the
> way a sensor was mounted inside the chassis/display of a particular
> device.
>
> Cheers
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
> An implementor might consider that for a hand-held device, a
> 'natural' orientation would be 'front facing camera at the top'.
> The main hardware reference frame could then be described as :
> * Y is in the plane of the screen and is positive towards the
> top of the screen ;
> * X is in the plane of the screen, perpendicular to Y axis, and
> positive towards the right hand side of the screen ;
> * Z is perpendicular to the screen plane and positive out of the
> screen.
Note though that this does not clearly define what
e.g. "positive towards the top of the screen" means if
you look at the axis illustrations in both the Windows,
Android and W3C docs they all have an arrow pointing
towards the top of the screen for the Y axis, which
matches the "positive towards the top of the screen"
wording.
Yet Android / W3C expect a positive reading when
the G force vector is pointing down. Which I still
find confusing.
This means that we to add a text similar to the Windows
docs here, say something like (made up by me not copy
pasted from Windows docs):
"The Z-axis reading will be -1G when a device is laying
flat on a table with its display facing up"
To fix the are we measuring gravity or force countering
gravity confusion.
Jonathan, shall I submit a patch to add this, maybe with
some extra text that we are following the Windows/HID
convention and that Android/W3C do things differently?
Regards,
Hans
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-27 20:23 ` Gwendal Grignou
2022-06-27 20:48 ` srinivas pandruvada
@ 2022-06-28 12:44 ` Hans de Goede
1 sibling, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2022-06-28 12:44 UTC (permalink / raw)
To: Gwendal Grignou, Bastien Nocera
Cc: srinivas pandruvada, Jonathan Cameron, jikos, wpsmith, linux-iio
Hi,
On 6/27/22 22:23, Gwendal Grignou wrote:
> On Mon, Jun 27, 2022 at 2:55 AM Bastien Nocera <hadess@hadess.net> wrote:
>>
>> On Mon, 2022-06-27 at 02:05 -0700, Gwendal Grignou wrote:
>>> of
>>>
>>> On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 6/25/22 22:52, Gwendal Grignou wrote:
>>>>> On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
>>>>> <srinivas.pandruvada@linux.intel.com> wrote:
>>>>>>
>>>>>> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
>>>>>>>>> rs#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
>>>>> That document does not clearly defined what the accelerometer
>>>>> should return.
>>>>>>>> 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
>>>>> But W3C and Android expect [ 0, 0, 1g ] when resting on the
>>>>> table. See
>>>>> commit message for W3C and for Android, see
>>>>> https://source.android.com/devices/sensors/sensor-types#accelerometer
>>>>> """When the device lies flat on a table, the acceleration value
>>>>> along
>>>>> z is +9.81 alo, which corresponds to the acceleration of the
>>>>> device (0
>>>>> m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why
>>>>> we
>>>>> need to change the sign to be consistent with these standards.
>>>>
>>>> Windows and HID devices use [ 0, 0, -1 ] for device face up on
>>>> a flat table and
>>>> this is what Linux has been reporting to userspace for many many
>>>> years now.
>>> This is not true. Most drivers for standalone sensor report [ 0, 0,
>>> 1]
>>> when the accelerometer sits face up on a table. For instance:
>>> - in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
>>> raw accelerometer for sensor KX023 (see datasheet
>>> https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
>>> page 17), as the scale is a positive value.
>>> - same for sensor BMI160 in driver
>>> drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
>>> and Android requirement (see
>>> https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
>>> page 107).
>>
>> Accelerometers can be installed in any orientation inside the shell of
>> the device, which means that raw data reported from the device needs to
>> be checked against how it's implemented.
>>
>>>> Auto-display-rotation based in accel
>>>> reading was first supported in GNOME through iio-sensor-proxy
>>>> in 2014 !
>>>>
>>>> And iio-sensor-proxy expects the Windows/HID sign of the axis
>>>> you can NOT just go and change this, that would break 8 years
>>>> of precedent of using the current Windows/HID sign (+/-) for
>>>> the axis!
>>>>
>>>> Also besides the kernel reporting a matrix it is also possible
>>>> for userspace to provide a matrix using udev's hwdb:
>>>>
>>>> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
>>> Note that the determinant of most of the matrices in this file equals
>>> -1. It means these matrices do not just describe a rotation, they
>>> first invert the axis (see
>>> https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
>>> 136 out of 160 matrices invert the axes. In particular, entries with
>>> acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out
>>> 31
>>> are, with acpi:SMO8500* 25 out 27 are.
>>>
>>> The matrices for directly connected accelerometers/IMU where axes are
>>> not inverted may be errors and confirm that some IIO drivers do
>>> report
>>> [ 0, 0, 1 ] for accelerometer ICs face up. I will send a more
>>> detailed
>>> message with my findings on the systemd mailing list.
>>
>> You're already in touch with the people who have worked on that code
>> here (that'd be Hans and myself).
>>
>>>
>>>>
>>>> This has been in place since 2016 and this sets a matrix adjusting
>>>> readings of non HID sensors to match the *HID/windows* definition
>>>> of the axis.
>>>>
>>>>>>> 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.
>>>>> This is a not a bug since HID sensors from 2 laptops from
>>>>> different
>>>>> manufacturers (HP and Dell) report the same values.
>>>>
>>>> Right, I wrongly assumed the Android definition would match the
>>>> IMHO much saner / much more intuitive HID/Windows definitions,
>>>> since the arrows in the phone image in the Android docs point
>>>> in the same direction as the Windows docs.
>>>>
>>>> But I have tested on an Android phone now and I see that
>>>> Android indeed has all the axis inverted.
>>>>
>>>>>>> Again HID-sensors already work fine on tons of existing
>>>>>>> devices
>>>>>>> without
>>>>>>> any matrix getting applied.
>>>>> Note this patch does not change the value reported by the sensor,
>>>>> just
>>>>> define a matrix to make sure the sensors values are consistent
>>>>> with
>>>>> Android/W3C.
>>>>
>>>> And iio-sensor-proxy will read the kernel provided matrix, apply it
>>>> and then put the image upside down since you've just mirrored both
>>>> the X and Y axis, breaking rotation for all existing GNOME and KDE
>>>> users.
>>>>
>>>> So still a big NACK from me.
>>>>
>>>> If Android needs the axis to be flipped, then it should do so
>>>> in the iio-sensor HAL inside android, not add a matrix which
>>>> breaks 8 years of userspace API convention.
>>>>
>>>> Breaking userspace is simply not acceptible, we have a very clear
>>>> no regressions policy. So this patch simply cannot be merged: NACK.
>>> I agree this patch is not right, but it has at least the merit to
>>> reveal an inconsistency among IIO drivers. As Jonathan suggested,
>>> some
>>> drivers should return a negative scale. We need to choose which ones
>>> should: either the group of drivers that reports unaltered data from
>>> the hardware sensors and conform to W3C standard and android
>>> specification, or the ones that conform to the HID specification.
>>
>> You need to realise that the drivers aren't created equal.
>>
>> Whether a driver returns:
>> - the raw readings from the sensor (which might or might not be aligned
>> with the Android, Windows 8 or IIO docs),
>> - adjusted readings (meant to be used as-is if the sensor is mounted
>> with the right orientation inside the right component of the device),
>> - or completely ludicrous readings because the sensor is in the wrong
>> part of the system, or tracks something else (all the HP "disk fall"
>> sensors)
>>
>> all those are essentially API. You can't "fix" this after the fact. I,
>> and others who contributed to the systemd quirks file, expected the
>> kernel readings to not change with the kernel version.
> Relying on a quirk file for all standalone sensors is not sustainable
> IMHO. I agree changing existing output in newer kernels is also very
> difficult.
> Should we add a new iio sysfs attribute to indicate the sensors report
> data according to either the W3C specification or HID specification?
Gwendal, I appreciate that you are trying to be flexible and come
up with a solution here.
But adding this attribute will just make things more confusion and
causing breakage when running a userspace which does not know
about this on a newer kernel.
> If present, and set to W3C, iio-sensor-proxy would invert the axis
> first before applying [new] rotation matrices which will never change
> the basis.
This means that using a non-updated iio-sensor-proxy with a new-kernel
which sets the W3C flag will break and we never break userspace API,
so also NACK for this proposal.
IMHO you really need to just *accept* that the IIO subsystem and
both kernel (e.g. from ACPI / devicetree) + udev hwdb exported
mount-matrices all will report the axis in Windows/HID handedness
(after applying the mount-matrix).
I realize this is not properly documented, I plan to submit
a patch to Documentation/ABI/testing/sysfs-bus-iio#n1838 to spell
this out clearly.
>> The right approach would be to define which is the expected orientation
>> in the IIO docs, although that already seems to be done[1], and
>> actually make sure this is respected in *new* implementations.
> All standalone sensors respect the 'natural' orientation. But when
> flat on a table they return the force exercised by the table to
> prevent further fall, as defined by W3C, while the HID sensor hub
> reports the gravity vector itself.
> This discrepancy should be fixed at the kernel level.
No just *no*, full stop. There is no reason what soever why this
cannot be done inside the Android sensor HAL. And there is
8 years of precedence for using the Windows/HID handedness.
So again please just accept that kernel provided (ACPI / devicetree)
mount-matrices will result in the axis values being reported
in Windows/HID handedness and adjust your userspace consumers
accordingly.
Regards,
Hans
>> [1]:
>> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
>> An implementor might consider that for a hand-held device, a
>> 'natural' orientation would be 'front facing camera at the top'.
>> The main hardware reference frame could then be described as :
>> * Y is in the plane of the screen and is positive towards the
>> top of the screen ;
>> * X is in the plane of the screen, perpendicular to Y axis, and
>> positive towards the right hand side of the screen ;
>> * Z is perpendicular to the screen plane and positive out of the
>> screen.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-28 12:33 ` Hans de Goede
@ 2022-07-13 15:58 ` Jonathan Cameron
2022-07-18 10:14 ` Linus Walleij
2022-07-19 1:57 ` Jakob Hauser
1 sibling, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2022-07-13 15:58 UTC (permalink / raw)
To: Hans de Goede
Cc: Bastien Nocera, Gwendal Grignou, srinivas pandruvada, jikos,
wpsmith, linux-iio, Linus Walleij
On Tue, 28 Jun 2022 14:33:09 +0200
Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 6/27/22 11:55, Bastien Nocera wrote:
> > On Mon, 2022-06-27 at 02:05 -0700, Gwendal Grignou wrote:
> >> of
> >>
> >> On Sun, Jun 26, 2022 at 7:42 AM Hans de Goede <hdegoede@redhat.com>
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 6/25/22 22:52, Gwendal Grignou wrote:
> >>>> On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada
> >>>> <srinivas.pandruvada@linux.intel.com> wrote:
> >>>>>
> >>>>> On Sat, 2022-06-25 at 14:33 +0200, 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/senso
> >>>>>>>> rs#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
> >>>> That document does not clearly defined what the accelerometer
> >>>> should return.
> >>>>>>> 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
> >>>> But W3C and Android expect [ 0, 0, 1g ] when resting on the
> >>>> table. See
> >>>> commit message for W3C and for Android, see
> >>>> https://source.android.com/devices/sensors/sensor-types#accelerometer
> >>>> """When the device lies flat on a table, the acceleration value
> >>>> along
> >>>> z is +9.81 alo, which corresponds to the acceleration of the
> >>>> device (0
> >>>> m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why
> >>>> we
> >>>> need to change the sign to be consistent with these standards.
> >>>
> >>> Windows and HID devices use [ 0, 0, -1 ] for device face up on
> >>> a flat table and
> >>> this is what Linux has been reporting to userspace for many many
> >>> years now.
> >> This is not true. Most drivers for standalone sensor report [ 0, 0,
> >> 1]
> >> when the accelerometer sits face up on a table. For instance:
> >> - in driver drivers/iio/accel/kxcjk-1013.c, read INFO_RAW reports the
> >> raw accelerometer for sensor KX023 (see datasheet
> >> https://kionixfs.azureedge.net/en/datasheet/KX023-1025%20Specifications%20Rev%2012.0.pdf
> >> page 17), as the scale is a positive value.
> >> - same for sensor BMI160 in driver
> >> drivers/iio/imu/bmi160/bmi160_core.c. The BMI160 axis conforms to W3C
> >> and Android requirement (see
> >> https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi160-ds000.pdf
> >> page 107).
> >
> > Accelerometers can be installed in any orientation inside the shell of
> > the device, which means that raw data reported from the device needs to
> > be checked against how it's implemented.
> >
> >>> Auto-display-rotation based in accel
> >>> reading was first supported in GNOME through iio-sensor-proxy
> >>> in 2014 !
> >>>
> >>> And iio-sensor-proxy expects the Windows/HID sign of the axis
> >>> you can NOT just go and change this, that would break 8 years
> >>> of precedent of using the current Windows/HID sign (+/-) for
> >>> the axis!
> >>>
> >>> Also besides the kernel reporting a matrix it is also possible
> >>> for userspace to provide a matrix using udev's hwdb:
> >>>
> >>> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
> >> Note that the determinant of most of the matrices in this file equals
> >> -1. It means these matrices do not just describe a rotation, they
> >> first invert the axis (see
> >> https://en.wikipedia.org/wiki/Determinant#Orientation_of_a_basis)
> >> 136 out of 160 matrices invert the axes. In particular, entries with
> >> acpi:KIOX... 42 out of 45 are inverted, with acpi:BOSC0200*, 22 out
> >> 31
> >> are, with acpi:SMO8500* 25 out 27 are.
> >>
> >> The matrices for directly connected accelerometers/IMU where axes are
> >> not inverted may be errors and confirm that some IIO drivers do
> >> report
> >> [ 0, 0, 1 ] for accelerometer ICs face up. I will send a more
> >> detailed
> >> message with my findings on the systemd mailing list.
> >
> > You're already in touch with the people who have worked on that code
> > here (that'd be Hans and myself).
> >
> >>
> >>>
> >>> This has been in place since 2016 and this sets a matrix adjusting
> >>> readings of non HID sensors to match the *HID/windows* definition
> >>> of the axis.
> >>>
> >>>>>> 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.
> >>>> This is a not a bug since HID sensors from 2 laptops from
> >>>> different
> >>>> manufacturers (HP and Dell) report the same values.
> >>>
> >>> Right, I wrongly assumed the Android definition would match the
> >>> IMHO much saner / much more intuitive HID/Windows definitions,
> >>> since the arrows in the phone image in the Android docs point
> >>> in the same direction as the Windows docs.
> >>>
> >>> But I have tested on an Android phone now and I see that
> >>> Android indeed has all the axis inverted.
> >>>
> >>>>>> Again HID-sensors already work fine on tons of existing
> >>>>>> devices
> >>>>>> without
> >>>>>> any matrix getting applied.
> >>>> Note this patch does not change the value reported by the sensor,
> >>>> just
> >>>> define a matrix to make sure the sensors values are consistent
> >>>> with
> >>>> Android/W3C.
> >>>
> >>> And iio-sensor-proxy will read the kernel provided matrix, apply it
> >>> and then put the image upside down since you've just mirrored both
> >>> the X and Y axis, breaking rotation for all existing GNOME and KDE
> >>> users.
> >>>
> >>> So still a big NACK from me.
> >>>
> >>> If Android needs the axis to be flipped, then it should do so
> >>> in the iio-sensor HAL inside android, not add a matrix which
> >>> breaks 8 years of userspace API convention.
> >>>
> >>> Breaking userspace is simply not acceptible, we have a very clear
> >>> no regressions policy. So this patch simply cannot be merged: NACK.
> >> I agree this patch is not right, but it has at least the merit to
> >> reveal an inconsistency among IIO drivers. As Jonathan suggested,
> >> some
> >> drivers should return a negative scale. We need to choose which ones
> >> should: either the group of drivers that reports unaltered data from
> >> the hardware sensors and conform to W3C standard and android
> >> specification, or the ones that conform to the HID specification.
> >
> > You need to realise that the drivers aren't created equal.
> >
> > Whether a driver returns:
> > - the raw readings from the sensor (which might or might not be aligned
> > with the Android, Windows 8 or IIO docs),
> > - adjusted readings (meant to be used as-is if the sensor is mounted
> > with the right orientation inside the right component of the device),
> > - or completely ludicrous readings because the sensor is in the wrong
> > part of the system, or tracks something else (all the HP "disk fall"
> > sensors)
> >
> > all those are essentially API. You can't "fix" this after the fact. I,
> > and others who contributed to the systemd quirks file, expected the
> > kernel readings to not change with the kernel version.
> >
> > The right approach would be to define which is the expected orientation
> > in the IIO docs, although that already seems to be done[1], and
> > actually make sure this is respected in *new* implementations.
> >
> > Finally, I'll also add that the implementation in iio-sensor-proxy
> > expects a single authoritative mount matrix to be provided for a
> > device, as a mount matrix is supposed to correct the readings for the
> > way a sensor was mounted inside the chassis/display of a particular
> > device.
> >
> > Cheers
> >
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
> > An implementor might consider that for a hand-held device, a
> > 'natural' orientation would be 'front facing camera at the top'.
> > The main hardware reference frame could then be described as :
> > * Y is in the plane of the screen and is positive towards the
> > top of the screen ;
> > * X is in the plane of the screen, perpendicular to Y axis, and
> > positive towards the right hand side of the screen ;
> > * Z is perpendicular to the screen plane and positive out of the
> > screen.
>
> Note though that this does not clearly define what
> e.g. "positive towards the top of the screen" means if
> you look at the axis illustrations in both the Windows,
> Android and W3C docs they all have an arrow pointing
> towards the top of the screen for the Y axis, which
> matches the "positive towards the top of the screen"
> wording.
>
> Yet Android / W3C expect a positive reading when
> the G force vector is pointing down. Which I still
> find confusing.
>
> This means that we to add a text similar to the Windows
> docs here, say something like (made up by me not copy
> pasted from Windows docs):
>
> "The Z-axis reading will be -1G when a device is laying
> flat on a table with its display facing up"
>
> To fix the are we measuring gravity or force countering
> gravity confusion.
>
> Jonathan, shall I submit a patch to add this, maybe with
> some extra text that we are following the Windows/HID
> convention and that Android/W3C do things differently?
I'm not going to rush this through late in a cycle.
So propose a text update and let's carry on the discussion
around that. (you may already have done so - I'm way behind!)
Given we have a bunch of mount matrices that aren't rotation
matrices out in the wild, we probably want to cover that as
well, potentially by relaxing the definition to allow
rotoinversions, or at least state they are out there even if
we actively discourage them going forwards.
Also, I just remembered Linus w wrote the docs in the first
place and there was a long discussion at the time so +CC
Thanks,
Jonathan
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>
>
>
>
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-07-13 15:58 ` Jonathan Cameron
@ 2022-07-18 10:14 ` Linus Walleij
0 siblings, 0 replies; 17+ messages in thread
From: Linus Walleij @ 2022-07-18 10:14 UTC (permalink / raw)
To: Jonathan Cameron, Jakob Hauser
Cc: Hans de Goede, Bastien Nocera, Gwendal Grignou,
srinivas pandruvada, jikos, wpsmith, linux-iio
On Wed, Jul 13, 2022 at 5:49 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 28 Jun 2022 14:33:09 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> > > [1]:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
> > > An implementor might consider that for a hand-held device, a
> > > 'natural' orientation would be 'front facing camera at the top'.
> > > The main hardware reference frame could then be described as :
> > > * Y is in the plane of the screen and is positive towards the
> > > top of the screen ;
> > > * X is in the plane of the screen, perpendicular to Y axis, and
> > > positive towards the right hand side of the screen ;
> > > * Z is perpendicular to the screen plane and positive out of the
> > > screen.
> >
> > Note though that this does not clearly define what
> > e.g. "positive towards the top of the screen" means if
> > you look at the axis illustrations in both the Windows,
> > Android and W3C docs they all have an arrow pointing
> > towards the top of the screen for the Y axis, which
> > matches the "positive towards the top of the screen"
> > wording.
> >
> > Yet Android / W3C expect a positive reading when
> > the G force vector is pointing down. Which I still
> > find confusing.
> >
> > This means that we to add a text similar to the Windows
> > docs here, say something like (made up by me not copy
> > pasted from Windows docs):
> >
> > "The Z-axis reading will be -1G when a device is laying
> > flat on a table with its display facing up"
> >
> > To fix the are we measuring gravity or force countering
> > gravity confusion.
> >
> > Jonathan, shall I submit a patch to add this, maybe with
> > some extra text that we are following the Windows/HID
> > convention and that Android/W3C do things differently?
>
> I'm not going to rush this through late in a cycle.
> So propose a text update and let's carry on the discussion
> around that. (you may already have done so - I'm way behind!)
>
> Given we have a bunch of mount matrices that aren't rotation
> matrices out in the wild, we probably want to cover that as
> well, potentially by relaxing the definition to allow
> rotoinversions, or at least state they are out there even if
> we actively discourage them going forwards.
>
> Also, I just remembered Linus w wrote the docs in the first
> place and there was a long discussion at the time so +CC
I recently discussed the mount matrix bindings WRT magnetometers
with Jakob Hauser who has made a deep analysis of that
subject.
The TL;DR is that it turns out that also magnetometers are
device-oriented, and thus breaking most of the paradigms
used by people like geophysicists.
However for the "sensor fusion" usecase for a phone/tablet
device is makes perfect sense since e.g. projecting the magnetic
line onto the plane of the screen so you can create a compass
application the linear algebra becomes more intuitive with
everything expressed in device-relative terms.
(Some rambiling here: https://wiki.postmarketos.org/wiki/Magnetometer)
This is a bit unlucky given that IIO as a whole is probably
more ambitious and not just appealing to consumer devices...
but now we are stuck with this and the geophysicists will
perpetually shake their head as they align their new Linux-driven
magnetometer, encapsulated in concrete and mounted in the
bedrock to be perfectly perpendicular to earths surface.
Maybe when the physicists come around we can add some
isometric-mount-matrix = ... as alternative?
Please include Jakob on subsequent patches I am sure we can
hash this out for all sensors.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] iio/hid: Add mount_matrix
2022-06-28 12:33 ` Hans de Goede
2022-07-13 15:58 ` Jonathan Cameron
@ 2022-07-19 1:57 ` Jakob Hauser
1 sibling, 0 replies; 17+ messages in thread
From: Jakob Hauser @ 2022-07-19 1:57 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Linus Walleij, Bastien Nocera, Gwendal Grignou,
srinivas pandruvada, jikos, wpsmith, linux-iio
Hi Hans,
On 28.06.22 14:33, Hans de Goede wrote:
> Hi,
>
> On 6/27/22 11:55, Bastien Nocera wrote:
...
>>
>> [1]:
>> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio#n1838
>> An implementor might consider that for a hand-held device, a
>> 'natural' orientation would be 'front facing camera at the top'.
>> The main hardware reference frame could then be described as :
>> * Y is in the plane of the screen and is positive towards the
>> top of the screen ;
>> * X is in the plane of the screen, perpendicular to Y axis, and
>> positive towards the right hand side of the screen ;
>> * Z is perpendicular to the screen plane and positive out of the
>> screen.
>
> Note though that this does not clearly define what
> e.g. "positive towards the top of the screen" means if
> you look at the axis illustrations in both the Windows,
> Android and W3C docs they all have an arrow pointing
> towards the top of the screen for the Y axis, which
> matches the "positive towards the top of the screen"
> wording.
The above actually just defines the general coordinate system of a
hand-held device. Similar to e.g. [1].
[1]
https://source.android.com/devices/sensors/sensor-types#sensor_axis_definition
> Yet Android / W3C expect a positive reading when
> the G force vector is pointing down. Which I still
> find confusing.
In the Android docs in link [2], the third example description states:
"When the device lies flat on a table, the acceleration value along z is
+9.81 alo, which corresponds to the acceleration of the device (0 m/s^2)
minus the force of gravity (-9.81 m/s^2)."
So it is 0 - (-9.81) = +9.81. Confusing indeed. I think this is what you
called "force countering gravity". More on this further below.
[2] https://source.android.com/devices/sensors/sensor-types#accelerometer
> This means that we to add a text similar to the Windows
> docs here, say something like (made up by me not copy
> pasted from Windows docs):
>
> "The Z-axis reading will be -1G when a device is laying
> flat on a table with its display facing up"
>
> To fix the are we measuring gravity or force countering
> gravity confusion.
As far as I can see, Windows distinguishes between "standard
accelerometer", "linear accelerometer" and "gravity accelerometer".
Links [3] and [4] are from the accelerometer sample, link [5] is from
the API. The general description [6] is not very clear on this.
[3]
https://github.com/Microsoft/Windows-universal-samples/tree/main/Samples/Accelerometer#accelerometer-sample
[4]
https://github.com/microsoft/Windows-universal-samples/blob/win10-2004/Samples/Accelerometer/cpp/Scenario0_Choose.xaml.cpp#L40-L47
[5]
https://docs.microsoft.com/en-us/uwp/api/windows.devices.sensors.accelerometerreadingtype?view=winrt-22621#fields
[6]
https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/sensors#accelerometer
W3C also has such differentiation called "Accelerometer",
"LinearAccelerationSensor" and "GravitySensor", see e.g. link [7]
chapter 6. A further link [8] indicates that the subtypes "are most
likely fusion sensors".
[7] https://w3c.github.io/accelerometer/#contents
[8] https://www.w3.org/TR/motion-sensors/#gravity-and-linear-acceleration
That doesn't explain much yet. I'm just raising the topic of different
purposes. It looks to me like at a device on a table with screen up, -1G
is the gravity vector and +1G is the acceleration acting on the device.
So according to that, we're measuring the gravity vector. That's fine
for auto-rotating the display.
What does this means for dynamic acceleration measurement? In the above
link [2] about Android accelerometer, there are four example
descriptions. Let's take our case with the device on the table, screen
up, measuring -1G. For the first example, free fall, we would need to
accelerate positively to get 0G. The fourth example, accelerating into
the sky, this would mean a negative acceleration in our case, getting
closer to -2G. Accordingly, in the second example, pushing the device
"on its left side toward the right", we would get a negative x acceleration.
I'm not fully sure I got this right, I don't have any practical
experience with accelerometers. Though from the last paragraph, it looks
to me that dynamic acceleration measurement return inverted values.
Maybe someone would need to confirm this, though it's not easy to take
measurements on a device being accelerated dynamically.
The Windows documentation in the above link [6] is not very clear about
this. It says "[...] Accelerometer sensor measures G-force values along
the X, Y, and Z axes of the device [...]". This sounds rather unspecific
to me. I don't really know what it does in the end. However, if they
mean "g-force" literally, it's the inverse reaction of acceleration [9].
Also it looks to me that Windows is not measuring acceleration in m/s^2
but "g-force" in "g", according to the sentence "Note that the
gravitational vector should normalize to 1 for a stationary device" in
the above link [6].
[9] https://en.wikipedia.org/wiki/G-force
My intention of writing these lines here is not to change the current
implementation but rather to get a clearer picture of what we're
actually measuring, thus helping to improve the documentation.
Summarizing it looks to me that statically we're measuring the gravity
vector and dynamically the centrifugal acceleration.
Although for gravity, within IIO subsystem there is another interface
called "in_gravity_" [10].
[10]
https://github.com/torvalds/linux/blob/v5.18/Documentation/ABI/testing/sysfs-bus-iio#L236-L244
> Jonathan, shall I submit a patch to add this, maybe with
> some extra text that we are following the Windows/HID
> convention and that Android/W3C do things differently?
The following file [11] would need to be adapted accordingly.
[11]
https://github.com/torvalds/linux/blob/v5.18/Documentation/devicetree/bindings/iio/mount-matrix.txt#L49-L91
Kind regards,
Jakob
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-19 1:57 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-24 22:33 [PATCH] iio/hid: Add mount_matrix Gwendal Grignou
2022-06-25 11:09 ` Jonathan Cameron
2022-06-25 12:33 ` Hans de Goede
2022-06-25 12:40 ` Hans de Goede
2022-06-25 12:52 ` Hans de Goede
2022-06-25 18:26 ` srinivas pandruvada
2022-06-25 20:52 ` Gwendal Grignou
2022-06-26 14:42 ` Hans de Goede
2022-06-27 9:05 ` Gwendal Grignou
2022-06-27 9:55 ` Bastien Nocera
2022-06-27 20:23 ` Gwendal Grignou
2022-06-27 20:48 ` srinivas pandruvada
2022-06-28 12:44 ` Hans de Goede
2022-06-28 12:33 ` Hans de Goede
2022-07-13 15:58 ` Jonathan Cameron
2022-07-18 10:14 ` Linus Walleij
2022-07-19 1:57 ` Jakob Hauser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).