[libcamera-devel] [PATCH 06/11] libcamera: Add event notification infrastructure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 7 21:25:07 CET 2019
Hi Jacopo,
On Monday, 7 January 2019 20:14:21 EET Jacopo Mondi wrote:
> Hi Laurent,
> wow, this is a lot to process... let's try...
>
> On Sun, Jan 06, 2019 at 04:33:23AM +0200, Laurent Pinchart wrote:
> > Add three new classes, EventDispatcher, EventNotifier and Timer, that
> > define APIs for file descriptor event notification and timers. The
> > implementation of the EventDispatcher is meant to be provided to
> > libcamera by the application.
> >
> > The event dispatcher is integrated twith the camera manager to implement
> > automatic registration of timers and events.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >
> > include/libcamera/camera_manager.h | 7 ++
> > include/libcamera/event_dispatcher.h | 33 ++++++++
> > include/libcamera/event_notifier.h | 42 ++++++++++
> > include/libcamera/libcamera.h | 1 +
> > include/libcamera/meson.build | 3 +
> > include/libcamera/timer.h | 37 +++++++++
> > src/libcamera/camera_manager.cpp | 42 +++++++++-
> > src/libcamera/event_dispatcher.cpp | 102 +++++++++++++++++++++++
> > src/libcamera/event_notifier.cpp | 118 +++++++++++++++++++++++++++
> > src/libcamera/meson.build | 3 +
> > src/libcamera/timer.cpp | 105 ++++++++++++++++++++++++
> > 11 files changed, 492 insertions(+), 1 deletion(-)
> > create mode 100644 include/libcamera/event_dispatcher.h
> > create mode 100644 include/libcamera/event_notifier.h
> > create mode 100644 include/libcamera/timer.h
> > create mode 100644 src/libcamera/event_dispatcher.cpp
> > create mode 100644 src/libcamera/event_notifier.cpp
> > create mode 100644 src/libcamera/timer.cpp
[snip]
> > diff --git a/include/libcamera/event_dispatcher.h
> > b/include/libcamera/event_dispatcher.h new file mode 100644
> > index 000000000000..c20518c6866d
> > --- /dev/null
> > +++ b/include/libcamera/event_dispatcher.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_dispatcher.h - Event dispatcher
> > + */
> > +#ifndef __LIBCAMERA_EVENT_DISPATCHER_H__
> > +#define __LIBCAMERA_EVENT_DISPATCHER_H__
> > +
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +class EventNotifier;
> > +class Timer;
>
> Any reason why forward declaring instead of including headers? afaict
> there is no risk of circular inclusion...
To lower the compilation time and to avoid potential future circular inclusion
problems.
> > +
> > +class EventDispatcher
> > +{
> > +public:
> > + virtual ~EventDispatcher();
> > +
> > + virtual void registerEventNotifier(EventNotifier *notifier) = 0;
> > + virtual void unregisterEventNotifier(EventNotifier *notifier) = 0;
> > +
> > + virtual void registerTimer(Timer *timer) = 0;
> > + virtual void unregisterTimer(Timer *timer) = 0;
> > +
> > + virtual void processEvents() = 0;
>
> Care to clarify me how you see the processEvents() function will be
> integrated in the application event loop?
Assuming usage of EventDispatcherPoll, it can be as simple as running, in the
application,
CameraManager *cm = CameraManager::instance();
while (1)
cm->eventDispatcher()->processEvents();
> The application should call into this to trigger processing of new
> events, and according to the below documentation, this call is expected
> to be blocking (for the poll based dispatcher, if no timer is active,
> it surely is).
>
> Do we expect application to deal with this running this in a dedicated
> thread? Should we take care of that and perform asynchronous event
> dispatching?
No, applications are not expected to run this in a separate thread, they're
expected to use the processEvents() function as part of their mail loop. If an
application uses an external main loop, it is expected to implement the
register and unregister functions so that its main loop listens for the file
descriptor events and timers, and call its own implementation of
processEvents() once the event loop guarantees than an event is available.
With the internal EventDispatcherPoll implementation, the poll() call is
inside processEvents(). When integrating an external event loop, the
equivalent to the poll() call is expected to be outside of processEvents(), in
the external event loop, and processEvents() is expected to be called from the
main loop to process the events detected by the poll() equivalent.
Note that libcamera does not call processEvents() internally, at least for
now. If we later need to spawn threads, with per-thread events, I expect we
will use EventDispatcherPoll internally for implement an event loop for those
threads. If we then still want to offer to the user the ability to use their
own event dispatcher for threads internal to libcamera (I'm not sure what the
use case would be though) then we would have to revisit this.
There is no technical requirement for processEvents() to be blocking right
now, except for the ability to implement the main event loop in a very simple
way using the above code. A custom EventDispatcher can thus implement
processEvents() in a non-blocking fashion as long as the event loop
implementation has a poll()-like call before calling processEvents(). The
processEvents() implementation could even be completely empty in that case.
When we will implement support for threads this will be different, the thread
event dispatcher will need to integrate with our internal thread loop, and
thus offer a blocking processEvents().
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EVENT_DISPATCHER_H__ */
> > diff --git a/include/libcamera/event_notifier.h
> > b/include/libcamera/event_notifier.h new file mode 100644
> > index 000000000000..278a5ff6f9e5
> > --- /dev/null
> > +++ b/include/libcamera/event_notifier.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_notifer.h - Event notifier
> > + */
> > +#ifndef __LIBCAMERA_EVENT_NOTIFIER_H__
> > +#define __LIBCAMERA_EVENT_NOTIFIER_H__
> > +
> > +#include <libcamera/signal.h>
> > +
> > +namespace libcamera {
> > +
> > +class EventNotifier
> > +{
> > +public:
> > + enum Type {
> > + Read,
> > + Write,
> > + Exception,
> > + };
> > +
> > + EventNotifier(int fd, Type type);
> > + virtual ~EventNotifier();
> > +
> > + Type type() const { return type_; }
> > + int fd() const { return fd_; }
> > +
> > + bool isEnabled() const { return enabled_; }
>
> We have used the 'enabled()' syntax so far for other getters in the
> library. Up to you.
I'll change this, consistency is important.
> > + void setEnabled(bool enable);
> > +
> > + Signal<EventNotifier *> activated;
> > +
> > +private:
> > + int fd_;
> > + Type type_;
> > + bool enabled_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EVENT_NOTIFIER_H__ */
[snip]
> > diff --git a/src/libcamera/event_dispatcher.cpp
> > b/src/libcamera/event_dispatcher.cpp new file mode 100644
> > index 000000000000..61013f218887
> > --- /dev/null
> > +++ b/src/libcamera/event_dispatcher.cpp
[snip]
> > +/**
> > + * \fn EventDispatcher::registerTimer()
> > + * \brief Register a timer
> > + * \param timer The timer to register
> > + *
> > + * Once the \a timer is registered with the dispatcher, the dispatcher
> > will emit + * the timer \ref Timer::timeout signal when the timer times
> > out. The timer can + * be unregistered with unregisterTimer() before it
> > times out, in which case the + * signal will not be emitted.
> > + *
> > + * When the \a timer times out, it is automatically unregistered by the
> > + * dispatcher and can be registered back as early as from the \ref
> > Timer::timeout
>
> Is 'back as early as' intentional?
Yes, "back" refers to "registered back", and the timer can be registered back
at any point, as early as from the Timer::timeout signal handlers. This points
out that the timer can be registered back from the signal handler, and that
the event dispatcher implementation must support that. This is why the
EventDispatcherPoll::processTimers() function uses a while (!timers_.empty())
loop, as the usual range-based for (auto timer : timers_) can't be used if we
invalidate the iterator from within the loop (which can be the case if timers
are added from the signal handler).
> > + * signal handlers.
> > + *
> > + * Registering the same timer multiple times is not allowed.
> > + */
[snip]
> > diff --git a/src/libcamera/event_notifier.cpp
> > b/src/libcamera/event_notifier.cpp new file mode 100644
> > index 000000000000..edaf66725dc5
> > --- /dev/null
> > +++ b/src/libcamera/event_notifier.cpp
> > @@ -0,0 +1,118 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_notifier.cpp - Event notifier
>
> This is actually a 'file events' notifier, it's probably worth
> mentioning it somewhere
I'll change it to "File descriptor event notifier".
> > + */
> > +
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/event_dispatcher.h>
> > +#include <libcamera/event_notifier.h>
> > +
> > +/**
> > + * \file event_notifier.h
> > + * \brief Event notifier
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class EventNotifier
> > + * \brief Notify of activity on a file descriptor
> > + *
>
> Ah, right :)
>
> > + * The EventNotifier models a file descriptor event source that can be
> > + * monitored. It is created with the file descriptor to be monitored and
> > the + * type of event, and is enabled by default. It will emit the \ref
> > activated + * signal whenever an event of the monitored type occurs on
> > the file descriptor. + *
> > + * Supported type of events are EventNotifier::Read, EventNotifier::Write
> > and + * EventNotifier::Exception. The type is specified when constructed
> > the
>
> When the notifier is constructed
I meant to say "when constructing the notifier". Fixed.
> > + * notifier, and can be retrieved using the type() function. To listen to
> > + * multiple event types on the same file descriptor multiple notifiers
> > must be + * created.
> > + *
> > + * The notifier can be disabled with the setEnable() function. When the
> > notifier + * is disabled it ignores events and does not emit the \ref
> > activated signal. + * The notifier can then be re-enabled with the
> > setEnable() function. + *
> > + * Notifier events are detected and dispatched from the
> > + * EventDispatcher::processEvents() function.
> > + */
[snip]
> > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > new file mode 100644
> > index 000000000000..57c49aa2ef36
> > --- /dev/null
> > +++ b/src/libcamera/timer.cpp
> > @@ -0,0 +1,105 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * timer.cpp - Generic timer
> > + */
> > +
> > +#include <time.h>
> > +
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/event_dispatcher.h>
> > +#include <libcamera/timer.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file timer.h
> > + * \brief Generic timer
>
> Maybe it is just me not having the Signal/Slot thing clear enough, but
> it would help mentioning that library components wanting to use timers
> (and even notifiers) should instantiate an instance of this class and
> connect the timeout (and activated) signals to the slots they want to be
> called upon events.
The documentation for EventNotifier already states that it "will emit the \ref
activated signal whenever an event of the monitored type occurs on the file
descriptor". And the Timer "class models a single-shot timer that is started
with start() and emits the \ref timeout signal when it times out". What do you
think is missing ?
> If this is specified somewhere else you can ignore this. If this is
> assumed to be known by how Signal/Slot works, maybe it is worth
> specifying it here for people not familiar with the concept.
>
> I'm now thinking how I would do that without Signals/Slots, and I
> would expect that I would create a Timer and register a timeout
> with a callback to be invoked at the timer expiration, and that's basically
> it. This can of course be built with the nice Signal/Slot implementation,
> but shouldn't we abstract this away to Timer (and EventNotifier) users
> providing a method to register a callback with an associated timeout?
> What are the advantages of having signal and slot connections exposed?
Signals and slots *are* the abstraction layer. They offer connection handling
(both the EventNotifier and Timer class would otherwise need to implement a
callback registration system that stores the callback in a list), ease slot
implementation without boilerplate code (otherwise you would likely have to
pass both the object pointer as a void pointer and a static method pointer to
the callback registration, and implement a static callback method that cast
the void pointer to the appropriate class pointer and calls the corresponding
class method, and this for every method of every object that can be used as a
callback), and offer compilation-time type safety.
Compare the following two implementations:
------------------------------- Without signal ------------------------------
class Emitter
{
public:
struct Callback {
void *obj;
void(*func)(int value);
};
~Emitter()
{
for (Callback *callback : callback_)
delete callback;
}
void registerValueChangedCallback(void *obj, void(*func)(int value))
{
callbacks_.push_back(new Callback{obj, func});
}
void unregisterValueChangedCallback(void *obj, void(*func)(int value))
{
/* Exercise let for the reader */
}
void changeValue(int value)
{
value_ = value;
/*
* This is flawed as it doesn't handle registration and
* unregistration of callbacks from within the callback
* functions. A more complex implementation is needed.
*/
foreach (Callback *callback : callback_)
callback->func(callback->obj, value);
}
private:
int value_;
std::list<Callback *> callbacks_;
};
class Observer;
static void static_value_changed_callback(void *obj, int value)
{
static_cast<Observer *>(obj)->valueChanged(value);
}
class Observer
{
public:
void valueChanged(int value) { ... };
void listen(Emitter *e)
{
e->registerValueChangedCallback(static_cast<void *>(this),
static_value_changed_callback);
}
};
-----------------------------------------------------------------------------
-------------------------------- With signal ---------------------------------
class Emitter
{
public:
void changeValue(int value)
{
value_ = value;
valueChanged.emit(value);
}
Signal<int> valueChanged;
private:
int value_;
};
class Observer
{
public:
void valueChanged(int value) { ... };
void listen(Emitter *e)
{
e->valueChanged.connect(this, &Observer::valueChanged);
}
};-----------------------------------------------------------------------------
Which one do you like best ? :-)
> > + */
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list