From 83d35c383a21850561779489d5e73e148386e38a Mon Sep 17 00:00:00 2001 From: Kevin Schoedel Date: Thu, 5 Aug 2021 18:00:34 -0400 Subject: [PATCH] TODOs from PR #6561 #### 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. --- .../java/CHIPDeviceController-JNI.cpp | 1 - src/inet/tests/TestInetCommon.h | 6 +++--- src/inet/tests/TestInetCommonPosix.cpp | 18 ++++++++---------- src/inet/tests/TestInetEndPoint.cpp | 11 ++++------- src/inet/tests/TestInetLayer.cpp | 7 ++----- src/inet/tests/TestInetLayerDNS.cpp | 12 +++++------- src/inet/tests/TestInetLayerMulticast.cpp | 7 ++----- src/inet/tests/TestLwIPDNS.cpp | 7 ++----- src/system/WatchableEventManagerLibevent.cpp | 9 ++------- src/system/WatchableEventManagerLibevent.h | 3 --- src/system/WatchableEventManagerSelect.cpp | 17 ++++++----------- src/system/WatchableEventManagerSelect.h | 3 --- src/system/tests/TestSystemTimer.cpp | 16 ++++------------ src/transport/raw/tests/NetworkTestHelpers.cpp | 7 ++----- 14 files changed, 40 insertions(+), 84 deletions(-) diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 6faaddeca5bbfb..dea357f9b7c7ca 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -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. diff --git a/src/inet/tests/TestInetCommon.h b/src/inet/tests/TestInetCommon.h index 958a4516cced77..8bf55c5dd7f55b 100644 --- a/src/inet/tests/TestInetCommon.h +++ b/src/inet/tests/TestInetCommon.h @@ -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); } diff --git a/src/inet/tests/TestInetCommonPosix.cpp b/src/inet/tests/TestInetCommonPosix.cpp index f4bea80ec8b580..a1e2ac86b3c331 100644 --- a/src/inet/tests/TestInetCommonPosix.cpp +++ b/src/inet/tests/TestInetCommonPosix.cpp @@ -50,7 +50,7 @@ #include #include #include -#include +#include #if CHIP_SYSTEM_CONFIG_USE_LWIP #include @@ -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 :( @@ -438,7 +436,7 @@ void InitNetwork() gInet.Init(gSystemLayer, lContext); } -void ServiceEvents(struct ::timeval & aSleepTime) +void ServiceEvents(uint32_t aSleepTimeMilliseconds) { static bool printed = false; @@ -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 @@ -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(); } } diff --git a/src/inet/tests/TestInetEndPoint.cpp b/src/inet/tests/TestInetEndPoint.cpp index 9d0e1765101c56..1d042b6acad086 100644 --- a/src/inet/tests/TestInetEndPoint.cpp +++ b/src/inet/tests/TestInetEndPoint.cpp @@ -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'; @@ -150,7 +147,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } @@ -162,7 +159,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } @@ -174,7 +171,7 @@ static void TestResolveHostAddress(nlTestSuite * inSuite, void * inContext) { while (!callbackHandlerCalled) { - ServiceNetwork(sleepTime); + ServiceNetwork(kSleepTimeMilliseconds); } } diff --git a/src/inet/tests/TestInetLayer.cpp b/src/inet/tests/TestInetLayer.cpp index 42b68ba2b9a766..efb9ceb2a88214 100644 --- a/src/inet/tests/TestInetLayer.cpp +++ b/src/inet/tests/TestInetLayer.cpp @@ -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); diff --git a/src/inet/tests/TestInetLayerDNS.cpp b/src/inet/tests/TestInetLayerDNS.cpp index 227acc78762e7c..3a39178db0e628 100644 --- a/src/inet/tests/TestInetLayerDNS.cpp +++ b/src/inet/tests/TestInetLayerDNS.cpp @@ -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; } diff --git a/src/inet/tests/TestInetLayerMulticast.cpp b/src/inet/tests/TestInetLayerMulticast.cpp index ef458af4b42879..1171936e597352 100644 --- a/src/inet/tests/TestInetLayerMulticast.cpp +++ b/src/inet/tests/TestInetLayerMulticast.cpp @@ -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); diff --git a/src/inet/tests/TestLwIPDNS.cpp b/src/inet/tests/TestLwIPDNS.cpp index a8e8580d89b33a..18501d29191bdb 100644 --- a/src/inet/tests/TestLwIPDNS.cpp +++ b/src/inet/tests/TestLwIPDNS.cpp @@ -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 diff --git a/src/system/WatchableEventManagerLibevent.cpp b/src/system/WatchableEventManagerLibevent.cpp index 9f0379f50d66ef..bf79e75ad08c19 100644 --- a/src/system/WatchableEventManagerLibevent.cpp +++ b/src/system/WatchableEventManagerLibevent.cpp @@ -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)) @@ -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__ diff --git a/src/system/WatchableEventManagerLibevent.h b/src/system/WatchableEventManagerLibevent.h index 346cee2b96d0cc..634162282559c2 100644 --- a/src/system/WatchableEventManagerLibevent.h +++ b/src/system/WatchableEventManagerLibevent.h @@ -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(), diff --git a/src/system/WatchableEventManagerSelect.cpp b/src/system/WatchableEventManagerSelect.cpp index 926e572a3c524f..5b318c0897b7fd 100644 --- a/src/system/WatchableEventManagerSelect.cpp +++ b/src/system/WatchableEventManagerSelect.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -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(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)) @@ -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; diff --git a/src/system/WatchableEventManagerSelect.h b/src/system/WatchableEventManagerSelect.h index f3abf4161b3401..57cd1eeca1e77d 100644 --- a/src/system/WatchableEventManagerSelect.h +++ b/src/system/WatchableEventManagerSelect.h @@ -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 diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp index ab3d0034bdf409..7f5d81743d2013 100644 --- a/src/system/tests/TestSystemTimer.cpp +++ b/src/system/tests/TestSystemTimer.cpp @@ -50,10 +50,10 @@ 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 @@ -61,8 +61,6 @@ static void ServiceEvents(Layer & aLayer, ::timeval & aSleepTime) #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 @@ -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); @@ -168,13 +163,10 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext) { TestContext & lContext = *static_cast(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 diff --git a/src/transport/raw/tests/NetworkTestHelpers.cpp b/src/transport/raw/tests/NetworkTestHelpers.cpp index a7f676cec25348..3040089da7e332 100644 --- a/src/transport/raw/tests/NetworkTestHelpers.cpp +++ b/src/transport/raw/tests/NetworkTestHelpers.cpp @@ -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 completionFunction)