FedeDP / Clight

A C daemon that turns your webcam into a light sensor. It will adjust screen backlight based on ambient brightness.
GNU General Public License v3.0
732 stars 27 forks source link

[SUPPORT] Getting Clight to work on the Pinephone #233

Open jclsn opened 3 years ago

jclsn commented 3 years ago

I am currently trying to get clight to run on the Pinephone, because there is not working solution for the backlight adjustment yet. I started out with zero configuration, but verbose turned on. It can detect the backlight values just fine and also finds a sensor, but then fails to capture it.

Here is the log

Clightd found, version: 5.4.
AC screen backlight curve: y = -0.024825 + 0.191641x + -0.008928x^2
BL
^
|        **
|      **  
|     *    
|    *     
|   *      
|  *       
|          
| *        
|*         
|          
*---------->BR
BATT screen backlight curve: y = -0.010629 + 0.153844x + -0.007284x^2
BL
^
|          
|          
|       ***
|     **   
|    *     
|   *      
|  *       
| *        
|          
|*         
*---------->BR
Set timeout of 30s 0ns on fd 15.
Failed to create org.freedesktop.ScreenSaver dbus interface: Unknown error -1
Monitoring requests to org.freedesktop.ScreenSaver name owner.
Disarmed timerfd on fd 39.
Not a laptop device. Killing UPower module.
Failed to retrieve AC state; fallback to ON_AC and OPEN lid.
Emitting 'AcState' property
idle_get_client(): Operation not permitted
Clightd idle error.
Failed to init. Killing module.
Set timeout of 0s 1ns on fd 26.
Emitting 'LidState' property
Set timeout of 30s 0ns on fd 26.
53.56 10.05 loaded from cache file.
idle_get_client(): Operation not permitted
Clightd idle error.
Failed to init. Killing module.
Loc distance: 53.56,10.05 -> 91.00,181.00 : 2449.69 km.
New location received: 53.56, 10.05.
Set timeout of 0s 1ns on fd 11.
Emitting 'Location' property
Next alarm due to: Sun Oct 17 17:49:00 2021
Set timeout of 18189s 0ns on fd 11.
Emitting 'Sunrise' property
Emitting 'Sunset' property
Emitting 'DayTime' property
set_temp(): Operation not permitted
Failed to set gamma temperature.
Set timeout of 0s 1ns on fd 12.
Sensor '/dev/iio:device1' is now available.
Resumed as a sensor is now available.
Emitting 'NextEvent' property
Emitting 'SensorAvail' property
capture_frames_brightness(): Operation not permitted
Set timeout of 600s 0ns on fd 12.
^CReceived 2. Leaving.
Latest location stored in cache file!

Do I have to add the user to some group maybe? It also says that it is killing uPower. Is that maybe the issue?

Checklist

FedeDP commented 3 years ago

Hi @coldspark29 ! I am so proud to hear that you're testing Clight on a pinephone! Looking at the verbose log you provided, it seems that Clightd is always responding EPERM (indeed, not just backlight, but gamma and idle too!). I am wondering what's causing the bug. Maybe Clightd is not running as root? Or polkit/user session is not correctly set up? (the latter one looks the most probable culprit to me!) To quickly test if it is a polkit issue, you can try: busctl call org.clightd.clightd /org/clightd/clightd/Backlight org.clightd.clightd.Backlight GetAll "s" ""

Ie: if Getters are working, but Setters are not, it means that we have a polkit issue (clightd does not allow non active user sessions to call its Setter methods!)

jclsn commented 3 years ago

And I was looking at your code yesterday and thought that it is some pretty sophisticated C code in an almost objective style. I couldn't figure out how you query the sensors really. I always use a plain read() command on sensors in /dev. I also couldn't cat /dev/iio:device1. Are brightness sensors different? Or is it the industrial IO?

Anyway seems like I don't need to understand your code. It was indeed a polkit issue and clight works on the Pinephone. It needs some brightness curve settings though. The defaults are too dark.

And well there is no brightness control solution yet. I think clight would be a good choice as it already implements a good basis. I think a cellphone solution should be easier accessible though. clight-gui seems not to be supported on ARM architectures. Maybe we could come up with a perfect config file for the Pinephone.

I think clight has one drawback though. When it is activated and you manually set the brightness higher, the override doesn't last that long. So it gets dark again and this could become very annoying on phones. But this is just a suspection. I will tinker with it a lot now. I wonder how iPhones and modern Android phones handle manual settings. Maybe there is some kind of machine learning that remembers the user's preferences or the override lasts longer - maybe until the screen turns off again.

I think with some adjustments clight could become the standard app on mobile Linux phones. We will see :)

jclsn commented 3 years ago

Can you somehow increase the polling interval to make it react faster to ambient light changes?

I was also wondering how iPhones do their amazing brightness control. I was watching how the brightness bar was changing on the train and it was reacting instantly. Really amazing! I believe it must be an observer pattern that registers to the XNU kernel and gets notified upon changes. Or it is a hardware controlled solution with its own small microcontroller. I think if clight did poll every few milliseconds, it would consume a lot of power, wouldn't it? For laptops such a dynamic brightness control is rarely needed, as they are mostly stationary, but for phones I think we would need to increase the polling interval a bit.

nullobsi commented 3 years ago

clight-gui seems not to be supported on ARM architectures.

i dont see why it shouldn't be--but the interface could hardly be called touch friendly 😆

but since I already have C++ models, perhaps a QML/QT Quick app is in order :)

FedeDP commented 3 years ago

And I was looking at your code yesterday and thought that it is some pretty sophisticated C code in an almost objective style

Yep, i use my own libmodule library to create an actor like application.
I think that architecture helps Clight be much easier to maintain and develop ;)

I also couldn't cat /dev/iio:device1. Are brightness sensors different? Or is it the industrial IO?

You should look for it in Clightd: https://github.com/FedeDP/Clightd/blob/master/src/modules/sensors/als.c. It uses the iio subsystem and libudev to find a matching device.

Anyway seems like I don't need to understand your code. It was indeed a polkit issue and clight works on the Pinephone. It needs some brightness curve settings though. The defaults are too dark.

Yaaaayyyyy!! That's great!

Maybe we could come up with a perfect config file for the Pinephone.

That would be fine, indeed; but i guess having a small gui would be great too!
I am not much into GUI though, hopefully someone in the pinephone community can step up :)

When it is activated and you manually set the brightness higher, the override doesn't last that long.

You mean that on next sensor polling, a different value is set?
This could be easily fixed by enabling the NoAutoCalib feature on Clight when user touches the backlight; then, if user wishes to go back to "auto", "NoAutoCalib" should be set to false, thus allowing clight to restart its calibration. It's super simple yet effective!

Note that Clight has quite a big dbus API that allows user/application to fully manage it! (you can even update the ambient brightness to backlight curve on the fly!)

I think with some adjustments clight could become the standard app on mobile Linux phones. We will see :)

You know, i'd be thrilled! I am looking forward for it; unfortunately, i do not own a pinephone, thus i am not able to help. Anyway, feel free to reach me for any issue/question!

Finally, if you are available, we could add a wiki page on Clight for Pinephone support! I can give you repo access if you are willing to!

FedeDP commented 3 years ago

PS: we can keep this open to further discuss!

FedeDP commented 3 years ago

Can you somehow increase the polling interval to make it react faster to ambient light changes?

Yes, see {ac,batt}_timeouts in conf file: https://github.com/FedeDP/Clight/blob/master/Extra/clight.conf#L71

FedeDP commented 3 years ago

@coldspark29

I think if clight did poll every few milliseconds, it would consume a lot of power, wouldn't it?

I don't think polling the ambient light sensor eg: every second, would be too much expensive; but obviously i never tried!

FedeDP commented 3 years ago

It also says that it is killing uPower. Is that maybe the issue?

Nope, but that's actually a feature. In the latest versions Clight kills UPower support if it is not on a laptop device (i never thought about the pinephone!); this can be easily reverted to support ac/battery state too!

jclsn commented 3 years ago

Okay I think I found some values that work. It can actually be done. I have an idea for the changing of the brightness. I think we would need a non-linear change of brightness. Atm you can just set a timeout and it will linearly increase or reduces the brightness, but on sudden changes, the brightness should change suddenly and else just slowly to not be too distracting.

On my laptop I have a trans step of 0.001 so I barely ever realize that it is doing something. On a phone you should also not realize it, but as the ambient lighting might change more dynamically, the transitions steps (or the transition speed with the same smooth steps) should also adjust dynamically. I think this would be a killer feature!

jclsn commented 3 years ago

Hmm getting an IO error now on capturing the frames. I already tried restarting. Did I maybe set the timeout too high with 1 second? How can I debug this? It doesn't seem to be related to the config file. Hopefully I didn't break anything :/

Set timeout of 30s 0ns on fd 11.
Not a laptop device. Killing UPower module.
Failed to retrieve AC state; fallback to ON_AC and OPEN lid.
Emitting 'AcState' property
Set timeout of 0s 1ns on fd 20.
Emitting 'LidState' property
Set timeout of 30s 0ns on fd 20.
53.61 9.93 loaded from cache file.
Loc distance: 53.61,9.93 -> 91.00,181.00 : 2445.85 km.
New location received: 53.61, 9.93.
Set timeout of 0s 1ns on fd 21.
Emitting 'Location' property
Currently inside an event.
Next alarm due to: Mon Oct 18 18:47:00 2021
Set timeout of 1513s 0ns on fd 21.
Set timeout of 0s 1ns on fd 27.
Sensor '/dev/iio:device1' is now available.
Resumed as a sensor is now available.
Emitting 'Sunrise' property
Emitting 'Sunset' property
capture_frames_brightness(): Input/output error
Set timeout of 1s 0ns on fd 27.
Emitting 'DayTime' property
Emitting 'NextEvent' property
Emitting 'InEvent' property
Emitting 'SensorAvail' property
capture_frames_brightness(): Input/output error
Set timeout of 1s 0ns on fd 27.
capture_frames_brightness(): Input/output error
Set timeout of 1s 0ns on fd 27.
capture_frames_brightness(): Input/output error
Set timeout of 1s 0ns on fd 27.
capture_frames_brightness(): Input/output error

Guess this will need some more work, but thank you for the support :)

jclsn commented 3 years ago

but since I already have C++ models, perhaps a QML/QT Quick app is in order :)

@nullobsi Great to see that you are motivated as well. :) I think for the start it would be good to have a control widget. In the end it would be nice to have it implemented in the various desktop environments, but that will take a long time anyway.

I would also want to see first how this is received by the others. I am not sure if the devs of the desktop environments are already working on something, but I think this code has come already so far, that it just needs some slight modifications to also work for phones.

FedeDP commented 3 years ago

Did I maybe set the timeout too high with 1 second?

It could be, maybe the ALS device does not allow for that frequent pings? That's weird though!

I think for the start it would be good to have a control widget.

IMO i think a plasma5 panel applet would be best!

@coldspark29 Can you go on testing and then writing here a checklist of things to do, keeping it updated? I am very interested in giving pinephone the best clight experience ;) Thank you very much both! We will make it!

FedeDP commented 3 years ago

On my laptop I have a trans step of 0.001 so I barely ever realize that it is doing something. On a phone you should also not realize it, but as the ambient lighting might change more dynamically, the transitions steps (or the transition speed with the same smooth steps) should also adjust dynamically. I think this would be a killer feature!

This is a nice feature; let me recap: you want a transition time that is constant, ie: going from 0.30 to 0.25 would take the same time as going from 1.0 to 0.2? I think it can easily added. Please, add this feature request to the aforementioned checklist! Thank you very much!

jclsn commented 3 years ago

Can you go on testing and then writing here a checklist of things to do, keeping it updated?

Sure, I can do that. :)

This is a nice feature; let me recap: you want a transition time that is constant, ie: going from 0.30 to 0.25 would take the same time as going from 1.0 to 0.2?

Yes, pretty much. I just verified this behavior on the iPhone. When you hold it in front of a lamp and then put your finger on the sensor, it goes from 100% brightness to 10 % brightness in an instant after like three seconds. For getting brighter is does this a little more slowly but basically the same behavior. Apart from that I have found quite some good settings now. They just don't work always realiably unfortunately, but it could be that there are some issues with the OS I have. I already asked in the Pinephone group. We will see who is interested in helping.

jclsn commented 3 years ago

I will post the optimal settings here and will update them if I find better ones. I am always open for improvements

verbose = true;

backlight:
{
    trans_step = 0.0005;
    trans_timeout = 2;
    ac_timeouts = [ 1, 1, 1 ];
    batt_timeouts = [ 1, 1, 1 ];
    # no_auto_calibration = true;
};

sensor:
{
    ac_regression_points = [ 0.1, 0.4, 0.5, 0.86, 0.88, 0.9, 0.92, 0.94, 0.96, 0.98, 1.0 ];
    batt_regression_points = [ 0.1, 0.4, 0.5, 0.86, 0.88, 0.9, 0.92, 0.94, 0.96, 0.98, 1.0 ];
};

keyboard:
{
    disabled = true;
};

gamma:
{
    disabled = false;
};

dimmer:
{
    disabled = true;
};

dpms:
{
    disabled = true;
}
FedeDP commented 3 years ago

Btw i find really stunning that we are actually talking about a phone! I mean, with geoclue, upower whatever! It's incredible!

jclsn commented 3 years ago

Yeah, it will be pretty amazing. They have just announced an upgraded version with a faster processor a few days ago, that will have the potential to be usable as daily phone and fully-fledged PC in your pocket.

FedeDP commented 3 years ago

It's crazy! I saw that too, fingers crossed for it to be a success!

jclsn commented 3 years ago

So regarding the IO errors: They only seem to occur after killing clight and starting it manually. What is the proper way to stop clight and start it for debugging? Or maybe they just occur occasionally which is weird. This should definitely be fixed.

I was also thinking about the new adaptive trans_timeout feature yesterday. There may be cases where this is unwanted, e.g. when flashes from another camera or the lights of a passing hit the sensor for an instant. So there should be a certain delay of a few seconds. So if a certain threshold of light difference is crossed AND that state persists for more than 3-5 seconds, adjust the brightness instantly.

EDIT: Actually I just figured that setting the ac and batt timeouts to 3 seconds accomplishes this already. It also makes the transitions a bit smoother and I think they are still fast enough. Maybe we don't need this new feature at all. I will update the suggested config file.

jclsn commented 3 years ago

Really weird, the IO errors don't really follow a pattern. Maybe the hardware is broken

FedeDP commented 3 years ago

Clightd returns EIO when no capture succedeed (ie: none of clight conf.num_captures returned a value !=0). Try to check dmesg if there is any message about the iio device! Clightd does not much more than using libudev to read the sensor value btw!

FedeDP commented 3 years ago

Latest Clight allows back UPower module to run on Pinephone too :) (ie: on all devices, not laptops only!)

jclsn commented 3 years ago

Will see if I find something tomorrow. I already compiled the latest version. What does UPower do exactly?

FedeDP commented 3 years ago

What does UPower do exactly?

UPower allows to manage ac states; ie: on battery, longer timeouts will be set, while on AC shorter ones (by default). This is good for battery usage on laptops.

jclsn commented 3 years ago

Guess that was why Clight did use the ac_timeout values. Was about to say that this isn't really necessary for a phone, but the Pinephone is supposed to be used as a desktop computer when hooked to the dock, so there is a point.

FedeDP commented 3 years ago

Yup, that's the point ;)

jclsn commented 3 years ago

So, I don't really see anything in dmesg. The IO errors just seem to appear after you have forcefully killed clight. Then is also doesn't work after restarting, which is weird. Then it works again suddenly.

FedeDP commented 3 years ago

Hi! Are you comfortable with building clightd with some modifications? You could try to add a printf("xxx %s\n", val); right after line 79 in als.c sensor plugin: https://github.com/FedeDP/Clightd/blob/master/src/modules/sensors/als.c#L79, and then starting Clightd/Clight manually. This way, you will see what value does Clightd actually read from the sensor; that will help to understand the root cause of the issue! Thank you very much for your help!

jclsn commented 3 years ago

Tried that but it doesn't print anything. Added one at the beginning of the function so the capture() function is not even called.

FedeDP commented 3 years ago

Did you compile and run the correct executable(it needs sudo, ie: sudo ./clightd) ?

jclsn commented 3 years ago

I ran the service

FedeDP commented 3 years ago

Uh no! The service still run the /usr/bin/clightd. You need to directly run the build/clightd executable, and then starting clight!

jclsn commented 3 years ago

Ah alright it is printed in clightd. Here are the values. It is weird that it is capturing ten values although I only put 5 in the config.

Capturing...
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
Capturing...     // Used a flashlight here
xxx (null)
xxx 29
xxx (null)
xxx 29
xxx (null)
xxx 29
xxx (null)
xxx 29
xxx (null)
xxx 29
Capturing...
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
xxx (null)
xxx 2
FedeDP commented 3 years ago

Sorry if I wasn't clear enough!

jclsn commented 3 years ago

No problem. I could have thought of that myself actually.

FedeDP commented 3 years ago

It is weird that it is capturing ten values although I only put 5 in the config.

Indeed it is fine: iio API exposes multiple attributes for the illumination read from sensor; some sensors will only expose one, other 2; thus, clightd tries the first that it found working between static const char *ill_names[] = { "in_illuminance_input", "in_illuminance_raw", "in_intensity_clear_raw" };.

I just pushed a commit to clightd with a small but important fix; care to try? Thanks!

jclsn commented 3 years ago

Tried but still the same behavior

FedeDP commented 3 years ago

Can you share another output? Thanks!

jclsn commented 3 years ago

The output is the same. What are you looking for exactly?

FedeDP commented 3 years ago

xxx 2 xxx (null) xxx 2 xxx (null) xxx 2 xxx (null) xxx 2 xxx (null) xxx 2

With the new fix, i'd expect the read values to be actually different.
Anyway, are you still experiencing the Input/Output errors right now? Ie: is above log taken from 3 captures that gave the error?

jclsn commented 3 years ago

Yes that were three captures. I put a printf at the beginning of every capture. The output is still the same and the IO errors are still there as well.

FedeDP commented 3 years ago

Mind to print also the scale value?

FedeDP commented 3 years ago

Sorry if it seems i make lots of weird question, but it is not easy to understand the root cause without the hardware :)

jclsn commented 3 years ago

I could give you SSH access to the hardware if that helps?

FedeDP commented 3 years ago

That would be great man! Before that, can you try with the newly pushed clightd?

jclsn commented 3 years ago

Seems like the IO errors are gone. :) This is the new output from clightd

[default]|BACKLIGHT2|: Target pct: 0.52
[default]|BACKLIGHT2|: backlight reached target backlight: 0.52.
xxx (null)
xxx 77
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
[default]|BACKLIGHT2|: Target pct: 0.52
[default]|BACKLIGHT2|: backlight reached target backlight: 0.52.
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
[default]|BACKLIGHT2|: Target pct: 0.52
[default]|BACKLIGHT2|: backlight reached target backlight: 0.52.
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
xxx (null)
xxx 78
FedeDP commented 3 years ago

Yayyyyy! That's huge! Thanks you very much for your patience :D

jclsn commented 3 years ago

It really hasn't occured in a while now and I checked that point in the list :)

Can you make it possible to set the ac_timeout and batt_timeout lower than one second? Since it is the lowest value the transition always occurs in pulses. Using a lower timeout could make it more smooth. Or do you have a better idea?

I think we will also need the adaptive transition timeout. If you set it too low the brightness changes too fast if you just barely move the phone. If you set it too high, it takes a long time to transition from bright to dark or vice versa.

FedeDP commented 3 years ago

It really hasn't occured in a while now and I checked that point in the list :)

Yay!!

Can you make it possible to set the ac_timeout and batt_timeout lower than one second? Since it is the lowest value the transition always occurs in pulses. Using a lower timeout could make it more smooth. Or do you have a better idea?

Mmmh i think i can try! We can try something like: setting the timeout > 100 will mean in ms, below instead seconds.
I don't want to break existing configurations (by parsing the value in milliseconds by default, instead of seconds). It would be weird but better than nothing i guess :) (obviously the behavior will be documented in the conf file!)

EDIT: this wouldn't work as actually timeouts are already larger than 100s by default :D that was pretty stupid!

I think we will also need the adaptive transition timeout. If you set it too low the brightness changes too fast if you just barely move the phone. If you set it too high, it takes a long time to transition from bright to dark or vice versa.

Mmh so as i suggested before, a fixed transition length? eg: i always want the backlight to change in 2s; if it has a large delta, it will make lots of fast backlight change, instead if delta is small, it would make a few slow steps. What do you think?