On Sat, 2022-06-25 at 14:33 +0200, Hans de Goede wrote: > Hi, > > Jonathan, thanks for Cc-ing me on this. > > On 6/25/22 13:09, Jonathan Cameron wrote: > > On Fri, 24 Jun 2022 15:33:41 -0700 > > Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote: > > > > > ISH based sensors do not naturally return data in the W3C > > > 'natural' > > > orientation. > > > They returns all data inverted, to match Microsoft Windows > > > requirement: > > > [ > > > https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/senso > > > rs#accelerometer] > > > """ If the device has the SimpleOrientation of FaceUp on a table, > > > then > > > the accelerometer would read -1 G on the Z axis. """ > > > > Probably reference the HID Usage Tables 1.3 spec rather than the MS > > one. > > https://usb.org/sites/default/files/hut1_3_0.pdf > > After some waving around of my left and right hand I'm fairly sure > > that says the same > > thing as the MS spec. Section 4.4 Vector Usages > > > > > While W3C defines > > > [https://www.w3.org/TR/motion-sensors/#accelerometer-sensor] > > > """The Accelerometer sensor is an inertial-frame sensor, this > > > means that > > > when the device is in free fall, the acceleration is 0 m/s2 in > > > the > > > falling direction, and when a device is laying flat on a table, > > > the > > > acceleration in upwards direction will be equal to the Earth > > > gravity, > > > i.e. g ≡ 9.8 m/s2 as it is measuring the force of the table > > > pushing the > > > device upwards.""" > > > > > > Fixes all HID sensors that defines IIO_MOD_[XYZ] attributes. > > > > > > Tested on "HP Spectre x360 Convertible 13" and "Dell XPS 13 > > > 9365". > > > > > > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > > > > Ah. Whilst this is a fix, it seems likely to break a whole bunch > > of existing > > users that are compensating for the wrong orientation in > > userspace. Also, do > > we know how universal this is? I have a nasty feeling it'll turn > > out some > > HID sensors do it the other way whatever the spec says. Bastien, > > are you > > carrying a local fix for this in your userspace code? > > > > +CC a few people who are likely to know more on just how bad that > > will be... > > Right, so Linux userspace expects an axis system similar to the > Android one, > which is actually the one which seems to be described here. > > The axis system expect is that when a tablet is layed flat on the > table, > the x and y axis are as one would expect when drawing a mathematics > graph on the surface of the tablet. > > So X-axis goes from left to right, with left side being lower numbers > and > right side higher numbers. > > And Y-axis goes from bottom to top, with the bottom being lower > numbers and > the top higher numbers. > > That leaves the Z-axis which comes out of the screen at a 90° angle > (to both > the X and Y axis) and the vector coming out of the screen towards to > the user / > observer of the screen indicates positive numbers where as imagining > the same > axis pointing down through the table on which the tables is lying > towards > the floor represents negative numbers. > > This means that the accel values of a tablet resting on a table, > expresses > in units of 1G are: [ 0, 0, -1 ] and I've seen quite a few HID > sensors > 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@xxxxxxxxx>"); > > > MODULE_DESCRIPTION("HID Sensor common attribute processing"); > > > MODULE_LICENSE("GPL"); > > > diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > b/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > index 8f0ad022c7f1b..b852f5166bb21 100644 > > > --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c > > > @@ -58,6 +58,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > .scan_index = CHANNEL_SCAN_INDEX_X, > > > + .ext_info = hid_sensor_ext_info, > > > }, { > > > .type = IIO_ANGL_VEL, > > > .modified = 1, > > > @@ -68,6 +69,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > .scan_index = CHANNEL_SCAN_INDEX_Y, > > > + .ext_info = hid_sensor_ext_info, > > > }, { > > > .type = IIO_ANGL_VEL, > > > .modified = 1, > > > @@ -78,6 +80,7 @@ static const struct iio_chan_spec > > > gyro_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > .scan_index = CHANNEL_SCAN_INDEX_Z, > > > + .ext_info = hid_sensor_ext_info, > > > }, > > > IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP) > > > }; > > > diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > b/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > index e85a3a8eea908..aefbdb9b0869a 100644 > > > --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c > > > @@ -74,6 +74,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SCALE) | > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > + .ext_info = hid_sensor_ext_info, > > > }, { > > > .type = IIO_MAGN, > > > .modified = 1, > > > @@ -83,6 +84,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SCALE) | > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > + .ext_info = hid_sensor_ext_info, > > > }, { > > > .type = IIO_MAGN, > > > .modified = 1, > > > @@ -92,6 +94,7 @@ static const struct iio_chan_spec > > > magn_3d_channels[] = { > > > BIT(IIO_CHAN_INFO_SCALE) | > > > BIT(IIO_CHAN_INFO_SAMP_FREQ) | > > > BIT(IIO_CHAN_INFO_HYSTERESIS), > > > + .ext_info = hid_sensor_ext_info, > > > }, { > > > .type = IIO_ROT, > > > .modified = 1, > > > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid- > > > sensor-hub.h > > > index c27329e2a5ad5..ee7d5b430a785 100644 > > > --- a/include/linux/hid-sensor-hub.h > > > +++ b/include/linux/hid-sensor-hub.h > > > @@ -236,6 +236,8 @@ struct hid_sensor_common { > > > struct work_struct work; > > > }; > > > > > > +extern const struct iio_chan_spec_ext_info > > > hid_sensor_ext_info[]; > > > + > > > /* Convert from hid unit expo to regular exponent */ > > > static inline int hid_sensor_convert_exponent(int unit_expo) > > > { > > >
