Add unit tests for SystemTimer.h (#12742)

#### Problem Only the higher-level `System::Layer` timer operations had tests; the utility classes in `system/SystemTimer.h` had no unit tests, and a recent refactor (#12628) introduced a bug that a proper unit test would have caught. Fixes #12729 Add unit tests for SystemTimer.h #### Change overview What's in this PR - Add tests covering `TimerData`, `TimeList`, and `TimerPool`. - Changed these helpers to take a `Timestamp` rather than a `Timeout`. - Fixed `TimerList::Remove(Node*)` to allow an empty list or null argument (matching its description). #### Testing Quis custodiet ipsos custodes?
project-chip · Jan 17, 2022 · 1314104 · 1314104
1 parent 45d2c33
commit 1314104
Show file tree

Hide file tree

Showing 5 changed files with 185 additions and 29 deletions.
diff --git a/src/system/SystemLayerImplLwIP.cpp b/src/system/SystemLayerImplLwIP.cpp
@@ -56,7 +56,7 @@ CHIP_ERROR LayerImplLwIP::StartTimer(Clock::Timeout delay, TimerCompleteCallback
 
     CancelTimer(onComplete, appState);
 
-    TimerList::Node * timer = mTimerPool.Create(*this, delay, onComplete, appState);
+    TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp() + delay, onComplete, appState);
     VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY);
 
     if (mTimerList.Add(timer) == timer)
@@ -87,7 +87,7 @@ CHIP_ERROR LayerImplLwIP::ScheduleWork(TimerCompleteCallback onComplete, void *
 {
     VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
 
-    TimerList::Node * timer = mTimerPool.Create(*this, System::Clock::kZero, onComplete, appState);
+    TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState);
     VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY);
 
     return ScheduleLambda([this, timer] { this->mTimerPool.Invoke(timer); });

diff --git a/src/system/SystemLayerImplSelect.cpp b/src/system/SystemLayerImplSelect.cpp
@@ -124,7 +124,7 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba
 
     CancelTimer(onComplete, appState);
 
-    TimerList::Node * timer = mTimerPool.Create(*this, delay, onComplete, appState);
+    TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp() + delay, onComplete, appState);
     VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY);
 
 #if CHIP_SYSTEM_CONFIG_USE_DISPATCH
@@ -186,7 +186,7 @@ CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void
 
     CancelTimer(onComplete, appState);
 
-    TimerList::Node * timer = mTimerPool.Create(*this, Clock::kZero, onComplete, appState);
+    TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState);
     VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY);
 
 #if CHIP_SYSTEM_CONFIG_USE_DISPATCH

diff --git a/src/system/SystemTimer.cpp b/src/system/SystemTimer.cpp
@@ -67,29 +67,30 @@ TimerList::Node * TimerList::Add(TimerList::Node * add)
 
 TimerList::Node * TimerList::Remove(TimerList::Node * remove)
 {
-    VerifyOrDie(mEarliestTimer != nullptr);
-
-    if (remove == mEarliestTimer)
-    {
-        mEarliestTimer = remove->mNextTimer;
-    }
-    else
+    if (mEarliestTimer != nullptr && remove != nullptr)
     {
-        TimerList::Node * lTimer = mEarliestTimer;
-
-        while (lTimer->mNextTimer)
+        if (remove == mEarliestTimer)
+        {
+            mEarliestTimer = remove->mNextTimer;
+        }
+        else
         {
-            if (remove == lTimer->mNextTimer)
+            TimerList::Node * lTimer = mEarliestTimer;
+
+            while (lTimer->mNextTimer)
             {
-                lTimer->mNextTimer = remove->mNextTimer;
-                break;
-            }
+                if (remove == lTimer->mNextTimer)
+                {
+                    lTimer->mNextTimer = remove->mNextTimer;
+                    break;
+                }
 
-            lTimer = lTimer->mNextTimer;
+                lTimer = lTimer->mNextTimer;
+            }
         }
-    }
 
-    remove->mNextTimer = nullptr;
+        remove->mNextTimer = nullptr;
+    }
     return mEarliestTimer;
 }
 

diff --git a/src/system/SystemTimer.h b/src/system/SystemTimer.h
@@ -44,6 +44,7 @@ namespace chip {
 namespace System {
 
 class Layer;
+class TestTimer;
 
 /**
  * Basic Timer information: time and callback.
@@ -57,7 +58,7 @@ class DLL_EXPORT TimerData
         Callback(Layer & systemLayer, TimerCompleteCallback onComplete, void * appState) :
             mSystemLayer(&systemLayer), mOnComplete(onComplete), mAppState(appState)
         {}
-        void Invoke() { mOnComplete(mSystemLayer, mAppState); }
+        void Invoke() const { mOnComplete(mSystemLayer, mAppState); }
         const TimerCompleteCallback & GetOnComplete() const { return mOnComplete; }
         void * GetAppState() const { return mAppState; }
         Layer * GetSystemLayer() const { return mSystemLayer; }
@@ -68,8 +69,8 @@ class DLL_EXPORT TimerData
         void * mAppState;
     };
 
-    TimerData(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) :
-        mAwakenTime(SystemClock().GetMonotonicTimestamp() + delay), mCallback(systemLayer, onComplete, appState)
+    TimerData(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState) :
+        mAwakenTime(awakenTime), mCallback(systemLayer, onComplete, appState)
     {}
     ~TimerData() = default;
 
@@ -106,8 +107,8 @@ class TimerList
     class Node : public TimerData
     {
     public:
-        Node(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) :
-            TimerData(systemLayer, delay, onComplete, appState), mNextTimer(nullptr)
+        Node(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState) :
+            TimerData(systemLayer, awakenTime, onComplete, appState), mNextTimer(nullptr)
         {}
         Node * mNextTimer;
     };
@@ -160,7 +161,7 @@ class TimerList
     /**
      * Test whether there are any timers.
      */
-    bool Empty() const { return mEarliestTimer != nullptr; }
+    bool Empty() const { return mEarliestTimer == nullptr; }
 
     /**
      * Remove and return all timers that expire before the given time @a t.
@@ -188,9 +189,9 @@ class TimerPool
     /**
      * Create a new timer from the pool.
      */
-    Timer * Create(Layer & systemLayer, System::Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState)
+    Timer * Create(Layer & systemLayer, System::Clock::Timestamp awakenTime, TimerCompleteCallback onComplete, void * appState)
     {
-        Timer * timer = mTimerPool.CreateObject(systemLayer, delay, onComplete, appState);
+        Timer * timer = mTimerPool.CreateObject(systemLayer, awakenTime, onComplete, appState);
         SYSTEM_STATS_INCREMENT(Stats::kSystemLayer_NumTimers);
         return timer;
     }
@@ -224,6 +225,7 @@ class TimerPool
     }
 
 private:
+    friend class TestTimer;
     ObjectPool<Timer, CHIP_SYSTEM_CONFIG_NUM_TIMERS> mTimerPool;
 };
 

diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp
@@ -168,6 +168,158 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext)
     ServiceEvents(lSys);
 }
 
+// Test the implementation helper classes TimerPool, TimerList, and TimerData.
+namespace chip {
+namespace System {
+class TestTimer
+{
+public:
+    static void CheckTimerPool(nlTestSuite * inSuite, void * aContext);
+};
+} // namespace System
+} // namespace chip
+
+void chip::System::TestTimer::CheckTimerPool(nlTestSuite * inSuite, void * aContext)
+{
+    TestContext & testContext = *static_cast<TestContext *>(aContext);
+    Layer & systemLayer       = *testContext.mLayer;
+    nlTestSuite * const suite = testContext.mTestSuite;
+
+    using Timer = TimerList::Node;
+    struct TestState
+    {
+        int count = 0;
+        static void Increment(Layer * layer, void * state) { ++static_cast<TestState *>(state)->count; }
+        static void Reset(Layer * layer, void * state) { static_cast<TestState *>(state)->count = 0; }
+    };
+    TestState testState;
+
+    using namespace Clock::Literals;
+    struct
+    {
+        Clock::Timestamp awakenTime;
+        TimerCompleteCallback onComplete;
+        Timer * timer;
+    } testTimer[] = {
+        { 111_ms, TestState::Increment }, // 0
+        { 100_ms, TestState::Increment }, // 1
+        { 202_ms, TestState::Reset },     // 2
+        { 303_ms, TestState::Increment }, // 3
+    };
+
+    TimerPool<Timer> pool;
+    NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 0);
+    SYSTEM_STATS_RESET(Stats::kSystemLayer_NumTimers);
+
+    // Test TimerPool::Create() and TimerData accessors.
+
+    for (auto & timer : testTimer)
+    {
+        timer.timer = pool.Create(systemLayer, timer.awakenTime, timer.onComplete, &testState);
+    }
+#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+    NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 4);
+#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+
+    for (auto & timer : testTimer)
+    {
+        NL_TEST_ASSERT(suite, timer.timer != nullptr);
+        NL_TEST_ASSERT(suite, timer.timer->AwakenTime() == timer.awakenTime);
+        NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetOnComplete() == timer.onComplete);
+        NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetAppState() == &testState);
+        NL_TEST_ASSERT(suite, timer.timer->GetCallback().GetSystemLayer() == &systemLayer);
+    }
+
+    // Test TimerList operations.
+
+    TimerList list;
+    NL_TEST_ASSERT(suite, list.Remove(nullptr) == nullptr);
+    NL_TEST_ASSERT(suite, list.Remove(nullptr, nullptr) == nullptr);
+    NL_TEST_ASSERT(suite, list.PopEarliest() == nullptr);
+    NL_TEST_ASSERT(suite, list.PopIfEarlier(500_ms) == nullptr);
+    NL_TEST_ASSERT(suite, list.Earliest() == nullptr);
+    NL_TEST_ASSERT(suite, list.Empty());
+
+    Timer * earliest = list.Add(testTimer[0].timer); // list: () → (0) returns: 0
+    NL_TEST_ASSERT(suite, earliest == testTimer[0].timer);
+    NL_TEST_ASSERT(suite, list.PopIfEarlier(10_ms) == nullptr);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer);
+    NL_TEST_ASSERT(suite, !list.Empty());
+
+    earliest = list.Add(testTimer[1].timer); // list: (0) → (1 0) returns: 1
+    NL_TEST_ASSERT(suite, earliest == testTimer[1].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer);
+
+    earliest = list.Add(testTimer[2].timer); // list: (1 0) → (1 0 2) returns: 1
+    NL_TEST_ASSERT(suite, earliest == testTimer[1].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer);
+
+    earliest = list.Add(testTimer[3].timer); // list: (1 0 2) → (1 0 2 3) returns: 1
+    NL_TEST_ASSERT(suite, earliest == testTimer[1].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[1].timer);
+
+    earliest = list.Remove(earliest); // list: (1 0 2 3) → (0 2 3) returns: 0
+    NL_TEST_ASSERT(suite, earliest == testTimer[0].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer);
+
+    earliest = list.Remove(TestState::Reset, &testState); // list: (0 2 3) → (0 3) returns: 2
+    NL_TEST_ASSERT(suite, earliest == testTimer[2].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[0].timer);
+
+    earliest = list.PopEarliest(); // list: (0 3) → (3) returns: 0
+    NL_TEST_ASSERT(suite, earliest == testTimer[0].timer);
+    NL_TEST_ASSERT(suite, list.Earliest() == testTimer[3].timer);
+
+    earliest = list.PopIfEarlier(10_ms); // list: (3) → (3) returns: nullptr
+    NL_TEST_ASSERT(suite, earliest == nullptr);
+
+    earliest = list.PopIfEarlier(500_ms); // list: (3) → () returns: 3
+    NL_TEST_ASSERT(suite, earliest == testTimer[3].timer);
+    NL_TEST_ASSERT(suite, list.Empty());
+
+    earliest = list.Add(testTimer[3].timer); // list: () → (3) returns: 3
+    list.Clear();                            // list: (3) → ()
+    NL_TEST_ASSERT(suite, earliest == testTimer[3].timer);
+    NL_TEST_ASSERT(suite, list.Empty());
+
+    for (auto & timer : testTimer)
+    {
+        list.Add(timer.timer);
+    }
+    TimerList early = list.ExtractEarlier(200_ms); // list: (1 0 2 3) → (2 3) returns: (1 0)
+    NL_TEST_ASSERT(suite, list.PopEarliest() == testTimer[2].timer);
+    NL_TEST_ASSERT(suite, list.PopEarliest() == testTimer[3].timer);
+    NL_TEST_ASSERT(suite, list.PopEarliest() == nullptr);
+    NL_TEST_ASSERT(suite, early.PopEarliest() == testTimer[1].timer);
+    NL_TEST_ASSERT(suite, early.PopEarliest() == testTimer[0].timer);
+    NL_TEST_ASSERT(suite, early.PopEarliest() == nullptr);
+
+    // Test TimerPool::Invoke()
+    NL_TEST_ASSERT(suite, testState.count == 0);
+    pool.Invoke(testTimer[0].timer);
+    testTimer[0].timer = nullptr;
+    NL_TEST_ASSERT(suite, testState.count == 1);
+    NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 3);
+#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+    NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 3);
+#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+
+    // Test TimerPool::Release()
+    pool.Release(testTimer[1].timer);
+    testTimer[1].timer = nullptr;
+    NL_TEST_ASSERT(suite, testState.count == 1);
+    NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 2);
+#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+    NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 2);
+#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+
+    pool.ReleaseAll();
+    NL_TEST_ASSERT(suite, pool.mTimerPool.Allocated() == 0);
+#if CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+    NL_TEST_ASSERT(suite, Stats::GetResourcesInUse()[Stats::kSystemLayer_NumTimers] == 0);
+#endif // CHIP_SYSTEM_CONFIG_PROVIDE_STATISTICS
+}
+
 // Test Suite
 
 /**
@@ -178,6 +330,7 @@ static const nlTest sTests[] =
 {
     NL_TEST_DEF("Timer::TestOverflow",             CheckOverflow),
     NL_TEST_DEF("Timer::TestTimerStarvation",      CheckStarvation),
+    NL_TEST_DEF("Timer::TestTimerPool",            chip::System::TestTimer::CheckTimerPool),
     NL_TEST_SENTINEL()
 };
 // clang-format on