[{"id":2510,"web_url":"https://patchwork.libcamera.org/comment/2510/","msgid":"<20190828095816.GL28351@bigcity.dyn.berto.se>","date":"2019-08-28T09:58:16","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","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-08-27 11:50:02 +0200, Jacopo Mondi wrote:\n> The control identifiers documentation was not generated as they're not\n> part of the controls.h file documented in controls.cpp.\n> \n> Move documentation for control id to the end of the controls.cpp and\n> document the control_ids.h file to have documentation for controls\n> properly generated.\n\nI'm a bit debated about this solution. I see why the change is needed \nbut I wonder if it's not nicer to create a control_ids.cpp file to hold \nthe documentation?\n\nAs this is as far as I know the first time we document two header files \nin one cpp file I want to know what others think about this practice. \nI'm not strongly against it, but found myself reading the diff 3 times \nbefore I understood it ;-)\n\nIf the consensus is that we should document multiple header files in one \ncpp file feel free to add (with the typo bellow fixed),\n\nReviewed-by: Niklas Söderlund \n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------\n> 1 file changed, 53 insertions(+), 47 deletions(-)\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 727fdbd9450d..9adc3badc254 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -181,53 +181,6 @@ std::string ControlValue::toString() const\n> \treturn \"\";\n> }\n> \n> -/**\n> - * \\enum ControlId\n> - * \\brief Numerical control ID\n> - */\n> -\n> -/**\n> - * \\var AwbEnable\n> - * ControlType: Bool\n> - *\n> - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> - */\n> -\n> -/**\n> - * \\var Brightness\n> - * ControlType: Integer\n> - *\n> - * Specify a fixed brightness parameter.\n> - */\n> -\n> -/**\n> - * \\var Contrast\n> - * ControlType: Integer\n> - *\n> - * Specify a fixed contrast parameter.\n> - */\n> -\n> -/**\n> - * \\var Saturation\n> - * ControlType: Integer\n> - *\n> - * Specify a fixed saturation parameter.\n> - */\n> -\n> -/**\n> - * \\var ManualExposure\n> - * ControlType: Integer\n> - *\n> - * Specify a fixed exposure time in milli-seconds\n> - */\n> -\n> -/**\n> - * \\var ManualGain\n> - * ControlType: Integer\n> - *\n> - * Specify a fixed gain parameter\n> - */\n> -\n> /**\n> * \\struct ControlIdentifier\n> * \\brief Describe a ControlId with control specific constant meta-data\n> @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)\n> \t}\n> }\n> \n> +/**\n> + * \\file control_ids.h\n> + * \\brief Definition of numerical identifiers for libcamera control and\n> + * properties\n\ns/control/controls/\n\n> + */\n> +\n> +/**\n> + * \\enum ControlId\n> + * \\brief Numerical control ID\n> + */\n> +\n> +/**\n> + * \\var AwbEnable\n> + * ControlType: Bool\n> + *\n> + * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> + */\n> +\n> +/**\n> + * \\var Brightness\n> + * ControlType: Integer\n> + *\n> + * Specify a fixed brightness parameter.\n> + */\n> +\n> +/**\n> + * \\var Contrast\n> + * ControlType: Integer\n> + *\n> + * Specify a fixed contrast parameter.\n> + */\n> +\n> +/**\n> + * \\var Saturation\n> + * ControlType: Integer\n> + *\n> + * Specify a fixed saturation parameter.\n> + */\n> +\n> +/**\n> + * \\var ManualExposure\n> + * ControlType: Integer\n> + *\n> + * Specify a fixed exposure time in milli-seconds\n> + */\n> +\n> +/**\n> + * \\var ManualGain\n> + * ControlType: Integer\n> + *\n> + * Specify a fixed gain parameter\n> + */\n> +\n> } /* namespace libcamera */\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-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94FB060BB0\n\tfor ;\n\tWed, 28 Aug 2019 11:58:19 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id u29so1614559lfk.7\n\tfor ;\n\tWed, 28 Aug 2019 02:58:19 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\tc22sm736227lfm.13.2019.08.28.02.58.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Aug 2019 02:58:17 -0700 (PDT)"],"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=QXPgITZwOi97OYiUfpx7b9TW1lHYSxLs2yyu2Lnyt0U=;\n\tb=cIIUOY0fJgAzEimgp1wDrQA7jMZn7t9qxMqaxKcxoDkRkk8qonuZ1bwSfVJUXTwJ9a\n\tXMejjQu8dUi9UzXfK4/M3b+Uc4kyxfdCykGRdNAHKuzE1iaflXVPv/vMbukbsaPAuRIL\n\tb+b6MvLnctg2fbszSG0yR6GDs5Cf+ISU/cvWwxPfia3RhnZmRYl5yHkdJXiTv6eEHtB2\n\ty4m+ROJPLe8FCZYYb35DNJLOBKhwER9FggmgxSHiDtYPxrq+atLOefrRvhRx+3BE23fn\n\tvZ0meFPkzAjqOGWIsFogvibCPhEO+5P8tS7ETtqk/I2f9pI86sUrn10tFP3Knsz93mt0\n\t3Bng==","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=QXPgITZwOi97OYiUfpx7b9TW1lHYSxLs2yyu2Lnyt0U=;\n\tb=jXT2byMPMtcRsE2vQi0R0QLsxl3R0Yo1Fw355NcwWxa0V9gT/SQfCM0BaZ2dUbMY58\n\tDViIIW64j92JYs5/8Y3ntf3SECIM0uwvM+HOh8Y7GF5TFnzJIfr/GoV+qPMFt2a+K7v4\n\tQkLV94aYe2bBuVznhqrusCkPNUXxfhHRqQosDKqhkpnAH1m9RuoFVYFNMgVPCeTF/Wx9\n\tmD8RJcJzHIaQRhRDmLN0IRKipi2W0NxkwZsgZYV6lvLcekRB7gO5QW61xBYeFg6BwyyV\n\tbJgpPVJ+Qdg1H2e4EVDpLokYmA01q/yWbA/H0tRY14v+y4NaCk/hZAFGQDQsHR6z/QRb\n\tFdGw==","X-Gm-Message-State":"APjAAAX0qf8Hyb0Yzgu9JuBaJWUMY8eLfxm4AjWOdoqzLsVxjtV0OvJM\n\tSyCv756cuheNUjTjagDu1w5oqYoQQMw=","X-Google-Smtp-Source":"APXvYqxpIkzo1kn+r3yHInIgtneLbFknWJ9v2cVW9kvILB6TWYJiR1uMYTVaBvOxr7wRInrmanq4CQ==","X-Received":"by 2002:a19:6556:: with SMTP id c22mr2092831lfj.90.1566986298729;\n\tWed, 28 Aug 2019 02:58:18 -0700 (PDT)","Date":"Wed, 28 Aug 2019 11:58:16 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828095816.GL28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-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":"<20190827095008.11405-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 09:58:19 -0000"}},{"id":2518,"web_url":"https://patchwork.libcamera.org/comment/2518/","msgid":"<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>","date":"2019-08-28T15:18:59","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Aug 28, 2019 at 11:58:16AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-08-27 11:50:02 +0200, Jacopo Mondi wrote:\n> > The control identifiers documentation was not generated as they're not\n> > part of the controls.h file documented in controls.cpp.\n> >\n> > Move documentation for control id to the end of the controls.cpp and\n> > document the control_ids.h file to have documentation for controls\n> > properly generated.\n>\n> I'm a bit debated about this solution. I see why the change is needed\n> but I wonder if it's not nicer to create a control_ids.cpp file to hold\n> the documentation?\n\nI was just reading ipa_module.c and that's what I've found there\n/**\n\n * \\file ipa_module.h\n * \\brief Image Processing Algorithm module\n */\n\n/**\n * \\file ipa_module_info.h\n * \\brief Image Processing Algorithm module information\n */\n\nIt's not a new thing then\n\nThanks\n j\n>\n> As this is as far as I know the first time we document two header files\n> in one cpp file I want to know what others think about this practice.\n> I'm not strongly against it, but found myself reading the diff 3 times\n> before I understood it ;-)\n>\n> If the consensus is that we should document multiple header files in one\n> cpp file feel free to add (with the typo bellow fixed),\n>\n> Reviewed-by: Niklas Söderlund \n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------\n> > 1 file changed, 53 insertions(+), 47 deletions(-)\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 727fdbd9450d..9adc3badc254 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -181,53 +181,6 @@ std::string ControlValue::toString() const\n> > \treturn \"\";\n> > }\n> >\n> > -/**\n> > - * \\enum ControlId\n> > - * \\brief Numerical control ID\n> > - */\n> > -\n> > -/**\n> > - * \\var AwbEnable\n> > - * ControlType: Bool\n> > - *\n> > - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> > - */\n> > -\n> > -/**\n> > - * \\var Brightness\n> > - * ControlType: Integer\n> > - *\n> > - * Specify a fixed brightness parameter.\n> > - */\n> > -\n> > -/**\n> > - * \\var Contrast\n> > - * ControlType: Integer\n> > - *\n> > - * Specify a fixed contrast parameter.\n> > - */\n> > -\n> > -/**\n> > - * \\var Saturation\n> > - * ControlType: Integer\n> > - *\n> > - * Specify a fixed saturation parameter.\n> > - */\n> > -\n> > -/**\n> > - * \\var ManualExposure\n> > - * ControlType: Integer\n> > - *\n> > - * Specify a fixed exposure time in milli-seconds\n> > - */\n> > -\n> > -/**\n> > - * \\var ManualGain\n> > - * ControlType: Integer\n> > - *\n> > - * Specify a fixed gain parameter\n> > - */\n> > -\n> > /**\n> > * \\struct ControlIdentifier\n> > * \\brief Describe a ControlId with control specific constant meta-data\n> > @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)\n> > \t}\n> > }\n> >\n> > +/**\n> > + * \\file control_ids.h\n> > + * \\brief Definition of numerical identifiers for libcamera control and\n> > + * properties\n>\n> s/control/controls/\n>\n> > + */\n> > +\n> > +/**\n> > + * \\enum ControlId\n> > + * \\brief Numerical control ID\n> > + */\n> > +\n> > +/**\n> > + * \\var AwbEnable\n> > + * ControlType: Bool\n> > + *\n> > + * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> > + */\n> > +\n> > +/**\n> > + * \\var Brightness\n> > + * ControlType: Integer\n> > + *\n> > + * Specify a fixed brightness parameter.\n> > + */\n> > +\n> > +/**\n> > + * \\var Contrast\n> > + * ControlType: Integer\n> > + *\n> > + * Specify a fixed contrast parameter.\n> > + */\n> > +\n> > +/**\n> > + * \\var Saturation\n> > + * ControlType: Integer\n> > + *\n> > + * Specify a fixed saturation parameter.\n> > + */\n> > +\n> > +/**\n> > + * \\var ManualExposure\n> > + * ControlType: Integer\n> > + *\n> > + * Specify a fixed exposure time in milli-seconds\n> > + */\n> > +\n> > +/**\n> > + * \\var ManualGain\n> > + * ControlType: Integer\n> > + *\n> > + * Specify a fixed gain parameter\n> > + */\n> > +\n> > } /* namespace libcamera */\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 relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4068C60C1E\n\tfor ;\n\tWed, 28 Aug 2019 17:17:28 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id AE0C9FF802;\n\tWed, 28 Aug 2019 15:17:27 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 28 Aug 2019 17:18:59 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-3-jacopo@jmondi.org>\n\t<20190828095816.GL28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ragtguzjicbrtwe3\"","Content-Disposition":"inline","In-Reply-To":"<20190828095816.GL28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 15:17:28 -0000"}},{"id":2577,"web_url":"https://patchwork.libcamera.org/comment/2577/","msgid":"<20190903201746.GF4788@pendragon.ideasonboard.com>","date":"2019-09-03T20:17:46","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Aug 28, 2019 at 05:18:59PM +0200, Jacopo Mondi wrote:\n> On Wed, Aug 28, 2019 at 11:58:16AM +0200, Niklas Söderlund wrote:\n> > On 2019-08-27 11:50:02 +0200, Jacopo Mondi wrote:\n> >> The control identifiers documentation was not generated as they're not\n> >> part of the controls.h file documented in controls.cpp.\n> >>\n> >> Move documentation for control id to the end of the controls.cpp and\n> >> document the control_ids.h file to have documentation for controls\n> >> properly generated.\n> >\n> > I'm a bit debated about this solution. I see why the change is needed\n> > but I wonder if it's not nicer to create a control_ids.cpp file to hold\n> > the documentation?\n> \n> I was just reading ipa_module.c and that's what I've found there\n> \n> /**\n> * \\file ipa_module.h\n> * \\brief Image Processing Algorithm module\n> */\n> \n> /**\n> * \\file ipa_module_info.h\n> * \\brief Image Processing Algorithm module information\n> */\n> \n> It's not a new thing then\n\nIt's not new, but we also have precedents for empty .cpp files just for\ndocumentation purpose (ipa_module.cpp is one of them), so I'm not\nopposed to creating control_ids.cpp. As I still plan to rework the\ncontrols implementation, decoupling it from the control ids\ndocumentation may make sense and make things simpler.\n\n> > As this is as far as I know the first time we document two header files\n> > in one cpp file I want to know what others think about this practice.\n> > I'm not strongly against it, but found myself reading the diff 3 times\n> > before I understood it ;-)\n> >\n> > If the consensus is that we should document multiple header files in one\n> > cpp file feel free to add (with the typo bellow fixed),\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------\n> >> 1 file changed, 53 insertions(+), 47 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >> index 727fdbd9450d..9adc3badc254 100644\n> >> --- a/src/libcamera/controls.cpp\n> >> +++ b/src/libcamera/controls.cpp\n> >> @@ -181,53 +181,6 @@ std::string ControlValue::toString() const\n> >> \treturn \"\";\n> >> }\n> >>\n> >> -/**\n> >> - * \\enum ControlId\n> >> - * \\brief Numerical control ID\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var AwbEnable\n> >> - * ControlType: Bool\n> >> - *\n> >> - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var Brightness\n> >> - * ControlType: Integer\n> >> - *\n> >> - * Specify a fixed brightness parameter.\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var Contrast\n> >> - * ControlType: Integer\n> >> - *\n> >> - * Specify a fixed contrast parameter.\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var Saturation\n> >> - * ControlType: Integer\n> >> - *\n> >> - * Specify a fixed saturation parameter.\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var ManualExposure\n> >> - * ControlType: Integer\n> >> - *\n> >> - * Specify a fixed exposure time in milli-seconds\n> >> - */\n> >> -\n> >> -/**\n> >> - * \\var ManualGain\n> >> - * ControlType: Integer\n> >> - *\n> >> - * Specify a fixed gain parameter\n> >> - */\n> >> -\n> >> /**\n> >> * \\struct ControlIdentifier\n> >> * \\brief Describe a ControlId with control specific constant meta-data\n> >> @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)\n> >> \t}\n> >> }\n> >>\n> >> +/**\n> >> + * \\file control_ids.h\n> >> + * \\brief Definition of numerical identifiers for libcamera control and\n> >> + * properties\n> >\n> > s/control/controls/\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\enum ControlId\n> >> + * \\brief Numerical control ID\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var AwbEnable\n> >> + * ControlType: Bool\n> >> + *\n> >> + * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var Brightness\n> >> + * ControlType: Integer\n> >> + *\n> >> + * Specify a fixed brightness parameter.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var Contrast\n> >> + * ControlType: Integer\n> >> + *\n> >> + * Specify a fixed contrast parameter.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var Saturation\n> >> + * ControlType: Integer\n> >> + *\n> >> + * Specify a fixed saturation parameter.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var ManualExposure\n> >> + * ControlType: Integer\n> >> + *\n> >> + * Specify a fixed exposure time in milli-seconds\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var ManualGain\n> >> + * ControlType: Integer\n> >> + *\n> >> + * Specify a fixed gain parameter\n> >> + */\n> >> +\n> >> } /* namespace libcamera */","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 48B1460BCF\n\tfor ;\n\tTue, 3 Sep 2019 22:17:55 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-18-41-nat.elisa-mobile.fi\n\t[85.76.18.41])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCE8B542;\n\tTue, 3 Sep 2019 22:17:54 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567541875;\n\tbh=YSgUWL7NBN7Wg45afEZjLoPe3Yo9YIzZE23JWsy8kA8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Hb3UWDNPkwU/wPADQIM4bTJxWoMq2C7fTrMW9RfCYdd2Obwl05M2x0iAMLQgEOT47\n\tpsa4f8o1ACfBshBMSgXZcnGQDQEwQxtNlbaoCIJTO9w1ZEiPsZnW7KTET2OAJy410F\n\trC4tEqX0RCkMGGRxPhuRgksn0HY4+AfE89lSdwPM=","Date":"Tue, 3 Sep 2019 23:17:46 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190903201746.GF4788@pendragon.ideasonboard.com>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-3-jacopo@jmondi.org>\n\t<20190828095816.GL28351@bigcity.dyn.berto.se>\n\t<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 03 Sep 2019 20:17:55 -0000"}},{"id":2583,"web_url":"https://patchwork.libcamera.org/comment/2583/","msgid":"<20190904122923.6mpl5w3ssnph63j3@uno.localdomain>","date":"2019-09-04T12:29:23","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello Niklas, Laurent,\n\nOn Tue, Sep 03, 2019 at 11:17:46PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Wed, Aug 28, 2019 at 05:18:59PM +0200, Jacopo Mondi wrote:\n> > On Wed, Aug 28, 2019 at 11:58:16AM +0200, Niklas Söderlund wrote:\n> > > On 2019-08-27 11:50:02 +0200, Jacopo Mondi wrote:\n> > >> The control identifiers documentation was not generated as they're not\n> > >> part of the controls.h file documented in controls.cpp.\n> > >>\n> > >> Move documentation for control id to the end of the controls.cpp and\n> > >> document the control_ids.h file to have documentation for controls\n> > >> properly generated.\n> > >\n> > > I'm a bit debated about this solution. I see why the change is needed\n> > > but I wonder if it's not nicer to create a control_ids.cpp file to hold\n> > > the documentation?\n> >\n> > I was just reading ipa_module.c and that's what I've found there\n> >\n> > /**\n> > * \\file ipa_module.h\n> > * \\brief Image Processing Algorithm module\n> > */\n> >\n> > /**\n> > * \\file ipa_module_info.h\n> > * \\brief Image Processing Algorithm module information\n> > */\n> >\n> > It's not a new thing then\n>\n> It's not new, but we also have precedents for empty .cpp files just for\n> documentation purpose (ipa_module.cpp is one of them), so I'm not\n> opposed to creating control_ids.cpp. As I still plan to rework the\n> controls implementation, decoupling it from the control ids\n> documentation may make sense and make things simpler.\n>\n\nWhen I first changed this, I have missed the following machinery in\nsrc/libcamera/meson.build\n\ngen_controls = files('gen-controls.awk')\ncontrol_types_cpp = custom_target('control_types_cpp',\n input : files('controls.cpp'),\n output : 'control_types.cpp',\n depend_files : gen_controls,\n command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])\n\nThat creates a control_types.cpp file parsing controls.cpp to extract\nthe documentation for control. Why this complication instead of simply\ndocumenting controls in their own .cpp file or create a\n/** \\file control_ids.h */ section in controls.cpp like this patch\ndoes ?\n\nThanks\n j\n\n> > > As this is as far as I know the first time we document two header files\n> > > in one cpp file I want to know what others think about this practice.\n> > > I'm not strongly against it, but found myself reading the diff 3 times\n> > > before I understood it ;-)\n> > >\n> > > If the consensus is that we should document multiple header files in one\n> > > cpp file feel free to add (with the typo bellow fixed),\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------\n> > >> 1 file changed, 53 insertions(+), 47 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > >> index 727fdbd9450d..9adc3badc254 100644\n> > >> --- a/src/libcamera/controls.cpp\n> > >> +++ b/src/libcamera/controls.cpp\n> > >> @@ -181,53 +181,6 @@ std::string ControlValue::toString() const\n> > >> \treturn \"\";\n> > >> }\n> > >>\n> > >> -/**\n> > >> - * \\enum ControlId\n> > >> - * \\brief Numerical control ID\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var AwbEnable\n> > >> - * ControlType: Bool\n> > >> - *\n> > >> - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var Brightness\n> > >> - * ControlType: Integer\n> > >> - *\n> > >> - * Specify a fixed brightness parameter.\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var Contrast\n> > >> - * ControlType: Integer\n> > >> - *\n> > >> - * Specify a fixed contrast parameter.\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var Saturation\n> > >> - * ControlType: Integer\n> > >> - *\n> > >> - * Specify a fixed saturation parameter.\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var ManualExposure\n> > >> - * ControlType: Integer\n> > >> - *\n> > >> - * Specify a fixed exposure time in milli-seconds\n> > >> - */\n> > >> -\n> > >> -/**\n> > >> - * \\var ManualGain\n> > >> - * ControlType: Integer\n> > >> - *\n> > >> - * Specify a fixed gain parameter\n> > >> - */\n> > >> -\n> > >> /**\n> > >> * \\struct ControlIdentifier\n> > >> * \\brief Describe a ControlId with control specific constant meta-data\n> > >> @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)\n> > >> \t}\n> > >> }\n> > >>\n> > >> +/**\n> > >> + * \\file control_ids.h\n> > >> + * \\brief Definition of numerical identifiers for libcamera control and\n> > >> + * properties\n> > >\n> > > s/control/controls/\n> > >\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\enum ControlId\n> > >> + * \\brief Numerical control ID\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var AwbEnable\n> > >> + * ControlType: Bool\n> > >> + *\n> > >> + * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var Brightness\n> > >> + * ControlType: Integer\n> > >> + *\n> > >> + * Specify a fixed brightness parameter.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var Contrast\n> > >> + * ControlType: Integer\n> > >> + *\n> > >> + * Specify a fixed contrast parameter.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var Saturation\n> > >> + * ControlType: Integer\n> > >> + *\n> > >> + * Specify a fixed saturation parameter.\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var ManualExposure\n> > >> + * ControlType: Integer\n> > >> + *\n> > >> + * Specify a fixed exposure time in milli-seconds\n> > >> + */\n> > >> +\n> > >> +/**\n> > >> + * \\var ManualGain\n> > >> + * ControlType: Integer\n> > >> + *\n> > >> + * Specify a fixed gain parameter\n> > >> + */\n> > >> +\n> > >> } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5648E60BB2\n\tfor ;\n\tWed, 4 Sep 2019 14:27:51 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 7AEEA20000C;\n\tWed, 4 Sep 2019 12:27:50 +0000 (UTC)"],"Date":"Wed, 4 Sep 2019 14:29:23 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190904122923.6mpl5w3ssnph63j3@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-3-jacopo@jmondi.org>\n\t<20190828095816.GL28351@bigcity.dyn.berto.se>\n\t<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>\n\t<20190903201746.GF4788@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fvgmplfgnyqvwrvd\"","Content-Disposition":"inline","In-Reply-To":"<20190903201746.GF4788@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 04 Sep 2019 12:27:51 -0000"}},{"id":2584,"web_url":"https://patchwork.libcamera.org/comment/2584/","msgid":"<20190904123444.GA9548@pendragon.ideasonboard.com>","date":"2019-09-04T12:34:44","subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 04, 2019 at 02:29:23PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 03, 2019 at 11:17:46PM +0300, Laurent Pinchart wrote:\n> > On Wed, Aug 28, 2019 at 05:18:59PM +0200, Jacopo Mondi wrote:\n> >> On Wed, Aug 28, 2019 at 11:58:16AM +0200, Niklas Söderlund wrote:\n> >>> On 2019-08-27 11:50:02 +0200, Jacopo Mondi wrote:\n> >>>> The control identifiers documentation was not generated as they're not\n> >>>> part of the controls.h file documented in controls.cpp.\n> >>>>\n> >>>> Move documentation for control id to the end of the controls.cpp and\n> >>>> document the control_ids.h file to have documentation for controls\n> >>>> properly generated.\n> >>>\n> >>> I'm a bit debated about this solution. I see why the change is needed\n> >>> but I wonder if it's not nicer to create a control_ids.cpp file to hold\n> >>> the documentation?\n> >>\n> >> I was just reading ipa_module.c and that's what I've found there\n> >>\n> >> /**\n> >> * \\file ipa_module.h\n> >> * \\brief Image Processing Algorithm module\n> >> */\n> >>\n> >> /**\n> >> * \\file ipa_module_info.h\n> >> * \\brief Image Processing Algorithm module information\n> >> */\n> >>\n> >> It's not a new thing then\n> >\n> > It's not new, but we also have precedents for empty .cpp files just for\n> > documentation purpose (ipa_module.cpp is one of them), so I'm not\n> > opposed to creating control_ids.cpp. As I still plan to rework the\n> > controls implementation, decoupling it from the control ids\n> > documentation may make sense and make things simpler.\n> \n> When I first changed this, I have missed the following machinery in\n> src/libcamera/meson.build\n> \n> gen_controls = files('gen-controls.awk')\n> control_types_cpp = custom_target('control_types_cpp',\n> input : files('controls.cpp'),\n> output : 'control_types.cpp',\n> depend_files : gen_controls,\n> command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])\n> \n> That creates a control_types.cpp file parsing controls.cpp to extract\n> the documentation for control. Why this complication instead of simply\n> documenting controls in their own .cpp file or create a\n> /** \\file control_ids.h */ section in controls.cpp like this patch\n> does ?\n\nIt's not about documentation, there's an automatically generated table\nof data in control_types.cpp.\n\n> >>> As this is as far as I know the first time we document two header files\n> >>> in one cpp file I want to know what others think about this practice.\n> >>> I'm not strongly against it, but found myself reading the diff 3 times\n> >>> before I understood it ;-)\n> >>>\n> >>> If the consensus is that we should document multiple header files in one\n> >>> cpp file feel free to add (with the typo bellow fixed),\n> >>>\n> >>> Reviewed-by: Niklas Söderlund \n> >>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------\n> >>>> 1 file changed, 53 insertions(+), 47 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>> index 727fdbd9450d..9adc3badc254 100644\n> >>>> --- a/src/libcamera/controls.cpp\n> >>>> +++ b/src/libcamera/controls.cpp\n> >>>> @@ -181,53 +181,6 @@ std::string ControlValue::toString() const\n> >>>> \treturn \"\";\n> >>>> }\n> >>>>\n> >>>> -/**\n> >>>> - * \\enum ControlId\n> >>>> - * \\brief Numerical control ID\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var AwbEnable\n> >>>> - * ControlType: Bool\n> >>>> - *\n> >>>> - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var Brightness\n> >>>> - * ControlType: Integer\n> >>>> - *\n> >>>> - * Specify a fixed brightness parameter.\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var Contrast\n> >>>> - * ControlType: Integer\n> >>>> - *\n> >>>> - * Specify a fixed contrast parameter.\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var Saturation\n> >>>> - * ControlType: Integer\n> >>>> - *\n> >>>> - * Specify a fixed saturation parameter.\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var ManualExposure\n> >>>> - * ControlType: Integer\n> >>>> - *\n> >>>> - * Specify a fixed exposure time in milli-seconds\n> >>>> - */\n> >>>> -\n> >>>> -/**\n> >>>> - * \\var ManualGain\n> >>>> - * ControlType: Integer\n> >>>> - *\n> >>>> - * Specify a fixed gain parameter\n> >>>> - */\n> >>>> -\n> >>>> /**\n> >>>> * \\struct ControlIdentifier\n> >>>> * \\brief Describe a ControlId with control specific constant meta-data\n> >>>> @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)\n> >>>> \t}\n> >>>> }\n> >>>>\n> >>>> +/**\n> >>>> + * \\file control_ids.h\n> >>>> + * \\brief Definition of numerical identifiers for libcamera control and\n> >>>> + * properties\n> >>>\n> >>> s/control/controls/\n> >>>\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\enum ControlId\n> >>>> + * \\brief Numerical control ID\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var AwbEnable\n> >>>> + * ControlType: Bool\n> >>>> + *\n> >>>> + * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var Brightness\n> >>>> + * ControlType: Integer\n> >>>> + *\n> >>>> + * Specify a fixed brightness parameter.\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var Contrast\n> >>>> + * ControlType: Integer\n> >>>> + *\n> >>>> + * Specify a fixed contrast parameter.\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var Saturation\n> >>>> + * ControlType: Integer\n> >>>> + *\n> >>>> + * Specify a fixed saturation parameter.\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var ManualExposure\n> >>>> + * ControlType: Integer\n> >>>> + *\n> >>>> + * Specify a fixed exposure time in milli-seconds\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\var ManualGain\n> >>>> + * ControlType: Integer\n> >>>> + *\n> >>>> + * Specify a fixed gain parameter\n> >>>> + */\n> >>>> +\n> >>>> } /* namespace libcamera */","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 6B09660BB2\n\tfor ;\n\tWed, 4 Sep 2019 14:34:51 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(mobile-access-bceeb5-228.dhcp.inet.fi [188.238.181.228])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5E25440;\n\tWed, 4 Sep 2019 14:34:50 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567600491;\n\tbh=1JO4sPBttvyMd2mpuqTlMul+igp+rrk786++ovaIriU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R09ntDrTK6z7C9/pkggZ6INsE4M6cZ6Er3YPNo0E/lEIs2nMBh2OUbkZ72jncVGG1\n\tluZFT1PnWkodjII3Jbcp3LAJ2ej2oeuGr6LRVqziS3gjYiFjLl5dTb9/oZG0JG6IEl\n\thEc31EBfUwLKkzCkXYLK0hsAsWLOydXmCAlyT0Y0=","Date":"Wed, 4 Sep 2019 15:34:44 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190904123444.GA9548@pendragon.ideasonboard.com>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-3-jacopo@jmondi.org>\n\t<20190828095816.GL28351@bigcity.dyn.berto.se>\n\t<20190828151859.m5wvwrezsfu3sewc@uno.localdomain>\n\t<20190903201746.GF4788@pendragon.ideasonboard.com>\n\t<20190904122923.6mpl5w3ssnph63j3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190904122923.6mpl5w3ssnph63j3@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document\n\tcontrol_ids.h","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 04 Sep 2019 12:34:51 -0000"}}]