[libcamera-devel] [PATCH v7 1/2] cam: event_loop: Add timer events to event loop

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Fri, Apr 08, 2022 at 05:00:14PM +0100, Eric Curtin via libcamera-devel wrote:
> Extend the EventLoop class to support periodic timer events. This can be
> used to run tasks periodically, such as handling the event loop of SDL.
> 
> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> ---
>  src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
>  src/cam/event_loop.h   |  3 +++
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..1e1a4269 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -60,6 +60,17 @@ void EventLoop::callLater(const std::function<void()> &func)
>  	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
>  }
>  
> +int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
> +{
> +	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
> +	if (!event->event_) {
> +		std::cerr << "Failed to create event for fd " << fd << std::endl;

This message will be a bit confusing for timer events, as it will print

	Failed to create event for fd -1

As there's really no other code sharing in this function, how about
dropping it, keeping addEvent() unchanged, and called event_new()
directlry in addTimerEvent() with a more appropriate message (such as
"Failed to create timer event") ?

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  void EventLoop::addEvent(int fd, EventType type,
>  			 const std::function<void()> &callback)
>  {
> @@ -68,10 +79,7 @@ void EventLoop::addEvent(int fd, EventType type,
>  		     | (type & Write ? EV_WRITE : 0)
>  		     | EV_PERSIST;
>  
> -	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
> -				  event.get());
> -	if (!event->event_) {
> -		std::cerr << "Failed to create event for fd " << fd << std::endl;
> +	if (eventNew(event, fd, events)) {
>  		return;
>  	}
>  
> @@ -84,6 +92,27 @@ void EventLoop::addEvent(int fd, EventType type,
>  	events_.push_back(std::move(event));
>  }
>  
> +void EventLoop::addTimerEvent(const suseconds_t period,
> +			      const std::function<void()> &callback)
> +{
> +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	if (eventNew(event, -1, EV_PERSIST)) {
> +		return;
> +	}
> +
> +	struct timeval tv;
> +	tv.tv_sec = 0;
> +	tv.tv_usec = period;
> +
> +	int ret = event_add(event->event_, &tv);
> +	if (ret < 0) {
> +		std::cerr << "Failed to add timer event" << std::endl;
> +		return;
> +	}
> +
> +	events_.push_back(std::move(event));
> +}
> +
>  void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
>  				 [[maybe_unused]] short flags, void *param)
>  {
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index a4613eb2..3d3561e5 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -36,6 +36,8 @@ public:
>  
>  	void addEvent(int fd, EventType type,
>  		      const std::function<void()> &handler);
> +	void addTimerEvent(const suseconds_t period,

This could be called addEvent() too, C++ will pick the right function.
The name addTimerEvent() makes the code more explicit though, which is
nice, but I'd then rename addEvent to addFdEvent() (in a patch before
this one).

> +			   const std::function<void()> &handler);
>  
>  private:
>  	struct Event {
> @@ -60,4 +62,5 @@ private:
>  	static void dispatchCallback(evutil_socket_t fd, short flags,
>  				     void *param);
>  	void dispatchCall();
> +	int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
>  };

-- 
Regards,

Laurent Pinchart