[{"id":3198,"web_url":"https://patchwork.libcamera.org/comment/3198/","msgid":"<20191206214203.GM28879@bigcity.dyn.berto.se>","date":"2019-12-06T21:42:03","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-12-05 21:43:43 +0100, Jacopo Mondi wrote:\n> Re-use the Control generation infrastructure to generate libcamera properties.\n> \n> Introduce three additional files, one that enumerates the properties ids\n> (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> one for each property (src/libcamera/property_ids.cpp) and provide\n> properties definitions in src/libcamera/property_ids.yaml\n\nThis is also hard for me to parse, how about\n\nIntroduce three additional files,\n\n- include/libcamera/property_ids.h\n Defines the properties ids.\n\n- src/libcamera/property_ids.cpp\n Defines the properties Control<> instances.\n\n- src/libcamera/property_ids.yaml\n Provide properties definitions.\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/meson.build | 26 +++++++++++------\n> include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> src/libcamera/meson.build | 21 ++++++++------\n> src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> 5 files changed, 140 insertions(+), 17 deletions(-)\n> create mode 100644 include/libcamera/property_ids.h.in\n> create mode 100644 src/libcamera/property_ids.cpp.in\n> create mode 100644 src/libcamera/property_ids.yaml\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 99abf0609940..25f503660960 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -23,15 +23,23 @@ install_headers(libcamera_api,\n> \n> gen_controls = files('../../src/libcamera/gen-controls.py')\n> \n> -control_ids_h = custom_target('control_ids_h',\n> - input : files('../../src/libcamera/control_ids.yaml', 'control_ids.h.in'),\n> - output : 'control_ids.h',\n> - depend_files : gen_controls,\n> - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> - install : true,\n> - install_dir : join_paths('include', include_dir))\n> -\n> -libcamera_api += control_ids_h\n> +control_source_files = [\n> + 'control_ids',\n> + 'property_ids',\n> +]\n> +\n> +control_headers = []\n> +\n> +foreach header : control_source_files\n> + input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')\n> + control_headers += custom_target(header + '_h',\n> + input : input_files,\n> + output : header + '.h',\n> + depend_files : gen_controls,\n> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> + install : true,\n> + install_dir : join_paths('include', include_dir))\n> +endforeach\n\nI would have split this commit in two and added the foreach loop in a \nseparate patch.\n\n> \n> gen_header = files('gen-header.sh')\n> \n> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> new file mode 100644\n> index 000000000000..62799b3e8c54\n> --- /dev/null\n> +++ b/include/libcamera/property_ids.h.in\n> @@ -0,0 +1,33 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * property_ids.h : Property ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> +#define __LIBCAMERA_PROPERTY_IDS_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace properties {\n> +\n> +enum {\n> +${ids}\n> +};\n> +\n> +${controls}\n> +\n> +extern const ControlIdMap properties;\n> +\n> +} /* namespace propertiess */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index c4f965bd7413..14aff6e5fc13 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -67,14 +67,19 @@ endif\n> \n> gen_controls = files('gen-controls.py')\n> \n> -control_ids_cpp = custom_target('control_ids_cpp',\n> - input : files('control_ids.yaml', 'control_ids.cpp.in'),\n> - output : 'control_ids.cpp',\n> - depend_files : gen_controls,\n> - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> -\n> -libcamera_sources += control_ids_cpp\n> -libcamera_sources += control_ids_h\n> +control_sources = []\n> +\n> +foreach source : control_source_files\n> + input_files = files(source +'.yaml', source + '.cpp.in')\n> + control_sources += custom_target(source + '_cpp',\n> + input : input_files,\n> + output : source + '.cpp',\n> + depend_files : gen_controls,\n> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> +endforeach\n\nSame here the foreach conversion could be put together with the one \nabove in a separate patch.\n\n> +\n> +libcamera_sources += control_headers\n> +libcamera_sources += control_sources\n> \n> gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> \n> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> new file mode 100644\n> index 000000000000..bfdd823f63b0\n> --- /dev/null\n> +++ b/src/libcamera/property_ids.cpp.in\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * property_ids.cpp : Property ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file property_ids.h\n> + * \\brief Camera property identifiers\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\brief Namespace for libcamera properties\n> + */\n> +namespace properties {\n> +\n> +${controls_doc}\n> +\n> +#ifndef __DOXYGEN__\n> +/*\n> + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> + * them as functions.\n> + */\n> +${controls_def}\n> +#endif\n> +\n> +/**\n> + * \\brief List of all supported libcamera properties\n> + */\n> +extern const ControlIdMap properties {\n> +${controls_map}\n> +};\n> +\n> +} /* namespace properties */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> new file mode 100644\n> index 000000000000..6ab5be83bc49\n> --- /dev/null\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -0,0 +1,34 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +#\n> +# Copyright (C) 2019, Google Inc.\n> +#\n> +%YAML 1.2\n> +---\n> +controls:\n> + - Location:\n> + type: int32_t\n> + description: |\n> + Camera mounting location\n> + values:\n> + - CAMERA_LOCATION_FRONT:\n> + value: 0\n> + description: |\n> + The camera is mounted on the front side of the device, facing the\n> + user\n> + - CAMERA_LOCATION_BACK:\n> + value: 1\n> + description: |\n> + The camera is mounted on the back facing side of the device\n> + - CAMERA_LOCATION_EXTERNAL:\n> + value: 2\n> + description: |\n> + The camera is attached to the device in a way that allows it to\n> + move freely\n> +\n> + - Rotation:\n> + type: int32_t\n> + description: |\n> + Camera mounting rotation expressed as counterclockwise rotation degrees\n> + towards the axis perpendicular to the sensor surface and directed away\n> + from it\n\nI know writing this description is really hard. This is the best one I \nseen so far and not sure I can do better. Only missing peace of \ninformation is where 0 degrees rotation really means. On a hand held \ndevice is that if the camera of a handheld device is not rotated while \nholding it in portrait or landscape mode?\n\n> +...\n> -- \n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1098D60BBC\n\tfor ;\n\tFri, 6 Dec 2019 22:42:05 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id d20so9148780ljc.12\n\tfor ;\n\tFri, 06 Dec 2019 13:42:05 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tu19sm7130721ljk.75.2019.12.06.13.42.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Dec 2019 13:42:03 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=iwnQGvbSfiUeKMkU7Rlww7luGlro5z7DWBXu+otHhF0=;\n\tb=fGDObEXbULpnXemqPmMMQiHRJf/yihS0T+AHginkd6POslIwqQcNIWgMZqRZlIOKkS\n\teTvBHIZsxz2DLRF5BKMzb+Rspb+3thn+6abKVMWc8ZPMMD9f527hqaI+idPJwb36lMN8\n\tK60zhPPcZ/JvD4DbfuHIyjUJlYnsFuapvJDfPLj3e3Prw5mrxCU/uqM5Q65+jHOYA0yj\n\tWqK9uWIIC6TEqhGwQ197hY1jUbzjJfGoox6RKPYSJ7Lhm3O5o1Kyev8fF4RF3rTcndNe\n\tdSlAPgyRK/gM8uxHLxnfIbX1k/edY2fexgXrR/VhIbqmYtdx0Xww0Imtr4WlPWLqPi7Z\n\ttddg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=iwnQGvbSfiUeKMkU7Rlww7luGlro5z7DWBXu+otHhF0=;\n\tb=uEmB/7LIeWcgEN3yRjCMy50pkFjDYVJVvSp2BN8BmEr2q46LSHY0+MvjKdam/uuh45\n\tyc+7WoRbh1EMNC3lFU2aZWQa/pDZCuqFOlLqsHe0wWC5C9uZI3WHJc2Y/6jfp0kBj6oT\n\tUBn5g9iTPosdpxU5IYjZ4B0ViNX7uJ4UjSJfpY4mJmGcMoPenYnjtuVv06kIvbkuzmE+\n\tXz15GQIjRLxh/gSQ2s/1vVEI4lK0m6lY9PVOTtEaIXEI7DBQ1cl5TqM088eoezyGZlSP\n\tljgzylSWbLeSKzEye9ggPhSmvUQRYV+Y8EHebI4caqOF3qczcxptS41PdcLqUeOdZnBT\n\tFw9w==","X-Gm-Message-State":"APjAAAWFeTzWrDwKPunOoRwQeCvLd4oB1vpf46lFi8gPsBOb/nqI6c70\n\tfgVOAdW7b+bGVHyCHYHOHxGQLg==","X-Google-Smtp-Source":"APXvYqxCBDkEFnA4vAn2BuRv+kZrX8A6mhtPR89E81kvWOxPQ8xXWI7kDNHTFWXENgENmN+5M3wI+Q==","X-Received":"by 2002:a2e:9708:: with SMTP id r8mr9933905lji.92.1575668524298; \n\tFri, 06 Dec 2019 13:42:04 -0800 (PST)","Date":"Fri, 6 Dec 2019 22:42:03 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191206214203.GM28879@bigcity.dyn.berto.se>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191205204350.28196-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","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","X-List-Received-Date":"Fri, 06 Dec 2019 21:42:05 -0000"}},{"id":3204,"web_url":"https://patchwork.libcamera.org/comment/3204/","msgid":"<20191206224439.GS28879@bigcity.dyn.berto.se>","date":"2019-12-06T22:44:39","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2019-12-06 22:42:04 +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n> \n> Thanks for your work.\n> \n> On 2019-12-05 21:43:43 +0100, Jacopo Mondi wrote:\n> > Re-use the Control generation infrastructure to generate libcamera properties.\n> > \n> > Introduce three additional files, one that enumerates the properties ids\n> > (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> > one for each property (src/libcamera/property_ids.cpp) and provide\n> > properties definitions in src/libcamera/property_ids.yaml\n> \n> This is also hard for me to parse, how about\n> \n> Introduce three additional files,\n> \n> - include/libcamera/property_ids.h\n> Defines the properties ids.\n> \n> - src/libcamera/property_ids.cpp\n> Defines the properties Control<> instances.\n> \n> - src/libcamera/property_ids.yaml\n> Provide properties definitions.\n> \n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/meson.build | 26 +++++++++++------\n> > include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> > src/libcamera/meson.build | 21 ++++++++------\n> > src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> > src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> > 5 files changed, 140 insertions(+), 17 deletions(-)\n> > create mode 100644 include/libcamera/property_ids.h.in\n> > create mode 100644 src/libcamera/property_ids.cpp.in\n> > create mode 100644 src/libcamera/property_ids.yaml\n> > \n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 99abf0609940..25f503660960 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -23,15 +23,23 @@ install_headers(libcamera_api,\n> > \n> > gen_controls = files('../../src/libcamera/gen-controls.py')\n> > \n> > -control_ids_h = custom_target('control_ids_h',\n> > - input : files('../../src/libcamera/control_ids.yaml', 'control_ids.h.in'),\n> > - output : 'control_ids.h',\n> > - depend_files : gen_controls,\n> > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > - install : true,\n> > - install_dir : join_paths('include', include_dir))\n> > -\n> > -libcamera_api += control_ids_h\n> > +control_source_files = [\n> > + 'control_ids',\n> > + 'property_ids',\n> > +]\n> > +\n> > +control_headers = []\n> > +\n> > +foreach header : control_source_files\n> > + input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')\n> > + control_headers += custom_target(header + '_h',\n> > + input : input_files,\n> > + output : header + '.h',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > + install : true,\n> > + install_dir : join_paths('include', include_dir))\n> > +endforeach\n> \n> I would have split this commit in two and added the foreach loop in a \n> separate patch.\n> \n> > \n> > gen_header = files('gen-header.sh')\n> > \n> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > new file mode 100644\n> > index 000000000000..62799b3e8c54\n> > --- /dev/null\n> > +++ b/include/libcamera/property_ids.h.in\n> > @@ -0,0 +1,33 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.h : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> > +#define __LIBCAMERA_PROPERTY_IDS_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace properties {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +${controls}\n> > +\n> > +extern const ControlIdMap properties;\n> > +\n> > +} /* namespace propertiess */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index c4f965bd7413..14aff6e5fc13 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -67,14 +67,19 @@ endif\n> > \n> > gen_controls = files('gen-controls.py')\n> > \n> > -control_ids_cpp = custom_target('control_ids_cpp',\n> > - input : files('control_ids.yaml', 'control_ids.cpp.in'),\n> > - output : 'control_ids.cpp',\n> > - depend_files : gen_controls,\n> > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > -\n> > -libcamera_sources += control_ids_cpp\n> > -libcamera_sources += control_ids_h\n> > +control_sources = []\n> > +\n> > +foreach source : control_source_files\n> > + input_files = files(source +'.yaml', source + '.cpp.in')\n> > + control_sources += custom_target(source + '_cpp',\n> > + input : input_files,\n> > + output : source + '.cpp',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > +endforeach\n> \n> Same here the foreach conversion could be put together with the one \n> above in a separate patch.\n> \n> > +\n> > +libcamera_sources += control_headers\n> > +libcamera_sources += control_sources\n> > \n> > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> > \n> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > new file mode 100644\n> > index 000000000000..bfdd823f63b0\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.cpp.in\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.cpp : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file property_ids.h\n> > + * \\brief Camera property identifiers\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\brief Namespace for libcamera properties\n> > + */\n> > +namespace properties {\n> > +\n> > +${controls_doc}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +/*\n> > + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> > + * them as functions.\n> > + */\n> > +${controls_def}\n> > +#endif\n> > +\n> > +/**\n> > + * \\brief List of all supported libcamera properties\n> > + */\n> > +extern const ControlIdMap properties {\n> > +${controls_map}\n> > +};\n> > +\n> > +} /* namespace properties */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > new file mode 100644\n> > index 000000000000..6ab5be83bc49\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -0,0 +1,34 @@\n> > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > +#\n> > +# Copyright (C) 2019, Google Inc.\n> > +#\n> > +%YAML 1.2\n> > +---\n> > +controls:\n> > + - Location:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting location\n> > + values:\n> > + - CAMERA_LOCATION_FRONT:\n> > + value: 0\n> > + description: |\n> > + The camera is mounted on the front side of the device, facing the\n> > + user\n> > + - CAMERA_LOCATION_BACK:\n> > + value: 1\n> > + description: |\n> > + The camera is mounted on the back facing side of the device\n> > + - CAMERA_LOCATION_EXTERNAL:\n> > + value: 2\n> > + description: |\n> > + The camera is attached to the device in a way that allows it to\n> > + move freely\n> > +\n> > + - Rotation:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting rotation expressed as counterclockwise rotation degrees\n> > + towards the axis perpendicular to the sensor surface and directed away\n> > + from it\n> \n> I know writing this description is really hard. This is the best one I \n> seen so far and not sure I can do better. Only missing peace of \n> information is where 0 degrees rotation really means. On a hand held \n> device is that if the camera of a handheld device is not rotated while \n> holding it in portrait or landscape mode?\n\nReading 9/10 I wonder if we should document a bound here, 0 - 360 \ndegrees and avoid having all users having to make sure it's in bounds.\n\n> \n> > +...\n> > -- \n> > 2.23.0\n> > \n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> \n> -- \n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D1C160BBC\n\tfor ;\n\tFri, 6 Dec 2019 23:44:41 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id z17so9265881ljk.13\n\tfor ;\n\tFri, 06 Dec 2019 14:44:41 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tg13sm7133632lfb.74.2019.12.06.14.44.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Dec 2019 14:44:40 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=Aeslmw/ttLQhgdP96bbzCdRkHjDZHLFJbHMwegCMJNc=;\n\tb=xkOA3dLvfXvg9FcMcGYj0lZZfE7xkKomrjdqQHdx7QUt2pFW3uiIqwytnpvx1vLdEC\n\tTboObqfFFiR7XoafTmn91LhruihuzlZm/bY8bVYrJBWHIOOUIh9bFPGlLRJqLLY81Kyg\n\tgYNYCywZtDks3blPrMD4wouuazexG92TYK1iak0lWFG5R6L8xH15Pzy9ck3yN/lnCwv3\n\tbjsl0n6VKE39jddaoQbe1piMufu6R+p9LveO9aR61GNFJfFIc+qNNs9a5kfbGOYJxR40\n\t5besmqd0J0MW0o22v6KukDbDtMo0gPXxvchjTFHwFoAc21JivuUtgIVpGqnlWfElWYBA\n\tlcYQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=Aeslmw/ttLQhgdP96bbzCdRkHjDZHLFJbHMwegCMJNc=;\n\tb=saWOZErqsZCH2NKcHEkxz+fxPEZDoH13LCNONyzdp54Z/LWlERRPR3ihzxeC+fOK3J\n\ttNj1FjIJF/9wKOT/HJWku4vfVpz55BSNTuo0slZWLGvxOCwiC7abLNAGQ9R1gs2NLr4c\n\tAIZdDGQ3QMZ6Eh8PAuH8xUTtuuKTodsVcG22MJgbFwTcxZNn+L8+TN0ZoVNNvXg7oqju\n\t92ISYST5Y3rHOYcpZpFiRNAGUVDpZ1uC6B030DKaD1enGVLIMVZjbfCMWVhiKYDHTF8F\n\tMSLLZZPKAUA7sYPhkNeBlt/qizxtBVZS7HQmDMeqwRV1yVTPONeQjB2DMG0+DPMDVb9d\n\tv19A==","X-Gm-Message-State":"APjAAAVw+OYOdxS9ELtO+cad3HZuJaRXdL/EGIj0SIwsGNatoxOf9bdH\n\tPCO+tgK+MDmyUWZbk5yn63lHnA==","X-Google-Smtp-Source":"APXvYqxOhxYFXSmM2W+BJQ+P7pSnadfOExnF4UyR2WaMdbnUAoqwQp2vKBdhBOuGWTb8U2tFeKPWdQ==","X-Received":"by 2002:a2e:294e:: with SMTP id\n\tu75mr10053042lje.173.1575672280704; \n\tFri, 06 Dec 2019 14:44:40 -0800 (PST)","Date":"Fri, 6 Dec 2019 23:44:39 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191206224439.GS28879@bigcity.dyn.berto.se>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-4-jacopo@jmondi.org>\n\t<20191206214203.GM28879@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191206214203.GM28879@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","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","X-List-Received-Date":"Fri, 06 Dec 2019 22:44:41 -0000"}},{"id":3210,"web_url":"https://patchwork.libcamera.org/comment/3210/","msgid":"<20191208181514.i3geia7molrs7ouh@uno.localdomain>","date":"2019-12-08T18:15:14","subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Dec 06, 2019 at 10:42:03PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-12-05 21:43:43 +0100, Jacopo Mondi wrote:\n> > Re-use the Control generation infrastructure to generate libcamera properties.\n> >\n> > Introduce three additional files, one that enumerates the properties ids\n> > (include/libcamera/property_ids.h) and one the defines Control<> instances,\n> > one for each property (src/libcamera/property_ids.cpp) and provide\n> > properties definitions in src/libcamera/property_ids.yaml\n>\n> This is also hard for me to parse, how about\n>\n> Introduce three additional files,\n>\n> - include/libcamera/property_ids.h\n> Defines the properties ids.\n>\n> - src/libcamera/property_ids.cpp\n> Defines the properties Control<> instances.\n>\n> - src/libcamera/property_ids.yaml\n> Provide properties definitions.\n>\n\nI agree, thanks\n\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/meson.build | 26 +++++++++++------\n> > include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++\n> > src/libcamera/meson.build | 21 ++++++++------\n> > src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++\n> > src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++\n> > 5 files changed, 140 insertions(+), 17 deletions(-)\n> > create mode 100644 include/libcamera/property_ids.h.in\n> > create mode 100644 src/libcamera/property_ids.cpp.in\n> > create mode 100644 src/libcamera/property_ids.yaml\n> >\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 99abf0609940..25f503660960 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -23,15 +23,23 @@ install_headers(libcamera_api,\n> >\n> > gen_controls = files('../../src/libcamera/gen-controls.py')\n> >\n> > -control_ids_h = custom_target('control_ids_h',\n> > - input : files('../../src/libcamera/control_ids.yaml', 'control_ids.h.in'),\n> > - output : 'control_ids.h',\n> > - depend_files : gen_controls,\n> > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > - install : true,\n> > - install_dir : join_paths('include', include_dir))\n> > -\n> > -libcamera_api += control_ids_h\n> > +control_source_files = [\n> > + 'control_ids',\n> > + 'property_ids',\n> > +]\n> > +\n> > +control_headers = []\n> > +\n> > +foreach header : control_source_files\n> > + input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')\n> > + control_headers += custom_target(header + '_h',\n> > + input : input_files,\n> > + output : header + '.h',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > + install : true,\n> > + install_dir : join_paths('include', include_dir))\n> > +endforeach\n>\n> I would have split this commit in two and added the foreach loop in a\n> separate patch.\n\nYes, but, really, why ?\n\nIs this something logically not connected with the introduction of two\nnew files in the build system ? Is this hard to parse ?\n\nWould you prefer one patch that introduce the new files, and one patch\nthat integrates them in the build system ? The first patch would not\neven compile the newly added items...\n\n>\n> >\n> > gen_header = files('gen-header.sh')\n> >\n> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > new file mode 100644\n> > index 000000000000..62799b3e8c54\n> > --- /dev/null\n> > +++ b/include/libcamera/property_ids.h.in\n> > @@ -0,0 +1,33 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.h : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_PROPERTY_IDS_H__\n> > +#define __LIBCAMERA_PROPERTY_IDS_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace properties {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +${controls}\n> > +\n> > +extern const ControlIdMap properties;\n> > +\n> > +} /* namespace propertiess */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif // __LIBCAMERA_PROPERTY_IDS_H__\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index c4f965bd7413..14aff6e5fc13 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -67,14 +67,19 @@ endif\n> >\n> > gen_controls = files('gen-controls.py')\n> >\n> > -control_ids_cpp = custom_target('control_ids_cpp',\n> > - input : files('control_ids.yaml', 'control_ids.cpp.in'),\n> > - output : 'control_ids.cpp',\n> > - depend_files : gen_controls,\n> > - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > -\n> > -libcamera_sources += control_ids_cpp\n> > -libcamera_sources += control_ids_h\n> > +control_sources = []\n> > +\n> > +foreach source : control_source_files\n> > + input_files = files(source +'.yaml', source + '.cpp.in')\n> > + control_sources += custom_target(source + '_cpp',\n> > + input : input_files,\n> > + output : source + '.cpp',\n> > + depend_files : gen_controls,\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > +endforeach\n>\n> Same here the foreach conversion could be put together with the one\n> above in a separate patch.\n>\n> > +\n> > +libcamera_sources += control_headers\n> > +libcamera_sources += control_sources\n> >\n> > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> >\n> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > new file mode 100644\n> > index 000000000000..bfdd823f63b0\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.cpp.in\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.cpp : Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file property_ids.h\n> > + * \\brief Camera property identifiers\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\brief Namespace for libcamera properties\n> > + */\n> > +namespace properties {\n> > +\n> > +${controls_doc}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +/*\n> > + * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> > + * them as functions.\n> > + */\n> > +${controls_def}\n> > +#endif\n> > +\n> > +/**\n> > + * \\brief List of all supported libcamera properties\n> > + */\n> > +extern const ControlIdMap properties {\n> > +${controls_map}\n> > +};\n> > +\n> > +} /* namespace properties */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > new file mode 100644\n> > index 000000000000..6ab5be83bc49\n> > --- /dev/null\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -0,0 +1,34 @@\n> > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > +#\n> > +# Copyright (C) 2019, Google Inc.\n> > +#\n> > +%YAML 1.2\n> > +---\n> > +controls:\n> > + - Location:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting location\n> > + values:\n> > + - CAMERA_LOCATION_FRONT:\n> > + value: 0\n> > + description: |\n> > + The camera is mounted on the front side of the device, facing the\n> > + user\n> > + - CAMERA_LOCATION_BACK:\n> > + value: 1\n> > + description: |\n> > + The camera is mounted on the back facing side of the device\n> > + - CAMERA_LOCATION_EXTERNAL:\n> > + value: 2\n> > + description: |\n> > + The camera is attached to the device in a way that allows it to\n> > + move freely\n> > +\n> > + - Rotation:\n> > + type: int32_t\n> > + description: |\n> > + Camera mounting rotation expressed as counterclockwise rotation degrees\n> > + towards the axis perpendicular to the sensor surface and directed away\n> > + from it\n>\n> I know writing this description is really hard. This is the best one I\n> seen so far and not sure I can do better. Only missing peace of\n> information is where 0 degrees rotation really means. On a hand held\n> device is that if the camera of a handheld device is not rotated while\n> holding it in portrait or landscape mode?\n\nI think I'm missing the point that both you and Laurent rised here.\nTo me it's not a matter of the device where the camera in installed,\nbut how the camera sensor is supposed to be mounted. I'm -sure- it has\na designated upright position, with its pixel (0,0) in its top left\ncorner (or maybe not, but that depends on the sensor, and I'm sure it\nis specified in the manual).\n\nThat said, this documentation comes from the DT bindings property I'm\npushing upstream, and while all those details on the sensor mounting\nposition are fair to be known at that level, for libcamera we might\nneed something different, I agree. I still don't get what's wrong with\nthe DT property description though (not the point you have rised).\n\nThanks\n j\n\n>\n> > +...\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A7F0660BD1\n\tfor ;\n\tSun, 8 Dec 2019 19:13:08 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id F2DAB100005;\n\tSun, 8 Dec 2019 18:13:07 +0000 (UTC)"],"Date":"Sun, 8 Dec 2019 19:15:14 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191208181514.i3geia7molrs7ouh@uno.localdomain>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-4-jacopo@jmondi.org>\n\t<20191206214203.GM28879@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a27p423mlxw2vsm3\"","Content-Disposition":"inline","In-Reply-To":"<20191206214203.GM28879@bigcity.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 03/10] libcamera: properties:\n\tGenerate libcamera properties","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","X-List-Received-Date":"Sun, 08 Dec 2019 18:13:08 -0000"}}]