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

Issue 708180 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 710472



Sign in to add a comment

Configure powerd to not inhibit internal keyboard when side power/volume events go through it

Project Member Reported by derat@chromium.org, Apr 4 2017

Issue description

We need to make powerd avoid inhibiting the internal keyboard while in tablet-like configurations on devices that route side power/volume button events through the keyboard driver.

I did this for caroline via some custom udev rules in https://chromium-review.googlesource.com/c/462380/, but it sounds like there are a lot of affected systems and it'd be nice to not need to copy this into a bunch of BSP packages.

I can think of two ways to do this:

a) powerd's existing 90-powerd-id.rules file currently includes the following:

SUBSYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1",    DRIVERS=="atkbd",        ENV{POWERD_ROLE}="internal_keyboard"
SUBSYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1",    DRIVERS=="cros-ec-keyb", ENV{POWERD_ROLE}="internal_keyboard"

We could add a new POWERD_ROLE like internal_keyboard_with_side_buttons that gets set for atkbd and then configure it the same way as in the caroline change in 91-powerd-tags.rules.

b) We could add a new optionally-installed 91-powerd-tags-keyboard-power-button.rules file that just contains the same stuff as the 92- file in the caroline change.

---

I think that a) could have unwanted side effects on non-convertible clamshells, since we'd no longer be inhibiting the keyboard there while in lid-closed mode, which may result in accidental events.

So b) is probably better. I think that the touchview USE flag is the closest thing we have to an indicator that the system is convertible. Would using caroline-like rules for touchview && (x86 || amd64) be correct?
 
Cc: furquan@chromium.org
I'm a little hesitant to make a broad architectural statement like touchview && (x86 || amd64).

In my opinion, we're having this problem because all of the x86 convertibles we designed so far didn't do a sane thing and have the side buttons as a separate logical input device like all ARM platforms do.

Furquan, for example, has charted a path forward for future x86 convertibles where side buttons are a logically separate input device from the internal keyboard.

In that case, we will have x86 systems where we don't have to do this hack and we can just treat the keyboard as an internal keyboard and that's it.

Comment 2 by derat@chromium.org, Apr 5 2017

Cc: vapier@chromium.org
Okay, sounds like continuing to do one-offs for the affected boards may be the way to go, then.

Another option would be introducing a new USE flag for this. We already have e.g. legacy_power_button, so there's some precedent for this. Then we at least wouldn't need to muck around with BSP packages. The downside is that we'd be building more versions of powerd (although we already key the installation of some powerd pref files off of various USE flags).

Any preferences?

i'd lean towards keeping the rules in a central place.  if we wanted to increase sharing between boards, we could split out these things into a sep package like "power_manager-settings" or something.  but that seems like a low priority to worry about optimization at this point.

i.e. invent another flag like USE=internal_keyboard_with_side_buttons or whatever.

Comment 4 by derat@chromium.org, Apr 5 2017

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 6 2017

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

commit 156511c9c6893200783d439ff7b1ebf1a27dc9d5
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 06 08:42:18 2017

power: Add optional keyboard-side-buttons udev rules file.

Add a new udev/optional directory containing a
92-powerd-tags-keyboard-side-buttons.rules file that can be
selectively installed by the power_manager ebuild for
convertible devices that need to avoid inhibiting the
internal keyboard while in tablet mode due to side
power/volume button events being routed through it.

BUG= chromium:703691 , chromium:708180 
TEST=none

Change-Id: I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c
Reviewed-on: https://chromium-review.googlesource.com/469251
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/156511c9c6893200783d439ff7b1ebf1a27dc9d5/power_manager/udev/optional/92-powerd-tags-keyboard-side-buttons.rules

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/5825e3c875ed14ae54c06a777bbb0f065c0edba1

commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 06 23:11:15 2017

power_manager: Check keyboard_includes_side_buttons USE flag

Make the power_manager package include udev rules files
preventing the keyboard from being inhibited in tablet mode
if the keyboard_includes_side_buttons USE flag is set.

This USE flag indicates that events from the power/volume
buttons on the side of a convertible device are reported via
the internal keyboard.

BUG= chromium:703691 , chromium:708180 
TEST=built with and without the USE flag
CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c

Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19
Reviewed-on: https://chromium-review.googlesource.com/469232
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/5825e3c875ed14ae54c06a777bbb0f065c0edba1/chromeos-base/power_manager/power_manager-9999.ebuild

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/9d2bf1275c24ca4ebc0dae546cc432043208be30

commit 9d2bf1275c24ca4ebc0dae546cc432043208be30
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 08 06:36:23 2017

overlays: Set keyboard_includes_side_buttons for x86 convertibles.

Set the keyboard_includes_side_buttons USE flag for the
following x86 convertible devices: caroline, cave, clapper,
cyan, eve, glimmer, nasher, pyro, reef, snappy, ultima, and
wizpig.

BUG=b:36403242,b:35872420, 708180 
TEST=manual: checked that the flag has the desired effect
     when set, i.e. the power_manager package installs
     /lib/udev/rules.d/92-powerd-tags-keyboard-side-buttons.rules

Change-Id: I9a38a4df812edae7ad55d8f00d2461972bddcfda
Reviewed-on: https://chromium-review.googlesource.com/470287
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>

[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-glimmer/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-pyro/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-caroline/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-nasher/profiles/base/make.defaults
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-ultima/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-snappy/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-reef/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-cyan/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-clapper/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-eve/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-wizpig/make.conf
[modify] https://crrev.com/9d2bf1275c24ca4ebc0dae546cc432043208be30/overlay-cave/make.conf

Comment 8 by derat@chromium.org, Apr 8 2017

Cc: bhthompson@chromium.org
Labels: Merge-Request-58
This obsoletes the earlier per-board config changes at e.g. http://b/36403242. Those have already been approved for merging to 58, and this does the same thing except for more boards that need it and via USE flags so we don't need to mess around with symlinks on BSP ebuilds.
Project Member

Comment 9 by sheriffbot@chromium.org, Apr 9 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 9 2017

Labels: merge-merged-release-R58-9334.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4

commit 52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4
Author: Daniel Erat <derat@chromium.org>
Date: Sun Apr 09 15:38:01 2017

power: Add optional keyboard-side-buttons udev rules file.

Add a new udev/optional directory containing a
92-powerd-tags-keyboard-side-buttons.rules file that can be
selectively installed by the power_manager ebuild for
convertible devices that need to avoid inhibiting the
internal keyboard while in tablet mode due to side
power/volume button events being routed through it.

BUG= chromium:703691 , chromium:708180 
TEST=none

Change-Id: I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c
Reviewed-on: https://chromium-review.googlesource.com/469251
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
(cherry picked from commit 156511c9c6893200783d439ff7b1ebf1a27dc9d5)
Reviewed-on: https://chromium-review.googlesource.com/472530

[add] https://crrev.com/52ffe5fab176d7dda2a3375514c6ac5c7ec63fc4/power_manager/udev/optional/92-powerd-tags-keyboard-side-buttons.rules

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/bdc171ef15c36a48aae6093e460e0310a60b8405

commit bdc171ef15c36a48aae6093e460e0310a60b8405
Author: Daniel Erat <derat@chromium.org>
Date: Sun Apr 09 15:48:32 2017

power_manager: Check keyboard_includes_side_buttons USE flag

Make the power_manager package include udev rules files
preventing the keyboard from being inhibited in tablet mode
if the keyboard_includes_side_buttons USE flag is set.

This USE flag indicates that events from the power/volume
buttons on the side of a convertible device are reported via
the internal keyboard.

BUG= chromium:703691 , chromium:708180 
TEST=built with and without the USE flag
CQ-DEPEND=I06ff6e1c8c291ea68f7315afdda5426ac5fcf33c

Change-Id: Iaeaa8ab5fb72700d2114248cef940f2de2661d19
Reviewed-on: https://chromium-review.googlesource.com/469232
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
(cherry picked from commit 5825e3c875ed14ae54c06a777bbb0f065c0edba1)
Reviewed-on: https://chromium-review.googlesource.com/472531
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/bdc171ef15c36a48aae6093e460e0310a60b8405/chromeos-base/power_manager/power_manager-9999.ebuild

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/7126a25f0db460328950d52f1c9f74f828b9d8da

commit 7126a25f0db460328950d52f1c9f74f828b9d8da
Author: Daniel Erat <derat@chromium.org>
Date: Sun Apr 09 15:55:54 2017

overlays: Set keyboard_includes_side_buttons for x86 convertibles.

Set the keyboard_includes_side_buttons USE flag for the
following x86 convertible devices: caroline, cave, clapper,
cyan, eve, glimmer, nasher, pyro, reef, snappy, ultima, and
wizpig.

BUG=b:36403242,b:35872420, 708180 
TEST=manual: checked that the flag has the desired effect
     when set, i.e. the power_manager package installs
     /lib/udev/rules.d/92-powerd-tags-keyboard-side-buttons.rules

Change-Id: I9a38a4df812edae7ad55d8f00d2461972bddcfda
Reviewed-on: https://chromium-review.googlesource.com/470287
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
(cherry picked from commit 9d2bf1275c24ca4ebc0dae546cc432043208be30)
Reviewed-on: https://chromium-review.googlesource.com/472567
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-glimmer/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-pyro/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-caroline/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-ultima/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-snappy/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-reef/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-cyan/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-clapper/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-eve/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-wizpig/make.conf
[modify] https://crrev.com/7126a25f0db460328950d52f1c9f74f828b9d8da/overlay-cave/make.conf

Labels: -Merge-Approved-58 Merge-Merged
Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Verified with version 58.0.3029.66/9334.40.0 beta-channel caroline
(1. Power button to wake up in tablet mode  2. Volume up and down button in tablet mode) 
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b

commit 5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b
Author: Daniel Erat <derat@chromium.org>
Date: Mon Apr 10 21:28:41 2017

overlays: Remove old powerd udev rules for caroline/cave.

Make chromeos-bsp-caroline and chromeos-bsp-cave stop
installing 92-powerd-overrides.rules. Identical rules are
installed by the power_manager package when the
keyboard_includes_side_buttons USE flag is set now.

BUG= chromium:708180 ,b:36403242
TEST=none

Change-Id: I055e1bb56ecb0c0c8995d748775622132e7788ae
Reviewed-on: https://chromium-review.googlesource.com/472229
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b/overlay-cave/chromeos-base/chromeos-bsp-cave/chromeos-bsp-cave-0.0.1.ebuild
[rename] https://crrev.com/5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b/overlay-cave/chromeos-base/chromeos-bsp-cave/chromeos-bsp-cave-0.0.1-r18.ebuild
[modify] https://crrev.com/5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b/overlay-caroline/chromeos-base/chromeos-bsp-caroline/chromeos-bsp-caroline-0.0.1.ebuild
[delete] https://crrev.com/7259e598787a7d5cdbfd23ed2d33d70a23599a24/overlay-caroline/chromeos-base/chromeos-bsp-caroline/files/92-powerd-overrides.rules
[rename] https://crrev.com/5ccb21543a68cc4b6d66d966fbb1d49c2e7f8a6b/overlay-caroline/chromeos-base/chromeos-bsp-caroline/chromeos-bsp-caroline-0.0.1-r13.ebuild
[delete] https://crrev.com/7259e598787a7d5cdbfd23ed2d33d70a23599a24/overlay-cave/chromeos-base/chromeos-bsp-cave/files/92-powerd-overrides.rules

Comment 16 by derat@chromium.org, Apr 11 2017

Blocking: 710472

Sign in to add a comment