[libcamera-devel] [PATCH 1/9] libcamera: timer: Remove the interval() method

[Laurent Pinchart]

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 at 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 at jmondi.org>
> 
> > +	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