Brightness controls try6

[tchx84]

This PR is for adding brightness control capabilities to Sugar. It includes a generic mechanism to interact with backlight devices, a jarabe model that acts as an abstraction for the sugar shell, keyboard control keys and a device frame UI.

Notes for those testing in sugar-build:

1. Disable broot.
2. Copy the generated polkit action file to the real filesystem. ie.,

$ cp ./build/out/install/share/polkit-1/actions/org.sugar.brightness.policy /usr/share/polkit-1/actions/

[tchx84]

I find it annoying how github messes with the commits order, randomly. Please see the original commits order at https://github.com/tchx84/sugar/commits/brightness-controls-try6

[tchx84]

One more thing, this PR requires this other PR sugarlabs/sugar-artwork#39

[quozl]

Reviewed, but not tested. I looked at the effort required to test; extract these patches and make a package with them, or apply the patches to the filesystem after install. Both were greater than the time I had available, and would be wasted if there were further changes after review. Sorry!

[tchx84]

You could also do:

git rebase -i 2cdba447c8762d7acda8d5cfb0ad74dbc5ca3cd3~

Then mark all the commits as "f" or "fix" (except for the first one, in descending order), and it would generate 1 BIG commit with all these changes, which you can then export to place it in the packaging files.

It shouldn't take long.

These changes were committed and formatted in such way so they actually could be reviewed, instead of just have 2-3 "unreviewable" huge commits, so thanks for reviewing them. Any comments?

[samdroid-apps]

I tested this. It works!!!

Here are my comments:

1. The 500ms delay to apply the setting is very off. It does not feel very responsive.

• And if you think about the use case; I don't usually know how bright (as a percentage) I want my screen, I just know when it is too bright (I start blinking) or when it is too dim (I can't see it). For those use cases, it is much better to have instant feedback.

2. (My fault) The "my display" section should probably go before the speech icon (so it is next to the speaker, not the speech). Then it will be ordered like: wifi network, speech, my display, my volume, my battery.
3. Frame and slider do not change with you press the keys. This is probably an osbuild issue, not an issue with your code.

[tchx84]

Thanks for testing @samdroid-apps!

1. I encourage you to play with lower values and to see how it feels, but also try removing that patch and move the progress bar aggressively it won't feel responsive either because it makes the UI lag.
2. We can change that. BTW, any progress on the icon side?
3. Weird, I even try it running the helper from outside sugar and I do see the frame I con and progress bar being updated.

EDIT: if you are using sugar-build you can also try to temporarily kill GSD to make sure Sugar is under full control http://askubuntu.com/questions/93578/how-do-i-kill-gnome-settings-daemon

[tchx84]

I think I understand what you saw on (3) @samdroid-apps :)

When changing the brightness value, via keys or progress bar, the monitor handler was being blocked more times than it was being unblocked. I would expect that blocking handler would be idempotent operation, but it doesn't seems to be. So, for every handler block there has a to be a corresponding number of unblocks.

I solved it now, to make sure it would only be blocked once and unblocked once.

Please give it a try.

[samdroid-apps]

New icons branch: sugarlabs/sugar-artwork#51

[tchx84]

Pushed another change to reduce progress bar delay to 200ms, it feels more responsive ;)

[tchx84]

Added two more fixes, updating the POTFILES files and fixing the polkit action policy file declaration in the data/Makefile

[godiard]

@tchx84 , please consolidate a little, is difficult to follow with 16 patches :)

[quozl]

Tested on commodity hardware using Ubuntu packages. Works well. Some suggestions:

• when the brightness or progress bar is being moved by hotkey, update with no delay at all; at the moment it feels like between one twentieth and one fifth of a second, and it is possible to get ahead of the system by several seconds by pressing brightness up and brightness down keys many times,
• when the progress bar is being moved by mouse, ensure the brightness is updated at least every tenth of a second, and within a tenth of a second of movement ceasing,
• when the progress bar is visible, decrease the time between an external change (echo > /sys) and the change being shown on the slider, to a tenth of a second.

When the mouse is placed in the area of the progress bar where the marker is not present, and then clicked, there is no response. Is this expected? For the audio icon the progress bars can be moved in this way.

I'm not sure it is time to consolidate patches, and would be fine with re-opening the pull request rather than losing discussion context by opening a new one.

[tchx84]

Hello @quozl, thanks for testing. Glad it works ;)

Regarding the comments:

• There is no intentional delay for hotkeys. I think this has to do with the way Gtk prioritizes events, and maybe there is something in the palette callback, like to force redraw (just guessing for now).
• Will look into it.
• I can increase the rate. Any suggestions on how can I measure the "cost" of these monitors?

[quozl]

There's no such delay with volume hotkeys. So I doubt it is event prioritisation. The delay occurs even if the frame icon is not visible, so it isn't redraw cost. It seems likely to be the long path through polkit and the helper.

The helper itself run from shell prompt takes 0.11s, which is huge. This may need to be recoded in C for speed. A comparable C program which writes a line of text to sysfs takes about 0.002s.

time (for x in $(seq 100); do
    /usr/bin/sugar-backlight-helper --set-brightness 800; done)

time (for x in $(seq 100); do
    /bin/echo 800 > /sys/.../brightness; done)

The output was 11.092s and 0.204s on commodity hardware.

I've not measure the cost of polkit.

For measuring the cost of monitors, count the calls, and time the duration of each call. A simple timing method is to call time.time(), and this is also used in logging messages, so adding a logging to each call can also be useful. But doing all this slows things further, so do it with care.

[quozl]

usd-backlight-helper is a C program that does the same thing, though ldd(1) shows it has many libraries compared to /bin/echo.

Testing the impact of polkit, using 1000 calls:

• usd-backlight-helper naked is 0.005409s,
• usd-backlight-helper wrapped by polkit is 0.041925s, an additional 0.036516s,
• sugar-backlight-helper naked is 0.111916s,
• sugar-backlight-helper wrapped by polkit is 0.150064s, an additional 0.038148s.

So polkit costs about 37ms. Polkit is not the problem.

Testing the impact of libraries used by the respective programs. Cold cache tests, using 100 calls:

• usd-backlight-helper naked is 0.10039s,
• sugar-backlight-helper naked is 0.37426s,

So using Python for this costs at least 107ms, and no more than 374ms. Python is the problem.

Making the call asynchronous will help, but then we have to worry about what to do when another call is made while the previous call is still running, which will happen if the key repeats.

At the moment, forcing a key repeat gives very interesting lag, as the events are buffered and processed slowly at 150ms intervals. The key repeat events arrive at 40ms intervals.

[quozl]

A shell script costs only 3ms!

I wrote /usr/bin/sugar-backlight-helper.sh:

#!/bin/sh
x=/var/run/sugar/backlight
y=brightness
case "$1" in
    "--set-brightness")
        echo "$2" > $x/$y
        ;;
    "--get-brightness")
        read value < $x/$y
        echo "$value"
        ;;
    "--get-max-brightness")
        read value < $x/max_brightness
        echo "$value"
        ;;
esac

Then I made some other changes:

mv /usr/bin/sugar-backlight-helper /usr/bin/sugar-backlight-helper.py
ln -sf $(dirname $(/usr/bin/sugar-backlight-helper.py --get-path)) \
    /var/run/sugar/backlight
ln -s /usr/bin/sugar-backlight-helper.sh /usr/bin/sugar-backlight-helper

Almost all of the response time problems went away. The only remaining problem was lag between moving the slider with the mouse and the brightness being changed.

So if we can identify and save the path using the existing sugar-backlight-helper (which calls GUdev), and then use another helper for changing the value, this will be better. Or look at adopting code from usd-backlight-helper, which is slower.

[quozl]

Now that it works well; there are only 8 usable levels of brightness, and off; but the XO laptop has 15, and off.

[tchx84]

Thanks for the research @quozl. I agree with you regarding async calls and I don't think is worth to add "yet another C code", at least until we exhaust current alternatives.

The helper problems can be broken in 3 parts:

1. Python.
2. Finding and setting the device path.
3. Changing the brightness value.

As you just showed, (1) and (3) can be easily solved by the shell script you provided, is just writing to a file anyway, but I am wondering if decoupling the find-and-setup of the device path (2) could make things a bit more complicated for installing and packaging sugar. I see two options:

A. We provide a separate setup script (for 2) and make sure it runs in a "appropriate" moment, probably when sugar launches. This setup should not be in sugar installation files (they are used when building packages when no device available), neither on package time installation because it would create problems ie., when generating images on different hardware (the device could no be present or be a different path than in the target machine).
B. Or, we add the find of the device path step in the same shell script.

In the first option (A) I don't think it would make sense to keep sugar-backlight-helper as it is now, should probably be broken in two. And if we go this way, we would also need to take some security considerations because this "generated" path will be written by the helper in privileged mode, the last thing we want to provide a "write to any file" tool. So both creation of the path and use of the path should be done in privileged mode, plus ownership checks(?).

[godiard]

@tchx84, just to understand a few questions:

• Find the device path, can be done the first time time the change brightness is executed, or the polkit need to know that too? Can we pass that path as a parameter to sugar-backlight-helper?
  Then, can be a simple shell script I assume?

[tchx84]

@godiard, we shouldn't allow passing the path as parameter, because it would turn the helper into a "write to any file" tool, and yes, we could make it find and setup the path during the first sugar start but it would need to (a) be authorized by polkit too and (b) the helper should make some kind of ownership check to the file before writing to it, otherwise anyone could just create (linking to any file) that file and use the helper to write it.

[tchx84]

@quozl, @godiard, for the option B I mentioned, we could use something like this:

#!/bin/sh

subsystem=/sys/class/backlight

driver=$(ls ${subsystem} | head -1)
if [ -z "${driver// }" ]; then
    echo  "Device not found."
    exit 1
fi

device=$subsystem/$driver
case "$1" in
    "--set-brightness")
        echo "$2" > $device/brightness
        ;;
    "--get-brightness")
        read value < $device/brightness
        echo "$value"
        ;;
    "--get-max-brightness")
        read value < $device/max_brightness
        echo "$value"
        ;;
    "--get-path")
        echo "$device"
        ;;
esac

Which is basically James script plus a simplistic way of determining the path.

[godiard]

@tchx84, +1

[quozl]

Martin and I discussed in IRC. Using GUdev has advantages but we don't know what they are. We will split into two helpers; one to get path, one to make brightness changes. We can manage the symlink in /var/run to be secure. We can use 21 levels of brightness like in GNOME or Unity.

[quozl]

Tested your most recent branch, please go ahead with any necessary rebasing.

Using mouse or arrow keys while the palette is shown can sometimes jump the brightness. Low severity.

I've studied the lag on external changes. The lag happens before the changed signal is emitted by Gio.FileMonitor. It seems to have a fixed rate, which we'll have to accept.

An strace shows the kernel is notifying the shell process instantly on change, so the lag is on the path between the kernel and Gio.FileMonitor.

You can safely reduce MONITOR_RATE and TIMEOUT_RATE to 100, as these have no impact on CPU time, as measured by top(1).