Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8c6f3c50 authored by Bookatz's avatar Bookatz
Browse files

Fix batterystat Counter misreporting when charging 

The BatteryStatsImpl.Counter would previously increment the count even
when the timeBase was off. Then, when the timeBase was turned back on,
the count would be decreased back to the correct value. Thus, when the
timeBase was on, the reported count would be correct, but when the
timeBase was off, the reported count would be wrong (too high). Here, we
fix this.
We also make some other minor improvements.

Bug: 36728346
Test: runtest -x frameworks/base/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java
Change-Id: I2fa566a8a4cad4cdff0e6caef37b1eef36a3f5c4
parent 61e2b158

core/java/com/android/internal/os/BatteryStatsImpl.java

+31 −24
Original line number Original line Diff line number Diff line
@@ -862,21 +862,19 @@ public class BatteryStatsImpl extends BatteryStats { 
        final AtomicInteger mCount = new AtomicInteger();

        final AtomicInteger mCount = new AtomicInteger();

        final TimeBase mTimeBase;

        final TimeBase mTimeBase;

        int mLoadedCount;

        int mLoadedCount;

        int mLastCount;

        int mUnpluggedCount;

        int mUnpluggedCount;

        int mPluggedCount;

        int mPluggedCount;





        Counter(TimeBase timeBase, Parcel in) {

        public Counter(TimeBase timeBase, Parcel in) {

            mTimeBase = timeBase;

            mTimeBase = timeBase;

            mPluggedCount = in.readInt();

            mPluggedCount = in.readInt();

            mCount.set(mPluggedCount);

            mCount.set(mPluggedCount);

            mLoadedCount = in.readInt();

            mLoadedCount = in.readInt();

            mLastCount = 0;

            mUnpluggedCount = in.readInt();

            mUnpluggedCount = in.readInt();

            timeBase.add(this);

            timeBase.add(this);

        }

        }





        Counter(TimeBase timeBase) {

        public Counter(TimeBase timeBase) {

            mTimeBase = timeBase;

            mTimeBase = timeBase;

            timeBase.add(this);

            timeBase.add(this);

        }

        }
@@ -887,11 +885,12 @@ public class BatteryStatsImpl extends BatteryStats { 
            out.writeInt(mUnpluggedCount);

            out.writeInt(mUnpluggedCount);

        }

        }





        @Override

        public void onTimeStarted(long elapsedRealtime, long baseUptime, long baseRealtime) {

        public void onTimeStarted(long elapsedRealtime, long baseUptime, long baseRealtime) {

            mUnpluggedCount = mPluggedCount;

            mUnpluggedCount = mPluggedCount;

            mCount.set(mPluggedCount);

        }

        }





        @Override

        public void onTimeStopped(long elapsedRealtime, long baseUptime, long baseRealtime) {

        public void onTimeStopped(long elapsedRealtime, long baseUptime, long baseRealtime) {

            mPluggedCount = mCount.get();

            mPluggedCount = mCount.get();

        }

        }
@@ -926,25 +925,30 @@ public class BatteryStatsImpl extends BatteryStats { 




        public void logState(Printer pw, String prefix) {

        public void logState(Printer pw, String prefix) {

            pw.println(prefix + "mCount=" + mCount.get()

            pw.println(prefix + "mCount=" + mCount.get()

                    + " mLoadedCount=" + mLoadedCount + " mLastCount=" + mLastCount

                    + " mLoadedCount=" + mLoadedCount

                    + " mUnpluggedCount=" + mUnpluggedCount

                    + " mUnpluggedCount=" + mUnpluggedCount

                    + " mPluggedCount=" + mPluggedCount);

                    + " mPluggedCount=" + mPluggedCount);

        }

        }





        void stepAtomic() {

        @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE)

        public void stepAtomic() {

            if (mTimeBase.isRunning()) {

                mCount.incrementAndGet();

                mCount.incrementAndGet();

            }

            }

        }





        void addAtomic(int delta) {

        void addAtomic(int delta) {

            if (mTimeBase.isRunning()) {

                mCount.addAndGet(delta);

                mCount.addAndGet(delta);

            }

            }

        }





        /**

        /**

         * Clear state of this counter.

         * Clear state of this counter.

         */

         */

        void reset(boolean detachIfReset) {

        void reset(boolean detachIfReset) {

            mCount.set(0);

            mCount.set(0);

            mLoadedCount = mLastCount = mPluggedCount = mUnpluggedCount = 0;

            mLoadedCount = mPluggedCount = mUnpluggedCount = 0;

            if (detachIfReset) {

            if (detachIfReset) {

                detach();

                detach();

            }

            }
@@ -954,15 +958,16 @@ public class BatteryStatsImpl extends BatteryStats { 
            mTimeBase.remove(this);

            mTimeBase.remove(this);

        }

        }





        void writeSummaryFromParcelLocked(Parcel out) {

        @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE)

        public void writeSummaryFromParcelLocked(Parcel out) {

            int count = mCount.get();

            int count = mCount.get();

            out.writeInt(count);

            out.writeInt(count);

        }

        }





        void readSummaryFromParcelLocked(Parcel in) {

        @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE)

        public void readSummaryFromParcelLocked(Parcel in) {

            mLoadedCount = in.readInt();

            mLoadedCount = in.readInt();

            mCount.set(mLoadedCount);

            mCount.set(mLoadedCount);

            mLastCount = 0;

            mUnpluggedCount = mPluggedCount = mLoadedCount;

            mUnpluggedCount = mPluggedCount = mLoadedCount;

        }

        }

    }

    }
@@ -998,7 +1003,6 @@ public class BatteryStatsImpl extends BatteryStats { 
        @Override

        @Override

        public void onTimeStarted(long elapsedRealTime, long baseUptime, long baseRealtime) {

        public void onTimeStarted(long elapsedRealTime, long baseUptime, long baseRealtime) {

            mUnpluggedCounts = copyArray(mPluggedCounts, mUnpluggedCounts);

            mUnpluggedCounts = copyArray(mPluggedCounts, mUnpluggedCounts);

            mCounts = copyArray(mPluggedCounts, mCounts);

        }

        }





        @Override

        @Override
@@ -1029,6 +1033,7 @@ public class BatteryStatsImpl extends BatteryStats { 
            if (counts == null) {

            if (counts == null) {

                return;

                return;

            }

            }

            if (mTimeBase.isRunning()) {

                if (mCounts == null) {

                if (mCounts == null) {

                    mCounts = new long[counts.length];

                    mCounts = new long[counts.length];

                }

                }
@@ -1036,6 +1041,7 @@ public class BatteryStatsImpl extends BatteryStats { 
                    mCounts[i] += counts[i];

                    mCounts[i] += counts[i];

                }

                }

            }

            }

        }





        /**

        /**

         * Clear state of this counter.

         * Clear state of this counter.
@@ -1104,13 +1110,13 @@ public class BatteryStatsImpl extends BatteryStats { 
            }

            }

        }

        }





        private void fillArray(long[] a, long val) {

        private static void fillArray(long[] a, long val) {

            if (a != null) {

            if (a != null) {

                Arrays.fill(a, val);

                Arrays.fill(a, val);

            }

            }

        }

        }





        private void subtract(@NonNull long[] val, long[] toSubtract) {

        private static void subtract(@NonNull long[] val, long[] toSubtract) {

            if (toSubtract == null) {

            if (toSubtract == null) {

                return;

                return;

            }

            }
@@ -1119,7 +1125,7 @@ public class BatteryStatsImpl extends BatteryStats { 
            }

            }

        }

        }





        private long[] copyArray(long[] src, long[] dest) {

        private static long[] copyArray(long[] src, long[] dest) {

            if (src == null) {

            if (src == null) {

                return null;

                return null;

            } else {

            } else {
@@ -1162,7 +1168,6 @@ public class BatteryStatsImpl extends BatteryStats { 
        @Override

        @Override

        public void onTimeStarted(long elapsedRealtime, long baseUptime, long baseRealtime) {

        public void onTimeStarted(long elapsedRealtime, long baseUptime, long baseRealtime) {

            mUnpluggedCount = mPluggedCount;

            mUnpluggedCount = mPluggedCount;

            mCount = mPluggedCount;

        }

        }





        @Override

        @Override
@@ -1189,8 +1194,10 @@ public class BatteryStatsImpl extends BatteryStats { 
        }

        }





        void addCountLocked(long count) {

        void addCountLocked(long count) {

            if (mTimeBase.isRunning()) {

                mCount += count;

                mCount += count;

            }

            }

        }





        /**

        /**

         * Clear state of this counter.

         * Clear state of this counter.

core/tests/coretests/src/com/android/internal/os/BatteryStatsCounterTest.java

0 → 100644
+148 −0
Original line number Original line Diff line number Diff line 
/*

 * Copyright (C) 2017 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.internal.os;



import android.os.BatteryStats;

import android.os.Parcel;

import android.support.test.filters.SmallTest;



import junit.framework.TestCase;



/**

 * Test BatteryStatsImpl.Counter.

 */

public class BatteryStatsCounterTest extends TestCase {



    @SmallTest

    public void testCounter() throws Exception {

        final MockClocks clocks = new MockClocks(); // holds realtime and uptime in ms

        final BatteryStatsImpl.TimeBase timeBase = new BatteryStatsImpl.TimeBase();

        timeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime());



        final BatteryStatsImpl.Counter counter = new BatteryStatsImpl.Counter(timeBase);



        // timeBase off (i.e. plugged in)

        timeBase.setRunning(false, 1, 1);

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        assertEquals(0, counter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(0, counter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(0, counter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase on (i.e. unplugged)

        timeBase.setRunning(true, 2, 2);

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase off (i.e. plugged in)

        timeBase.setRunning(false, 3, 3);

        counter.stepAtomic();

        counter.stepAtomic();

        counter.stepAtomic();

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(4, counter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase on (i.e. unplugged)

        timeBase.setRunning(true, 4, 4);

        counter.stepAtomic();

        counter.stepAtomic();

        assertEquals(6, counter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(6, counter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(2, counter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));

    }





    @SmallTest

    public void testParceling() throws Exception {

        final MockClocks clocks = new MockClocks(); // holds realtime and uptime in ms

        final BatteryStatsImpl.TimeBase timeBase = new BatteryStatsImpl.TimeBase();

        timeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime());



        final BatteryStatsImpl.Counter origCounter = new BatteryStatsImpl.Counter(timeBase);



        // timeBase on (i.e. unplugged)

        timeBase.setRunning(true, 1, 1);

        origCounter.stepAtomic(); origCounter.stepAtomic(); origCounter.stepAtomic(); // three times

        assertEquals(3, origCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(3, origCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(3, origCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // Test summary parcelling (from origCounter)

        final Parcel summaryParcel = Parcel.obtain();

        origCounter.writeSummaryFromParcelLocked(summaryParcel);

        summaryParcel.setDataPosition(0);

        final BatteryStatsImpl.Counter summaryCounter = new BatteryStatsImpl.Counter(timeBase);

        summaryCounter.stepAtomic(); // occurs before reading the summary, so must not count later

        summaryCounter.readSummaryFromParcelLocked(summaryParcel);



        // timeBase still on (i.e. unplugged)

        summaryCounter.stepAtomic(); // once

        assertEquals(4, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(1, summaryCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(1, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase off (i.e. plugged in)

        timeBase.setRunning(false, 3, 3);

        summaryCounter.stepAtomic(); summaryCounter.stepAtomic(); // twice

        assertEquals(4, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(1, summaryCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(1, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase on (i.e. unplugged)

        timeBase.setRunning(true, 4, 4);

        summaryCounter.stepAtomic(); summaryCounter.stepAtomic(); // twice

        assertEquals(6, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(3, summaryCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(2, summaryCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));





        // Test full parcelling (from summaryCounter)

        final Parcel fullParcel = Parcel.obtain();

        summaryCounter.writeToParcel(fullParcel);

        fullParcel.setDataPosition(0);

        final BatteryStatsImpl.Counter fullParcelCounter

                = new BatteryStatsImpl.Counter(timeBase, fullParcel);



        // timeBase still on (i.e. unplugged)

        fullParcelCounter.stepAtomic(); // once

        assertEquals(7, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(4, fullParcelCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(3, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase off (i.e. plugged in)

        timeBase.setRunning(false, 5, 5);

        fullParcelCounter.stepAtomic(); fullParcelCounter.stepAtomic(); // twice

        assertEquals(7, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(4, fullParcelCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(3, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));



        // timeBase on (i.e. unplugged)

        timeBase.setRunning(true, 6, 6);

        fullParcelCounter.stepAtomic(); fullParcelCounter.stepAtomic(); // twice

        assertEquals(9, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_CHARGED));

        assertEquals(6, fullParcelCounter.getCountLocked(BatteryStats.STATS_CURRENT));

        assertEquals(2, fullParcelCounter.getCountLocked(BatteryStats.STATS_SINCE_UNPLUGGED));

    }

}

core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java

+4 −3
Original line number Original line Diff line number Diff line
@@ -5,17 +5,18 @@ import org.junit.runners.Suite; 




@RunWith(Suite.class)

@RunWith(Suite.class)

@Suite.SuiteClasses({

@Suite.SuiteClasses({

        BatteryStatsBackgroundStatsTest.class,

        BatteryStatsCounterTest.class,

        BatteryStatsDualTimerTest.class,

        BatteryStatsDualTimerTest.class,

        BatteryStatsDurationTimerTest.class,

        BatteryStatsDurationTimerTest.class,

        BatteryStatsNoteTest.class,

        BatteryStatsSamplingTimerTest.class,

        BatteryStatsSamplingTimerTest.class,

        BatteryStatsSensorTest.class,

        BatteryStatsServTest.class,

        BatteryStatsServTest.class,

        BatteryStatsStopwatchTimerTest.class,

        BatteryStatsStopwatchTimerTest.class,

        BatteryStatsTimeBaseTest.class,

        BatteryStatsTimeBaseTest.class,

        BatteryStatsTimerTest.class,

        BatteryStatsTimerTest.class,

        BatteryStatsUidTest.class,

        BatteryStatsUidTest.class,

        BatteryStatsSensorTest.class,

        BatteryStatsBackgroundStatsTest.class,

        BatteryStatsNoteTest.class,

    })

    })

public class BatteryStatsTests {

public class BatteryStatsTests {

}

}