[libcamera-devel,v4,05/12] libcamera: controls: Generate an array of valid values
diff mbox series

Message ID 20201023171116.24899-6-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 23, 2020, 5:11 p.m. UTC 
For each Control that support enumerated values generate a vector
of ControlValues which contains the full list of values.

At the expense of a slight increase in memory occupation this change
allows the construction of the ControlInfo associated with a Control
from the values list, defaulting the minimum and maximum values
reported by the ControlInfo.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/control_ids.h.in |  1 +
 src/libcamera/control_ids.cpp.in   |  1 +
 utils/gen-controls.py              | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+)

Comments

Laurent Pinchart Oct. 23, 2020, 10:05 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Fri, Oct 23, 2020 at 07:11:09PM +0200, Jacopo Mondi wrote:
> For each Control that support enumerated values generate a vector

s/support/supports/
s/a vector/an array/

> of ControlValues which contains the full list of values.
> 
> At the expense of a slight increase in memory occupation this change
> allows the construction of the ControlInfo associated with a Control
> from the values list, defaulting the minimum and maximum values
> reported by the ControlInfo.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If anyone is looking for a newbies task, converting the controls and
properties templates to jinja is a good candidate, I think it will
simplify the scripts and make them easier to maintain.

> ---
>  include/libcamera/control_ids.h.in |  1 +
>  src/libcamera/control_ids.cpp.in   |  1 +
>  utils/gen-controls.py              | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> index baadca83b103..7edeb6b65e32 100644
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -10,6 +10,7 @@
>  #ifndef __LIBCAMERA_CONTROL_IDS_H__
>  #define __LIBCAMERA_CONTROL_IDS_H__
>  
> +#include <array>
>  #include <stdint.h>
>  
>  #include <libcamera/controls.h>
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index 056645cfbdfb..5fb1c2c30558 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
>  
>  /**
>   * \file control_ids.h
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index bf681503f86a..a0bd43fabc31 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -33,6 +33,12 @@ ${description}''')
>  ${description}
>   */''')
>      def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
> +    enum_values_doc = string.Template('''/**
> + * \\var ${name}Values
> + * \\brief List of all $name supported values
> + */''')
> +    enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
> +    enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
>  
>      ctrls_doc = []
>      ctrls_def = []
> @@ -68,6 +74,7 @@ ${description}
>              enum_doc = []
>              enum_doc.append(enum_doc_start_template.substitute(info))
>  
> +            num_entries = 0;
>              for entry in enum:
>                  value_info = {
>                      'name' : name,
> @@ -75,11 +82,25 @@ ${description}
>                      'description': format_description(entry['description']),
>                  }
>                  enum_doc.append(enum_doc_value_template.substitute(value_info))
> +                num_entries += 1
>  
>              enum_doc = '\n *\n'.join(enum_doc)
>              enum_doc += '\n */'
>              target_doc.append(enum_doc)
>  
> +            values_info = {
> +                'name' : info['name'],
> +                'size' : num_entries,
> +            }
> +            target_doc.append(enum_values_doc.substitute(values_info))
> +            target_def.append(enum_values_start.substitute(values_info))
> +            for entry in enum:
> +                value_info = {
> +                    'name' : entry['name']
> +                }
> +                target_def.append(enum_values_values.substitute(value_info))
> +            target_def.append("};")
> +
>          target_doc.append(doc_template.substitute(info))
>          target_def.append(def_template.substitute(info))
>  
> @@ -100,6 +121,7 @@ ${description}
>  def generate_h(controls):
>      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;''')
>      template = string.Template('''extern const Control<${type}> ${name};''')
>  
>      ctrls = []
> @@ -132,14 +154,22 @@ def generate_h(controls):
>          if enum:
>              target_ctrls.append(enum_template_start.substitute(info))
>  
> +            num_entries = 0
>              for entry in enum:
>                  value_info = {
>                      'name': entry['name'],
>                      'value': entry['value'],
>                  }
>                  target_ctrls.append(enum_value_template.substitute(value_info))
> +                num_entries += 1
>              target_ctrls.append("};")
>  
> +            values_info = {
> +                'name' : info['name'],
> +                'size' : num_entries,
> +            }
> +            target_ctrls.append(enum_values_template.substitute(values_info))
> +
>          target_ctrls.append(template.substitute(info))
>          id_value += 1
>
Laurent Pinchart Oct. 24, 2020, 10:39 p.m. UTC | #2 
Hi Jacopo,

On Sat, Oct 24, 2020 at 01:05:49AM +0300, Laurent Pinchart wrote:
> On Fri, Oct 23, 2020 at 07:11:09PM +0200, Jacopo Mondi wrote:
> > For each Control that support enumerated values generate a vector
> 
> s/support/supports/
> s/a vector/an array/
> 
> > of ControlValues which contains the full list of values.
> > 
> > At the expense of a slight increase in memory occupation this change
> > allows the construction of the ControlInfo associated with a Control
> > from the values list, defaulting the minimum and maximum values
> > reported by the ControlInfo.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I've just noticed that checkstyle.py reports a few warnings. Could you
address them ? 

> If anyone is looking for a newbies task, converting the controls and
> properties templates to jinja is a good candidate, I think it will
> simplify the scripts and make them easier to maintain.
> 
> > ---
> >  include/libcamera/control_ids.h.in |  1 +
> >  src/libcamera/control_ids.cpp.in   |  1 +
> >  utils/gen-controls.py              | 30 ++++++++++++++++++++++++++++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > index baadca83b103..7edeb6b65e32 100644
> > --- a/include/libcamera/control_ids.h.in
> > +++ b/include/libcamera/control_ids.h.in
> > @@ -10,6 +10,7 @@
> >  #ifndef __LIBCAMERA_CONTROL_IDS_H__
> >  #define __LIBCAMERA_CONTROL_IDS_H__
> >  
> > +#include <array>
> >  #include <stdint.h>
> >  
> >  #include <libcamera/controls.h>
> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > index 056645cfbdfb..5fb1c2c30558 100644
> > --- a/src/libcamera/control_ids.cpp.in
> > +++ b/src/libcamera/control_ids.cpp.in
> > @@ -8,6 +8,7 @@
> >   */
> >  
> >  #include <libcamera/control_ids.h>
> > +#include <libcamera/controls.h>
> >  
> >  /**
> >   * \file control_ids.h
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index bf681503f86a..a0bd43fabc31 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -33,6 +33,12 @@ ${description}''')
> >  ${description}
> >   */''')
> >      def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
> > +    enum_values_doc = string.Template('''/**
> > + * \\var ${name}Values
> > + * \\brief List of all $name supported values
> > + */''')
> > +    enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
> > +    enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> >  
> >      ctrls_doc = []
> >      ctrls_def = []
> > @@ -68,6 +74,7 @@ ${description}
> >              enum_doc = []
> >              enum_doc.append(enum_doc_start_template.substitute(info))
> >  
> > +            num_entries = 0;
> >              for entry in enum:
> >                  value_info = {
> >                      'name' : name,
> > @@ -75,11 +82,25 @@ ${description}
> >                      'description': format_description(entry['description']),
> >                  }
> >                  enum_doc.append(enum_doc_value_template.substitute(value_info))
> > +                num_entries += 1
> >  
> >              enum_doc = '\n *\n'.join(enum_doc)
> >              enum_doc += '\n */'
> >              target_doc.append(enum_doc)
> >  
> > +            values_info = {
> > +                'name' : info['name'],
> > +                'size' : num_entries,
> > +            }
> > +            target_doc.append(enum_values_doc.substitute(values_info))
> > +            target_def.append(enum_values_start.substitute(values_info))
> > +            for entry in enum:
> > +                value_info = {
> > +                    'name' : entry['name']
> > +                }
> > +                target_def.append(enum_values_values.substitute(value_info))
> > +            target_def.append("};")
> > +
> >          target_doc.append(doc_template.substitute(info))
> >          target_def.append(def_template.substitute(info))
> >  
> > @@ -100,6 +121,7 @@ ${description}
> >  def generate_h(controls):
> >      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;''')
> >      template = string.Template('''extern const Control<${type}> ${name};''')
> >  
> >      ctrls = []
> > @@ -132,14 +154,22 @@ def generate_h(controls):
> >          if enum:
> >              target_ctrls.append(enum_template_start.substitute(info))
> >  
> > +            num_entries = 0
> >              for entry in enum:
> >                  value_info = {
> >                      'name': entry['name'],
> >                      'value': entry['value'],
> >                  }
> >                  target_ctrls.append(enum_value_template.substitute(value_info))
> > +                num_entries += 1
> >              target_ctrls.append("};")
> >  
> > +            values_info = {
> > +                'name' : info['name'],
> > +                'size' : num_entries,
> > +            }
> > +            target_ctrls.append(enum_values_template.substitute(values_info))
> > +
> >          target_ctrls.append(template.substitute(info))
> >          id_value += 1
> >
Jacopo Mondi Oct. 25, 2020, 2:16 p.m. UTC | #3 
Hi Laurent,

On Sun, Oct 25, 2020 at 01:39:14AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sat, Oct 24, 2020 at 01:05:49AM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 23, 2020 at 07:11:09PM +0200, Jacopo Mondi wrote:
> > > For each Control that support enumerated values generate a vector
> >
> > s/support/supports/
> > s/a vector/an array/
> >
> > > of ControlValues which contains the full list of values.
> > >
> > > At the expense of a slight increase in memory occupation this change
> > > allows the construction of the ControlInfo associated with a Control
> > > from the values list, defaulting the minimum and maximum values
> > > reported by the ControlInfo.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I've just noticed that checkstyle.py reports a few warnings. Could you
> address them ?
>

Oh, I thought they all were false positives, but the style used to
define dictionaries in the rest of the codebase actually has not ' '
before ':'.

I'll fix, thanks for reporting it

> > If anyone is looking for a newbies task, converting the controls and
> > properties templates to jinja is a good candidate, I think it will
> > simplify the scripts and make them easier to maintain.
> >
> > > ---
> > >  include/libcamera/control_ids.h.in |  1 +
> > >  src/libcamera/control_ids.cpp.in   |  1 +
> > >  utils/gen-controls.py              | 30 ++++++++++++++++++++++++++++++
> > >  3 files changed, 32 insertions(+)
> > >
> > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > > index baadca83b103..7edeb6b65e32 100644
> > > --- a/include/libcamera/control_ids.h.in
> > > +++ b/include/libcamera/control_ids.h.in
> > > @@ -10,6 +10,7 @@
> > >  #ifndef __LIBCAMERA_CONTROL_IDS_H__
> > >  #define __LIBCAMERA_CONTROL_IDS_H__
> > >
> > > +#include <array>
> > >  #include <stdint.h>
> > >
> > >  #include <libcamera/controls.h>
> > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > > index 056645cfbdfb..5fb1c2c30558 100644
> > > --- a/src/libcamera/control_ids.cpp.in
> > > +++ b/src/libcamera/control_ids.cpp.in
> > > @@ -8,6 +8,7 @@
> > >   */
> > >
> > >  #include <libcamera/control_ids.h>
> > > +#include <libcamera/controls.h>
> > >
> > >  /**
> > >   * \file control_ids.h
> > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > > index bf681503f86a..a0bd43fabc31 100755
> > > --- a/utils/gen-controls.py
> > > +++ b/utils/gen-controls.py
> > > @@ -33,6 +33,12 @@ ${description}''')
> > >  ${description}
> > >   */''')
> > >      def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
> > > +    enum_values_doc = string.Template('''/**
> > > + * \\var ${name}Values
> > > + * \\brief List of all $name supported values
> > > + */''')
> > > +    enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
> > > +    enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> > >
> > >      ctrls_doc = []
> > >      ctrls_def = []
> > > @@ -68,6 +74,7 @@ ${description}
> > >              enum_doc = []
> > >              enum_doc.append(enum_doc_start_template.substitute(info))
> > >
> > > +            num_entries = 0;
> > >              for entry in enum:
> > >                  value_info = {
> > >                      'name' : name,
> > > @@ -75,11 +82,25 @@ ${description}
> > >                      'description': format_description(entry['description']),
> > >                  }
> > >                  enum_doc.append(enum_doc_value_template.substitute(value_info))
> > > +                num_entries += 1
> > >
> > >              enum_doc = '\n *\n'.join(enum_doc)
> > >              enum_doc += '\n */'
> > >              target_doc.append(enum_doc)
> > >
> > > +            values_info = {
> > > +                'name' : info['name'],
> > > +                'size' : num_entries,
> > > +            }
> > > +            target_doc.append(enum_values_doc.substitute(values_info))
> > > +            target_def.append(enum_values_start.substitute(values_info))
> > > +            for entry in enum:
> > > +                value_info = {
> > > +                    'name' : entry['name']
> > > +                }
> > > +                target_def.append(enum_values_values.substitute(value_info))
> > > +            target_def.append("};")
> > > +
> > >          target_doc.append(doc_template.substitute(info))
> > >          target_def.append(def_template.substitute(info))
> > >
> > > @@ -100,6 +121,7 @@ ${description}
> > >  def generate_h(controls):
> > >      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;''')
> > >      template = string.Template('''extern const Control<${type}> ${name};''')
> > >
> > >      ctrls = []
> > > @@ -132,14 +154,22 @@ def generate_h(controls):
> > >          if enum:
> > >              target_ctrls.append(enum_template_start.substitute(info))
> > >
> > > +            num_entries = 0
> > >              for entry in enum:
> > >                  value_info = {
> > >                      'name': entry['name'],
> > >                      'value': entry['value'],
> > >                  }
> > >                  target_ctrls.append(enum_value_template.substitute(value_info))
> > > +                num_entries += 1
> > >              target_ctrls.append("};")
> > >
> > > +            values_info = {
> > > +                'name' : info['name'],
> > > +                'size' : num_entries,
> > > +            }
> > > +            target_ctrls.append(enum_values_template.substitute(values_info))
> > > +
> > >          target_ctrls.append(template.substitute(info))
> > >          id_value += 1
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
index baadca83b103..7edeb6b65e32 100644
--- a/include/libcamera/control_ids.h.in
+++ b/include/libcamera/control_ids.h.in
@@ -10,6 +10,7 @@ 
 #ifndef __LIBCAMERA_CONTROL_IDS_H__
 #define __LIBCAMERA_CONTROL_IDS_H__
 
+#include <array>
 #include <stdint.h>
 
 #include <libcamera/controls.h>
diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
index 056645cfbdfb..5fb1c2c30558 100644
--- a/src/libcamera/control_ids.cpp.in
+++ b/src/libcamera/control_ids.cpp.in
@@ -8,6 +8,7 @@ 
  */
 
 #include <libcamera/control_ids.h>
+#include <libcamera/controls.h>
 
 /**
  * \file control_ids.h
diff --git a/utils/gen-controls.py b/utils/gen-controls.py
index bf681503f86a..a0bd43fabc31 100755
--- a/utils/gen-controls.py
+++ b/utils/gen-controls.py
@@ -33,6 +33,12 @@  ${description}''')
 ${description}
  */''')
     def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
+    enum_values_doc = string.Template('''/**
+ * \\var ${name}Values
+ * \\brief List of all $name supported values
+ */''')
+    enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
+    enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
 
     ctrls_doc = []
     ctrls_def = []
@@ -68,6 +74,7 @@  ${description}
             enum_doc = []
             enum_doc.append(enum_doc_start_template.substitute(info))
 
+            num_entries = 0;
             for entry in enum:
                 value_info = {
                     'name' : name,
@@ -75,11 +82,25 @@  ${description}
                     'description': format_description(entry['description']),
                 }
                 enum_doc.append(enum_doc_value_template.substitute(value_info))
+                num_entries += 1
 
             enum_doc = '\n *\n'.join(enum_doc)
             enum_doc += '\n */'
             target_doc.append(enum_doc)
 
+            values_info = {
+                'name' : info['name'],
+                'size' : num_entries,
+            }
+            target_doc.append(enum_values_doc.substitute(values_info))
+            target_def.append(enum_values_start.substitute(values_info))
+            for entry in enum:
+                value_info = {
+                    'name' : entry['name']
+                }
+                target_def.append(enum_values_values.substitute(value_info))
+            target_def.append("};")
+
         target_doc.append(doc_template.substitute(info))
         target_def.append(def_template.substitute(info))
 
@@ -100,6 +121,7 @@  ${description}
 def generate_h(controls):
     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;''')
     template = string.Template('''extern const Control<${type}> ${name};''')
 
     ctrls = []
@@ -132,14 +154,22 @@  def generate_h(controls):
         if enum:
             target_ctrls.append(enum_template_start.substitute(info))
 
+            num_entries = 0
             for entry in enum:
                 value_info = {
                     'name': entry['name'],
                     'value': entry['value'],
                 }
                 target_ctrls.append(enum_value_template.substitute(value_info))
+                num_entries += 1
             target_ctrls.append("};")
 
+            values_info = {
+                'name' : info['name'],
+                'size' : num_entries,
+            }
+            target_ctrls.append(enum_values_template.substitute(values_info))
+
         target_ctrls.append(template.substitute(info))
         id_value += 1