client/site_tests: add power_BacklightControl test [chromiumos/third_party/autotest : master]

[Simon Que (Code Review)]

Simon Que has uploaded a new change for review.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

client/site_tests: add power_BacklightControl test

Tests the power manager backlight controller and whether it sets the
brightness level and panel on/off correctly.

BUG=chromium-os:36061
TEST=run power_BacklightControl successfully

Change-Id: Id399956ee55443d711429c191839bb227885da16
Signed-off-by: Simon Que <sq...@chromium.org>
---
A client/site_tests/power_BacklightControl/control
A client/site_tests/power_BacklightControl/power_BacklightControl.py
2 files changed, 234 insertions(+), 0 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/autotest refs/changes/88/37388/1
--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (11 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 79:
nit: one blank line between methods

Line 83: os.system('rm %s/*' % self._pref_path)
it'd be better to back up the existing values at the beginning of the test and restore them afterwards

Line 94: if utils.system_output('status powerd').find('start/running') !=-1:
nit: space after !=

Line 117: time.sleep(adjustment_delay_s)
this will be flaky and/or slow. please don't make tests depend on the system doing something within a particular amount of time

Line 120: if num_steps_taken > max_num_steps:
how can this ever fail? the previous while loop breaks when num_steps_taken == max_num_steps

Line 131: if num_steps_taken > max_num_steps:
what's being tested here? did you mean to have some other condition in the previous while statement?

Line 140: # Turn off the screen by decreasing brightness one more time.
nit: "... decreasing brightness one more time with allow_off=True.'

Line 196: pass
why is this here?

Line 201: pass
or here?

Line 205: cmd = 'backlight-tool --get_max_brightness'
this seems circular; it's using powerd's backlight code to verify that powerd's backlight code works correctly

Line 220: XSET = 'LD_LIBRARY_PATH=/usr/local/lib xset'
what's this for? isn't X already configured to e.g. disable dpms?

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

Gerrit-Reviewer: Daniel Erat <de...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Sameer Nanda <sna...@chromium.org>
Gerrit-Reviewer: Simon Que <sq...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Gerrit-Reviewer: Todd Broch <tbr...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (8 inline comments)

....................................................
File client/site_tests/power_BacklightControl/control
Line 15: - There are too many steps (> 16) between min and max brightness.
Aren't there supposed to be exactly 16 steps? If not please details the nuances. We should also check too few steps in that case as well.

Based on Chrome OS requirements

Backlight Driver Behavior The backlight for the display must be controlled by the operating system and not by the EC. A sysfs interface is required, the driver must support a node under /sys/class/backlight/ providing readable actual_brightness and max_brightness files as well as a read/write brightness file. If multiple potential devices are found the one with the largest range is selected - this can happen if there is a direct driver as well as a legacy ACPI driver.

Should this test verify that we're using the 'largest range' backlight in sysfs to satisfy the req?

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py

Line 94: if utils.system_output('status powerd').find('start/running') !=-1:

Won't file tagger/inotify see these file changes and re-read? Do you need to manipulate powerd at all?

Line 106:
Nit,

logging.debug('num_outputs_on = %d', ...)

Line 112: # Set brightness to max.
shouldn't you set initial brightness to minimum

Line 117: time.sleep(adjustment_delay_s)
Perhaps dbus-monitor can be used inside self._*_brightness to determine arrival of the message.

Line 120: if num_steps_taken > max_num_steps:

Don't we expect exactly 16 brightness steps? If not then we must also want to check a min_num_steps as well?

Nit: instead of raising exception just log error so that test can continue w/ other subtests then raise final exception w/ summary of errors

Line 205: cmd = 'backlight-tool --get_max_brightness'

Agreed. Comparison at least out to be drawn to

preferred_max = int(utils.system_output("cat /sys/class/backlight/*/max_brightness | sort -nr | head -n 1").strip())

Line 207: if max_brightness < 4:
could 5 be min_num_steps? In any case lets make it a constant

if max_brightness < min_num_steps - 1:
...

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 94: if utils.system_output('status powerd').find('start/running') !=-1:

I'm pretty sure that powerd only updates its internal settings in response to changes to a few specific files, e.g. the lock-on-suspend pref that the user can change at chrome://settings.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (12 inline comments)

....................................................
File client/site_tests/power_BacklightControl/control
Line 15: - There are too many steps (> 16) between min and max brightness.

Done.

I think that should be done in power_Backlight.

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 79:

Done

Line 83: os.system('rm %s/*' % self._pref_path)

Done

Line 106:
Maybe these should be keyvals?

Line 112: # Set brightness to max.

I'm setting brightness from: (initial brightness) -> max -> min -> 0 -> max, that should cover the whole range.

Line 117: time.sleep(adjustment_delay_s)
The dbus message indicating changing brightness is sent out at the beginning, not at the end.

I we could just poll backlight-tool.

Line 140: # Turn off the screen by decreasing brightness one more time.

Done

Line 196: pass
Done

Line 201: pass
Done

Line 205: cmd = 'backlight-tool --get_max_brightness'

This is meant to test the backlight controller, not the backlight interface. Any failures in backlight-tool should be caught by power_Backlight.

Line 207: if max_brightness < 4:

Done

Line 220: XSET = 'LD_LIBRARY_PATH=/usr/local/lib xset'

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (2 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 106:
sgtm. Can we keyval num_outputs_on and the steps between max -> 0 & 0 -> max

Line 112: # Set brightness to max.

gotcha

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 4: (3 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 112: max_num_steps = self._max_num_steps
Nit, why do you re-assign these locally. Is it just length of var name?

Line 193: while True:
What if 'break' never reached. Perhaps condition could be

while num_steps_taken < self._max_num_steps

Line 212: while True:
same thought as above

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (4 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 106:
Done

Line 117: time.sleep(adjustment_delay_s)
Actually even that would be tricky since the backlight changes over several steps.

We could have powerd send out a dbus signal at the end of a brightness transition.

Line 120: if num_steps_taken > max_num_steps:

Done

Line 131: if num_steps_taken > max_num_steps:

Taken care of by changing it to "num_steps_taken != max_num_steps - 1"

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 106:

Let's get this in, and do it in a subsequent patch.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 4: (3 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 112: max_num_steps = self._max_num_steps
Earlier I was using all of them repeatedly so I didn't like having "self._" in front of all the references. Now that is not the case except for max_num_steps

Line 193: while True:
Done

Line 212: while True:
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (7 inline comments)

....................................................
File client/site_tests/power_BacklightControl/control
Line 5: AUTHOR = "Chrome OS Team"
see https://docs.google.com/a/google.com/document/d/11CNiKB10MRKGK6b8YWAVYpO9fTwNhAp-Yxve9l1q2kg/edit#heading=h.uwerexe4f1aj

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 88: os.system('mkdir %s/backup' % pref_path)
put this somewhere under /tmp so the rm command later doesn't produce an error

Line 197: os.system('rm -r %s/backup' % pref_path)
rmdir

Line 204: command = ('dbus-send --type=method_call --system ' + \
just to check, do these calls block? (they should)

regardless, i'm worried that this test will be flaky when used with an external backlight, since powerd sends an async request to powerm to change the level. note also that i'm planning to check in some changes soon that make us use a similar path for internal backlight changes.

Line 217: def _set_brightness_to_max(self):
add a comment documenting what this returns so people don't need to read the code to figure it out

Line 236: def _set_brightness_to_min(self):
ditto

Line 253: cmd = 'backlight-tool --get_max_brightness'
my concern here is that backlight-tool uses the same code to read the brightness that powerd uses to write it, so if that code breaks (e.g. it's changed to write to some random file), this test won't test it. there are already unit tests covering the behavior of that code and the BacklightController implementations, so it's not clear to me that this test covers much that isn't already covered. it's testing powerd's d-bus interface, at least.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py

Line 204: command = ('dbus-send --type=method_call --system ' + \

suggestion: i don't think that python ships with inotify support (there's a pyinotify module, but i don't know if we currently install it). if not, how about making _get_current_brightness() look something like the following:

def _get_current_brightness(expected_brightness, timeout_sec=10):
actual_brightness = -1
start_time_sec = time.time()
while true:
# read brightness from sysfs
if actual_brightness == expected_brightness or \
time.time() - start_time_sec > timeout_sec:
break
time.sleep(0.1)
return actual_brightness

?

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Todd Broch (Code Review)]

Todd Broch has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py

Line 149: time.sleep(self._adjustment_delay_s)
revisiting Dan's comment & Simon's response.

Dan: How do we get rid of sleep?

Simon: poll via backlight-tool.

Solution tempered by Dan's concern about using backlight-tool to get the brightness minimizing the test coverage below.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (7 inline comments)

....................................................
File client/site_tests/power_BacklightControl/control
Line 5: AUTHOR = "Chrome OS Team"

Done

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 88: os.system('mkdir %s/backup' % pref_path)

Done

Line 197: os.system('rm -r %s/backup' % pref_path)

Done

Line 204: command = ('dbus-send --type=method_call --system ' + \

How will we know |expected_brightness|, unless we use the formula used in backlight controller?

Line 217: def _set_brightness_to_max(self):
Done

Line 236: def _set_brightness_to_min(self):
Done

Line 253: cmd = 'backlight-tool --get_max_brightness'

I'll measure power, then.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py

Line 204: command = ('dbus-send --type=method_call --system ' + \

it's easy for the max case. for the min case, this test is already just checking that the brightness is less than max/2, so you could do something similar here:

def _get_current_brightness(min_threshold=-1,
max_threshold=-1,
timeout_sec=10):
"""Returns the current brightness once it goes below min_threshold
or above max_threshold. Either threshold can be set to a negative
value to disable it; if both are disabled, the current level will be
returned immediately. Waits at most timeout_sec seconds for the
brightness to reach an acceptable value."""

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 204: command = ('dbus-send --type=method_call --system ' + \

If I'm understanding you correctly, this is only useful for:
1. in the start -> max transition, the last step
2. in the max -> min transition, the later steps that go below max / 2.

Correct me if I'm wrong.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 204: command = ('dbus-send --type=method_call --system ' + \

yes, and

3. in the min->0 transition
4. in the 0->max transition

right?

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 204: command = ('dbus-send --type=method_call --system ' + \

What about all the intermediate steps? We won't know the exact step levels unless we use the formula that powerd uses.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 204: command = ('dbus-send --type=method_call --system ' + \

i don't understand -- are you saying that you want to check all of the intermediate steps? i think that that's outside the scope of what can be tested reliably.

it should be sufficient to test that we eventually reach 100% within X seconds when we're increasing to the max, or 0% when we're turning the display off, or some level below some intermediate percent when we're going to the minimum-visible (since you don't know what the internal 6.25% or whatever maps to).

having a method that loops quickly until the expected value is reached is good because it means that the test doesn't waste too much extra time when things are fast, but can still have a long time out in case load is randomly high and it takes a longer time for the condition to be met.

in the 100% case, max_threshold would be set to 100 (it should probably be named max_threshold_percent instead so the code that calls it doesn't need to pass in the actual level). for 0%, min_threshold_percent would be 0. for min-visible, you could set min_threshold_percent to 25. the function can wait up to timeout_sec for 100 times level divided by max_level to go >= max_threshold_percent or <= min_threshold_percent.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 8: (6 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 148: time.sleep(10)
need to update this

Line 173: time.sleep(self._screen_off_delay_s)
similarly, instead of sleeping here, you should make get_num_outputs_on() poll until an expected value is reached or a timeout is hit

Line 282: def _get_current_brightness(self, min_threshold=-1, max_threshold=-1,
suggestion that i should've mentioned earlier: None is probably a better choice than -1 for the defaults; then use 'is not None' in the tests later

also, one argument per line since they don't all fit on the first line

Line 286: |max_threshold| are set, returns immediately.
s/set/unset/ ?

Line 303: if (min_threshold != -1 and actual_brightness <= min_threshold) or \
you could combine these if statements:

if (min_threshold is None and max_threshold is None) or \
(min_threshold is not None and ...) or \
(max_threshold is not None and ...):
break

Line 308: if time.time() - start_time_sec >= timeout_sec:
test and break before sleeping? no need to sleep if we're going to break immediately afterwards

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 9: (9 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 44: if line[0:5] == 'Screen':
line.startswith('Screen')

Line 149: time.sleep(self._stabilizing_delay_s)
why do you need this? shouldn't you be using _wait_for_value() here is well?

Line 218: keyvals['final_power_w'] <= keyvals['screen_off_power_w']:
this seems like it has potential to be flaky

Line 263: time.sleep(self._adjustment_delay_s)
why do you need to sleep here?

Line 265: return num_steps_taken
you can get rid of the return value; it's not being used anymore

Line 278: time.sleep(self._adjustment_delay_s)
don't need to sleep here either, right?

Line 280: return num_steps_taken
get rid of this too

Line 285: if max_brightness <= self._min_num_steps:
nit: move this check into run_once(), after the only place where this method is called

Line 294: def _wait_for_value(self,
nit: make this a function at the top of the file instead of a member of the class

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 9

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 9: (8 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 44: if line[0:5] == 'Screen':

Done

Line 149: time.sleep(self._stabilizing_delay_s)
But I don't know the value. I suppose I could just use min=0.

Line 218: keyvals['final_power_w'] <= keyvals['screen_off_power_w']:

Good point... there shouldn't be anything else running sporadically.

Maybe restart ui?

Line 263: time.sleep(self._adjustment_delay_s)
To avoid overspamming dbus... is it not a concern?

Line 265: return num_steps_taken
Done

Line 280: return num_steps_taken
Done

Line 285: if max_brightness <= self._min_num_steps:

Done

Line 294: def _wait_for_value(self,
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 9

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 9: (2 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 263: time.sleep(self._adjustment_delay_s)
Done

Line 278: time.sleep(self._adjustment_delay_s)
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 9

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 9: (2 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py

Line 149: time.sleep(self._stabilizing_delay_s)
why can't you use min_threshold=(self._max_brightness / 2 - 1)? that's what you're asserting later, isn't it?

Line 218: keyvals['final_power_w'] <= keyvals['screen_off_power_w']:

my main concern was that the power rate reading might not have increased by the time that this gets called. any reason why you can't use _wait_for_value() here as well?

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 9

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 12: (4 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 178: logging.error('Power did not increase when brightness was ' + \
s/Power/Power draw/

Line 188: min_threshold=0)
i don't understand why you're using min_threshold=0 here. doesn't that mean that we'll always hit the 10-second timeout? i suggested (self._max_brightness / 2 - 1) since that's what you're checking for in the next assertion. is there a reason why this won't work?

Line 201: logging.error('Power did not decrease when brightness was ' + \
s/Power/Power draw/

Line 229: logging.error('Power did not decrease when screen was ' + \
s/Power/Power draw/

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 12: (4 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 178: logging.error('Power did not increase when brightness was ' + \

Done

Line 188: min_threshold=0)
Done

Line 201: logging.error('Power did not decrease when brightness was ' + \

Done

Line 229: logging.error('Power did not decrease when screen was ' + \

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16
Gerrit-PatchSet: 12

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Simon Que <sq...@chromium.org>

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 13: Fails

The problem I'm still running into is distiguishing between "less/greater than" vs "less/greater than or equal to" thresholds in wait_for_value(). Currently the thresholds are used with ">=" or "<=" comparators. But in some of these cases they should really be ">" or "<"... I'm thinking of introducing a flag called "must_exceed_threshold" or something.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16

Gerrit-PatchSet: 13

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 13: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 188: min_threshold=(self._max_brightness / 2))
i think you need (self._max_brightness / 2 - 1), as i suggested in the last comment: you are failing later if >= self._max_brightness / 2. you don't want to fail if the wait_for_value() call happens to see the boundary value mid-transition just before it returns.

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................


Patch Set 13: (1 inline comment)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 188: min_threshold=(self._max_brightness / 2))

I added a flag to use "greater/less than" vs "greater/less than or equal to" to take care of this distinction.

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 14:

Power draw rates in watts for various devices, off / max / min

Daisy: 3.9 / 4.5 / 6.3
Parrot: 7.5 / 8.6 / 10.3
Link: 8.7 / 12.1 / 17.2
Lumpy: 5.6 / 7.2 / 9.9

Based on these values, I'd say a 10% threshold is a safe bet. However, we should wait between adjustments for power to stabilize before moving onto the next brightness level.

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16

Gerrit-PatchSet: 14

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 15: (9 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 112: _min_num_steps = 4
add comments describing these, e.g.

# Minimum and maximum number of brightness steps expected
# between the minimum and maximum brightness levels.

Line 114: _energy_rate_change_threshold_percent = 5.0
add a comment describing what this is

Line 117: # Require that this ben run on battery so we can measure power draw.
s/ben/is/ ?

Line 146: max_to_min_factor = \
add comments describing what these represent

Line 147: 1.0 - self._energy_rate_change_threshold_percent / 100
nit: 100.0 to make sure you don't use integer division if someone changes the threshold; ditto in next sattement

Line 245: keyvals['initial_brightness'] < self._max_brightness * 0.75 and \
you shouldn't be using the initial brightness that was set beforehand -- this makes the test non-deterministic. i'd delete this. what bugs can it catch that wouldn't already be caught by comparing max to min to off?

Line 351: window_size=10,
does it really take 10 seconds for the reported values to stabilize?

Line 355: The the last |window_size| samples of energy rate do not deviate
s/The the/The/

Line 376: max_variation_factor = float(max_variation_percent) / 100
can just divide by 100.0 instead of casting to float, right?

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16

Gerrit-PatchSet: 15

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................


Patch Set 15: (9 inline comments)

....................................................
File client/site_tests/power_BacklightControl/power_BacklightControl.py
Line 112: _min_num_steps = 4

Done

Line 114: _energy_rate_change_threshold_percent = 5.0

Done

Line 117: # Require that this ben run on battery so we can measure power draw.

Done

Line 146: max_to_min_factor = \
Done

Line 147: 1.0 - self._energy_rate_change_threshold_percent / 100

Done

Line 245: keyvals['initial_brightness'] < self._max_brightness * 0.75 and \

Done

Line 351: window_size=10,
On some of the systems I tested, it seems to be the case.

Also, I'm wary of sampling too quickly and increasing the pwoer load. I'm also wary of taking too few samples. Hence, 10 seconds is a safe bet.

Line 355: The the last |window_size| samples of energy rate do not deviate

Done

Line 376: max_variation_factor = float(max_variation_percent) / 100

Done

[Daniel Erat (Code Review)]

Daniel Erat has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 16: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/37388
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id399956ee55443d711429c191839bb227885da16

Gerrit-PatchSet: 16

[Simon Que (Code Review)]

Simon Que has posted comments on this change.

Change subject: client/site_tests: add power_BacklightControl test
......................................................................

Patch Set 16: Verified; Ready