commit 4b238fb0a9bd5533651030faf22391675fc88024
author Dmitri Plotnikov <dplotnikov@google.com> Tue Nov 09 11:52:08 2021 -0800
committer Dmitri Plotnikov <dplotnikov@google.com> Thu Nov 11 10:07:58 2021 -0800
tree eb703c24618acfe00a8039ccde3ad7638985ef59
parent 7804d910bf010c06e5d38bca68230defae06bc07

Reset time-since-update if the tracked value is nonmonotonic

The absence of this reset led to timeInStateSinceUpdate exceeding
total timeSinceUpdate.  This resulted in inflated counts,
which are smeared using the `delta * timeInState / timeSinceUpdate` formula.
So, instead of assigning a portion of the time to a specific state
it would assign a multiple of the time to it.

Bug: 204087731
Test: atest libbattery_test

Change-Id: I7805d5c6aa314f4030c8a1ac9541f2d439a471cb

libs/battery/MultiStateCounter.h

diff --git a/libs/battery/MultiStateCounter.h b/libs/battery/MultiStateCounter.h
index 03e1f2a..a2b59a8 100644
--- a/libs/battery/MultiStateCounter.h
+++ b/libs/battery/MultiStateCounter.h
@@ -195,10 +195,18 @@
                         << ", which is lower than the previous value " << valueToString(lastValue)
                         << "\n";
                     ALOGE("%s", str.str().c_str());
+
+                    for (int i = 0; i < stateCount; i++) {
+                        states[i].timeInStateSinceUpdate = 0;
+                    }
                 }
             } else if (timestamp < lastUpdateTimestamp) {
                 ALOGE("updateValue is called with an earlier timestamp: %lu, previous: %lu\n",
                       (unsigned long)timestamp, (unsigned long)lastUpdateTimestamp);
+
+                for (int i = 0; i < stateCount; i++) {
+                    states[i].timeInStateSinceUpdate = 0;
+                }
             }
         }
     }

libs/battery/MultiStateCounterTest.cpp

diff --git a/libs/battery/MultiStateCounterTest.cpp b/libs/battery/MultiStateCounterTest.cpp
index 848fd10..876bf74 100644
--- a/libs/battery/MultiStateCounterTest.cpp
+++ b/libs/battery/MultiStateCounterTest.cpp
@@ -176,7 +176,7 @@
     testCounter.setState(0, 0);
     testCounter.updateValue(6.0, 2000);
 
-    // Time moves back. The negative delta from 2000 to 1000 is ignored
+    // Time moves back. The delta over the negative interval from 2000 to 1000 is ignored
     testCounter.updateValue(8.0, 1000);
     double delta = testCounter.updateValue(11.0, 3000);
 
@@ -189,6 +189,27 @@
     EXPECT_DOUBLE_EQ(3.0, delta);
 }
 
+TEST_F(MultiStateCounterTest, updateValue_nonmonotonic) {
+    DoubleMultiStateCounter testCounter(2, 0);
+    testCounter.updateValue(0, 0);
+    testCounter.setState(0, 0);
+    testCounter.updateValue(6.0, 2000);
+
+    // Value goes down. The negative delta from 6.0 to 4.0 is ignored
+    testCounter.updateValue(4.0, 3000);
+
+    // Value goes up again. The positive delta from 4.0 to 7.0 is accumulated.
+    double delta = testCounter.updateValue(7.0, 4000);
+
+    // The total accumulated count is:
+    //  6.0          // For the period 0-2000
+    //  +(7.0-4.0)   // For the period 3000-4000
+    EXPECT_DOUBLE_EQ(9.0, testCounter.getCount(0));
+
+    //  7.0-4.0
+    EXPECT_DOUBLE_EQ(3.0, delta);
+}
+
 TEST_F(MultiStateCounterTest, addValue) {
     DoubleMultiStateCounter testCounter(1, 0);
     testCounter.updateValue(0, 0);