From 35b4b01cbea55b4e6f07974203962e45a427f237 Mon Sep 17 00:00:00 2001
From: Nicolas Pope <nwpope@utu.fi>
Date: Sun, 18 Aug 2019 18:09:22 +0300
Subject: [PATCH] Use a TimerHandle

---
 components/common/cpp/include/ftl/timer.hpp   | 21 ++++++++----
 components/common/cpp/src/timer.cpp           | 22 +++++++++---
 components/common/cpp/test/timer_unit.cpp     | 34 ++++++++++---------
 .../rgbd-sources/include/ftl/rgbd/group.hpp   |  7 ++--
 components/rgbd-sources/src/group.cpp         |  9 ++---
 5 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/components/common/cpp/include/ftl/timer.hpp b/components/common/cpp/include/ftl/timer.hpp
index d93724aee..b64152e17 100644
--- a/components/common/cpp/include/ftl/timer.hpp
+++ b/components/common/cpp/include/ftl/timer.hpp
@@ -23,6 +23,19 @@ enum timerlevel_t {
 	kTimerMAXLEVEL
 };
 
+/**
+ * Represents a timer job for control purposes. Use to remove timer jobs in
+ * a destructor, for example.
+ */
+struct TimerHandle {
+	const int id = -1;
+
+	void cancel() const;
+	void pause() const;
+	void unpause() const;
+	TimerHandle &operator=(const TimerHandle &h) { const_cast<int&>(id) = h.id; return *this; }
+};
+
 /**
  * Milliseconds between calls.
  */
@@ -44,13 +57,7 @@ void setClockAdjustment(int64_t ms);
  * If all high precision callbacks together take more than 1ms to complete, a
  * warning is produced.
  */
-int add(timerlevel_t, const std::function<void(int64_t ts)> &);
-
-
-/**
- * Remove a callback using an ID returned when it was added.
- */
-void remove(int);
+const TimerHandle add(timerlevel_t, const std::function<void(int64_t ts)> &);
 
 /**
  * Initiate the timer and optionally block the current process.
diff --git a/components/common/cpp/src/timer.cpp b/components/common/cpp/src/timer.cpp
index d6719058b..0707124ee 100644
--- a/components/common/cpp/src/timer.cpp
+++ b/components/common/cpp/src/timer.cpp
@@ -77,16 +77,16 @@ void ftl::timer::setClockAdjustment(int64_t ms) {
 	clock_adjust = ms;
 }
 
-int ftl::timer::add(timerlevel_t l, const std::function<void(int64_t ts)> &f) {
-	if (l < 0 || l >= kTimerMAXLEVEL) return -1;
+const TimerHandle ftl::timer::add(timerlevel_t l, const std::function<void(int64_t ts)> &f) {
+	if (l < 0 || l >= kTimerMAXLEVEL) return {-1};
 
 	UNIQUE_LOCK(mtx, lk);
 	int newid = last_id++;
 	jobs[l].push_back({newid, f, false});
-	return newid;
+	return {newid};
 }
 
-void ftl::timer::remove(int id) {
+static void removeJob(int id) {
 	UNIQUE_LOCK(mtx, lk);
 	if (id < 0) return;
 	for (size_t j=0; j<kTimerMAXLEVEL; ++j) {
@@ -180,3 +180,17 @@ 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 {
+	
+}
diff --git a/components/common/cpp/test/timer_unit.cpp b/components/common/cpp/test/timer_unit.cpp
index c0137ef44..6244341a1 100644
--- a/components/common/cpp/test/timer_unit.cpp
+++ b/components/common/cpp/test/timer_unit.cpp
@@ -16,13 +16,13 @@ TEST_CASE( "Timer::add() High Precision Accuracy" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
 			didrun = true;
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::start(true);
 		REQUIRE( didrun == true );
@@ -33,14 +33,14 @@ TEST_CASE( "Timer::add() High Precision Accuracy" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
 			didrun = true;
 			std::this_thread::sleep_for(std::chrono::milliseconds(5));
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::start(true);
 		REQUIRE( didrun == true );
@@ -51,13 +51,13 @@ TEST_CASE( "Timer::add() High Precision Accuracy" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
 			didrun[0] = true;
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::add(ftl::timer::kTimerHighPrecision, [&didrun](int64_t ts) {
 			didrun[1] = true;
@@ -84,13 +84,13 @@ TEST_CASE( "Timer::add() Main job" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerMain, [&didrun](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerMain, [&didrun](int64_t ts) {
 			didrun = true;
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::start(true);
 		REQUIRE( didrun == true );
@@ -101,14 +101,14 @@ TEST_CASE( "Timer::add() Main job" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerMain, [&didrun](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerMain, [&didrun](int64_t ts) {
 			didrun = true;
 			std::this_thread::sleep_for(std::chrono::milliseconds(60));
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::start(true);
 		REQUIRE( didrun == true );
@@ -120,14 +120,14 @@ TEST_CASE( "Timer::add() Main job" ) {
 
 		ftl::timer::reset();
 
-		int rc = ftl::timer::add(ftl::timer::kTimerMain, [&job1](int64_t ts) {
+		auto rc = ftl::timer::add(ftl::timer::kTimerMain, [&job1](int64_t ts) {
 			job1++;
 			std::this_thread::sleep_for(std::chrono::milliseconds(60));
 			ftl::timer::stop(false);
 			return;
 		});
 
-		REQUIRE( (rc >= 0) );
+		REQUIRE( (rc.id >= 0) );
 
 		ftl::timer::add(ftl::timer::kTimerMain, [&job2](int64_t ts) {
 			job2++;
@@ -139,7 +139,7 @@ TEST_CASE( "Timer::add() Main job" ) {
 	}
 }
 
-TEST_CASE( "Timer::remove()" ) {
+TEST_CASE( "TimerHandle::cancel()" ) {
 	SECTION( "Invalid id" ) {
 		bool didjob = false;
 		ftl::timer::reset();
@@ -150,7 +150,9 @@ TEST_CASE( "Timer::remove()" ) {
 			return;
 		});
 
-		ftl::timer::remove(55);
+		// Fake Handle
+		ftl::timer::TimerHandle h = {44};
+		h.cancel();
 		ftl::timer::start(true);
 		REQUIRE( didjob );
 	}
@@ -159,13 +161,13 @@ TEST_CASE( "Timer::remove()" ) {
 		bool didjob = false;
 		ftl::timer::reset();
 
-		int id = ftl::timer::add(ftl::timer::kTimerMain, [&didjob](int64_t ts) {
+		auto id = ftl::timer::add(ftl::timer::kTimerMain, [&didjob](int64_t ts) {
 			didjob = true;
 			ftl::timer::stop(false);
 			return;
 		});
 
-		ftl::timer::remove(id);
+		id.cancel();
 		ftl::timer::start(false);
 		std::this_thread::sleep_for(std::chrono::milliseconds(10));
 		ftl::timer::stop();
diff --git a/components/rgbd-sources/include/ftl/rgbd/group.hpp b/components/rgbd-sources/include/ftl/rgbd/group.hpp
index 31499dbd3..c1a7e4521 100644
--- a/components/rgbd-sources/include/ftl/rgbd/group.hpp
+++ b/components/rgbd-sources/include/ftl/rgbd/group.hpp
@@ -2,6 +2,7 @@
 #define _FTL_RGBD_GROUP_HPP_
 
 #include <ftl/threads.hpp>
+#include <ftl/timer.hpp>
 
 #include <opencv2/opencv.hpp>
 #include <vector>
@@ -92,9 +93,9 @@ class Group {
 	int64_t last_ts_;
 	std::atomic<int> jobs_;
 	volatile bool skip_;
-	int cap_id_;
-	int swap_id_;
-	int main_id_;
+	ftl::timer::TimerHandle cap_id_;
+	ftl::timer::TimerHandle swap_id_;
+	ftl::timer::TimerHandle main_id_;
 
 	/* Insert a new frameset into the buffer, along with all intermediate
 	 * framesets between the last in buffer and the new one.
diff --git a/components/rgbd-sources/src/group.cpp b/components/rgbd-sources/src/group.cpp
index d9b8888b6..81e667aa4 100644
--- a/components/rgbd-sources/src/group.cpp
+++ b/components/rgbd-sources/src/group.cpp
@@ -15,9 +15,6 @@ Group::Group() : framesets_(kFrameBufferSize), head_(0) {
 	framesets_[0].timestamp = -1;
 	jobs_ = 0;
 	skip_ = false;
-	cap_id_ = -1;
-	swap_id_ = -1;
-	main_id_ = -1;
 	setFPS(20);
 	setLatency(5);
 }
@@ -27,9 +24,9 @@ Group::~Group() {
 		s->removeCallback();
 	}
 
-	ftl::timer::remove(main_id_);
-	ftl::timer::remove(swap_id_);
-	ftl::timer::remove(cap_id_);
+	main_id_.cancel();
+	swap_id_.cancel();
+	cap_id_.cancel();
 
 	UNIQUE_LOCK(mutex_, lk);
 	// Make sure all jobs have finished
-- 
GitLab