[libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot communication mechanism
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 7 17:25:17 CET 2019
On Mon, Jan 07, 2019 at 05:15:58PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
> a few more things I have noticed while staring at path 6/11..
>
> On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> > Introduce a Signal class that allows connecting event sources (signals)
> > to event listeners (slots) without adding any boilerplate code usually
> > associated with the observer or listener design patterns.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Documentation/Doxyfile.in | 3 +-
> > include/libcamera/meson.build | 1 +
> > include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++
> > src/libcamera/meson.build | 1 +
> > src/libcamera/signal.cpp | 44 +++++++++++++
> > 5 files changed, 165 insertions(+), 1 deletion(-)
> > create mode 100644 include/libcamera/signal.h
> > create mode 100644 src/libcamera/signal.cpp
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index b1a70d36eee5..558a1ce04377 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =
> > # Note that the wildcards are matched against the file with absolute path, so to
> > # exclude all test directories use the pattern */test/*
> >
> > -EXCLUDE_SYMBOLS =
> > +EXCLUDE_SYMBOLS = libcamera::SlotBase \
> > + libcamera::Slot
> >
> > # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> > # that contain example code fragments that are included (see the \include
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 3e04557d66b1..6f87689ea528 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_api = files([
> > 'camera.h',
> > 'camera_manager.h',
> > 'libcamera.h',
> > + 'signal.h',
> > ])
> >
> > install_headers(libcamera_api,
> > diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> > new file mode 100644
> > index 000000000000..fceb852158ec
> > --- /dev/null
> > +++ b/include/libcamera/signal.h
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.h - Signal & slot implementation
> > + */
> > +#ifndef __LIBCAMERA_SIGNAL_H__
> > +#define __LIBCAMERA_SIGNAL_H__
> > +
> > +#include <list>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +template<typename... Args>
> > +class Signal;
> > +
> > +template<typename... Args>
> > +class SlotBase
> > +{
> > +public:
> > + SlotBase(void *obj)
> > + : obj_(obj) { }
> > + virtual ~SlotBase() { }
> > +
> > + virtual void invoke(Args... args) = 0;
> > +
> > +protected:
> > + friend class Signal<Args...>;
> > + void *obj_;
> > +};
> > +
> > +template<typename T, typename... Args>
> > +class Slot : public SlotBase<Args...>
> > +{
> > +public:
> > + Slot(T *obj, void(T::*func)(Args...))
> > + : SlotBase<Args...>(obj), func_(func) { }
> > +
> > + void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> > +
> > +private:
> > + friend class Signal<Args...>;
> > + void(T::*func_)(Args...);
> > +};
> > +
> > +template<typename... Args>
> > +class Signal
> > +{
> > +public:
> > + Signal() { }
> > + ~Signal()
> > + {
> > + for (SlotBase<Args...> *slot : slots_)
> > + delete slot;
> > + }
> > +
> > + template<typename T>
> > + void connect(T *object, void(T::*func)(Args...))
> > + {
> > + slots_.push_back(new Slot<T, Args...>(object, func));
> > + }
> > +
> > + void disconnect()
> > + {
> > + for (SlotBase<Args...> *slot : slots_)
> > + delete slot;
> > + slots_.clear();
> > + }
> > +
> > + template<typename T>
> > + void disconnect(T *object)
> > + {
> > + for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > + SlotBase<Args...> *slot = *iter;
> > + if (slot->obj_ == object) {
> > + iter = slots_.erase(iter);
> > + delete slot;
> > + } else {
> > + ++iter;
> > + }
> > + }
> > + }
> > +
> > + template<typename T>
> > + void disconnect(T *object, void(T::*func)(Args...))
> > + {
> > + for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> > + SlotBase<Args...> *slot = *iter;
>
> I wonder if it wouldn't be cleaner to downcast this to Slot here
> instead of in the condition
>
> > + if (slot->obj_ == object &&
> > + reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> > + iter = slots_.erase(iter);
> > + delete slot;
> > + } else {
> > + ++iter;
> > + }
>
> This one and the above would probably read better as:
>
> template<typename T>
> void disconnect(T *object)
> {
> for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> if (slot->obj_ != object)
> ++iter;
obviosuly missing a continue here
>
> iter = slots_.erase(iter);
> delete slot;
> }
> }
>
> template<typename T>
> void disconnect(T *object, void(T::*func)(Args...))
> {
> for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
> if (slot->obj_ != object || slot->func_ != func) {
> ++ iter;
> continue;
> }
>
> iter = slots_.erase(iter);
> delete slot;
> break; <--- I think you could break here, or
> could the same slot be registered
> twice? In that case, would this
> call delete all of them or just
> the first one?
> }
> }
>
>
> > + }
> > +
> > + void emit(Args... args)
> > + {
> > + /*
> > + * Make a copy of the slots list as the slot could call the
> > + * disconnect operation, invalidating the iterator.
> > + */
> > + std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> > + for (SlotBase<Args...> *slot : slots)
> > + slot->invoke(args...);
> > + }
> > +
> > +private:
> > + std::list<SlotBase<Args...> *> slots_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SIGNAL_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 78562299fc42..3ec86e75b57e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -6,6 +6,7 @@ libcamera_sources = files([
> > 'media_device.cpp',
> > 'media_object.cpp',
> > 'pipeline_handler.cpp',
> > + 'signal.cpp',
> > ])
> >
> > libcamera_headers = files([
> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > new file mode 100644
> > index 000000000000..8b5a6c285c55
> > --- /dev/null
> > +++ b/src/libcamera/signal.cpp
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * signal.cpp - Signal & slot implementation
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class Signal
> > + * \brief Generic signal and slot communication mechanism
> > + *
> > + * Signals and slots are a language construct aimed at communication between
> > + * objects through the observer pattern without the need for boilerplate code.
> > + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.
>
> As much as I could like Qt's signals and slots, I feel for the
> Qt-uneducated ones, the documentation we have here is pretty thin. Mentioning
> the Qt implementation (from which we borrow the concept and the
> terminology) is imho not enough, and a few more words in the functions
> documentation might help. At least, I would have apreciated to have
> them here when i first tried to get my head around this.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::connect()
> > + * \brief Connect the signal to a slot
>
> Slots are not part of the generated documentation, and we rely on the
> Qt definition. I'm not against using slots internally, but or we
> either document them, or we have to be careful introducing terms.o
>
> In example, I would here say that connect() ties a Signal instance to a
> callback \a func, that will be executed on the template \a object
> argument.
>
> Multiple slots can be connected to the same signal, and each slot will
> be executed upon a signal emission, which is triggered by the
> Signal::emit() function.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect()
> > + * \brief Disconnect the signal from all slots
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object)
> > + * \brief Disconnect the signal from all slots of the \a object
> > + */
> > +
> > +/**
> > + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> > + * \brief Disconnect the signal from a slot of the \a object
>
> Feel free to expand these if you think it is useful.
>
> > + */
> > +
> > +/**
> > + * \fn Signal::emit()
> > + * \brief Emit the signal and call all connected slots
>
> "When emit() is called on a Signal instance, the list of connected
> slots is traversed and each one of them is called one after the
> other."
>
> Are there more thing worth being mentioned here, such as the calling
> order and possible conflicts if the same slot is registered more than
> once?
>
>
>
> > + */
> > +
> > +} /* namespace libcamera */
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190107/b1b28c31/attachment.sig>
More information about the libcamera-devel
mailing list