Skip to content

Commit

Permalink
Add unit tests for SystemTimer.h
Browse files Browse the repository at this point in the history 
#### 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 (project-chip#12628) introduced a bug that a proper unit
test would have caught.

Fixes project-chip#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?
  • Loading branch information
@kpschoedel
kpschoedel committed Dec 8, 2021
1 parent 4684f29 commit 9b350b8
Show file tree
Hide file tree
Showing 5 changed files with 185 additions and 29 deletions.
4 changes: 2 additions & 2 deletions src/system/SystemLayerImplLwIP.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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); });
Expand Down
4 changes: 2 additions & 2 deletions src/system/SystemLayerImplSelect.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
35 changes: 18 additions & 17 deletions src/system/SystemTimer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
18 changes: 10 additions & 8 deletions src/system/SystemTimer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ namespace chip {
namespace System {

class Layer;
class TestTimer;

/**
* Basic Timer information: time and callback.
Expand All @@ -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; }
Expand All @@ -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;

Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -224,6 +225,7 @@ class TimerPool
}

private:
friend class TestTimer;
ObjectPool<Timer, CHIP_SYSTEM_CONFIG_NUM_TIMERS> mTimerPool;
};

Expand Down
153 changes: 153 additions & 0 deletions src/system/tests/TestSystemTimer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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
Expand Down

0 comments on commit 9b350b8

Please sign in to comment.