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

[Laurent Pinchart]

Hi Eric,

Thank you for the patch.

On Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin 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.
> 
> Also delete all events in the list, before we event_base_loopbreak, in
> an effort to avoid race conditions.

What race condition in particular does this solve ?

> Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++
>  src/cam/event_loop.h   |  7 ++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 2e3ce995..315da38a 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -47,6 +47,8 @@ int EventLoop::exec()
>  void EventLoop::exit(int code)
>  {
>  	exitCode_ = code;
> +	events_.clear();
> +
>  	event_base_loopbreak(base_);
>  }
>  
> @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,
>  	events_.push_back(std::move(event));
>  }
>  
> +void EventLoop::addTimerEvent(const duration period,
> +			      const std::function<void()> &callback)
> +{
> +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
> +				  event.get());
> +	if (!event->event_) {
> +		std::cerr << "Failed to create timer event" << std::endl;
> +		return;
> +	}
> +
> +	struct timeval tv;
> +	const uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds>(period).count();
> +	tv.tv_sec = usecs / 1000000ULL;
> +	tv.tv_usec = usecs % 1000000ULL;
> +
> +	const int ret = event_add(event->event_, &tv);

No need for const here.

> +	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 79902d87..22769ea5 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -7,9 +7,10 @@
>  
>  #pragma once
>  
> +#include <chrono>
>  #include <functional>
> -#include <memory>
>  #include <list>
> +#include <memory>
>  #include <mutex>
>  
>  #include <event2/util.h>
> @@ -37,6 +38,10 @@ public:
>  	void addFdEvent(int fd, EventType type,
>  			const std::function<void()> &handler);
>  
> +	using duration = std::chrono::steady_clock::duration;

Is there a need to use the duration of steady_clock, is there any
drawback in using microseconds or milliseconds ?

I can address these small issues when applying if the other patches in
the series are fine.

> +	void addTimerEvent(const duration period,
> +			   const std::function<void()> &handler);
> +
>  private:
>  	struct Event {
>  		Event(const std::function<void()> &callback);

-- 
Regards,

Laurent Pinchart