TODOs from PR project-chip#6561 (project-chip#8900)

* TODOs from PR project-chip#6561

#### Problem

Some TODOs left over from PR project-chip#6561.

#### Change overview

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

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

#### Testing

Ran the affected tests.

* fix kSleepTimeMilliseconds

* review

src/controller/java/CHIPDeviceController-JNI.cpp

@@ -1057,7 +1057,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.

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);
 }

src/inet/tests/TestInetCommonPosix.cpp

@@ -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>
@@ -398,10 +398,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 :(
@@ -446,7 +444,7 @@ void InitNetwork()
     gInet.Init(gSystemLayer, lContext);
 }
 
-void ServiceEvents(struct ::timeval & aSleepTime)
+void ServiceEvents(uint32_t aSleepTimeMilliseconds)
 {
     static bool printed = false;
 
@@ -462,8 +460,12 @@ void ServiceEvents(struct ::timeval & aSleepTime)
         }
     }
 
+    // Start a timer (with a no-op callback) to ensure that WaitForEvents() does not block longer than aSleepTimeMilliseconds.
+    gSystemLayer.StartTimer(
+        aSleepTimeMilliseconds, [](System::Layer *, void *) -> 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
@@ -483,9 +485,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();
         }
     }

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 = 10;
 
     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);
         }
     }
 

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 = 10;
+        ServiceNetwork(kSleepTimeMilliseconds);
 
         CheckSucceededOrFailed(sTestState, lSucceeded, lFailed);
 

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 = 10;
 
     while (!gDone)
     {
-        ServiceNetwork(sleepTime);
+        ServiceNetwork(kSleepTimeMilliseconds);
 
-        if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMS)
+        if (System::Clock::GetMonotonicMilliseconds() >= timeoutTimeMilliseconds)
         {
             break;
         }

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 = 10;
+        ServiceNetwork(kSleepTimeMilliseconds);
 
         CheckSucceededOrFailed(sTestState, lSucceeded, lFailed);
 

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 = 10;
+        ServiceNetwork(kSleepTimeMilliseconds);
     }
 
     // Expected cached response

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__

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(),

src/system/WatchableEventManagerSelect.cpp

@@ -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>
@@ -294,16 +295,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))
@@ -312,10 +307,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;

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

src/system/tests/TestSystemTimer.cpp

@@ -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
@@ -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<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

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<bool(void)> completionFunction)