| Message ID | 20220624223341.2625231-1-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio/hid: Add mount_matrix | expand |
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) > {
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) >> { >
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) >>> { >>
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) >>>> { >>>
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) > > > { > > >
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) > > > > { > > > > > >
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) >>>>> { >>>> >>> >> >
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) > >>>>> { > >>>> > >>> > >> > > >
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.
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.
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
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 >
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. >
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 > > > > > > > > > > > > > >
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
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
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) {
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(+)