[libcamera-devel] [PATCH 06/11] libcamera: Add event notification infrastructure

[Jacopo Mondi]

Hi Laurent,
   not sure it is worth replying, since v2 is out, but since you
spent time writing this....

On Mon, Jan 07, 2019 at 10:25:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

> On Monday, 7 January 2019 20:14:21 EET Jacopo Mondi wrote:
> > > 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 ?
>

It was not clear to me that the user of a timer should have had
connected the Timer signal to its own slot. Again, it might just be my
poor understanding of this.

> > 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 ? :-)

Oh, the Signal one, absolutely.

I was just wondering if the timer class should expose something like

        Timer::registerCallback(void *, void(*func)(...))

and deal internally with the connection to the slot. It is probably
not worth going through all those void * casts, and I'm not sure this
even works. So please ditch my comment.

Thanks
  j

>
> > > + */
>
> [snip]
>
> --
> 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/20190108/cb02b460/attachment.sig>