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

[Niklas Söderlund]

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

> +		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 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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund