[libcamera-devel] [PATCH v2 13/18] test: Add EventNotifier thread move test

[Laurent Pinchart]

Hi Niklas,

On Sat, Aug 17, 2019 at 05:38:37PM +0200, Niklas Söderlund wrote:
> On 2019-08-17 18:20:59 +0300, Laurent Pinchart wrote:
> > The test verifies correct behaviour of an enabled event notifier moved
> > to a different thread.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  test/event-thread.cpp | 112 ++++++++++++++++++++++++++++++++++++++++++
> >  test/meson.build      |   1 +
> >  2 files changed, 113 insertions(+)
> >  create mode 100644 test/event-thread.cpp
> > 
> > diff --git a/test/event-thread.cpp b/test/event-thread.cpp
> > new file mode 100644
> > index 000000000000..4a82d49b94f1
> > --- /dev/null
> > +++ b/test/event-thread.cpp
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event-thread.cpp - Threaded event test
> > + */
> > +
> > +#include <chrono>
> > +#include <iostream>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/event_notifier.h>
> > +#include <libcamera/timer.h>
> > +
> > +#include "test.h"
> > +#include "thread.h"
> > +
> > +using namespace std;
> > +using namespace libcamera;
> > +
> > +class EventHandler : public Object
> > +{
> > +public:
> > +	EventHandler()
> > +	{
> 
> I think you should set notified_ to false here.

Good point.

> > +		pipe(pipefd_);
> > +
> > +		notifier_ = new EventNotifier(pipefd_[0], EventNotifier::Read);
> > +		notifier_->activated.connect(this, &EventHandler::readReady);
> > +	}
> > +
> > +	~EventHandler()
> > +	{
> > +		delete notifier_;
> > +
> > +		close(pipefd_[0]);
> > +		close(pipefd_[1]);
> > +	}
> > +
> > +	int notify()
> > +	{
> > +		std::string data("H2G2");
> > +		ssize_t ret;
> > +
> > +		memset(data_, 0, sizeof(data_));
> > +		size_ = 0;
> > +
> > +		ret = write(pipefd_[1], data.data(), data.size());
> > +		if (ret < 0) {
> > +			cout << "Pipe write failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	bool notified() const
> > +	{
> > +		return notified_;
> > +	}
> > +
> > +	void moveToThread(Thread *thread)
> > +	{
> > +		Object::moveToThread(thread);
> > +		notifier_->moveToThread(thread);
> > +	}
> > +
> > +private:
> > +	void readReady(EventNotifier *notifier)
> > +	{
> > +		size_ = read(notifier->fd(), data_, sizeof(data_));
> > +		notified_ = true;
> > +	}
> > +
> > +	EventNotifier *notifier_;
> > +
> > +	int pipefd_[2];
> > +
> > +	bool notified_;
> > +	char data_[16];
> > +	ssize_t size_;
> > +};
> > +
> > +class EventThreadTest : public Test
> > +{
> > +protected:
> > +	int run()
> > +	{
> > +		Thread thread;
> > +		thread.start();
> > +
> > +		EventHandler handler;
> > +		handler.notify();
> > +		handler.moveToThread(&thread);
> 
> I had to think twice when reading this. Maybe add comment to clarify 
> that it's OK to generate the event first and then move it as the main 
> thread don't process events it's only possible for the event to be 
> processed in a different thread.
> 
> Or maybe I just have to get used to this API to make it apparent that 
> this is the only possibility.

No, you have a good point. I'll add this comment.

                /*
                 * Fire the event notifier and then move the notifier to a
                 * different thread. The notifier will not notice the event
                 * immediately as there is no event dispatcher loop running in
                 * the main thread. This tests that a notifier being moved to a
                 * different thread will correctly process already pending
                 * events in the new thread.
                 */

> With the initialization of notified_ fixed and with or without a comment,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > +
> > +		this_thread::sleep_for(chrono::milliseconds(100));
> > +
> > +		/* Must stop thread before destroying the handler. */
> > +		thread.exit(0);
> > +		thread.wait();
> > +
> > +		if (!handler.notified()) {
> > +			cout << "Thread event handling test failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +};
> > +
> > +TEST_REGISTER(EventThreadTest)
> > diff --git a/test/meson.build b/test/meson.build
> > index c6601813db78..f695ffd7be44 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -23,6 +23,7 @@ public_tests = [
> >  
> >  internal_tests = [
> >      ['camera-sensor',                   'camera-sensor.cpp'],
> > +    ['event-thread',                    'event-thread.cpp'],
> >      ['message',                         'message.cpp'],
> >      ['object',                          'object.cpp'],
> >      ['object-invoke',                   'object-invoke.cpp'],

-- 
Regards,

Laurent Pinchart