[{"id":3197,"web_url":"https://patchwork.libcamera.org/comment/3197/","msgid":"<20191206190904.GL28879@bigcity.dyn.berto.se>","date":"2019-12-06T19:09:04","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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:42 +0100, Jacopo Mondi wrote:\n> In preparation to add libcamera Camera definition by re-using the\n\nFirst sentence is hard for me to parse.\n\n> control generation framework, augment the gen_controls.py script to\n> support parsing the 'values' yaml tag and generate documentation and\n> definition of possible values associated with a Control or a Property.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n> \n> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n> index 940386cc68c8..f86f01f6759b 100755\n> --- a/src/libcamera/gen-controls.py\n> +++ b/src/libcamera/gen-controls.py\n> @@ -17,10 +17,15 @@ def snake_case(s):\n> \n> \n> def generate_cpp(controls):\n> + value_doc_template = string.Template('''/**\n> + * \\\\def ${name}\n> + * \\\\brief ${description} */''')\n> +\n> doc_template = string.Template('''/**\n> * \\\\var extern const Control<${type}> ${name}\n> ${description}\n> */''')\n> +\n> def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n> \n> ctrls_doc = []\n> @@ -35,13 +40,27 @@ ${description}\n> description[0] = '\\\\brief ' + description[0]\n> description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n> \n> + try:\n> + values = ctrl['values']\n> + except KeyError:\n> + values = \"\"\n> +\n> info = {\n> 'name': name,\n> 'type': ctrl['type'],\n> 'description': description,\n> 'id_name': id_name,\n> + 'values' : values,\n> }\n> \n> + for value in values:\n> + value_info = {\n> + 'name': list(value.keys())[0],\n> + 'value': value['value'],\n> + 'description': value['description']\n> + }\n> + ctrls_doc.append(value_doc_template.substitute(value_info))\n> +\n> ctrls_doc.append(doc_template.substitute(info))\n> ctrls_def.append(def_template.substitute(info))\n> ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n> @@ -54,6 +73,7 @@ ${description}\n> \n> \n> def generate_h(controls):\n> + value_template = string.Template('''#define ${name} ${value}''')\n\nDefines are probably the way we want to play this but the notion of \nusing enums jumped into my head, is that something you think could be \nused or is it more trouble then it's worth?\n\nWith or without enums and but with an updated commit message,\n\nReviewed-by: Niklas Söderlund \n\n> template = string.Template('''extern const Control<${type}> ${name};''')\n> \n> ctrls = []\n> @@ -66,11 +86,25 @@ def generate_h(controls):\n> \n> ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> \n> + try:\n> + values = ctrl['values']\n> + except KeyError:\n> + values = \"\"\n> +\n> info = {\n> 'name': name,\n> 'type': ctrl['type'],\n> + 'values' : values,\n> }\n> \n> + for value in values:\n> + value_info = {\n> + 'name': list(value.keys())[0],\n> + 'value': value['value'],\n> + 'description': value['description']\n> + }\n> + ctrls.append(value_template.substitute(value_info))\n> +\n> ctrls.append(template.substitute(info))\n> id_value += 1\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-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D54CA60BBC\n\tfor ;\n\tFri, 6 Dec 2019 20:09:05 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id j6so8818733lja.2\n\tfor ;\n\tFri, 06 Dec 2019 11:09: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\ty14sm7080568ljk.46.2019.12.06.11.09.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Dec 2019 11:09:04 -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=4Da03MJrBsFT+hcNN3Nh3e4Vm2i2BmBhbIua686kWJw=;\n\tb=NkSG+i2lBwRmZ5AgpHkWzSOUb5ZDNPHKPjeO1Gug9U3QYnQMh/6oYSXug3PzED5QJN\n\tw6KNCxKOjHXBXBA6aAzxfY35yjX2mG7ZqhfFfG2pTa+tzV2u9niGjb8LTtUTv6tl4On4\n\tHc+m0nbneFk/EJNSZzmDeXtPzsmt/nhNTYu5rUHW133tkjASbdTlXP7Gc5Gs9lVTwNGK\n\t+SB8oPKSG67yZHhBuSAytooXPorOyUd8eV7rqLnI85ftO1ikduryVujALFOFwa2fHn85\n\tziTEhKoOJ3GbJ39T4HkyHpAx9TSDFxt+0RHVShtPWy5vF7xLTy5EGbPrGE2e9qbl95MK\n\tGr7A==","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=4Da03MJrBsFT+hcNN3Nh3e4Vm2i2BmBhbIua686kWJw=;\n\tb=LEJHIGRdHdGYKP8Tabz0ldY8UHg2yM/9bqk7jETqiWYNBYJiFaRrUzd8dSw3f3KSUs\n\t6nbeVxD4QMqUODyVyXRrhZYJJtfFSakfj9weD3SsYfoBiVzbMq33UNEMQZIJLvhsM+sR\n\tytI9vQBrS2JiPP7VVks4QhPcHxORcJfj0y0hlGfSuxXmTe6zMBQIjx6ARSekomvS0K8c\n\tpC8jAXVu4bEz0Xr1WzteZFfAGG/rEcCiuckN+r8t3vE51nl+6t86yAwfKuLJnekwXqEQ\n\tLNNbWOj1fwFAQVMAGBV8uCbOh0KXD0g9rA4FQN4y/Ek8sp5iddIMY9ed0gLAYMREhmgg\n\tnsQQ==","X-Gm-Message-State":"APjAAAVTJ7pXmi0vZ5VrfxbBbKl74aCHPUz90rk+RKUOLYJhHYco7zgc\n\tNZolV7FLQ4umbd66F826ky0F+YSy9AE=","X-Google-Smtp-Source":"APXvYqwzaiPbyb5hz3h2WoQpbLM/52G6eGD7C+ssxtV+MycBglq/yGcJiYh6s8MoEGmDlW8vCYowpw==","X-Received":"by 2002:a2e:88c5:: with SMTP id a5mr9725130ljk.201.1575659345231;\n\tFri, 06 Dec 2019 11:09:05 -0800 (PST)","Date":"Fri, 6 Dec 2019 20:09:04 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191206190904.GL28879@bigcity.dyn.berto.se>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-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-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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 19:09:06 -0000"}},{"id":3209,"web_url":"https://patchwork.libcamera.org/comment/3209/","msgid":"<20191208180747.iujblmulqrnu2xfd@uno.localdomain>","date":"2019-12-08T18:08:00","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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 08:09:04PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n> > In preparation to add libcamera Camera definition by re-using the\n>\n> First sentence is hard for me to parse.\n>\n\nIndeed.. I've left the subject behind :)\n\nIn preparation to add libcamera Camera -properties- definition by re-using the\n\n> > control generation framework, augment the gen_controls.py script to\n> > support parsing the 'values' yaml tag and generate documentation and\n> > definition of possible values associated with a Control or a Property.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n> > 1 file changed, 34 insertions(+)\n> >\n> > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n> > index 940386cc68c8..f86f01f6759b 100755\n> > --- a/src/libcamera/gen-controls.py\n> > +++ b/src/libcamera/gen-controls.py\n> > @@ -17,10 +17,15 @@ def snake_case(s):\n> >\n> >\n> > def generate_cpp(controls):\n> > + value_doc_template = string.Template('''/**\n> > + * \\\\def ${name}\n> > + * \\\\brief ${description} */''')\n> > +\n> > doc_template = string.Template('''/**\n> > * \\\\var extern const Control<${type}> ${name}\n> > ${description}\n> > */''')\n> > +\n> > def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n> >\n> > ctrls_doc = []\n> > @@ -35,13 +40,27 @@ ${description}\n> > description[0] = '\\\\brief ' + description[0]\n> > description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n> >\n> > + try:\n> > + values = ctrl['values']\n> > + except KeyError:\n> > + values = \"\"\n> > +\n> > info = {\n> > 'name': name,\n> > 'type': ctrl['type'],\n> > 'description': description,\n> > 'id_name': id_name,\n> > + 'values' : values,\n> > }\n> >\n> > + for value in values:\n> > + value_info = {\n> > + 'name': list(value.keys())[0],\n> > + 'value': value['value'],\n> > + 'description': value['description']\n> > + }\n> > + ctrls_doc.append(value_doc_template.substitute(value_info))\n> > +\n> > ctrls_doc.append(doc_template.substitute(info))\n> > ctrls_def.append(def_template.substitute(info))\n> > ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n> > @@ -54,6 +73,7 @@ ${description}\n> >\n> >\n> > def generate_h(controls):\n> > + value_template = string.Template('''#define ${name} ${value}''')\n>\n> Defines are probably the way we want to play this but the notion of\n> using enums jumped into my head, is that something you think could be\n> used or is it more trouble then it's worth?\n\nI admit I have considered enums as well... I think defines are fine\nfor now, but we can add support for enums on top if we think it's\nbetter. The only thing I would consider from the very beginning is how\nto do so.\n\nI would keep the 'values' tag for defines and add an 'enum' tag which\nmight look like this\n\ncontrols:\n - CONTROL_NAME:\n type: int32_t\n desription: -|\n blah blah\n enum:\n - NAME_OF_THE_ENUM:\n values:\n - FIRST_ENUM_ELEMENT:\n value: x\n description: -|\n blah blah\n - SECOND_ENUM_ELEMENT:\n value: x\n description: -|\n blah blah\n\nThe only reason why we should think how enum definition should look\nlike is to make it plays nice with the current use of values, which,\nfor reference is something like\n\ncontrols:\n - CONTROL_NAME:\n type: int32_t\n desription: -|\n blah blah\n values:\n - NAME_OF_THE_DEFINE:\n value: x\n description: -|\n blah blah\n\nI'm not sure I like it.\n\nI would rather introduce a 'values' section, which could contains\n(alternatively) a set of definition or an enumeration\n\ncontrols:\n - CONTROL_NAME:\n type: int32_t\n desription: -|\n blah blah\n values:\n defines:\n - NAME_OF_THE_DEFINE:\n value: x\n description: -|\n blah blah\n - NAME_OF_ANOHER_DEFINE:\n value: y\n description: -|\n blah blah\n\nOr\n\ncontrols:\n - CONTROL_NAME:\n type: int32_t\n desription: -|\n blah blah\n values:\n enum:\n - NAME_OF_THE_ENUM:\n values:\n - FIRST_ENUM_ELEMENT:\n value: x\n description: -|\n blah blah\n - SECOND_ENUM_ELEMENT:\n value: y\n description: -|\n blah blah\n\nSo that the only part that changes is the content of the 'values'\nsection.\n\nCongratulation, I've just proposed myself more work\n\nWhat do you think, should we consider enum from the beginning, is this\nan overkill ?\n\nThanks\n j\n\n>\n> With or without enums and but with an updated commit message,\n>\n> Reviewed-by: Niklas Söderlund \n>\n> > template = string.Template('''extern const Control<${type}> ${name};''')\n> >\n> > ctrls = []\n> > @@ -66,11 +86,25 @@ def generate_h(controls):\n> >\n> > ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> >\n> > + try:\n> > + values = ctrl['values']\n> > + except KeyError:\n> > + values = \"\"\n> > +\n> > info = {\n> > 'name': name,\n> > 'type': ctrl['type'],\n> > + 'values' : values,\n> > }\n> >\n> > + for value in values:\n> > + value_info = {\n> > + 'name': list(value.keys())[0],\n> > + 'value': value['value'],\n> > + 'description': value['description']\n> > + }\n> > + ctrls.append(value_template.substitute(value_info))\n> > +\n> > ctrls.append(template.substitute(info))\n> > id_value += 1\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 relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 511ED60BD1\n\tfor ;\n\tSun, 8 Dec 2019 19:05:54 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id B04F2C0003;\n\tSun, 8 Dec 2019 18:05:53 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sun, 8 Dec 2019 19:08:00 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191208180747.iujblmulqrnu2xfd@uno.localdomain>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6kbxgjbqlacurfnw\"","Content-Disposition":"inline","In-Reply-To":"<20191206190904.GL28879@bigcity.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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:05:54 -0000"}},{"id":3216,"web_url":"https://patchwork.libcamera.org/comment/3216/","msgid":"<20191209125429.GB9158@pendragon.ideasonboard.com>","date":"2019-12-09T12:54:29","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:\n> On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n> > > In preparation to add libcamera Camera definition by re-using the\n> >\n> > First sentence is hard for me to parse.\n> \n> Indeed.. I've left the subject behind :)\n> \n> In preparation to add libcamera Camera -properties- definition by re-using the\n> \n> > > control generation framework, augment the gen_controls.py script to\n> > > support parsing the 'values' yaml tag and generate documentation and\n> > > definition of possible values associated with a Control or a Property.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n> > > 1 file changed, 34 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n> > > index 940386cc68c8..f86f01f6759b 100755\n> > > --- a/src/libcamera/gen-controls.py\n> > > +++ b/src/libcamera/gen-controls.py\n> > > @@ -17,10 +17,15 @@ def snake_case(s):\n> > >\n> > >\n> > > def generate_cpp(controls):\n> > > + value_doc_template = string.Template('''/**\n> > > + * \\\\def ${name}\n> > > + * \\\\brief ${description} */''')\n> > > +\n> > > doc_template = string.Template('''/**\n> > > * \\\\var extern const Control<${type}> ${name}\n> > > ${description}\n> > > */''')\n> > > +\n> > > def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n> > >\n> > > ctrls_doc = []\n> > > @@ -35,13 +40,27 @@ ${description}\n> > > description[0] = '\\\\brief ' + description[0]\n> > > description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n> > >\n> > > + try:\n> > > + values = ctrl['values']\n> > > + except KeyError:\n> > > + values = \"\"\n> > > +\n> > > info = {\n> > > 'name': name,\n> > > 'type': ctrl['type'],\n> > > 'description': description,\n> > > 'id_name': id_name,\n> > > + 'values' : values,\n> > > }\n> > >\n> > > + for value in values:\n> > > + value_info = {\n> > > + 'name': list(value.keys())[0],\n> > > + 'value': value['value'],\n> > > + 'description': value['description']\n> > > + }\n> > > + ctrls_doc.append(value_doc_template.substitute(value_info))\n> > > +\n> > > ctrls_doc.append(doc_template.substitute(info))\n> > > ctrls_def.append(def_template.substitute(info))\n> > > ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n> > > @@ -54,6 +73,7 @@ ${description}\n> > >\n> > >\n> > > def generate_h(controls):\n> > > + value_template = string.Template('''#define ${name} ${value}''')\n> >\n> > Defines are probably the way we want to play this but the notion of\n> > using enums jumped into my head, is that something you think could be\n> > used or is it more trouble then it's worth?\n> \n> I admit I have considered enums as well... I think defines are fine\n> for now, but we can add support for enums on top if we think it's\n> better. The only thing I would consider from the very beginning is how\n> to do so.\n> \n> I would keep the 'values' tag for defines and add an 'enum' tag which\n> might look like this\n> \n> controls:\n> - CONTROL_NAME:\n> type: int32_t\n> desription: -|\n> blah blah\n> enum:\n> - NAME_OF_THE_ENUM:\n> values:\n> - FIRST_ENUM_ELEMENT:\n> value: x\n> description: -|\n> blah blah\n> - SECOND_ENUM_ELEMENT:\n> value: x\n> description: -|\n> blah blah\n> \n> The only reason why we should think how enum definition should look\n> like is to make it plays nice with the current use of values, which,\n> for reference is something like\n> \n> controls:\n> - CONTROL_NAME:\n> type: int32_t\n> desription: -|\n> blah blah\n> values:\n> - NAME_OF_THE_DEFINE:\n> value: x\n> description: -|\n> blah blah\n> \n> I'm not sure I like it.\n> \n> I would rather introduce a 'values' section, which could contains\n> (alternatively) a set of definition or an enumeration\n> \n> controls:\n> - CONTROL_NAME:\n> type: int32_t\n> desription: -|\n> blah blah\n> values:\n> defines:\n> - NAME_OF_THE_DEFINE:\n> value: x\n> description: -|\n> blah blah\n> - NAME_OF_ANOHER_DEFINE:\n> value: y\n> description: -|\n> blah blah\n> \n> Or\n> \n> controls:\n> - CONTROL_NAME:\n> type: int32_t\n> desription: -|\n> blah blah\n> values:\n> enum:\n> - NAME_OF_THE_ENUM:\n> values:\n> - FIRST_ENUM_ELEMENT:\n> value: x\n> description: -|\n> blah blah\n> - SECOND_ENUM_ELEMENT:\n> value: y\n> description: -|\n> blah blah\n> \n> So that the only part that changes is the content of the 'values'\n> section.\n> \n> Congratulation, I've just proposed myself more work\n> \n> What do you think, should we consider enum from the beginning, is this\n> an overkill ?\n\nI think we should only have enums, not defines, and I think it shouldn't\nbe controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,\nyou can name it enum ControlNameValues.\n\n------- control_ids.yaml ------\ncontrols:\n - WhiteBalanceMode:\n type: int32_t\n desription: -|\n blah blah\n enum:\n - WhiteBalanceModeAuto:\n value: 0\n description: -|\n blah blah\n - WhiteBalanceModeSunny:\n value: 1\n description: -|\n blah blah\n - WhiteBalanceModeCloudy:\n value: 1\n description: -|\n blah blah\n-------------------------------\n\nshould generate\n\n-------- control_ids.h --------\nnamespace controls {\n\nenum WhiteBalanceModeValues {\n\tWhiteBalanceModeAuto = 0,\n\tWhiteBalanceModeSunny = 1,\n\tWhiteBalanceModeCloudy = 2,\n\t...\n};\n\nextern const Control WhiteBalanceMode;\n\n}\n-------------------------------\n\n> > With or without enums and but with an updated commit message,\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > template = string.Template('''extern const Control<${type}> ${name};''')\n> > >\n> > > ctrls = []\n> > > @@ -66,11 +86,25 @@ def generate_h(controls):\n> > >\n> > > ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> > >\n> > > + try:\n> > > + values = ctrl['values']\n> > > + except KeyError:\n> > > + values = \"\"\n> > > +\n> > > info = {\n> > > 'name': name,\n> > > 'type': ctrl['type'],\n> > > + 'values' : values,\n> > > }\n> > >\n> > > + for value in values:\n> > > + value_info = {\n> > > + 'name': list(value.keys())[0],\n> > > + 'value': value['value'],\n> > > + 'description': value['description']\n> > > + }\n> > > + ctrls.append(value_template.substitute(value_info))\n> > > +\n> > > ctrls.append(template.substitute(info))\n> > > id_value += 1\n> > >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8548560BC4\n\tfor ;\n\tMon, 9 Dec 2019 13:54:36 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE1E999A;\n\tMon, 9 Dec 2019 13:54:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575896076;\n\tbh=IuyxiddLH2l+lRmObT3gZ9Gs4tA/TiwepEunVOpS79Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YKlYZDNSGEVHSiQ4ejNYyTz/9vJ83lHm//PSXasm9PW22QFlWkXDFj+dH/tOxcfZ0\n\taUq7i11J015OuSKQFGyWsNXmSd9f20RPpASJseJOxmB7BHa1FuLyqi/d986AkZoIG4\n\tTkYkNbFSaKFVhuvbOKODcayph1dIc2E78z7msq9g=","Date":"Mon, 9 Dec 2019 14:54:29 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191209125429.GB9158@pendragon.ideasonboard.com>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>\n\t<20191208180747.iujblmulqrnu2xfd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191208180747.iujblmulqrnu2xfd@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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":"Mon, 09 Dec 2019 12:54:36 -0000"}},{"id":3218,"web_url":"https://patchwork.libcamera.org/comment/3218/","msgid":"<11f8a2a1-d534-c037-da6a-c97254a7cc99@ideasonboard.com>","date":"2019-12-09T13:16:08","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo, and all,\n\nOn 09/12/2019 12:54, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:\n>> On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:\n>>> Hi Jacopo,\n>>>\n>>> Thanks for your work.\n>>>\n>>> On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n>>>> In preparation to add libcamera Camera definition by re-using the\n>>>\n>>> First sentence is hard for me to parse.\n>>\n>> Indeed.. I've left the subject behind :)\n>>\n>> In preparation to add libcamera Camera -properties- definition by re-using the\n>>\n>>>> control generation framework, augment the gen_controls.py script to\n>>>> support parsing the 'values' yaml tag and generate documentation and\n>>>> definition of possible values associated with a Control or a Property.\n>>>>\n>>>> Signed-off-by: Jacopo Mondi \n>>>> ---\n>>>> src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n>>>> 1 file changed, 34 insertions(+)\n>>>>\n>>>> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n>>>> index 940386cc68c8..f86f01f6759b 100755\n>>>> --- a/src/libcamera/gen-controls.py\n>>>> +++ b/src/libcamera/gen-controls.py\n>>>> @@ -17,10 +17,15 @@ def snake_case(s):\n>>>>\n>>>>\n>>>> def generate_cpp(controls):\n>>>> + value_doc_template = string.Template('''/**\n>>>> + * \\\\def ${name}\n>>>> + * \\\\brief ${description} */''')\n>>>> +\n>>>> doc_template = string.Template('''/**\n>>>> * \\\\var extern const Control<${type}> ${name}\n>>>> ${description}\n>>>> */''')\n>>>> +\n>>>> def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n>>>>\n>>>> ctrls_doc = []\n>>>> @@ -35,13 +40,27 @@ ${description}\n>>>> description[0] = '\\\\brief ' + description[0]\n>>>> description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n>>>>\n>>>> + try:\n>>>> + values = ctrl['values']\n>>>> + except KeyError:\n>>>> + values = \"\"\n>>>> +\n>>>> info = {\n>>>> 'name': name,\n>>>> 'type': ctrl['type'],\n>>>> 'description': description,\n>>>> 'id_name': id_name,\n>>>> + 'values' : values,\n>>>> }\n>>>>\n>>>> + for value in values:\n>>>> + value_info = {\n>>>> + 'name': list(value.keys())[0],\n>>>> + 'value': value['value'],\n>>>> + 'description': value['description']\n>>>> + }\n>>>> + ctrls_doc.append(value_doc_template.substitute(value_info))\n>>>> +\n>>>> ctrls_doc.append(doc_template.substitute(info))\n>>>> ctrls_def.append(def_template.substitute(info))\n>>>> ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n>>>> @@ -54,6 +73,7 @@ ${description}\n>>>>\n>>>>\n>>>> def generate_h(controls):\n>>>> + value_template = string.Template('''#define ${name} ${value}''')\n>>>\n>>> Defines are probably the way we want to play this but the notion of\n>>> using enums jumped into my head, is that something you think could be\n>>> used or is it more trouble then it's worth?\n>>\n>> I admit I have considered enums as well... I think defines are fine\n>> for now, but we can add support for enums on top if we think it's\n>> better. The only thing I would consider from the very beginning is how\n>> to do so.\n>>\n>> I would keep the 'values' tag for defines and add an 'enum' tag which\n>> might look like this\n>>\n>> controls:\n>> - CONTROL_NAME:\n>> type: int32_t\n>> desription: -|\n>> blah blah\n>> enum:\n>> - NAME_OF_THE_ENUM:\n>> values:\n>> - FIRST_ENUM_ELEMENT:\n>> value: x\n>> description: -|\n>> blah blah\n>> - SECOND_ENUM_ELEMENT:\n>> value: x\n>> description: -|\n>> blah blah\n>>\n>> The only reason why we should think how enum definition should look\n>> like is to make it plays nice with the current use of values, which,\n>> for reference is something like\n>>\n>> controls:\n>> - CONTROL_NAME:\n>> type: int32_t\n>> desription: -|\n>> blah blah\n>> values:\n>> - NAME_OF_THE_DEFINE:\n>> value: x\n>> description: -|\n>> blah blah\n>>\n>> I'm not sure I like it.\n>>\n>> I would rather introduce a 'values' section, which could contains\n>> (alternatively) a set of definition or an enumeration\n>>\n>> controls:\n>> - CONTROL_NAME:\n>> type: int32_t\n>> desription: -|\n>> blah blah\n>> values:\n>> defines:\n>> - NAME_OF_THE_DEFINE:\n>> value: x\n>> description: -|\n>> blah blah\n>> - NAME_OF_ANOHER_DEFINE:\n>> value: y\n>> description: -|\n>> blah blah\n>>\n>> Or\n>>\n>> controls:\n>> - CONTROL_NAME:\n>> type: int32_t\n>> desription: -|\n>> blah blah\n>> values:\n>> enum:\n>> - NAME_OF_THE_ENUM:\n>> values:\n>> - FIRST_ENUM_ELEMENT:\n>> value: x\n>> description: -|\n>> blah blah\n>> - SECOND_ENUM_ELEMENT:\n>> value: y\n>> description: -|\n>> blah blah\n>>\n>> So that the only part that changes is the content of the 'values'\n>> section.\n>>\n>> Congratulation, I've just proposed myself more work\n>>\n>> What do you think, should we consider enum from the beginning, is this\n>> an overkill ?\n> \n> I think we should only have enums, not defines, and I think it shouldn't\n> be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,\n> you can name it enum ControlNameValues.\n> \n> ------- control_ids.yaml ------\n> controls:\n> - WhiteBalanceMode:\n> type: int32_t\n> desription: -|\n> blah blah\n> enum:\n> - WhiteBalanceModeAuto:\n> value: 0\n> description: -|\n> blah blah\n> - WhiteBalanceModeSunny:\n> value: 1\n> description: -|\n> blah blah\n> - WhiteBalanceModeCloudy:\n> value: 1\n\nI suspect you mean value: 2 here, but I think that's just a typo in the\npsuedo code.\n\n\n> description: -|\n> blah blah\n> -------------------------------\n> \n> should generate\n> \n> -------- control_ids.h --------\n> namespace controls {\n> \n> enum WhiteBalanceModeValues {\n> \tWhiteBalanceModeAuto = 0,\n> \tWhiteBalanceModeSunny = 1,\n> \tWhiteBalanceModeCloudy = 2,\n> \t...\n> };\n> \n> extern const Control WhiteBalanceMode;\n> \n> }\n> -------------------------------\n\n\nWould we support automatic values in enums?\n\nI prefer using enums here over defines, as it scopes the values and will\ngive the compiler/debug-info more information as to their context too,\nwhich means we will be able to perform more context validation (such as\nensuring all enum values are correctly identified within a switch)\n\nOnce defined, the numbers become part of the ABI, so they'd have to be\nconstant ... So we probably / possibly need to either document or define\nsome rule that any new additions are only appended, and not added in any\nsort-order.\n\nOtherwise, +1 on the YAML style Laurent has posted. It seems concise and\nfills the requirements I can see.\n\n--\nKieran\n\n\n\n\n>>> With or without enums and but with an updated commit message,\n>>>\n>>> Reviewed-by: Niklas Söderlund \n>>>\n>>>> template = string.Template('''extern const Control<${type}> ${name};''')\n>>>>\n>>>> ctrls = []\n>>>> @@ -66,11 +86,25 @@ def generate_h(controls):\n>>>>\n>>>> ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n>>>>\n>>>> + try:\n>>>> + values = ctrl['values']\n>>>> + except KeyError:\n>>>> + values = \"\"\n>>>> +\n>>>> info = {\n>>>> 'name': name,\n>>>> 'type': ctrl['type'],\n>>>> + 'values' : values,\n>>>> }\n>>>>\n>>>> + for value in values:\n>>>> + value_info = {\n>>>> + 'name': list(value.keys())[0],\n>>>> + 'value': value['value'],\n>>>> + 'description': value['description']\n>>>> + }\n>>>> + ctrls.append(value_template.substitute(value_info))\n>>>> +\n>>>> ctrls.append(template.substitute(info))\n>>>> id_value += 1\n>>>>\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCDC260BC4\n\tfor ;\n\tMon, 9 Dec 2019 14:16:11 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E32052B;\n\tMon, 9 Dec 2019 14:16:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575897371;\n\tbh=kl43aultmHq/ug3mTXEHKlnltR9oBgJjaGewCk3ytRk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ITlULvVz6fGnizGIi4ASjTMi9FprIzgP6x1mrSDXoypEdnEqr0RykGBsygH0F9dIq\n\t9icHRfOhFq2YUuhyAGHPVRBkfgLvTtKG/kstxwY/VemJtE5pSSYjSb8Uw0XCs2At07\n\t+R5Z/VIjWd+T5lj6YkYocuVL5vJgf7xJN4SQ4WA8=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>\n\t<20191208180747.iujblmulqrnu2xfd@uno.localdomain>\n\t<20191209125429.GB9158@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<11f8a2a1-d534-c037-da6a-c97254a7cc99@ideasonboard.com>","Date":"Mon, 9 Dec 2019 13:16:08 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20191209125429.GB9158@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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":"Mon, 09 Dec 2019 13:16:12 -0000"}},{"id":3219,"web_url":"https://patchwork.libcamera.org/comment/3219/","msgid":"<20191209131745.rlq7w6rvulnuvnbf@uno.localdomain>","date":"2019-12-09T13:17:45","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Dec 09, 2019 at 02:54:29PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:\n> > On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n> > > > In preparation to add libcamera Camera definition by re-using the\n> > >\n> > > First sentence is hard for me to parse.\n> >\n> > Indeed.. I've left the subject behind :)\n> >\n> > In preparation to add libcamera Camera -properties- definition by re-using the\n> >\n> > > > control generation framework, augment the gen_controls.py script to\n> > > > support parsing the 'values' yaml tag and generate documentation and\n> > > > definition of possible values associated with a Control or a Property.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n> > > > 1 file changed, 34 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n> > > > index 940386cc68c8..f86f01f6759b 100755\n> > > > --- a/src/libcamera/gen-controls.py\n> > > > +++ b/src/libcamera/gen-controls.py\n> > > > @@ -17,10 +17,15 @@ def snake_case(s):\n> > > >\n> > > >\n> > > > def generate_cpp(controls):\n> > > > + value_doc_template = string.Template('''/**\n> > > > + * \\\\def ${name}\n> > > > + * \\\\brief ${description} */''')\n> > > > +\n> > > > doc_template = string.Template('''/**\n> > > > * \\\\var extern const Control<${type}> ${name}\n> > > > ${description}\n> > > > */''')\n> > > > +\n> > > > def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n> > > >\n> > > > ctrls_doc = []\n> > > > @@ -35,13 +40,27 @@ ${description}\n> > > > description[0] = '\\\\brief ' + description[0]\n> > > > description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n> > > >\n> > > > + try:\n> > > > + values = ctrl['values']\n> > > > + except KeyError:\n> > > > + values = \"\"\n> > > > +\n> > > > info = {\n> > > > 'name': name,\n> > > > 'type': ctrl['type'],\n> > > > 'description': description,\n> > > > 'id_name': id_name,\n> > > > + 'values' : values,\n> > > > }\n> > > >\n> > > > + for value in values:\n> > > > + value_info = {\n> > > > + 'name': list(value.keys())[0],\n> > > > + 'value': value['value'],\n> > > > + 'description': value['description']\n> > > > + }\n> > > > + ctrls_doc.append(value_doc_template.substitute(value_info))\n> > > > +\n> > > > ctrls_doc.append(doc_template.substitute(info))\n> > > > ctrls_def.append(def_template.substitute(info))\n> > > > ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n> > > > @@ -54,6 +73,7 @@ ${description}\n> > > >\n> > > >\n> > > > def generate_h(controls):\n> > > > + value_template = string.Template('''#define ${name} ${value}''')\n> > >\n> > > Defines are probably the way we want to play this but the notion of\n> > > using enums jumped into my head, is that something you think could be\n> > > used or is it more trouble then it's worth?\n> >\n> > I admit I have considered enums as well... I think defines are fine\n> > for now, but we can add support for enums on top if we think it's\n> > better. The only thing I would consider from the very beginning is how\n> > to do so.\n> >\n> > I would keep the 'values' tag for defines and add an 'enum' tag which\n> > might look like this\n> >\n> > controls:\n> > - CONTROL_NAME:\n> > type: int32_t\n> > desription: -|\n> > blah blah\n> > enum:\n> > - NAME_OF_THE_ENUM:\n> > values:\n> > - FIRST_ENUM_ELEMENT:\n> > value: x\n> > description: -|\n> > blah blah\n> > - SECOND_ENUM_ELEMENT:\n> > value: x\n> > description: -|\n> > blah blah\n> >\n> > The only reason why we should think how enum definition should look\n> > like is to make it plays nice with the current use of values, which,\n> > for reference is something like\n> >\n> > controls:\n> > - CONTROL_NAME:\n> > type: int32_t\n> > desription: -|\n> > blah blah\n> > values:\n> > - NAME_OF_THE_DEFINE:\n> > value: x\n> > description: -|\n> > blah blah\n> >\n> > I'm not sure I like it.\n> >\n> > I would rather introduce a 'values' section, which could contains\n> > (alternatively) a set of definition or an enumeration\n> >\n> > controls:\n> > - CONTROL_NAME:\n> > type: int32_t\n> > desription: -|\n> > blah blah\n> > values:\n> > defines:\n> > - NAME_OF_THE_DEFINE:\n> > value: x\n> > description: -|\n> > blah blah\n> > - NAME_OF_ANOHER_DEFINE:\n> > value: y\n> > description: -|\n> > blah blah\n> >\n> > Or\n> >\n> > controls:\n> > - CONTROL_NAME:\n> > type: int32_t\n> > desription: -|\n> > blah blah\n> > values:\n> > enum:\n> > - NAME_OF_THE_ENUM:\n> > values:\n> > - FIRST_ENUM_ELEMENT:\n> > value: x\n> > description: -|\n> > blah blah\n> > - SECOND_ENUM_ELEMENT:\n> > value: y\n> > description: -|\n> > blah blah\n> >\n> > So that the only part that changes is the content of the 'values'\n> > section.\n> >\n> > Congratulation, I've just proposed myself more work\n> >\n> > What do you think, should we consider enum from the beginning, is this\n> > an overkill ?\n>\n> I think we should only have enums, not defines, and I think it shouldn't\n> be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,\n> you can name it enum ControlNameValues.\n>\n> ------- control_ids.yaml ------\n> controls:\n> - WhiteBalanceMode:\n> type: int32_t\n> desription: -|\n> blah blah\n> enum:\n> - WhiteBalanceModeAuto:\n> value: 0\n> description: -|\n> blah blah\n> - WhiteBalanceModeSunny:\n> value: 1\n> description: -|\n> blah blah\n> - WhiteBalanceModeCloudy:\n> value: 1\n> description: -|\n> blah blah\n> -------------------------------\n>\n> should generate\n>\n> -------- control_ids.h --------\n> namespace controls {\n>\n> enum WhiteBalanceModeValues {\n> \tWhiteBalanceModeAuto = 0,\n> \tWhiteBalanceModeSunny = 1,\n> \tWhiteBalanceModeCloudy = 2,\n> \t...\n> };\n>\n> extern const Control WhiteBalanceMode;\n>\n> }\n\nThis is actually less work than what I have proposed, and adding\ndefines to enumerations is easier, so we could go for this one first\nprobably.\n\nThanks\n j\n\n> -------------------------------\n>\n> > > With or without enums and but with an updated commit message,\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > template = string.Template('''extern const Control<${type}> ${name};''')\n> > > >\n> > > > ctrls = []\n> > > > @@ -66,11 +86,25 @@ def generate_h(controls):\n> > > >\n> > > > ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> > > >\n> > > > + try:\n> > > > + values = ctrl['values']\n> > > > + except KeyError:\n> > > > + values = \"\"\n> > > > +\n> > > > info = {\n> > > > 'name': name,\n> > > > 'type': ctrl['type'],\n> > > > + 'values' : values,\n> > > > }\n> > > >\n> > > > + for value in values:\n> > > > + value_info = {\n> > > > + 'name': list(value.keys())[0],\n> > > > + 'value': value['value'],\n> > > > + 'description': value['description']\n> > > > + }\n> > > > + ctrls.append(value_template.substitute(value_info))\n> > > > +\n> > > > ctrls.append(template.substitute(info))\n> > > > id_value += 1\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 219FE60BC4\n\tfor ;\n\tMon, 9 Dec 2019 14:16:51 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 3E1D420007;\n\tMon, 9 Dec 2019 13:16:50 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Mon, 9 Dec 2019 14:17:45 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191209131745.rlq7w6rvulnuvnbf@uno.localdomain>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>\n\t<20191208180747.iujblmulqrnu2xfd@uno.localdomain>\n\t<20191209125429.GB9158@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"e2gsl3ghlxecu53f\"","Content-Disposition":"inline","In-Reply-To":"<20191209125429.GB9158@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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":"Mon, 09 Dec 2019 13:16:51 -0000"}},{"id":3220,"web_url":"https://patchwork.libcamera.org/comment/3220/","msgid":"<20191209132016.GC9158@pendragon.ideasonboard.com>","date":"2019-12-09T13:20:16","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Dec 09, 2019 at 01:16:08PM +0000, Kieran Bingham wrote:\n> On 09/12/2019 12:54, Laurent Pinchart wrote:\n> > On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:\n> >> On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:\n> >>> On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n> >>>> In preparation to add libcamera Camera definition by re-using the\n> >>>\n> >>> First sentence is hard for me to parse.\n> >>\n> >> Indeed.. I've left the subject behind :)\n> >>\n> >> In preparation to add libcamera Camera -properties- definition by re-using the\n> >>\n> >>>> control generation framework, augment the gen_controls.py script to\n> >>>> support parsing the 'values' yaml tag and generate documentation and\n> >>>> definition of possible values associated with a Control or a Property.\n> >>>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n> >>>> 1 file changed, 34 insertions(+)\n> >>>>\n> >>>> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n> >>>> index 940386cc68c8..f86f01f6759b 100755\n> >>>> --- a/src/libcamera/gen-controls.py\n> >>>> +++ b/src/libcamera/gen-controls.py\n> >>>> @@ -17,10 +17,15 @@ def snake_case(s):\n> >>>>\n> >>>>\n> >>>> def generate_cpp(controls):\n> >>>> + value_doc_template = string.Template('''/**\n> >>>> + * \\\\def ${name}\n> >>>> + * \\\\brief ${description} */''')\n> >>>> +\n> >>>> doc_template = string.Template('''/**\n> >>>> * \\\\var extern const Control<${type}> ${name}\n> >>>> ${description}\n> >>>> */''')\n> >>>> +\n> >>>> def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n> >>>>\n> >>>> ctrls_doc = []\n> >>>> @@ -35,13 +40,27 @@ ${description}\n> >>>> description[0] = '\\\\brief ' + description[0]\n> >>>> description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n> >>>>\n> >>>> + try:\n> >>>> + values = ctrl['values']\n> >>>> + except KeyError:\n> >>>> + values = \"\"\n> >>>> +\n> >>>> info = {\n> >>>> 'name': name,\n> >>>> 'type': ctrl['type'],\n> >>>> 'description': description,\n> >>>> 'id_name': id_name,\n> >>>> + 'values' : values,\n> >>>> }\n> >>>>\n> >>>> + for value in values:\n> >>>> + value_info = {\n> >>>> + 'name': list(value.keys())[0],\n> >>>> + 'value': value['value'],\n> >>>> + 'description': value['description']\n> >>>> + }\n> >>>> + ctrls_doc.append(value_doc_template.substitute(value_info))\n> >>>> +\n> >>>> ctrls_doc.append(doc_template.substitute(info))\n> >>>> ctrls_def.append(def_template.substitute(info))\n> >>>> ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n> >>>> @@ -54,6 +73,7 @@ ${description}\n> >>>>\n> >>>>\n> >>>> def generate_h(controls):\n> >>>> + value_template = string.Template('''#define ${name} ${value}''')\n> >>>\n> >>> Defines are probably the way we want to play this but the notion of\n> >>> using enums jumped into my head, is that something you think could be\n> >>> used or is it more trouble then it's worth?\n> >>\n> >> I admit I have considered enums as well... I think defines are fine\n> >> for now, but we can add support for enums on top if we think it's\n> >> better. The only thing I would consider from the very beginning is how\n> >> to do so.\n> >>\n> >> I would keep the 'values' tag for defines and add an 'enum' tag which\n> >> might look like this\n> >>\n> >> controls:\n> >> - CONTROL_NAME:\n> >> type: int32_t\n> >> desription: -|\n> >> blah blah\n> >> enum:\n> >> - NAME_OF_THE_ENUM:\n> >> values:\n> >> - FIRST_ENUM_ELEMENT:\n> >> value: x\n> >> description: -|\n> >> blah blah\n> >> - SECOND_ENUM_ELEMENT:\n> >> value: x\n> >> description: -|\n> >> blah blah\n> >>\n> >> The only reason why we should think how enum definition should look\n> >> like is to make it plays nice with the current use of values, which,\n> >> for reference is something like\n> >>\n> >> controls:\n> >> - CONTROL_NAME:\n> >> type: int32_t\n> >> desription: -|\n> >> blah blah\n> >> values:\n> >> - NAME_OF_THE_DEFINE:\n> >> value: x\n> >> description: -|\n> >> blah blah\n> >>\n> >> I'm not sure I like it.\n> >>\n> >> I would rather introduce a 'values' section, which could contains\n> >> (alternatively) a set of definition or an enumeration\n> >>\n> >> controls:\n> >> - CONTROL_NAME:\n> >> type: int32_t\n> >> desription: -|\n> >> blah blah\n> >> values:\n> >> defines:\n> >> - NAME_OF_THE_DEFINE:\n> >> value: x\n> >> description: -|\n> >> blah blah\n> >> - NAME_OF_ANOHER_DEFINE:\n> >> value: y\n> >> description: -|\n> >> blah blah\n> >>\n> >> Or\n> >>\n> >> controls:\n> >> - CONTROL_NAME:\n> >> type: int32_t\n> >> desription: -|\n> >> blah blah\n> >> values:\n> >> enum:\n> >> - NAME_OF_THE_ENUM:\n> >> values:\n> >> - FIRST_ENUM_ELEMENT:\n> >> value: x\n> >> description: -|\n> >> blah blah\n> >> - SECOND_ENUM_ELEMENT:\n> >> value: y\n> >> description: -|\n> >> blah blah\n> >>\n> >> So that the only part that changes is the content of the 'values'\n> >> section.\n> >>\n> >> Congratulation, I've just proposed myself more work\n> >>\n> >> What do you think, should we consider enum from the beginning, is this\n> >> an overkill ?\n> > \n> > I think we should only have enums, not defines, and I think it shouldn't\n> > be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,\n> > you can name it enum ControlNameValues.\n> > \n> > ------- control_ids.yaml ------\n> > controls:\n> > - WhiteBalanceMode:\n> > type: int32_t\n> > desription: -|\n> > blah blah\n> > enum:\n> > - WhiteBalanceModeAuto:\n> > value: 0\n> > description: -|\n> > blah blah\n> > - WhiteBalanceModeSunny:\n> > value: 1\n> > description: -|\n> > blah blah\n> > - WhiteBalanceModeCloudy:\n> > value: 1\n> \n> I suspect you mean value: 2 here, but I think that's just a typo in the\n> psuedo code.\n\nYes, it's a typo, sorry.\n\n> > description: -|\n> > blah blah\n> > -------------------------------\n> > \n> > should generate\n> > \n> > -------- control_ids.h --------\n> > namespace controls {\n> > \n> > enum WhiteBalanceModeValues {\n> > \tWhiteBalanceModeAuto = 0,\n> > \tWhiteBalanceModeSunny = 1,\n> > \tWhiteBalanceModeCloudy = 2,\n> > \t...\n> > };\n> > \n> > extern const Control WhiteBalanceMode;\n> > \n> > }\n> > -------------------------------\n> \n> \n> Would we support automatic values in enums?\n> \n> I prefer using enums here over defines, as it scopes the values and will\n> give the compiler/debug-info more information as to their context too,\n> which means we will be able to perform more context validation (such as\n> ensuring all enum values are correctly identified within a switch)\n> \n> Once defined, the numbers become part of the ABI, so they'd have to be\n> constant ... So we probably / possibly need to either document or define\n> some rule that any new additions are only appended, and not added in any\n> sort-order.\n\nI've thought about that, but it can be a bit error-prone as inserting a\nnew value in the middle may not be caught during review. Furthermore, I\nthink having the explicit values in the documentation is useful for\nbinary IPAs that don't want to link to libcamera. For those reasons I\nthink there's more cons than pros for automatic values.\n\n> Otherwise, +1 on the YAML style Laurent has posted. It seems concise and\n> fills the requirements I can see.\n> \n> >>> With or without enums and but with an updated commit message,\n> >>>\n> >>> Reviewed-by: Niklas Söderlund \n> >>>\n> >>>> template = string.Template('''extern const Control<${type}> ${name};''')\n> >>>>\n> >>>> ctrls = []\n> >>>> @@ -66,11 +86,25 @@ def generate_h(controls):\n> >>>>\n> >>>> ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> >>>>\n> >>>> + try:\n> >>>> + values = ctrl['values']\n> >>>> + except KeyError:\n> >>>> + values = \"\"\n> >>>> +\n> >>>> info = {\n> >>>> 'name': name,\n> >>>> 'type': ctrl['type'],\n> >>>> + 'values' : values,\n> >>>> }\n> >>>>\n> >>>> + for value in values:\n> >>>> + value_info = {\n> >>>> + 'name': list(value.keys())[0],\n> >>>> + 'value': value['value'],\n> >>>> + 'description': value['description']\n> >>>> + }\n> >>>> + ctrls.append(value_template.substitute(value_info))\n> >>>> +\n> >>>> ctrls.append(template.substitute(info))\n> >>>> id_value += 1\n> >>>>","headers":{"Return-Path":"","Received":["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 2815660BC4\n\tfor ;\n\tMon, 9 Dec 2019 14:20:24 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C77699A;\n\tMon, 9 Dec 2019 14:20:23 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575897623;\n\tbh=FeQZl0XXxnSINsySt0yRtAbjcySqWssCZh7lOvTivz4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cY2feNYWyfNmb/ugv2joWzDw3Z9e8cmgxVC6Snssf70MwYxBiAnU0qvpyy5OwRCXX\n\tHDhCEW0azph7pJCGZJHh9fHW6f1rPRibRsrebV+ANv1199qm6okzNlAC9uQHjrOaaD\n\teFi9uwBqTVnJgkJVtuosIN2meDTjw03NSuh8G0HM=","Date":"Mon, 9 Dec 2019 15:20:16 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191209132016.GC9158@pendragon.ideasonboard.com>","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>\n\t<20191208180747.iujblmulqrnu2xfd@uno.localdomain>\n\t<20191209125429.GB9158@pendragon.ideasonboard.com>\n\t<11f8a2a1-d534-c037-da6a-c97254a7cc99@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<11f8a2a1-d534-c037-da6a-c97254a7cc99@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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":"Mon, 09 Dec 2019 13:20:24 -0000"}},{"id":3222,"web_url":"https://patchwork.libcamera.org/comment/3222/","msgid":"<9271b185-0051-b946-013a-5d3da35e9937@ideasonboard.com>","date":"2019-12-09T13:25:51","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 09/12/2019 13:20, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Dec 09, 2019 at 01:16:08PM +0000, Kieran Bingham wrote:\n>> On 09/12/2019 12:54, Laurent Pinchart wrote:\n>>> On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:\n>>>> On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:\n>>>>> On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:\n>>>>>> In preparation to add libcamera Camera definition by re-using the\n>>>>>\n>>>>> First sentence is hard for me to parse.\n>>>>\n>>>> Indeed.. I've left the subject behind :)\n>>>>\n>>>> In preparation to add libcamera Camera -properties- definition by re-using the\n>>>>\n>>>>>> control generation framework, augment the gen_controls.py script to\n>>>>>> support parsing the 'values' yaml tag and generate documentation and\n>>>>>> definition of possible values associated with a Control or a Property.\n>>>>>>\n>>>>>> Signed-off-by: Jacopo Mondi \n>>>>>> ---\n>>>>>> src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++\n>>>>>> 1 file changed, 34 insertions(+)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py\n>>>>>> index 940386cc68c8..f86f01f6759b 100755\n>>>>>> --- a/src/libcamera/gen-controls.py\n>>>>>> +++ b/src/libcamera/gen-controls.py\n>>>>>> @@ -17,10 +17,15 @@ def snake_case(s):\n>>>>>>\n>>>>>>\n>>>>>> def generate_cpp(controls):\n>>>>>> + value_doc_template = string.Template('''/**\n>>>>>> + * \\\\def ${name}\n>>>>>> + * \\\\brief ${description} */''')\n>>>>>> +\n>>>>>> doc_template = string.Template('''/**\n>>>>>> * \\\\var extern const Control<${type}> ${name}\n>>>>>> ${description}\n>>>>>> */''')\n>>>>>> +\n>>>>>> def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, \"${name}\");')\n>>>>>>\n>>>>>> ctrls_doc = []\n>>>>>> @@ -35,13 +40,27 @@ ${description}\n>>>>>> description[0] = '\\\\brief ' + description[0]\n>>>>>> description = '\\n'.join([(line and ' * ' or ' *') + line for line in description])\n>>>>>>\n>>>>>> + try:\n>>>>>> + values = ctrl['values']\n>>>>>> + except KeyError:\n>>>>>> + values = \"\"\n>>>>>> +\n>>>>>> info = {\n>>>>>> 'name': name,\n>>>>>> 'type': ctrl['type'],\n>>>>>> 'description': description,\n>>>>>> 'id_name': id_name,\n>>>>>> + 'values' : values,\n>>>>>> }\n>>>>>>\n>>>>>> + for value in values:\n>>>>>> + value_info = {\n>>>>>> + 'name': list(value.keys())[0],\n>>>>>> + 'value': value['value'],\n>>>>>> + 'description': value['description']\n>>>>>> + }\n>>>>>> + ctrls_doc.append(value_doc_template.substitute(value_info))\n>>>>>> +\n>>>>>> ctrls_doc.append(doc_template.substitute(info))\n>>>>>> ctrls_def.append(def_template.substitute(info))\n>>>>>> ctrls_map.append('\\t{ ' + id_name + ', &' + name + ' },')\n>>>>>> @@ -54,6 +73,7 @@ ${description}\n>>>>>>\n>>>>>>\n>>>>>> def generate_h(controls):\n>>>>>> + value_template = string.Template('''#define ${name} ${value}''')\n>>>>>\n>>>>> Defines are probably the way we want to play this but the notion of\n>>>>> using enums jumped into my head, is that something you think could be\n>>>>> used or is it more trouble then it's worth?\n>>>>\n>>>> I admit I have considered enums as well... I think defines are fine\n>>>> for now, but we can add support for enums on top if we think it's\n>>>> better. The only thing I would consider from the very beginning is how\n>>>> to do so.\n>>>>\n>>>> I would keep the 'values' tag for defines and add an 'enum' tag which\n>>>> might look like this\n>>>>\n>>>> controls:\n>>>> - CONTROL_NAME:\n>>>> type: int32_t\n>>>> desription: -|\n>>>> blah blah\n>>>> enum:\n>>>> - NAME_OF_THE_ENUM:\n>>>> values:\n>>>> - FIRST_ENUM_ELEMENT:\n>>>> value: x\n>>>> description: -|\n>>>> blah blah\n>>>> - SECOND_ENUM_ELEMENT:\n>>>> value: x\n>>>> description: -|\n>>>> blah blah\n>>>>\n>>>> The only reason why we should think how enum definition should look\n>>>> like is to make it plays nice with the current use of values, which,\n>>>> for reference is something like\n>>>>\n>>>> controls:\n>>>> - CONTROL_NAME:\n>>>> type: int32_t\n>>>> desription: -|\n>>>> blah blah\n>>>> values:\n>>>> - NAME_OF_THE_DEFINE:\n>>>> value: x\n>>>> description: -|\n>>>> blah blah\n>>>>\n>>>> I'm not sure I like it.\n>>>>\n>>>> I would rather introduce a 'values' section, which could contains\n>>>> (alternatively) a set of definition or an enumeration\n>>>>\n>>>> controls:\n>>>> - CONTROL_NAME:\n>>>> type: int32_t\n>>>> desription: -|\n>>>> blah blah\n>>>> values:\n>>>> defines:\n>>>> - NAME_OF_THE_DEFINE:\n>>>> value: x\n>>>> description: -|\n>>>> blah blah\n>>>> - NAME_OF_ANOHER_DEFINE:\n>>>> value: y\n>>>> description: -|\n>>>> blah blah\n>>>>\n>>>> Or\n>>>>\n>>>> controls:\n>>>> - CONTROL_NAME:\n>>>> type: int32_t\n>>>> desription: -|\n>>>> blah blah\n>>>> values:\n>>>> enum:\n>>>> - NAME_OF_THE_ENUM:\n>>>> values:\n>>>> - FIRST_ENUM_ELEMENT:\n>>>> value: x\n>>>> description: -|\n>>>> blah blah\n>>>> - SECOND_ENUM_ELEMENT:\n>>>> value: y\n>>>> description: -|\n>>>> blah blah\n>>>>\n>>>> So that the only part that changes is the content of the 'values'\n>>>> section.\n>>>>\n>>>> Congratulation, I've just proposed myself more work\n>>>>\n>>>> What do you think, should we consider enum from the beginning, is this\n>>>> an overkill ?\n>>>\n>>> I think we should only have enums, not defines, and I think it shouldn't\n>>> be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,\n>>> you can name it enum ControlNameValues.\n>>>\n>>> ------- control_ids.yaml ------\n>>> controls:\n>>> - WhiteBalanceMode:\n>>> type: int32_t\n>>> desription: -|\n>>> blah blah\n>>> enum:\n>>> - WhiteBalanceModeAuto:\n>>> value: 0\n>>> description: -|\n>>> blah blah\n>>> - WhiteBalanceModeSunny:\n>>> value: 1\n>>> description: -|\n>>> blah blah\n>>> - WhiteBalanceModeCloudy:\n>>> value: 1\n>>\n>> I suspect you mean value: 2 here, but I think that's just a typo in the\n>> psuedo code.\n> \n> Yes, it's a typo, sorry.\n> \n>>> description: -|\n>>> blah blah\n>>> -------------------------------\n>>>\n>>> should generate\n>>>\n>>> -------- control_ids.h --------\n>>> namespace controls {\n>>>\n>>> enum WhiteBalanceModeValues {\n>>> \tWhiteBalanceModeAuto = 0,\n>>> \tWhiteBalanceModeSunny = 1,\n>>> \tWhiteBalanceModeCloudy = 2,\n>>> \t...\n>>> };\n>>>\n>>> extern const Control WhiteBalanceMode;\n>>>\n>>> }\n>>> -------------------------------\n>>\n>>\n>> Would we support automatic values in enums?\n>>\n>> I prefer using enums here over defines, as it scopes the values and will\n>> give the compiler/debug-info more information as to their context too,\n>> which means we will be able to perform more context validation (such as\n>> ensuring all enum values are correctly identified within a switch)\n>>\n>> Once defined, the numbers become part of the ABI, so they'd have to be\n>> constant ... So we probably / possibly need to either document or define\n>> some rule that any new additions are only appended, and not added in any\n>> sort-order.\n> \n> I've thought about that, but it can be a bit error-prone as inserting a\n> new value in the middle may not be caught during review. Furthermore, I\n> think having the explicit values in the documentation is useful for\n> binary IPAs that don't want to link to libcamera. For those reasons I\n> think there's more cons than pros for automatic values.\n\nGood, we're in agreement. That was my point! :-)\n\nWe need to ensure the numbers are consistent.\n\n--\nKieran\n\n> \n>> Otherwise, +1 on the YAML style Laurent has posted. It seems concise and\n>> fills the requirements I can see.\n>>\n>>>>> With or without enums and but with an updated commit message,\n>>>>>\n>>>>> Reviewed-by: Niklas Söderlund \n>>>>>\n>>>>>> template = string.Template('''extern const Control<${type}> ${name};''')\n>>>>>>\n>>>>>> ctrls = []\n>>>>>> @@ -66,11 +86,25 @@ def generate_h(controls):\n>>>>>>\n>>>>>> ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n>>>>>>\n>>>>>> + try:\n>>>>>> + values = ctrl['values']\n>>>>>> + except KeyError:\n>>>>>> + values = \"\"\n>>>>>> +\n>>>>>> info = {\n>>>>>> 'name': name,\n>>>>>> 'type': ctrl['type'],\n>>>>>> + 'values' : values,\n>>>>>> }\n>>>>>>\n>>>>>> + for value in values:\n>>>>>> + value_info = {\n>>>>>> + 'name': list(value.keys())[0],\n>>>>>> + 'value': value['value'],\n>>>>>> + 'description': value['description']\n>>>>>> + }\n>>>>>> + ctrls.append(value_template.substitute(value_info))\n>>>>>> +\n>>>>>> ctrls.append(template.substitute(info))\n>>>>>> id_value += 1\n>>>>>>\n>","headers":{"Return-Path":"","Received":["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 902F160BC4\n\tfor ;\n\tMon, 9 Dec 2019 14:25:55 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A436499A;\n\tMon, 9 Dec 2019 14:25:54 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575897955;\n\tbh=WHy/y2nRp1OFZYO95RGaHD0xeDUdNXOoYWx28ivemAw=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=fCkI8r/7RyLbgeR7bVovf5IavI05L1+jq/lhkn7dXdAYDsFXj/Y96o9kGAyS/KGG0\n\t6L6lHH6/7Cz53O1275FmDa+g0LtayEqh+lQoIK2NX/V2YWfmFbxBu0GA6pZYCTn+nU\n\tir67YYTDklC3gmBAVkJm0zA11nO40LWxwoe3FmI0=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20191205204350.28196-1-jacopo@jmondi.org>\n\t<20191205204350.28196-3-jacopo@jmondi.org>\n\t<20191206190904.GL28879@bigcity.dyn.berto.se>\n\t<20191208180747.iujblmulqrnu2xfd@uno.localdomain>\n\t<20191209125429.GB9158@pendragon.ideasonboard.com>\n\t<11f8a2a1-d534-c037-da6a-c97254a7cc99@ideasonboard.com>\n\t<20191209132016.GC9158@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<9271b185-0051-b946-013a-5d3da35e9937@ideasonboard.com>","Date":"Mon, 9 Dec 2019 13:25:51 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20191209132016.GC9158@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse\n\t'values' in gen-controls.py","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":"Mon, 09 Dec 2019 13:25:55 -0000"}}]