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

Issue 783845 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ectools temps should include units

Project Member Reported by drinkcat@chromium.org, Nov 10 2017

Issue description

Currently, 'ectool temps all' just prints "300". This may easily be mistakenly read as tenth of degree C (30.0 C), as the value appears to make sense (close to room temperature). However, the value is actually 300 K (27 C).

Let's add the unit (K) in ectool output, and fix autotests that rely on the exact answer format.
 
Cc: ayatane@chromium.org sha...@chromium.org
EC fix, that changes format from:
0: 300
to
0: 300 K
https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/763996

Autotest fix:
https://chromium-review.googlesource.com/#/c/chromiumos/third_party/autotest/+/763578

I also looked at:
 - client/cros/ec.py: That uses regex that will work with units as well: '''TEMP_SENSOR_RE = "Reading temperature...([0-9]*)"'''
 - server/site_tests/firmware_ECAdc/firmware_ECAdc.py: This test seems broken anyway (tries to run ectool temps that would not work?)
 - server/site_tests/firmware_ECThermal/firmware_ECThermal.py: Uses a regex that'll work too: '''pattern = re.compile('Reading temperature...(\d*)')'''
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/bce7b5b4b69f5d397f6ee17b0400a31a4d385317

commit bce7b5b4b69f5d397f6ee17b0400a31a4d385317
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Sat Nov 11 20:13:42 2017

ectool: Print temperature unit in ectool temps output

Currently, 'ectool temps all|<n>' just prints "300". This may easily
be mistakenly read as tenth of degree C (30.0 C), as the value
appears to make sense (close to room temperature). However, the value
is actually 300 K (27 C).

CQ-DEPEND=CL:763578
BRANCH=none
BUG= chromium:783845 
TEST=ectool temps all shows temperature unit (K)

Change-Id: I70f7f04d061cb1d4f741d59f8b48c7963dd8280f
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/763996
Reviewed-by: Shawn N <shawnn@chromium.org>

[modify] https://crrev.com/bce7b5b4b69f5d397f6ee17b0400a31a4d385317/util/ectool.c

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 11 2017

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

commit eebd1225a4b967e98ca417eb4e373ece67670f49
Author: Nicolas Boichat <drinkcat@google.com>
Date: Sat Nov 11 20:13:42 2017

utils: Allow ectool temps all to show units

To reduce potential confusion, ectool temps all now includes
temperature unit. Adjust get_ec_temperatures to handle that.

BUG= chromium:783845 
TEST=python
     >>> pattern = re.compile('.*: (\d+)')
     >>> line='0: 300'
     >>> matched = pattern.match(line)
     >>> int(matched.group(1)) - 273
     27
     >>> line='0: 300 K'
     >>> matched = pattern.match(line)
     >>> int(matched.group(1)) - 273
     27

Change-Id: Ia8bd34dbef18ccdc23d58729eb45b0ab4edd002b
Reviewed-on: https://chromium-review.googlesource.com/763578
Commit-Ready: Nicolas Boichat <drinkcat@chromium.org>
Tested-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-by: Wai-Hong Tam <waihong@google.com>

[modify] https://crrev.com/eebd1225a4b967e98ca417eb4e373ece67670f49/client/bin/utils.py

Status: Fixed (was: Assigned)

Comment 5 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 6 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment