Hi, On 6/25/22 22:52, Gwendal Grignou wrote: > On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada > <srinivas.pandruvada@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> >>>> >>>> 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@xxxxxxxxx>"); >>>>> 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) >>>>> { >>>> >>> >> >
