[libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse 'values' in gen-controls.py

[Jacopo Mondi]

Hi Niklas,

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 ?

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
> >
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20191208/27bc4ec5/attachment.sig>