[02/10] libcamera: controls: Generate enum value-name maps
diff mbox series

Message ID 20240322131451.3092931-3-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise Agc into libipa
Related show

Commit Message

Dan Scally March 22, 2024, 1:14 p.m. UTC
Generate maps associating control enumerated control values with a
string representing that value to provide a central reference for
them across all pipeline handlers.

Signed-off-by: Daniel Scally <dan.scally@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(+)

Comments

Stefan Klug March 25, 2024, 4:01 p.m. UTC | #1
Hi Daniel,

thank you for the patch.

On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote:
> Generate maps associating control enumerated control values with a
> string representing that value to provide a central reference for
> them across all pipeline handlers.

I think
s/control enumerated control/enumerated control/ 

Otherwise, it looks good to me.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan

> 
> Signed-off-by: Daniel Scally <dan.scally@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(+)
> 
> 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} },''')
>  
>      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
> -- 
> 2.34.1
>
Jacopo Mondi March 25, 2024, 4:01 p.m. UTC | #2
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

>
> Signed-off-by: Daniel Scally <dan.scally@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@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} },''')
>
>      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
> --
> 2.34.1
>
Laurent Pinchart April 5, 2024, 9:39 p.m. UTC | #3
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@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@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
Paul Elder April 8, 2024, 7:05 a.m. UTC | #4
Hi Dan,

On Fri, Mar 22, 2024 at 01:14:43PM +0000, Daniel Scally wrote:
> Generate maps associating control enumerated control values with a
> string representing that value to provide a central reference for
> them across all pipeline handlers.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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(+)
> 
> 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} },''')
>  
>      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
> -- 
> 2.34.1
>

Patch
diff mbox series

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