power_Draw/Idle/LoadTest: Support temperature sensing for x86

power_Draw/Idle/LoadTest: Support temperature sensing for x86

Added power_status class to read thermal data (temperature and fans).
Currently only supported for x86.

Tests updated to use it:
- power_Draw
- power_Idle
- power_LoadTest

BUG=chromium-os:13866
TEST=Run these three tests on both x86 and ARM, with reduced time
settings if necessary.  They should pass on both.  Check max_temp and
min_temp in the test results.  They should be properly defined on x86
(something realistic) and undefined on ARM (two absurdly large-magnitude
numbers).

Signed-off-by: Simon Que <sque@chromium.org>;
Change-Id: Ib098ead4d6de2f87e1a97a646a64a86ed0e11bf8

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=d8e41b7

[Duncan Laurie, 2011-04-06 16:57:34]

LGTM

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:48: Fields:
I think the list in the comments was copy+paste from below but should probably
be covering the thermal status fields instead of the battery status fields.

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:77: for i in range(10):
10 is probably safe but it may be useful to make this a constant somewhere in
case we run into systems with more trip points.  (I think there may end up being
1 more with ZGB)

[Simon Que, 2011-04-06 17:48:10]

PTAL

[Sameer Nanda, 2011-04-06 17:53:53]

I am still going through the code but here're my initial comments.

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:60: float voltage_now:        Voltage now [V]
comments need to fixed up to match thermal fields.

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:77: for i in range(10):
lets use python glob to find all matching names instead of restricting us to 10
(look at the SysStat class below for example code for glob).

[Sameer Nanda, 2011-04-06 18:13:40]

Looking good in general.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:97: for i in range(self.num_trip_points):
probably better to just save the names of all trip point temp files in the loop
in __init__ and just loop through them.  That way there is no requirement for
temp files names to be numerically contiguous.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:207: thermal_path =
'/sys/class/thermal/thermal_zone0'
do you want to use thermal_zone* here instead?  I think if you switch to glob,
this may just work naturally.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:238: try:
lets add a logging.info for the temperature that was just read.  This way we
have the data to plot out a temperature curve over the duration of a test.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
File client/site_tests/power_Draw/power_Draw.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
client/site_tests/power_Draw/power_Draw.py:25: time.sleep(1)
this might be too aggressive since the BIOS currently only polls every 10
seconds and might have slight impact on power draw as well.  I would say sleep
for > 10 seconds here.  Maybe pass the amount of time to sleep as a parameter
run_once with a default setting of 10 seconds.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
client/site_tests/power_Draw/power_Draw.py:41: keyvals['w_min_temp'] =
status.min_temp
use c instead of w.  The first letter indicates the unit, deg centigrades
instead of watts.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
File client/site_tests/power_Idle/power_Idle.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
client/site_tests/power_Idle/power_Idle.py:28: time.sleep(1)
same comment as power_Draw regarding adding increasing sleep time to 10 sec.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
client/site_tests/power_Idle/power_Idle.py:78: keyvals['w_max_temp'] =
self.status.max_temp
same comment as power_Draw to change name to c_ instead of w_

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Load...
File client/site_tests/power_LoadTest/power_LoadTest.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Load...
client/site_tests/power_LoadTest/power_LoadTest.py:238: keyvals['w_max_temp'] =
self._power_status.max_temp
name change to c_ from w_

[Simon Que, 2011-04-07 20:59:46]

PTAL

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:48: Fields:
On 2011/04/06 16:57:34, Duncan Laurie wrote:
> I think the list in the comments was copy+paste from below but should probably
> be covering the thermal status fields instead of the battery status fields.

Done.

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:60: float voltage_now:        Voltage now [V]
On 2011/04/06 17:53:54, Sameer Nanda wrote:
> comments need to fixed up to match thermal fields.

Done.

http://codereview.chromium.org/6800002/diff/1/client/cros/power_status.py#new...
client/cros/power_status.py:77: for i in range(10):
On 2011/04/06 17:53:54, Sameer Nanda wrote:
> lets use python glob to find all matching names instead of restricting us to
10
> (look at the SysStat class below for example code for glob).

Done.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:97: if self.temp >
self.read_val('trip_point_%d_temp' % i, int):
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> probably better to just save the names of all trip point temp files in the
loop
> in __init__ and just loop through them.  That way there is no requirement for
> temp files names to be numerically contiguous.

Done.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:207: 
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> do you want to use thermal_zone* here instead?  I think if you switch to glob,
> this may just work naturally.

Done.

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:238: if self.thermal[0].temp < self.min_temp:
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> lets add a logging.info for the temperature that was just read.  This way we
> have the data to plot out a temperature curve over the duration of a test.

This isn't trivial, as there may be other log messages between the refresh()
calls.  Also, the SysStat object does not know the current time, so it can only
record a sequence of temp values.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
File client/site_tests/power_Draw/power_Draw.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
client/site_tests/power_Draw/power_Draw.py:25: time.sleep(1)
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> this might be too aggressive since the BIOS currently only polls every 10
> seconds and might have slight impact on power draw as well.  I would say sleep
> for > 10 seconds here.  Maybe pass the amount of time to sleep as a parameter
> run_once with a default setting of 10 seconds.

Done.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Draw...
client/site_tests/power_Draw/power_Draw.py:41: keyvals['w_min_temp'] =
status.min_temp
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> use c instead of w.  The first letter indicates the unit, deg centigrades
> instead of watts.

Done.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
File client/site_tests/power_Idle/power_Idle.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
client/site_tests/power_Idle/power_Idle.py:28: time.sleep(1)
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> same comment as power_Draw regarding adding increasing sleep time to 10 sec.

Done.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Idle...
client/site_tests/power_Idle/power_Idle.py:78: keyvals['w_max_temp'] =
self.status.max_temp
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> same comment as power_Draw to change name to c_ instead of w_

Done.

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Load...
File client/site_tests/power_LoadTest/power_LoadTest.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/site_tests/power_Load...
client/site_tests/power_LoadTest/power_LoadTest.py:238: keyvals['w_max_temp'] =
self._power_status.max_temp
On 2011/04/06 18:13:40, Sameer Nanda wrote:
> name change to c_ from w_

Done.

[Sameer Nanda, 2011-04-07 21:27:39]

Lets chat about your temperature logging comment.

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:49: (All temperatures are in degrees Celsius times
1000.)
may want to call it millidegrees Celsius instead.

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:76: field = file[len(path + '/'):]
this breaks (in the unlikely) case of zone# > 9, I think. I think you want to
use os.path.basename here?

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:101: self.num_points_tripped += 1
can you add a logging.info message here printing out which one tripped?

http://codereview.chromium.org/6800002/diff/10001/client/site_tests/power_Dra...
File client/site_tests/power_Draw/power_Draw.py (right):

http://codereview.chromium.org/6800002/diff/10001/client/site_tests/power_Dra...
client/site_tests/power_Draw/power_Draw.py:42: keyvals['c_max_temp'] =
status.max_temp
my bad. I should've suggested the units to be mc instead of c since the
temperature is in millidegrees celsius.  can you please fix this and other tests
as well?

[Simon Que, 2011-04-07 22:50:03]

PTAL

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:49: (All temperatures are in degrees Celsius times
1000.)
On 2011/04/07 21:27:39, Sameer Nanda wrote:
> may want to call it millidegrees Celsius instead.

Done.

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:76: field = file[len(path + '/'):]
On 2011/04/07 21:27:39, Sameer Nanda wrote:
> this breaks (in the unlikely) case of zone# > 9, I think. I think you want to
> use os.path.basename here?

Are you sure?  the zone # is passed in as part of path.

http://codereview.chromium.org/6800002/diff/10001/client/cros/power_status.py...
client/cros/power_status.py:101: self.num_points_tripped += 1
On 2011/04/07 21:27:39, Sameer Nanda wrote:
> can you add a logging.info message here printing out which one tripped?

Done.

http://codereview.chromium.org/6800002/diff/10001/client/site_tests/power_Dra...
File client/site_tests/power_Draw/power_Draw.py (right):

http://codereview.chromium.org/6800002/diff/10001/client/site_tests/power_Dra...
client/site_tests/power_Draw/power_Draw.py:42: keyvals['c_max_temp'] =
status.max_temp
On 2011/04/07 21:27:39, Sameer Nanda wrote:
> my bad. I should've suggested the units to be mc instead of c since the
> temperature is in millidegrees celsius.  can you please fix this and other
tests
> as well?

Done.

[Simon Que, 2011-04-07 23:30:51]

PTAL

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py
File client/cros/power_status.py (right):

http://codereview.chromium.org/6800002/diff/1005/client/cros/power_status.py#...
client/cros/power_status.py:238: try:
On 2011/04/07 20:59:47, Simon Que wrote:
> On 2011/04/06 18:13:40, Sameer Nanda wrote:
> > lets add a logging.info for the temperature that was just read.  This way we
> > have the data to plot out a temperature curve over the duration of a test.
> 
> This isn't trivial, as there may be other log messages between the refresh()
> calls.  Also, the SysStat object does not know the current time, so it can
only
> record a sequence of temp values.

Done.

[Sameer Nanda, 2011-04-07 23:33:13]

LGTM. Thanks!