[libcamera-devel] [PATCH] libcamera: base: timer: Drop start() overload with int argument
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 23 02:02:36 CET 2022
On Tue, Mar 22, 2022 at 10:49:14PM +0000, Kieran Bingham wrote:
> 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)
Not to my knowledge, no. I was inspired by Naush's patch, but didn't
make an attempt at addressing the same issue.
> 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
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list