On Sat, Jun 25, 2022 at 11:27 AM srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote: > > On Sat, 2022-06-25 at 14:33 +0200, Hans de Goede wrote: > > Hi, > > > > Jonathan, thanks for Cc-ing me on this. > > > > On 6/25/22 13:09, Jonathan Cameron wrote: > > > On Fri, 24 Jun 2022 15:33:41 -0700 > > > Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote: > > > > > > > ISH based sensors do not naturally return data in the W3C > > > > 'natural' > > > > orientation. > > > > They returns all data inverted, to match Microsoft Windows > > > > requirement: > > > > [ > > > > https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/senso > > > > rs#accelerometer] > > > > """ If the device has the SimpleOrientation of FaceUp on a table, > > > > then > > > > the accelerometer would read -1 G on the Z axis. """ > > > > > > Probably reference the HID Usage Tables 1.3 spec rather than the MS > > > one. > > > https://usb.org/sites/default/files/hut1_3_0.pdf That document does not clearly defined what the accelerometer should return. > > > After some waving around of my left and right hand I'm fairly sure > > > that says the same > > > thing as the MS spec. Section 4.4 Vector Usages > > > > > > > While W3C defines > > > > [https://www.w3.org/TR/motion-sensors/#accelerometer-sensor] > > > > """The Accelerometer sensor is an inertial-frame sensor, this > > > > means that > > > > when the device is in free fall, the acceleration is 0 m/s2 in > > > > the > > > > falling direction, and when a device is laying flat on a table, > > > > the > > > > acceleration in upwards direction will be equal to the Earth > > > > gravity, > > > > i.e. g ≡ 9.8 m/s2 as it is measuring the force of the table > > > > pushing the > > > > device upwards.""" > > > > > > > > Fixes all HID sensors that defines IIO_MOD_[XYZ] attributes. > > > > > > > > Tested on "HP Spectre x360 Convertible 13" and "Dell XPS 13 > > > > 9365". > > > > > > > > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > > > > > > Ah. Whilst this is a fix, it seems likely to break a whole bunch > > > of existing > > > users that are compensating for the wrong orientation in > > > userspace. Also, do > > > we know how universal this is? I have a nasty feeling it'll turn > > > out some > > > HID sensors do it the other way whatever the spec says. Bastien, > > > are you > > > carrying a local fix for this in your userspace code? > > > > > > +CC a few people who are likely to know more on just how bad that > > > will be... > > > > Right, so Linux userspace expects an axis system similar to the > > Android one, > > which is actually the one which seems to be described here. > > > > The axis system expect is that when a tablet is layed flat on the > > table, > > the x and y axis are as one would expect when drawing a mathematics > > graph on the surface of the tablet. > > > > So X-axis goes from left to right, with left side being lower numbers > > and > > right side higher numbers. > > > > And Y-axis goes from bottom to top, with the bottom being lower > > numbers and > > the top higher numbers. > > > > That leaves the Z-axis which comes out of the screen at a 90° angle > > (to both > > the X and Y axis) and the vector coming out of the screen towards to > > the user / > > observer of the screen indicates positive numbers where as imagining > > the same > > axis pointing down through the table on which the tables is lying > > towards > > the floor represents negative numbers. > > > > This means that the accel values of a tablet resting on a table, > > expresses > > in units of 1G are: [ 0, 0, -1 ] and I've seen quite a few HID > > sensors But W3C and Android expect [ 0, 0, 1g ] when resting on the table. See commit message for W3C and for Android, see https://source.android.com/devices/sensors/sensor-types#accelerometer """When the device lies flat on a table, the acceleration value along z is +9.81 alo, which corresponds to the acceleration of the device (0 m/s^2) minus the force of gravity (-9.81 m/s^2)."""". That is why we need to change the sign to be consistent with these standards. > > 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@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) > > > > { > > > > > >
