[libcamera-devel] [PATCH] libcamera: base: timer: Drop start() overload with int argument
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Mar 22 23:49:14 CET 2022
Hi Laurent,
Quoting Laurent Pinchart via libcamera-devel (2022-03-22 20:42:48)
> The start(unsigned int msec) overload is error-prone, as the argument
> unit can easily be mistaken in callers. Drop it and update all callers
> to use the start(std::chrono::milliseconds) overload instead.
>
> The callers now need to use std::chrono_literals. The using statement
> could be added to timer.h for convenience, but "using" is discouraged in
> header files to avoid namespace pollution. Update the callers instead,
> and while at it, sort the "using" statements alphabetically in tests.
>
Does a utils::Duration implicitly get cast/converted to a
std::chrono::milliseconds() with this now?
I.e. - does this help ease Naush's patches to accept a utils::duration?
(Which is a good-thing-tm I think)
Anyway, I have a lot of fondness for chrono-literals. I think they
really help improve readability, and I expect prevent bugs.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/base/timer.h | 1 -
> src/libcamera/base/timer.cpp | 10 --------
> src/libcamera/ipc_pipe_unixsocket.cpp | 4 ++-
> test/camera/buffer_import.cpp | 3 ++-
> test/camera/camera_reconfigure.cpp | 3 ++-
> test/camera/capture.cpp | 3 ++-
> test/event-dispatcher.cpp | 9 ++++---
> test/event.cpp | 11 +++++----
> test/fence.cpp | 6 ++---
> test/hotplug-cameras.cpp | 5 ++--
> test/ipa/ipa_interface_test.cpp | 9 ++++---
> test/ipc/unixsocket.cpp | 5 ++--
> test/log/log_process.cpp | 5 ++--
> test/process/process_test.cpp | 5 ++--
> test/timer-thread.cpp | 7 +++---
> test/timer.cpp | 31 ++++++++++++------------
> test/v4l2_videodevice/buffer_sharing.cpp | 3 ++-
> test/v4l2_videodevice/capture_async.cpp | 3 ++-
> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 5 ++--
> 19 files changed, 67 insertions(+), 61 deletions(-)
>
> diff --git a/include/libcamera/base/timer.h b/include/libcamera/base/timer.h
> index 09f1d3229bd5..759b68ada1e8 100644
> --- a/include/libcamera/base/timer.h
> +++ b/include/libcamera/base/timer.h
> @@ -25,7 +25,6 @@ public:
> Timer(Object *parent = nullptr);
> ~Timer();
>
> - void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); }
> void start(std::chrono::milliseconds duration);
> void start(std::chrono::steady_clock::time_point deadline);
> void stop();
> diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> index 187336e3a1a4..74b060af32ff 100644
> --- a/src/libcamera/base/timer.cpp
> +++ b/src/libcamera/base/timer.cpp
> @@ -62,16 +62,6 @@ Timer::~Timer()
> stop();
> }
>
> -/**
> - * \fn Timer::start(unsigned int msec)
> - * \brief Start or restart the timer with a timeout of \a msec
> - * \param[in] msec The timer duration in milliseconds
> - *
> - * If the timer is already running it will be stopped and restarted.
> - *
> - * \context This function is \threadbound.
> - */
> -
> /**
> * \brief Start or restart the timer with a timeout of \a duration
> * \param[in] duration The timer duration in milliseconds
> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
> index 3ef907090131..da2cffc3b149 100644
> --- a/src/libcamera/ipc_pipe_unixsocket.cpp
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> @@ -18,6 +18,8 @@
> #include "libcamera/internal/ipc_unixsocket.h"
> #include "libcamera/internal/process.h"
>
> +using namespace std::chrono_literals;
> +
> namespace libcamera {
>
> LOG_DECLARE_CATEGORY(IPCPipe)
> @@ -126,7 +128,7 @@ int IPCPipeUnixSocket::call(const IPCUnixSocket::Payload &message,
> }
>
> /* \todo Make this less dangerous, see IPCPipe::sendSync() */
> - timeout.start(2000);
> + timeout.start(2000ms);
> while (!iter->second.done) {
> if (!timeout.isRunning()) {
> LOG(IPCPipe, Error) << "Call timeout!";
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index c504ea09e64b..9288400474a7 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -25,6 +25,7 @@
> #include "test.h"
>
> using namespace libcamera;
> +using namespace std::chrono_literals;
>
> namespace {
>
> @@ -135,7 +136,7 @@ protected:
> EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
>
> Timer timer;
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning())
> dispatcher->processEvents();
>
> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp
> index 0fd8ab70d561..f6076baa0a31 100644
> --- a/test/camera/camera_reconfigure.cpp
> +++ b/test/camera/camera_reconfigure.cpp
> @@ -23,6 +23,7 @@
>
> using namespace libcamera;
> using namespace std;
> +using namespace std::chrono_literals;
>
> namespace {
>
> @@ -117,7 +118,7 @@ private:
> EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
>
> Timer timer;
> - timer.start(100);
> + timer.start(100ms);
> while (timer.isRunning())
> dispatcher->processEvents();
>
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index f3824f95cbd3..de824083dfed 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -18,6 +18,7 @@
>
> using namespace libcamera;
> using namespace std;
> +using namespace std::chrono_literals;
>
> namespace {
>
> @@ -137,7 +138,7 @@ protected:
> EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
>
> Timer timer;
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning())
> dispatcher->processEvents();
>
> diff --git a/test/event-dispatcher.cpp b/test/event-dispatcher.cpp
> index 1cc17b045ec0..9b07ab2b61d7 100644
> --- a/test/event-dispatcher.cpp
> +++ b/test/event-dispatcher.cpp
> @@ -16,8 +16,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> static EventDispatcher *dispatcher;
> static bool interrupt;
> @@ -50,7 +51,7 @@ protected:
> /* Event processing interruption by signal. */
> std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now();
>
> - timer.start(1000);
> + timer.start(1000ms);
>
> struct itimerval itimer = {};
> itimer.it_value.tv_usec = 500000;
> @@ -69,7 +70,7 @@ protected:
> }
>
> /* Event processing interruption. */
> - timer.start(1000);
> + timer.start(1000ms);
> dispatcher->interrupt();
>
> dispatcher->processEvents();
> @@ -79,7 +80,7 @@ protected:
> return TestFail;
> }
>
> - timer.start(1000);
> + timer.start(1000ms);
> itimer.it_value.tv_usec = 500000;
> interrupt = true;
> setitimer(ITIMER_REAL, &itimer, nullptr);
> diff --git a/test/event.cpp b/test/event.cpp
> index d4765eb14d12..19dceae123dd 100644
> --- a/test/event.cpp
> +++ b/test/event.cpp
> @@ -16,8 +16,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class EventTest : public Test
> {
> @@ -55,7 +56,7 @@ protected:
> return TestFail;
> }
>
> - timeout.start(100);
> + timeout.start(100ms);
> dispatcher->processEvents();
> timeout.stop();
>
> @@ -67,7 +68,7 @@ protected:
> /* Test read notification without data. */
> notified_ = false;
>
> - timeout.start(100);
> + timeout.start(100ms);
> dispatcher->processEvents();
> timeout.stop();
>
> @@ -86,7 +87,7 @@ protected:
> return TestFail;
> }
>
> - timeout.start(100);
> + timeout.start(100ms);
> dispatcher->processEvents();
> timeout.stop();
>
> @@ -99,7 +100,7 @@ protected:
> notified_ = false;
> notifier_->setEnabled(true);
>
> - timeout.start(100);
> + timeout.start(100ms);
> dispatcher->processEvents();
> timeout.stop();
>
> diff --git a/test/fence.cpp b/test/fence.cpp
> index 524db2a10757..1e38bc2f8790 100644
> --- a/test/fence.cpp
> +++ b/test/fence.cpp
> @@ -22,9 +22,9 @@
> #include "camera_test.h"
> #include "test.h"
>
> -using namespace std::chrono_literals;
> using namespace libcamera;
> using namespace std;
> +using namespace std::chrono_literals;
>
> class FenceTest : public CameraTest, public Test
> {
> @@ -316,7 +316,7 @@ int FenceTest::run()
>
> /* Loop for one second. */
> Timer timer;
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && expectedCompletionResult_) {
> if (completedRequest_ == signalledRequestId_ && setFence_)
> /*
> @@ -324,7 +324,7 @@ int FenceTest::run()
> * been re-queued with a fence. Start the timer to
> * signal the fence in 10 msec.
> */
> - fenceTimer.start(10);
> + fenceTimer.start(10ms);
>
> dispatcher_->processEvents();
> }
> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp
> index df56040350c5..5d9260a241ec 100644
> --- a/test/hotplug-cameras.cpp
> +++ b/test/hotplug-cameras.cpp
> @@ -22,6 +22,7 @@
> #include "test.h"
>
> using namespace libcamera;
> +using namespace std::chrono_literals;
>
> class HotplugTest : public Test
> {
> @@ -88,7 +89,7 @@ protected:
> std::ofstream(uvcDriverDir_ + "unbind", std::ios::binary)
> << uvcDeviceDir;
> Timer timer;
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && !cameraRemoved_)
> Thread::current()->eventDispatcher()->processEvents();
> if (!cameraRemoved_) {
> @@ -99,7 +100,7 @@ protected:
> /* Bind the camera again and process events. */
> std::ofstream(uvcDriverDir_ + "bind", std::ios::binary)
> << uvcDeviceDir;
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && !cameraAdded_)
> Thread::current()->eventDispatcher()->processEvents();
> if (!cameraAdded_) {
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 43562e608506..3c0df843ea61 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -27,8 +27,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class IPAInterfaceTest : public Test, public Object
> {
> @@ -111,7 +112,7 @@ protected:
> return TestFail;
> }
>
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && trace_ != ipa::vimc::IPAOperationInit)
> dispatcher->processEvents();
>
> @@ -123,7 +124,7 @@ protected:
>
> /* Test start of IPA module. */
> ipa_->start();
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && trace_ != ipa::vimc::IPAOperationStart)
> dispatcher->processEvents();
>
> @@ -134,7 +135,7 @@ protected:
>
> /* Test stop of IPA module. */
> ipa_->stop();
> - timer.start(1000);
> + timer.start(1000ms);
> while (timer.isRunning() && trace_ != ipa::vimc::IPAOperationStop)
> dispatcher->processEvents();
>
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> index 7e90e629e9ac..304e613bc984 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -30,8 +30,9 @@
> #define CMD_LEN_CMP 3
> #define CMD_JOIN 4
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> int calculateLength(int fd)
> {
> @@ -430,7 +431,7 @@ private:
> if (ret)
> return ret;
>
> - timeout.start(200);
> + timeout.start(200ms);
> while (!callDone_) {
> if (!timeout.isRunning()) {
> cerr << "Call timeout!" << endl;
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index 2484c58f2fa9..966b80cf3af7 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -26,8 +26,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> static const string message("hello from the child");
>
> @@ -80,7 +81,7 @@ protected:
> return TestFail;
> }
>
> - timeout.start(200);
> + timeout.start(200ms);
> while (timeout.isRunning())
> dispatcher->processEvents();
>
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index b410756b3288..cb6940c6a7db 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -18,8 +18,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class ProcessTestChild
> {
> @@ -61,7 +62,7 @@ protected:
> return TestFail;
> }
>
> - timeout.start(2000);
> + timeout.start(2000ms);
> while (timeout.isRunning() && exitStatus_ == Process::NotExited)
> dispatcher->processEvents();
>
> diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp
> index f7e8743da9e6..618217538779 100644
> --- a/test/timer-thread.cpp
> +++ b/test/timer-thread.cpp
> @@ -14,8 +14,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class TimeoutHandler : public Object
> {
> @@ -24,13 +25,13 @@ public:
> : timer_(this), timeout_(false)
> {
> timer_.timeout.connect(this, &TimeoutHandler::timeoutHandler);
> - timer_.start(100);
> + timer_.start(100ms);
> }
>
> void restart()
> {
> timeout_ = false;
> - timer_.start(100);
> + timer_.start(100ms);
> }
>
> bool timeout() const
> diff --git a/test/timer.cpp b/test/timer.cpp
> index be79d0100a58..0f01c3cb00ea 100644
> --- a/test/timer.cpp
> +++ b/test/timer.cpp
> @@ -14,8 +14,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class ManagedTimer : public Timer
> {
> @@ -26,7 +27,7 @@ public:
> timeout.connect(this, &ManagedTimer::timeoutHandler);
> }
>
> - void start(int msec)
> + void start(std::chrono::milliseconds msec)
> {
> count_ = 0;
> start_ = std::chrono::steady_clock::now();
> @@ -82,7 +83,7 @@ protected:
> ManagedTimer timer2;
>
> /* Timer expiration. */
> - timer.start(1000);
> + timer.start(1000ms);
>
> if (!timer.isRunning()) {
> cout << "Timer expiration test failed" << endl;
> @@ -101,7 +102,7 @@ protected:
> * Nanosecond resolution in a 32 bit value wraps at 4.294967
> * seconds (0xFFFFFFFF / 1000000)
> */
> - timer.start(4295);
> + timer.start(4295ms);
> dispatcher->processEvents();
>
> if (timer.hasFailed()) {
> @@ -110,7 +111,7 @@ protected:
> }
>
> /* Timer restart. */
> - timer.start(500);
> + timer.start(500ms);
>
> if (!timer.isRunning()) {
> cout << "Timer restart test failed" << endl;
> @@ -125,9 +126,9 @@ protected:
> }
>
> /* Timer restart before expiration. */
> - timer.start(50);
> - timer.start(100);
> - timer.start(150);
> + timer.start(50ms);
> + timer.start(100ms);
> + timer.start(150ms);
>
> dispatcher->processEvents();
>
> @@ -147,8 +148,8 @@ protected:
> }
>
> /* Two timers. */
> - timer.start(1000);
> - timer2.start(300);
> + timer.start(1000ms);
> + timer2.start(300ms);
>
> dispatcher->processEvents();
>
> @@ -170,8 +171,8 @@ protected:
> }
>
> /* Restart timer before expiration. */
> - timer.start(1000);
> - timer2.start(300);
> + timer.start(1000ms);
> + timer2.start(300ms);
>
> dispatcher->processEvents();
>
> @@ -180,7 +181,7 @@ protected:
> return TestFail;
> }
>
> - timer.start(1000);
> + timer.start(1000ms);
>
> dispatcher->processEvents();
>
> @@ -194,10 +195,10 @@ protected:
> * deleted. This will result in a crash on failure.
> */
> ManagedTimer *dyntimer = new ManagedTimer();
> - dyntimer->start(100);
> + dyntimer->start(100ms);
> delete dyntimer;
>
> - timer.start(200);
> + timer.start(200ms);
> dispatcher->processEvents();
>
> return TestPass;
> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> index 75ee93ce5261..fa856ab6b5a8 100644
> --- a/test/v4l2_videodevice/buffer_sharing.cpp
> +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> @@ -21,6 +21,7 @@
> #include "v4l2_videodevice_test.h"
>
> using namespace libcamera;
> +using namespace std::chrono_literals;
>
> class BufferSharingTest : public V4L2VideoDeviceTest
> {
> @@ -145,7 +146,7 @@ protected:
> return TestFail;
> }
>
> - timeout.start(10000);
> + timeout.start(10000ms);
> while (timeout.isRunning()) {
> dispatcher->processEvents();
> if (framesCaptured_ > 30 && framesOutput_ > 30)
> diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> index 3aa4ca0b6955..42e1e671790b 100644
> --- a/test/v4l2_videodevice/capture_async.cpp
> +++ b/test/v4l2_videodevice/capture_async.cpp
> @@ -16,6 +16,7 @@
> #include "v4l2_videodevice_test.h"
>
> using namespace libcamera;
> +using namespace std::chrono_literals;
>
> class CaptureAsyncTest : public V4L2VideoDeviceTest
> {
> @@ -60,7 +61,7 @@ protected:
> if (ret)
> return TestFail;
>
> - timeout.start(10000);
> + timeout.start(10000ms);
> while (timeout.isRunning()) {
> dispatcher->processEvents();
> if (frames > 30)
> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> index ebf3e245f86b..852b853fa81a 100644
> --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> @@ -19,8 +19,9 @@
>
> #include "test.h"
>
> -using namespace std;
> using namespace libcamera;
> +using namespace std;
> +using namespace std::chrono_literals;
>
> class V4L2M2MDeviceTest : public Test
> {
> @@ -155,7 +156,7 @@ protected:
> }
>
> Timer timeout;
> - timeout.start(5000);
> + timeout.start(5000ms);
> while (timeout.isRunning()) {
> dispatcher->processEvents();
> if (captureFrames_ > 30)
>
> base-commit: a8284e3570de133960458c5703e75dc9e8e737c8
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list