[libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot communication mechanism
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 7 19:28:54 CET 2019
Hi Laurent,
On Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Monday, 7 January 2019 18:15:58 EET 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
>
> [snip]
>
> > > 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);
>
> This may compile, but isn't right. *iter may not be of type Slot<T, Args...>
> *, it may point to a Slot with a different object type T than the one this
> function has been called with. While I may still work, casting a pointer to a
> potentially incorrect type and then dereferencing it would worry me.
Right, the 'T's might be different :)
It's probably not a big deal here, as you only access obj_ which is
provided by the base class, but it's defintely an issue when accessing
func_
Anyway, doesn't this line suffer from the same problem?
reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func)
That T might be a different one from the one this function has been
called with. Actually the check for (slot->obj_ == object) might
protect us from actually accessing anything wrong, so the issue is
only potential...
>
> > if (slot->obj_ != object)
> > ++iter;
> >
> > 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?
>
> I don't expect the same signal to be connected to the same slot multiple
> times, but if it is, I think this function should delete all connections
>
No break then, this is fine.
> > }
> > }
> >
> > > + }
> > > +
> > > + 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__ */
>
> [snip]
>
> > > 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.
>
> I agree more documentation would be nice, I'll try to expand a bit, but I
> don't have time now to write dozens of pages about this concept :-)
>
> > > + */
> > > +
> > > +/**
> > > + * \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.
>
> I'll explain the term slot in the class documentation.
>
Thanks
> > > + */
> > > +
> > > +/**
> > > + * \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?
>
> Should we document the order, or leave it as implementation-defined ? They are
> currently called in the order they are connected, do you think it could cause
> a problem later t guarantee that ? Or would it make the API less usable if we
> don't guarantee the order ?
>
I think it's worth mentioning the calling order is the registration
one. If a class derives the Signal one to implement, say, priorities
when calling slots, this shall be documented even more, otherwise the
here provided base Signal class provides that already.
Thanks
j
> > > + */
> > > +
> > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
-------------- 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/96fa3cc1/attachment.sig>
More information about the libcamera-devel
mailing list