[{"id":17550,"web_url":"https://patchwork.libcamera.org/comment/17550/","msgid":"","date":"2021-06-15T03:33:28","subject":"Re: [libcamera-devel] [PATCH v7 3/5] lc-compliance: Add Environment\n\tsingleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Thu, Jun 10, 2021 at 03:37:30PM -0300, Nícolas F. R. A. Prado wrote:\n> Add a singleton Environment class in order to make the camera available\n> inside all tests. This is needed for the Googletest refactor, otherwise\n> the tests, which are statically declared, won't be able to access the\n> camera.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> Changes in v7:\n> - Thanks to Jacopo:\n> - Fixed style issue\n> - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n> \n> Changes in v6:\n> - Thanks to Niklas:\n> - Made cameraId() return const& \n> \n> Changes in v5:\n> - Thanks to Laurent:\n> - Stored CameraManager and cameraId instead of Camera in Environment\n> - Thanks to Laurent & Niklas:\n> - Improved Environment singleton class instantiation and destruction\n> \n> Added in v4\n> \n> src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++\n> src/lc-compliance/environment.h | 31 +++++++++++++++++++++++++++++++\n> src/lc-compliance/meson.build | 1 +\n> 3 files changed, 52 insertions(+)\n> create mode 100644 src/lc-compliance/environment.cpp\n> create mode 100644 src/lc-compliance/environment.h\n> \n> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp\n> new file mode 100644\n> index 000000000000..3a539848362f\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.cpp\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.cpp - Common environment for tests\n> + */\n> +\n> +#include \"environment.h\"\n> +\n> +Environment *Environment::get()\n> +{\n> +\tstatic Environment instance;\n> +\treturn &instance;\n> +}\n> +\n> +void Environment::setup(CameraManager* cm, std::string cameraId)\n> +{\n> +\tcm_ = cm;\n> +\tcameraId_ = cameraId;\n> +}\n> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h\n> new file mode 100644\n> index 000000000000..614eb2777800\n> --- /dev/null\n> +++ b/src/lc-compliance/environment.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Collabora Ltd.\n> + *\n> + * environment.h - Common environment for tests\n> + */\n> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__\n> +#define __LC_COMPLIANCE_ENVIRONMENT_H__\n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +class Environment\n> +{\n> +public:\n> +\tstatic Environment *get();\n> +\n> +\tvoid setup(CameraManager* cm, std::string cameraId);\n> +\n> +\tconst std::string &cameraId() const { return cameraId_; }\n> +\tCameraManager* cm() const { return cm_; }\n> +\n> +private:\n> +\tEnvironment() {};\n\nThis breaks compilation with clang:\n\n../../src/lc-compliance/environment.h:25:18: error: extra ';' after member function definition [-Werror,-Wextra-semi]\n Environment() {};\n ^\n1 error generated.\n\nI'd use\n\n\tEnvironment() = default;\n\nto generate a private default constructor, instead of creating one\nmanually.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\tstd::string cameraId_;\n> +\tCameraManager* cm_;\n> +};\n> +\n> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index a2bfcceb1259..6dd49085569f 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -12,6 +12,7 @@ lc_compliance_enabled = true\n> lc_compliance_sources = files([\n> '../cam/event_loop.cpp',\n> '../cam/options.cpp',\n> + 'environment.cpp',\n> 'main.cpp',\n> 'results.cpp',\n> 'simple_capture.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BD93CC3218\n\tfor ;\n\tTue, 15 Jun 2021 03:33:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3ADCF687EF;\n\tTue, 15 Jun 2021 05:33:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A3766050D\n\tfor ;\n\tTue, 15 Jun 2021 05:33:49 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73F76436;\n\tTue, 15 Jun 2021 05:33:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"d6iZmpTi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623728028;\n\tbh=HbpaoiINB0QN3bS8v7ftccU1mOF4RQpZbcDNge0SGlk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d6iZmpTidMsfzwABVbLREYmdOb4TZc/+WPKSmkE7LtkYdjSMbXkEV+wPnBXdk9dTM\n\tBvV5E5f3uiDs6YE11ATRReUBA6hVZ932JiUwRnk3DYGmb712SEyyC72D9G9jcYk6uC\n\to7X3ffgaGVQWnWy/4La0LTuMNbHLmM8yuz3z+JQ4=","Date":"Tue, 15 Jun 2021 06:33:28 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210610183732.174697-1-nfraprado@collabora.com>\n\t<20210610183732.174697-4-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210610183732.174697-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v7 3/5] lc-compliance: Add Environment\n\tsingleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17559,"web_url":"https://patchwork.libcamera.org/comment/17559/","msgid":"","date":"2021-06-15T09:17:21","subject":"Re: [libcamera-devel] [PATCH v7 3/5] lc-compliance: Add Environment\n\tsingleton","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 15/06/2021 04:33, Laurent Pinchart wrote:\n> Hi Nícolas,\n> \n> Thank you for the patch.\n> \n> On Thu, Jun 10, 2021 at 03:37:30PM -0300, Nícolas F. R. A. Prado wrote:\n>> Add a singleton Environment class in order to make the camera available\n>> inside all tests. This is needed for the Googletest refactor, otherwise\n>> the tests, which are statically declared, won't be able to access the\n>> camera.\n>>\n>> Signed-off-by: Nícolas F. R. A. Prado \n>> Reviewed-by: Niklas Söderlund \n>> Reviewed-by: Jacopo Mondi \n>> ---\n>> Changes in v7:\n>> - Thanks to Jacopo:\n>> - Fixed style issue\n>> - Made CameraManager a unique_ptr and passed to Environment as raw pointer\n>>\n>> Changes in v6:\n>> - Thanks to Niklas:\n>> - Made cameraId() return const& \n>>\n>> Changes in v5:\n>> - Thanks to Laurent:\n>> - Stored CameraManager and cameraId instead of Camera in Environment\n>> - Thanks to Laurent & Niklas:\n>> - Improved Environment singleton class instantiation and destruction\n>>\n>> Added in v4\n>>\n>> src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++\n>> src/lc-compliance/environment.h | 31 +++++++++++++++++++++++++++++++\n>> src/lc-compliance/meson.build | 1 +\n>> 3 files changed, 52 insertions(+)\n>> create mode 100644 src/lc-compliance/environment.cpp\n>> create mode 100644 src/lc-compliance/environment.h\n>>\n>> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp\n>> new file mode 100644\n>> index 000000000000..3a539848362f\n>> --- /dev/null\n>> +++ b/src/lc-compliance/environment.cpp\n>> @@ -0,0 +1,20 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Collabora Ltd.\n>> + *\n>> + * environment.cpp - Common environment for tests\n>> + */\n>> +\n>> +#include \"environment.h\"\n>> +\n>> +Environment *Environment::get()\n>> +{\n>> +\tstatic Environment instance;\n>> +\treturn &instance;\n>> +}\n>> +\n>> +void Environment::setup(CameraManager* cm, std::string cameraId)\n>> +{\n>> +\tcm_ = cm;\n>> +\tcameraId_ = cameraId;\n>> +}\n>> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h\n>> new file mode 100644\n>> index 000000000000..614eb2777800\n>> --- /dev/null\n>> +++ b/src/lc-compliance/environment.h\n>> @@ -0,0 +1,31 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Collabora Ltd.\n>> + *\n>> + * environment.h - Common environment for tests\n>> + */\n>> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__\n>> +#define __LC_COMPLIANCE_ENVIRONMENT_H__\n>> +\n>> +#include \n>> +\n>> +using namespace libcamera;\n>> +\n>> +class Environment\n>> +{\n>> +public:\n>> +\tstatic Environment *get();\n>> +\n>> +\tvoid setup(CameraManager* cm, std::string cameraId);\n\nThere are some checkstyle failures in here too. CameraManager *cm\ninsteaad of CameraManager* cm...\n\nplease install checkstyle as a post-commit hook to always get\nnotifcations about these.\n\n cp utils/hooks/post-commit .git/hooks\n\n\n>> +\n>> +\tconst std::string &cameraId() const { return cameraId_; }\n>> +\tCameraManager* cm() const { return cm_; }\n>> +\n>> +private:\n>> +\tEnvironment() {};\n> \n> This breaks compilation with clang:\n> \n> ../../src/lc-compliance/environment.h:25:18: error: extra ';' after member function definition [-Werror,-Wextra-semi]\n> Environment() {};\n> ^\n> 1 error generated.\n> \n> I'd use\n> \n> \tEnvironment() = default;\n> \n> to generate a private default constructor, instead of creating one\n> manually.\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\n>> +\tstd::string cameraId_;\n>> +\tCameraManager* cm_;\n>> +};\n>> +\n>> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */\n>> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n>> index a2bfcceb1259..6dd49085569f 100644\n>> --- a/src/lc-compliance/meson.build\n>> +++ b/src/lc-compliance/meson.build\n>> @@ -12,6 +12,7 @@ lc_compliance_enabled = true\n>> lc_compliance_sources = files([\n>> '../cam/event_loop.cpp',\n>> '../cam/options.cpp',\n>> + 'environment.cpp',\n>> 'main.cpp',\n>> 'results.cpp',\n>> 'simple_capture.cpp',\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2DD34BD78E\n\tfor ;\n\tTue, 15 Jun 2021 09:17:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D42D68944;\n\tTue, 15 Jun 2021 11:17:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A22646892D\n\tfor ;\n\tTue, 15 Jun 2021 11:17:25 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0773D436;\n\tTue, 15 Jun 2021 11:17:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fuLKzcPF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623748645;\n\tbh=JwLgOHKfUtY8WvPkVG32SCids3eku3bXsZGHfXPEk+Y=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=fuLKzcPFlPAKO9+5R6KLBi0/LmvrtPRLwUZdUQxXeZQKgfmTHnYAECi+RRBoMNADF\n\tUn+m+uCPgxLtmpnNQ4qCrA6mTh6nr5FGX8hr0hlF7wiNHG3XaMyRqbdY/e4n+Le1VH\n\t6txqvTb90tEflaIvtrPzFv+U6PSX9o5KD1YbN2vA=","To":"Laurent Pinchart , =?utf-8?b?TsOt?=\n\t=?utf-8?q?colas_F=2E_R=2E_A=2E_Prado?= ","References":"<20210610183732.174697-1-nfraprado@collabora.com>\n\t<20210610183732.174697-4-nfraprado@collabora.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 15 Jun 2021 10:17:21 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v7 3/5] lc-compliance: Add Environment\n\tsingleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]