[libcamera-devel,v7,3/5] lc-compliance: Add Environment singleton
diff mbox series

Message ID 20210610183732.174697-4-nfraprado@collabora.com
State Superseded
Headers show
Series
  • lc-compliance: Refactor using Googletest
Related show

Commit Message

Nícolas F. R. A. Prado June 10, 2021, 6:37 p.m. UTC 
Add a singleton Environment class in order to make the camera available
inside all tests. This is needed for the Googletest refactor, otherwise
the tests, which are statically declared, won't be able to access the
camera.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes in v7:
- Thanks to Jacopo:
  - Fixed style issue
  - Made CameraManager a unique_ptr and passed to Environment as raw pointer

Changes in v6:
- Thanks to Niklas:
  - Made cameraId() return const& 

Changes in v5:
- Thanks to Laurent:
  - Stored CameraManager and cameraId instead of Camera in Environment
- Thanks to Laurent & Niklas:
  - Improved Environment singleton class instantiation and destruction

Added in v4

 src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++
 src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++++++
 src/lc-compliance/meson.build     |  1 +
 3 files changed, 52 insertions(+)
 create mode 100644 src/lc-compliance/environment.cpp
 create mode 100644 src/lc-compliance/environment.h

Comments

Laurent Pinchart June 15, 2021, 3:33 a.m. UTC | #1 
Hi Nícolas,

Thank you for the patch.

On Thu, Jun 10, 2021 at 03:37:30PM -0300, Nícolas F. R. A. Prado wrote:
> Add a singleton Environment class in order to make the camera available
> inside all tests. This is needed for the Googletest refactor, otherwise
> the tests, which are statically declared, won't be able to access the
> camera.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> Changes in v7:
> - Thanks to Jacopo:
>   - Fixed style issue
>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
> 
> Changes in v6:
> - Thanks to Niklas:
>   - Made cameraId() return const& 
> 
> Changes in v5:
> - Thanks to Laurent:
>   - Stored CameraManager and cameraId instead of Camera in Environment
> - Thanks to Laurent & Niklas:
>   - Improved Environment singleton class instantiation and destruction
> 
> Added in v4
> 
>  src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++
>  src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++++++
>  src/lc-compliance/meson.build     |  1 +
>  3 files changed, 52 insertions(+)
>  create mode 100644 src/lc-compliance/environment.cpp
>  create mode 100644 src/lc-compliance/environment.h
> 
> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp
> new file mode 100644
> index 000000000000..3a539848362f
> --- /dev/null
> +++ b/src/lc-compliance/environment.cpp
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Collabora Ltd.
> + *
> + * environment.cpp - Common environment for tests
> + */
> +
> +#include "environment.h"
> +
> +Environment *Environment::get()
> +{
> +	static Environment instance;
> +	return &instance;
> +}
> +
> +void Environment::setup(CameraManager* cm, std::string cameraId)
> +{
> +	cm_ = cm;
> +	cameraId_ = cameraId;
> +}
> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
> new file mode 100644
> index 000000000000..614eb2777800
> --- /dev/null
> +++ b/src/lc-compliance/environment.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Collabora Ltd.
> + *
> + * environment.h - Common environment for tests
> + */
> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__
> +#define __LC_COMPLIANCE_ENVIRONMENT_H__
> +
> +#include <libcamera/libcamera.h>
> +
> +using namespace libcamera;
> +
> +class Environment
> +{
> +public:
> +	static Environment *get();
> +
> +	void setup(CameraManager* cm, std::string cameraId);
> +
> +	const std::string &cameraId() const { return cameraId_; }
> +	CameraManager* cm() const { return cm_; }
> +
> +private:
> +	Environment() {};

This breaks compilation with clang:

../../src/lc-compliance/environment.h:25:18: error: extra ';' after member function definition [-Werror,-Wextra-semi]
        Environment() {};
                        ^
1 error generated.

I'd use

	Environment() = default;

to generate a private default constructor, instead of creating one
manually.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	std::string cameraId_;
> +	CameraManager* cm_;
> +};
> +
> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> index a2bfcceb1259..6dd49085569f 100644
> --- a/src/lc-compliance/meson.build
> +++ b/src/lc-compliance/meson.build
> @@ -12,6 +12,7 @@ lc_compliance_enabled = true
>  lc_compliance_sources = files([
>      '../cam/event_loop.cpp',
>      '../cam/options.cpp',
> +    'environment.cpp',
>      'main.cpp',
>      'results.cpp',
>      'simple_capture.cpp',
Kieran Bingham June 15, 2021, 9:17 a.m. UTC | #2 
Hi Nicolas,

On 15/06/2021 04:33, Laurent Pinchart wrote:
> Hi Nícolas,
> 
> Thank you for the patch.
> 
> On Thu, Jun 10, 2021 at 03:37:30PM -0300, Nícolas F. R. A. Prado wrote:
>> Add a singleton Environment class in order to make the camera available
>> inside all tests. This is needed for the Googletest refactor, otherwise
>> the tests, which are statically declared, won't be able to access the
>> camera.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>> Changes in v7:
>> - Thanks to Jacopo:
>>   - Fixed style issue
>>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
>>
>> Changes in v6:
>> - Thanks to Niklas:
>>   - Made cameraId() return const& 
>>
>> Changes in v5:
>> - Thanks to Laurent:
>>   - Stored CameraManager and cameraId instead of Camera in Environment
>> - Thanks to Laurent & Niklas:
>>   - Improved Environment singleton class instantiation and destruction
>>
>> Added in v4
>>
>>  src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++
>>  src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++++++
>>  src/lc-compliance/meson.build     |  1 +
>>  3 files changed, 52 insertions(+)
>>  create mode 100644 src/lc-compliance/environment.cpp
>>  create mode 100644 src/lc-compliance/environment.h
>>
>> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp
>> new file mode 100644
>> index 000000000000..3a539848362f
>> --- /dev/null
>> +++ b/src/lc-compliance/environment.cpp
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2021, Collabora Ltd.
>> + *
>> + * environment.cpp - Common environment for tests
>> + */
>> +
>> +#include "environment.h"
>> +
>> +Environment *Environment::get()
>> +{
>> +	static Environment instance;
>> +	return &instance;
>> +}
>> +
>> +void Environment::setup(CameraManager* cm, std::string cameraId)
>> +{
>> +	cm_ = cm;
>> +	cameraId_ = cameraId;
>> +}
>> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
>> new file mode 100644
>> index 000000000000..614eb2777800
>> --- /dev/null
>> +++ b/src/lc-compliance/environment.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2021, Collabora Ltd.
>> + *
>> + * environment.h - Common environment for tests
>> + */
>> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__
>> +#define __LC_COMPLIANCE_ENVIRONMENT_H__
>> +
>> +#include <libcamera/libcamera.h>
>> +
>> +using namespace libcamera;
>> +
>> +class Environment
>> +{
>> +public:
>> +	static Environment *get();
>> +
>> +	void setup(CameraManager* cm, std::string cameraId);

There are some checkstyle failures in here too. CameraManager *cm
insteaad of CameraManager* cm...

please install checkstyle as a post-commit hook to always get
notifcations about these.

  cp utils/hooks/post-commit .git/hooks


>> +
>> +	const std::string &cameraId() const { return cameraId_; }
>> +	CameraManager* cm() const { return cm_; }
>> +
>> +private:
>> +	Environment() {};
> 
> This breaks compilation with clang:
> 
> ../../src/lc-compliance/environment.h:25:18: error: extra ';' after member function definition [-Werror,-Wextra-semi]
>         Environment() {};
>                         ^
> 1 error generated.
> 
> I'd use
> 
> 	Environment() = default;
> 
> to generate a private default constructor, instead of creating one
> manually.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +
>> +	std::string cameraId_;
>> +	CameraManager* cm_;
>> +};
>> +
>> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
>> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
>> index a2bfcceb1259..6dd49085569f 100644
>> --- a/src/lc-compliance/meson.build
>> +++ b/src/lc-compliance/meson.build
>> @@ -12,6 +12,7 @@ lc_compliance_enabled = true
>>  lc_compliance_sources = files([
>>      '../cam/event_loop.cpp',
>>      '../cam/options.cpp',
>> +    'environment.cpp',
>>      'main.cpp',
>>      'results.cpp',
>>      'simple_capture.cpp',
>

Patch
diff mbox series 

diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp
new file mode 100644
index 000000000000..3a539848362f
--- /dev/null
+++ b/src/lc-compliance/environment.cpp
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2021, Collabora Ltd.
+ *
+ * environment.cpp - Common environment for tests
+ */
+
+#include "environment.h"
+
+Environment *Environment::get()
+{
+	static Environment instance;
+	return &instance;
+}
+
+void Environment::setup(CameraManager* cm, std::string cameraId)
+{
+	cm_ = cm;
+	cameraId_ = cameraId;
+}
diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
new file mode 100644
index 000000000000..614eb2777800
--- /dev/null
+++ b/src/lc-compliance/environment.h
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2021, Collabora Ltd.
+ *
+ * environment.h - Common environment for tests
+ */
+#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__
+#define __LC_COMPLIANCE_ENVIRONMENT_H__
+
+#include <libcamera/libcamera.h>
+
+using namespace libcamera;
+
+class Environment
+{
+public:
+	static Environment *get();
+
+	void setup(CameraManager* cm, std::string cameraId);
+
+	const std::string &cameraId() const { return cameraId_; }
+	CameraManager* cm() const { return cm_; }
+
+private:
+	Environment() {};
+
+	std::string cameraId_;
+	CameraManager* cm_;
+};
+
+#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
index a2bfcceb1259..6dd49085569f 100644
--- a/src/lc-compliance/meson.build
+++ b/src/lc-compliance/meson.build
@@ -12,6 +12,7 @@  lc_compliance_enabled = true
 lc_compliance_sources = files([
     '../cam/event_loop.cpp',
     '../cam/options.cpp',
+    'environment.cpp',
     'main.cpp',
     'results.cpp',
     'simple_capture.cpp',