[PATCH 02/10] libcamera: controls: Generate enum value-name maps

[Laurent Pinchart]

On Mon, Mar 25, 2024 at 05:01:55PM +0100, Jacopo Mondi wrote:
> Hi Dan
> 
> On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote:
> > Generate maps associating control enumerated control values with a
> s/control enumerated control values/enumerated control values/ ?
> 
> > string representing that value to provide a central reference for
> > them across all pipeline handlers.
> 
> As noticed by Kieran, it would be nice to expand a bit on what this
> would be used for

I was going to mention the same :-) A good commit message needs to
include *why* a change is needed and desirable. Describing what it does
is secondary to the reason.

> > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > ---
> >  include/libcamera/control_ids.h.in  |  2 ++
> >  include/libcamera/property_ids.h.in |  2 ++
> >  utils/gen-controls.py               | 19 +++++++++++++++++++
> >  3 files changed, 23 insertions(+)
> 
> I like this
> 
> The only alternative proposal would be to unify the ControlValue and
> the name string into a struct and enumerate them in a single array
> to make sure they stay in sync.
> 
> But since this is auto-generated and adjusting all users of the *Values
> array is probably a big job, I'm not sure it's even worth considering the
> alternative proposal.
> 
> What you have here is more than fine!
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > index d53b1cf7..58dd48e1 100644
> > --- a/include/libcamera/control_ids.h.in
> > +++ b/include/libcamera/control_ids.h.in
> > @@ -10,7 +10,9 @@
> >  #pragma once
> >
> >  #include <array>
> > +#include <map>
> >  #include <stdint.h>
> > +#include <string>
> >
> >  #include <libcamera/controls.h>
> >
> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> > index 43372c71..f51ba028 100644
> > --- a/include/libcamera/property_ids.h.in
> > +++ b/include/libcamera/property_ids.h.in
> > @@ -9,7 +9,9 @@
> >
> >  #pragma once
> >
> > +#include <map>
> >  #include <stdint.h>
> > +#include <string>
> >
> >  #include <libcamera/controls.h>
> >
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index 6cd5e362..4fe1e705 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -140,6 +140,12 @@ ${description}
> >   */''')
> >      enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
> >      enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> > +    name_value_map_doc = string.Template('''/**
> > + * \\var ${name}NameValueMap
> > + * \\brief Map of all $name supported value names (in std::string format) to value
> > + */''')
> > +    name_value_map_start = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap = {''')
> > +    name_value_values = string.Template('''\t{ "${name}", ${name} },''')

We should really switch to jinja templates, this is harder to read (no a
prerequisite for this series).

> >
> >      ctrls_doc = {}
> >      ctrls_def = {}
> > @@ -183,6 +189,7 @@ ${description}
> >
> >              values_info = {
> >                  'name': info['name'],
> > +                'type': ctrl.type,
> >                  'size': num_entries,
> >              }
> >              target_doc.append(enum_values_doc.substitute(values_info))
> > @@ -194,6 +201,15 @@ ${description}
> >                  target_def.append(enum_values_values.substitute(value_info))
> >              target_def.append("};")
> >
> > +            target_doc.append(name_value_map_doc.substitute(values_info))
> > +            target_def.append(name_value_map_start.substitute(values_info))
> > +            for enum in ctrl.enum_values:
> > +                value_info = {
> > +                    'name': enum.name
> > +                }
> > +                target_def.append(name_value_values.substitute(value_info))
> > +            target_def.append("};")
> > +
> >          target_doc.append(doc_template.substitute(info))
> >          target_def.append(def_template.substitute(info))
> >
> > @@ -231,6 +247,7 @@ def generate_h(controls, mode, ranges):
> >      enum_template_start = string.Template('''enum ${name}Enum {''')
> >      enum_value_template = string.Template('''\t${name} = ${value},''')
> >      enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')
> > +    name_value_map_template = string.Template('''extern const std::map<std::string, ${type}> ${name}NameValueMap;''')
> >      template = string.Template('''extern const Control<${type}> ${name};''')
> >
> >      ctrls = {}
> > @@ -273,9 +290,11 @@ def generate_h(controls, mode, ranges):
> >
> >              values_info = {
> >                  'name': info['name'],
> > +                'type': ctrl.type,
> >                  'size': num_entries,
> >              }
> >              target_ctrls.append(enum_values_template.substitute(values_info))
> > +            target_ctrls.append(name_value_map_template.substitute(values_info))
> >
> >          target_ctrls.append(template.substitute(info))
> >          id_value[vendor] += 1

-- 
Regards,

Laurent Pinchart