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

[Laurent Pinchart]

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;

}
-------------------------------

> > 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