SoapyRemote
SoapyRemote copied to clipboard
SSDP: periodic message handling broken
This is just an observation, feel free to convince me that I misunderstood something :)
Current behaviour:
- record lastTimeNotify = now0
- in handlerLoop establish now1 + 60sec
- 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:
- record lastTimeNotify = now0
- in handlerLoop establish now1 - 60sec
- 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);
}
I'd say the usual way to write this is:
- record lastTimeAction
- in handlerLoop establish now
- 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)
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?
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)