New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 765991 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Task



Sign in to add a comment

Consider cleaning up names of powerd backlight-related tools

Project Member Reported by derat@chromium.org, Sep 17 2017

Issue description

The chromeos-base/power_manager package installs various tools for working with backlights (source code at https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/tools/):

- backlight_tool: Uses powerd's system::InternalBacklight class to get the panel or keyboard backlight's current/max levels and set the current level. It only supports simple linear conversions between percentages and levels, and can't really be used while powerd is running (since powerd will just overwrite its changes).

- backlight_dbus_tool: Calls powerd's {Get,Set}ScreenBrightness and {Increase,Decrease}ScreenBrightness D-Bus methods, along with the equivalent keyboard backlight calls. This is the same interface used by Chrome to query and set the brightness. It uses non-linear percents (because conversions to levels happen within powerd).

- get_powerd_initial_backlight_level: Initializes a bunch of powerd's internal classes to determine the default panel and keyboard brightness levels that powerd would use. Also has --level_to_percent and --percent_to_level flags that convert between levels and non-linear percents, which has nothing to do with initial levels but was convenient to put here (see https://chromium-review.googlesource.com/c/chromiumos/platform2/+/658320).

As Eric pointed out, this is pretty confusing.

- backlight_tool is used by a zillion things (chromeos-boot-alert, factory wipe, tons of Autotest-based tests, etc.).

- backlight_dbus_tool doesn't look like it's used by anything, although maybe some developers sometimes run it manually.

- get_powerd_initial_backlight_level is used by some Autotest-based tests that want to measure battery life, I think.

I can potentially delete backlight_dbus_tool after sending out a PSA to the power team. I might be able to move --level_to_percent and --percent_to_level into backlight_tool after a bit of refactoring to make that code easier to call from InternalBacklightController, too.
 

Comment 1 by derat@chromium.org, Sep 21 2017

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b51e67c6c09246f37472a8ed5b74d057f7b7c568

commit b51e67c6c09246f37472a8ed5b74d057f7b7c568
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 22 13:10:15 2017

power: Update backlight_tool to convert between units.

Add new flags to the backlight_tool helper for converting
between hardware brightness levels, linear brightness
percents, and the nonlinear percents used within powerd:

  --level_to_linear
  --level_to_nonlinear
  --linear_to_level
  --linear_to_nonlinear
  --nonlinear_to_level
  --nonlinear_to_linear

Also remove get_powerd_initial_backlight_level's
recently-added --level_to_percent and --percent_to_level
flags and suppress log spam from both tools.

BUG= chromium:765991 
TEST=manual: ran it; values look right

Change-Id: I7eee8998be2838d484dd8f7dc40b785f8eddd8b0
Reviewed-on: https://chromium-review.googlesource.com/677412
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/tools/backlight_tool.cc
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/backlight_controller_stub.cc
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/tools/get_powerd_initial_backlight_level.cc
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/internal_backlight_controller.h
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/backlight_controller.h
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/internal_backlight_controller.cc
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/power_manager.gyp
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/external_backlight_controller.h
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/keyboard_backlight_controller.cc
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/keyboard_backlight_controller.h
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/backlight_controller_stub.h
[modify] https://crrev.com/b51e67c6c09246f37472a8ed5b74d057f7b7c568/power_manager/powerd/policy/external_backlight_controller.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/6d2efc601ab026b43082a5d1466cf37c7a17c70c

commit 6d2efc601ab026b43082a5d1466cf37c7a17c70c
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 23 07:40:04 2017

power: Add --get_initial_brightness to backlight_tool.

Add the rest of get_powerd_initial_backlight_level's
functionality to backlight tool. --get_initial_brightness,
--force_battery, and --lux flags are added.

BUG= chromium:765991 ,b:38208252
TEST=manually verified that --get_initial_brightness prints
     same values as get_powerd_initial_backlight_level

Change-Id: If97553ab3f4e8b477cafa60418c8c5033c76eb06
Reviewed-on: https://chromium-review.googlesource.com/678437
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/6d2efc601ab026b43082a5d1466cf37c7a17c70c/power_manager/tools/backlight_tool.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/3aa91e670a60e23ffea7d666da63ba45ab0d604f

commit 3aa91e670a60e23ffea7d666da63ba45ab0d604f
Author: Daniel Erat <derat@chromium.org>
Date: Mon Sep 25 22:52:45 2017

autotest: Stop using get_powerd_initial_backlight_level.

Update the get_powerd_initial_backlight_level calls in
power_utils.Backlight and KbdBacklight to use backlight_tool
instead. I plan to delete the former program in favor of the
latter, which now has a --get_initial_brightness flag.

BUG= chromium:765991 
TEST=ran power_LoadTest.fast and checked that it logged
     default backlight level

Change-Id: I445df29ca6a740b5d8276616407bbd4fa3bd8bd4
Reviewed-on: https://chromium-review.googlesource.com/679984
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/3aa91e670a60e23ffea7d666da63ba45ab0d604f/client/cros/power_utils.py

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/70c3bf39416b0985ee73908fd245bc8179b465be

commit 70c3bf39416b0985ee73908fd245bc8179b465be
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 30 06:25:35 2017

power_manager: Stop installing unneeded backlight tools.

Make chromeos-base/power_manager stop installing
backlight_dbus_tool and get_powerd_initial_backlight_level.
See the chromium-os-dev PSA at https://goo.gl/wP2Rvn for
more details.

BUG= chromium:765991 
TEST=compiled it

Change-Id: If4e05619acb52a530f6bb13422ac7e0be3388ab2
Reviewed-on: https://chromium-review.googlesource.com/693299
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/70c3bf39416b0985ee73908fd245bc8179b465be/chromeos-base/power_manager/power_manager-9999.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c435758fc9d43d62e5c139fbd96d903704293fcc

commit c435758fc9d43d62e5c139fbd96d903704293fcc
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 30 06:25:33 2017

power: Force hovering in backlight_tool.

Make backlight_tool force the hovering state when run with
--keyboard. This can affect the value printed by
--get_initial_brightness and matches what was done by
get_powerd_initial_backlight_level.

BUG= chromium:765991 
TEST=it builds

Change-Id: Ief389f2f19865a81438312ce7d803aa977c574f5
Reviewed-on: https://chromium-review.googlesource.com/693306
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/c435758fc9d43d62e5c139fbd96d903704293fcc/power_manager/tools/backlight_tool.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/94891cf1e7bfc29baa9ec0590329a7fdda85d7d0

commit 94891cf1e7bfc29baa9ec0590329a7fdda85d7d0
Author: Daniel Erat <derat@chromium.org>
Date: Mon Oct 02 10:03:41 2017

power: Delete redundant backlight-related tools.

Delete backlight_dbus_tool and
get_powerd_initial_backlight_level. See the chromium-os-dev
PSA at https://goo.gl/wP2Rvn for more details.

BUG= chromium:765991 
TEST=emerged the power_manager package
CQ-DEPEND=If4e05619acb52a530f6bb13422ac7e0be3388ab2

Change-Id: If04562476ecb8bb4a6e1a4b09424c898416662eb
Reviewed-on: https://chromium-review.googlesource.com/693304
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[delete] https://crrev.com/58c65f95b39a8daec19b3993c784d13bd1cfaee8/power_manager/tools/backlight_dbus_tool.cc
[modify] https://crrev.com/94891cf1e7bfc29baa9ec0590329a7fdda85d7d0/power_manager/power_manager.gyp
[delete] https://crrev.com/58c65f95b39a8daec19b3993c784d13bd1cfaee8/power_manager/tools/get_powerd_initial_backlight_level.cc

Comment 8 by derat@chromium.org, Oct 2 2017

Status: Verified (was: Started)

Sign in to add a comment