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

Issue 747501 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

Consider running powerd in factory mode

Project Member Reported by derat@chromium.org, Jul 21 2017

Issue description

Right now powerd is stopped on Chrome OS devices in factory mode. Per Hung-Te at http://b/63878094#comment15:

"We had some discussion with old powerd owner that we want lots of stuff, including no suspend (both idle and lid-close), no panel off, no sleep, .... etc, and in the end the owner gives up adding a new mode for us and said stop is probably the most easy solution."

I've been the powerd owner for a while now and am happy to add this if it means we don't need workarounds like https://chromium-review.googlesource.com/c/581247. :-)

If you give me a list of what you need, I'll add one or more powerd prefs for configuring it. Then it'd just be a matter of doing something like "echo 1>/var/lib/power_manager/factory_mode".
 

Comment 1 by hungte@chromium.org, Jul 21 2017

Cc: chromeos-factory-eng@google.com
Hmmm. I may not be able to list all features we need now since we've been testing with powerd turned off for a very long time (and those mails were gone, thanks to gmail), so we'll need to start with few features and evaluate in the incoming builds.

We need at least:
 1. Don't do idle suspend (but we may trigger suspend when the test needs to).
 2. Don't do lid close suspend (but we do want a way to detect lid close event).
 3. Never turn off screen on idle.
 4. Never turn off network on idle.

+chromeos-factory-eng to see if anyone can think of more.

Comment 2 by hungte@chromium.org, Jul 21 2017

Oh we also need to control charging/discharge. we do this by checking battery state and invoke ectool chargecontrol, but powerd should not change what we set.

Comment 3 by derat@chromium.org, Jul 21 2017

Status: Started (was: Assigned)
Do you want the screen to dim when the user is inactive? Do you want the brightness to be adjusted if the ambient light level changes? Alternately, let me know if you want the backlight to always remain at 100% brightness.

What behavior do you want from the keyboard backlight?

For the most part, powerd doesn't control charging; it just reports what's going on to Chrome. If the user selects a different Type-C charging source via Chrome, powerd passes the request on to the kernel, but I assume that that won't be happening in factory mode.

Comment 4 by hungte@chromium.org, Jul 21 2017

> Do you want the screen to dim when the user is inactive?
 No because we have tests to verify specified screen backlight level.

> Do you want the brightness to be adjusted if the ambient light level changes?
 No because the ALS is not calibrated yet during most of the manufacturing stage.

> Alternately, let me know if you want the backlight to always remain at 100% brightness.
 We want it to keep 100% or some default value, and changes when we invoke the tool to change backlight (and keep changed value until we set it again).

> What behavior do you want from the keyboard backlight?
 Just keep it in a default state and can be changed when we try to change it by commands. That's also a test item.

> but I assume that that won't be happening in factory mode.
 The factory needs to test if both parts can charge, so we do ask op to attach different ports and verify the pd state, but no, we don't let OP change selection from Chrome.

In short, we want everything to keep in default values but still change-able when we run related commands.

Comment 5 by derat@chromium.org, Jul 21 2017

>> Alternately, let me know if you want the backlight to always remain at 100% brightness.
>
> We want it to keep 100% or some default value, and changes when we invoke the tool to
> change backlight (and keep changed value until we set it again).

What tool do you use? Is it the backlight_tool program from the power_manager package?

I'll probably look into just making powerd not touch the display or keyboard backlight devices.

Comment 6 by hungte@chromium.org, Jul 21 2017

In current general implementation, sysfs: https://chromium.googlesource.com/chromiumos/platform/factory/+/master/py/device/display.py#45

We can use backlight_tool on cros if needed.

Comment 7 by derat@chromium.org, Jul 21 2017

Sticking with sysfs should be fine. (Calling backlight_tool instead of interacting directly with sysfs may simplify your code, though.)

I've uploaded https://chromium-review.googlesource.com/c/582231/. Are you set up to give it a try? Writing "1" to /var/lib/power_manager/factory_mode and restarting powerd or rebooting should be all it takes.

Are factory images still created by running a script that modifies a regular image? If so, you'll probably want to make it create /usr/share/power_manager/factory_mode.

Note that powerd still inhibits input devices as needed for laptop, tablet, lid-closed, etc. modes. Let me know if you need it to leave all input devices active at all times instead (although I think the EC will still disable the keyboard when in tablet mode, for instance).

Comment 8 by hungte@chromium.org, Jul 22 2017

@pihsun, can you try the CL in c#7 and see if that would work for tests like lid close / laptop mode switch?

Re#7
factory images are currently using standard "test" image, plus additional files in /usr/local/factory. We can't create /usr/share/power_manager/factory_mode since that's in test image's rootfs (ro).


Comment 9 by derat@chromium.org, Jul 22 2017

Okay, got it. Using /var/lib/power_manager/factory_mode on the stateful partition should work, then.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 23 2017

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

commit b4e79726f0231b95a0aafdbbd98ab44c846d33d9
Author: Daniel Erat <derat@chromium.org>
Date: Sun Jul 23 16:26:55 2017

power: Add factory_mode pref.

Add support for a new "factory_mode" powerd pref that can be
used to request factory-specific behavior from powerd:

- Don't change screen or keyboard brightness.
- Do nothing when system is idle.
- Do nothing when lid is closed.

BUG= chromium:747501 
TEST=added unit tests; also manually tested that system
     behaves as described after writing "1" to
     /var/lib/power_manager/factory_mode

Change-Id: I3db3ed7188cc4ba487e4c0028c214a54a53a58cd
Reviewed-on: https://chromium-review.googlesource.com/582231
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/common/power_constants.cc
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/policy/state_controller_unittest.cc
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/common/power_constants.h
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/daemon.cc
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/policy/state_controller.h
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/daemon.h
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/b4e79726f0231b95a0aafdbbd98ab44c846d33d9/power_manager/powerd/policy/state_controller.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/6ddc2fded87c2762792ee4ce544b59ab767e75fa

commit 6ddc2fded87c2762792ee4ce544b59ab767e75fa
Author: Peter Shih <pihsun@chromium.org>
Date: Mon Jul 24 13:27:56 2017

enable powerd with factory mode.

powerd support factory mode after CL:582231, so we can enable powerd in
factory, and don't need special hack to make pytest like tablet_rotation
to work.

BUG= chromium:747501 
TEST=manually, test on DUT and check that there's no idle suspend.

Change-Id: I85fc1ec850bb57a0ba0b54e2775b84215a9da0e5
Reviewed-on: https://chromium-review.googlesource.com/582252
Commit-Ready: Pi-Hsun Shih <pihsun@chromium.org>
Tested-by: Pi-Hsun Shih <pihsun@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[delete] https://crrev.com/a6b560919185397e63d475badb7a5d9df5ee04f0/init/common.d/inhibit_jobs/powerd
[add] https://crrev.com/6ddc2fded87c2762792ee4ce544b59ab767e75fa/init/common.d/powerd_factory_mode.sh

Comment 12 by derat@chromium.org, Jul 24 2017

https://chromium-review.googlesource.com/c/582252/3/init/common.d/inhibit_jobs/powerd says "we need to disable powerd to access power button". Is that still the case? If so, can you tell me the behavior that you need? Do you want powerd to not report power button presses and releases to Chrome (which makes decisions about when to lock the screen or shut down the system) while in factory mode?
Re#12 oh that's right. In keyboard test we have to make sure all keys can be pressed properly, so we need to receive power button key down and up.

We need to capture the key down / up events from evdev. We don't really care if Chrome received the key events or not, but pressing power button should not trigger Chrome to start logout / shutdown, so I think the answer is powerd should not report power button presses to Chrome.

Comment 14 by derat@chromium.org, Jul 24 2017

I've uploaded https://chromium-review.googlesource.com/c/582869 to make powerd ignore power button events when factory_mode is set. Let me know if anything else comes up.
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 25 2017

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

commit c7cf62c9c507abc6852dddf74e092e3e2cf57044
Author: Daniel Erat <derat@chromium.org>
Date: Tue Jul 25 05:54:05 2017

power: Ignore power button for factory mode.

Make powerd ignore power button events when the factory_mode
pref is set. This is needed for keyboard testing, in which
all keys are pressed and evdev is used to watch for events.

BUG= chromium:747501 
TEST=added test

Change-Id: I09c42ba288179d8032e16bed835c0f6464a81eba
Reviewed-on: https://chromium-review.googlesource.com/582869
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Pi-Hsun Shih <pihsun@chromium.org>

[modify] https://crrev.com/c7cf62c9c507abc6852dddf74e092e3e2cf57044/power_manager/powerd/policy/input_event_handler.cc
[modify] https://crrev.com/c7cf62c9c507abc6852dddf74e092e3e2cf57044/power_manager/powerd/policy/input_event_handler.h
[modify] https://crrev.com/c7cf62c9c507abc6852dddf74e092e3e2cf57044/power_manager/powerd/policy/input_event_handler_unittest.cc

Comment 16 by derat@chromium.org, Jul 25 2017

Another small difference that I just thought of between powerd's factory_mode and not running powerd at all: right now, powerd shuts the system down when its battery is almost empty. The EC would do this anyway, but powerd may do it at around ~1% higher. Is this something that you test in the factory?
Re#16
We do test battery level and charging, but it's using a higher minimal level, say 5% ( https://storage.googleapis.com/chromeos-factory-docs/sdk/pytests/battery_cycle.html ) - although the number may vary per project.

If powerd will only increase that at 1% I think it's probably fine (but again, this depends on OEM's requirement per project) - but yes it would be even better if powerd won't add 1%. 
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 26 2017

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

commit 1ffee1606dda97dc80b0fd957181956d4d811478
Author: Daniel Erat <derat@chromium.org>
Date: Wed Jul 26 18:34:46 2017

power: Don't shut down for low battery in factory mode.

Make powerd avoid shutting the system down in response to a
low battery charge when the factory_mode pref is set to
true.

BUG= chromium:747501 
TEST=added test

Change-Id: Id0fc48fb2bc51758baf21cf7f96d88d444b4e1eb
Reviewed-on: https://chromium-review.googlesource.com/585859
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/1ffee1606dda97dc80b0fd957181956d4d811478/power_manager/powerd/daemon.cc
[modify] https://crrev.com/1ffee1606dda97dc80b0fd957181956d4d811478/power_manager/powerd/daemon.h
[modify] https://crrev.com/1ffee1606dda97dc80b0fd957181956d4d811478/power_manager/powerd/daemon_unittest.cc

Comment 19 by derat@chromium.org, Jul 26 2017

Status: Fixed (was: Started)
Feel free to reopen this or file a new bug against me if you need anything else!

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

Status: Archived (was: Fixed)

Sign in to add a comment