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

Issue 806146 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature


Sign in to add a comment

[servo power] create measure_power.py helper module and script

Project Member Reported by coconutruben@chromium.org, Jan 26 2018

Issue description

This bug is to track the work done to create a module and accompanying simple script that leverages recent introductions to servo to measure power.
 
Blockedon: 867381
Blockedon: 867383
Blockedon: 867384
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/89baff4808b9d8b1eab04d1a56ad916836fee3d0

commit 89baff4808b9d8b1eab04d1a56ad916836fee3d0
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Aug 01 07:05:02 2018

servod: introduce dut-power command

dut-power and measure_power are a cmdline tool & module to collect
power numbers using servod. Once servod is running, use like:

dut-power -p $SERVOD_PORT -t 10

for a 10s power measurement. This will use any onboard INAs that it
can find, and the ec vbat command, before printing familiar output.

CQ-DEPEND=CL:1140032

BUG=chromium:806146
TEST=manual testing
sudo servod -b soraka -c poppy_r3.xml &
dut-power -t 10
> get ec numbers and ina numbers

sudo servod -b soraka
dut-power -t 10
> get ec numbers only

Change-Id: I701a1eec50998126a9b7c8198e6db0303c4e5def
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1131034
Reviewed-by: Todd Broch <tbroch@chromium.org>

[add] https://crrev.com/89baff4808b9d8b1eab04d1a56ad916836fee3d0/servo/dut_power.py
[add] https://crrev.com/89baff4808b9d8b1eab04d1a56ad916836fee3d0/servo/timelined_stats_manager_unittest.py
[modify] https://crrev.com/89baff4808b9d8b1eab04d1a56ad916836fee3d0/setup.py
[add] https://crrev.com/89baff4808b9d8b1eab04d1a56ad916836fee3d0/servo/timelined_stats_manager.py
[add] https://crrev.com/89baff4808b9d8b1eab04d1a56ad916836fee3d0/servo/measure_power.py

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/61d6a466cf73302b5eea83b56d625e6247af409c

commit 61d6a466cf73302b5eea83b56d625e6247af409c
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Aug 01 07:05:02 2018

servod: measure_power: add more data retrieval mechanisms

This CL adds the ability to save:
- save the raw data collected
- summary
- the logs
from the measure_power.py

BUG=chromium:806146
TEST=tested out savings and they work as expected

Change-Id: Ief8ce53ad0419090ae8ac1bf3027ed24ec98d50b
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1131035
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/61d6a466cf73302b5eea83b56d625e6247af409c/servo/dut_power.py
[modify] https://crrev.com/61d6a466cf73302b5eea83b56d625e6247af409c/servo/measure_power.py

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 1

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

commit cde46f61e06817f90c78de6d3900071c6dd2db50
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Aug 01 07:05:06 2018

dut_power: preemptible cmdline tool

This CL allows Ctrl+C (or SIGTERM) signals to dut-power to be handled
gracefully by setting the measurement to conclude prematurely, but still
retrieve all the data collected until that point.

BUG=chromium:806146
TEST=manual testing
$ dut-power -t 15
(wait 4-5s)
Ctrl+C
> [regular output]

Change-Id: Ia6fdbfef3b96e3c4395d833d80088b36dff8bdcd
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1149428
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/cde46f61e06817f90c78de6d3900071c6dd2db50/servo/dut_power.py

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 1

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

commit 5a060f1a56f915dca41f4ee77673f66a89a5d0ea
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Aug 01 07:05:05 2018

stats_manager: accept_nan support

This CL introduces a flag to StatsManager that allows for 'NaN' values
to be recorded inside StatsManager. The motivation here is that if a
sample fails to record it might be more desirable to record a 'NaN'
than to just skip the sample, to keep timelines correct, and to not hide
errors in the test-run.
Also adds necessary tests for that behavior.

BRANCH=None
BUG=chromium:806146, chromium:760267
TEST=unit tests still pass

Change-Id: If17b7f52ba4a05e9e007c73bfa5d667fe36b74b3
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1140031
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/5a060f1a56f915dca41f4ee77673f66a89a5d0ea/extra/usb_power/stats_manager.py
[modify] https://crrev.com/5a060f1a56f915dca41f4ee77673f66a89a5d0ea/extra/usb_power/stats_manager_unittest.py

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 1

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

commit 60881d6546a1b8833ff7e5e675bb901112d89c42
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Aug 01 07:05:05 2018

stats_manager: more informative nan summary output

Now for the formatted output string, if any value a domain is NaN,
the domains gets tagged with a * and a help text gets added at the end
of the summary to highlight this issue.

CQ-DEPEND=CL:1140025

BRANCH=None
BUG=chromium:806146, chromium:760267
TEST=unit tests added & pass

Change-Id: I30791053bb1645065fa2bfd8305cc840a4a88031
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1140032
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/60881d6546a1b8833ff7e5e675bb901112d89c42/extra/usb_power/stats_manager.py
[modify] https://crrev.com/60881d6546a1b8833ff7e5e675bb901112d89c42/extra/usb_power/stats_manager_unittest.py

Blockedon: 870956
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/8bf56aef9dc23196d8c01a53a5e2445b6bdb07f3

commit 8bf56aef9dc23196d8c01a53a5e2445b6bdb07f3
Author: Mengqi Guo <mqg@chromium.org>
Date: Sat Aug 11 05:43:54 2018

servod: add default to dut-power args

Add default to dut-power -h messages for better UX. Fix default
for mutually exclusive pairs.

BUG=chromium:806146
TEST=manual testing
dut-power -h
usage: dut-power [-h] [--host HOST] [-p PORT] [-f] [-w WAIT] [-t TIME]
                 [--ina-rate INA_RATE] [--vbat-rate VBAT_RATE] [-v]
                 [--no-output] [-o OUTDIR] [-m MESSAGE]
                 [--save-raw-data | --no-save-raw-data]
                 [--save-summary | --no-save-summary]
                 [--save-logs | --no-save-logs] [--save-all SAVE_ALL]

Measure power using servod.

optional arguments:
  -h, --help            show this help message and exit
  --host HOST           servod host (default: localhost)
  -p PORT, --port PORT  servod port (default: 9999)
  -f, --fast            if fast no verification cmds are done (default: False)
  -w WAIT, --wait WAIT  time (sec) to wait before measuring power (default: 0)
  -t TIME, --time TIME  time (sec) to measure power for (default: 60)
  --ina-rate INA_RATE   rate (sec) to query the INAs (default: 1)
  --vbat-rate VBAT_RATE
                        rate (sec) to query the ec vbat command (default: 60)
  -v, --verbose         print debug log (default: False)
  --no-output           do not output anything into stdout (default: False)
  -o OUTDIR, --outdir OUTDIR
                        directory to save data into (default: None)
  -m MESSAGE, --message MESSAGE
                        message to append to each summary file stored
                        (default: None)
  --save-raw-data       save raw-data (default: False)
  --no-save-raw-data    don't save raw-data
  --save-summary        save summary (default: True)
  --no-save-summary     don't save summary
  --save-logs           save logs (default: True)
  --no-save-logs        don't save logs
  --save-all SAVE_ALL   Equivalent to --save-summary --save-logs --save-raw-
                        data. Overwrites any of those if specified. (default:
                        False)

Change-Id: I3afdd125c68a99ffb04a1d9f6c28b52c70da23ab
Signed-off-by: Mengqi Guo <mqg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1170319
Reviewed-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>

[modify] https://crrev.com/8bf56aef9dc23196d8c01a53a5e2445b6bdb07f3/servo/dut_power.py

Blockedon: 873802
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 17

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

commit c4c972972065b1aa16da6c8316cf2303758dc8c9
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Fri Aug 17 18:17:50 2018

servod: add data trimming to TimelinedStatsManager

This CL introduces the ability to trim to data inside a
TimelinedStatsManager to only the samples where tstart <= t <= tend for
the timeline. This trimming process is in place and operates on the raw
data of the TimelinedStatsmanager.
The logic for this is from: go/power-status-numpy-magic

BUG=chromium:806146
TEST=added unit tests and ran them, seems to work fine.

Change-Id: I372df1b04163f6704ffdc27b363dcabc7c78fb2a
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1163595
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/c4c972972065b1aa16da6c8316cf2303758dc8c9/servo/timelined_stats_manager_unittest.py
[modify] https://crrev.com/c4c972972065b1aa16da6c8316cf2303758dc8c9/servo/timelined_stats_manager.py

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 23

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

commit d767aa397d3a6b5b5fbbce2b124f08bba67193f2
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Thu Aug 23 14:37:02 2018

servod: pull stats calculation into main thread for measure_power.

This CL pull the stats calculation into the main thread for measure
power. This has a couple of benefits:
- We can trim the data without having to do double calculations
- We can get rid of processing_done signal
- We can hide stop_signal internally behind FinishMeasurement

BUG=chromium:806146
TEST=Tested by mqg@ on functional servo/DUT & by me on a naked servo
Both report no errors in the flow.

Change-Id: I299e204d634f10abc775f42549b1825de8f16531
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1179101
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/d767aa397d3a6b5b5fbbce2b124f08bba67193f2/servo/dut_power.py
[modify] https://crrev.com/d767aa397d3a6b5b5fbbce2b124f08bba67193f2/servo/measure_power.py

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/6c57c6bb10662bb44613e9c266bc527d1870d6b1

commit 6c57c6bb10662bb44613e9c266bc527d1870d6b1
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Fri Aug 31 12:23:32 2018

servod: add power_tools.xml by default

Right now measure_power breaks if no power map (i.e. poppy_r3.xml) is
not added to servod. It breaks because the introspection controls are
not available. This adds the power introspection to all servod
invocations.

BUG=chromium:806146
TEST=manual testing
sudo servod -b soraka
dut-control raw_calib_rails
> raw_calib_rails:

Change-Id: If33dac1c8c970bde0b94beea6fd8992bc366910e
Reviewed-on: https://chromium-review.googlesource.com/1195271
Commit-Ready: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Tested-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/6c57c6bb10662bb44613e9c266bc527d1870d6b1/servo/data/common.xml

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 9

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

commit daec74156f4d322be25e65b2adade2d2da0e5af1
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Sun Sep 09 17:33:51 2018

servod, measure_power: provide better logging on tracker error

When a PowerTracker verification failed, PowerMeasurement clobbers the
resulting error message. This makes sure that the error message gets
logged before raising an error.

BUG=chromium:806146
TEST=manual testing w/ EC off
Failed to get ec_board, setting to unknown.
Failed to test servod commands. Tested: ['ppvar_vbat_mw']
Tracker EC failed verification. Not using it.

Change-Id: I9a7cd4f969edd6b9fb9b73b283d7ae42fbf7e3e2
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1198810
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/daec74156f4d322be25e65b2adade2d2da0e5af1/servo/measure_power.py

Blockedon: 884924
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/7871bbe98020996e86800a6c11c02a3c53ef699e

commit 7871bbe98020996e86800a6c11c02a3c53ef699e
Author: Mengqi Guo <mqg@chromium.org>
Date: Fri Sep 21 20:34:13 2018

dut_power: add option to not query EC vbat

Add an option to dut-power tool to not query EC vbat. This is
helpful when measuring standby power where querying EC vbat
constantly increases total power.

BUG=chromium:806146, b:111616837
TEST=manual testing

sudo servod -b nami -c nami_rev1_inas.xml

dut-power -h
shows the help message

dut-power
works, provide both INA and EC data

dut-power --ina-rate=-1 --vbat-rate=-1
works, throws exception as expected

dut-power --ina-rate=1 --vbat-rate=-1
works, only query ina

dut-power --ina-rate=-1 --vbat-rate=1
works, only query EC

Change-Id: I29c5ec3d7bcd578aea0c7463b1b646cd733b48cd
Signed-off-by: Mengqi Guo <mqg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1235137
Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/7871bbe98020996e86800a6c11c02a3c53ef699e/servo/dut_power.py
[modify] https://crrev.com/7871bbe98020996e86800a6c11c02a3c53ef699e/servo/measure_power.py

Project Member

Comment 18 by bugdroid1@chromium.org, Sep 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/165ff6d6abd6344a9a02687b5cf62c386552ee76

commit 165ff6d6abd6344a9a02687b5cf62c386552ee76
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Sep 26 17:33:01 2018

servod: remove unnecessary Cleanup from measure_power

In a previous version of measure_power, the measurement would create a
default directory in anticipation of needing to store results.
This has been replaced with a lazy creation, only generating the
dirname, but creating the directory only when there will be files output
into it.
This makes the Cleanup logic obsolete, as no directory should now come
without contents.

BUG=chromium:806146
TEST=manual testing, dut-power still runs fine

Change-Id: I33f4ae9385c08369b4aeb04c3cd7a27fae2a1525
Reviewed-on: https://chromium-review.googlesource.com/1237633
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/165ff6d6abd6344a9a02687b5cf62c386552ee76/servo/measure_power.py

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/0c84f5b95e21abd3513a9e66ccb1a38d4f8d7117

commit 0c84f5b95e21abd3513a9e66ccb1a38d4f8d7117
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Mon Oct 29 18:21:37 2018

servod: small measure_power refactor

This refactor gets rid of the split between PowerTracker and
ServodPowerTracker.
Originally the motivation here had been to allow for flexibility in also
having a powerlog power-tracker. But since our current efforts are on
unifying behind the servod interface, this is an unnecessary
modularization.
Additionally this CL improves some documentation that was lacking.

BUG=chromium:806146
TEST=manual testing
dut-power -t 120

Change-Id: I8f44aa544ac030de890658120f729f705269bfc5
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1229534
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/0c84f5b95e21abd3513a9e66ccb1a38d4f8d7117/servo/measure_power.py

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 29

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

commit db4e48142be7498a812a8ffc23654ff1566d5fb1
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Mon Oct 29 18:21:37 2018

servod: use avg_ppvar in measure_power.py

This change leverages avg_ppvar in the measure_power script.
It does so by ensuring that the ECPowerTracker always runs the first
sample with normal ppvar_vbat_mw (to not get the baggage of the previous
minute) and then switches to avg_ppvar_vbat_mw if that is available on
the DUT's EC image.

BUG=chromium:806146,  chromium:867383 
TEST=manual testing
dut-control -t 130 on a dut with avg_ppvar enabled.

Change-Id: I6abf4aebd0eea236fc42878c89c852990e939281
Signed-off-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1229535
Reviewed-by: Mengqi Guo <mqg@chromium.org>

[modify] https://crrev.com/db4e48142be7498a812a8ffc23654ff1566d5fb1/servo/measure_power.py

Project Member

Comment 21 by bugdroid1@chromium.org, Oct 31

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

commit dc23da07b9ee0240b4190eae074c3324c208a210
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Oct 31 19:42:35 2018

servod: use dsleep in measure_power to increase ec vbat accuracy

This adds a prepare stage to the ECPowerTracker to issue a dsleep
control to the EC console. This allows the EC (if dsleep is supported)
to enter dee-sleep after 2s and not the standard 15. This greatly
improves accuracy, especially in suspend/power-off measurement
scenarios.

BUG=chromium:806146
TEST=manual testing
control gets sent to the EC successfully

Change-Id: Iaffa88e17acfae70d7e826bc7ac98dea6aeb47b2
Reviewed-on: https://chromium-review.googlesource.com/1229536
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org>

[modify] https://crrev.com/dc23da07b9ee0240b4190eae074c3324c208a210/servo/measure_power.py

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 31

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

commit b19a0035716f45f8639ff7ddadfef4877c314d4f
Author: Mengqi Guo <mqg@chromium.org>
Date: Wed Oct 31 19:42:40 2018

servod: add padding when trimming data

While trimming extra data outside of test time, for data points
that average over their corresponding sample intervals, a padding
is added to tstart and tend to ensure that only data points with
over half samples between tstart and tend are kept. No padding
is needed for data points that are instant data.

BUG=chromium:806146, b:111616837
TEST=python -m unittest -v timelined_stats_manager_unittest
test_that <dut ip> power_ServodWrapper \
--autotest_dir ~/trunk/src/third_party/autotest/files/ \
--args 'test=power_Dummy servo_host=localhost servo_port=9999 ina_rate=1 vbat_rate=2'

Change-Id: Ie1b895873bbbc8d5fe5b79416cb19d7c9767e11a
Signed-off-by: Mengqi Guo <mqg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1298536
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org>

[modify] https://crrev.com/b19a0035716f45f8639ff7ddadfef4877c314d4f/servo/timelined_stats_manager_unittest.py
[modify] https://crrev.com/b19a0035716f45f8639ff7ddadfef4877c314d4f/servo/timelined_stats_manager.py
[modify] https://crrev.com/b19a0035716f45f8639ff7ddadfef4877c314d4f/servo/measure_power.py

Components: Tools>ChromeOSDebugBoards
Blockedon: 917763

Sign in to add a comment