From c89ba29e927c3c0320f6b78aee9ed5303515a6fe Mon Sep 17 00:00:00 2001 From: Lei Yu Date: Tue, 28 May 2019 10:42:19 -0700 Subject: [PATCH] Fix issues in battery usage accounting 1. In High usage dialog, show top apps based on battery usage, not app time. 2. Refactor the check for hidden system modules into ShouldHideSipper, however don't smear it, this is also the current logic before this CL. Bug: 133445008 Test: RunSettingsRoboTests Change-Id: I851a1c9ef9b79a934ba0501cd96001f2e450bda4 --- .../BatteryAppListPreferenceController.java | 11 +--- .../android/settings/fuelgauge/BatteryUtils.java | 29 +++++++++- .../batterytip/detectors/HighUsageDetector.java | 46 ++++++++------- .../BatteryAppListPreferenceControllerTest.java | 13 ++--- .../settings/fuelgauge/BatteryUtilsTest.java | 9 +++ .../detectors/HighUsageDetectorTest.java | 66 ++++++++++++---------- 6 files changed, 105 insertions(+), 69 deletions(-) diff --git a/src/com/android/settings/fuelgauge/BatteryAppListPreferenceController.java b/src/com/android/settings/fuelgauge/BatteryAppListPreferenceController.java index 5366ba1998..7741a979a9 100644 --- a/src/com/android/settings/fuelgauge/BatteryAppListPreferenceController.java +++ b/src/com/android/settings/fuelgauge/BatteryAppListPreferenceController.java @@ -352,15 +352,10 @@ public class BatteryAppListPreferenceController extends AbstractPreferenceContro @VisibleForTesting boolean shouldHideSipper(BatterySipper sipper) { - // Don't show hidden system module - final String packageName = mBatteryUtils.getPackageName(sipper.getUid()); - if (!TextUtils.isEmpty(packageName) - && AppUtils.isHiddenSystemModule(mContext, packageName)) { - return true; - } - // Don't show over-counted and unaccounted in any condition + // Don't show over-counted, unaccounted and hidden system module in any condition return sipper.drainType == BatterySipper.DrainType.OVERCOUNTED - || sipper.drainType == BatterySipper.DrainType.UNACCOUNTED; + || sipper.drainType == BatterySipper.DrainType.UNACCOUNTED + || mBatteryUtils.isHiddenSystemModule(sipper); } @VisibleForTesting diff --git a/src/com/android/settings/fuelgauge/BatteryUtils.java b/src/com/android/settings/fuelgauge/BatteryUtils.java index b293695ab8..e4e249c372 100644 --- a/src/com/android/settings/fuelgauge/BatteryUtils.java +++ b/src/com/android/settings/fuelgauge/BatteryUtils.java @@ -47,6 +47,7 @@ import com.android.settings.fuelgauge.batterytip.AnomalyInfo; import com.android.settings.fuelgauge.batterytip.BatteryDatabaseManager; import com.android.settings.fuelgauge.batterytip.StatsManagerConfig; import com.android.settings.overlay.FeatureFactory; +import com.android.settingslib.applications.AppUtils; import com.android.settingslib.fuelgauge.Estimate; import com.android.settingslib.fuelgauge.EstimateKt; import com.android.settingslib.fuelgauge.PowerWhitelistBackend; @@ -187,8 +188,10 @@ public class BatteryUtils { && sipper.drainType != BatterySipper.DrainType.UNACCOUNTED && sipper.drainType != BatterySipper.DrainType.BLUETOOTH && sipper.drainType != BatterySipper.DrainType.WIFI - && sipper.drainType != BatterySipper.DrainType.IDLE) { - // Don't add it if it is overcounted, unaccounted, wifi, bluetooth, or screen + && sipper.drainType != BatterySipper.DrainType.IDLE + && !isHiddenSystemModule(sipper)) { + // Don't add it if it is overcounted, unaccounted, wifi, bluetooth, screen + // or hidden system modules proportionalSmearPowerMah += sipper.totalPowerMah; } } @@ -251,7 +254,27 @@ public class BatteryUtils { || drainType == BatterySipper.DrainType.WIFI || (sipper.totalPowerMah * SECONDS_IN_HOUR) < MIN_POWER_THRESHOLD_MILLI_AMP || mPowerUsageFeatureProvider.isTypeService(sipper) - || mPowerUsageFeatureProvider.isTypeSystem(sipper); + || mPowerUsageFeatureProvider.isTypeSystem(sipper) + || isHiddenSystemModule(sipper); + } + + /** + * Return {@code true} if one of packages in {@code sipper} is hidden system modules + */ + public boolean isHiddenSystemModule(BatterySipper sipper) { + if (sipper.uidObj == null) { + return false; + } + sipper.mPackages = mPackageManager.getPackagesForUid(sipper.getUid()); + if (sipper.mPackages != null) { + for (int i = 0, length = sipper.mPackages.length; i < length; i++) { + if (AppUtils.isHiddenSystemModule(mContext, sipper.mPackages[i])) { + return true; + } + } + } + + return false; } /** diff --git a/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetector.java b/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetector.java index 02af00c39a..067046ca85 100644 --- a/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetector.java +++ b/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetector.java @@ -20,7 +20,6 @@ import static com.android.settings.Utils.SETTINGS_PACKAGE_NAME; import android.content.Context; import android.os.BatteryStats; -import android.text.format.DateUtils; import androidx.annotation.VisibleForTesting; @@ -72,22 +71,33 @@ public class HighUsageDetector implements BatteryTipDetector { if (mPolicy.highUsageEnabled && mDischarging) { parseBatteryData(); if (mDataParser.isDeviceHeavilyUsed() || mPolicy.testHighUsageTip) { - final List batterySippers = mBatteryStatsHelper.getUsageList(); - for (int i = 0, size = batterySippers.size(); i < size; i++) { - final BatterySipper batterySipper = batterySippers.get(i); - if (!mBatteryUtils.shouldHideSipper(batterySipper)) { - final long foregroundTimeMs = mBatteryUtils.getProcessTimeMs( - BatteryUtils.StatusType.FOREGROUND, batterySipper.uidObj, - BatteryStats.STATS_SINCE_CHARGED); - if (foregroundTimeMs >= DateUtils.MINUTE_IN_MILLIS) { - mHighUsageAppList.add(new AppInfo.Builder() - .setUid(batterySipper.getUid()) - .setPackageName( - mBatteryUtils.getPackageName(batterySipper.getUid())) - .setScreenOnTimeMs(foregroundTimeMs) - .build()); - } + final BatteryStats batteryStats = mBatteryStatsHelper.getStats(); + final List batterySippers + = new ArrayList<>(mBatteryStatsHelper.getUsageList()); + final double totalPower = mBatteryStatsHelper.getTotalPower(); + final int dischargeAmount = batteryStats != null + ? batteryStats.getDischargeAmount(BatteryStats.STATS_SINCE_CHARGED) + : 0; + + Collections.sort(batterySippers, + (sipper1, sipper2) -> Double.compare(sipper2.totalSmearedPowerMah, + sipper1.totalSmearedPowerMah)); + for (BatterySipper batterySipper : batterySippers) { + final double percent = mBatteryUtils.calculateBatteryPercent( + batterySipper.totalSmearedPowerMah, totalPower, 0, dischargeAmount); + if ((percent + 0.5f < 1f) || mBatteryUtils.shouldHideSipper(batterySipper)) { + // Don't show it if we should hide or usage percentage is lower than 1% + continue; + } + mHighUsageAppList.add(new AppInfo.Builder() + .setUid(batterySipper.getUid()) + .setPackageName( + mBatteryUtils.getPackageName(batterySipper.getUid())) + .build()); + if (mHighUsageAppList.size() >= mPolicy.highUsageAppCount) { + break; } + } // When in test mode, add an app if necessary @@ -97,10 +107,6 @@ public class HighUsageDetector implements BatteryTipDetector { .setScreenOnTimeMs(TimeUnit.HOURS.toMillis(3)) .build()); } - - Collections.sort(mHighUsageAppList, Collections.reverseOrder()); - mHighUsageAppList = mHighUsageAppList.subList(0, - Math.min(mPolicy.highUsageAppCount, mHighUsageAppList.size())); } } diff --git a/tests/robotests/src/com/android/settings/fuelgauge/BatteryAppListPreferenceControllerTest.java b/tests/robotests/src/com/android/settings/fuelgauge/BatteryAppListPreferenceControllerTest.java index 32829755a6..f8bcf01645 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/BatteryAppListPreferenceControllerTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/BatteryAppListPreferenceControllerTest.java @@ -20,12 +20,14 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import android.content.Context; import android.content.pm.ModuleInfo; import android.content.pm.PackageManager; +import android.os.BatteryStats; import android.os.UserManager; import android.text.TextUtils; import android.text.format.DateUtils; @@ -94,6 +96,7 @@ public class BatteryAppListPreferenceControllerTest { when(mNormalBatterySipper.getPackages()).thenReturn(PACKAGE_NAMES); when(mNormalBatterySipper.getUid()).thenReturn(UID); mNormalBatterySipper.drainType = BatterySipper.DrainType.APP; + mNormalBatterySipper.uidObj = mock(BatteryStats.Uid.class); mPreferenceController = new BatteryAppListPreferenceController(mContext, KEY_APP_LIST, null, mSettingsActivity, mFragment); @@ -203,15 +206,7 @@ public class BatteryAppListPreferenceControllerTest { @Test public void testShouldHideSipper_hiddenSystemModule_returnTrue() { - ReflectionHelpers.setStaticField(ApplicationsState.class, "sInstance", null); - final String packageName = "test.hidden.module"; - final ModuleInfo moduleInfo = new ModuleInfo(); - moduleInfo.setPackageName(packageName); - moduleInfo.setHidden(true); - final List modules = new ArrayList<>(); - modules.add(moduleInfo); - when(mBatteryUtils.getPackageName(anyInt() /* uid */)).thenReturn(packageName); - when(mPackageManager.getInstalledModules(anyInt() /* flags */)).thenReturn(modules); + when(mBatteryUtils.isHiddenSystemModule(mNormalBatterySipper)).thenReturn(true); assertThat(mPreferenceController.shouldHideSipper(mNormalBatterySipper)).isTrue(); } diff --git a/tests/robotests/src/com/android/settings/fuelgauge/BatteryUtilsTest.java b/tests/robotests/src/com/android/settings/fuelgauge/BatteryUtilsTest.java index a60460b8ce..9deaddd2e6 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/BatteryUtilsTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/BatteryUtilsTest.java @@ -368,6 +368,15 @@ public class BatteryUtilsTest { } @Test + public void testShouldHideSipper_hiddenSystemModule_ReturnTrue() { + mNormalBatterySipper.drainType = BatterySipper.DrainType.APP; + when(mNormalBatterySipper.getUid()).thenReturn(UID); + when(mBatteryUtils.isHiddenSystemModule(mNormalBatterySipper)).thenReturn(true); + + assertThat(mBatteryUtils.shouldHideSipper(mNormalBatterySipper)).isTrue(); + } + + @Test public void testCalculateBatteryPercent() { assertThat(mBatteryUtils.calculateBatteryPercent(BATTERY_SYSTEM_USAGE, TOTAL_BATTERY_USAGE, HIDDEN_USAGE, DISCHARGE_AMOUNT)) diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetectorTest.java b/tests/robotests/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetectorTest.java index 7b9e5a5c73..5c56f46c4b 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetectorTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/batterytip/detectors/HighUsageDetectorTest.java @@ -18,6 +18,7 @@ package com.android.settings.fuelgauge.batterytip.detectors; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -40,6 +41,7 @@ import com.android.settings.fuelgauge.batterytip.tips.HighUsageTip; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Answers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.robolectric.RobolectricTestRunner; @@ -52,22 +54,25 @@ import java.util.List; @RunWith(RobolectricTestRunner.class) public class HighUsageDetectorTest { private static final int UID_HIGH = 123; - private static final int UID_ZERO = 345; - private static final long SCREEN_ON_TIME_MS = DateUtils.HOUR_IN_MILLIS; + private static final int UID_LOW = 345; + private static final double POWER_HIGH = 20000; + private static final double POWER_LOW = 10000; private Context mContext; - @Mock + @Mock(answer = Answers.RETURNS_DEEP_STUBS) private BatteryStatsHelper mBatteryStatsHelper; @Mock - private BatteryUtils mBatteryUtils; - @Mock private BatterySipper mHighBatterySipper; @Mock - private BatterySipper mZeroBatterySipper; + private BatterySipper mLowBatterySipper; + @Mock + private BatterySipper mSystemBatterySipper; @Mock private HighUsageDataParser mDataParser; - private AppInfo mAppInfo; + private AppInfo mHighAppInfo; + private AppInfo mLowAppInfo; private BatteryTipPolicy mPolicy; + private BatteryUtils mBatteryUtils; private HighUsageDetector mHighUsageDetector; private List mUsageList; @@ -77,28 +82,37 @@ public class HighUsageDetectorTest { mContext = RuntimeEnvironment.application; mPolicy = spy(new BatteryTipPolicy(mContext)); + mBatteryUtils = spy(BatteryUtils.getInstance(mContext)); mHighUsageDetector = spy(new HighUsageDetector(mContext, mPolicy, mBatteryStatsHelper, true /* mDischarging */)); mHighUsageDetector.mBatteryUtils = mBatteryUtils; mHighUsageDetector.mDataParser = mDataParser; doNothing().when(mHighUsageDetector).parseBatteryData(); doReturn(UID_HIGH).when(mHighBatterySipper).getUid(); + doReturn(UID_LOW).when(mLowBatterySipper).getUid(); mHighBatterySipper.uidObj = mock(BatteryStats.Uid.class); - mZeroBatterySipper.uidObj = mock(BatteryStats.Uid.class); - doReturn(UID_ZERO).when(mZeroBatterySipper).getUid(); - mAppInfo = new AppInfo.Builder() + mHighBatterySipper.drainType = BatterySipper.DrainType.APP; + mHighBatterySipper.totalSmearedPowerMah = POWER_HIGH; + mLowBatterySipper.uidObj = mock(BatteryStats.Uid.class); + mLowBatterySipper.drainType = BatterySipper.DrainType.APP; + mLowBatterySipper.totalSmearedPowerMah = POWER_LOW; + when(mBatteryUtils.shouldHideSipper(mSystemBatterySipper)).thenReturn(true); + when(mBatteryUtils.shouldHideSipper(mHighBatterySipper)).thenReturn(false); + when(mBatteryUtils.shouldHideSipper(mLowBatterySipper)).thenReturn(false); + when(mBatteryStatsHelper.getStats().getDischargeAmount(anyInt())).thenReturn(100); + when(mBatteryStatsHelper.getTotalPower()).thenReturn(POWER_HIGH + POWER_LOW); + + + mHighAppInfo = new AppInfo.Builder() .setUid(UID_HIGH) - .setScreenOnTimeMs(SCREEN_ON_TIME_MS) .build(); - - doReturn(SCREEN_ON_TIME_MS).when(mBatteryUtils).getProcessTimeMs( - BatteryUtils.StatusType.FOREGROUND, mHighBatterySipper.uidObj, - BatteryStats.STATS_SINCE_CHARGED); - doReturn(0L).when(mBatteryUtils).getProcessTimeMs( - BatteryUtils.StatusType.FOREGROUND, mZeroBatterySipper.uidObj, - BatteryStats.STATS_SINCE_CHARGED); + mLowAppInfo = new AppInfo.Builder() + .setUid(UID_LOW) + .build(); mUsageList = new ArrayList<>(); + mUsageList.add(mSystemBatterySipper); + mUsageList.add(mLowBatterySipper); mUsageList.add(mHighBatterySipper); when(mBatteryStatsHelper.getUsageList()).thenReturn(mUsageList); } @@ -128,21 +142,15 @@ public class HighUsageDetectorTest { } @Test - public void testDetect_containsHighUsageApp_tipVisible() { + public void testDetect_containsHighUsageApp_tipVisibleAndSorted() { doReturn(true).when(mDataParser).isDeviceHeavilyUsed(); final HighUsageTip highUsageTip = (HighUsageTip) mHighUsageDetector.detect(); assertThat(highUsageTip.isVisible()).isTrue(); - assertThat(highUsageTip.getHighUsageAppList()).containsExactly(mAppInfo); - } - - @Test - public void testDetect_containsHighUsageApp_removeZeroOne() { - doReturn(true).when(mDataParser).isDeviceHeavilyUsed(); - mUsageList.add(mZeroBatterySipper); - final HighUsageTip highUsageTip = (HighUsageTip) mHighUsageDetector.detect(); - assertThat(highUsageTip.isVisible()).isTrue(); - assertThat(highUsageTip.getHighUsageAppList()).containsExactly(mAppInfo); + // Contain two appInfo and large one comes first + final List appInfos = highUsageTip.getHighUsageAppList(); + assertThat(appInfos).containsExactly(mLowAppInfo, mHighAppInfo); + assertThat(appInfos.get(0)).isEqualTo(mHighAppInfo); } } -- 2.11.0