From 62cf867383f5b6747f938dfecceaccf017ad25c6 Mon Sep 17 00:00:00 2001
From: Nicolas Pope <nwpope@utu.fi>
Date: Sat, 13 Jun 2020 19:27:04 +0300
Subject: [PATCH] Switch timer to use handler class
---
CMakeLists.txt | 2 +-
applications/vision/src/main.cpp | 2 +-
components/audio/include/ftl/audio/source.hpp | 4 +-
components/common/cpp/include/ftl/handle.hpp | 62 ++++++++++++++++++-
components/common/cpp/include/ftl/timer.hpp | 46 +++-----------
components/common/cpp/src/configuration.cpp | 2 +-
components/common/cpp/src/timer.cpp | 54 +++++++---------
components/common/cpp/test/timer_unit.cpp | 25 ++------
.../net/cpp/include/ftl/net/universe.hpp | 13 ++--
components/net/cpp/src/universe.cpp | 2 +-
.../include/ftl/rgbd/frameset.hpp | 2 +-
.../rgbd-sources/include/ftl/rgbd/group.hpp | 6 +-
.../streams/include/ftl/streams/builder.hpp | 2 +-
.../include/ftl/streams/filestream.hpp | 2 +-
14 files changed, 112 insertions(+), 112 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4df38ff5c..55bb87265 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -394,7 +394,7 @@ if (WIN32) # TODO(nick) Should do based upon compiler (VS)
set(OS_LIBS "")
else()
add_definitions(-DUNIX)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -fPIC -msse3 -Wall")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -fPIC -msse3 -Wall -Werror=unused-result")
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_DEBUG -pg")
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O3 -mfpmath=sse")
set(OS_LIBS "dl")
diff --git a/applications/vision/src/main.cpp b/applications/vision/src/main.cpp
index 2ed9ed444..ceddbaf4c 100644
--- a/applications/vision/src/main.cpp
+++ b/applications/vision/src/main.cpp
@@ -86,7 +86,7 @@ static void run(ftl::Configurable *root) {
ftl::ctrl::Master ctrl(root, net);
// Sync clocks!
- ftl::timer::add(ftl::timer::kTimerMain, [&time_peer,&sync_counter,net](int64_t ts) {
+ auto timer = ftl::timer::add(ftl::timer::kTimerMain, [&time_peer,&sync_counter,net](int64_t ts) {
if (sync_counter-- <= 0 && time_peer != ftl::UUID(0) ) {
sync_counter = 20;
auto start = std::chrono::high_resolution_clock::now();
diff --git a/components/audio/include/ftl/audio/source.hpp b/components/audio/include/ftl/audio/source.hpp
index ff0663ddc..fa8249d23 100644
--- a/components/audio/include/ftl/audio/source.hpp
+++ b/components/audio/include/ftl/audio/source.hpp
@@ -37,8 +37,8 @@ class Source : public ftl::Configurable, public ftl::audio::Generator {
private:
ftl::audio::FrameState state_;
bool active_;
- ftl::timer::TimerHandle timer_hp_;
- ftl::timer::TimerHandle timer_main_;
+ ftl::Handle timer_hp_;
+ ftl::Handle timer_main_;
ftl::audio::FrameSet::Callback cb_;
ftl::audio::Buffer<short> *buffer_;
diff --git a/components/common/cpp/include/ftl/handle.hpp b/components/common/cpp/include/ftl/handle.hpp
index d7e2f8089..92b99fdcc 100644
--- a/components/common/cpp/include/ftl/handle.hpp
+++ b/components/common/cpp/include/ftl/handle.hpp
@@ -2,6 +2,7 @@
#define _FTL_HANDLE_HPP_
#include <ftl/threads.hpp>
+#include <ftl/exception.hpp>
#include <functional>
#include <unordered_map>
@@ -22,7 +23,7 @@ struct BaseHandler {
* A `Handle` is used to manage registered callbacks, allowing them to be
* removed safely whenever the `Handle` instance is destroyed.
*/
-struct Handle {
+struct [[nodiscard]] Handle {
friend struct BaseHandler;
/**
@@ -72,7 +73,7 @@ struct Handler : BaseHandler {
* Add a new callback function. It returns a `Handle` object that must
* remain in scope, the destructor of the `Handle` will remove the callback.
*/
- [[nodiscard]] Handle on(const std::function<bool(ARGS...)> &f) {
+ Handle on(const std::function<bool(ARGS...)> &f) {
std::unique_lock<std::mutex> lk(mutex_);
int id = id_++;
callbacks_[id] = f;
@@ -111,6 +112,63 @@ struct Handler : BaseHandler {
std::unordered_map<int, std::function<bool(ARGS...)>> callbacks_;
};
+/**
+ * This class is used to manage callbacks. The template parameters are the
+ * arguments to be passed to the callback when triggered. This class is already
+ * thread-safe. Note that this version only allows a single callback at a time
+ * and throws an exception if multiple are added without resetting.
+ */
+template <typename ...ARGS>
+struct SingletonHandler : BaseHandler {
+ /**
+ * Add a new callback function. It returns a `Handle` object that must
+ * remain in scope, the destructor of the `Handle` will remove the callback.
+ */
+ [[nodiscard]] Handle on(const std::function<bool(ARGS...)> &f) {
+ std::unique_lock<std::mutex> lk(mutex_);
+ if (callback_) throw FTL_Error("Callback already bound");
+ callback_ = f;
+ return make_handle(this, id_++);
+ }
+
+ /**
+ * Safely trigger all callbacks. Note that `Handler` is locked when
+ * triggering so callbacks cannot make modifications to it or they will
+ * lock up. To remove a callback, return false from the callback, else
+ * return true.
+ */
+ bool trigger(ARGS ...args) {
+ std::unique_lock<std::mutex> lk(mutex_);
+ if (callback_) {
+ bool keep = callback_(std::forward<ARGS>(args)...);
+ if (!keep) callback_ = nullptr;
+ return keep;
+ } else {
+ return false;
+ }
+ //} catch (const std::exception &e) {
+ // LOG(ERROR) << "Exception in callback: " << e.what();
+ //}
+ }
+
+ /**
+ * Remove a callback using its `Handle`. This is equivalent to allowing the
+ * `Handle` to be destroyed or cancelled. If the handle does not match the
+ * currently bound callback then the callback is not removed.
+ */
+ void remove(const Handle &h) override {
+ std::unique_lock<std::mutex> lk(mutex_);
+ if (h.id() == id_-1) callback_ = nullptr;
+ }
+
+ void reset() { callback_ = nullptr; }
+
+ operator bool() const { return (bool)callback_; }
+
+ private:
+ std::function<bool(ARGS...)> callback_;
+};
+
}
ftl::Handle ftl::BaseHandler::make_handle(BaseHandler *h, int id) {
diff --git a/components/common/cpp/include/ftl/timer.hpp b/components/common/cpp/include/ftl/timer.hpp
index bf378425d..9ccb0b31f 100644
--- a/components/common/cpp/include/ftl/timer.hpp
+++ b/components/common/cpp/include/ftl/timer.hpp
@@ -1,6 +1,7 @@
#ifndef _FTL_COMMON_TIMER_HPP_
#define _FTL_COMMON_TIMER_HPP_
+#include <ftl/handle.hpp>
#include <functional>
namespace ftl {
@@ -15,6 +16,11 @@ namespace ftl {
*/
namespace timer {
+/**
+ * Timer level determines in what order and when a timer callback is called.
+ * This allows some timers to operate at higher precision / lower latency
+ * than others, as well as having idle callbacks.
+ */
enum timerlevel_t {
kTimerHighPrecision = 0,
kTimerSwap,
@@ -24,44 +30,6 @@ enum timerlevel_t {
kTimerMAXLEVEL
};
-/**
- * Represents a timer job for control purposes. Use to remove timer jobs in
- * a destructor, for example.
- */
-struct TimerHandle {
- TimerHandle() : id_(-1) {}
- explicit TimerHandle(int i) : id_(i) {}
- TimerHandle(const TimerHandle &t) : id_(t.id()) {}
-
- /**
- * Cancel the timer job. If currently executing it will block and wait for
- * the job to complete.
- */
- void cancel() const;
- void pause() const;
- void unpause() const;
-
- /**
- * Do the timer job every N frames.
- */
- void setMultiplier(unsigned int) const;
-
- /**
- * Give the timer job a name for logging output.
- */
- void setName(const std::string &) const;
-
- /**
- * Allow copy assignment.
- */
- TimerHandle &operator=(const TimerHandle &h) { id_ = h.id(); return *this; }
-
- inline int id() const { return id_; }
-
- private:
- int id_;
-};
-
int64_t get_time();
/**
@@ -114,7 +82,7 @@ void setClockSlave(bool);
* If all high precision callbacks together take more than 1ms to complete, a
* warning is produced.
*/
-const TimerHandle add(timerlevel_t, const std::function<bool(int64_t ts)> &);
+ftl::Handle add(timerlevel_t, const std::function<bool(int64_t ts)> &);
/**
* Initiate the timer and optionally block the current process.
diff --git a/components/common/cpp/src/configuration.cpp b/components/common/cpp/src/configuration.cpp
index 0d2b79a97..111154022 100644
--- a/components/common/cpp/src/configuration.cpp
+++ b/components/common/cpp/src/configuration.cpp
@@ -664,7 +664,7 @@ std::vector<nlohmann::json*> ftl::config::_createArray(ftl::Configurable *parent
//LOG(WARNING) << "Expected an array for '" << name << "' in " << parent->getID();
}
- return std::move(result);
+ return result;
}
nlohmann::json &ftl::config::_create(ftl::Configurable *parent, const std::string &name) {
diff --git a/components/common/cpp/src/timer.cpp b/components/common/cpp/src/timer.cpp
index bc8f97e2d..54ab2cb64 100644
--- a/components/common/cpp/src/timer.cpp
+++ b/components/common/cpp/src/timer.cpp
@@ -29,9 +29,9 @@ static int last_id = 0;
static bool clock_slave = true;
struct TimerJob {
- int id;
- function<bool(int64_t)> job;
- volatile bool active;
+ int id=0;
+ ftl::SingletonHandler<int64_t> job;
+ volatile bool active=false;
// TODO: (Nick) Implement richer forms of timer
//bool paused;
//int multiplier;
@@ -76,7 +76,7 @@ static void waitTimePoint() {
auto idle_job = jobs[kTimerIdle10].begin();
while (idle_job != jobs[kTimerIdle10].end() && msdelay >= 10 && sincelast != mspf) {
(*idle_job).active = true;
- bool doremove = !(*idle_job).job(now);
+ bool doremove = !(*idle_job).job.trigger(now);
if (doremove) {
idle_job = jobs[kTimerIdle10].erase(idle_job);
@@ -132,13 +132,16 @@ bool ftl::timer::isClockSlave() {
return clock_slave;
}
-const TimerHandle ftl::timer::add(timerlevel_t l, const std::function<bool(int64_t ts)> &f) {
+ftl::Handle ftl::timer::add(timerlevel_t l, const std::function<bool(int64_t ts)> &f) {
if (l < 0 || l >= kTimerMAXLEVEL) return {};
UNIQUE_LOCK(mtx, lk);
int newid = last_id++;
- jobs[l].push_back({newid, f, false, "NoName"});
- return TimerHandle(newid);
+ auto &j = jobs[l].emplace_back();
+ j.id = newid;
+ j.name = "NoName";
+ ftl::Handle h = j.job.on(f);
+ return h;
}
static void removeJob(int id) {
@@ -165,14 +168,14 @@ static void trigger_jobs() {
// First do non-blocking high precision callbacks
const int64_t before = get_time();
for (auto &j : jobs[kTimerHighPrecision]) {
- j.job(ts);
+ j.job.trigger(ts);
}
const int64_t after = get_time();
if (after - before > 1) LOG(WARNING) << "Precision jobs took too long (" << (after-before) << "ms)";
// Then do also non-blocking swap callbacks
for (auto &j : jobs[kTimerSwap]) {
- j.job(ts);
+ j.job.trigger(ts);
}
// Now use thread jobs to do more intensive callbacks
@@ -187,12 +190,21 @@ static void trigger_jobs() {
auto *pj = &j;
ftl::pool.push([pj,ts](int id) {
- bool doremove = !pj->job(ts);
+ bool doremove = !pj->job.trigger(ts);
pj->active = false;
active_jobs--;
if (doremove) removeJob(pj->id);
});
}
+
+ // Final cleanup of stale jobs
+ for (size_t j=0; j<kTimerMAXLEVEL; ++j) {
+ for (auto i=jobs[j].begin(); i!=jobs[j].end(); i++) {
+ if ((bool)((*i).job) == false) {
+ i = jobs[j].erase(i);
+ }
+ }
+ }
}
namespace ftl {
@@ -246,25 +258,3 @@ void ftl::timer::reset() {
jobs[i].clear();
}
}
-
-// ===== TimerHandle ===========================================================
-
-void ftl::timer::TimerHandle::cancel() const {
- removeJob(id());
-}
-
-void ftl::timer::TimerHandle::pause() const {
-
-}
-
-void ftl::timer::TimerHandle::unpause() const {
-
-}
-
-void ftl::timer::TimerHandle::setMultiplier(unsigned int N) const {
-
-}
-
-void ftl::timer::TimerHandle::setName(const std::string &name) const {
-
-}
diff --git a/components/common/cpp/test/timer_unit.cpp b/components/common/cpp/test/timer_unit.cpp
index 2fdc70034..2c7602646 100644
--- a/components/common/cpp/test/timer_unit.cpp
+++ b/components/common/cpp/test/timer_unit.cpp
@@ -59,13 +59,13 @@ TEST_CASE( "Timer::add() High Precision Accuracy" ) {
REQUIRE( (rc.id() >= 0) );
- ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
+ auto h = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
didrun[1] = true;
ftl::timer::stop(false);
return true;
});
- ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
+ auto h2 = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
didrun[2] = true;
ftl::timer::stop(false);
return true;
@@ -184,7 +184,7 @@ TEST_CASE( "Timer::add() Main job" ) {
REQUIRE( (rc.id() >= 0) );
- ftl::timer::add(ftl::timer::kTimerMain, [&job2](int64_t ts) {
+ auto h = ftl::timer::add(ftl::timer::kTimerMain, [&job2](int64_t ts) {
job2++;
return true;
});
@@ -212,24 +212,7 @@ TEST_CASE( "Timer::add() Main job" ) {
}
}
-TEST_CASE( "TimerHandle::cancel()" ) {
- SECTION( "Invalid id" ) {
- bool didjob = false;
- ftl::timer::reset();
-
- ftl::timer::add(ftl::timer::kTimerMain, [&didjob](int64_t ts) {
- didjob = true;
- ftl::timer::stop(false);
- return true;
- });
-
- // Fake Handle
- ftl::timer::TimerHandle h(44);
- h.cancel();
- ftl::timer::start(true);
- REQUIRE( didjob );
- }
-
+TEST_CASE( "Timer Handle::cancel()" ) {
SECTION( "Valid id" ) {
bool didjob = false;
ftl::timer::reset();
diff --git a/components/net/cpp/include/ftl/net/universe.hpp b/components/net/cpp/include/ftl/net/universe.hpp
index f5af2edef..54748fdc6 100644
--- a/components/net/cpp/include/ftl/net/universe.hpp
+++ b/components/net/cpp/include/ftl/net/universe.hpp
@@ -244,6 +244,7 @@ class Universe : public ftl::Configurable {
std::list<ReconnectInfo> reconnects_;
size_t phase_;
std::list<ftl::net::Peer*> garbage_;
+ ftl::Handle garbage_timer_;
size_t send_size_;
size_t recv_size_;
@@ -342,9 +343,9 @@ std::optional<R> Universe::findOne(const std::string &name, ARGS... args) {
// Cancel any further results
lk.lock();
for (auto p : peers_) {
- auto m = record.find(p);
- if (m != record.end()) {
- p->cancelCall(m->second);
+ auto mm = record.find(p);
+ if (mm != record.end()) {
+ p->cancelCall(mm->second);
}
}
}
@@ -387,9 +388,9 @@ std::vector<R> Universe::findAll(const std::string &name, ARGS... args) {
// Cancel any further results
lk.lock();
for (auto p : peers_) {
- auto m = record.find(p);
- if (m != record.end()) {
- p->cancelCall(m->second);
+ auto mm = record.find(p);
+ if (mm != record.end()) {
+ p->cancelCall(mm->second);
}
}
}
diff --git a/components/net/cpp/src/universe.cpp b/components/net/cpp/src/universe.cpp
index d5aeb743a..783d44707 100644
--- a/components/net/cpp/src/universe.cpp
+++ b/components/net/cpp/src/universe.cpp
@@ -76,7 +76,7 @@ Universe::Universe(nlohmann::json &config) :
// Add an idle timer job to garbage collect peer objects
// Note: Important to be a timer job to ensure no other timer jobs are
// using the object.
- ftl::timer::add(ftl::timer::kTimerIdle10, [this](int64_t ts) {
+ garbage_timer_ = ftl::timer::add(ftl::timer::kTimerIdle10, [this](int64_t ts) {
if (garbage_.size() > 0) {
UNIQUE_LOCK(net_mutex_,lk);
if (ftl::pool.n_idle() == ftl::pool.size()) {
diff --git a/components/rgbd-sources/include/ftl/rgbd/frameset.hpp b/components/rgbd-sources/include/ftl/rgbd/frameset.hpp
index 1c4f7da20..30ced7b65 100644
--- a/components/rgbd-sources/include/ftl/rgbd/frameset.hpp
+++ b/components/rgbd-sources/include/ftl/rgbd/frameset.hpp
@@ -157,7 +157,7 @@ class Builder : public Generator {
int id_;
std::atomic<int> jobs_;
volatile bool skip_;
- ftl::timer::TimerHandle main_id_;
+ ftl::Handle main_id_;
size_t size_;
size_t bufferSize_;
std::vector<ftl::rgbd::FrameState*> states_;
diff --git a/components/rgbd-sources/include/ftl/rgbd/group.hpp b/components/rgbd-sources/include/ftl/rgbd/group.hpp
index eed75bbc5..1b227ead0 100644
--- a/components/rgbd-sources/include/ftl/rgbd/group.hpp
+++ b/components/rgbd-sources/include/ftl/rgbd/group.hpp
@@ -105,9 +105,9 @@ class Group : public ftl::rgbd::Generator {
std::atomic<int> jobs_;
std::atomic<int> cjobs_;
volatile bool skip_;
- ftl::timer::TimerHandle cap_id_;
- ftl::timer::TimerHandle swap_id_;
- ftl::timer::TimerHandle main_id_;
+ ftl::Handle cap_id_;
+ ftl::Handle swap_id_;
+ ftl::Handle main_id_;
std::string name_;
MUTEX mutex_;
diff --git a/components/streams/include/ftl/streams/builder.hpp b/components/streams/include/ftl/streams/builder.hpp
index 4cd32246a..7bfc42050 100644
--- a/components/streams/include/ftl/streams/builder.hpp
+++ b/components/streams/include/ftl/streams/builder.hpp
@@ -71,7 +71,7 @@ class Builder {
int id_;
std::atomic<int> jobs_;
volatile bool skip_;
- ftl::timer::TimerHandle main_id_;
+ ftl::Handle main_id_;
size_t size_;
size_t bufferSize_;
diff --git a/components/streams/include/ftl/streams/filestream.hpp b/components/streams/include/ftl/streams/filestream.hpp
index d1fea1d9f..ecbb427bb 100644
--- a/components/streams/include/ftl/streams/filestream.hpp
+++ b/components/streams/include/ftl/streams/filestream.hpp
@@ -71,7 +71,7 @@ class File : public Stream {
int64_t first_ts_;
bool active_;
int version_;
- ftl::timer::TimerHandle timer_;
+ ftl::Handle timer_;
bool is_video_;
bool save_data_;
--
GitLab