New issue
Advanced search Search tips

Issue 683303 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Polish powerd_suspend shell script

Project Member Reported by benchan@chromium.org, Jan 20 2017

Issue description

Noticed a few areas of improvement when modifying powerd_suspend:
- Make variable and literal quoting more robust and conform with shell style guide (https://google.github.io/styleguide/shell.xml)
- Avoid altering current directory when toggle gpio via sysfs.
- Use /run instead of /var/run
- Use more robust regex for checking HWID
- Other minor issue with naming and comment style/consistency
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 21 2017

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

commit 0bd1127b956c7b6781778e09c0340bee564f4016
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 08:15:32 2017

power: powerd_suspend: use `printf` instead of `echo -n`

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I086cb9de2c078921884b5a8c23c6a598ca713c84
Reviewed-on: https://chromium-review.googlesource.com/430954
Commit-Ready: Dan Shi <dshi@google.com>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/0bd1127b956c7b6781778e09c0340bee564f4016/power_manager/powerd/powerd_suspend

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 21 2017

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

commit 5bcecc2309acd4fe7cf22482c05312201f04b732
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 08:19:50 2017

power: powerd_suspend: use -eq instead of = for integer comparison

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I6931b8613a07e9160879d20cc27b10f30ef4fa9d
Reviewed-on: https://chromium-review.googlesource.com/430955
Commit-Ready: Dan Shi <dshi@google.com>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/5bcecc2309acd4fe7cf22482c05312201f04b732/power_manager/powerd/powerd_suspend

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 21 2017

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

commit d81ee5383166b01a62d951ba3461d24738caa844
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 08:23:03 2017

power: powerd_suspend: declare function-scope variable 'local'

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I501911c007a68e9251c74a278a5f6e3b25e3efbc
Reviewed-on: https://chromium-review.googlesource.com/430956
Commit-Ready: Dan Shi <dshi@google.com>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/d81ee5383166b01a62d951ba3461d24738caa844/power_manager/powerd/powerd_suspend

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 21 2017

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

commit 797b215ecbbd5b5b983650ee328d64dc6a8d477c
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 19:31:28 2017

power: powerd_suspend: use /run instead of /var/run

/var/run is now a symlink to /run. This CL changes powerd_suspend to
directly use /run instead of /var/run.

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I28bd3f63a3cbd66478f13fa9e0c2bbcaf1b8d9ed
Reviewed-on: https://chromium-review.googlesource.com/431081
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/797b215ecbbd5b5b983650ee328d64dc6a8d477c/power_manager/powerd/powerd_suspend

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 21 2017

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

commit a5ecd7cc12c130574a492457a0ed01d198746efa
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 19:36:17 2017

power: powerd_suspend: pull $1 into a local variable

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I2ba4aa896ed306fe19d31e91b1a0b13dd605e313
Reviewed-on: https://chromium-review.googlesource.com/431082
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/a5ecd7cc12c130574a492457a0ed01d198746efa/power_manager/powerd/powerd_suspend

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 24 2017

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

commit a8edbf98048445cf777c097a6e2675dfe4ff714e
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 08:24:51 2017

power: powerd_suspend: fix variable and literal quoting

This CL fixes the variable and literal quoting in powerd_suspend to
conform with the Shell Style Guide
(https://google.github.io/styleguide/shell.xml).

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: I3ca630c98485fe905d8a81c01ac9f697ce745481
Reviewed-on: https://chromium-review.googlesource.com/430957
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/a8edbf98048445cf777c097a6e2675dfe4ff714e/power_manager/powerd/powerd_suspend

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 24 2017

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

commit f37c8f3356bbea2b79d7062e27cf64e296aaa6f9
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 19:42:22 2017

power: powerd_suspend: improve do_blaze_huawei_modem

This CL improves the `do_blaze_huawei_modem` function in powerd_suspend
to make it more consistent with ec_power_modem:
- Rename from `do_blaze_huawei_modem` to `power_blaze_modem` for
  better readability and consistency. The `huawei` part is dropped
  because the function simply drives the MODEM_EN GPIO signal to the
  built-in modem, which strictly speaking doesn't necessarily to be a
  Huawei modem.
- Validate for the function argument.
- Return 1 in case of an error.

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend` on Blaze with
     built-in Huawei modem.

Change-Id: I0b5e58711bd750547dd9e6a35496763eb9cc641a
Reviewed-on: https://chromium-review.googlesource.com/431083
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/f37c8f3356bbea2b79d7062e27cf64e296aaa6f9/power_manager/powerd/powerd_suspend

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 24 2017

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

commit 6d9fff383fc557f5dfdcbb659770346269ddba33
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 19:59:22 2017

power: powerd_suspend: reference `grep` instead of `/bin/grep`

Most of the code in powerd_suspend simply references `grep` instead of
`/bin/grep`. This CL changes the only reference to `/bin/grep` to
`grep` for consistency.

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: If2fd3adcc73937a7d0cdc5ae64ad22937d1c5f89
Reviewed-on: https://chromium-review.googlesource.com/431084
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/6d9fff383fc557f5dfdcbb659770346269ddba33/power_manager/powerd/powerd_suspend

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 24 2017

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

commit 27128ea677388e210652f940bc807a5e9e98a55b
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 20:06:01 2017

power: powerd_suspend: simply pass file to grep as argument

This CL changes a grep invocation in powerd_suspend to pass the file as
argument instead of passing the file content via standard input.

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: Id22be38f64b1b440627adfaa9fa66c44585e631f
Reviewed-on: https://chromium-review.googlesource.com/431085
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/27128ea677388e210652f940bc807a5e9e98a55b/power_manager/powerd/powerd_suspend

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 24 2017

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

commit dff07f80294e7ab1e0ad94d0f14c40942bfe3d24
Author: Ben Chan <benchan@chromium.org>
Date: Fri Jan 20 20:11:12 2017

power: powerd_suspend: other minor cleanups

BUG= chromium:683303 
TEST=Tested suspend/resume with `powerd_dbus_suspend`.

Change-Id: Ice3fc02189620eeda260ce9f737d36880b75be84
Reviewed-on: https://chromium-review.googlesource.com/431086
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/dff07f80294e7ab1e0ad94d0f14c40942bfe3d24/power_manager/powerd/powerd_suspend

Status: Started (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 26 2017

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

commit f22ff5325b7cb6ade04fe571af0c3a02f232a7b0
Author: Ruben Rodriguez Buchillon <coconutruben@chromium.org>
Date: Wed Jan 25 20:56:57 2017

power: update regex expression in power_suspend.py

regex expression in power_suspend.py for hwclock used to be
r'(.+) (-?[0-9.]+) seconds'
this worked fine when the script powerd_suspend had
echo ${TIME} > "${TIMESTAMP_FILE}"
as its call. After updating it to be
echo "${TIME}" > "${TIMESTAMP_FILE}"
there's now an extra whitespace between date and seconds. So the
updated regex to use is
r'(.+\w)\s+(-?[0-9.]+) seconds'
otherwise there is a parsing error

BUG= chromium:683303 
TEST=manually ran powerd_dbus_suspend and power_Resume

Change-Id: Ibfc75f37821ab2ad1c8c9ed37e14ad3db8899d4e
Reviewed-on: https://chromium-review.googlesource.com/433197
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/f22ff5325b7cb6ade04fe571af0c3a02f232a7b0/client/cros/power_suspend.py

Status: Fixed (was: Started)

Comment 14 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 15 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 17 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment