SSDP: periodic message handling broken

[konimaru]

This is just an observation, feel free to convince me that I misunderstood something :)

Current behaviour:

1. record lastTimeNotify = now₀
2. in handlerLoop establish now₁ + 60sec
3. check if lastTimeNotify is greater than (2) then send new notify

This never works, I only ever get one notification once the server is started, then never again.

Proposed change:

1. record lastTimeNotify = now₀
2. in handlerLoop establish now₁ - 60sec
3. check if lastTimeNotify is less than (2) then send new notify

With that I do get periodic notifications.

diff --git a/common/SoapySSDPEndpoint.cpp b/common/SoapySSDPEndpoint.cpp
index 8c860a4..0538ff2 100644
--- a/common/SoapySSDPEndpoint.cpp
+++ b/common/SoapySSDPEndpoint.cpp
@@ -289,7 +289,7 @@ void SoapySSDPEndpoint::handlerLoop(void)
         }
 
         const auto timeNow = std::chrono::high_resolution_clock::now();
-        const auto triggerExpired = timeNow + std::chrono::seconds(TRIGGER_TIMEOUT_SECONDS);
+        const auto triggerExpired = timeNow - std::chrono::seconds(TRIGGER_TIMEOUT_SECONDS);
 
         //remove old cache entries
         for (auto &ipPair : _impl->usnToURL)
@@ -308,13 +308,13 @@ void SoapySSDPEndpoint::handlerLoop(void)
         for (auto &data : _impl->handlers)
         {
             //check trigger for periodic search
-            if (this->periodicSearchEnabled and data->lastTimeSearch > triggerExpired)
+            if (this->periodicSearchEnabled and data->lastTimeSearch < triggerExpired)
             {
                 this->sendSearchHeader(data);
             }
 
             //check trigger for periodic notify
-            if (this->periodicNotifyEnabled and data->lastTimeNotify > triggerExpired)
+            if (this->periodicNotifyEnabled and data->lastTimeNotify < triggerExpired)
             {
                 this->sendNotifyHeader(data, NTS_ALIVE);
             }

[zuckschwerdt]

I'd say the usual way to write this is:

1. record lastTimeAction
2. in handlerLoop establish now
3. check if now is greater than or equal lastTimeAction + interval then take action again

Your logic is sound but to avoid the subtraction and for clarity I would propose something like:

         const auto timeNow = std::chrono::high_resolution_clock::now();
-        const auto triggerExpired = timeNow + std::chrono::seconds(TRIGGER_TIMEOUT_SECONDS);
+        const auto triggerInterval = std::chrono::seconds(TRIGGER_TIMEOUT_SECONDS);
 
             //check trigger for periodic search
-            if (this->periodicSearchEnabled and data->lastTimeSearch > triggerExpired)
+            if (this->periodicSearchEnabled and data->lastTimeSearch + triggerInterval <= timeNow)
             //check trigger for periodic notify

-            if (this->periodicNotifyEnabled and data->lastTimeNotify > triggerExpired)
+            if (this->periodicNotifyEnabled and data->lastTimeNotify + triggerInterval <= timeNow)

[konimaru]

We are on the same page then. Admittedly, my proposed version is harder to read/understand but was the quickest way to get it working (minimal change).

How is this handled here, pull request as usual?

[zuckschwerdt]

A PR is fine.

(and the reasoning why I dislike the substraction is that high_resolution_clock might be a steady_clock which in turn might have unsigned overflow, maybe if this is run in the first 60 seconds after a reboot)