From c6bd4243a46acc85058fcad2eb6957c42f591006 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Fri, 9 Dec 2016 19:04:50 -0800 Subject: Fixing issue in current and max duration calculations. It looks like one operation was done out of order and some of the times used in the calculations were leading to incorrect results. BUG: 31023263 Test: bit FrameworksCoreTests:com.android.internal.os.BatteryStatsDurationTimerTest Change-Id: I417cc28c5a55748067b6c7f682a66fe3dbc09f09 (cherry picked from commit 47db5a8bf74a77306b811d14e3c052cdf86ef704) --- core/java/android/os/BatteryStats.java | 2 +- .../com/android/internal/os/BatteryStatsImpl.java | 31 +++++++++++----------- .../internal/os/BatteryStatsDurationTimerTest.java | 21 +++++++-------- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/core/java/android/os/BatteryStats.java b/core/java/android/os/BatteryStats.java index a0c2efd407ba..0bb05b5a22a2 100644 --- a/core/java/android/os/BatteryStats.java +++ b/core/java/android/os/BatteryStats.java @@ -182,7 +182,7 @@ public abstract class BatteryStats implements Parcelable { * New in version 19: * - Wakelock data (wl) gets current and max times. */ - static final String CHECKIN_VERSION = "19"; + static final String CHECKIN_VERSION = "20"; /** * Old version, we hit 9 and ran out of room, need to remove. diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index 3b3344e7d934..38e0d5bd0f9c 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -108,7 +108,7 @@ public class BatteryStatsImpl extends BatteryStats { private static final int MAGIC = 0xBA757475; // 'BATSTATS' // Current on-disk Parcel version - private static final int VERSION = 150 + (USE_OLD_HISTORY ? 1000 : 0); + private static final int VERSION = 151 + (USE_OLD_HISTORY ? 1000 : 0); // Maximum number of items we will record in the history. private static final int MAX_HISTORY_ITEMS = 2000; @@ -1593,7 +1593,7 @@ public class BatteryStatsImpl extends BatteryStats { @Override public void writeToParcel(Parcel out, long elapsedRealtimeUs) { super.writeToParcel(out, elapsedRealtimeUs); - out.writeLong(mMaxDurationMs); + out.writeLong(getMaxDurationMsLocked(elapsedRealtimeUs / 1000)); } /** @@ -1606,7 +1606,7 @@ public class BatteryStatsImpl extends BatteryStats { @Override public void writeSummaryFromParcelLocked(Parcel out, long elapsedRealtimeUs) { super.writeSummaryFromParcelLocked(out, elapsedRealtimeUs); - out.writeLong(mMaxDurationMs); + out.writeLong(getMaxDurationMsLocked(elapsedRealtimeUs / 1000)); } /** @@ -1630,7 +1630,7 @@ public class BatteryStatsImpl extends BatteryStats { public void onTimeStarted(long elapsedRealtimeUs, long baseUptime, long baseRealtime) { super.onTimeStarted(elapsedRealtimeUs, baseUptime, baseRealtime); if (mNesting > 0) { - mStartTimeMs = mTimeBase.getRealtime(mClocks.elapsedRealtime()*1000) / 1000; + mStartTimeMs = baseRealtime / 1000; } } @@ -1640,10 +1640,11 @@ public class BatteryStatsImpl extends BatteryStats { * If the timer is running, add the duration into mCurrentDurationMs. */ @Override - public void onTimeStopped(long elapsedRealtimeUs, long baseUptime, long baseRealtime) { - super.onTimeStopped(elapsedRealtimeUs, baseUptime, baseRealtime); + public void onTimeStopped(long elapsedRealtimeUs, long baseUptime, long baseRealtimeUs) { + super.onTimeStopped(elapsedRealtimeUs, baseUptime, baseRealtimeUs); if (mNesting > 0) { - mCurrentDurationMs += (elapsedRealtimeUs / 1000) - mStartTimeMs; + // baseRealtimeUs has already been converted to the timebase's realtime. + mCurrentDurationMs += (baseRealtimeUs / 1000) - mStartTimeMs; } mStartTimeMs = -1; } @@ -1658,7 +1659,7 @@ public class BatteryStatsImpl extends BatteryStats { super.startRunningLocked(elapsedRealtimeMs); if (mNesting == 1 && mTimeBase.isRunning()) { // Just started - mStartTimeMs = mTimeBase.getRealtime(mClocks.elapsedRealtime()*1000) / 1000; + mStartTimeMs = mTimeBase.getRealtime(elapsedRealtimeMs * 1000) / 1000; } } @@ -1670,8 +1671,7 @@ public class BatteryStatsImpl extends BatteryStats { */ @Override public void stopRunningLocked(long elapsedRealtimeMs) { - super.stopRunningLocked(elapsedRealtimeMs); - if (mNesting == 0) { + if (mNesting == 1) { final long durationMs = getCurrentDurationMsLocked(elapsedRealtimeMs); if (durationMs > mMaxDurationMs) { mMaxDurationMs = durationMs; @@ -1679,6 +1679,9 @@ public class BatteryStatsImpl extends BatteryStats { mStartTimeMs = -1; mCurrentDurationMs = 0; } + // super method decrements mNesting, which getCurrentDurationMsLocked relies on, + // so call super.stopRunningLocked after calling getCurrentDurationMsLocked. + super.stopRunningLocked(elapsedRealtimeMs); } @Override @@ -1720,11 +1723,9 @@ public class BatteryStatsImpl extends BatteryStats { @Override public long getCurrentDurationMsLocked(long elapsedRealtimeMs) { long durationMs = mCurrentDurationMs; - if (mNesting > 0) { - if (mTimeBase.isRunning()) { - durationMs += (mTimeBase.getRealtime(elapsedRealtimeMs*1000)/1000) - - mStartTimeMs; - } + if (mNesting > 0 && mTimeBase.isRunning()) { + durationMs += (mTimeBase.getRealtime(elapsedRealtimeMs*1000)/1000) + - mStartTimeMs; } return durationMs; } diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsDurationTimerTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsDurationTimerTest.java index a15e3679a181..f1aeecc46265 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsDurationTimerTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsDurationTimerTest.java @@ -18,12 +18,9 @@ package com.android.internal.os; import android.os.BatteryStats; import android.os.Parcel; import android.support.test.filters.SmallTest; -import android.util.Log; import junit.framework.TestCase; -import org.mockito.Mockito; - /** * Test BatteryStatsImpl.DurationTimer. * @@ -39,7 +36,7 @@ public class BatteryStatsDurationTimerTest extends TestCase { final BatteryStatsImpl.TimeBase timeBase = new BatteryStatsImpl.TimeBase(); timeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime()); - final BatteryStatsImpl.DurationTimer timer = new BatteryStatsImpl.DurationTimer(clocks, + final BatteryStatsImpl.DurationTimer timer = new BatteryStatsImpl.DurationTimer(clocks, null, BatteryStats.WAKE_TYPE_PARTIAL, null, timeBase); // TimeBase running, timer not running: current and max are 0 @@ -82,15 +79,15 @@ public class BatteryStatsDurationTimerTest extends TestCase { // Stop the TimeBase. The values should be frozen. timeBase.setRunning(false, /* uptimeUs */ 10, /* realtimeUs */ 55000*1000); assertTrue(timer.isRunningLocked()); - assertEquals(28100, timer.getCurrentDurationMsLocked(110100)); // Why 28100 and not 28000? - assertEquals(28100, timer.getMaxDurationMsLocked(110101)); + assertEquals(28000, timer.getCurrentDurationMsLocked(110100)); + assertEquals(28000, timer.getMaxDurationMsLocked(110101)); // Start the TimeBase. The values should be the old value plus the delta // between when the timer restarted and the current time timeBase.setRunning(true, /* uptimeUs */ 10, /* realtimeUs */ 220100*1000); assertTrue(timer.isRunningLocked()); - assertEquals(28300, timer.getCurrentDurationMsLocked(220300)); // extra 100 from above?? - assertEquals(28301, timer.getMaxDurationMsLocked(220301)); + assertEquals(28200, timer.getCurrentDurationMsLocked(220300)); + assertEquals(28201, timer.getMaxDurationMsLocked(220301)); } @SmallTest @@ -104,7 +101,7 @@ public class BatteryStatsDurationTimerTest extends TestCase { final BatteryStatsImpl.TimeBase timeBase = new BatteryStatsImpl.TimeBase(); timeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime()); - final BatteryStatsImpl.DurationTimer timer = new BatteryStatsImpl.DurationTimer(clocks, + final BatteryStatsImpl.DurationTimer timer = new BatteryStatsImpl.DurationTimer(clocks, null, BatteryStats.WAKE_TYPE_PARTIAL, null, timeBase); // Start running on battery. @@ -124,7 +121,7 @@ public class BatteryStatsDurationTimerTest extends TestCase { summaryParcel.setDataPosition(0); // Read summary - final BatteryStatsImpl.DurationTimer summary = new BatteryStatsImpl.DurationTimer(clocks, + final BatteryStatsImpl.DurationTimer summary = new BatteryStatsImpl.DurationTimer(clocks, null, BatteryStats.WAKE_TYPE_PARTIAL, null, timeBase); summary.startRunningLocked(3100); summary.readSummaryFromParcelLocked(summaryParcel); @@ -138,9 +135,9 @@ public class BatteryStatsDurationTimerTest extends TestCase { final Parcel fullParcel = Parcel.obtain(); timer.writeToParcel(fullParcel, 1500*1000); fullParcel.setDataPosition(0); - + // Read full - Should be the same as the summary as far as DurationTimer is concerned. - final BatteryStatsImpl.DurationTimer full = new BatteryStatsImpl.DurationTimer(clocks, + final BatteryStatsImpl.DurationTimer full = new BatteryStatsImpl.DurationTimer(clocks, null, BatteryStats.WAKE_TYPE_PARTIAL, null, timeBase, fullParcel); // The new one shouldn't be running, and therefore 0 for current time assertFalse(full.isRunningLocked()); -- cgit v1.2.3-59-g8ed1b