GNOME Bugzilla – Bug 745181
[PATCH] power: Add support for ambient light sensors
Last modified: 2015-05-22 13:25:24 UTC
Created attachment 297922 [details] [review] initial patch This supports the ColorHugALS USB device as well as built-in kernel-supported sensor devices. You may have to load an out-of-tree module for kernel support. Allan has ackd the new UI for this (new bug for g-c-c coming up in a few mins) -- I'm also happy to send a free USB device to anyone that wants to help with this functionality. Comments welcome, thanks.
It would be really nice if you could support the applesmc driver - it's a hwmon driver that has a single "light" sysfs attribute with a single number or two numbers formatted (%d,%d), depending on the number of sensors installed. Or even better, ideally all drivers would support the same interface, and in my opinion (but I'm not a kernel dev) the hwmon makes the most sense and it's already supported by a mainline driver. Modifying the ACPI patches to use this interface should be trivial, while writing a colorhug kernel driver might be harder but would definitely simplify the userspace and avoid device specific code.
Review of attachment 297922 [details] [review]: ::: data/org.gnome.settings-daemon.plugins.power.gschema.xml.in.in @@ +68,3 @@ </key> + + <!-- ambient light sensor --> Don't like the amount of tweaking here, there really shouldn't be anything but "enabled". @@ +72,3 @@ + <default>true</default> + <summary>Enable the ALS sensor</summary> + <description>If the amient light sensor functionality is enabled.</description> ambient. ::: plugins/power/gsd-power-manager.c @@ +2199,3 @@ + } + + ch_ambient_get_value_async (manager->priv->ambient, NULL, As mentioned by Giovanni, I'd prefer if the interface used was something already supported by most ALS sensors in the kernel. Furthermore, there are a number of IIO sensors that implement the "light" interface. I could add support for that directly in iio-sensor-proxy and work some more on that interface. (There's such a piece of hardware in the Carbon X1 gen 2 I have, but the driver didn't get merged yet: http://patchwork.ozlabs.org/patch/331551/ ) @@ +2209,3 @@ + +static void +ch_backlight_start_poll (GsdPowerManager *manager) The poll should only be started when the session becomes the current one again. @@ +2875,3 @@ + value = manager->priv->kbd_brightness_now; + retval = g_variant_new_int32 (value); + } This won't return errors when trying to get the Brightness property on the ALS interface.
I've added ALS support to iio-sensor-proxy at: https://github.com/hadess/iio-sensor-proxy Please review the (sparse) API there for now.
Err, even getting the values from iio-sensor-proxy we still need the algorithm to actually change the backlight brightness. Reopening, but understood about the driver issue.
Oh, yes. Duh.
Created attachment 303590 [details] [review] new patch using the iio-proxy API this is a *much* smaller patch. It still needs more testing, but works well enough for me, and well enough for a first proper review. Thanks!
Review of attachment 303590 [details] [review]: ::: data/org.gnome.settings-daemon.plugins.power.gschema.xml.in.in @@ +75,3 @@ + </key> + <key name="ambient-smooth" type="d"> + <default>0.3</default> Does this really need to be an option? We should hard-code it for now, and make it into an option later if necessary. ::: plugins/power/gsd-power-manager.c @@ +2,3 @@ * * Copyright (C) 2007 William Jon McCann <mccann@jhu.edu> + * Copyright (C) 2011-2015 Richard Hughes <richard@hughsie.com> "2011-2012, 2015" @@ +2443,3 @@ */ + if (manager->priv->backlight_available) { + manager->priv->ambient_percentage_old = backlight_get_percentage (manager->priv->rr_screen, NULL); Don't we need to do the same thing in handle_method_call_screen() (at least)? Maybe do this in backlight_iface_emit_changed() if iface == GSD_POWER_DBUS_INTERFACE_SCREEN? @@ +2479,3 @@ + if (val_has == NULL || !g_variant_get_boolean (val_has)) + goto out; + val_lux = g_dbus_proxy_get_cached_property (proxy, "LightLevel"); This might not be in lux though. See the LightLevelUnit property. @@ +2562,3 @@ + /* setup ambient light support */ + manager->priv->iio_proxy = + g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, As with the gnome-control-center patch, we need to handle the iio-sensor-proxy daemon coming and going.
(In reply to Bastien Nocera from comment #7) > Review of attachment 303590 [details] [review] [review]: > > ::: data/org.gnome.settings-daemon.plugins.power.gschema.xml.in.in > @@ +75,3 @@ > + </key> > + <key name="ambient-smooth" type="d"> > + <default>0.3</default> > > Does this really need to be an option? We should hard-code it for now, and > make it into an option later if necessary. I think it does; this is exactly the kind of thing I'd expect to see in gnome-tweak-tool. Some people want to have the backlight change from one brightness to another as they walk inside/ouside and other people just want the brightness to be altered slowly throughout the day so it's not noticed. I don't think you can have one smoothing algorithm that can adapt to each situation. > ::: plugins/power/gsd-power-manager.c > @@ +2,3 @@ > * > * Copyright (C) 2007 William Jon McCann <mccann@jhu.edu> > + * Copyright (C) 2011-2015 Richard Hughes <richard@hughsie.com> > > "2011-2012, 2015" Fixed. > @@ +2443,3 @@ > */ > + if (manager->priv->backlight_available) { > + manager->priv->ambient_percentage_old = > backlight_get_percentage (manager->priv->rr_screen, NULL); > > Don't we need to do the same thing in handle_method_call_screen() (at least)? No, we set the ambient_norm_required flag in this method which means basically that we renormalise the ambient weighting to the new current backlight level. We do the renorm after the backlight step up so the user can always override the value. > @@ +2479,3 @@ > + if (val_has == NULL || !g_variant_get_boolean (val_has)) > + goto out; > + val_lux = g_dbus_proxy_get_cached_property (proxy, "LightLevel"); > > This might not be in lux though. See the LightLevelUnit property. Yes, val_lux was a bad choice of variable name. The normalisation code was written in this relative way to account for the disagreement on the lux value between sensors. As long as the value obtained from LightLevel is approximately linear the offset and scale isn't important, and so the unit isn't so important. If the value is a radiometric measure then we'd need to know the full spectrum to convert it to a Lux measurement, which I'm guessing an ALS sensor won't do. > @@ +2562,3 @@ > + /* setup ambient light support */ > + manager->priv->iio_proxy = > + g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, > > As with the gnome-control-center patch, we need to handle the > iio-sensor-proxy daemon coming and going. Fixed. New patch coming up soon.
Created attachment 303737 [details] [review] new patch using the iio-proxy API
Review of attachment 303737 [details] [review]: Note, I'm going to make some changes to the Claim/Release API: https://github.com/hadess/iio-sensor-proxy/issues/23 The problem is that it's too complicated for clients to start claiming the device when the sensor appears, and if the daemon is there, it's much easier to start polling if necessary if the sensor type is already claimed. ::: data/org.gnome.settings-daemon.plugins.power.gschema.xml.in.in @@ +69,3 @@ + <description>If the ambient light sensor functionality is enabled.</description> + </key> + <key name="ambient-smooth" type="d"> Still not wanting it as a config yet. ::: plugins/power/gsd-power-manager.c @@ +994,3 @@ + + if (!g_dbus_proxy_call_sync (manager->priv->iio_proxy, + active ? "ClaimLight" : "ReleaseLight", Might want to check if it's already claimed. @@ +2506,3 @@ + val_has = g_dbus_proxy_get_cached_property (proxy, "HasAmbientLight"); + if (val_has == NULL || !g_variant_get_boolean (val_has)) + goto out; If the light sensor appears while the proxy is there, you'll never actually claim it. @@ +2563,3 @@ + g_signal_connect (manager->priv->iio_proxy, "g-properties-changed", + G_CALLBACK (iio_proxy_changed_cb), manager); + iio_proxy_claim_light (manager, TRUE); You can't claim if there's no hardware. Cache has_als in iio_proxy_changed_cb() ? (and reset it when the proxy disappears below)
Pushed to master after removing the config smoothing, thanks!