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 _______________________________________________ systemd-devel mailing list systemd-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/systemd-devel
