[libcamera-devel] [PATCH v8 1/4] cam: event_loop: Add timer events to event loop
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 18 21:23:33 CEST 2022
Hi Eric,
On Tue, May 17, 2022 at 08:19:34PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:48)
> > 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.
Can you extend the commit message to explain why this is needed ?
> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > ---
> > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++
> > src/cam/event_loop.h | 6 ++++++
> > 2 files changed, 39 insertions(+)
> >
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index e25784c0..181c971c 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -47,6 +47,10 @@ int EventLoop::exec()
> > void EventLoop::exit(int code)
> > {
> > exitCode_ = code;
> > + for (auto const &e : events_) {
> > + event_del(e->event_);
> > + }
>
> No need for { } on a single line statement.
And as events_ is a vector of unique_ptr, just
events_.clear();
will do.
> > +
> > event_base_loopbreak(base_);
> > }
> >
> > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type,
> > events_.push_back(std::move(event));
> > }
> >
> > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv)
> > +{
> > + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count();
>
> Do you need to subtract the seconds from here now?
>
> > + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count();
>
> Or does this wrap accordingly?
>
> I.e. would 1.5 seconds end up as :
> tv.tv_sec = 1;
> tv.tv_usec = 1500000;
Looks like an issue indeed. There's an implementation in
src/libcamera/base/utils.h that you can copy:
uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds(d).count();
tv.tv_sec = usecs / 1000000ULL;
tv.tv_usec = usecs % 1000000ULL;
As this is only three lines of code, and used once in addTimerEvent(),
I'd just move it to that function and drom toTimeval().
> With that resolved, I think that's a
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +}
> > +
> > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d,
As this adds a periodic timer event, let's call the variable period (or
interval, up to you) instead of d.
> > + 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;
> > + toTimeval(d, tv);
>
> It's a shame that it's frowned upon to override std namespace to have
> custom constructors or having a construction to allow:
> struct timeval tv(d);
>
> would be nice ... (but I don't think it's something we should do here).
That's something that the C++ standard library could have provided. We
have a helper function in base/utils.h but that's not public.
> > +
> > + 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..89215dce 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -14,6 +14,8 @@
You should include <chrono> here.
> >
> > #include <event2/util.h>
> >
> > +using namespace std::chrono_literals;
I think this can be dropped, as you don't use literals in this file.
With these small changes you'll have my Rb tag.
> > +
> > struct event_base;
> >
> > class EventLoop
> > @@ -37,6 +39,9 @@ public:
> > void addEvent(int fd, EventType type,
> > const std::function<void()> &handler);
> >
> > + void addTimerEvent(const std::chrono::milliseconds d,
> > + const std::function<void()> &handler);
> > +
> > private:
> > struct Event {
> > Event(const std::function<void()> &callback);
> > @@ -57,6 +62,7 @@ private:
> > std::list<std::unique_ptr<Event>> events_;
> > std::mutex lock_;
> >
> > + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv);
> > static void dispatchCallback(evutil_socket_t fd, short flags,
> > void *param);
> > void dispatchCall();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list