[systemd-devel] Bad accelerometer values cause incorrect screen rotation

[Hans de Goede]

Hi Daniel,

On 05-09-19 11:05, Daniel Drake wrote:
> Hi,

<snip>

> so sometimes we even apply the
> wrong quirks. Two recent examples:
> https://bugzilla.redhat.com/show_bug.cgi?id=1717712 (more on this case below)

<snip>

> I see the latest development of having the hwdb specify whether the
> accelerometer is in the base or the display of the device. This was
> implemented for dealing with a device with accelerometers in both
> positions (https://github.com/hadess/iio-sensor-proxy/pull/262) -
> clearly the screen rotation should only follow thy display-mounted
> accelerometer in that case.

The ACCEL_LOCATION attribute was always intended not only for
360 degree hinges 2-in-1s but also for clamshells with only
a sensor in the base and the intention was always for
iio-sensor-proxy to ignore sensors in the base for display
rotation.

See the very first systemd issue filed about this:
https://github.com/systemd/systemd/issues/6557
The subject of this is "Add "orientation ignore" property for accelerometers"
which says it all.

Using sensors in the base for rotation simply make no sense,
for 1 normal clamshell devices can only be used in one orientation
(without getting creative) so not doing rotation on them makes the
most sense. Also there is no way to reliably translate base
accel results to display accel results since we do not know the
angle between the 2 we could blindly assume 90 degrees, but that
would just be a guess really.

> However now that iio-sensor-proxy ignores "base" accelerometers
> (regardless of whether or not there's another accelerometer in the
> display), that's being misused at least in this case:
> https://bugzilla.redhat.com/show_bug.cgi?id=1717712

It is not being misused, since this is not a tablet or 2-in-1
disabling rotation is the right thing to do.

> This is a HP laptop and it looked like the policy was once "we will
> not accept quirks for HP machines that use the lis3lv02d device, they
> should go in the hp_accel.c driver instead"
> (https://github.com/systemd/systemd/commit/0d1a2be93e16aa03026f1c36e81951097e8dad2c)
> but that doesn't seem to have been followed here.

Getting the orientation right is an orthogonal problem to
specifying the location. Someone can still submit a kernel patch
for this.  Although I wonder if in the cases where people can
play neverball with the accelerometer we are not dealing with
tablets, so that the sensor is actually in the screen.

> Although in this case the quirk just states that the sensor is in the
> base of the laptop, which now causes iio-sensor-proxy to ignore it.
> But since the kernel data was wrong in the first place, the user is
> now left with an accelerometer that can't be used to play neverball,
> nor can be used to rotate the screen.

The laptop in question weighs 2.3 kilos holding that in front of
you so that you can play neverball seems something which you
will grow tired of very quickly.

> In the single accelerometer case, for an ordinary clamshell laptop, it
> doesn't actually matter if the accelerometer is in the display or not.
> Since the accelerometer measures all 3 dimensions and screen position
> relative to the base is known, a mount matrix can be applied to deal
> with any positioning scheme.

First of all screen position relatively to the base is not known it
can be anywhere from 90 to 180 degrees angle.

Second of all using accelerometer based rotation on a clamshell is
almost always wrong.

> What if we had an ordinary clamshell laptop producing "bad" data where
> the accelerometer is actually mounted behind the display (was this
> even checked here?) - would a ACCEL_POSITION=base quirk still then be
> accepted?

No the laptop was not opened to check the location of the accelerometer,
nor was an elaborate dance done to check things at different display
open angles. I purely guessed the location of the sensor, but that
seems like a pretty safe check, since the primary use of this sensor
in HP laptops is for freefall detection / HD protection.

Also they can either put it on the mainboard, or route an extra cable
through the hinges to put it on an extra PCB behind the display, which
just is not logical to do.

> Although this fix seems somewhat technically incorrect, perhaps it has
> its roots in the same things I'm trying to say here:

I disagree that the fix is technically incorrect.

> when the bug
> bites it's really quite serious, it's tricky and annoying to fix the
> root cause, and why do we even care about automatic screen rotation on
> standard clamshell laptops anyway - the product itself is totally
> unusable when in any position other than "sitting flat on the desk" -
> just try typing or using the touchpad...

So we seem to be in agreement that having automatic screen rotation
on clamshells is useless. The problem is we cannot reliably identify
clamshells, DMI chassis-type info is not reliable enough for this.

One possible solution which I see is to have iio-sensor-proxy use
a white-list like this:

1) Is ACCEL_ORIENTATION set ? Yes -> ok
2) Is this a HID bases sensor ? Yes -> ok  (sensors following the HID spec generally get the axis correct)
3) Everything else -> do not do automatic rotation

I think this will catch the majority of the currently supported
devices, but it will definitely cause regressions on some device
where the orientation matrix is correct by default. In the end
fixing those regressions with some more hwdb quirks might be
better then the current problem though.  Bastien, what do you think?

Regards,

Hans