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

1831 diff mbox series

The test verifies correct behaviour of an enabled event notifier moved
to a different thread.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 test/event-thread.cpp | 112 ++++++++++++++++++++++++++++++++++++++++++
 test/meson.build      |   1 +
 2 files changed, 113 insertions(+)
 create mode 100644 test/event-thread.cpp

Hi Laurent,

Thanks for your work.

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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@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.

> +		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.

With the initialization of notified_ fixed and with or without a comment,

Reviewed-by: Niklas Söderlund <niklas.soderlund@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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@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@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'],