From a5c1b20fe4ef440b90fe5eb4604ff0ce32f1cebf Mon Sep 17 00:00:00 2001 From: Michael Wachenschwanz Date: Fri, 29 Jan 2021 00:56:31 -0800 Subject: [PATCH] Fix MeasureEnergyArray impl array size getEnergyConsumptionData wrongly set the MeasuredEnergyArray size to the received EnergyConsumerResult array. Instead the size should be equal to the number of valid Subsystems added to the array. Also add BatteryExternalStatsWorkerTest to FrameworkServicesTests Fixes: 178747292 Test: atest FrameworksServicesTests:BatteryExternalStatsWorkerTest Change-Id: I43793c7ddf9ff1b1badf478c0d9c3accb1a328d5 --- .../server/am/BatteryExternalStatsWorker.java | 57 +++++- .../am/BatteryExternalStatsWorkerTest.java | 187 ++++++++++++++++++ 2 files changed, 234 insertions(+), 10 deletions(-) create mode 100644 services/tests/servicestests/src/com/android/server/am/BatteryExternalStatsWorkerTest.java diff --git a/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java b/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java index b153cfdb182cd..170c34d2e8593 100644 --- a/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java +++ b/services/core/java/com/android/server/am/BatteryExternalStatsWorker.java @@ -43,6 +43,7 @@ import android.util.SparseIntArray; import android.util.SparseLongArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.os.BatteryStatsImpl; import com.android.internal.power.MeasuredEnergyArray; import com.android.internal.power.MeasuredEnergyStats; @@ -96,8 +97,6 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { return t; }); - private final Context mContext; - @GuardedBy("mStats") private final BatteryStatsImpl mStats; @@ -168,15 +167,39 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { @GuardedBy("this") private long mLastCollectionTimeStamp; + final Injector mInjector; + + @VisibleForTesting + public static class Injector { + private final Context mContext; + + Injector(Context context) { + mContext = context; + } + + public T getSystemService(Class serviceClass) { + return mContext.getSystemService(serviceClass); + } + + public T getLocalService(Class serviceClass) { + return LocalServices.getService(serviceClass); + } + } + BatteryExternalStatsWorker(Context context, BatteryStatsImpl stats) { - mContext = context; + this(new Injector(context), stats); + } + + @VisibleForTesting + BatteryExternalStatsWorker(Injector injector, BatteryStatsImpl stats) { + mInjector = injector; mStats = stats; } public void systemServicesReady() { - final WifiManager wm = mContext.getSystemService(WifiManager.class); - final TelephonyManager tm = mContext.getSystemService(TelephonyManager.class); - final PowerStatsInternal psi = LocalServices.getService(PowerStatsInternal.class); + final WifiManager wm = mInjector.getSystemService(WifiManager.class); + final TelephonyManager tm = mInjector.getSystemService(TelephonyManager.class); + final PowerStatsInternal psi = mInjector.getLocalService(PowerStatsInternal.class); synchronized (mWorkerLock) { mWifiManager = wm; mTelephony = tm; @@ -747,7 +770,8 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { * EnergyConsumerResult}[] */ @GuardedBy("mWorkerLock") - private @Nullable MeasuredEnergyArray getEnergyConsumptionData() { + @VisibleForTesting + public @Nullable MeasuredEnergyArray getEnergyConsumptionData() { final EnergyConsumerResult[] results; try { results = mPowerStatsInternal.getEnergyConsumedAsync(new int[0]) @@ -761,28 +785,41 @@ class BatteryExternalStatsWorker implements BatteryStatsImpl.ExternalStatsSync { final int[] subsystems = new int[size]; final long[] energyUJ = new long[size]; + int count = 0; for (int i = 0; i < size; i++) { final EnergyConsumerResult consumer = results[i]; final int subsystem = mEnergyConsumerToSubsystemMap.get(consumer.id, MeasuredEnergyArray.SUBSYSTEM_UNKNOWN); if (subsystem == MeasuredEnergyArray.SUBSYSTEM_UNKNOWN) continue; - subsystems[i] = subsystem; - energyUJ[i] = consumer.energyUWs; + subsystems[count] = subsystem; + energyUJ[count] = consumer.energyUWs; + count++; } + final int arraySize = count; return new MeasuredEnergyArray() { @Override public int getSubsystem(int index) { + if (index >= size()) { + throw new IllegalArgumentException( + "Out of bounds subsystem index! index : " + index + ", size : " + + size()); + } return subsystems[index]; } @Override public long getEnergy(int index) { + if (index >= size()) { + throw new IllegalArgumentException( + "Out of bounds subsystem index! index : " + index + ", size : " + + size()); + } return energyUJ[index]; } @Override public int size() { - return size; + return arraySize; } }; } diff --git a/services/tests/servicestests/src/com/android/server/am/BatteryExternalStatsWorkerTest.java b/services/tests/servicestests/src/com/android/server/am/BatteryExternalStatsWorkerTest.java new file mode 100644 index 0000000000000..e44b0836bd186 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/am/BatteryExternalStatsWorkerTest.java @@ -0,0 +1,187 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.am; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; + +import android.content.Context; +import android.hardware.power.stats.EnergyConsumer; +import android.hardware.power.stats.EnergyConsumerResult; +import android.hardware.power.stats.EnergyConsumerType; +import android.power.PowerStatsInternal; +import android.util.SparseArray; +import android.util.SparseLongArray; + +import androidx.test.InstrumentationRegistry; + +import com.android.internal.os.BatteryStatsImpl; +import com.android.internal.power.MeasuredEnergyArray; + +import org.junit.Before; +import org.junit.Test; + +import java.util.concurrent.CompletableFuture; + +/** + * Tests for {@link BatteryExternalStatsWorker}. + * + * Build/Install/Run: + * atest FrameworksServicesTests:BatteryExternalStatsWorkerTest + */ +public class BatteryExternalStatsWorkerTest { + private BatteryExternalStatsWorker mBatteryExternalStatsWorker; + private TestBatteryStatsImpl mBatteryStatsImpl; + private TestPowerStatsInternal mPowerStatsInternal; + + @Before + public void setUp() { + final Context context = InstrumentationRegistry.getContext(); + + mBatteryStatsImpl = new TestBatteryStatsImpl(); + mPowerStatsInternal = new TestPowerStatsInternal(); + mBatteryExternalStatsWorker = new BatteryExternalStatsWorker(new TestInjector(context), + mBatteryStatsImpl); + } + + @Test + public void getEnergyConsumptionData() { + SparseLongArray expectSubsystems = new SparseLongArray(); + // Add some energy consumers used by BatteryExternalStatsWorker. + final int displayId = mPowerStatsInternal.addEnergyConsumer(EnergyConsumerType.DISPLAY, 0, + "display"); + mPowerStatsInternal.incrementEnergyConsumption(displayId, 12345); + expectSubsystems.put(MeasuredEnergyArray.SUBSYSTEM_DISPLAY, 12345); + + // Add an arbitrary energy consumer unused by BatteryExternalStatsWorker. + // Must be changed if '154' ever becomes an EnergyConsumerType used by BESW. + final int someId = mPowerStatsInternal.addEnergyConsumer((byte) 154, 0, "some_consumer"); + mPowerStatsInternal.incrementEnergyConsumption(someId, 34567); + + // Inform BESW that PowerStatsInternal is ready to query + mBatteryExternalStatsWorker.systemServicesReady(); + + MeasuredEnergyArray energies = mBatteryExternalStatsWorker.getEnergyConsumptionData(); + + assertEquals(expectSubsystems.size(), energies.size()); + final int size = expectSubsystems.size(); + + for (int i = 0; i < size; i++) { + int subsystem = expectSubsystems.keyAt(i); + // find the subsystem in the returned MeasuredEnergyArray + int subsystemIndex = -1; + for (int j = 0; j < size; j++) { + if (subsystem == energies.getSubsystem(i)) { + subsystemIndex = i; + break; + } + } + assertNotEquals("Subsystem " + subsystem + " not found in MeasuredEnergyArray", -1, + subsystemIndex); + assertEquals(expectSubsystems.valueAt(i), energies.getEnergy(subsystemIndex)); + } + } + + public class TestInjector extends BatteryExternalStatsWorker.Injector { + public TestInjector(Context context) { + super(context); + } + + public T getSystemService(Class serviceClass) { + return null; + } + + public T getLocalService(Class serviceClass) { + if (serviceClass == PowerStatsInternal.class) { + return (T) mPowerStatsInternal; + } + return null; + } + } + + public class TestBatteryStatsImpl extends BatteryStatsImpl { + } + + public class TestPowerStatsInternal extends PowerStatsInternal { + private final SparseArray mEnergyConsumers = new SparseArray<>(); + private final SparseArray mEnergyConsumerResults = + new SparseArray<>(); + private final int mTimeSinceBoot = 0; + + @Override + public EnergyConsumer[] getEnergyConsumerInfo() { + final int size = mEnergyConsumers.size(); + final EnergyConsumer[] consumers = new EnergyConsumer[size]; + for (int i = 0; i < size; i++) { + consumers[i] = mEnergyConsumers.valueAt(i); + } + return consumers; + } + + @Override + public CompletableFuture getEnergyConsumedAsync( + int[] energyConsumerIds) { + final CompletableFuture future = new CompletableFuture(); + final int size = mEnergyConsumerResults.size(); + final EnergyConsumerResult[] results = new EnergyConsumerResult[size]; + for (int i = 0; i < size; i++) { + results[i] = mEnergyConsumerResults.valueAt(i); + } + future.complete(results); + return future; + } + + /** + * Util method to add a new EnergyConsumer for testing + * + * @return the EnergyConsumer id of the new EnergyConsumer + */ + public int addEnergyConsumer(@EnergyConsumerType byte type, int ordinal, String name) { + final EnergyConsumer consumer = new EnergyConsumer(); + final int id = getNextAvailableId(); + consumer.id = id; + consumer.type = type; + consumer.ordinal = ordinal; + consumer.name = name; + mEnergyConsumers.put(id, consumer); + + final EnergyConsumerResult result = new EnergyConsumerResult(); + result.id = id; + result.timestampMs = mTimeSinceBoot; + result.energyUWs = 0; + mEnergyConsumerResults.put(id, result); + return id; + } + + public void incrementEnergyConsumption(int id, long energyUWs) { + EnergyConsumerResult result = mEnergyConsumerResults.get(id, null); + assertNotNull(result); + result.energyUWs += energyUWs; + } + + private int getNextAvailableId() { + final int size = mEnergyConsumers.size(); + // Just return the first index that does not match the key (aka the EnergyConsumer id) + for (int i = size - 1; i >= 0; i--) { + if (mEnergyConsumers.keyAt(i) == i) return i + 1; + } + // Otherwise return the lowest id + return 0; + } + } +}