[libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse 'values' in gen-controls.py
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 9 14:17:45 CET 2019
Hi Laurent,
On Mon, Dec 09, 2019 at 02:54:29PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:
> > On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:
> > > > In preparation to add libcamera Camera definition by re-using the
> > >
> > > First sentence is hard for me to parse.
> >
> > Indeed.. I've left the subject behind :)
> >
> > In preparation to add libcamera Camera -properties- definition by re-using the
> >
> > > > control generation framework, augment the gen_controls.py script to
> > > > support parsing the 'values' yaml tag and generate documentation and
> > > > definition of possible values associated with a Control or a Property.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > > src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
> > > > index 940386cc68c8..f86f01f6759b 100755
> > > > --- a/src/libcamera/gen-controls.py
> > > > +++ b/src/libcamera/gen-controls.py
> > > > @@ -17,10 +17,15 @@ def snake_case(s):
> > > >
> > > >
> > > > def generate_cpp(controls):
> > > > + value_doc_template = string.Template('''/**
> > > > + * \\def ${name}
> > > > + * \\brief ${description} */''')
> > > > +
> > > > doc_template = string.Template('''/**
> > > > * \\var extern const Control<${type}> ${name}
> > > > ${description}
> > > > */''')
> > > > +
> > > > def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
> > > >
> > > > ctrls_doc = []
> > > > @@ -35,13 +40,27 @@ ${description}
> > > > description[0] = '\\brief ' + description[0]
> > > > description = '\n'.join([(line and ' * ' or ' *') + line for line in description])
> > > >
> > > > + try:
> > > > + values = ctrl['values']
> > > > + except KeyError:
> > > > + values = ""
> > > > +
> > > > info = {
> > > > 'name': name,
> > > > 'type': ctrl['type'],
> > > > 'description': description,
> > > > 'id_name': id_name,
> > > > + 'values' : values,
> > > > }
> > > >
> > > > + for value in values:
> > > > + value_info = {
> > > > + 'name': list(value.keys())[0],
> > > > + 'value': value['value'],
> > > > + 'description': value['description']
> > > > + }
> > > > + ctrls_doc.append(value_doc_template.substitute(value_info))
> > > > +
> > > > ctrls_doc.append(doc_template.substitute(info))
> > > > ctrls_def.append(def_template.substitute(info))
> > > > ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
> > > > @@ -54,6 +73,7 @@ ${description}
> > > >
> > > >
> > > > def generate_h(controls):
> > > > + value_template = string.Template('''#define ${name} ${value}''')
> > >
> > > Defines are probably the way we want to play this but the notion of
> > > using enums jumped into my head, is that something you think could be
> > > used or is it more trouble then it's worth?
> >
> > I admit I have considered enums as well... I think defines are fine
> > for now, but we can add support for enums on top if we think it's
> > better. The only thing I would consider from the very beginning is how
> > to do so.
> >
> > I would keep the 'values' tag for defines and add an 'enum' tag which
> > might look like this
> >
> > controls:
> > - CONTROL_NAME:
> > type: int32_t
> > desription: -|
> > blah blah
> > enum:
> > - NAME_OF_THE_ENUM:
> > values:
> > - FIRST_ENUM_ELEMENT:
> > value: x
> > description: -|
> > blah blah
> > - SECOND_ENUM_ELEMENT:
> > value: x
> > description: -|
> > blah blah
> >
> > The only reason why we should think how enum definition should look
> > like is to make it plays nice with the current use of values, which,
> > for reference is something like
> >
> > controls:
> > - CONTROL_NAME:
> > type: int32_t
> > desription: -|
> > blah blah
> > values:
> > - NAME_OF_THE_DEFINE:
> > value: x
> > description: -|
> > blah blah
> >
> > I'm not sure I like it.
> >
> > I would rather introduce a 'values' section, which could contains
> > (alternatively) a set of definition or an enumeration
> >
> > controls:
> > - CONTROL_NAME:
> > type: int32_t
> > desription: -|
> > blah blah
> > values:
> > defines:
> > - NAME_OF_THE_DEFINE:
> > value: x
> > description: -|
> > blah blah
> > - NAME_OF_ANOHER_DEFINE:
> > value: y
> > description: -|
> > blah blah
> >
> > Or
> >
> > controls:
> > - CONTROL_NAME:
> > type: int32_t
> > desription: -|
> > blah blah
> > values:
> > enum:
> > - NAME_OF_THE_ENUM:
> > values:
> > - FIRST_ENUM_ELEMENT:
> > value: x
> > description: -|
> > blah blah
> > - SECOND_ENUM_ELEMENT:
> > value: y
> > description: -|
> > blah blah
> >
> > So that the only part that changes is the content of the 'values'
> > section.
> >
> > Congratulation, I've just proposed myself more work
> >
> > What do you think, should we consider enum from the beginning, is this
> > an overkill ?
>
> I think we should only have enums, not defines, and I think it shouldn't
> be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,
> you can name it enum ControlNameValues.
>
> ------- control_ids.yaml ------
> controls:
> - WhiteBalanceMode:
> type: int32_t
> desription: -|
> blah blah
> enum:
> - WhiteBalanceModeAuto:
> value: 0
> description: -|
> blah blah
> - WhiteBalanceModeSunny:
> value: 1
> description: -|
> blah blah
> - WhiteBalanceModeCloudy:
> value: 1
> description: -|
> blah blah
> -------------------------------
>
> should generate
>
> -------- control_ids.h --------
> namespace controls {
>
> enum WhiteBalanceModeValues {
> WhiteBalanceModeAuto = 0,
> WhiteBalanceModeSunny = 1,
> WhiteBalanceModeCloudy = 2,
> ...
> };
>
> extern const Control<int32_t> WhiteBalanceMode;
>
> }
This is actually less work than what I have proposed, and adding
defines to enumerations is easier, so we could go for this one first
probably.
Thanks
j
> -------------------------------
>
> > > With or without enums and but with an updated commit message,
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >
> > > > template = string.Template('''extern const Control<${type}> ${name};''')
> > > >
> > > > ctrls = []
> > > > @@ -66,11 +86,25 @@ def generate_h(controls):
> > > >
> > > > ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> > > >
> > > > + try:
> > > > + values = ctrl['values']
> > > > + except KeyError:
> > > > + values = ""
> > > > +
> > > > info = {
> > > > 'name': name,
> > > > 'type': ctrl['type'],
> > > > + 'values' : values,
> > > > }
> > > >
> > > > + for value in values:
> > > > + value_info = {
> > > > + 'name': list(value.keys())[0],
> > > > + 'value': value['value'],
> > > > + 'description': value['description']
> > > > + }
> > > > + ctrls.append(value_template.substitute(value_info))
> > > > +
> > > > ctrls.append(template.substitute(info))
> > > > id_value += 1
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191209/ad1333bb/attachment.sig>
More information about the libcamera-devel
mailing list