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

Message ID 20191205204350.28196-3-jacopo@jmondi.org
State Superseded
Headers show
Series
  • Introduce camera properties
Related show

Commit Message

Jacopo Mondi Dec. 5, 2019, 8:43 p.m. UTC
In preparation to add libcamera Camera 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@jmondi.org>
---
 src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Niklas Söderlund Dec. 6, 2019, 7:09 p.m. UTC | #1
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.

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

With or without enums and but with an updated commit message,

Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Dec. 8, 2019, 6:08 p.m. UTC | #2
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@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart Dec. 9, 2019, 12:54 p.m. UTC | #3
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@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@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
> > >
Kieran Bingham Dec. 9, 2019, 1:16 p.m. UTC | #4
Hi Jacopo, and all,

On 09/12/2019 12:54, 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@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

I suspect you mean value: 2 here, but I think that's just a typo in the
psuedo code.


>           description: -|
>              blah blah
> -------------------------------
> 
> should generate
> 
> -------- control_ids.h --------
> namespace controls {
> 
> enum WhiteBalanceModeValues {
> 	WhiteBalanceModeAuto = 0,
> 	WhiteBalanceModeSunny = 1,
> 	WhiteBalanceModeCloudy = 2,
> 	...
> };
> 
> extern const Control<int32_t> WhiteBalanceMode;
> 
> }
> -------------------------------


Would we support automatic values in enums?

I prefer using enums here over defines, as it scopes the values and will
give the compiler/debug-info more information as to their context too,
which means we will be able to perform more context validation (such as
ensuring all enum values are correctly identified within a switch)

Once defined, the numbers become part of the ABI, so they'd have to be
constant ... So we probably / possibly need to either document or define
some rule that any new additions are only appended, and not added in any
sort-order.

Otherwise, +1 on the YAML style Laurent has posted. It seems concise and
fills the requirements I can see.

--
Kieran




>>> With or without enums and but with an updated commit message,
>>>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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
>>>>
>
Jacopo Mondi Dec. 9, 2019, 1:17 p.m. UTC | #5
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@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@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
Laurent Pinchart Dec. 9, 2019, 1:20 p.m. UTC | #6
Hi Kieran,

On Mon, Dec 09, 2019 at 01:16:08PM +0000, Kieran Bingham wrote:
> On 09/12/2019 12:54, Laurent Pinchart wrote:
> > 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:
> >>> 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@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
> 
> I suspect you mean value: 2 here, but I think that's just a typo in the
> psuedo code.

Yes, it's a typo, sorry.

> >           description: -|
> >              blah blah
> > -------------------------------
> > 
> > should generate
> > 
> > -------- control_ids.h --------
> > namespace controls {
> > 
> > enum WhiteBalanceModeValues {
> > 	WhiteBalanceModeAuto = 0,
> > 	WhiteBalanceModeSunny = 1,
> > 	WhiteBalanceModeCloudy = 2,
> > 	...
> > };
> > 
> > extern const Control<int32_t> WhiteBalanceMode;
> > 
> > }
> > -------------------------------
> 
> 
> Would we support automatic values in enums?
> 
> I prefer using enums here over defines, as it scopes the values and will
> give the compiler/debug-info more information as to their context too,
> which means we will be able to perform more context validation (such as
> ensuring all enum values are correctly identified within a switch)
> 
> Once defined, the numbers become part of the ABI, so they'd have to be
> constant ... So we probably / possibly need to either document or define
> some rule that any new additions are only appended, and not added in any
> sort-order.

I've thought about that, but it can be a bit error-prone as inserting a
new value in the middle may not be caught during review. Furthermore, I
think having the explicit values in the documentation is useful for
binary IPAs that don't want to link to libcamera. For those reasons I
think there's more cons than pros for automatic values.

> Otherwise, +1 on the YAML style Laurent has posted. It seems concise and
> fills the requirements I can see.
> 
> >>> With or without enums and but with an updated commit message,
> >>>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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
> >>>>
Kieran Bingham Dec. 9, 2019, 1:25 p.m. UTC | #7
Hi Laurent,

On 09/12/2019 13:20, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Dec 09, 2019 at 01:16:08PM +0000, Kieran Bingham wrote:
>> On 09/12/2019 12:54, Laurent Pinchart wrote:
>>> 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:
>>>>> 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@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
>>
>> I suspect you mean value: 2 here, but I think that's just a typo in the
>> psuedo code.
> 
> Yes, it's a typo, sorry.
> 
>>>           description: -|
>>>              blah blah
>>> -------------------------------
>>>
>>> should generate
>>>
>>> -------- control_ids.h --------
>>> namespace controls {
>>>
>>> enum WhiteBalanceModeValues {
>>> 	WhiteBalanceModeAuto = 0,
>>> 	WhiteBalanceModeSunny = 1,
>>> 	WhiteBalanceModeCloudy = 2,
>>> 	...
>>> };
>>>
>>> extern const Control<int32_t> WhiteBalanceMode;
>>>
>>> }
>>> -------------------------------
>>
>>
>> Would we support automatic values in enums?
>>
>> I prefer using enums here over defines, as it scopes the values and will
>> give the compiler/debug-info more information as to their context too,
>> which means we will be able to perform more context validation (such as
>> ensuring all enum values are correctly identified within a switch)
>>
>> Once defined, the numbers become part of the ABI, so they'd have to be
>> constant ... So we probably / possibly need to either document or define
>> some rule that any new additions are only appended, and not added in any
>> sort-order.
> 
> I've thought about that, but it can be a bit error-prone as inserting a
> new value in the middle may not be caught during review. Furthermore, I
> think having the explicit values in the documentation is useful for
> binary IPAs that don't want to link to libcamera. For those reasons I
> think there's more cons than pros for automatic values.

Good, we're in agreement. That was my point! :-)

We need to ensure the numbers are consistent.

--
Kieran

> 
>> Otherwise, +1 on the YAML style Laurent has posted. It seems concise and
>> fills the requirements I can see.
>>
>>>>> With or without enums and but with an updated commit message,
>>>>>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@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
>>>>>>
>

Patch

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}''')
     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