[libcamera-devel] [PATCH 01/10] libcamera: Support draft controls and properties

[Laurent Pinchart]

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 at 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 at 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 at 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 at ideasonboard.com>

> 
> 
>  def fill_template(template, data):

-- 
Regards,

Laurent Pinchart