[libcamera-devel,13/18] test: Add EventNotifier thread move test
diff mbox series

Message ID 20190812124642.24287-14-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Object & Thread enhancements
Related show

Commit Message

Laurent Pinchart Aug. 12, 2019, 12:46 p.m. UTC 
The test verifies correct behaviour of an enabled event notifier moved
to a different thread.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/event_notifier.cpp |   2 +-
 test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
 test/meson.build                 |   1 +
 3 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 test/event-thread.cpp

Comments

Jacopo Mondi Aug. 15, 2019, 9:50 a.m. UTC | #1 
HI Laurent,

On Mon, Aug 12, 2019 at 03:46:37PM +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>
> ---
>  src/libcamera/event_notifier.cpp |   2 +-
>  test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
>  test/meson.build                 |   1 +
>  3 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 test/event-thread.cpp
>
> diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> index 515e6d1770a1..96be27601982 100644
> --- a/src/libcamera/event_notifier.cpp
> +++ b/src/libcamera/event_notifier.cpp
> @@ -127,7 +127,7 @@ void EventNotifier::message(Message *msg)
>  	if (msg->type() == Message::ThreadMoveMessage) {
>  		if (enabled_) {
>  			setEnabled(false);
> -			invokeMethod(this, &EventNotifier::setEnabled, true);
> +			invokeMethod(&EventNotifier::setEnabled, true);

I've missed the reason for this change. Also, isnt't this the syntax
to invoce static bound methods?

>  		}
>  	}
>
> 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()
> +	{
> +		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);
> +
> +		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
Laurent Pinchart Aug. 15, 2019, 9:56 a.m. UTC | #2 
Hi Jacopo,

On Thu, Aug 15, 2019 at 11:50:15AM +0200, Jacopo Mondi wrote:
> On Mon, Aug 12, 2019 at 03:46:37PM +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>
> > ---
> >  src/libcamera/event_notifier.cpp |   2 +-
> >  test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
> >  test/meson.build                 |   1 +
> >  3 files changed, 114 insertions(+), 1 deletion(-)
> >  create mode 100644 test/event-thread.cpp
> >
> > diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> > index 515e6d1770a1..96be27601982 100644
> > --- a/src/libcamera/event_notifier.cpp
> > +++ b/src/libcamera/event_notifier.cpp
> > @@ -127,7 +127,7 @@ void EventNotifier::message(Message *msg)
> >  	if (msg->type() == Message::ThreadMoveMessage) {
> >  		if (enabled_) {
> >  			setEnabled(false);
> > -			invokeMethod(this, &EventNotifier::setEnabled, true);
> > +			invokeMethod(&EventNotifier::setEnabled, true);
> 
> I've missed the reason for this change. Also, isnt't this the syntax
> to invoce static bound methods?

This should be part of "libcamera: object: Add an asynchronous method
invocation method", I'll move it there. invokeMethod() used to be a
static method, hence the first argument. I've changed it to a member
method and this is a leftover.

> >  		}
> >  	}
> >
> > 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()
> > +	{
> > +		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);
> > +
> > +		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'],
Laurent Pinchart Aug. 15, 2019, 9:58 a.m. UTC | #3 
On Thu, Aug 15, 2019 at 12:56:02PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Thu, Aug 15, 2019 at 11:50:15AM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 12, 2019 at 03:46:37PM +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>
> > > ---
> > >  src/libcamera/event_notifier.cpp |   2 +-
> > >  test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
> > >  test/meson.build                 |   1 +
> > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > >  create mode 100644 test/event-thread.cpp
> > >
> > > diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> > > index 515e6d1770a1..96be27601982 100644
> > > --- a/src/libcamera/event_notifier.cpp
> > > +++ b/src/libcamera/event_notifier.cpp
> > > @@ -127,7 +127,7 @@ void EventNotifier::message(Message *msg)
> > >  	if (msg->type() == Message::ThreadMoveMessage) {
> > >  		if (enabled_) {
> > >  			setEnabled(false);
> > > -			invokeMethod(this, &EventNotifier::setEnabled, true);
> > > +			invokeMethod(&EventNotifier::setEnabled, true);
> > 
> > I've missed the reason for this change. Also, isnt't this the syntax
> > to invoce static bound methods?
> 
> This should be part of "libcamera: object: Add an asynchronous method
> invocation method", I'll move it there. invokeMethod() used to be a
> static method, hence the first argument. I've changed it to a member
> method and this is a leftover.

"libcamera: object: Add an asynchronous method invocation method" isn't
the right patch. Looks like I've already reorganised this in my private
branch anyway, so it will be moved to the right place in the next
version.

> > >  		}
> > >  	}
> > >
> > > 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()
> > > +	{
> > > +		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);
> > > +
> > > +		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
Jacopo Mondi Aug. 15, 2019, 10:12 a.m. UTC | #4 
Hi Laurent,

On Thu, Aug 15, 2019 at 12:56:02PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Aug 15, 2019 at 11:50:15AM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 12, 2019 at 03:46:37PM +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>
> > > ---
> > >  src/libcamera/event_notifier.cpp |   2 +-
> > >  test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
> > >  test/meson.build                 |   1 +
> > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > >  create mode 100644 test/event-thread.cpp
> > >
> > > diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> > > index 515e6d1770a1..96be27601982 100644
> > > --- a/src/libcamera/event_notifier.cpp
> > > +++ b/src/libcamera/event_notifier.cpp
> > > @@ -127,7 +127,7 @@ void EventNotifier::message(Message *msg)
> > >  	if (msg->type() == Message::ThreadMoveMessage) {
> > >  		if (enabled_) {
> > >  			setEnabled(false);
> > > -			invokeMethod(this, &EventNotifier::setEnabled, true);
> > > +			invokeMethod(&EventNotifier::setEnabled, true);
> >
> > I've missed the reason for this change. Also, isnt't this the syntax
> > to invoce static bound methods?
>
> This should be part of "libcamera: object: Add an asynchronous method
> invocation method", I'll move it there. invokeMethod() used to be a
> static method, hence the first argument. I've changed it to a member
> method and this is a leftover.
>

Not sure I follow here. The line of code you have here changed has
been introduced by 11/18 but you say this change should be part of
6/18 ?

In general I thought the first argument of invokeMethod should be a
pointer to instance the method should be invoked on to provide a
typename to 'obj'

I'm most probably not reading this right (from 6/18)

 +       template<typename T, typename... Args, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>
 +       void invokeMethod(void (T::*func)(Args...), Args... args)
 +       {
 +               T *obj = static_cast<T *>(this);
 +               BoundMethodBase *method = new BoundMemberMethod<T, Args...>(obj, this, func);
 +               void *pack = new typename BoundMemberMethod<T, Args...>::PackType{ args... };
 +
 +               invokeMethod(method, pack);
 +       }


> > >  		}
> > >  	}
> > >
> > > 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()
> > > +	{
> > > +		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);
> > > +
> > > +		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
Jacopo Mondi Aug. 15, 2019, 10:13 a.m. UTC | #5 
Hi Laurent,

On Thu, Aug 15, 2019 at 12:58:48PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 15, 2019 at 12:56:02PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Thu, Aug 15, 2019 at 11:50:15AM +0200, Jacopo Mondi wrote:
> > > On Mon, Aug 12, 2019 at 03:46:37PM +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>
> > > > ---
> > > >  src/libcamera/event_notifier.cpp |   2 +-
> > > >  test/event-thread.cpp            | 112 +++++++++++++++++++++++++++++++
> > > >  test/meson.build                 |   1 +
> > > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > > >  create mode 100644 test/event-thread.cpp
> > > >
> > > > diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> > > > index 515e6d1770a1..96be27601982 100644
> > > > --- a/src/libcamera/event_notifier.cpp
> > > > +++ b/src/libcamera/event_notifier.cpp
> > > > @@ -127,7 +127,7 @@ void EventNotifier::message(Message *msg)
> > > >  	if (msg->type() == Message::ThreadMoveMessage) {
> > > >  		if (enabled_) {
> > > >  			setEnabled(false);
> > > > -			invokeMethod(this, &EventNotifier::setEnabled, true);
> > > > +			invokeMethod(&EventNotifier::setEnabled, true);
> > >
> > > I've missed the reason for this change. Also, isnt't this the syntax
> > > to invoce static bound methods?
> >
> > This should be part of "libcamera: object: Add an asynchronous method
> > invocation method", I'll move it there. invokeMethod() used to be a
> > static method, hence the first argument. I've changed it to a member
> > method and this is a leftover.
>
> "libcamera: object: Add an asynchronous method invocation method" isn't
> the right patch. Looks like I've already reorganised this in my private
> branch anyway, so it will be moved to the right place in the next
> version.
>

I see. Please be aware that a similar hunk (but for timers) is present
in the following patch (14/18)

> > > >  		}
> > > >  	}
> > > >
> > > > 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()
> > > > +	{
> > > > +		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);
> > > > +
> > > > +		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
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
index 515e6d1770a1..96be27601982 100644
--- a/src/libcamera/event_notifier.cpp
+++ b/src/libcamera/event_notifier.cpp
@@ -127,7 +127,7 @@  void EventNotifier::message(Message *msg)
 	if (msg->type() == Message::ThreadMoveMessage) {
 		if (enabled_) {
 			setEnabled(false);
-			invokeMethod(this, &EventNotifier::setEnabled, true);
+			invokeMethod(&EventNotifier::setEnabled, true);
 		}
 	}
 
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()
+	{
+		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);
+
+		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'],