From b42d0c70c2f47672af66cd9dac3071c69176c0f8 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Mon, 29 Jun 2020 11:37:34 -0700 Subject: [PATCH] Fix and expand battery saver stats tracking. The STATE_CHARGING value had the BatterySaverState bits to resemble "ON", so dumpsys would incorrectly say battery saver was on whenever the device was plugged in. It also meant that the enabled count would incorrectly increase whenever the device was plugged and unplugged. This change fixes that by explicitly checking for the magic values before incrementing the counters. In addition, this adds more logging around adaptive battery saver stats and removes the STATE_CHARGING magic value in favor of an explicit PLUGGED/UNPLUGGED state to move away from the assumption that battery saver is off whenever the device is plugged in. Bug: 32423528 Bug: 119261320 Bug: 158581564 Test: atest com.android.server.power.batterysaver.BatterySaverPolicyTest Test: atest com.android.server.power.batterysaver.BatterySaverStateMachineTest Test: atest com.android.server.power.batterysaver.BatterySavingStatsTest Test: atest com.android.server.power.PowerManagerServiceTest Change-Id: I071d47299360a003febd5251c2515c0b55b3cfff --- .../batterysaver/BatterySaverController.java | 8 +- .../batterysaver/BatterySavingStats.java | 145 +++++++++++++----- .../batterysaver/BatterySavingStatsTest.java | 101 ++++++++---- 3 files changed, 176 insertions(+), 78 deletions(-) diff --git a/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java b/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java index 6ff069bd049df..dd287ca6ed001 100644 --- a/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java +++ b/services/core/java/com/android/server/power/batterysaver/BatterySaverController.java @@ -49,6 +49,7 @@ import com.android.server.power.batterysaver.BatterySaverPolicy.Policy; import com.android.server.power.batterysaver.BatterySavingStats.BatterySaverState; import com.android.server.power.batterysaver.BatterySavingStats.DozeState; import com.android.server.power.batterysaver.BatterySavingStats.InteractiveState; +import com.android.server.power.batterysaver.BatterySavingStats.PlugState; import java.util.ArrayList; import java.util.Objects; @@ -551,17 +552,14 @@ public class BatterySaverController implements BatterySaverPolicyListener { : DozeState.NOT_DOZING; synchronized (mLock) { - if (mIsPluggedIn) { - mBatterySavingStats.startCharging(); - return; - } mBatterySavingStats.transitionState( getFullEnabledLocked() ? BatterySaverState.ON : (getAdaptiveEnabledLocked() ? BatterySaverState.ADAPTIVE : BatterySaverState.OFF), isInteractive ? InteractiveState.INTERACTIVE : InteractiveState.NON_INTERACTIVE, - dozeMode); + dozeMode, + mIsPluggedIn ? PlugState.PLUGGED : PlugState.UNPLUGGED); } } diff --git a/services/core/java/com/android/server/power/batterysaver/BatterySavingStats.java b/services/core/java/com/android/server/power/batterysaver/BatterySavingStats.java index 3dbc0072f8b40..05695d919910f 100644 --- a/services/core/java/com/android/server/power/batterysaver/BatterySavingStats.java +++ b/services/core/java/com/android/server/power/batterysaver/BatterySavingStats.java @@ -17,8 +17,8 @@ package com.android.server.power.batterysaver; import android.os.BatteryManagerInternal; import android.os.SystemClock; -import android.util.ArrayMap; import android.util.Slog; +import android.util.SparseArray; import android.util.TimeUtils; import com.android.internal.annotations.GuardedBy; @@ -93,6 +93,20 @@ public class BatterySavingStats { } } + /** Whether the device is plugged in or not. */ + interface PlugState { + int UNPLUGGED = 0; + int PLUGGED = 1; + + int SHIFT = DozeState.SHIFT + DozeState.BITS; + int BITS = 1; + int MASK = (1 << BITS) - 1; + + static int fromIndex(int index) { + return (index >> SHIFT) & MASK; + } + } + /** * Various stats in each state. */ @@ -140,7 +154,6 @@ public class BatterySavingStats { private BatteryManagerInternal mBatteryManagerInternal; private static final int STATE_NOT_INITIALIZED = -1; - private static final int STATE_CHARGING = -2; /** * Current state, one of STATE_* or values returned by {@link #statesToIndex}. @@ -153,20 +166,26 @@ public class BatterySavingStats { */ @VisibleForTesting @GuardedBy("mLock") - final ArrayMap mStats = new ArrayMap<>(); + final SparseArray mStats = new SparseArray<>(); @GuardedBy("mLock") private int mBatterySaverEnabledCount = 0; - @GuardedBy("mLock") - private boolean mIsBatterySaverEnabled; - @GuardedBy("mLock") private long mLastBatterySaverEnabledTime = 0; @GuardedBy("mLock") private long mLastBatterySaverDisabledTime = 0; + @GuardedBy("mLock") + private int mAdaptiveBatterySaverEnabledCount = 0; + + @GuardedBy("mLock") + private long mLastAdaptiveBatterySaverEnabledTime = 0; + + @GuardedBy("mLock") + private long mLastAdaptiveBatterySaverDisabledTime = 0; + /** Visible for unit tests */ @VisibleForTesting public BatterySavingStats(Object lock) { @@ -189,10 +208,11 @@ public class BatterySavingStats { */ @VisibleForTesting static int statesToIndex( - int batterySaverState, int interactiveState, int dozeState) { + int batterySaverState, int interactiveState, int dozeState, int plugState) { int ret = batterySaverState & BatterySaverState.MASK; ret |= (interactiveState & InteractiveState.MASK) << InteractiveState.SHIFT; ret |= (dozeState & DozeState.MASK) << DozeState.SHIFT; + ret |= (plugState & PlugState.MASK) << PlugState.SHIFT; return ret; } @@ -204,12 +224,11 @@ public class BatterySavingStats { switch (state) { case STATE_NOT_INITIALIZED: return "NotInitialized"; - case STATE_CHARGING: - return "Charging"; } return "BS=" + BatterySaverState.fromIndex(state) + ",I=" + InteractiveState.fromIndex(state) - + ",D=" + DozeState.fromIndex(state); + + ",D=" + DozeState.fromIndex(state) + + ",P=" + PlugState.fromIndex(state); } /** @@ -230,8 +249,9 @@ public class BatterySavingStats { /** * @return {@link Stat} fo a given state triplet. */ - private Stat getStat(int batterySaverState, int interactiveState, int dozeState) { - return getStat(statesToIndex(batterySaverState, interactiveState, dozeState)); + private Stat getStat(int batterySaverState, int interactiveState, int dozeState, + int plugState) { + return getStat(statesToIndex(batterySaverState, interactiveState, dozeState, plugState)); } @VisibleForTesting @@ -258,26 +278,17 @@ public class BatterySavingStats { } /** - * Called from the outside whenever any of the states changes, when the device is not plugged - * in. + * Called from the outside whenever any of the states change. */ - public void transitionState(int batterySaverState, int interactiveState, int dozeState) { + void transitionState(int batterySaverState, int interactiveState, int dozeState, + int plugState) { synchronized (mLock) { final int newState = statesToIndex( - batterySaverState, interactiveState, dozeState); + batterySaverState, interactiveState, dozeState, plugState); transitionStateLocked(newState); } } - /** - * Called from the outside when the device is plugged in. - */ - public void startCharging() { - synchronized (mLock) { - transitionStateLocked(STATE_CHARGING); - } - } - @GuardedBy("mLock") private void transitionStateLocked(int newState) { if (mCurrentState == newState) { @@ -287,17 +298,33 @@ public class BatterySavingStats { final int batteryLevel = injectBatteryLevel(); final int batteryPercent = injectBatteryPercent(); - final boolean oldBatterySaverEnabled = - BatterySaverState.fromIndex(mCurrentState) != BatterySaverState.OFF; - final boolean newBatterySaverEnabled = - BatterySaverState.fromIndex(newState) != BatterySaverState.OFF; - if (oldBatterySaverEnabled != newBatterySaverEnabled) { - mIsBatterySaverEnabled = newBatterySaverEnabled; - if (newBatterySaverEnabled) { - mBatterySaverEnabledCount++; - mLastBatterySaverEnabledTime = injectCurrentTime(); - } else { - mLastBatterySaverDisabledTime = injectCurrentTime(); + final int oldBatterySaverState = mCurrentState < 0 + ? BatterySaverState.OFF : BatterySaverState.fromIndex(mCurrentState); + final int newBatterySaverState = newState < 0 + ? BatterySaverState.OFF : BatterySaverState.fromIndex(newState); + if (oldBatterySaverState != newBatterySaverState) { + switch (newBatterySaverState) { + case BatterySaverState.ON: + mBatterySaverEnabledCount++; + mLastBatterySaverEnabledTime = now; + if (oldBatterySaverState == BatterySaverState.ADAPTIVE) { + mLastAdaptiveBatterySaverDisabledTime = now; + } + break; + case BatterySaverState.OFF: + if (oldBatterySaverState == BatterySaverState.ON) { + mLastBatterySaverDisabledTime = now; + } else { + mLastAdaptiveBatterySaverDisabledTime = now; + } + break; + case BatterySaverState.ADAPTIVE: + mAdaptiveBatterySaverEnabledCount++; + mLastAdaptiveBatterySaverEnabledTime = now; + if (oldBatterySaverState == BatterySaverState.ON) { + mLastBatterySaverDisabledTime = now; + } + break; } } @@ -377,7 +404,18 @@ public class BatterySavingStats { pw.print(indent); pw.print("Battery Saver is currently: "); - pw.println(mIsBatterySaverEnabled ? "ON" : "OFF"); + switch (BatterySaverState.fromIndex(mCurrentState)) { + case BatterySaverState.OFF: + pw.println("OFF"); + break; + case BatterySaverState.ON: + pw.println("ON"); + break; + case BatterySaverState.ADAPTIVE: + pw.println("ADAPTIVE"); + break; + } + if (mLastBatterySaverEnabledTime > 0) { pw.print(indent); pw.print(" "); @@ -400,9 +438,34 @@ public class BatterySavingStats { pw.print(indent); pw.print(" "); - pw.print("Times enabled: "); + pw.print("Times full enabled: "); pw.println(mBatterySaverEnabledCount); + if (mLastAdaptiveBatterySaverEnabledTime > 0) { + pw.print(indent); + pw.print(" "); + pw.print("Last ADAPTIVE ON time: "); + pw.print(sdf.format( + new Date(now - nowElapsed + mLastAdaptiveBatterySaverEnabledTime))); + pw.print(" "); + TimeUtils.formatDuration(mLastAdaptiveBatterySaverEnabledTime, nowElapsed, pw); + pw.println(); + } + if (mLastAdaptiveBatterySaverDisabledTime > 0) { + pw.print(indent); + pw.print(" "); + pw.print("Last ADAPTIVE OFF time: "); + pw.print(sdf.format( + new Date(now - nowElapsed + mLastAdaptiveBatterySaverDisabledTime))); + pw.print(" "); + TimeUtils.formatDuration(mLastAdaptiveBatterySaverDisabledTime, nowElapsed, pw); + pw.println(); + } + pw.print(indent); + pw.print(" "); + pw.print("Times adaptive enabled: "); + pw.println(mAdaptiveBatterySaverEnabledCount); + pw.println(); pw.print(indent); @@ -436,8 +499,10 @@ public class BatterySavingStats { pw.print(interactiveLabel); pw.print(": "); - final Stat offStat = getStat(BatterySaverState.OFF, interactiveState, dozeState); - final Stat onStat = getStat(BatterySaverState.ON, interactiveState, dozeState); + final Stat offStat = getStat(BatterySaverState.OFF, interactiveState, dozeState, + PlugState.UNPLUGGED); + final Stat onStat = getStat(BatterySaverState.ON, interactiveState, dozeState, + PlugState.UNPLUGGED); pw.println(String.format("%6dm %6dmAh(%3d%%) %8.1fmAh/h %6dm %6dmAh(%3d%%) %8.1fmAh/h", offStat.totalMinutes(), diff --git a/services/tests/servicestests/src/com/android/server/power/batterysaver/BatterySavingStatsTest.java b/services/tests/servicestests/src/com/android/server/power/batterysaver/BatterySavingStatsTest.java index 76239fcd93858..72d6caf1a5be4 100644 --- a/services/tests/servicestests/src/com/android/server/power/batterysaver/BatterySavingStatsTest.java +++ b/services/tests/servicestests/src/com/android/server/power/batterysaver/BatterySavingStatsTest.java @@ -33,6 +33,7 @@ import com.android.internal.logging.MetricsLogger; import com.android.server.power.batterysaver.BatterySavingStats.BatterySaverState; import com.android.server.power.batterysaver.BatterySavingStats.DozeState; import com.android.server.power.batterysaver.BatterySavingStats.InteractiveState; +import com.android.server.power.batterysaver.BatterySavingStats.PlugState; import org.junit.Test; import org.junit.runner.RunWith; @@ -118,7 +119,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(4); target.drainBattery(100); @@ -126,7 +128,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(2); target.drainBattery(500); @@ -134,7 +137,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(4); target.drainBattery(100); @@ -142,7 +146,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(2); target.drainBattery(500); @@ -150,7 +155,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(3); target.drainBattery(100); @@ -158,7 +164,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.LIGHT); + DozeState.LIGHT, + PlugState.UNPLUGGED); target.advanceClock(5); target.drainBattery(100); @@ -166,7 +173,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.DEEP); + DozeState.DEEP, + PlugState.UNPLUGGED); target.advanceClock(1); target.drainBattery(200); @@ -174,7 +182,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.ON, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(1); target.drainBattery(300); @@ -182,7 +191,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(3); target.drainBattery(500); @@ -190,12 +200,17 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.ON, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(3); target.drainBattery(500); - target.startCharging(); + target.transitionState( + BatterySaverState.ON, + InteractiveState.INTERACTIVE, + DozeState.NOT_DOZING, + PlugState.PLUGGED); target.advanceClock(5); target.drainBattery(1000); @@ -203,28 +218,34 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.ON, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); target.advanceClock(5); target.drainBattery(100); - target.startCharging(); + target.transitionState( + BatterySaverState.ON, + InteractiveState.INTERACTIVE, + DozeState.NOT_DOZING, + PlugState.PLUGGED); target.assertDumpable(); assertEquals( - "BS=0,I=0,D=0:{4m,1000,15000.00uA/H,1500.00%}\n" + - "BS=1,I=0,D=0:{0m,0,0.00uA/H,0.00%}\n" + - "BS=0,I=1,D=0:{14m,800,3428.57uA/H,342.86%}\n" + - "BS=1,I=1,D=0:{9m,900,6000.00uA/H,600.00%}\n" + - "BS=0,I=0,D=1:{5m,100,1200.00uA/H,120.00%}\n" + - "BS=1,I=0,D=1:{0m,0,0.00uA/H,0.00%}\n" + - "BS=0,I=1,D=1:{0m,0,0.00uA/H,0.00%}\n" + - "BS=1,I=1,D=1:{0m,0,0.00uA/H,0.00%}\n" + - "BS=0,I=0,D=2:{1m,200,12000.00uA/H,1200.00%}\n" + - "BS=1,I=0,D=2:{0m,0,0.00uA/H,0.00%}\n" + - "BS=0,I=1,D=2:{0m,0,0.00uA/H,0.00%}\n" + - "BS=1,I=1,D=2:{0m,0,0.00uA/H,0.00%}", + "BS=0,I=0,D=0,P=0:{4m,1000,15000.00uA/H,1500.00%}\n" + + "BS=1,I=0,D=0,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=0,I=1,D=0,P=0:{14m,800,3428.57uA/H,342.86%}\n" + + "BS=1,I=1,D=0,P=0:{9m,900,6000.00uA/H,600.00%}\n" + + "BS=0,I=0,D=1,P=0:{5m,100,1200.00uA/H,120.00%}\n" + + "BS=1,I=0,D=1,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=0,I=1,D=1,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=1,I=1,D=1,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=0,I=0,D=2,P=0:{1m,200,12000.00uA/H,1200.00%}\n" + + "BS=1,I=0,D=2,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=0,I=1,D=2,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=1,I=1,D=2,P=0:{0m,0,0.00uA/H,0.00%}\n" + + "BS=1,I=1,D=0,P=1:{5m,1000,12000.00uA/H,1200.00%}", target.toDebugString()); } @@ -242,7 +263,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); verify(mMetricsLogger, times(0)).count(anyString(), anyInt()); @@ -253,7 +275,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); assertLog(); @@ -264,7 +287,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.DEEP); + DozeState.DEEP, + PlugState.UNPLUGGED); target.advanceClock(1); target.drainBattery(2000); @@ -274,7 +298,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.OFF, InteractiveState.NON_INTERACTIVE, - DozeState.LIGHT); + DozeState.LIGHT, + PlugState.UNPLUGGED); target.advanceClock(1); target.drainBattery(2000); @@ -284,7 +309,8 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.ON, InteractiveState.INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); assertLog(); @@ -292,7 +318,11 @@ public class BatterySavingStatsTest { target.drainBattery(10000); reset(mMetricsLogger); - target.startCharging(); + target.transitionState( + BatterySaverState.ON, + InteractiveState.INTERACTIVE, + DozeState.NOT_DOZING, + PlugState.PLUGGED); assertLog(); @@ -303,14 +333,19 @@ public class BatterySavingStatsTest { target.transitionState( BatterySaverState.ON, InteractiveState.NON_INTERACTIVE, - DozeState.NOT_DOZING); + DozeState.NOT_DOZING, + PlugState.UNPLUGGED); verify(mMetricsLogger, times(0)).count(anyString(), anyInt()); target.advanceClock(1); target.drainBattery(2000); - target.startCharging(); + target.transitionState( + BatterySaverState.ON, + InteractiveState.NON_INTERACTIVE, + DozeState.NOT_DOZING, + PlugState.PLUGGED); assertLog(); }