Skip to content

Commit

Permalink
TODOs from PR #6561
Browse files Browse the repository at this point in the history
#### Problem

Some TODOs left over from PR #6561.

#### Change overview

- Remove the temporary `PrepareEventsWithTimeout()`, and have the tests
  that used it use a timer instead.
- Remove obsolete TODOs.

Fixes #7760 _Some unit tests supply a timeout at low level_
Fixes #7756 _add a timer for sleepTime.tv_sec = 10; sleepTime.tv_usec = 0;_

#### Testing

Ran the affected tests.
  • Loading branch information
kpschoedel committed Aug 11, 2021
1 parent 6ffaf3a commit 83d35c3
Show file tree
Hide file tree
Showing 14 changed files with 40 additions and 84 deletions.
1 change: 0 additions & 1 deletion src/controller/java/CHIPDeviceController-JNI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1066,7 +1066,6 @@ void * IOThreadMain(void * arg)
// Loop until we are told to exit.
while (!quit.load(std::memory_order_relaxed))
{
// TODO(#5556): add a timer for `sleepTime.tv_sec = 10; sleepTime.tv_usec = 0;`
watchState.PrepareEvents();

// Unlock the stack so that Java threads can make API calls.
Expand Down
6 changes: 3 additions & 3 deletions src/inet/tests/TestInetCommon.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ void ShutdownSystemLayer();
void InetFailError(CHIP_ERROR err, const char * msg);

void InitNetwork();
void ServiceEvents(struct ::timeval & aSleepTime);
void ServiceEvents(uint32_t aSleepTimeMilliseconds);
void ShutdownNetwork();
void DumpMemory(const uint8_t * mem, uint32_t len, const char * prefix, uint32_t rowWidth);
void DumpMemory(const uint8_t * mem, uint32_t len, const char * prefix);

inline static void ServiceNetwork(struct ::timeval & aSleepTime)
inline static void ServiceNetwork(uint32_t aSleepTimeMilliseconds)
{
ServiceEvents(aSleepTime);
ServiceEvents(aSleepTimeMilliseconds);
}
18 changes: 8 additions & 10 deletions src/inet/tests/TestInetCommonPosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
#include <support/CHIPMem.h>
#include <support/ErrorStr.h>
#include <support/ScopedBuffer.h>
#include <system/SystemTimer.h>
#include <system/SystemClock.h>

#if CHIP_SYSTEM_CONFIG_USE_LWIP
#include <lwip/dns.h>
Expand Down Expand Up @@ -390,10 +390,8 @@ void InitNetwork()

while (!NetworkIsReady())
{
struct timeval lSleepTime;
lSleepTime.tv_sec = 0;
lSleepTime.tv_usec = 100000;
ServiceEvents(lSleepTime);
constexpr uint32_t kSleepTimeMilliseconds = 100;
ServiceEvents(kSleepTimeMilliseconds);
}

// FIXME: this is kinda nasty :(
Expand Down Expand Up @@ -438,7 +436,7 @@ void InitNetwork()
gInet.Init(gSystemLayer, lContext);
}

void ServiceEvents(struct ::timeval & aSleepTime)
void ServiceEvents(uint32_t aSleepTimeMilliseconds)
{
static bool printed = false;

Expand All @@ -454,8 +452,11 @@ void ServiceEvents(struct ::timeval & aSleepTime)
}
}

gSystemLayer.StartTimer(
aSleepTimeMilliseconds, [](System::Layer *, void *, CHIP_ERROR) -> void {}, nullptr);

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
gSystemLayer.WatchableEventsManager().PrepareEventsWithTimeout(aSleepTime);
gSystemLayer.WatchableEventsManager().PrepareEvents();
gSystemLayer.WatchableEventsManager().WaitForEvents();
gSystemLayer.WatchableEventsManager().HandleEvents();
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
Expand All @@ -475,9 +476,6 @@ void ServiceEvents(struct ::timeval & aSleepTime)
else
sRemainingSystemLayerEventDelay--;

// TODO: Currently timers are delayed by aSleepTime above. A improved solution would have a mechanism to reduce
// aSleepTime according to the next timer.

gSystemLayer.WatchableEventsManager().HandlePlatformTimer();
}
}
Expand Down
11 changes: 4 additions & 7 deletions src/inet/tests/TestInetEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,9 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext)
char testHostName2[20] = "127.0.0.1";
char testHostName3[20] = "";
char testHostName4[260];
struct timeval sleepTime;
IPAddress testDestAddr[1] = { IPAddress::Any };
CHIP_ERROR err;

sleepTime.tv_sec = 0;
sleepTime.tv_usec = 10000;
constexpr uint32_t kSleepTimeMilliseconds = 100;

memset(testHostName4, 'w', sizeof(testHostName4));
testHostName4[259] = '\0';
Expand All @@ -150,7 +147,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext)
{
while (!callbackHandlerCalled)
{
ServiceNetwork(sleepTime);
ServiceNetwork(kSleepTimeMilliseconds);
}
}

Expand All @@ -162,7 +159,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext)
{
while (!callbackHandlerCalled)
{
ServiceNetwork(sleepTime);
ServiceNetwork(kSleepTimeMilliseconds);
}
}

Expand All @@ -174,7 +171,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext)
{
while (!callbackHandlerCalled)
{
ServiceNetwork(sleepTime);
ServiceNetwork(kSleepTimeMilliseconds);
}
}

Expand Down
7 changes: 2 additions & 5 deletions src/inet/tests/TestInetLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -281,14 +281,11 @@ int main(int argc, char * argv[])

while (Common::IsTesting(sTestState.mStatus))
{
struct timeval sleepTime;
bool lSucceeded = true;
bool lFailed = false;

sleepTime.tv_sec = 0;
sleepTime.tv_usec = 10000;

ServiceNetwork(sleepTime);
constexpr uint32_t kSleepTimeMilliseconds = 100;
ServiceNetwork(kSleepTimeMilliseconds);

CheckSucceededOrFailed(sTestState, lSucceeded, lFailed);

Expand Down
12 changes: 5 additions & 7 deletions src/inet/tests/TestInetLayerDNS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -652,18 +652,16 @@ static void HandleResolutionComplete(void * appState, CHIP_ERROR err, uint8_t ad
}
}

static void ServiceNetworkUntilDone(uint32_t timeoutMS)
static void ServiceNetworkUntilDone(uint32_t timeoutMilliseconds)
{
uint64_t timeoutTimeMS = System::Clock::GetMonotonicMilliseconds() + timeoutMS;
struct timeval sleepTime;
sleepTime.tv_sec = 0;
sleepTime.tv_usec = 10000;
uint64_t timeoutTimeMilliseconds = System::Clock::GetMonotonicMilliseconds() + timeoutMilliseconds;
constexpr uint32_t kSleepTimeMilliseconds = 100;

while (!gDone)
{
ServiceNetwork(sleepTime);
ServiceNetwork(kSleepTimeMilliseconds);

if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMS)
if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMilliseconds)
{
break;
}
Expand Down
7 changes: 2 additions & 5 deletions src/inet/tests/TestInetLayerMulticast.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,11 @@ int main(int argc, char * argv[])

while (Common::IsTesting(sTestState.mStatus))
{
struct timeval sleepTime;
bool lSucceeded = true;
bool lFailed = false;

sleepTime.tv_sec = 0;
sleepTime.tv_usec = 10000;

ServiceNetwork(sleepTime);
constexpr uint32_t kSleepTimeMilliseconds = 100;
ServiceNetwork(kSleepTimeMilliseconds);

CheckSucceededOrFailed(sTestState, lSucceeded, lFailed);

Expand Down
7 changes: 2 additions & 5 deletions src/inet/tests/TestLwIPDNS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,8 @@ static void TestLwIPDNS(void)

while (!Done)
{
struct timeval sleepTime;
sleepTime.tv_sec = 0;
sleepTime.tv_usec = 10000;

ServiceNetwork(sleepTime);
constexpr uint32_t kSleepTimeMilliseconds = 100;
ServiceNetwork(kSleepTimeMilliseconds);
}

// Expected cached response
Expand Down
9 changes: 2 additions & 7 deletions src/system/WatchableEventManagerLibevent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,9 @@ CHIP_ERROR WatchableEventManager::Init(System::Layer & systemLayer)
}

void WatchableEventManager::PrepareEvents()
{
timeval nextTimeout = { 0, 0 };
PrepareEventsWithTimeout(nextTimeout);
}

void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeout)
{
const Clock::MonotonicMilliseconds currentTime = Clock::GetMonotonicMilliseconds();
Clock::MonotonicMilliseconds awakenTime = currentTime + TimevalToMilliseconds(nextTimeout);
Clock::MonotonicMilliseconds awakenTime = currentTime;

Timer * timer = mTimerList.Earliest();
if (timer && Clock::IsEarlier(timer->mAwakenTime, awakenTime))
Expand All @@ -106,6 +100,7 @@ void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeou
}

const Clock::MonotonicMilliseconds sleepTime = (awakenTime > currentTime) ? (awakenTime - currentTime) : 0;
timeval nextTimeout;
MillisecondsToTimeval(sleepTime, nextTimeout);

#if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ && !__MBED__
Expand Down
3 changes: 0 additions & 3 deletions src/system/WatchableEventManagerLibevent.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ class WatchableEventManager
void HandleEvents();
void EventLoopEnds() {}

// TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.
void PrepareEventsWithTimeout(timeval & nextTimeout);

private:
/*
* In this implementation, libevent invokes LibeventCallbackHandler from beneath WaitForEvents(),
Expand Down
17 changes: 6 additions & 11 deletions src/system/WatchableEventManagerSelect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include <platform/LockTracker.h>
#include <support/CodeUtils.h>
#include <support/TimeUtils.h>
#include <system/SystemFaultInjection.h>
#include <system/SystemLayer.h>
#include <system/WatchableEventManager.h>
Expand Down Expand Up @@ -285,16 +286,10 @@ void WatchableEventManager::PrepareEvents()
{
assertChipStackLockedByCurrentThread();

// Max out this duration and let CHIP set it appropriately.
mNextTimeout.tv_sec = DEFAULT_MIN_SLEEP_PERIOD;
mNextTimeout.tv_usec = 0;
PrepareEventsWithTimeout(mNextTimeout);
}

void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeout)
{
constexpr Clock::MonotonicMilliseconds kMaxTimeout =
static_cast<Clock::MonotonicMilliseconds>(DEFAULT_MIN_SLEEP_PERIOD) * kMillisecondsPerSecond;
const Clock::MonotonicMilliseconds currentTime = Clock::GetMonotonicMilliseconds();
Clock::MonotonicMilliseconds awakenTime = currentTime + TimevalToMilliseconds(nextTimeout);
Clock::MonotonicMilliseconds awakenTime = currentTime + kMaxTimeout;

Timer * timer = mTimerList.Earliest();
if (timer && Clock::IsEarlier(timer->mAwakenTime, awakenTime))
Expand All @@ -303,10 +298,10 @@ void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeou
}

const Clock::MonotonicMilliseconds sleepTime = (awakenTime > currentTime) ? (awakenTime - currentTime) : 0;
MillisecondsToTimeval(sleepTime, nextTimeout);
MillisecondsToTimeval(sleepTime, mNextTimeout);

#if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ && !__MBED__
chip::Mdns::GetMdnsTimeout(nextTimeout);
chip::Mdns::GetMdnsTimeout(mNextTimeout);
#endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__

mSelected = mRequest;
Expand Down
3 changes: 0 additions & 3 deletions src/system/WatchableEventManagerSelect.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@ class WatchableEventManager
void HandleEvents();
void EventLoopEnds() {}

// TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.
void PrepareEventsWithTimeout(timeval & nextTimeout);

static SocketEvents SocketEventsFromFDs(int socket, const fd_set & readfds, const fd_set & writefds, const fd_set & exceptfds);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
Expand Down
16 changes: 4 additions & 12 deletions src/system/tests/TestSystemTimer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,17 @@
using chip::ErrorStr;
using namespace chip::System;

static void ServiceEvents(Layer & aLayer, ::timeval & aSleepTime)
static void ServiceEvents(Layer & aLayer)
{
#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
aLayer.WatchableEventsManager().PrepareEventsWithTimeout(aSleepTime);
aLayer.WatchableEventsManager().PrepareEvents();
aLayer.WatchableEventsManager().WaitForEvents();
aLayer.WatchableEventsManager().HandleEvents();
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK

#if CHIP_SYSTEM_CONFIG_USE_LWIP
if (aLayer.State() == kLayerState_Initialized)
{
// TODO: Currently timers are delayed by aSleepTime above. A improved solution would have a mechanism to reduce
// aSleepTime according to the next timer.
aLayer.WatchableEventsManager().HandlePlatformTimer();
}
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
Expand Down Expand Up @@ -138,10 +136,7 @@ static void CheckOverflow(nlTestSuite * inSuite, void * aContext)

while (!sOverflowTestDone)
{
struct timeval sleepTime;
sleepTime.tv_sec = 0;
sleepTime.tv_usec = 1000; // 1 ms tick
ServiceEvents(lSys, sleepTime);
ServiceEvents(lSys);
}

lSys.CancelTimer(HandleTimerFailed, aContext);
Expand All @@ -168,13 +163,10 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext)
{
TestContext & lContext = *static_cast<TestContext *>(aContext);
Layer & lSys = *lContext.mLayer;
struct timeval sleepTime;

lSys.StartTimer(0, HandleGreedyTimer, aContext);

sleepTime.tv_sec = 0;
sleepTime.tv_usec = 1000; // 1 ms tick
ServiceEvents(lSys, sleepTime);
ServiceEvents(lSys);
}

// Test Suite
Expand Down
7 changes: 2 additions & 5 deletions src/transport/raw/tests/NetworkTestHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,8 @@ CHIP_ERROR IOContext::Shutdown()
void IOContext::DriveIO()
{
// Set the select timeout to 100ms
struct timeval aSleepTime;
aSleepTime.tv_sec = 0;
aSleepTime.tv_usec = 100 * 1000;

ServiceEvents(aSleepTime);
constexpr uint32_t kSleepTimeMilliseconds = 100;
ServiceEvents(kSleepTimeMilliseconds);
}

void IOContext::DriveIOUntil(unsigned maxWaitMs, std::function<bool(void)> completionFunction)
Expand Down

0 comments on commit 83d35c3

Please sign in to comment.