[libcamera-devel,01/10] libcamera: Support draft controls and properties
diff mbox series

Message ID 20201009122101.73858-2-jacopo@jmondi.org
State Accepted
Headers show
Series
  • android: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 9, 2020, 12:20 p.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Extend the control and property framework to support exposing draft
controls and properties in a scoped namespace.

The controls/properties themselves will retain the same ordering in the
relevant enum/id maps - but the access to any draft control will require
explicitly referencing through its' draft:: namespace prefix.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
# Added missing hunk in control_ids.cpp.in and changed subject
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/control_ids.h.in  |  6 +++++
 include/libcamera/property_ids.h.in |  6 +++++
 src/libcamera/control_ids.cpp.in    | 13 +++++++++++
 src/libcamera/property_ids.cpp.in   | 13 +++++++++++
 utils/gen-controls.py               | 36 ++++++++++++++++++++++-------
 5 files changed, 66 insertions(+), 8 deletions(-)

--
2.28.0

Comments

Kieran Bingham Oct. 9, 2020, 12:20 p.m. UTC | #1 
Hi Jacopo,

On 09/10/2020 13:20, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Extend the control and property framework to support exposing draft
> controls and properties in a scoped namespace.
> 
> The controls/properties themselves will retain the same ordering in the
> relevant enum/id maps - but the access to any draft control will require
> explicitly referencing through its' draft:: namespace prefix.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> # Added missing hunk in control_ids.cpp.in and changed subject
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Additions are fine to me, some trivial white space - but that can be
changed if desired before pushing.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  include/libcamera/control_ids.h.in  |  6 +++++
>  include/libcamera/property_ids.h.in |  6 +++++
>  src/libcamera/control_ids.cpp.in    | 13 +++++++++++
>  src/libcamera/property_ids.cpp.in   | 13 +++++++++++
>  utils/gen-controls.py               | 36 ++++++++++++++++++++++-------
>  5 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> index 95a7a7f1e260..baadca83b103 100644
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -26,6 +26,12 @@ ${controls}
> 
>  extern const ControlIdMap controls;
> 
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
>  } /* namespace controls */
> 
>  } /* namespace libcamera */
> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> index e4dea335cd3b..52646c1f78ae 100644
> --- a/include/libcamera/property_ids.h.in
> +++ b/include/libcamera/property_ids.h.in
> @@ -24,6 +24,12 @@ ${ids}
> 
>  ${controls}
> 
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
>  extern const ControlIdMap properties;
> 
>  } /* namespace properties */
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index cba6258d68dd..d31c1f5011b9 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -23,14 +23,27 @@ namespace controls {
> 
>  ${controls_doc}
> 
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
>  #ifndef __DOXYGEN__
>  /*
>   * Keep the controls definitions hidden from doxygen as it incorrectly parses
>   * them as functions.
>   */
>  ${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */

property_ids.cpp.in has a newline here ... either add here, or remove
there - just to make them the same I think ;)

>  #endif
> 
> +

Spurious new line crept in? or intentional?


>  /**
>   * \brief List of all supported libcamera controls
>   *
> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> index bfdd823f63b0..275c1caff3ec 100644
> --- a/src/libcamera/property_ids.cpp.in
> +++ b/src/libcamera/property_ids.cpp.in
> @@ -23,12 +23,25 @@ namespace properties {
> 
>  ${controls_doc}
> 
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
>  #ifndef __DOXYGEN__
>  /*
>   * Keep the properties definitions hidden from doxygen as it incorrectly parses
>   * them as functions.
>   */
>  ${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */
> +
>  #endif
> 
>  /**
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index 87c3d52ada6d..f28db27ba19f 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -36,6 +36,8 @@ ${description}
> 
>      ctrls_doc = []
>      ctrls_def = []
> +    draft_ctrls_doc = []
> +    draft_ctrls_def = []
>      ctrls_map = []
> 
>      for ctrl in controls:
> @@ -55,6 +57,12 @@ ${description}
>              'id_name': id_name,
>          }
> 
> +        target_doc = ctrls_doc
> +        target_def = ctrls_def
> +        if ctrl.get('draft'):
> +            target_doc = draft_ctrls_doc
> +            target_def = draft_ctrls_def
> +
>          enum = ctrl.get('enum')
>          if enum:
>              enum_doc = []
> @@ -70,15 +78,21 @@ ${description}
> 
>              enum_doc = '\n *\n'.join(enum_doc)
>              enum_doc += '\n */'
> -            ctrls_doc.append(enum_doc)
> +            target_doc.append(enum_doc)
> +
> +        target_doc.append(doc_template.substitute(info))
> +        target_def.append(def_template.substitute(info))
> +
> +        if ctrl.get('draft'):
> +            name = 'draft::' + name
> 
> -        ctrls_doc.append(doc_template.substitute(info))
> -        ctrls_def.append(def_template.substitute(info))
>          ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
> 
>      return {
>          'controls_doc': '\n\n'.join(ctrls_doc),
>          'controls_def': '\n'.join(ctrls_def),
> +        'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
> +        'draft_controls_def': '\n\n'.join(draft_ctrls_def),
>          'controls_map': '\n'.join(ctrls_map),
>      }
> 
> @@ -89,6 +103,7 @@ def generate_h(controls):
>      template = string.Template('''extern const Control<${type}> ${name};''')
> 
>      ctrls = []
> +    draft_ctrls = []
>      ids = []
>      id_value = 1
> 
> @@ -109,22 +124,27 @@ def generate_h(controls):
>              'type': ctrl_type,
>          }
> 
> +        target_ctrls = ctrls
> +        if ctrl.get('draft'):
> +            target_ctrls = draft_ctrls
> +
>          enum = ctrl.get('enum')
>          if enum:
> -            ctrls.append(enum_template_start.substitute(info))
> +            target_ctrls.append(enum_template_start.substitute(info))
> 
>              for entry in enum:
>                  value_info = {
>                      'name': entry['name'],
>                      'value': entry['value'],
>                  }
> -                ctrls.append(enum_value_template.substitute(value_info))
> -            ctrls.append("};")
> +                target_ctrls.append(enum_value_template.substitute(value_info))
> +            target_ctrls.append("};")
> 
> -        ctrls.append(template.substitute(info))
> +        target_ctrls.append(template.substitute(info))
>          id_value += 1
> 
> -    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls)}
> +    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls),
> +            'draft_controls': '\n'.join(draft_ctrls)}
> 
> 
>  def fill_template(template, data):
> --
> 2.28.0
>
Jacopo Mondi Oct. 9, 2020, 12:40 p.m. UTC | #2 
Hi Kieran

On Fri, Oct 09, 2020 at 01:20:44PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 09/10/2020 13:20, Jacopo Mondi wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Extend the control and property framework to support exposing draft
> > controls and properties in a scoped namespace.
> >
> > The controls/properties themselves will retain the same ordering in the
> > relevant enum/id maps - but the access to any draft control will require
> > explicitly referencing through its' draft:: namespace prefix.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > # Added missing hunk in control_ids.cpp.in and changed subject
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Additions are fine to me, some trivial white space - but that can be
> changed if desired before pushing.

Ups, I thought I fixed them, as I did notice the same as well.
Sorry for being sloppy, I'll fix when applying.

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Thanks
  j

> > ---
> >  include/libcamera/control_ids.h.in  |  6 +++++
> >  include/libcamera/property_ids.h.in |  6 +++++
> >  src/libcamera/control_ids.cpp.in    | 13 +++++++++++
> >  src/libcamera/property_ids.cpp.in   | 13 +++++++++++
> >  utils/gen-controls.py               | 36 ++++++++++++++++++++++-------
> >  5 files changed, 66 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > index 95a7a7f1e260..baadca83b103 100644
> > --- a/include/libcamera/control_ids.h.in
> > +++ b/include/libcamera/control_ids.h.in
> > @@ -26,6 +26,12 @@ ${controls}
> >
> >  extern const ControlIdMap controls;
> >
> > +namespace draft {
> > +
> > +${draft_controls}
> > +
> > +} /* namespace draft */
> > +
> >  } /* namespace controls */
> >
> >  } /* namespace libcamera */
> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> > index e4dea335cd3b..52646c1f78ae 100644
> > --- a/include/libcamera/property_ids.h.in
> > +++ b/include/libcamera/property_ids.h.in
> > @@ -24,6 +24,12 @@ ${ids}
> >
> >  ${controls}
> >
> > +namespace draft {
> > +
> > +${draft_controls}
> > +
> > +} /* namespace draft */
> > +
> >  extern const ControlIdMap properties;
> >
> >  } /* namespace properties */
> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > index cba6258d68dd..d31c1f5011b9 100644
> > --- a/src/libcamera/control_ids.cpp.in
> > +++ b/src/libcamera/control_ids.cpp.in
> > @@ -23,14 +23,27 @@ namespace controls {
> >
> >  ${controls_doc}
> >
> > +namespace draft {
> > +
> > +${draft_controls_doc}
> > +
> > +} /* namespace draft */
> > +
> >  #ifndef __DOXYGEN__
> >  /*
> >   * Keep the controls definitions hidden from doxygen as it incorrectly parses
> >   * them as functions.
> >   */
> >  ${controls_def}
> > +
> > +namespace draft {
> > +
> > +${draft_controls_def}
> > +
> > +} /* namespace draft */
>
> property_ids.cpp.in has a newline here ... either add here, or remove
> there - just to make them the same I think ;)
>
> >  #endif
> >
> > +
>
> Spurious new line crept in? or intentional?
>
>
> >  /**
> >   * \brief List of all supported libcamera controls
> >   *
> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> > index bfdd823f63b0..275c1caff3ec 100644
> > --- a/src/libcamera/property_ids.cpp.in
> > +++ b/src/libcamera/property_ids.cpp.in
> > @@ -23,12 +23,25 @@ namespace properties {
> >
> >  ${controls_doc}
> >
> > +namespace draft {
> > +
> > +${draft_controls_doc}
> > +
> > +} /* namespace draft */
> > +
> >  #ifndef __DOXYGEN__
> >  /*
> >   * Keep the properties definitions hidden from doxygen as it incorrectly parses
> >   * them as functions.
> >   */
> >  ${controls_def}
> > +
> > +namespace draft {
> > +
> > +${draft_controls_def}
> > +
> > +} /* namespace draft */
> > +
> >  #endif
> >
> >  /**
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index 87c3d52ada6d..f28db27ba19f 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -36,6 +36,8 @@ ${description}
> >
> >      ctrls_doc = []
> >      ctrls_def = []
> > +    draft_ctrls_doc = []
> > +    draft_ctrls_def = []
> >      ctrls_map = []
> >
> >      for ctrl in controls:
> > @@ -55,6 +57,12 @@ ${description}
> >              'id_name': id_name,
> >          }
> >
> > +        target_doc = ctrls_doc
> > +        target_def = ctrls_def
> > +        if ctrl.get('draft'):
> > +            target_doc = draft_ctrls_doc
> > +            target_def = draft_ctrls_def
> > +
> >          enum = ctrl.get('enum')
> >          if enum:
> >              enum_doc = []
> > @@ -70,15 +78,21 @@ ${description}
> >
> >              enum_doc = '\n *\n'.join(enum_doc)
> >              enum_doc += '\n */'
> > -            ctrls_doc.append(enum_doc)
> > +            target_doc.append(enum_doc)
> > +
> > +        target_doc.append(doc_template.substitute(info))
> > +        target_def.append(def_template.substitute(info))
> > +
> > +        if ctrl.get('draft'):
> > +            name = 'draft::' + name
> >
> > -        ctrls_doc.append(doc_template.substitute(info))
> > -        ctrls_def.append(def_template.substitute(info))
> >          ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
> >
> >      return {
> >          'controls_doc': '\n\n'.join(ctrls_doc),
> >          'controls_def': '\n'.join(ctrls_def),
> > +        'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
> > +        'draft_controls_def': '\n\n'.join(draft_ctrls_def),
> >          'controls_map': '\n'.join(ctrls_map),
> >      }
> >
> > @@ -89,6 +103,7 @@ def generate_h(controls):
> >      template = string.Template('''extern const Control<${type}> ${name};''')
> >
> >      ctrls = []
> > +    draft_ctrls = []
> >      ids = []
> >      id_value = 1
> >
> > @@ -109,22 +124,27 @@ def generate_h(controls):
> >              'type': ctrl_type,
> >          }
> >
> > +        target_ctrls = ctrls
> > +        if ctrl.get('draft'):
> > +            target_ctrls = draft_ctrls
> > +
> >          enum = ctrl.get('enum')
> >          if enum:
> > -            ctrls.append(enum_template_start.substitute(info))
> > +            target_ctrls.append(enum_template_start.substitute(info))
> >
> >              for entry in enum:
> >                  value_info = {
> >                      'name': entry['name'],
> >                      'value': entry['value'],
> >                  }
> > -                ctrls.append(enum_value_template.substitute(value_info))
> > -            ctrls.append("};")
> > +                target_ctrls.append(enum_value_template.substitute(value_info))
> > +            target_ctrls.append("};")
> >
> > -        ctrls.append(template.substitute(info))
> > +        target_ctrls.append(template.substitute(info))
> >          id_value += 1
> >
> > -    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls)}
> > +    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls),
> > +            'draft_controls': '\n'.join(draft_ctrls)}
> >
> >
> >  def fill_template(template, data):
> > --
> > 2.28.0
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart Oct. 12, 2020, 12:01 a.m. UTC | #3 
Hi Jacopo and Kieran,

Thank you for the patch.

On Fri, Oct 09, 2020 at 02:20:52PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Extend the control and property framework to support exposing draft
> controls and properties in a scoped namespace.
> 
> The controls/properties themselves will retain the same ordering in the
> relevant enum/id maps - but the access to any draft control will require
> explicitly referencing through its' draft:: namespace prefix.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> # Added missing hunk in control_ids.cpp.in and changed subject

Isn't the usual syntax

[Added missing hunk in control_ids.cpp.in and changed subject]

? Or is that kernel-only ?

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/control_ids.h.in  |  6 +++++
>  include/libcamera/property_ids.h.in |  6 +++++
>  src/libcamera/control_ids.cpp.in    | 13 +++++++++++
>  src/libcamera/property_ids.cpp.in   | 13 +++++++++++
>  utils/gen-controls.py               | 36 ++++++++++++++++++++++-------
>  5 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> index 95a7a7f1e260..baadca83b103 100644
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -26,6 +26,12 @@ ${controls}
> 
>  extern const ControlIdMap controls;
> 
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
>  } /* namespace controls */
> 
>  } /* namespace libcamera */
> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> index e4dea335cd3b..52646c1f78ae 100644
> --- a/include/libcamera/property_ids.h.in
> +++ b/include/libcamera/property_ids.h.in
> @@ -24,6 +24,12 @@ ${ids}
> 
>  ${controls}
> 
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
>  extern const ControlIdMap properties;
> 
>  } /* namespace properties */
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index cba6258d68dd..d31c1f5011b9 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -23,14 +23,27 @@ namespace controls {
> 
>  ${controls_doc}
> 
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
>  #ifndef __DOXYGEN__
>  /*
>   * Keep the controls definitions hidden from doxygen as it incorrectly parses
>   * them as functions.
>   */
>  ${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */
>  #endif
> 
> +

Extra blank line.

>  /**
>   * \brief List of all supported libcamera controls
>   *
> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> index bfdd823f63b0..275c1caff3ec 100644
> --- a/src/libcamera/property_ids.cpp.in
> +++ b/src/libcamera/property_ids.cpp.in
> @@ -23,12 +23,25 @@ namespace properties {
> 
>  ${controls_doc}
> 
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
>  #ifndef __DOXYGEN__
>  /*
>   * Keep the properties definitions hidden from doxygen as it incorrectly parses
>   * them as functions.
>   */
>  ${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */
> +

This one isn't needed either.

>  #endif
> 
>  /**
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index 87c3d52ada6d..f28db27ba19f 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -36,6 +36,8 @@ ${description}
> 
>      ctrls_doc = []
>      ctrls_def = []
> +    draft_ctrls_doc = []
> +    draft_ctrls_def = []
>      ctrls_map = []
> 
>      for ctrl in controls:
> @@ -55,6 +57,12 @@ ${description}
>              'id_name': id_name,
>          }
> 
> +        target_doc = ctrls_doc
> +        target_def = ctrls_def
> +        if ctrl.get('draft'):
> +            target_doc = draft_ctrls_doc
> +            target_def = draft_ctrls_def
> +
>          enum = ctrl.get('enum')
>          if enum:
>              enum_doc = []
> @@ -70,15 +78,21 @@ ${description}
> 
>              enum_doc = '\n *\n'.join(enum_doc)
>              enum_doc += '\n */'
> -            ctrls_doc.append(enum_doc)
> +            target_doc.append(enum_doc)
> +
> +        target_doc.append(doc_template.substitute(info))
> +        target_def.append(def_template.substitute(info))
> +
> +        if ctrl.get('draft'):
> +            name = 'draft::' + name
> 
> -        ctrls_doc.append(doc_template.substitute(info))
> -        ctrls_def.append(def_template.substitute(info))
>          ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
> 
>      return {
>          'controls_doc': '\n\n'.join(ctrls_doc),
>          'controls_def': '\n'.join(ctrls_def),
> +        'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
> +        'draft_controls_def': '\n\n'.join(draft_ctrls_def),
>          'controls_map': '\n'.join(ctrls_map),
>      }
> 
> @@ -89,6 +103,7 @@ def generate_h(controls):
>      template = string.Template('''extern const Control<${type}> ${name};''')
> 
>      ctrls = []
> +    draft_ctrls = []
>      ids = []
>      id_value = 1
> 
> @@ -109,22 +124,27 @@ def generate_h(controls):
>              'type': ctrl_type,
>          }
> 
> +        target_ctrls = ctrls
> +        if ctrl.get('draft'):
> +            target_ctrls = draft_ctrls
> +
>          enum = ctrl.get('enum')
>          if enum:
> -            ctrls.append(enum_template_start.substitute(info))
> +            target_ctrls.append(enum_template_start.substitute(info))
> 
>              for entry in enum:
>                  value_info = {
>                      'name': entry['name'],
>                      'value': entry['value'],
>                  }
> -                ctrls.append(enum_value_template.substitute(value_info))
> -            ctrls.append("};")
> +                target_ctrls.append(enum_value_template.substitute(value_info))
> +            target_ctrls.append("};")
> 
> -        ctrls.append(template.substitute(info))
> +        target_ctrls.append(template.substitute(info))
>          id_value += 1
> 
> -    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls)}
> +    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls),
> +            'draft_controls': '\n'.join(draft_ctrls)}

You could format this as

    return {
    	'ids': '\n'.join(ids),
	'controls': '\n'.join(ctrls),
        'draft_controls': '\n'.join(draft_ctrls),
    }

to make it a bit more readable.

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

> 
> 
>  def fill_template(template, data):
Laurent Pinchart Oct. 12, 2020, 1:03 a.m. UTC | #4 
Hello,

One more thing.

On Mon, Oct 12, 2020 at 03:01:03AM +0300, Laurent Pinchart wrote:
> Hi Jacopo and Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Oct 09, 2020 at 02:20:52PM +0200, Jacopo Mondi wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Extend the control and property framework to support exposing draft
> > controls and properties in a scoped namespace.
> > 
> > The controls/properties themselves will retain the same ordering in the
> > relevant enum/id maps - but the access to any draft control will require
> > explicitly referencing through its' draft:: namespace prefix.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > # Added missing hunk in control_ids.cpp.in and changed subject
> 
> Isn't the usual syntax
> 
> [Added missing hunk in control_ids.cpp.in and changed subject]
> 
> ? Or is that kernel-only ?
> 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/control_ids.h.in  |  6 +++++
> >  include/libcamera/property_ids.h.in |  6 +++++
> >  src/libcamera/control_ids.cpp.in    | 13 +++++++++++
> >  src/libcamera/property_ids.cpp.in   | 13 +++++++++++
> >  utils/gen-controls.py               | 36 ++++++++++++++++++++++-------
> >  5 files changed, 66 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> > index 95a7a7f1e260..baadca83b103 100644
> > --- a/include/libcamera/control_ids.h.in
> > +++ b/include/libcamera/control_ids.h.in
> > @@ -26,6 +26,12 @@ ${controls}
> > 
> >  extern const ControlIdMap controls;
> > 
> > +namespace draft {
> > +
> > +${draft_controls}
> > +
> > +} /* namespace draft */
> > +
> >  } /* namespace controls */
> > 
> >  } /* namespace libcamera */
> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> > index e4dea335cd3b..52646c1f78ae 100644
> > --- a/include/libcamera/property_ids.h.in
> > +++ b/include/libcamera/property_ids.h.in
> > @@ -24,6 +24,12 @@ ${ids}
> > 
> >  ${controls}
> > 
> > +namespace draft {
> > +
> > +${draft_controls}
> > +
> > +} /* namespace draft */
> > +
> >  extern const ControlIdMap properties;
> > 
> >  } /* namespace properties */
> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> > index cba6258d68dd..d31c1f5011b9 100644
> > --- a/src/libcamera/control_ids.cpp.in
> > +++ b/src/libcamera/control_ids.cpp.in
> > @@ -23,14 +23,27 @@ namespace controls {
> > 
> >  ${controls_doc}
> > 

This requires

/**
 * \brief Namespace for libcamera draft controls
 */

> > +namespace draft {
> > +
> > +${draft_controls_doc}
> > +
> > +} /* namespace draft */
> > +
> >  #ifndef __DOXYGEN__
> >  /*
> >   * Keep the controls definitions hidden from doxygen as it incorrectly parses
> >   * them as functions.
> >   */
> >  ${controls_def}
> > +
> > +namespace draft {
> > +
> > +${draft_controls_def}
> > +
> > +} /* namespace draft */
> >  #endif
> > 
> > +
> 
> Extra blank line.
> 
> >  /**
> >   * \brief List of all supported libcamera controls
> >   *
> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> > index bfdd823f63b0..275c1caff3ec 100644
> > --- a/src/libcamera/property_ids.cpp.in
> > +++ b/src/libcamera/property_ids.cpp.in
> > @@ -23,12 +23,25 @@ namespace properties {
> > 
> >  ${controls_doc}
> > 

And here

/**
 * \brief Namespace for libcamera draft properties
 */

Otherwise documentation isn't properly generated. Could you double-check
the documentation output ?

> > +namespace draft {
> > +
> > +${draft_controls_doc}
> > +
> > +} /* namespace draft */
> > +
> >  #ifndef __DOXYGEN__
> >  /*
> >   * Keep the properties definitions hidden from doxygen as it incorrectly parses
> >   * them as functions.
> >   */
> >  ${controls_def}
> > +
> > +namespace draft {
> > +
> > +${draft_controls_def}
> > +
> > +} /* namespace draft */
> > +
> 
> This one isn't needed either.
> 
> >  #endif
> > 
> >  /**
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index 87c3d52ada6d..f28db27ba19f 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -36,6 +36,8 @@ ${description}
> > 
> >      ctrls_doc = []
> >      ctrls_def = []
> > +    draft_ctrls_doc = []
> > +    draft_ctrls_def = []
> >      ctrls_map = []
> > 
> >      for ctrl in controls:
> > @@ -55,6 +57,12 @@ ${description}
> >              'id_name': id_name,
> >          }
> > 
> > +        target_doc = ctrls_doc
> > +        target_def = ctrls_def
> > +        if ctrl.get('draft'):
> > +            target_doc = draft_ctrls_doc
> > +            target_def = draft_ctrls_def
> > +
> >          enum = ctrl.get('enum')
> >          if enum:
> >              enum_doc = []
> > @@ -70,15 +78,21 @@ ${description}
> > 
> >              enum_doc = '\n *\n'.join(enum_doc)
> >              enum_doc += '\n */'
> > -            ctrls_doc.append(enum_doc)
> > +            target_doc.append(enum_doc)
> > +
> > +        target_doc.append(doc_template.substitute(info))
> > +        target_def.append(def_template.substitute(info))
> > +
> > +        if ctrl.get('draft'):
> > +            name = 'draft::' + name
> > 
> > -        ctrls_doc.append(doc_template.substitute(info))
> > -        ctrls_def.append(def_template.substitute(info))
> >          ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
> > 
> >      return {
> >          'controls_doc': '\n\n'.join(ctrls_doc),
> >          'controls_def': '\n'.join(ctrls_def),
> > +        'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
> > +        'draft_controls_def': '\n\n'.join(draft_ctrls_def),
> >          'controls_map': '\n'.join(ctrls_map),
> >      }
> > 
> > @@ -89,6 +103,7 @@ def generate_h(controls):
> >      template = string.Template('''extern const Control<${type}> ${name};''')
> > 
> >      ctrls = []
> > +    draft_ctrls = []
> >      ids = []
> >      id_value = 1
> > 
> > @@ -109,22 +124,27 @@ def generate_h(controls):
> >              'type': ctrl_type,
> >          }
> > 
> > +        target_ctrls = ctrls
> > +        if ctrl.get('draft'):
> > +            target_ctrls = draft_ctrls
> > +
> >          enum = ctrl.get('enum')
> >          if enum:
> > -            ctrls.append(enum_template_start.substitute(info))
> > +            target_ctrls.append(enum_template_start.substitute(info))
> > 
> >              for entry in enum:
> >                  value_info = {
> >                      'name': entry['name'],
> >                      'value': entry['value'],
> >                  }
> > -                ctrls.append(enum_value_template.substitute(value_info))
> > -            ctrls.append("};")
> > +                target_ctrls.append(enum_value_template.substitute(value_info))
> > +            target_ctrls.append("};")
> > 
> > -        ctrls.append(template.substitute(info))
> > +        target_ctrls.append(template.substitute(info))
> >          id_value += 1
> > 
> > -    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls)}
> > +    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls),
> > +            'draft_controls': '\n'.join(draft_ctrls)}
> 
> You could format this as
> 
>     return {
>     	'ids': '\n'.join(ids),
> 	'controls': '\n'.join(ctrls),
>         'draft_controls': '\n'.join(draft_ctrls),
>     }
> 
> to make it a bit more readable.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > 
> > 
> >  def fill_template(template, data):

Patch
diff mbox series 

diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
index 95a7a7f1e260..baadca83b103 100644
--- a/include/libcamera/control_ids.h.in
+++ b/include/libcamera/control_ids.h.in
@@ -26,6 +26,12 @@  ${controls}

 extern const ControlIdMap controls;

+namespace draft {
+
+${draft_controls}
+
+} /* namespace draft */
+
 } /* namespace controls */

 } /* namespace libcamera */
diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
index e4dea335cd3b..52646c1f78ae 100644
--- a/include/libcamera/property_ids.h.in
+++ b/include/libcamera/property_ids.h.in
@@ -24,6 +24,12 @@  ${ids}

 ${controls}

+namespace draft {
+
+${draft_controls}
+
+} /* namespace draft */
+
 extern const ControlIdMap properties;

 } /* namespace properties */
diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
index cba6258d68dd..d31c1f5011b9 100644
--- a/src/libcamera/control_ids.cpp.in
+++ b/src/libcamera/control_ids.cpp.in
@@ -23,14 +23,27 @@  namespace controls {

 ${controls_doc}

+namespace draft {
+
+${draft_controls_doc}
+
+} /* namespace draft */
+
 #ifndef __DOXYGEN__
 /*
  * Keep the controls definitions hidden from doxygen as it incorrectly parses
  * them as functions.
  */
 ${controls_def}
+
+namespace draft {
+
+${draft_controls_def}
+
+} /* namespace draft */
 #endif

+
 /**
  * \brief List of all supported libcamera controls
  *
diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
index bfdd823f63b0..275c1caff3ec 100644
--- a/src/libcamera/property_ids.cpp.in
+++ b/src/libcamera/property_ids.cpp.in
@@ -23,12 +23,25 @@  namespace properties {

 ${controls_doc}

+namespace draft {
+
+${draft_controls_doc}
+
+} /* namespace draft */
+
 #ifndef __DOXYGEN__
 /*
  * Keep the properties definitions hidden from doxygen as it incorrectly parses
  * them as functions.
  */
 ${controls_def}
+
+namespace draft {
+
+${draft_controls_def}
+
+} /* namespace draft */
+
 #endif

 /**
diff --git a/utils/gen-controls.py b/utils/gen-controls.py
index 87c3d52ada6d..f28db27ba19f 100755
--- a/utils/gen-controls.py
+++ b/utils/gen-controls.py
@@ -36,6 +36,8 @@  ${description}

     ctrls_doc = []
     ctrls_def = []
+    draft_ctrls_doc = []
+    draft_ctrls_def = []
     ctrls_map = []

     for ctrl in controls:
@@ -55,6 +57,12 @@  ${description}
             'id_name': id_name,
         }

+        target_doc = ctrls_doc
+        target_def = ctrls_def
+        if ctrl.get('draft'):
+            target_doc = draft_ctrls_doc
+            target_def = draft_ctrls_def
+
         enum = ctrl.get('enum')
         if enum:
             enum_doc = []
@@ -70,15 +78,21 @@  ${description}

             enum_doc = '\n *\n'.join(enum_doc)
             enum_doc += '\n */'
-            ctrls_doc.append(enum_doc)
+            target_doc.append(enum_doc)
+
+        target_doc.append(doc_template.substitute(info))
+        target_def.append(def_template.substitute(info))
+
+        if ctrl.get('draft'):
+            name = 'draft::' + name

-        ctrls_doc.append(doc_template.substitute(info))
-        ctrls_def.append(def_template.substitute(info))
         ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')

     return {
         'controls_doc': '\n\n'.join(ctrls_doc),
         'controls_def': '\n'.join(ctrls_def),
+        'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
+        'draft_controls_def': '\n\n'.join(draft_ctrls_def),
         'controls_map': '\n'.join(ctrls_map),
     }

@@ -89,6 +103,7 @@  def generate_h(controls):
     template = string.Template('''extern const Control<${type}> ${name};''')

     ctrls = []
+    draft_ctrls = []
     ids = []
     id_value = 1

@@ -109,22 +124,27 @@  def generate_h(controls):
             'type': ctrl_type,
         }

+        target_ctrls = ctrls
+        if ctrl.get('draft'):
+            target_ctrls = draft_ctrls
+
         enum = ctrl.get('enum')
         if enum:
-            ctrls.append(enum_template_start.substitute(info))
+            target_ctrls.append(enum_template_start.substitute(info))

             for entry in enum:
                 value_info = {
                     'name': entry['name'],
                     'value': entry['value'],
                 }
-                ctrls.append(enum_value_template.substitute(value_info))
-            ctrls.append("};")
+                target_ctrls.append(enum_value_template.substitute(value_info))
+            target_ctrls.append("};")

-        ctrls.append(template.substitute(info))
+        target_ctrls.append(template.substitute(info))
         id_value += 1

-    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls)}
+    return {'ids': '\n'.join(ids), 'controls': '\n'.join(ctrls),
+            'draft_controls': '\n'.join(draft_ctrls)}


 def fill_template(template, data):