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

[Eric Curtin]

On Fri, 15 Apr 2022 at 17:25, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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).

Funnily enough I started changing to addFdEvent, but then reverted as
it was touching many files, and didn't want the patch to spread to so
many files. I'll go back to doing that.

>
> > +                        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
>