[libcamera-devel,v3,06/14] libcamera: controls: Generate a vector of enumerated values
diff mbox series

Message ID 20201021143635.22846-7-jacopo@jmondi.org
State Superseded, archived
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 21, 2020, 2:36 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>
---
 src/libcamera/control_ids.cpp.in |  2 ++
 utils/gen-controls.py            | 10 ++++++++++
 2 files changed, 12 insertions(+)

Comments

Laurent Pinchart Oct. 22, 2020, 2:41 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Wed, Oct 21, 2020 at 04:36:27PM +0200, Jacopo Mondi wrote:
> 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>
> ---
>  src/libcamera/control_ids.cpp.in |  2 ++
>  utils/gen-controls.py            | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index 056645cfbdfb..ca0b5b22f899 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -6,8 +6,10 @@
>   *
>   * This file is auto-generated. Do not edit.
>   */
> +#include <vector>

This should go to control_ids.h.in as that's where you add
std::vector<>.

>  #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..23aace50f666 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -100,6 +100,8 @@ ${description}
>  def generate_h(controls):
>      enum_template_start = string.Template('''enum ${name}Enum {''')
>      enum_value_template = string.Template('''\t${name} = ${value},''')
> +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
> +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')

Shouldn't you declare the vector as extern const here, and define it in
control_ids.cpp ? Otherwise you'll duplicate the values in every
compilation unit that includes control_ids.h.

I would also make it an std::array, to avoid global objects that have
non-trivial constructors. But the ControlValue stored in the vector has
a non-trivial constructor :-/ Do you expect enumerated controls with
other types than Integer32 ? If not we could make the API more specific,
but maybe that's not a very good idea in case we need to extend it
later.

std::array may be best anyway, even with ControlValue, as it will be
simpler. Given that we know here how many enumerated values we generate,
setting the number of elements in the std::array template parameter
shouldn't be a problem. Then, you would need to modify the ControlInfo
constructor to take a Span<ControlValue> instead of a std::vector<>.

>      template = string.Template('''extern const Control<${type}> ${name};''')
>  
>      ctrls = []
> @@ -140,6 +142,14 @@ def generate_h(controls):
>                  target_ctrls.append(enum_value_template.substitute(value_info))
>              target_ctrls.append("};")
>  
> +            target_ctrls.append(enum_list_start.substitute(info))
> +            for entry in enum:
> +                value_info = {
> +                    'name': entry['name'],
> +                }
> +                target_ctrls.append(enum_list_values.substitute(value_info))
> +            target_ctrls.append("};")
> +

Not a candidate for this series, but given that we will soon depend on
jinja at build time (for IPA IPC and for tracing), I think we could
upgrade the control and property templates to jinja templates, it should
simplify this code.

>          target_ctrls.append(template.substitute(info))
>          id_value += 1
>
Jacopo Mondi Oct. 23, 2020, 11:11 a.m. UTC | #2 
Hi Laurent,

On Thu, Oct 22, 2020 at 05:41:13AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Oct 21, 2020 at 04:36:27PM +0200, Jacopo Mondi wrote:
> > 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>
> > ---
> >  src/libcamera/control_ids.cpp.in |  2 ++
> >  utils/gen-controls.py            | 10 ++++++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > index 056645cfbdfb..ca0b5b22f899 100644
> > --- a/src/libcamera/control_ids.cpp.in
> > +++ b/src/libcamera/control_ids.cpp.in
> > @@ -6,8 +6,10 @@
> >   *
> >   * This file is auto-generated. Do not edit.
> >   */
> > +#include <vector>
>
> This should go to control_ids.h.in as that's where you add
> std::vector<>.
>
> >  #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..23aace50f666 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -100,6 +100,8 @@ ${description}
> >  def generate_h(controls):
> >      enum_template_start = string.Template('''enum ${name}Enum {''')
> >      enum_value_template = string.Template('''\t${name} = ${value},''')
> > +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
> > +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
>
> Shouldn't you declare the vector as extern const here, and define it in
> control_ids.cpp ? Otherwise you'll duplicate the values in every
> compilation unit that includes control_ids.h.

Right right right!

I used a static const as I was wrongly generating the vector
definition in the .h file as extern const, as I got a rightfull linker
error for a redefinition.

What I do have now looks like (for AeMeteringControl)

control_ids.h:
enum AeMeteringModeEnum {
	MeteringCentreWeighted = 0,
	MeteringSpot = 1,
	MeteringMatrix = 2,
	MeteringCustom = 3,
};
extern const std::vector<ControlValue> AeMeteringModeValues;
extern const Control<int32_t> AeMeteringMode;

control_ids.cpp:
extern const std::vector<ControlValue> AeMeteringModeValues = {
	static_cast<int32_t>(MeteringCentreWeighted),
	static_cast<int32_t>(MeteringSpot),
	static_cast<int32_t>(MeteringMatrix),
	static_cast<int32_t>(MeteringCustom),
};
extern const Control<int32_t> AeMeteringMode(AE_METERING_MODE, "AeMeteringMode");

Which sounds about right to me.

>
> I would also make it an std::array, to avoid global objects that have
> non-trivial constructors. But the ControlValue stored in the vector has
> a non-trivial constructor :-/ Do you expect enumerated controls with
> other types than Integer32 ? If not we could make the API more specific,
> but maybe that's not a very good idea in case we need to extend it
> later.
>
> std::array may be best anyway, even with ControlValue, as it will be
> simpler. Given that we know here how many enumerated values we generate,
> setting the number of elements in the std::array template parameter
> shouldn't be a problem. Then, you would need to modify the ControlInfo
> constructor to take a Span<ControlValue> instead of a std::vector<>.
>

I struggled for the last hour to have a construct like this one work
using std::array<> and Span<>

Using std::array<> is fine, I can easily generate that.
Using Span<> to transport the array content in the ControlInfo is not
as passing a new Span<ControlValue> contructed from an array<ControlValue, N>
calls the ControlValue contructor

	ControlValue(const T &value)
		: type_(ControlTypeNone), numElements_(0)
	{
		set(details::control_type<std::remove_cv_t<T>>::value, true,
		    value.data(), value.size(), sizeof(typename T::value_type));
	}

and not the copy constructor. The construction than fails at
        details::control_type<std::remove_cv_t<T>>::value

As no specialization is provided for control_type<ControlValue>
(rightfully.

I got away with:
        ControlInfo(ControlValue *data, unsigned int size);
Which can be easily retrieved from std::array::data() and std::array::size()

In example (raspberrypi.h)
	{
          &controls::AeMeteringMode,
          ControlInfo(controls::AeMeteringModeValues.data(),
          controls::AeMeteringModeValues.size())
        }

Which is rather ugly but can be shortened with some macro magic

To sum up:
- array: yes
- Span<ControlValue>: no
- so using array I access the raw data and transport them to the
  ControlInfo (copying them in an class member vector at the moment,
  but it can be improved)

Sincerely, I don't see much gain in all this churn compared to using a
plain std::container but I might be missing something.


> >      template = string.Template('''extern const Control<${type}> ${name};''')
> >
> >      ctrls = []
> > @@ -140,6 +142,14 @@ def generate_h(controls):
> >                  target_ctrls.append(enum_value_template.substitute(value_info))
> >              target_ctrls.append("};")
> >
> > +            target_ctrls.append(enum_list_start.substitute(info))
> > +            for entry in enum:
> > +                value_info = {
> > +                    'name': entry['name'],
> > +                }
> > +                target_ctrls.append(enum_list_values.substitute(value_info))
> > +            target_ctrls.append("};")
> > +
>
> Not a candidate for this series, but given that we will soon depend on
> jinja at build time (for IPA IPC and for tracing), I think we could
> upgrade the control and property templates to jinja templates, it should
> simplify this code.
>
> >          target_ctrls.append(template.substitute(info))
> >          id_value += 1
> >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 23, 2020, 8:24 p.m. UTC | #3 
Hi Jacopo,

On Fri, Oct 23, 2020 at 01:11:42PM +0200, Jacopo Mondi wrote:
> On Thu, Oct 22, 2020 at 05:41:13AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 21, 2020 at 04:36:27PM +0200, Jacopo Mondi wrote:
> > > 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>
> > > ---
> > >  src/libcamera/control_ids.cpp.in |  2 ++
> > >  utils/gen-controls.py            | 10 ++++++++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > > index 056645cfbdfb..ca0b5b22f899 100644
> > > --- a/src/libcamera/control_ids.cpp.in
> > > +++ b/src/libcamera/control_ids.cpp.in
> > > @@ -6,8 +6,10 @@
> > >   *
> > >   * This file is auto-generated. Do not edit.
> > >   */
> > > +#include <vector>
> >
> > This should go to control_ids.h.in as that's where you add
> > std::vector<>.
> >
> > >  #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..23aace50f666 100755
> > > --- a/utils/gen-controls.py
> > > +++ b/utils/gen-controls.py
> > > @@ -100,6 +100,8 @@ ${description}
> > >  def generate_h(controls):
> > >      enum_template_start = string.Template('''enum ${name}Enum {''')
> > >      enum_value_template = string.Template('''\t${name} = ${value},''')
> > > +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
> > > +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> >
> > Shouldn't you declare the vector as extern const here, and define it in
> > control_ids.cpp ? Otherwise you'll duplicate the values in every
> > compilation unit that includes control_ids.h.
> 
> Right right right!
> 
> I used a static const as I was wrongly generating the vector
> definition in the .h file as extern const, as I got a rightfull linker
> error for a redefinition.
> 
> What I do have now looks like (for AeMeteringControl)
> 
> control_ids.h:
> enum AeMeteringModeEnum {
> 	MeteringCentreWeighted = 0,
> 	MeteringSpot = 1,
> 	MeteringMatrix = 2,
> 	MeteringCustom = 3,
> };
> extern const std::vector<ControlValue> AeMeteringModeValues;
> extern const Control<int32_t> AeMeteringMode;
> 
> control_ids.cpp:
> extern const std::vector<ControlValue> AeMeteringModeValues = {
> 	static_cast<int32_t>(MeteringCentreWeighted),
> 	static_cast<int32_t>(MeteringSpot),
> 	static_cast<int32_t>(MeteringMatrix),
> 	static_cast<int32_t>(MeteringCustom),
> };
> extern const Control<int32_t> AeMeteringMode(AE_METERING_MODE, "AeMeteringMode");
> 
> Which sounds about right to me.

Seems much more than about right to me :-)

> > I would also make it an std::array, to avoid global objects that have
> > non-trivial constructors. But the ControlValue stored in the vector has
> > a non-trivial constructor :-/ Do you expect enumerated controls with
> > other types than Integer32 ? If not we could make the API more specific,
> > but maybe that's not a very good idea in case we need to extend it
> > later.
> >
> > std::array may be best anyway, even with ControlValue, as it will be
> > simpler. Given that we know here how many enumerated values we generate,
> > setting the number of elements in the std::array template parameter
> > shouldn't be a problem. Then, you would need to modify the ControlInfo
> > constructor to take a Span<ControlValue> instead of a std::vector<>.
> 
> I struggled for the last hour to have a construct like this one work
> using std::array<> and Span<>
> 
> Using std::array<> is fine, I can easily generate that.
> Using Span<> to transport the array content in the ControlInfo is not
> as passing a new Span<ControlValue> contructed from an array<ControlValue, N>
> calls the ControlValue contructor
> 
> 	ControlValue(const T &value)
> 		: type_(ControlTypeNone), numElements_(0)
> 	{
> 		set(details::control_type<std::remove_cv_t<T>>::value, true,
> 		    value.data(), value.size(), sizeof(typename T::value_type));
> 	}
> 
> and not the copy constructor. The construction than fails at
>         details::control_type<std::remove_cv_t<T>>::value
>
> As no specialization is provided for control_type<ControlValue>
> (rightfully.

If someone wants to introduce this, I think I'll give up on computers
and go plant tomatoes in a warm country :-)

> I got away with:
>         ControlInfo(ControlValue *data, unsigned int size);
> Which can be easily retrieved from std::array::data() and std::array::size()
> 
> In example (raspberrypi.h)
> 	{
>           &controls::AeMeteringMode,
>           ControlInfo(controls::AeMeteringModeValues.data(),
>           controls::AeMeteringModeValues.size())
>         }
> 
> Which is rather ugly but can be shortened with some macro magic
> 
> To sum up:
> - array: yes
> - Span<ControlValue>: no
> - so using array I access the raw data and transport them to the
>   ControlInfo (copying them in an class member vector at the moment,
>   but it can be improved)
> 
> Sincerely, I don't see much gain in all this churn compared to using a
> plain std::container but I might be missing something.

If it's too much churn then it's probably not worth it indeed. In
general it's best to avoid non-trivial types for global variables as
much as possible, but not to the extent that would require us to rewrite
the C++ standard and implement our own compiler of course :-)

> > >      template = string.Template('''extern const Control<${type}> ${name};''')
> > >
> > >      ctrls = []
> > > @@ -140,6 +142,14 @@ def generate_h(controls):
> > >                  target_ctrls.append(enum_value_template.substitute(value_info))
> > >              target_ctrls.append("};")
> > >
> > > +            target_ctrls.append(enum_list_start.substitute(info))
> > > +            for entry in enum:
> > > +                value_info = {
> > > +                    'name': entry['name'],
> > > +                }
> > > +                target_ctrls.append(enum_list_values.substitute(value_info))
> > > +            target_ctrls.append("};")
> > > +
> >
> > Not a candidate for this series, but given that we will soon depend on
> > jinja at build time (for IPA IPC and for tracing), I think we could
> > upgrade the control and property templates to jinja templates, it should
> > simplify this code.
> >
> > >          target_ctrls.append(template.substitute(info))
> > >          id_value += 1
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
index 056645cfbdfb..ca0b5b22f899 100644
--- a/src/libcamera/control_ids.cpp.in
+++ b/src/libcamera/control_ids.cpp.in
@@ -6,8 +6,10 @@ 
  *
  * This file is auto-generated. Do not edit.
  */
+#include <vector>
 
 #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..23aace50f666 100755
--- a/utils/gen-controls.py
+++ b/utils/gen-controls.py
@@ -100,6 +100,8 @@  ${description}
 def generate_h(controls):
     enum_template_start = string.Template('''enum ${name}Enum {''')
     enum_value_template = string.Template('''\t${name} = ${value},''')
+    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
+    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
     template = string.Template('''extern const Control<${type}> ${name};''')
 
     ctrls = []
@@ -140,6 +142,14 @@  def generate_h(controls):
                 target_ctrls.append(enum_value_template.substitute(value_info))
             target_ctrls.append("};")
 
+            target_ctrls.append(enum_list_start.substitute(info))
+            for entry in enum:
+                value_info = {
+                    'name': entry['name'],
+                }
+                target_ctrls.append(enum_list_values.substitute(value_info))
+            target_ctrls.append("};")
+
         target_ctrls.append(template.substitute(info))
         id_value += 1