| Message ID | 20191006053226.8976-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sun, Oct 06, 2019 at 08:32:18AM +0300, Laurent Pinchart wrote: > The libcamera timers are single-shot timers. They are started with a > duration, but fire once only, not based on an interval. Remove the > interval concept by removing the interval() method, and rename other > occurences of interval to duration. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/timer.h | 4 +--- > src/libcamera/timer.cpp | 19 ++++++------------- > src/qcam/qt_event_dispatcher.cpp | 7 ++++++- > 3 files changed, 13 insertions(+), 17 deletions(-) > > diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h > index 476ae45f1e53..09f426a59993 100644 > --- a/include/libcamera/timer.h > +++ b/include/libcamera/timer.h > @@ -24,11 +24,10 @@ public: > ~Timer(); > > void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); } > - void start(std::chrono::milliseconds interval); > + void start(std::chrono::milliseconds duration); > void stop(); > bool isRunning() const; > > - std::chrono::milliseconds interval() const { return interval_; } > std::chrono::steady_clock::time_point deadline() const { return deadline_; } > > Signal<Timer *> timeout; > @@ -40,7 +39,6 @@ private: > void registerTimer(); > void unregisterTimer(); > > - std::chrono::milliseconds interval_; > std::chrono::steady_clock::time_point deadline_; > }; > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > index b3cea3dadb49..34410bab0fb0 100644 > --- a/src/libcamera/timer.cpp > +++ b/src/libcamera/timer.cpp > @@ -61,19 +61,18 @@ Timer::~Timer() > */ > > /** > - * \brief Start or restart the timer with a timeout of \a interval > - * \param[in] interval The timer duration in milliseconds > + * \brief Start or restart the timer with a timeout of \a duration > + * \param[in] duration The timer duration in milliseconds > * > * If the timer is already running it will be stopped and restarted. > */ > -void Timer::start(std::chrono::milliseconds interval) > +void Timer::start(std::chrono::milliseconds duration) > { > - interval_ = interval; > - deadline_ = utils::clock::now() + interval; > + deadline_ = utils::clock::now() + duration; > > LOG(Timer, Debug) > - << "Starting timer " << this << " with interval " > - << interval.count() << ": deadline " > + << "Starting timer " << this << " with duration " > + << duration.count() << ": deadline " > << utils::time_point_to_string(deadline_); Shouldn't you print the duration instead of the expected deadline here? > > registerTimer(); > @@ -113,12 +112,6 @@ bool Timer::isRunning() const > return deadline_ != utils::time_point(); > } > > -/** > - * \fn Timer::interval() > - * \brief Retrieve the timer interval > - * \return The timer interval in milliseconds > - */ > - > /** > * \fn Timer::deadline() > * \brief Retrieve the timer deadline > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > index 994af3ead82a..9e989bef7d53 100644 > --- a/src/qcam/qt_event_dispatcher.cpp > +++ b/src/qcam/qt_event_dispatcher.cpp > @@ -5,6 +5,7 @@ > * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher > */ > > +#include <chrono> > #include <iostream> > > #include <QAbstractEventDispatcher> > @@ -112,7 +113,11 @@ void QtEventDispatcher::exceptionNotifierActivated(int socket) > > void QtEventDispatcher::registerTimer(Timer *timer) > { > - int timerId = startTimer(timer->interval()); > + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); > + std::chrono::steady_clock::duration duration = timer->deadline() - now; Would it make sense an helper to get this from the timer ? (the deadline_ - now part). Nothing urgent though Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + std::chrono::milliseconds msec = > + std::chrono::duration_cast<std::chrono::milliseconds>(duration); > + int timerId = startTimer(msec); > timers_[timerId] = timer; > timerIds_[timer] = timerId; > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi again On Sun, Oct 06, 2019 at 08:32:18AM +0300, Laurent Pinchart wrote: > The libcamera timers are single-shot timers. They are started with a > duration, but fire once only, not based on an interval. Remove the > interval concept by removing the interval() method, and rename other > occurences of interval to duration. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/timer.h | 4 +--- > src/libcamera/timer.cpp | 19 ++++++------------- > src/qcam/qt_event_dispatcher.cpp | 7 ++++++- > 3 files changed, 13 insertions(+), 17 deletions(-) > > diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h > index 476ae45f1e53..09f426a59993 100644 > --- a/include/libcamera/timer.h > +++ b/include/libcamera/timer.h > @@ -24,11 +24,10 @@ public: > ~Timer(); > > void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); } > - void start(std::chrono::milliseconds interval); > + void start(std::chrono::milliseconds duration); > void stop(); > bool isRunning() const; > > - std::chrono::milliseconds interval() const { return interval_; } > std::chrono::steady_clock::time_point deadline() const { return deadline_; } > > Signal<Timer *> timeout; > @@ -40,7 +39,6 @@ private: > void registerTimer(); > void unregisterTimer(); > > - std::chrono::milliseconds interval_; > std::chrono::steady_clock::time_point deadline_; just noticed, can we use the libcamera defined time related types in utils here ? Thanks j > }; > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > index b3cea3dadb49..34410bab0fb0 100644 > --- a/src/libcamera/timer.cpp > +++ b/src/libcamera/timer.cpp > @@ -61,19 +61,18 @@ Timer::~Timer() > */ > > /** > - * \brief Start or restart the timer with a timeout of \a interval > - * \param[in] interval The timer duration in milliseconds > + * \brief Start or restart the timer with a timeout of \a duration > + * \param[in] duration The timer duration in milliseconds > * > * If the timer is already running it will be stopped and restarted. > */ > -void Timer::start(std::chrono::milliseconds interval) > +void Timer::start(std::chrono::milliseconds duration) > { > - interval_ = interval; > - deadline_ = utils::clock::now() + interval; > + deadline_ = utils::clock::now() + duration; > > LOG(Timer, Debug) > - << "Starting timer " << this << " with interval " > - << interval.count() << ": deadline " > + << "Starting timer " << this << " with duration " > + << duration.count() << ": deadline " > << utils::time_point_to_string(deadline_); > > registerTimer(); > @@ -113,12 +112,6 @@ bool Timer::isRunning() const > return deadline_ != utils::time_point(); > } > > -/** > - * \fn Timer::interval() > - * \brief Retrieve the timer interval > - * \return The timer interval in milliseconds > - */ > - > /** > * \fn Timer::deadline() > * \brief Retrieve the timer deadline > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > index 994af3ead82a..9e989bef7d53 100644 > --- a/src/qcam/qt_event_dispatcher.cpp > +++ b/src/qcam/qt_event_dispatcher.cpp > @@ -5,6 +5,7 @@ > * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher > */ > > +#include <chrono> > #include <iostream> > > #include <QAbstractEventDispatcher> > @@ -112,7 +113,11 @@ void QtEventDispatcher::exceptionNotifierActivated(int socket) > > void QtEventDispatcher::registerTimer(Timer *timer) > { > - int timerId = startTimer(timer->interval()); > + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); > + std::chrono::steady_clock::duration duration = timer->deadline() - now; > + std::chrono::milliseconds msec = > + std::chrono::duration_cast<std::chrono::milliseconds>(duration); > + int timerId = startTimer(msec); > timers_[timerId] = timer; > timerIds_[timer] = timerId; > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Sun, Oct 06, 2019 at 08:22:49PM +0200, Jacopo Mondi wrote: > On Sun, Oct 06, 2019 at 08:32:18AM +0300, Laurent Pinchart wrote: > > The libcamera timers are single-shot timers. They are started with a > > duration, but fire once only, not based on an interval. Remove the > > interval concept by removing the interval() method, and rename other > > occurences of interval to duration. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/timer.h | 4 +--- > > src/libcamera/timer.cpp | 19 ++++++------------- > > src/qcam/qt_event_dispatcher.cpp | 7 ++++++- > > 3 files changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h > > index 476ae45f1e53..09f426a59993 100644 > > --- a/include/libcamera/timer.h > > +++ b/include/libcamera/timer.h > > @@ -24,11 +24,10 @@ public: > > ~Timer(); > > > > void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); } > > - void start(std::chrono::milliseconds interval); > > + void start(std::chrono::milliseconds duration); > > void stop(); > > bool isRunning() const; > > > > - std::chrono::milliseconds interval() const { return interval_; } > > std::chrono::steady_clock::time_point deadline() const { return deadline_; } > > > > Signal<Timer *> timeout; > > @@ -40,7 +39,6 @@ private: > > void registerTimer(); > > void unregisterTimer(); > > > > - std::chrono::milliseconds interval_; > > std::chrono::steady_clock::time_point deadline_; > > just noticed, can we use the libcamera defined time related types in > utils here ? No, because utils isn't public. > > }; > > > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > > index b3cea3dadb49..34410bab0fb0 100644 > > --- a/src/libcamera/timer.cpp > > +++ b/src/libcamera/timer.cpp > > @@ -61,19 +61,18 @@ Timer::~Timer() > > */ > > > > /** > > - * \brief Start or restart the timer with a timeout of \a interval > > - * \param[in] interval The timer duration in milliseconds > > + * \brief Start or restart the timer with a timeout of \a duration > > + * \param[in] duration The timer duration in milliseconds > > * > > * If the timer is already running it will be stopped and restarted. > > */ > > -void Timer::start(std::chrono::milliseconds interval) > > +void Timer::start(std::chrono::milliseconds duration) > > { > > - interval_ = interval; > > - deadline_ = utils::clock::now() + interval; > > + deadline_ = utils::clock::now() + duration; > > > > LOG(Timer, Debug) > > - << "Starting timer " << this << " with interval " > > - << interval.count() << ": deadline " > > + << "Starting timer " << this << " with duration " > > + << duration.count() << ": deadline " > > << utils::time_point_to_string(deadline_); > > > > registerTimer(); > > @@ -113,12 +112,6 @@ bool Timer::isRunning() const > > return deadline_ != utils::time_point(); > > } > > > > -/** > > - * \fn Timer::interval() > > - * \brief Retrieve the timer interval > > - * \return The timer interval in milliseconds > > - */ > > - > > /** > > * \fn Timer::deadline() > > * \brief Retrieve the timer deadline > > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > > index 994af3ead82a..9e989bef7d53 100644 > > --- a/src/qcam/qt_event_dispatcher.cpp > > +++ b/src/qcam/qt_event_dispatcher.cpp > > @@ -5,6 +5,7 @@ > > * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher > > */ > > > > +#include <chrono> > > #include <iostream> > > > > #include <QAbstractEventDispatcher> > > @@ -112,7 +113,11 @@ void QtEventDispatcher::exceptionNotifierActivated(int socket) > > > > void QtEventDispatcher::registerTimer(Timer *timer) > > { > > - int timerId = startTimer(timer->interval()); > > + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); > > + std::chrono::steady_clock::duration duration = timer->deadline() - now; > > + std::chrono::milliseconds msec = > > + std::chrono::duration_cast<std::chrono::milliseconds>(duration); > > + int timerId = startTimer(msec); > > timers_[timerId] = timer; > > timerIds_[timer] = timerId; > > }
Hi Jacopo, On Sun, Oct 06, 2019 at 08:16:31PM +0200, Jacopo Mondi wrote: > On Sun, Oct 06, 2019 at 08:32:18AM +0300, Laurent Pinchart wrote: > > The libcamera timers are single-shot timers. They are started with a > > duration, but fire once only, not based on an interval. Remove the > > interval concept by removing the interval() method, and rename other > > occurences of interval to duration. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/timer.h | 4 +--- > > src/libcamera/timer.cpp | 19 ++++++------------- > > src/qcam/qt_event_dispatcher.cpp | 7 ++++++- > > 3 files changed, 13 insertions(+), 17 deletions(-) > > > > diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h > > index 476ae45f1e53..09f426a59993 100644 > > --- a/include/libcamera/timer.h > > +++ b/include/libcamera/timer.h > > @@ -24,11 +24,10 @@ public: > > ~Timer(); > > > > void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); } > > - void start(std::chrono::milliseconds interval); > > + void start(std::chrono::milliseconds duration); > > void stop(); > > bool isRunning() const; > > > > - std::chrono::milliseconds interval() const { return interval_; } > > std::chrono::steady_clock::time_point deadline() const { return deadline_; } > > > > Signal<Timer *> timeout; > > @@ -40,7 +39,6 @@ private: > > void registerTimer(); > > void unregisterTimer(); > > > > - std::chrono::milliseconds interval_; > > std::chrono::steady_clock::time_point deadline_; > > }; > > > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > > index b3cea3dadb49..34410bab0fb0 100644 > > --- a/src/libcamera/timer.cpp > > +++ b/src/libcamera/timer.cpp > > @@ -61,19 +61,18 @@ Timer::~Timer() > > */ > > > > /** > > - * \brief Start or restart the timer with a timeout of \a interval > > - * \param[in] interval The timer duration in milliseconds > > + * \brief Start or restart the timer with a timeout of \a duration > > + * \param[in] duration The timer duration in milliseconds > > * > > * If the timer is already running it will be stopped and restarted. > > */ > > -void Timer::start(std::chrono::milliseconds interval) > > +void Timer::start(std::chrono::milliseconds duration) > > { > > - interval_ = interval; > > - deadline_ = utils::clock::now() + interval; > > + deadline_ = utils::clock::now() + duration; > > > > LOG(Timer, Debug) > > - << "Starting timer " << this << " with interval " > > - << interval.count() << ": deadline " > > + << "Starting timer " << this << " with duration " > > + << duration.count() << ": deadline " > > << utils::time_point_to_string(deadline_); > > Shouldn't you print the duration instead of the expected deadline > here? I'm not sure to follow you, I'm printing both. > > registerTimer(); > > @@ -113,12 +112,6 @@ bool Timer::isRunning() const > > return deadline_ != utils::time_point(); > > } > > > > -/** > > - * \fn Timer::interval() > > - * \brief Retrieve the timer interval > > - * \return The timer interval in milliseconds > > - */ > > - > > /** > > * \fn Timer::deadline() > > * \brief Retrieve the timer deadline > > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > > index 994af3ead82a..9e989bef7d53 100644 > > --- a/src/qcam/qt_event_dispatcher.cpp > > +++ b/src/qcam/qt_event_dispatcher.cpp > > @@ -5,6 +5,7 @@ > > * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher > > */ > > > > +#include <chrono> > > #include <iostream> > > > > #include <QAbstractEventDispatcher> > > @@ -112,7 +113,11 @@ void QtEventDispatcher::exceptionNotifierActivated(int socket) > > > > void QtEventDispatcher::registerTimer(Timer *timer) > > { > > - int timerId = startTimer(timer->interval()); > > + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); > > + std::chrono::steady_clock::duration duration = timer->deadline() - now; > > Would it make sense an helper to get this from the timer ? (the > deadline_ - now part). Nothing urgent though We could, but I'd rather expose from the Timer class only the data specific to the timer, and leave the rest to the caller to figure out. Otherwise we could end up with lots of helpers with very few users for each. If some helpers tend to be very useful we can reconsider, but I wouldn't do so yet. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + std::chrono::milliseconds msec = > > + std::chrono::duration_cast<std::chrono::milliseconds>(duration); > > + int timerId = startTimer(msec); > > timers_[timerId] = timer; > > timerIds_[timer] = timerId; > > }
diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h index 476ae45f1e53..09f426a59993 100644 --- a/include/libcamera/timer.h +++ b/include/libcamera/timer.h @@ -24,11 +24,10 @@ public: ~Timer(); void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); } - void start(std::chrono::milliseconds interval); + void start(std::chrono::milliseconds duration); void stop(); bool isRunning() const; - std::chrono::milliseconds interval() const { return interval_; } std::chrono::steady_clock::time_point deadline() const { return deadline_; } Signal<Timer *> timeout; @@ -40,7 +39,6 @@ private: void registerTimer(); void unregisterTimer(); - std::chrono::milliseconds interval_; std::chrono::steady_clock::time_point deadline_; }; diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp index b3cea3dadb49..34410bab0fb0 100644 --- a/src/libcamera/timer.cpp +++ b/src/libcamera/timer.cpp @@ -61,19 +61,18 @@ Timer::~Timer() */ /** - * \brief Start or restart the timer with a timeout of \a interval - * \param[in] interval The timer duration in milliseconds + * \brief Start or restart the timer with a timeout of \a duration + * \param[in] duration The timer duration in milliseconds * * If the timer is already running it will be stopped and restarted. */ -void Timer::start(std::chrono::milliseconds interval) +void Timer::start(std::chrono::milliseconds duration) { - interval_ = interval; - deadline_ = utils::clock::now() + interval; + deadline_ = utils::clock::now() + duration; LOG(Timer, Debug) - << "Starting timer " << this << " with interval " - << interval.count() << ": deadline " + << "Starting timer " << this << " with duration " + << duration.count() << ": deadline " << utils::time_point_to_string(deadline_); registerTimer(); @@ -113,12 +112,6 @@ bool Timer::isRunning() const return deadline_ != utils::time_point(); } -/** - * \fn Timer::interval() - * \brief Retrieve the timer interval - * \return The timer interval in milliseconds - */ - /** * \fn Timer::deadline() * \brief Retrieve the timer deadline diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp index 994af3ead82a..9e989bef7d53 100644 --- a/src/qcam/qt_event_dispatcher.cpp +++ b/src/qcam/qt_event_dispatcher.cpp @@ -5,6 +5,7 @@ * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher */ +#include <chrono> #include <iostream> #include <QAbstractEventDispatcher> @@ -112,7 +113,11 @@ void QtEventDispatcher::exceptionNotifierActivated(int socket) void QtEventDispatcher::registerTimer(Timer *timer) { - int timerId = startTimer(timer->interval()); + std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); + std::chrono::steady_clock::duration duration = timer->deadline() - now; + std::chrono::milliseconds msec = + std::chrono::duration_cast<std::chrono::milliseconds>(duration); + int timerId = startTimer(msec); timers_[timerId] = timer; timerIds_[timer] = timerId; }
The libcamera timers are single-shot timers. They are started with a duration, but fire once only, not based on an interval. Remove the interval concept by removing the interval() method, and rename other occurences of interval to duration. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/timer.h | 4 +--- src/libcamera/timer.cpp | 19 ++++++------------- src/qcam/qt_event_dispatcher.cpp | 7 ++++++- 3 files changed, 13 insertions(+), 17 deletions(-)