Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hd-app-mgr: Add iio-sensor-proxy backend for rotation. #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IMbackK
Copy link

@IMbackK IMbackK commented Aug 2, 2020

Add iio-sensor-proxy backend used preferentially to mce. mce is still used when iio-sensor-proxy is not available.

Not tested in a fully functional manner, as portrait mode results in a black screen on my device (droid 4), only tested that the signal is received correctly.

@freemangordon
Copy link

IIUC, that code belongs to mce, not h-d

@IMbackK
Copy link
Author

IMbackK commented Aug 2, 2020

this is a prototype. to be looked at/tested by people with devices where randr rotation works correctly (pinephone? n900?)

@IMbackK
Copy link
Author

IMbackK commented Aug 2, 2020

@freemangordon mce provides a dbus interface to the accelerometer that hildon-desktop uses to determine when to rotate to portrait. This provides the option to use iio-sensor-proxy instead of mce.

this is beneficial because iio-sesnor-proxy supports all kinds of sensor divers while mce supports only one (n900)

It is my intention to remove all sensor handling code from mce and replace it with iio-sesnor-proxy everywhere

@freemangordon
Copy link

I think we shall keep the current interfaces intact and teach mce in iio-sensors. @MerlijnWajer - what do you think?

@IMbackK
Copy link
Author

IMbackK commented Aug 2, 2020

[21:42] <freemangordon> uvos: accel (and in general - sensors) control does not belong to h-d, but mce.
[21:43] <uvos> freemangordon: you missunderstand what this pr is
[21:43] <uvos> read the comment on the pr
[21:43] <freemangordon> I did it
[21:43] <uvos> ok
[21:44] <freemangordon> why shall h-d use 2 sources?
[21:44] <uvos> h-d shal use only one in the end 
[21:44] <uvos> i want to replace mce sensor handling with iio-sensor-proxy everywhere
[21:44] <freemangordon> IMO mce shall read iio-sensors and h-d shall use the existing interface to mce
[21:44] <uvos> but the mce backend can stay
[21:44] <uvos> why not
[21:44] <uvos> hmm thats needless redirection
[21:45] <uvos> mce and iio-sensors  provide the same dbus interface
[21:45] <freemangordon> mce interface is used by more parties than h-d
[21:45] <uvos> just the name differs 
[21:45] <freemangordon> so by removing it we'll break stuff without a need
[21:45] <uvos> all that mce provides (sensors wise) is a dbus proparty that says roated or not rotated
[21:45] <uvos> afaik this is only usefull for hildon 
[21:46] <freemangordon> yes, but it is used not only in h-d
[21:46] <uvos> everything else that uses the accel in maemo uses the input interface afaik
[21:46] <freemangordon> no
[21:46] <uvos> well what else is there?
[21:46] <uvos> i will miagrate everything relevant
[21:46] <uvos> rather than carry this legacy interface
[21:46] <freemangordon> for sure phone-ui and browser, but those are not relevant anyways
[21:47] <freemangordon> uvos: please, lets not break stuff for no particular reason
[21:47] <uvos> what dose browser use it for?
[21:47] <uvos> or phone-ui
[21:47] <freemangordon> rotation :)
[21:47] <uvos> they get rotatied by hildon
[21:47] <uvos> they get roataed by the hildon code
[21:47] <uvos> its in there
[21:47] <freemangordon> no, they get rotated by themselves
[21:48] <freemangordon> also, there are more applications/libs that depend on mce dbus interface
[21:48] <freemangordon> I don;t really understand why you put a label "obsolete" on mce
[21:49] <uvos> well ok
[21:49] <uvos> here we go :P
[21:49] <freemangordon> neither why shall we spread the functionality over different daemons
[21:50] <freemangordon> we have stable and tested daemon/interface. lets just attach it to the current sensors provider (be it iio-sensors lib or evdev or /sys)
[21:50] <uvos> well mce is a collection of random bits really, i replaces x11 blanking functions, it dose random input processing (eg home key), random audio stuff. 
[21:51] <uvos> replaceing mce with standart deamons in places where these are well supported makes a lot of sense form a mantainability perspective
[21:51] <freemangordon> I would not call it random, as most of those are connected more or less
[21:51] <uvos> iio-sensor-proxy is a standart interface now so lets use it
[21:51] <freemangordon> sure, but lets use it in mce
[21:51] <freemangordon> and keep the current interfaces
[21:52] <freemangordon> BTW, how is keyboard slide handled?
[21:52] <uvos> ? hildon checks the state
[21:52] <uvos> before rotating
[21:52] <freemangordon> but it gets the state from mce, right?
[21:53] <freemangordon> or not
[21:53] <branon> <buZz> https://fossbytes.com/linux-based-pinephone-will-soon-come-with-nokia-n900-style-keyboard/
[21:53] <Entitlement> branon - [ Linux-based PinePhone Will Soon Come With Nokia N900-Style Keyboard ]
[21:53] <uvos> maybe, but thats unaffected
[21:53] <branon> excited
[21:53] <uvos> i think in places where the interfaces are used very little it just makes sense to abandon it, rather than maintain a module in mce 
[21:53] <uvos> the accelerometer protrait information is the only sensor interface in mce 
[21:53] <uvos> makes no sense to have a singe interface to one random sensor 
[21:53] <uvos> it just dosent
[21:54] <sicelo> branon: i wonder why they said N900 .. looks nothing like N900 keyboard, and the video seems to show N950 instead :p
[21:54] <-- jonsger ([email protected]) has left this server (Ping timeout: 256 seconds).
[21:54] <uvos> honestly if hildon gets the keyboard state from mce, it shoud not
[21:54] <uvos> or maybe not at least
[21:54] <uvos> i have to think about it
[21:54] <uvos> i should get it from evdev maybe
[21:55] <freemangordon> uvos - my point being - mce is the one that is in charge of controlling the various device states
[21:56] <freemangordon> uvos, ok, lemme think about that for a while. alos I want to hear from Wizzup about it
[21:56] <uvos> ok
[21:56] <freemangordon> this is a major architectural change I am not sure we want to make ATM
[21:56] <uvos> its not major really
[21:57] <uvos> mce dosent do mutch sensor stuff anyways (interface wise)
[21:57] <uvos> you position makes sense conceptually
[21:57] <freemangordon> exactly, my point being about the architecture
[21:58] <uvos> i just think mce should be a small as possible with as little device dependant stuff in it as possible from a mantainability perspective. and to use standart interfaces where they are avaiable everywhere (not just as inputs to mce) to enable maemo-leste componants to work independatly from one another without the whole maemo stack
[21:59] <freemangordon> uvos: also, you;re missing the actdead mode where there is no h-d running
[21:59] <uvos> what about it?
[21:59] <freemangordon> but mce is running
[21:59] <uvos> so?
[21:59] <freemangordon> and you have matchbox WM there
[21:59] <uvos> so? mce dosnet rotate the screen by itself
[21:59] <freemangordon> so, you'll have to integrate with iio-sensors there there as well
[21:59] <freemangordon> sure
[22:00] <uvos> matchbox uses mce to rotate??
[22:00] <uvos> sure ill do it if needed
[22:00] <freemangordon> the point being that we don't know in advance what else will be affected
[22:00] <uvos> right 
[22:00] <uvos> i just suspect its not mutch
[22:00] <uvos> i accept the burdon of fixing things
[22:01] <freemangordon> anyway, lets discuss it when we have Wizzup back

@MerlijnWajer
Copy link
Member

Nice, thanks for the PR!

I view mce as the abstraction to many hw parts of the phone. IMO, if h-d doesn't use much beyond rotation, let's have it rely on mce instead. That way we can also have one centralised way where we tweak the policy. Like, when do we decide we have actually rotated our device? We don't want that to go out of sync between programs: one thinking it is landscape, the other thinking it is portrait.

I agree that having full access to the sensor is helpful for other programs, but h-d doesn't seem to be one of them, from a quick glance?

@MerlijnWajer
Copy link
Member

There's also things like orientation lock applet, which if I recall correctly, relies on gconf settings to override the rotation settings, so I think that policy part is really best handled in mce.

@IMbackK
Copy link
Author

IMbackK commented Aug 3, 2020

mce itself dosent use the accelerometer for anything and hildon desktop reads the gconf state by itself with no input from mce at all, so the policy is not handled in mce at all atm either.

the only thing mce dose do is turn off poling when the display is off and during various other times, but because of how the iio interface works haveing mce do that with iio devices would be highly problematic so its best to have hildon unsubscribe from whatever dbus interface used to get the data. (iio or mce)

@MerlijnWajer
Copy link
Member

As far as I can tell, modules/accelerometer.c has special code to deal with the different orientations, and also for example see if the device is using a stand (like the n900 has).

mce also sends the new orientation over dbus. Are you saying that's not used?

@MerlijnWajer
Copy link
Member

What is problematic about mce doing that? I don't understand.

@IMbackK
Copy link
Author

IMbackK commented Aug 3, 2020

mce also sends the new orientation over dbus. Are you saying that's not used?

it is used by hildon as one of many factors to decide when to rotate.

As far as I can tell, modules/accelerometer.c has special code to deal with the different orientations, and also for example see if the device is using a stand (like the n900 has).

This single input that mce accelerometer.c uses besides the accelerometer (and callibration data, also used by the proxy) is not policy. It is sensor fusion at best and not very usefull at that because the accelerometer will show landscape when the n900 is on the stand. its also very n900 specific so a replacment to accelerometer.c would likely ditch this anyways.

All the real policy is in hildon, hildon decides when to rotate based on the roation-lock state, the current client, the keyboard state and so on. the real policy decisions are already in hildon, where imho they make sense. When mce sends the orientation signal it has no clue wheather this is reflected in the rotation state, so hildon and mce already "get out of sync".

Mce is in this case really nothing other than an interface to the accelerometer, with no orientation policy in between. This is why mce adds nothing that iio-sensor-proxy dose not provide and therefore becomes suplus to requirements

also note that iio-sensor-proxy dose not provide raw accelerometer data, it provides device roataion state (eg prortait, landscape not x,y,z m/s^2)

@IMbackK
Copy link
Author

IMbackK commented Aug 3, 2020

What is problematic about mce doing that? I don't understand.

mce chaning the poling rate on iio interface would cause all clients to that interface to get data at a different rate.
we defiantly should not change the rate behind all the other clients back. this is best done with the clients consent, poling rate is only reduced when all high rate clients unsubscribe from the interface.

@freemangordon
Copy link

wait, what other accelerometer clients we have besides iio-sensors? also, if mce talks to iio-sensors, then it is iio-sensor's job to control the polling (!?!) speed. And how is that different if the user is h-d and not mce.

mce reporting n900 on stand as landscape is correct and very useful and this is applicable for other devices as well - like I have a tablet here with a case that have a stand (and a keyboard).

Keep in mind that (at least on n900) enabling the accelerometer increases the power usage significantly, so I would really like to have only one 'proxy' between applications and HW. A proxy which is aware it runs on a battery operated device, which I doubt is the case for iio-sensors. So, the 'policy' about when to enable and how often to poll and what state to report should be in mce, not in h-d or in the other applications.

@IMbackK
Copy link
Author

IMbackK commented Aug 3, 2020

in current leste we have mce and any client that wants raw x,y,z data in parallel using the kernel driver directly (because mce dose not provide this) and mce affects the polling rate behind everyones back, this is bad.

with iio-sensor-proxy we have the same thing except that i iio-sensor-proxy uses only 1 polling rate, and closes the interface when none is suscribed to the iio-sensor-proxy dbus interface. Since, again, iio-sensor-proxy dose not provide raw x,y,z data any application that wants it will need to use the kernel driver directly. This senario is only mildly more predictable than the current behavior because only one poling rate is used and the kernel idles the chip when the last client stops using it. its still bad.

the good news is that there is a branch of iio-sensor-proxy that also provides raw x,y,z data from the accelerometer next to all the other things iio-sensor-proxy provides (light lux and proximity).

hildon could take the stand into considoration just like it dose the keyboard. the current situation makes no sense for sure, the stand is handled in mce and everything else is handled in hildon "policy" wise.

i gues handling everything in mce also makes some sense. but since hildon already dose most of this while mce dose almost nothing and this makes hildon independant of mce moving everything into hildon makes the most sense.

@IMbackK
Copy link
Author

IMbackK commented Aug 3, 2020

also note that the above polling problems dident exist in fremantle because the evdev accel interface was used. this interface however is declared obsolete by upstream and no current or future accelerometer driver will ever use it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants