[{"id":28099,"web_url":"https://patchwork.libcamera.org/comment/28099/","msgid":"<20231120031740.GE524@pendragon.ideasonboard.com>","date":"2023-11-20T03:17:40","subject":"Re: [libcamera-devel] [PATCH v1 1/5] controls: Add vendor\n\tcontrol/property support to generation scripts","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Nov 10, 2023 at 10:59:58AM +0000, Naushir Patuck via libcamera-devel wrote:\n> Add support for vendor specific controls and properties to libcamera.\n> The controls/properties are defined by a \"vendor\" tag in the YAML\n> control description, for example:\n> \n>   - RpiVendorControlExample:\n\nIf this was included in documentation I would avoid starting with\n\"RpiVendor\" to avoid implying that vendor controls should be prefixed\nwith a vendor name, but for the commit message it's fine.\n\n>       type: string\n>       vendor: rpi\n>       description: |\n>         Test for libcamera vendor specific controls.\n\nAs indicated in a reply to the cover letter, I'm concerned that having\nto tag each control with the vendor, coupled with the ability to declare\nvendor controls in the main control_ids.yaml file, will make it a bit\nmessy and difficult to maintain. I would prefer enforcing separation of\nvendor controls in per-vendor files right away.  You can then add the\nvendor property at the top level in the YAML file, instead of specifying\nit per-control.\n\nI'm also thinking that we should probably move the control and property\nYAML files to a subdirectory, but that's something I can do on top.\n\n> This will now generate a control id in the libcamera::controls::vendor::rpi\n\nUnless I'm mistaken, that's libcamera::controls::rpi, there's no vendor\nnamespace.\n\n> namespace, ensuring no id conflict between different vendors, core or\n> draft libcamera controls. Similarly, a ControlIdMap control is generated\n> in the libcamera::controls::rpi namespace.\n> \n> A #define LIBCAMERA_RPI_VENDOR_CONTROLS is also generated to allow\n> applications to conditionally compile code if the specific vendor\n> controls are present.\n\nI wonder if this should go to a generated config.h at some point, and be\nnamed LIBCAMERA_HAS_*. If anyone has an opinion on the topic, let's\ndiscuss this already, and we can then update the implementation on top\nof this series.\n\n> For the python bindings, the control is available\n> with libcamera.controls.rpi.RpiVendorControlExample. The above controls\n> example applies similarly to properties.\n> \n> A new --mode flag is added to gen-controls.py to specify the mode of\n> operation, either 'controls' or 'properties' to allow the code generator\n> to correctly set the #define string.\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  include/libcamera/control_ids.h.in            |   2 +\n>  include/libcamera/meson.build                 |  13 +-\n>  include/libcamera/property_ids.h.in           |   2 +\n>  src/libcamera/control_ids.cpp.in              |   6 +\n>  src/libcamera/meson.build                     |   5 +-\n>  src/libcamera/property_ids.cpp.in             |   6 +\n>  src/py/libcamera/gen-py-controls.py           |  16 +-\n>  src/py/libcamera/py_controls_generated.cpp.in |   3 +\n>  .../libcamera/py_properties_generated.cpp.in  |   3 +\n>  utils/gen-controls.py                         | 144 ++++++++++++++----\n>  10 files changed, 162 insertions(+), 38 deletions(-)\n> \n> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> index 0718a8886f6c..c97b09a82450 100644\n> --- a/include/libcamera/control_ids.h.in\n> +++ b/include/libcamera/control_ids.h.in\n> @@ -32,6 +32,8 @@ ${draft_controls}\n>  \n>  } /* namespace draft */\n>  \n> +${vendor_controls}\n> +\n\nShould we split vendor controls to separate files ? It would make it\npossible for vendors to ship their control definitions separately from\nlibcamera, in case they have a closed-source IPA modules.\n\n>  } /* namespace controls */\n>  \n>  } /* namespace libcamera */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index a24c50d66a82..f736cca07228 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -33,19 +33,20 @@ install_headers(libcamera_public_headers,\n>  libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir\n>  \n>  # control_ids.h and property_ids.h\n> -control_source_files = [\n> -    'control_ids',\n> -    'property_ids',\n> -]\n> +control_source_files = {\n> +    'control_ids': 'controls',\n> +    'property_ids': 'properties',\n> +}\n>  \n>  control_headers = []\n>  \n> -foreach header : control_source_files\n> +foreach header, mode : control_source_files\n>      input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')\n>      control_headers += custom_target(header + '_h',\n>                                       input : input_files,\n>                                       output : header + '.h',\n> -                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> +                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',\n> +                                                '--mode', mode],\n>                                       install : true,\n>                                       install_dir : libcamera_headers_install_dir)\n>  endforeach\n> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> index ff0194083af0..47c5d6bf2e28 100644\n> --- a/include/libcamera/property_ids.h.in\n> +++ b/include/libcamera/property_ids.h.in\n> @@ -31,6 +31,8 @@ ${draft_controls}\n>  \n>  extern const ControlIdMap properties;\n>  \n> +${vendor_controls}\n> +\n>  } /* namespace properties */\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index 5fb1c2c30558..d26eb930b30c 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -33,6 +33,8 @@ ${draft_controls_doc}\n>  \n>  } /* namespace draft */\n>  \n> +${vendor_controls_doc}\n> +\n>  #ifndef __DOXYGEN__\n>  /*\n>   * Keep the controls definitions hidden from doxygen as it incorrectly parses\n> @@ -45,6 +47,8 @@ namespace draft {\n>  ${draft_controls_def}\n>  \n>  } /* namespace draft */\n> +\n> +${vendor_controls_def}\n>  #endif\n>  \n>  /**\n> @@ -57,6 +61,8 @@ extern const ControlIdMap controls {\n>  ${controls_map}\n>  };\n>  \n> +${vendor_controls_map}\n> +\n>  } /* namespace controls */\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index d0e26f6b4141..e49bf850b355 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -127,12 +127,13 @@ endif\n>  \n>  control_sources = []\n>  \n> -foreach source : control_source_files\n> +foreach source, mode : control_source_files\n>      input_files = files(source +'.yaml', source + '.cpp.in')\n>      control_sources += custom_target(source + '_cpp',\n>                                       input : input_files,\n>                                       output : source + '.cpp',\n> -                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> +                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',\n> +                                                '--mode', mode])\n>  endforeach\n>  \n>  libcamera_sources += control_sources\n> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> index f917e3349766..ddbe714c3f00 100644\n> --- a/src/libcamera/property_ids.cpp.in\n> +++ b/src/libcamera/property_ids.cpp.in\n> @@ -32,6 +32,8 @@ ${draft_controls_doc}\n>  \n>  } /* namespace draft */\n>  \n> +${vendor_controls_doc}\n> +\n>  #ifndef __DOXYGEN__\n>  /*\n>   * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> @@ -44,6 +46,8 @@ namespace draft {\n>  ${draft_controls_def}\n>  \n>  } /* namespace draft */\n> +\n> +${vendor_controls_def}\n>  #endif\n>  \n>  /**\n> @@ -53,6 +57,8 @@ extern const ControlIdMap properties {\n>  ${controls_map}\n>  };\n>  \n> +${vendor_controls_map}\n> +\n>  } /* namespace properties */\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> index 9948c41e42b1..e89de674966a 100755\n> --- a/src/py/libcamera/gen-py-controls.py\n> +++ b/src/py/libcamera/gen-py-controls.py\n> @@ -24,12 +24,24 @@ def find_common_prefix(strings):\n>  def generate_py(controls, mode):\n>      out = ''\n>  \n> +    vendors_class_def = []\n> +    vendor_defs = []\n> +    vendors = []\n>      for ctrl in controls:\n>          name, ctrl = ctrl.popitem()\n>  \n> +        vendor = ctrl.get('vendor')\n> +        if vendor and vendor not in vendors:\n> +            vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> +            vendor_defs.append('\\tauto {} = py::class_<Py{}Controls>(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n\nIt looks like it may be time to switch to jinja templates. Not a\nprerequisite for this series, although you may find it easier to rework\nthe generation script.\n\nI won't review the details of the scripts and templates below just yet,\nas they may be reworked based on the outcome of the discussions above.\n\n> +            vendors.append(vendor)\n> +\n>          if ctrl.get('draft'):\n>              ns = 'libcamera::{}::draft::'.format(mode)\n>              container = 'draft'\n> +        elif vendor:\n> +            ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> +            container = vendor\n>          else:\n>              ns = 'libcamera::{}::'.format(mode)\n>              container = 'controls'\n> @@ -62,7 +74,9 @@ def generate_py(controls, mode):\n>  \n>          out += '\\t;\\n\\n'\n>  \n> -    return {'controls': out}\n> +    return {'controls': out,\n> +            'vendors_class_def': '\\n'.join(vendors_class_def),\n> +            'vendors_defs': '\\n'.join(vendor_defs)}\n>  \n>  \n>  def fill_template(template, data):\n> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n> index 18fa57d948ea..ec4b55ef2011 100644\n> --- a/src/py/libcamera/py_controls_generated.cpp.in\n> +++ b/src/py/libcamera/py_controls_generated.cpp.in\n> @@ -21,10 +21,13 @@ class PyDraftControls\n>  {\n>  };\n>  \n> +${vendors_class_def}\n> +\n>  void init_py_controls_generated(py::module& m)\n>  {\n>  \tauto controls = py::class_<PyControls>(m, \"controls\");\n>  \tauto draft = py::class_<PyDraftControls>(controls, \"draft\");\n> +${vendors_defs}\n>  \n>  ${controls}\n>  }\n> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in\n> index e49b6e91bb83..87bc5e5d937e 100644\n> --- a/src/py/libcamera/py_properties_generated.cpp.in\n> +++ b/src/py/libcamera/py_properties_generated.cpp.in\n> @@ -21,10 +21,13 @@ class PyDraftProperties\n>  {\n>  };\n>  \n> +${vendors_class_def}\n> +\n>  void init_py_properties_generated(py::module& m)\n>  {\n>  \tauto controls = py::class_<PyProperties>(m, \"properties\");\n>  \tauto draft = py::class_<PyDraftProperties>(controls, \"draft\");\n> +${vendors_defs}\t\n>  \n>  ${controls}\n>  }\n> diff --git a/utils/gen-controls.py b/utils/gen-controls.py\n> index 1075ae302ce1..dd55753e792c 100755\n> --- a/utils/gen-controls.py\n> +++ b/utils/gen-controls.py\n> @@ -89,6 +89,11 @@ class Control(object):\n>          \"\"\"Is the control a draft control\"\"\"\n>          return self.__data.get('draft') is not None\n>  \n> +    @property\n> +    def vendor(self):\n> +        \"\"\"The vendor string, or None\"\"\"\n> +        return self.__data.get('vendor') if 'vendor' in self.__data else None\n> +\n>      @property\n>      def name(self):\n>          \"\"\"The control name (CamelCase)\"\"\"\n> @@ -145,15 +150,22 @@ ${description}\n>      enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')\n>      enum_values_values = string.Template('''\\tstatic_cast<int32_t>(${name}),''')\n>  \n> -    ctrls_doc = []\n> -    ctrls_def = []\n> -    draft_ctrls_doc = []\n> -    draft_ctrls_def = []\n> -    ctrls_map = []\n> +    ctrls_doc = {}\n> +    ctrls_def = {}\n> +    ctrls_map = {}\n>  \n>      for ctrl in controls:\n>          id_name = snake_case(ctrl.name).upper()\n>  \n> +        vendor = ctrl.vendor\n> +        if vendor is None:\n> +            vendor = 'draft' if ctrl.is_draft else 'libcamera'\n> +\n> +        if vendor not in ctrls_doc:\n> +            ctrls_doc[vendor] = []\n> +            ctrls_def[vendor] = []\n> +            ctrls_map[vendor] = []\n> +\n>          info = {\n>              'name': ctrl.name,\n>              'type': ctrl.type,\n> @@ -161,11 +173,9 @@ ${description}\n>              'id_name': id_name,\n>          }\n>  \n> -        target_doc = ctrls_doc\n> -        target_def = ctrls_def\n> -        if ctrl.is_draft:\n> -            target_doc = draft_ctrls_doc\n> -            target_def = draft_ctrls_def\n> +        target_doc = ctrls_doc[vendor]\n> +        target_def = ctrls_def[vendor]\n> +        target_ctrls_map = ctrls_map[vendor]\n>  \n>          if ctrl.is_enum:\n>              enum_doc = []\n> @@ -201,41 +211,90 @@ ${description}\n>          target_doc.append(doc_template.substitute(info))\n>          target_def.append(def_template.substitute(info))\n>  \n> -        ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> +        target_ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> +\n> +    vendor_ctrl_doc_sub = []\n> +    vendor_ctrl_template = string.Template('''\n> +namespace ${vendor} {\n> +\n> +${vendor_controls_str}\n> +\n> +} /* namespace ${vendor} */''')\n> +\n> +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> +        vendor_ctrl_doc_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\\n\\n'.join(ctrls_doc[vendor])}))\n> +\n> +    vendor_ctrl_def_sub = []\n> +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> +        vendor_ctrl_def_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\\n'.join(ctrls_def[vendor])}))\n> +\n> +    vendor_ctrl_map_sub = []\n> +    vendor_ctrl_template = string.Template('''namespace ${vendor} {\n> +/**\n> + * \\\\brief List of all supported ${vendor} vendor controls\n> + *\n> + * Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> + * set through Request::controls() and returned out through Request::metadata().\n> + */\n> +extern const ControlIdMap controls {\n> +${vendor_controls_map}\n> +};\n> +\n> +} /* namespace ${vendor} */\n> +''')\n> +\n> +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> +        vendor_ctrl_map_sub.append(vendor_ctrl_template.substitute({'vendor': vendor,\n> +                                                                    'vendor_controls_map': '\\n'.join(ctrls_map[vendor])}))\n>  \n>      return {\n> -        'controls_doc': '\\n\\n'.join(ctrls_doc),\n> -        'controls_def': '\\n'.join(ctrls_def),\n> -        'draft_controls_doc': '\\n\\n'.join(draft_ctrls_doc),\n> -        'draft_controls_def': '\\n\\n'.join(draft_ctrls_def),\n> -        'controls_map': '\\n'.join(ctrls_map),\n> +        'controls_doc': '\\n\\n'.join(ctrls_doc['libcamera']),\n> +        'controls_def': '\\n'.join(ctrls_def['libcamera']),\n> +        'draft_controls_doc': '\\n\\n'.join(ctrls_doc['draft']),\n> +        'draft_controls_def': '\\n\\n'.join(ctrls_def['draft']),\n> +        'controls_map': '\\n'.join(ctrls_map['libcamera'] + ctrls_map['draft']),\n> +        'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> +        'vendor_controls_def': '\\n'.join(vendor_ctrl_def_sub),\n> +        'vendor_controls_map': '\\n'.join(vendor_ctrl_map_sub),\n>      }\n>  \n>  \n> -def generate_h(controls):\n> +def generate_h(controls, mode):\n>      enum_template_start = string.Template('''enum ${name}Enum {''')\n>      enum_value_template = string.Template('''\\t${name} = ${value},''')\n>      enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')\n>      template = string.Template('''extern const Control<${type}> ${name};''')\n>  \n> -    ctrls = []\n> -    draft_ctrls = []\n> -    ids = []\n> -    id_value = 1\n> +    ctrls = {}\n> +    ids = {}\n> +    id_value = {}\n>  \n>      for ctrl in controls:\n>          id_name = snake_case(ctrl.name).upper()\n>  \n> -        ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> +        vendor = ctrl.vendor\n> +        if vendor is None:\n> +            vendor = 'draft' if ctrl.is_draft else 'libcamera'\n> +\n> +        if vendor not in ctrls:\n> +            ids[vendor] = []\n> +            id_value[vendor] = 1\n> +            ctrls[vendor] = []\n> +\n> +        # Core and draft controls use the same ID value\n> +        target_ids = ids['libcamera'] if vendor in ['libcamera', 'draft'] else ids[vendor]\n> +        target_ids.append('\\t' + id_name + ' = ' + str(id_value[vendor]) + ',')\n>  \n>          info = {\n>              'name': ctrl.name,\n>              'type': ctrl.type,\n>          }\n>  \n> -        target_ctrls = ctrls\n> +        target_ctrls = ctrls['libcamera']\n>          if ctrl.is_draft:\n> -            target_ctrls = draft_ctrls\n> +            target_ctrls = ctrls['draft']\n> +        elif vendor != 'libcamera':\n> +            target_ctrls = ctrls[vendor]\n>  \n>          if ctrl.is_enum:\n>              target_ctrls.append(enum_template_start.substitute(info))\n> @@ -257,12 +316,37 @@ def generate_h(controls):\n>              target_ctrls.append(enum_values_template.substitute(values_info))\n>  \n>          target_ctrls.append(template.substitute(info))\n> -        id_value += 1\n> +        id_value[vendor] += 1\n> +\n> +    vendor_template = string.Template('''\n> +namespace ${vendor} {\n> +\n> +#define LIBCAMERA_${vendor_def}_VENDOR_${mode}\n> +\n> +enum {\n> +${vendor_enums}\n> +};\n> +\n> +extern const ControlIdMap controls;\n> +\n> +${vendor_controls}\n> +\n> +} /* namespace ${vendor} */\n> +''')\n> +\n> +    vendor_sub = []\n> +    for vendor in [v for v in ctrls.keys() if v not in ['libcamera', 'draft']]:\n> +        vendor_sub.append(vendor_template.substitute({'mode': mode.upper(),\n> +                                                      'vendor': vendor,\n> +                                                      'vendor_def': vendor.upper(),\n> +                                                      'vendor_enums': '\\n'.join(ids[vendor]),\n> +                                                      'vendor_controls': '\\n'.join(ctrls[vendor])}))\n>  \n>      return {\n> -        'ids': '\\n'.join(ids),\n> -        'controls': '\\n'.join(ctrls),\n> -        'draft_controls': '\\n'.join(draft_ctrls)\n> +        'ids': '\\n'.join(ids['libcamera']),\n> +        'controls': '\\n'.join(ctrls['libcamera']),\n> +        'draft_controls': '\\n'.join(ctrls['draft']),\n> +        'vendor_controls': '\\n'.join(vendor_sub)\n>      }\n>  \n>  \n> @@ -284,6 +368,8 @@ def main(argv):\n>                          help='Input file name.')\n>      parser.add_argument('template', type=str,\n>                          help='Template file name.')\n> +    parser.add_argument('--mode', type=str, required=True, choices=['controls', 'properties'],\n> +                        help='Mode of operation')\n>      args = parser.parse_args(argv[1:])\n>  \n>      data = open(args.input, 'rb').read()\n> @@ -293,7 +379,7 @@ def main(argv):\n>      if args.template.endswith('.cpp.in'):\n>          data = generate_cpp(controls)\n>      elif args.template.endswith('.h.in'):\n> -        data = generate_h(controls)\n> +        data = generate_h(controls, args.mode)\n>      else:\n>          raise RuntimeError('Unknown template type')\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7281AC3200\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Nov 2023 03:17:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0018629BC;\n\tMon, 20 Nov 2023 04:17:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A381461DA9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Nov 2023 04:17:34 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB642C80;\n\tMon, 20 Nov 2023 04:17:04 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700450256;\n\tbh=9SxU1DJFcJnNKWz/L4crvWdIWJIHUBQqrqh/xfuTDGY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3NdTfnCqhOh8OYw/250coktDIfbfhJa1cc9WNrFubDGFG5Bb411R1+x6DFKwn+5Zd\n\tBirQ8H4MlPcLbBn0bZwLfR21Ji11a/5ewXlfT9FMVYL0zXzuSvGQkvEGDiJ+P6kirf\n\tsDxC3EQ5QXOia9YiYiLk6p7CLrv4TXPbSC5gDZwOPKGERbTye706iXm08S8lcD48Z2\n\tbfsErk2/T6MRik2QOTb7h6ZU9zgFPDRhyhf0jbYGS9MXA61g59THwDvnCIXtpkG+dg\n\ty4c8sVNAmpbO70q77+hHnsJKC+3dBllDoWAEd11DR42ju3MxGL8idDL7h9OKytGdiF\n\tfM5gvMYjJ9mjw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1700450225;\n\tbh=9SxU1DJFcJnNKWz/L4crvWdIWJIHUBQqrqh/xfuTDGY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pmFMM4aDmC00CsPQB/EUOg+vehLbtrZNfCq0uCssPBOeQqhQI5Gttn84xz8ftJl6R\n\t9P30m3StdMiBYBtTuZNQV8YCODAsIZ/BEZ9rjn0srJxhs9xFarMik1DprmqaN0cnWr\n\tzWdVUkEKtV2wbw/MnI8ZBnUfYSXdcHecm3ub8lds="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pmFMM4aD\"; dkim-atps=neutral","Date":"Mon, 20 Nov 2023 05:17:40 +0200","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20231120031740.GE524@pendragon.ideasonboard.com>","References":"<20231110110002.21381-1-naush@raspberrypi.com>\n\t<20231110110002.21381-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231110110002.21381-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/5] controls: Add vendor\n\tcontrol/property support to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28107,"web_url":"https://patchwork.libcamera.org/comment/28107/","msgid":"<CAEmqJPrx0+TreQALXZa2ybQGe3FJRHZTjXzMAX=49PGf04mFQw@mail.gmail.com>","date":"2023-11-20T10:13:28","subject":"Re: [libcamera-devel] [PATCH v1 1/5] controls: Add vendor\n\tcontrol/property support to generation scripts","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the feedback.\n\nOn Mon, 20 Nov 2023 at 03:17, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Nov 10, 2023 at 10:59:58AM +0000, Naushir Patuck via libcamera-devel wrote:\n> > Add support for vendor specific controls and properties to libcamera.\n> > The controls/properties are defined by a \"vendor\" tag in the YAML\n> > control description, for example:\n> >\n> >   - RpiVendorControlExample:\n>\n> If this was included in documentation I would avoid starting with\n> \"RpiVendor\" to avoid implying that vendor controls should be prefixed\n> with a vendor name, but for the commit message it's fine.\n\nI can remove this from the commit message.\n\n>\n> >       type: string\n> >       vendor: rpi\n> >       description: |\n> >         Test for libcamera vendor specific controls.\n>\n> As indicated in a reply to the cover letter, I'm concerned that having\n> to tag each control with the vendor, coupled with the ability to declare\n> vendor controls in the main control_ids.yaml file, will make it a bit\n> messy and difficult to maintain. I would prefer enforcing separation of\n> vendor controls in per-vendor files right away.  You can then add the\n> vendor property at the top level in the YAML file, instead of specifying\n> it per-control.\n\nAck, the next version will do this.\n\n>\n> I'm also thinking that we should probably move the control and property\n> YAML files to a subdirectory, but that's something I can do on top.\n>\n> > This will now generate a control id in the libcamera::controls::vendor::rpi\n>\n> Unless I'm mistaken, that's libcamera::controls::rpi, there's no vendor\n> namespace.\n\nTrue, this is a typo in the commit message, i'll fix it.\n\n>\n> > namespace, ensuring no id conflict between different vendors, core or\n> > draft libcamera controls. Similarly, a ControlIdMap control is generated\n> > in the libcamera::controls::rpi namespace.\n> >\n> > A #define LIBCAMERA_RPI_VENDOR_CONTROLS is also generated to allow\n> > applications to conditionally compile code if the specific vendor\n> > controls are present.\n>\n> I wonder if this should go to a generated config.h at some point, and be\n> named LIBCAMERA_HAS_*. If anyone has an opinion on the topic, let's\n> discuss this already, and we can then update the implementation on top\n> of this series.\n\nAgree, I prefer LIBCAMERA_HAS_* as well.\n\nRegarding moving this to a new config.h generated file, I have a\nslight preference to keep it in the existing control_ids.h file only\nbecause it will always be #included in the application if they use\ncontrols.  But we can change if folks a separate file may be better.\n\nRegards,\nNaush\n\n>\n> > For the python bindings, the control is available\n> > with libcamera.controls.rpi.RpiVendorControlExample. The above controls\n> > example applies similarly to properties.\n> >\n> > A new --mode flag is added to gen-controls.py to specify the mode of\n> > operation, either 'controls' or 'properties' to allow the code generator\n> > to correctly set the #define string.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  include/libcamera/control_ids.h.in            |   2 +\n> >  include/libcamera/meson.build                 |  13 +-\n> >  include/libcamera/property_ids.h.in           |   2 +\n> >  src/libcamera/control_ids.cpp.in              |   6 +\n> >  src/libcamera/meson.build                     |   5 +-\n> >  src/libcamera/property_ids.cpp.in             |   6 +\n> >  src/py/libcamera/gen-py-controls.py           |  16 +-\n> >  src/py/libcamera/py_controls_generated.cpp.in |   3 +\n> >  .../libcamera/py_properties_generated.cpp.in  |   3 +\n> >  utils/gen-controls.py                         | 144 ++++++++++++++----\n> >  10 files changed, 162 insertions(+), 38 deletions(-)\n> >\n> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> > index 0718a8886f6c..c97b09a82450 100644\n> > --- a/include/libcamera/control_ids.h.in\n> > +++ b/include/libcamera/control_ids.h.in\n> > @@ -32,6 +32,8 @@ ${draft_controls}\n> >\n> >  } /* namespace draft */\n> >\n> > +${vendor_controls}\n> > +\n>\n> Should we split vendor controls to separate files ? It would make it\n> possible for vendors to ship their control definitions separately from\n> libcamera, in case they have a closed-source IPA modules.\n>\n> >  } /* namespace controls */\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index a24c50d66a82..f736cca07228 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -33,19 +33,20 @@ install_headers(libcamera_public_headers,\n> >  libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir\n> >\n> >  # control_ids.h and property_ids.h\n> > -control_source_files = [\n> > -    'control_ids',\n> > -    'property_ids',\n> > -]\n> > +control_source_files = {\n> > +    'control_ids': 'controls',\n> > +    'property_ids': 'properties',\n> > +}\n> >\n> >  control_headers = []\n> >\n> > -foreach header : control_source_files\n> > +foreach header, mode : control_source_files\n> >      input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')\n> >      control_headers += custom_target(header + '_h',\n> >                                       input : input_files,\n> >                                       output : header + '.h',\n> > -                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > +                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',\n> > +                                                '--mode', mode],\n> >                                       install : true,\n> >                                       install_dir : libcamera_headers_install_dir)\n> >  endforeach\n> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > index ff0194083af0..47c5d6bf2e28 100644\n> > --- a/include/libcamera/property_ids.h.in\n> > +++ b/include/libcamera/property_ids.h.in\n> > @@ -31,6 +31,8 @@ ${draft_controls}\n> >\n> >  extern const ControlIdMap properties;\n> >\n> > +${vendor_controls}\n> > +\n> >  } /* namespace properties */\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index 5fb1c2c30558..d26eb930b30c 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -33,6 +33,8 @@ ${draft_controls_doc}\n> >\n> >  } /* namespace draft */\n> >\n> > +${vendor_controls_doc}\n> > +\n> >  #ifndef __DOXYGEN__\n> >  /*\n> >   * Keep the controls definitions hidden from doxygen as it incorrectly parses\n> > @@ -45,6 +47,8 @@ namespace draft {\n> >  ${draft_controls_def}\n> >\n> >  } /* namespace draft */\n> > +\n> > +${vendor_controls_def}\n> >  #endif\n> >\n> >  /**\n> > @@ -57,6 +61,8 @@ extern const ControlIdMap controls {\n> >  ${controls_map}\n> >  };\n> >\n> > +${vendor_controls_map}\n> > +\n> >  } /* namespace controls */\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index d0e26f6b4141..e49bf850b355 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -127,12 +127,13 @@ endif\n> >\n> >  control_sources = []\n> >\n> > -foreach source : control_source_files\n> > +foreach source, mode : control_source_files\n> >      input_files = files(source +'.yaml', source + '.cpp.in')\n> >      control_sources += custom_target(source + '_cpp',\n> >                                       input : input_files,\n> >                                       output : source + '.cpp',\n> > -                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > +                                     command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',\n> > +                                                '--mode', mode])\n> >  endforeach\n> >\n> >  libcamera_sources += control_sources\n> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > index f917e3349766..ddbe714c3f00 100644\n> > --- a/src/libcamera/property_ids.cpp.in\n> > +++ b/src/libcamera/property_ids.cpp.in\n> > @@ -32,6 +32,8 @@ ${draft_controls_doc}\n> >\n> >  } /* namespace draft */\n> >\n> > +${vendor_controls_doc}\n> > +\n> >  #ifndef __DOXYGEN__\n> >  /*\n> >   * Keep the properties definitions hidden from doxygen as it incorrectly parses\n> > @@ -44,6 +46,8 @@ namespace draft {\n> >  ${draft_controls_def}\n> >\n> >  } /* namespace draft */\n> > +\n> > +${vendor_controls_def}\n> >  #endif\n> >\n> >  /**\n> > @@ -53,6 +57,8 @@ extern const ControlIdMap properties {\n> >  ${controls_map}\n> >  };\n> >\n> > +${vendor_controls_map}\n> > +\n> >  } /* namespace properties */\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > index 9948c41e42b1..e89de674966a 100755\n> > --- a/src/py/libcamera/gen-py-controls.py\n> > +++ b/src/py/libcamera/gen-py-controls.py\n> > @@ -24,12 +24,24 @@ def find_common_prefix(strings):\n> >  def generate_py(controls, mode):\n> >      out = ''\n> >\n> > +    vendors_class_def = []\n> > +    vendor_defs = []\n> > +    vendors = []\n> >      for ctrl in controls:\n> >          name, ctrl = ctrl.popitem()\n> >\n> > +        vendor = ctrl.get('vendor')\n> > +        if vendor and vendor not in vendors:\n> > +            vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > +            vendor_defs.append('\\tauto {} = py::class_<Py{}Controls>(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n>\n> It looks like it may be time to switch to jinja templates. Not a\n> prerequisite for this series, although you may find it easier to rework\n> the generation script.\n>\n> I won't review the details of the scripts and templates below just yet,\n> as they may be reworked based on the outcome of the discussions above.\n>\n> > +            vendors.append(vendor)\n> > +\n> >          if ctrl.get('draft'):\n> >              ns = 'libcamera::{}::draft::'.format(mode)\n> >              container = 'draft'\n> > +        elif vendor:\n> > +            ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > +            container = vendor\n> >          else:\n> >              ns = 'libcamera::{}::'.format(mode)\n> >              container = 'controls'\n> > @@ -62,7 +74,9 @@ def generate_py(controls, mode):\n> >\n> >          out += '\\t;\\n\\n'\n> >\n> > -    return {'controls': out}\n> > +    return {'controls': out,\n> > +            'vendors_class_def': '\\n'.join(vendors_class_def),\n> > +            'vendors_defs': '\\n'.join(vendor_defs)}\n> >\n> >\n> >  def fill_template(template, data):\n> > diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in\n> > index 18fa57d948ea..ec4b55ef2011 100644\n> > --- a/src/py/libcamera/py_controls_generated.cpp.in\n> > +++ b/src/py/libcamera/py_controls_generated.cpp.in\n> > @@ -21,10 +21,13 @@ class PyDraftControls\n> >  {\n> >  };\n> >\n> > +${vendors_class_def}\n> > +\n> >  void init_py_controls_generated(py::module& m)\n> >  {\n> >       auto controls = py::class_<PyControls>(m, \"controls\");\n> >       auto draft = py::class_<PyDraftControls>(controls, \"draft\");\n> > +${vendors_defs}\n> >\n> >  ${controls}\n> >  }\n> > diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in\n> > index e49b6e91bb83..87bc5e5d937e 100644\n> > --- a/src/py/libcamera/py_properties_generated.cpp.in\n> > +++ b/src/py/libcamera/py_properties_generated.cpp.in\n> > @@ -21,10 +21,13 @@ class PyDraftProperties\n> >  {\n> >  };\n> >\n> > +${vendors_class_def}\n> > +\n> >  void init_py_properties_generated(py::module& m)\n> >  {\n> >       auto controls = py::class_<PyProperties>(m, \"properties\");\n> >       auto draft = py::class_<PyDraftProperties>(controls, \"draft\");\n> > +${vendors_defs}\n> >\n> >  ${controls}\n> >  }\n> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py\n> > index 1075ae302ce1..dd55753e792c 100755\n> > --- a/utils/gen-controls.py\n> > +++ b/utils/gen-controls.py\n> > @@ -89,6 +89,11 @@ class Control(object):\n> >          \"\"\"Is the control a draft control\"\"\"\n> >          return self.__data.get('draft') is not None\n> >\n> > +    @property\n> > +    def vendor(self):\n> > +        \"\"\"The vendor string, or None\"\"\"\n> > +        return self.__data.get('vendor') if 'vendor' in self.__data else None\n> > +\n> >      @property\n> >      def name(self):\n> >          \"\"\"The control name (CamelCase)\"\"\"\n> > @@ -145,15 +150,22 @@ ${description}\n> >      enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')\n> >      enum_values_values = string.Template('''\\tstatic_cast<int32_t>(${name}),''')\n> >\n> > -    ctrls_doc = []\n> > -    ctrls_def = []\n> > -    draft_ctrls_doc = []\n> > -    draft_ctrls_def = []\n> > -    ctrls_map = []\n> > +    ctrls_doc = {}\n> > +    ctrls_def = {}\n> > +    ctrls_map = {}\n> >\n> >      for ctrl in controls:\n> >          id_name = snake_case(ctrl.name).upper()\n> >\n> > +        vendor = ctrl.vendor\n> > +        if vendor is None:\n> > +            vendor = 'draft' if ctrl.is_draft else 'libcamera'\n> > +\n> > +        if vendor not in ctrls_doc:\n> > +            ctrls_doc[vendor] = []\n> > +            ctrls_def[vendor] = []\n> > +            ctrls_map[vendor] = []\n> > +\n> >          info = {\n> >              'name': ctrl.name,\n> >              'type': ctrl.type,\n> > @@ -161,11 +173,9 @@ ${description}\n> >              'id_name': id_name,\n> >          }\n> >\n> > -        target_doc = ctrls_doc\n> > -        target_def = ctrls_def\n> > -        if ctrl.is_draft:\n> > -            target_doc = draft_ctrls_doc\n> > -            target_def = draft_ctrls_def\n> > +        target_doc = ctrls_doc[vendor]\n> > +        target_def = ctrls_def[vendor]\n> > +        target_ctrls_map = ctrls_map[vendor]\n> >\n> >          if ctrl.is_enum:\n> >              enum_doc = []\n> > @@ -201,41 +211,90 @@ ${description}\n> >          target_doc.append(doc_template.substitute(info))\n> >          target_def.append(def_template.substitute(info))\n> >\n> > -        ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > +        target_ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > +\n> > +    vendor_ctrl_doc_sub = []\n> > +    vendor_ctrl_template = string.Template('''\n> > +namespace ${vendor} {\n> > +\n> > +${vendor_controls_str}\n> > +\n> > +} /* namespace ${vendor} */''')\n> > +\n> > +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> > +        vendor_ctrl_doc_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\\n\\n'.join(ctrls_doc[vendor])}))\n> > +\n> > +    vendor_ctrl_def_sub = []\n> > +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> > +        vendor_ctrl_def_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\\n'.join(ctrls_def[vendor])}))\n> > +\n> > +    vendor_ctrl_map_sub = []\n> > +    vendor_ctrl_template = string.Template('''namespace ${vendor} {\n> > +/**\n> > + * \\\\brief List of all supported ${vendor} vendor controls\n> > + *\n> > + * Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > + * set through Request::controls() and returned out through Request::metadata().\n> > + */\n> > +extern const ControlIdMap controls {\n> > +${vendor_controls_map}\n> > +};\n> > +\n> > +} /* namespace ${vendor} */\n> > +''')\n> > +\n> > +    for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:\n> > +        vendor_ctrl_map_sub.append(vendor_ctrl_template.substitute({'vendor': vendor,\n> > +                                                                    'vendor_controls_map': '\\n'.join(ctrls_map[vendor])}))\n> >\n> >      return {\n> > -        'controls_doc': '\\n\\n'.join(ctrls_doc),\n> > -        'controls_def': '\\n'.join(ctrls_def),\n> > -        'draft_controls_doc': '\\n\\n'.join(draft_ctrls_doc),\n> > -        'draft_controls_def': '\\n\\n'.join(draft_ctrls_def),\n> > -        'controls_map': '\\n'.join(ctrls_map),\n> > +        'controls_doc': '\\n\\n'.join(ctrls_doc['libcamera']),\n> > +        'controls_def': '\\n'.join(ctrls_def['libcamera']),\n> > +        'draft_controls_doc': '\\n\\n'.join(ctrls_doc['draft']),\n> > +        'draft_controls_def': '\\n\\n'.join(ctrls_def['draft']),\n> > +        'controls_map': '\\n'.join(ctrls_map['libcamera'] + ctrls_map['draft']),\n> > +        'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > +        'vendor_controls_def': '\\n'.join(vendor_ctrl_def_sub),\n> > +        'vendor_controls_map': '\\n'.join(vendor_ctrl_map_sub),\n> >      }\n> >\n> >\n> > -def generate_h(controls):\n> > +def generate_h(controls, mode):\n> >      enum_template_start = string.Template('''enum ${name}Enum {''')\n> >      enum_value_template = string.Template('''\\t${name} = ${value},''')\n> >      enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')\n> >      template = string.Template('''extern const Control<${type}> ${name};''')\n> >\n> > -    ctrls = []\n> > -    draft_ctrls = []\n> > -    ids = []\n> > -    id_value = 1\n> > +    ctrls = {}\n> > +    ids = {}\n> > +    id_value = {}\n> >\n> >      for ctrl in controls:\n> >          id_name = snake_case(ctrl.name).upper()\n> >\n> > -        ids.append('\\t' + id_name + ' = ' + str(id_value) + ',')\n> > +        vendor = ctrl.vendor\n> > +        if vendor is None:\n> > +            vendor = 'draft' if ctrl.is_draft else 'libcamera'\n> > +\n> > +        if vendor not in ctrls:\n> > +            ids[vendor] = []\n> > +            id_value[vendor] = 1\n> > +            ctrls[vendor] = []\n> > +\n> > +        # Core and draft controls use the same ID value\n> > +        target_ids = ids['libcamera'] if vendor in ['libcamera', 'draft'] else ids[vendor]\n> > +        target_ids.append('\\t' + id_name + ' = ' + str(id_value[vendor]) + ',')\n> >\n> >          info = {\n> >              'name': ctrl.name,\n> >              'type': ctrl.type,\n> >          }\n> >\n> > -        target_ctrls = ctrls\n> > +        target_ctrls = ctrls['libcamera']\n> >          if ctrl.is_draft:\n> > -            target_ctrls = draft_ctrls\n> > +            target_ctrls = ctrls['draft']\n> > +        elif vendor != 'libcamera':\n> > +            target_ctrls = ctrls[vendor]\n> >\n> >          if ctrl.is_enum:\n> >              target_ctrls.append(enum_template_start.substitute(info))\n> > @@ -257,12 +316,37 @@ def generate_h(controls):\n> >              target_ctrls.append(enum_values_template.substitute(values_info))\n> >\n> >          target_ctrls.append(template.substitute(info))\n> > -        id_value += 1\n> > +        id_value[vendor] += 1\n> > +\n> > +    vendor_template = string.Template('''\n> > +namespace ${vendor} {\n> > +\n> > +#define LIBCAMERA_${vendor_def}_VENDOR_${mode}\n> > +\n> > +enum {\n> > +${vendor_enums}\n> > +};\n> > +\n> > +extern const ControlIdMap controls;\n> > +\n> > +${vendor_controls}\n> > +\n> > +} /* namespace ${vendor} */\n> > +''')\n> > +\n> > +    vendor_sub = []\n> > +    for vendor in [v for v in ctrls.keys() if v not in ['libcamera', 'draft']]:\n> > +        vendor_sub.append(vendor_template.substitute({'mode': mode.upper(),\n> > +                                                      'vendor': vendor,\n> > +                                                      'vendor_def': vendor.upper(),\n> > +                                                      'vendor_enums': '\\n'.join(ids[vendor]),\n> > +                                                      'vendor_controls': '\\n'.join(ctrls[vendor])}))\n> >\n> >      return {\n> > -        'ids': '\\n'.join(ids),\n> > -        'controls': '\\n'.join(ctrls),\n> > -        'draft_controls': '\\n'.join(draft_ctrls)\n> > +        'ids': '\\n'.join(ids['libcamera']),\n> > +        'controls': '\\n'.join(ctrls['libcamera']),\n> > +        'draft_controls': '\\n'.join(ctrls['draft']),\n> > +        'vendor_controls': '\\n'.join(vendor_sub)\n> >      }\n> >\n> >\n> > @@ -284,6 +368,8 @@ def main(argv):\n> >                          help='Input file name.')\n> >      parser.add_argument('template', type=str,\n> >                          help='Template file name.')\n> > +    parser.add_argument('--mode', type=str, required=True, choices=['controls', 'properties'],\n> > +                        help='Mode of operation')\n> >      args = parser.parse_args(argv[1:])\n> >\n> >      data = open(args.input, 'rb').read()\n> > @@ -293,7 +379,7 @@ def main(argv):\n> >      if args.template.endswith('.cpp.in'):\n> >          data = generate_cpp(controls)\n> >      elif args.template.endswith('.h.in'):\n> > -        data = generate_h(controls)\n> > +        data = generate_h(controls, args.mode)\n> >      else:\n> >          raise RuntimeError('Unknown template type')\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C00EBDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Nov 2023 10:14:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1F0B61DAD;\n\tMon, 20 Nov 2023 11:14:06 +0100 (CET)","from mail-yw1-x1133.google.com (mail-yw1-x1133.google.com\n\t[IPv6:2607:f8b0:4864:20::1133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A56161DAD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Nov 2023 11:14:04 +0100 (CET)","by mail-yw1-x1133.google.com with SMTP id\n\t00721157ae682-5af6c445e9eso46096217b3.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Nov 2023 02:14:04 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1700475246;\n\tbh=RAkZ9G5D8S1RMxAwWt7z7YMLzAaPnZKnOOCqAhL/UZg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=czMd8i12o+xMoyvLk0W5RLi2LY5QLwzNnBZYNiRLzN8Nnzb2/KOBe6GFUdyLEf+rQ\n\t0xIOBbWstorQRAamJUF/ktgLFBjzXbC9/rNfzpTbmACiyg9sP2NUmscijUpRxQsJWF\n\t1myPb3Vmx/onHTklE2cgfaBfZNWzhzoijipGSxI164en3PWNPg1wkHcSBXPdNIyodh\n\tHcaf7MZcDZUMYslo7xbQZJTppWZIzWiMGgo2QFRbE0VnQjmGwjBhnaUWdbJOStbN+P\n\tZRzNqiT11+ua9EENUzobi7TxKtWNff1+XmxZh2s0BI44rxnID8vFUnujiSuqt9wC0P\n\tu30r0E0I73OGw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1700475243; x=1701080043;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=vSFKiXx6AFYWtYAkTz8XwpVX9X1LLSf1puJ74tV8XFk=;\n\tb=VYEyYAJiAh+/BGycMdAUzoFj3yF1Nn714IZBFU7coBT+2Nt8dME7fYNdCM/rUiqSr3\n\tDn9VcaHmK+9fm4w32C/nh2w8EhFow10f8Uud7BNFU2NNu//X7daQ1zS2WL5YoDua7/nf\n\tRy+vsY+eyK6+bFS7Ak0RNrNYKjqF0D6dt9O0nC5fjd4m3mJVlEbCAemwtbK6hhqN982c\n\tlYNTCkiVBOmlIQ/GCxpVGk+NuQeJqSwShsS+Q4uW6TLtthoPuGzU8Dk7IaeEoCTaZ1uY\n\tSUcs2IS3xktd6JFupuNusXXRfyn0C58WfxR4jyWmBl67H/58hsP6gaC+/w4fVj8KZDpj\n\tHvaA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"VYEyYAJi\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1700475243; x=1701080043;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=vSFKiXx6AFYWtYAkTz8XwpVX9X1LLSf1puJ74tV8XFk=;\n\tb=lFmEOtPHLYtxXg71AeapgnDe1NpR88vS/fTpF5LjrXB3ldj5DKCeWXJ98JRI/WAhlx\n\t90JMpwFl856qLyrNmTvLnRPO0zgD7BvCi2UO0oLNvDErTZqFmR34cVQNlIdQa/1atELZ\n\tFx7XwoMMvrdmRX+xKt0artM759FLjzkyRYZTx88LnVxh3oRuvvdMVxft04QJCoJsuMAg\n\tLJ0eveg/POsGJE2jYuN8WdTXswTAl2Dkn1TUYTL1HdDbwfrkHRkyjZoi6B3YB4Z3q+Xz\n\tcp5a06aGke0R57OcSdjn7x8z5aRYtCGGso4JTvoqbkIYJhoJW+kyO16e8nOUXdXZmTke\n\tNdzA==","X-Gm-Message-State":"AOJu0YzAILy05275fsGmP9zvTCHg+2wIEYD+68n9pXFlrqzy5W6sqyVZ\n\tduGTrupGmIczykKfZWJRsUwS+ZYZUwbKE03epqLnOkoTV/CG0h9ZDmA=","X-Google-Smtp-Source":"AGHT+IEFNoiPx2dKES4fnpWTu0dT8zsTaoSOWu07WhoMQW772IfXyaEIDB40ld7a6yfhP6JEZIj3J7bAwnC0iaKDxB0=","X-Received":"by 2002:a0d:ce44:0:b0:5b3:21cd:ba76 with SMTP id\n\tq65-20020a0dce44000000b005b321cdba76mr7278616ywd.4.1700475242760;\n\tMon, 20 Nov 2023 02:14:02 -0800 (PST)","MIME-Version":"1.0","References":"<20231110110002.21381-1-naush@raspberrypi.com>\n\t<20231110110002.21381-2-naush@raspberrypi.com>\n\t<20231120031740.GE524@pendragon.ideasonboard.com>","In-Reply-To":"<20231120031740.GE524@pendragon.ideasonboard.com>","Date":"Mon, 20 Nov 2023 10:13:28 +0000","Message-ID":"<CAEmqJPrx0+TreQALXZa2ybQGe3FJRHZTjXzMAX=49PGf04mFQw@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 1/5] controls: Add vendor\n\tcontrol/property support to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]