[{"id":28179,"web_url":"https://patchwork.libcamera.org/comment/28179/","msgid":"","date":"2023-11-27T16:43:37","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n>\n> vendor: rpi\n> controls:\n> - MyExampleControl:\n> type: string\n> description: |\n> Test for libcamera vendor-specific controls.\n>\n> This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> applications to conditionally compile code if the specific vendor\n> controls are present. For the python bindings, the control is available\n> with libcamera.controls.rpi.MyExampleControl. The above controls\n> example applies similarly to properties.\n>\n> Existing libcamera controls defined in control_ids.yaml are given the\n> \"libcamera\" vendor tag.\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> As a drive-by, sort and redefine the output command line argument in\n> gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> consistency.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/control_ids.h.in | 2 +\n> include/libcamera/meson.build | 15 ++-\n> include/libcamera/property_ids.h.in | 2 +\n> src/libcamera/control_ids.cpp.in | 4 +\n> src/libcamera/control_ids.yaml | 1 +\n> src/libcamera/meson.build | 5 +-\n> src/libcamera/property_ids.cpp.in | 4 +\n> src/libcamera/property_ids.yaml | 1 +\n> src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> .../libcamera/py_properties_generated.cpp.in | 3 +\n> utils/gen-controls.py | 120 +++++++++++++-----\n> 12 files changed, 171 insertions(+), 70 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> } /* namespace controls */\n>\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index a24c50d66a82..2c8c0258c95e 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n>\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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\nIf you add an emtpy line here already, you can avoid doing so in\n\" libcamera: controls: Use vendor tags for draft controls and properties\"\n\n> #endif\n>\n> /**\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 5827d7ecef49..ff74ce1deedb 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -6,6 +6,7 @@\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> +vendor: libcamera\n> controls:\n> - AeEnable:\n> type: bool\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..0771ac5c091f 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\nSame comment as above. Now I wonder if that's intentional then :)\n\n> #endif\n>\n> /**\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index f35563842a5a..45f3609b4236 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -4,6 +4,7 @@\n> #\n> %YAML 1.1\n> ---\n> +vendor: libcamera\n> controls:\n> - Location:\n> type: int32_t\n> diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> index 9948c41e42b1..dfd7c179a883 100755\n> --- a/src/py/libcamera/gen-py-controls.py\n> +++ b/src/py/libcamera/gen-py-controls.py\n> @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> def generate_py(controls, mode):\n> out = ''\n>\n> - for ctrl in controls:\n> - name, ctrl = ctrl.popitem()\n> -\n> - if ctrl.get('draft'):\n> - ns = 'libcamera::{}::draft::'.format(mode)\n> - container = 'draft'\n> - else:\n> - ns = 'libcamera::{}::'.format(mode)\n> - container = 'controls'\n> + vendors_class_def = []\n> + vendor_defs = []\n> + vendors = []\n> + for vendor, ctrl_list in controls.items():\n> + for ctrls in ctrl_list:\n> + name, ctrl = ctrls.popitem()\n> +\n> + if vendor not in vendors and vendor != 'libcamera':\n> + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> + vendors.append(vendor)\n> +\n> + if ctrl.get('draft'):\n> + ns = 'libcamera::{}::draft::'.format(mode)\n> + container = 'draft'\n> + elif vendor != 'libcamera':\n> + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> + container = vendor\n> + else:\n> + ns = 'libcamera::{}::'.format(mode)\n> + container = 'controls'\n>\n> - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n>\n> - enum = ctrl.get('enum')\n> - if not enum:\n> - continue\n> + enum = ctrl.get('enum')\n> + if not enum:\n> + continue\n>\n> - cpp_enum = name + 'Enum'\n> + cpp_enum = name + 'Enum'\n>\n> - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n>\n> - if mode == 'controls':\n> - # Adjustments for controls\n> - if name == 'LensShadingMapMode':\n> - prefix = 'LensShadingMapMode'\n> + if mode == 'controls':\n> + # Adjustments for controls\n> + if name == 'LensShadingMapMode':\n> + prefix = 'LensShadingMapMode'\n> + else:\n> + prefix = find_common_prefix([e['name'] for e in enum])\n> else:\n> + # Adjustments for properties\n> prefix = find_common_prefix([e['name'] for e in enum])\n> - else:\n> - # Adjustments for properties\n> - prefix = find_common_prefix([e['name'] for e in enum])\n>\n> - for entry in enum:\n> - cpp_enum = entry['name']\n> - py_enum = entry['name'][len(prefix):]\n> + for entry in enum:\n> + cpp_enum = entry['name']\n> + py_enum = entry['name'][len(prefix):]\n>\n> - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n>\n> - out += '\\t;\\n\\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\nThis part \"looks\" ok to me, but I'm not familiar with this code, so\njust \"ack\"\n\n>\n>\n> def fill_template(template, data):\n> @@ -75,14 +89,14 @@ def fill_template(template, data):\n> def main(argv):\n> # Parse command line arguments\n> parser = argparse.ArgumentParser()\n> - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> + parser.add_argument('--mode', '-m', type=str, required=True,\n> + help='Mode is either \"controls\" or \"properties\"')\n> + parser.add_argument('--output', '-o', metavar='file', type=str,\n> help='Output file name. Defaults to standard output if not specified.')\n> parser.add_argument('input', type=str,\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,\n> - help='Mode is either \"controls\" or \"properties\"')\n> args = parser.parse_args(argv[1:])\n>\n> if args.mode not in ['controls', 'properties']:\n> @@ -90,7 +104,10 @@ def main(argv):\n> return -1\n>\n> data = open(args.input, 'rb').read()\n> - controls = yaml.safe_load(data)['controls']\n> +\n> + controls = {}\n> + vendor = yaml.safe_load(data)['vendor']\n> + controls[vendor] = yaml.safe_load(data)['controls']\n>\n> data = generate_py(controls, args.mode)\n>\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_(m, \"controls\");\n> \tauto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> \tauto draft = py::class_(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..c1172cb26e7b 100755\n> --- a/utils/gen-controls.py\n> +++ b/utils/gen-controls.py\n> @@ -35,11 +35,12 @@ class ControlEnum(object):\n>\n>\n> class Control(object):\n> - def __init__(self, name, data):\n> + def __init__(self, name, data, vendor):\n> self.__name = name\n> self.__data = data\n> self.__enum_values = None\n> self.__size = None\n> + self.__vendor = vendor\n>\n> enum_values = data.get('enum')\n> if enum_values is not None:\n> @@ -89,6 +90,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.__vendor\n> +\n> @property\n> def name(self):\n> \"\"\"The control name (CamelCase)\"\"\"\n> @@ -145,15 +151,18 @@ ${description}\n> enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n>\n> - ctrls_doc = []\n> - ctrls_def = []\n> - draft_ctrls_doc = []\n> - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> + if vendor not in ctrls_doc:\n> + ctrls_doc[vendor] = []\n> + ctrls_def[vendor] = []\n> +\n> info = {\n> 'name': ctrl.name,\n> 'type': ctrl.type,\n> @@ -161,11 +170,8 @@ ${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>\n> if ctrl.is_enum:\n> enum_doc = []\n> @@ -203,39 +209,68 @@ ${description}\n>\n> ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n>\n> + vendor_ctrl_doc_sub = []\n> + vendor_ctrl_template = string.Template('''\n> +/**\n> + * \\\\brief Namespace for ${vendor} controls\n> + */\n> +namespace ${vendor} {\n> +\n> +${vendor_controls_str}\n> +\n> +} /* namespace ${vendor} */''')\n> +\n> + for vendor in [v for v in ctrls_doc.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_def.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\nIt would have been nicer if 'libcamera', 'draft' and vendor controls\nwould be handled through the same mechanism, but this would probably\nrequire changing how the .cpp.in file is structured and to assign a\ndedicated ::core namespace to libcamera controls which would make\nhandling them more verbose than necessary.\n\nI think this is fine even if a bit convoluted\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_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),\n> + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n> +\n> +enum {\n> +${vendor_enums}\n> +};\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\nSame comment here as per the cpp file. I guess it's fine!\n\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> @@ -278,22 +336,26 @@ def main(argv):\n>\n> # Parse command line arguments\n> parser = argparse.ArgumentParser()\n> - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> + help='Mode of operation')\n> + parser.add_argument('--output', '-o', metavar='file', type=str,\n> help='Output file name. Defaults to standard output if not specified.')\n> parser.add_argument('input', type=str,\n> help='Input file name.')\n> parser.add_argument('template', type=str,\n> help='Template file name.')\n> +\n> args = parser.parse_args(argv[1:])\n>\n> data = open(args.input, 'rb').read()\n> + vendor = yaml.safe_load(data)['vendor']\n> controls = yaml.safe_load(data)['controls']\n> - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n>\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\nMinors and the 'ack I trust you' on the py part, this looks good to\nme! Thanks\n\nReviewed-by: Jacopo Mondi \n\n>\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","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 B4359BDE6B\n\tfor ;\n\tMon, 27 Nov 2023 16:43:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA6CA629BC;\n\tMon, 27 Nov 2023 17:43:42 +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 8A143629B6\n\tfor ;\n\tMon, 27 Nov 2023 17:43:41 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4528512;\n\tMon, 27 Nov 2023 17:43:06 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701103423;\n\tbh=DiJwmz/+edKTG78Ah6ViUhKIFlZlUPT743+XXRO3uvo=;\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=0aXLQPapMqxIE1mYNRIz0opRp91WvBgfS+OnnKB7WLX+gQye9Szd7Cu3NXaFEGgnF\n\tfZjEvzU6d23Xk4OX20vz9XrlsCKRSib75LN1ONpO8gZ0lrIsfGjbqhsyLcSUjRvQdP\n\tQqtSaS9MQuahWSxAAg6b8wun3dzLEPmw16KXISGTEQse8Kh7eHFzuKXHxX33ZBY1Xv\n\tMs7i7N7kDyFyB85u7dR7Wx+iSp6H6j+lzyQJLX23fkVPPzJTugb0dEkFzU9wco22aY\n\tx5uo8POdzM0ZeY3xUymJM7dclm8qELb6Gld4w+d8d7Zm5xAh46LFGSFkHiMG1AEos0\n\tDtB6oCBCDbuUQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701103386;\n\tbh=DiJwmz/+edKTG78Ah6ViUhKIFlZlUPT743+XXRO3uvo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p8ruc6E/9c+RpjXadgypyfRFVHO+S93qymEM4CzpOEhENRjTTtOwr2anb7/KxeakS\n\tRDznaxBPtdKnUNkeKyz1fxWybG+7wM2CqmiRPcqfAqjOXuWoaj8tOYGrlBY5PXVJQP\n\t8hdhZUISxJLkQbXqTtLCk9uQU6XmZrpH/bHa8oCk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"p8ruc6E/\"; dkim-atps=neutral","Date":"Mon, 27 Nov 2023 17:43:37 +0100","To":"Naushir Patuck ","Message-ID":"","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231127112916.26364-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28193,"web_url":"https://patchwork.libcamera.org/comment/28193/","msgid":"<170116611323.630990.17283607147485135126@ping.linuxembedded.co.uk>","date":"2023-11-28T10:08:33","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2023-11-27 16:43:37)\n> Hi Naush\n> \n> On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> >\n> > vendor: rpi\n> > controls:\n> > - MyExampleControl:\n> > type: string\n> > description: |\n> > Test for libcamera vendor-specific controls.\n> >\n> > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > applications to conditionally compile code if the specific vendor\n> > controls are present. For the python bindings, the control is available\n> > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > example applies similarly to properties.\n> >\n> > Existing libcamera controls defined in control_ids.yaml are given the\n> > \"libcamera\" vendor tag.\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> > As a drive-by, sort and redefine the output command line argument in\n> > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > consistency.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/control_ids.h.in | 2 +\n> > include/libcamera/meson.build | 15 ++-\n> > include/libcamera/property_ids.h.in | 2 +\n> > src/libcamera/control_ids.cpp.in | 4 +\n> > src/libcamera/control_ids.yaml | 1 +\n> > src/libcamera/meson.build | 5 +-\n> > src/libcamera/property_ids.cpp.in | 4 +\n> > src/libcamera/property_ids.yaml | 1 +\n> > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > utils/gen-controls.py | 120 +++++++++++++-----\n> > 12 files changed, 171 insertions(+), 70 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> > } /* namespace controls */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index a24c50d66a82..2c8c0258c95e 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> \n> If you add an emtpy line here already, you can avoid doing so in\n> \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> \n> > #endif\n> >\n> > /**\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 5827d7ecef49..ff74ce1deedb 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -6,6 +6,7 @@\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> > +vendor: libcamera\n> > controls:\n> > - AeEnable:\n> > type: bool\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..0771ac5c091f 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> \n> Same comment as above. Now I wonder if that's intentional then :)\n> \n> > #endif\n> >\n> > /**\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index f35563842a5a..45f3609b4236 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -4,6 +4,7 @@\n> > #\n> > %YAML 1.1\n> > ---\n> > +vendor: libcamera\n> > controls:\n> > - Location:\n> > type: int32_t\n> > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > index 9948c41e42b1..dfd7c179a883 100755\n> > --- a/src/py/libcamera/gen-py-controls.py\n> > +++ b/src/py/libcamera/gen-py-controls.py\n> > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > def generate_py(controls, mode):\n> > out = ''\n> >\n> > - for ctrl in controls:\n> > - name, ctrl = ctrl.popitem()\n> > -\n> > - if ctrl.get('draft'):\n> > - ns = 'libcamera::{}::draft::'.format(mode)\n> > - container = 'draft'\n> > - else:\n> > - ns = 'libcamera::{}::'.format(mode)\n> > - container = 'controls'\n> > + vendors_class_def = []\n> > + vendor_defs = []\n> > + vendors = []\n> > + for vendor, ctrl_list in controls.items():\n> > + for ctrls in ctrl_list:\n> > + name, ctrl = ctrls.popitem()\n> > +\n> > + if vendor not in vendors and vendor != 'libcamera':\n> > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > + vendors.append(vendor)\n> > +\n> > + if ctrl.get('draft'):\n> > + ns = 'libcamera::{}::draft::'.format(mode)\n> > + container = 'draft'\n> > + elif vendor != 'libcamera':\n> > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > + container = vendor\n> > + else:\n> > + ns = 'libcamera::{}::'.format(mode)\n> > + container = 'controls'\n> >\n> > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> >\n> > - enum = ctrl.get('enum')\n> > - if not enum:\n> > - continue\n> > + enum = ctrl.get('enum')\n> > + if not enum:\n> > + continue\n> >\n> > - cpp_enum = name + 'Enum'\n> > + cpp_enum = name + 'Enum'\n> >\n> > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> >\n> > - if mode == 'controls':\n> > - # Adjustments for controls\n> > - if name == 'LensShadingMapMode':\n> > - prefix = 'LensShadingMapMode'\n> > + if mode == 'controls':\n> > + # Adjustments for controls\n> > + if name == 'LensShadingMapMode':\n> > + prefix = 'LensShadingMapMode'\n> > + else:\n> > + prefix = find_common_prefix([e['name'] for e in enum])\n> > else:\n> > + # Adjustments for properties\n> > prefix = find_common_prefix([e['name'] for e in enum])\n> > - else:\n> > - # Adjustments for properties\n> > - prefix = find_common_prefix([e['name'] for e in enum])\n> >\n> > - for entry in enum:\n> > - cpp_enum = entry['name']\n> > - py_enum = entry['name'][len(prefix):]\n> > + for entry in enum:\n> > + cpp_enum = entry['name']\n> > + py_enum = entry['name'][len(prefix):]\n> >\n> > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> >\n> > - out += '\\t;\\n\\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> This part \"looks\" ok to me, but I'm not familiar with this code, so\n> just \"ack\"\n> \n> >\n> >\n> > def fill_template(template, data):\n> > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > def main(argv):\n> > # Parse command line arguments\n> > parser = argparse.ArgumentParser()\n> > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > + help='Mode is either \"controls\" or \"properties\"')\n> > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > help='Output file name. Defaults to standard output if not specified.')\n> > parser.add_argument('input', type=str,\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,\n> > - help='Mode is either \"controls\" or \"properties\"')\n> > args = parser.parse_args(argv[1:])\n> >\n> > if args.mode not in ['controls', 'properties']:\n> > @@ -90,7 +104,10 @@ def main(argv):\n> > return -1\n> >\n> > data = open(args.input, 'rb').read()\n> > - controls = yaml.safe_load(data)['controls']\n> > +\n> > + controls = {}\n> > + vendor = yaml.safe_load(data)['vendor']\n> > + controls[vendor] = yaml.safe_load(data)['controls']\n> >\n> > data = generate_py(controls, args.mode)\n> >\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_(m, \"controls\");\n> > auto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > auto draft = py::class_(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..c1172cb26e7b 100755\n> > --- a/utils/gen-controls.py\n> > +++ b/utils/gen-controls.py\n> > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> >\n> >\n> > class Control(object):\n> > - def __init__(self, name, data):\n> > + def __init__(self, name, data, vendor):\n> > self.__name = name\n> > self.__data = data\n> > self.__enum_values = None\n> > self.__size = None\n> > + self.__vendor = vendor\n> >\n> > enum_values = data.get('enum')\n> > if enum_values is not None:\n> > @@ -89,6 +90,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.__vendor\n> > +\n> > @property\n> > def name(self):\n> > \"\"\"The control name (CamelCase)\"\"\"\n> > @@ -145,15 +151,18 @@ ${description}\n> > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> >\n> > - ctrls_doc = []\n> > - ctrls_def = []\n> > - draft_ctrls_doc = []\n> > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > + if vendor not in ctrls_doc:\n> > + ctrls_doc[vendor] = []\n> > + ctrls_def[vendor] = []\n> > +\n> > info = {\n> > 'name': ctrl.name,\n> > 'type': ctrl.type,\n> > @@ -161,11 +170,8 @@ ${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> >\n> > if ctrl.is_enum:\n> > enum_doc = []\n> > @@ -203,39 +209,68 @@ ${description}\n> >\n> > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> >\n> > + vendor_ctrl_doc_sub = []\n> > + vendor_ctrl_template = string.Template('''\n> > +/**\n> > + * \\\\brief Namespace for ${vendor} controls\n> > + */\n> > +namespace ${vendor} {\n> > +\n> > +${vendor_controls_str}\n> > +\n> > +} /* namespace ${vendor} */''')\n> > +\n> > + for vendor in [v for v in ctrls_doc.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_def.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> \n> It would have been nicer if 'libcamera', 'draft' and vendor controls\n> would be handled through the same mechanism, but this would probably\n> require changing how the .cpp.in file is structured and to assign a\n> dedicated ::core namespace to libcamera controls which would make\n> handling them more verbose than necessary.\n> \n> I think this is fine even if a bit convoluted\n\nAbsolutely. I don't think we should have to specify\nlibcamera::core::brightness for instance, so stripping out (only) the\ncore namespace is reasonable.\n\nBut ... why is draft referenced here? Are draft controls not just\ntreated like a vendor control?\n\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_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),\n> > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n\nAha, I guess we don't want to define things like\nLIBCAMERA_HAS_DRAFT_VENDOR_CONTROLS so I can see why draft is still\ntreated as a bit of a special case for now.\n\nWith Jacopo's minors...\n\nReviewed-by: Kieran Bingham \n\n\n> > +\n> > +enum {\n> > +${vendor_enums}\n> > +};\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> Same comment here as per the cpp file. I guess it's fine!\n> \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> > @@ -278,22 +336,26 @@ def main(argv):\n> >\n> > # Parse command line arguments\n> > parser = argparse.ArgumentParser()\n> > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > + help='Mode of operation')\n> > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > help='Output file name. Defaults to standard output if not specified.')\n> > parser.add_argument('input', type=str,\n> > help='Input file name.')\n> > parser.add_argument('template', type=str,\n> > help='Template file name.')\n> > +\n> > args = parser.parse_args(argv[1:])\n> >\n> > data = open(args.input, 'rb').read()\n> > + vendor = yaml.safe_load(data)['vendor']\n> > controls = yaml.safe_load(data)['controls']\n> > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> >\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> Minors and the 'ack I trust you' on the py part, this looks good to\n> me! Thanks\n> \n> Reviewed-by: Jacopo Mondi \n> \n> >\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","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 38BAFC31E9\n\tfor ;\n\tTue, 28 Nov 2023 10:08:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96440629BD;\n\tTue, 28 Nov 2023 11:08:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B10B61DA5\n\tfor ;\n\tTue, 28 Nov 2023 11:08:36 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 16A2BBEB;\n\tTue, 28 Nov 2023 11:08:01 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701166118;\n\tbh=lSBPSIjL0SEcf3iLg38o1dcoO5tda5lSJJgQn0GJuUE=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rU9Qz1/t65yG+dGwO7/aMqM4cZj4BYcwTD8Lja85HwtK2ileiFyAdqP4A7ErPSDEM\n\tFcHx1F0ohqgo6mX+Eho6HvZWmknn1c3oVnbcCRnmlZl5Jk9A41S6CPTbqQDE6CAaJZ\n\trnBZOztq08ghmyEqzv8YXdo8uK5Zncetvf3l64+NOUKb/7rI1STFaK5YveSyxTjNIw\n\tlT3H/irvEU0YOP5xrKYWcrDG/zR9DIbaGfk0d3LVFZzowCmVrC0NIvGOrsNDP+0xMB\n\titT8hwKC0Vt5C1A7ZVirHtOg3AAVHAntJWLZHQGH1SE3q2vxdDtnPGz5I+SLqcdJcP\n\tjafUeNlXmqdnQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701166081;\n\tbh=lSBPSIjL0SEcf3iLg38o1dcoO5tda5lSJJgQn0GJuUE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=fOc1wIdCOhsMwSq4H9UepQSgkQRySO1DxHoO1QaTa91XKPWlzWaR4u1IC8I7hiD63\n\tVy9Hvk++PhleVTyJnFRhMWJZMBH03K6rpWk4prmi3vNdTXbrAiAJT0qQD33qy5Db66\n\tJoREMtd2H9yLQeGNZAMJFWcdq3tYIf1Fs6pMJdN4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fOc1wIdC\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t","To":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel , \n\tNaushir Patuck ","Date":"Tue, 28 Nov 2023 10:08:33 +0000","Message-ID":"<170116611323.630990.17283607147485135126@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28194,"web_url":"https://patchwork.libcamera.org/comment/28194/","msgid":"","date":"2023-11-28T10:13:27","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Tue, Nov 28, 2023 at 10:08:33AM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2023-11-27 16:43:37)\n> > Hi Naush\n> >\n> > On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> > >\n> > > vendor: rpi\n> > > controls:\n> > > - MyExampleControl:\n> > > type: string\n> > > description: |\n> > > Test for libcamera vendor-specific controls.\n> > >\n> > > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > > applications to conditionally compile code if the specific vendor\n> > > controls are present. For the python bindings, the control is available\n> > > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > > example applies similarly to properties.\n> > >\n> > > Existing libcamera controls defined in control_ids.yaml are given the\n> > > \"libcamera\" vendor tag.\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> > > As a drive-by, sort and redefine the output command line argument in\n> > > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > > consistency.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/control_ids.h.in | 2 +\n> > > include/libcamera/meson.build | 15 ++-\n> > > include/libcamera/property_ids.h.in | 2 +\n> > > src/libcamera/control_ids.cpp.in | 4 +\n> > > src/libcamera/control_ids.yaml | 1 +\n> > > src/libcamera/meson.build | 5 +-\n> > > src/libcamera/property_ids.cpp.in | 4 +\n> > > src/libcamera/property_ids.yaml | 1 +\n> > > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > > utils/gen-controls.py | 120 +++++++++++++-----\n> > > 12 files changed, 171 insertions(+), 70 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> > > } /* namespace controls */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index a24c50d66a82..2c8c0258c95e 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> > >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> >\n> > If you add an emtpy line here already, you can avoid doing so in\n> > \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> >\n> > > #endif\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 5827d7ecef49..ff74ce1deedb 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -6,6 +6,7 @@\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> > > +vendor: libcamera\n> > > controls:\n> > > - AeEnable:\n> > > type: bool\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..0771ac5c091f 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> >\n> > Same comment as above. Now I wonder if that's intentional then :)\n> >\n> > > #endif\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > index f35563842a5a..45f3609b4236 100644\n> > > --- a/src/libcamera/property_ids.yaml\n> > > +++ b/src/libcamera/property_ids.yaml\n> > > @@ -4,6 +4,7 @@\n> > > #\n> > > %YAML 1.1\n> > > ---\n> > > +vendor: libcamera\n> > > controls:\n> > > - Location:\n> > > type: int32_t\n> > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > > index 9948c41e42b1..dfd7c179a883 100755\n> > > --- a/src/py/libcamera/gen-py-controls.py\n> > > +++ b/src/py/libcamera/gen-py-controls.py\n> > > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > > def generate_py(controls, mode):\n> > > out = ''\n> > >\n> > > - for ctrl in controls:\n> > > - name, ctrl = ctrl.popitem()\n> > > -\n> > > - if ctrl.get('draft'):\n> > > - ns = 'libcamera::{}::draft::'.format(mode)\n> > > - container = 'draft'\n> > > - else:\n> > > - ns = 'libcamera::{}::'.format(mode)\n> > > - container = 'controls'\n> > > + vendors_class_def = []\n> > > + vendor_defs = []\n> > > + vendors = []\n> > > + for vendor, ctrl_list in controls.items():\n> > > + for ctrls in ctrl_list:\n> > > + name, ctrl = ctrls.popitem()\n> > > +\n> > > + if vendor not in vendors and vendor != 'libcamera':\n> > > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > > + vendors.append(vendor)\n> > > +\n> > > + if ctrl.get('draft'):\n> > > + ns = 'libcamera::{}::draft::'.format(mode)\n> > > + container = 'draft'\n> > > + elif vendor != 'libcamera':\n> > > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > > + container = vendor\n> > > + else:\n> > > + ns = 'libcamera::{}::'.format(mode)\n> > > + container = 'controls'\n> > >\n> > > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > >\n> > > - enum = ctrl.get('enum')\n> > > - if not enum:\n> > > - continue\n> > > + enum = ctrl.get('enum')\n> > > + if not enum:\n> > > + continue\n> > >\n> > > - cpp_enum = name + 'Enum'\n> > > + cpp_enum = name + 'Enum'\n> > >\n> > > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > >\n> > > - if mode == 'controls':\n> > > - # Adjustments for controls\n> > > - if name == 'LensShadingMapMode':\n> > > - prefix = 'LensShadingMapMode'\n> > > + if mode == 'controls':\n> > > + # Adjustments for controls\n> > > + if name == 'LensShadingMapMode':\n> > > + prefix = 'LensShadingMapMode'\n> > > + else:\n> > > + prefix = find_common_prefix([e['name'] for e in enum])\n> > > else:\n> > > + # Adjustments for properties\n> > > prefix = find_common_prefix([e['name'] for e in enum])\n> > > - else:\n> > > - # Adjustments for properties\n> > > - prefix = find_common_prefix([e['name'] for e in enum])\n> > >\n> > > - for entry in enum:\n> > > - cpp_enum = entry['name']\n> > > - py_enum = entry['name'][len(prefix):]\n> > > + for entry in enum:\n> > > + cpp_enum = entry['name']\n> > > + py_enum = entry['name'][len(prefix):]\n> > >\n> > > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > >\n> > > - out += '\\t;\\n\\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> > This part \"looks\" ok to me, but I'm not familiar with this code, so\n> > just \"ack\"\n> >\n> > >\n> > >\n> > > def fill_template(template, data):\n> > > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > > def main(argv):\n> > > # Parse command line arguments\n> > > parser = argparse.ArgumentParser()\n> > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > > + help='Mode is either \"controls\" or \"properties\"')\n> > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > help='Output file name. Defaults to standard output if not specified.')\n> > > parser.add_argument('input', type=str,\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,\n> > > - help='Mode is either \"controls\" or \"properties\"')\n> > > args = parser.parse_args(argv[1:])\n> > >\n> > > if args.mode not in ['controls', 'properties']:\n> > > @@ -90,7 +104,10 @@ def main(argv):\n> > > return -1\n> > >\n> > > data = open(args.input, 'rb').read()\n> > > - controls = yaml.safe_load(data)['controls']\n> > > +\n> > > + controls = {}\n> > > + vendor = yaml.safe_load(data)['vendor']\n> > > + controls[vendor] = yaml.safe_load(data)['controls']\n> > >\n> > > data = generate_py(controls, args.mode)\n> > >\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_(m, \"controls\");\n> > > auto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > > auto draft = py::class_(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..c1172cb26e7b 100755\n> > > --- a/utils/gen-controls.py\n> > > +++ b/utils/gen-controls.py\n> > > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> > >\n> > >\n> > > class Control(object):\n> > > - def __init__(self, name, data):\n> > > + def __init__(self, name, data, vendor):\n> > > self.__name = name\n> > > self.__data = data\n> > > self.__enum_values = None\n> > > self.__size = None\n> > > + self.__vendor = vendor\n> > >\n> > > enum_values = data.get('enum')\n> > > if enum_values is not None:\n> > > @@ -89,6 +90,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.__vendor\n> > > +\n> > > @property\n> > > def name(self):\n> > > \"\"\"The control name (CamelCase)\"\"\"\n> > > @@ -145,15 +151,18 @@ ${description}\n> > > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> > >\n> > > - ctrls_doc = []\n> > > - ctrls_def = []\n> > > - draft_ctrls_doc = []\n> > > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > > + if vendor not in ctrls_doc:\n> > > + ctrls_doc[vendor] = []\n> > > + ctrls_def[vendor] = []\n> > > +\n> > > info = {\n> > > 'name': ctrl.name,\n> > > 'type': ctrl.type,\n> > > @@ -161,11 +170,8 @@ ${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> > >\n> > > if ctrl.is_enum:\n> > > enum_doc = []\n> > > @@ -203,39 +209,68 @@ ${description}\n> > >\n> > > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > >\n> > > + vendor_ctrl_doc_sub = []\n> > > + vendor_ctrl_template = string.Template('''\n> > > +/**\n> > > + * \\\\brief Namespace for ${vendor} controls\n> > > + */\n> > > +namespace ${vendor} {\n> > > +\n> > > +${vendor_controls_str}\n> > > +\n> > > +} /* namespace ${vendor} */''')\n> > > +\n> > > + for vendor in [v for v in ctrls_doc.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_def.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> >\n> > It would have been nicer if 'libcamera', 'draft' and vendor controls\n> > would be handled through the same mechanism, but this would probably\n> > require changing how the .cpp.in file is structured and to assign a\n> > dedicated ::core namespace to libcamera controls which would make\n> > handling them more verbose than necessary.\n> >\n> > I think this is fine even if a bit convoluted\n>\n> Absolutely. I don't think we should have to specify\n> libcamera::core::brightness for instance, so stripping out (only) the\n> core namespace is reasonable.\n>\n> But ... why is draft referenced here? Are draft controls not just\n> treated like a vendor control?\n>\n\nThey will be broken out to a dedicated file and to a dedicated\nnamespace (like other vendor controls) in the next patches in the\nseries\n\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_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),\n> > > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n>\n> Aha, I guess we don't want to define things like\n> LIBCAMERA_HAS_DRAFT_VENDOR_CONTROLS so I can see why draft is still\n> treated as a bit of a special case for now.\n>\n> With Jacopo's minors...\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > > +\n> > > +enum {\n> > > +${vendor_enums}\n> > > +};\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> > Same comment here as per the cpp file. I guess it's fine!\n> >\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> > > @@ -278,22 +336,26 @@ def main(argv):\n> > >\n> > > # Parse command line arguments\n> > > parser = argparse.ArgumentParser()\n> > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > > + help='Mode of operation')\n> > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > help='Output file name. Defaults to standard output if not specified.')\n> > > parser.add_argument('input', type=str,\n> > > help='Input file name.')\n> > > parser.add_argument('template', type=str,\n> > > help='Template file name.')\n> > > +\n> > > args = parser.parse_args(argv[1:])\n> > >\n> > > data = open(args.input, 'rb').read()\n> > > + vendor = yaml.safe_load(data)['vendor']\n> > > controls = yaml.safe_load(data)['controls']\n> > > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> > >\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> > Minors and the 'ack I trust you' on the py part, this looks good to\n> > me! Thanks\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > >\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","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 30A25BDE6B\n\tfor ;\n\tTue, 28 Nov 2023 10:13:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 988A361DA5;\n\tTue, 28 Nov 2023 11:13:32 +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 7EDCE61DA5\n\tfor ;\n\tTue, 28 Nov 2023 11:13:31 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 185EABB2;\n\tTue, 28 Nov 2023 11:12:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701166412;\n\tbh=0mn8uJnGkIonpQrJESxNCVHCZ5JarwVnuR+mhySRQ9s=;\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=kp7mcSe2SbYkocSSt0JCI8AhcOQ3KIOMxsezuSt1XvLS222yhc4nXK78rWB3TFz7K\n\tXvL+S7Dm/TVaHoY7m+PaE7N0lppPndvfBE0c9DFPG1aMZsFba3aXMNS4nyi3ihFdi6\n\teFxDCc9Ytb9o+p+GCkQ5EIH2lygoMJZhU8bFthdOa/huFLROU32a4q9fwCwfGXSjJ3\n\t9s76OcUQ5W12zKf84ZRUBbQ48SqEsqoztEHyATVAG9z335ClsYH/hIEfaB/cvE6sFM\n\tgVgDdlMlQqXy30WCIbWwKfh3ZduaK44nWxCdJmKv5xykmM8/Pdh+jJqAAEsgbXdfv0\n\tU9qwWxaf7+syg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701166376;\n\tbh=0mn8uJnGkIonpQrJESxNCVHCZ5JarwVnuR+mhySRQ9s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XnlPH+vQDvdi3JNFBLe8IewmOv2amnZleqhUmBmO9pXXc2W5Z8nATHHo/9ld48wWg\n\t1XQY89ZM3JOt+Jk+9LNb2mrJYT/h2tKUWr26ZzBYAm50jnv0wmeW0PLm/7O4eGaht4\n\t9Lxq0rMStA6eZhl5ujht7AlP99JxK+lbry77oisc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XnlPH+vQ\"; dkim-atps=neutral","Date":"Tue, 28 Nov 2023 11:13:27 +0100","To":"Kieran Bingham ","Message-ID":"","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t\n\t<170116611323.630990.17283607147485135126@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<170116611323.630990.17283607147485135126@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28216,"web_url":"https://patchwork.libcamera.org/comment/28216/","msgid":"<20231130085833.GC8402@pendragon.ideasonboard.com>","date":"2023-11-30T08:58:33","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 27, 2023 at 05:43:37PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> >\n> > vendor: rpi\n> > controls:\n> > - MyExampleControl:\n> > type: string\n> > description: |\n> > Test for libcamera vendor-specific controls.\n> >\n> > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > applications to conditionally compile code if the specific vendor\n> > controls are present. For the python bindings, the control is available\n> > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > example applies similarly to properties.\n> >\n> > Existing libcamera controls defined in control_ids.yaml are given the\n> > \"libcamera\" vendor tag.\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> > As a drive-by, sort and redefine the output command line argument in\n> > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > consistency.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/control_ids.h.in | 2 +\n> > include/libcamera/meson.build | 15 ++-\n> > include/libcamera/property_ids.h.in | 2 +\n> > src/libcamera/control_ids.cpp.in | 4 +\n> > src/libcamera/control_ids.yaml | 1 +\n> > src/libcamera/meson.build | 5 +-\n> > src/libcamera/property_ids.cpp.in | 4 +\n> > src/libcamera/property_ids.yaml | 1 +\n> > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > utils/gen-controls.py | 120 +++++++++++++-----\n> > 12 files changed, 171 insertions(+), 70 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> > } /* namespace controls */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index a24c50d66a82..2c8c0258c95e 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> \n> If you add an emtpy line here already, you can avoid doing so in\n> \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> \n> > #endif\n> >\n> > /**\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 5827d7ecef49..ff74ce1deedb 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -6,6 +6,7 @@\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> > +vendor: libcamera\n> > controls:\n> > - AeEnable:\n> > type: bool\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..0771ac5c091f 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> \n> Same comment as above. Now I wonder if that's intentional then :)\n> \n> > #endif\n> >\n> > /**\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index f35563842a5a..45f3609b4236 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -4,6 +4,7 @@\n> > #\n> > %YAML 1.1\n> > ---\n> > +vendor: libcamera\n> > controls:\n> > - Location:\n> > type: int32_t\n> > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > index 9948c41e42b1..dfd7c179a883 100755\n> > --- a/src/py/libcamera/gen-py-controls.py\n> > +++ b/src/py/libcamera/gen-py-controls.py\n> > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > def generate_py(controls, mode):\n> > out = ''\n> >\n> > - for ctrl in controls:\n> > - name, ctrl = ctrl.popitem()\n> > -\n> > - if ctrl.get('draft'):\n> > - ns = 'libcamera::{}::draft::'.format(mode)\n> > - container = 'draft'\n> > - else:\n> > - ns = 'libcamera::{}::'.format(mode)\n> > - container = 'controls'\n> > + vendors_class_def = []\n> > + vendor_defs = []\n> > + vendors = []\n> > + for vendor, ctrl_list in controls.items():\n> > + for ctrls in ctrl_list:\n> > + name, ctrl = ctrls.popitem()\n> > +\n> > + if vendor not in vendors and vendor != 'libcamera':\n> > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > + vendors.append(vendor)\n> > +\n> > + if ctrl.get('draft'):\n> > + ns = 'libcamera::{}::draft::'.format(mode)\n> > + container = 'draft'\n> > + elif vendor != 'libcamera':\n> > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > + container = vendor\n> > + else:\n> > + ns = 'libcamera::{}::'.format(mode)\n> > + container = 'controls'\n> >\n> > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> >\n> > - enum = ctrl.get('enum')\n> > - if not enum:\n> > - continue\n> > + enum = ctrl.get('enum')\n> > + if not enum:\n> > + continue\n> >\n> > - cpp_enum = name + 'Enum'\n> > + cpp_enum = name + 'Enum'\n> >\n> > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> >\n> > - if mode == 'controls':\n> > - # Adjustments for controls\n> > - if name == 'LensShadingMapMode':\n> > - prefix = 'LensShadingMapMode'\n> > + if mode == 'controls':\n> > + # Adjustments for controls\n> > + if name == 'LensShadingMapMode':\n> > + prefix = 'LensShadingMapMode'\n> > + else:\n> > + prefix = find_common_prefix([e['name'] for e in enum])\n> > else:\n> > + # Adjustments for properties\n> > prefix = find_common_prefix([e['name'] for e in enum])\n> > - else:\n> > - # Adjustments for properties\n> > - prefix = find_common_prefix([e['name'] for e in enum])\n> >\n> > - for entry in enum:\n> > - cpp_enum = entry['name']\n> > - py_enum = entry['name'][len(prefix):]\n> > + for entry in enum:\n> > + cpp_enum = entry['name']\n> > + py_enum = entry['name'][len(prefix):]\n> >\n> > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> >\n> > - out += '\\t;\\n\\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> This part \"looks\" ok to me, but I'm not familiar with this code, so\n> just \"ack\"\n> \n> >\n> >\n> > def fill_template(template, data):\n> > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > def main(argv):\n> > # Parse command line arguments\n> > parser = argparse.ArgumentParser()\n> > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > + help='Mode is either \"controls\" or \"properties\"')\n> > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > help='Output file name. Defaults to standard output if not specified.')\n> > parser.add_argument('input', type=str,\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,\n> > - help='Mode is either \"controls\" or \"properties\"')\n> > args = parser.parse_args(argv[1:])\n> >\n> > if args.mode not in ['controls', 'properties']:\n> > @@ -90,7 +104,10 @@ def main(argv):\n> > return -1\n> >\n> > data = open(args.input, 'rb').read()\n> > - controls = yaml.safe_load(data)['controls']\n> > +\n> > + controls = {}\n> > + vendor = yaml.safe_load(data)['vendor']\n> > + controls[vendor] = yaml.safe_load(data)['controls']\n> >\n> > data = generate_py(controls, args.mode)\n> >\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_(m, \"controls\");\n> > \tauto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > \tauto draft = py::class_(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..c1172cb26e7b 100755\n> > --- a/utils/gen-controls.py\n> > +++ b/utils/gen-controls.py\n> > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> >\n> >\n> > class Control(object):\n> > - def __init__(self, name, data):\n> > + def __init__(self, name, data, vendor):\n> > self.__name = name\n> > self.__data = data\n> > self.__enum_values = None\n> > self.__size = None\n> > + self.__vendor = vendor\n> >\n> > enum_values = data.get('enum')\n> > if enum_values is not None:\n> > @@ -89,6 +90,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.__vendor\n> > +\n> > @property\n> > def name(self):\n> > \"\"\"The control name (CamelCase)\"\"\"\n> > @@ -145,15 +151,18 @@ ${description}\n> > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> >\n> > - ctrls_doc = []\n> > - ctrls_def = []\n> > - draft_ctrls_doc = []\n> > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > + if vendor not in ctrls_doc:\n> > + ctrls_doc[vendor] = []\n> > + ctrls_def[vendor] = []\n> > +\n> > info = {\n> > 'name': ctrl.name,\n> > 'type': ctrl.type,\n> > @@ -161,11 +170,8 @@ ${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> >\n> > if ctrl.is_enum:\n> > enum_doc = []\n> > @@ -203,39 +209,68 @@ ${description}\n> >\n> > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> >\n> > + vendor_ctrl_doc_sub = []\n> > + vendor_ctrl_template = string.Template('''\n> > +/**\n> > + * \\\\brief Namespace for ${vendor} controls\n> > + */\n> > +namespace ${vendor} {\n> > +\n> > +${vendor_controls_str}\n> > +\n> > +} /* namespace ${vendor} */''')\n> > +\n> > + for vendor in [v for v in ctrls_doc.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_def.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> \n> It would have been nicer if 'libcamera', 'draft' and vendor controls\n> would be handled through the same mechanism, but this would probably\n> require changing how the .cpp.in file is structured and to assign a\n> dedicated ::core namespace to libcamera controls which would make\n> handling them more verbose than necessary.\n\nI'm tempted by \"controls::core::\" actually. The code change can happen\nlater, but let's discuss it already: any opinion from anyone ?\n\n> I think this is fine even if a bit convoluted\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_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),\n> > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n> > +\n> > +enum {\n> > +${vendor_enums}\n> > +};\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> Same comment here as per the cpp file. I guess it's fine!\n> \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> > @@ -278,22 +336,26 @@ def main(argv):\n> >\n> > # Parse command line arguments\n> > parser = argparse.ArgumentParser()\n> > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > + help='Mode of operation')\n> > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > help='Output file name. Defaults to standard output if not specified.')\n> > parser.add_argument('input', type=str,\n> > help='Input file name.')\n> > parser.add_argument('template', type=str,\n> > help='Template file name.')\n> > +\n> > args = parser.parse_args(argv[1:])\n> >\n> > data = open(args.input, 'rb').read()\n> > + vendor = yaml.safe_load(data)['vendor']\n> > controls = yaml.safe_load(data)['controls']\n> > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> >\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> Minors and the 'ack I trust you' on the py part, this looks good to\n> me! Thanks\n> \n> Reviewed-by: Jacopo Mondi ","headers":{"Return-Path":"","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 F2E04C31E9\n\tfor ;\n\tThu, 30 Nov 2023 08:58:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C8F5629AF;\n\tThu, 30 Nov 2023 09:58:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A4A1629AF\n\tfor ;\n\tThu, 30 Nov 2023 09:58:27 +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 1BD0966F;\n\tThu, 30 Nov 2023 09:57:50 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701334709;\n\tbh=jp1GstM7ed6/VIN+7pe8FJwWJq73uWQzJJ9IhyUxcEY=;\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=NtHdX6TVOWean12MT9qEndKjNbLHpuwM1Xb5Fh1VEM5xQ0OxOwRCC5DbwIHkdLjlp\n\t+WdexqdU5UCyFVFXaS8D3G9H/7sPDv49npO/GOr1QcyCKbl2Jnf1UTF1JAd4F3exv8\n\ta5dMoogssqteOhzWOGhhItn95E/rsETrSeT3j1ghiWiV13k4wCynb+vNy1aqhBMfv3\n\tByc6Xtx9y28rq5ZJ/GjslewiF+542FKGpg8KmROGQL8hYRogt1YTJiLyLWioQii9Xt\n\tSEuHq9sXMZoUPV1cYMt1ddRxNHc4K0gp7ROECUlxXcnYQ8S/dadJcJruTsN0k/7fvo\n\tEGe6h6PjZH7bw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701334670;\n\tbh=jp1GstM7ed6/VIN+7pe8FJwWJq73uWQzJJ9IhyUxcEY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Cnb7bwqPDV3J7TCTQhqVAXAamGLmitptSMxLsRci1neaujb/0gQnpmKzVvySDyszA\n\tZCI8K9aN+Jfa3G959dIGkvr3D4gEe0qnuX0xEXDFtYSV978n+yDfejBL3Ee7KnTPyJ\n\tbCLHWQ/ZsXVmLIr/ttps7NzZMf5ztTnwYDFIgwYU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Cnb7bwqP\"; dkim-atps=neutral","Date":"Thu, 30 Nov 2023 10:58:33 +0200","To":"Jacopo Mondi ","Message-ID":"<20231130085833.GC8402@pendragon.ideasonboard.com>","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28217,"web_url":"https://patchwork.libcamera.org/comment/28217/","msgid":"","date":"2023-11-30T09:04:43","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 30 Nov 2023 at 08:58, Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> On Mon, Nov 27, 2023 at 05:43:37PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> > >\n> > > vendor: rpi\n> > > controls:\n> > > - MyExampleControl:\n> > > type: string\n> > > description: |\n> > > Test for libcamera vendor-specific controls.\n> > >\n> > > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > > applications to conditionally compile code if the specific vendor\n> > > controls are present. For the python bindings, the control is available\n> > > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > > example applies similarly to properties.\n> > >\n> > > Existing libcamera controls defined in control_ids.yaml are given the\n> > > \"libcamera\" vendor tag.\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> > > As a drive-by, sort and redefine the output command line argument in\n> > > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > > consistency.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/control_ids.h.in | 2 +\n> > > include/libcamera/meson.build | 15 ++-\n> > > include/libcamera/property_ids.h.in | 2 +\n> > > src/libcamera/control_ids.cpp.in | 4 +\n> > > src/libcamera/control_ids.yaml | 1 +\n> > > src/libcamera/meson.build | 5 +-\n> > > src/libcamera/property_ids.cpp.in | 4 +\n> > > src/libcamera/property_ids.yaml | 1 +\n> > > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > > utils/gen-controls.py | 120 +++++++++++++-----\n> > > 12 files changed, 171 insertions(+), 70 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> > > } /* namespace controls */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index a24c50d66a82..2c8c0258c95e 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> > >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> >\n> > If you add an emtpy line here already, you can avoid doing so in\n> > \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> >\n> > > #endif\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 5827d7ecef49..ff74ce1deedb 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -6,6 +6,7 @@\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> > > +vendor: libcamera\n> > > controls:\n> > > - AeEnable:\n> > > type: bool\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..0771ac5c091f 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> >\n> > Same comment as above. Now I wonder if that's intentional then :)\n> >\n> > > #endif\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > index f35563842a5a..45f3609b4236 100644\n> > > --- a/src/libcamera/property_ids.yaml\n> > > +++ b/src/libcamera/property_ids.yaml\n> > > @@ -4,6 +4,7 @@\n> > > #\n> > > %YAML 1.1\n> > > ---\n> > > +vendor: libcamera\n> > > controls:\n> > > - Location:\n> > > type: int32_t\n> > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > > index 9948c41e42b1..dfd7c179a883 100755\n> > > --- a/src/py/libcamera/gen-py-controls.py\n> > > +++ b/src/py/libcamera/gen-py-controls.py\n> > > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > > def generate_py(controls, mode):\n> > > out = ''\n> > >\n> > > - for ctrl in controls:\n> > > - name, ctrl = ctrl.popitem()\n> > > -\n> > > - if ctrl.get('draft'):\n> > > - ns = 'libcamera::{}::draft::'.format(mode)\n> > > - container = 'draft'\n> > > - else:\n> > > - ns = 'libcamera::{}::'.format(mode)\n> > > - container = 'controls'\n> > > + vendors_class_def = []\n> > > + vendor_defs = []\n> > > + vendors = []\n> > > + for vendor, ctrl_list in controls.items():\n> > > + for ctrls in ctrl_list:\n> > > + name, ctrl = ctrls.popitem()\n> > > +\n> > > + if vendor not in vendors and vendor != 'libcamera':\n> > > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > > + vendors.append(vendor)\n> > > +\n> > > + if ctrl.get('draft'):\n> > > + ns = 'libcamera::{}::draft::'.format(mode)\n> > > + container = 'draft'\n> > > + elif vendor != 'libcamera':\n> > > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > > + container = vendor\n> > > + else:\n> > > + ns = 'libcamera::{}::'.format(mode)\n> > > + container = 'controls'\n> > >\n> > > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > >\n> > > - enum = ctrl.get('enum')\n> > > - if not enum:\n> > > - continue\n> > > + enum = ctrl.get('enum')\n> > > + if not enum:\n> > > + continue\n> > >\n> > > - cpp_enum = name + 'Enum'\n> > > + cpp_enum = name + 'Enum'\n> > >\n> > > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > >\n> > > - if mode == 'controls':\n> > > - # Adjustments for controls\n> > > - if name == 'LensShadingMapMode':\n> > > - prefix = 'LensShadingMapMode'\n> > > + if mode == 'controls':\n> > > + # Adjustments for controls\n> > > + if name == 'LensShadingMapMode':\n> > > + prefix = 'LensShadingMapMode'\n> > > + else:\n> > > + prefix = find_common_prefix([e['name'] for e in enum])\n> > > else:\n> > > + # Adjustments for properties\n> > > prefix = find_common_prefix([e['name'] for e in enum])\n> > > - else:\n> > > - # Adjustments for properties\n> > > - prefix = find_common_prefix([e['name'] for e in enum])\n> > >\n> > > - for entry in enum:\n> > > - cpp_enum = entry['name']\n> > > - py_enum = entry['name'][len(prefix):]\n> > > + for entry in enum:\n> > > + cpp_enum = entry['name']\n> > > + py_enum = entry['name'][len(prefix):]\n> > >\n> > > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > >\n> > > - out += '\\t;\\n\\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> > This part \"looks\" ok to me, but I'm not familiar with this code, so\n> > just \"ack\"\n> >\n> > >\n> > >\n> > > def fill_template(template, data):\n> > > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > > def main(argv):\n> > > # Parse command line arguments\n> > > parser = argparse.ArgumentParser()\n> > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > > + help='Mode is either \"controls\" or \"properties\"')\n> > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > help='Output file name. Defaults to standard output if not specified.')\n> > > parser.add_argument('input', type=str,\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,\n> > > - help='Mode is either \"controls\" or \"properties\"')\n> > > args = parser.parse_args(argv[1:])\n> > >\n> > > if args.mode not in ['controls', 'properties']:\n> > > @@ -90,7 +104,10 @@ def main(argv):\n> > > return -1\n> > >\n> > > data = open(args.input, 'rb').read()\n> > > - controls = yaml.safe_load(data)['controls']\n> > > +\n> > > + controls = {}\n> > > + vendor = yaml.safe_load(data)['vendor']\n> > > + controls[vendor] = yaml.safe_load(data)['controls']\n> > >\n> > > data = generate_py(controls, args.mode)\n> > >\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_(m, \"controls\");\n> > > auto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > > auto draft = py::class_(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..c1172cb26e7b 100755\n> > > --- a/utils/gen-controls.py\n> > > +++ b/utils/gen-controls.py\n> > > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> > >\n> > >\n> > > class Control(object):\n> > > - def __init__(self, name, data):\n> > > + def __init__(self, name, data, vendor):\n> > > self.__name = name\n> > > self.__data = data\n> > > self.__enum_values = None\n> > > self.__size = None\n> > > + self.__vendor = vendor\n> > >\n> > > enum_values = data.get('enum')\n> > > if enum_values is not None:\n> > > @@ -89,6 +90,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.__vendor\n> > > +\n> > > @property\n> > > def name(self):\n> > > \"\"\"The control name (CamelCase)\"\"\"\n> > > @@ -145,15 +151,18 @@ ${description}\n> > > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> > >\n> > > - ctrls_doc = []\n> > > - ctrls_def = []\n> > > - draft_ctrls_doc = []\n> > > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > > + if vendor not in ctrls_doc:\n> > > + ctrls_doc[vendor] = []\n> > > + ctrls_def[vendor] = []\n> > > +\n> > > info = {\n> > > 'name': ctrl.name,\n> > > 'type': ctrl.type,\n> > > @@ -161,11 +170,8 @@ ${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> > >\n> > > if ctrl.is_enum:\n> > > enum_doc = []\n> > > @@ -203,39 +209,68 @@ ${description}\n> > >\n> > > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > >\n> > > + vendor_ctrl_doc_sub = []\n> > > + vendor_ctrl_template = string.Template('''\n> > > +/**\n> > > + * \\\\brief Namespace for ${vendor} controls\n> > > + */\n> > > +namespace ${vendor} {\n> > > +\n> > > +${vendor_controls_str}\n> > > +\n> > > +} /* namespace ${vendor} */''')\n> > > +\n> > > + for vendor in [v for v in ctrls_doc.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_def.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> >\n> > It would have been nicer if 'libcamera', 'draft' and vendor controls\n> > would be handled through the same mechanism, but this would probably\n> > require changing how the .cpp.in file is structured and to assign a\n> > dedicated ::core namespace to libcamera controls which would make\n> > handling them more verbose than necessary.\n>\n> I'm tempted by \"controls::core::\" actually. The code change can happen\n> later, but let's discuss it already: any opinion from anyone ?\n\nI quite like this. It would also allow us to simplify the control\ngeneration scripts. But it does make things more verbose to type...\n\n>\n> > I think this is fine even if a bit convoluted\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_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),\n> > > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n> > > +\n> > > +enum {\n> > > +${vendor_enums}\n> > > +};\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> > Same comment here as per the cpp file. I guess it's fine!\n> >\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> > > @@ -278,22 +336,26 @@ def main(argv):\n> > >\n> > > # Parse command line arguments\n> > > parser = argparse.ArgumentParser()\n> > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > > + help='Mode of operation')\n> > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > help='Output file name. Defaults to standard output if not specified.')\n> > > parser.add_argument('input', type=str,\n> > > help='Input file name.')\n> > > parser.add_argument('template', type=str,\n> > > help='Template file name.')\n> > > +\n> > > args = parser.parse_args(argv[1:])\n> > >\n> > > data = open(args.input, 'rb').read()\n> > > + vendor = yaml.safe_load(data)['vendor']\n> > > controls = yaml.safe_load(data)['controls']\n> > > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> > >\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> > Minors and the 'ack I trust you' on the py part, this looks good to\n> > me! Thanks\n> >\n> > Reviewed-by: Jacopo Mondi \n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 025B3BDE6B\n\tfor ;\n\tThu, 30 Nov 2023 09:05:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 674A1629C2;\n\tThu, 30 Nov 2023 10:05:17 +0100 (CET)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24A3C629AF\n\tfor ;\n\tThu, 30 Nov 2023 10:05:16 +0100 (CET)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-5d3758fdd2eso1961477b3.0\n\tfor ;\n\tThu, 30 Nov 2023 01:05:16 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701335117;\n\tbh=U1Aw1iJHoq1/VyFYVZKxfcyRn4Oc+/VB6vx1so0Tev4=;\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=baTJBJhuYP2V/TfDYcVm/oS1x+mIiioVFb5sb7Wy6Cn2Tm2l+P5gJCZvFpLMwUk+I\n\tP54qu5IfzmyY+RR1FRDz4po8RfP69NHqAHbe4uksPmuq1Jo/XkdudyOLWZy2Z52ThY\n\tDeMn07qRnuen5xAMK5q7rf1lnMN8vSB30/d4J/XYfMcvqVUfnwmGPX82pjLDCOscMG\n\tvy0ykUTA9f6mZPtQXDLZUGEvCFVuRWHkRDTpgA/qGXcAhh2Blq6yHMns5G4Ai/85RQ\n\tZLFgDSO6FYGvpUqTwk5rAoPWi2vNqbSxQp9OOB0AYsY2oE5oCrwg1MxgWrENwh/i1d\n\tZRpfoxERjWrRQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1701335115; x=1701939915;\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=KBSvSw1iqshU50kBSA48MAagKm49fKey2q1AHh4Dk5c=;\n\tb=T1Z8YViZc3sy7HuXg6n02V6PcMlL84MsQkxCilMsvyRNSt9MCf7eQNOMfJDvSsRqZV\n\tBhk9N007N09LCHgMy4TrCOFhUtU8C80HGxLR4j8gNHeyANG0jlVQ/E/HntluLCEKSAvb\n\tJRaWxGtrcrRFAo2NFleoynqHyYD7Le6/RAYWyVcesPT0V9LhT8/+Yg5ULYOa7qTnl6k3\n\tKu3uHSUEqjRn8czPsTpwIoh2MIpGoZMtuRNZMVj0uuHZ+ZGWDPU5kYsaSFHQYn3Paq1q\n\tvhz0D69yXnElTimuk7bQgoE1KhuAJZVeqleQBB3sngMTVaNy0xV1WoKYTq9GT4OC6H8Q\n\tJ9QA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"T1Z8YViZ\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1701335115; x=1701939915;\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=KBSvSw1iqshU50kBSA48MAagKm49fKey2q1AHh4Dk5c=;\n\tb=K/b/hNDBQBkxt3ou0oVb2vuuwj1o2PEthv0VdeSmYNuFyc+PIHhYGPVc8R3sKq1etk\n\t60ELoT93+wLMjtDWkNkmgiFw1vUgVEjSwccTEJUuK5ipFqIMAAmpXJU+ryNvAsma9vTv\n\t1tTrbB/MV3Vn8QnGEIekrUc1uokn59gg4cawfYIwKCobUgxiIsoh0/vBz2kKjfdRAKSu\n\tPmoE8GpElFGxEJ1S6iZYvnzKETxWNW3EZxyf4WPlYQvJSt7mgfEmH5n0xfe9rR5jHTRu\n\t19qlCAt5UFTpBJI9PdrMLMauo2UTRumqcja8s23A4t13gxcn035LX7lPyoHP8cTSxABc\n\tFVww==","X-Gm-Message-State":"AOJu0YzUjKA6QjCYMkc+XaTcwag+ikB4luGI8uj0kFNLu5YbtkFME8CI\n\toJyshs8Zv7KrwpCgAEILWAjBKrEql0ZJLbxBbWiYGKWhYf4f2VsRsG0=","X-Google-Smtp-Source":"AGHT+IFmVImQn4MqF9mEwR8+Bo5tsym2YPzIFwNbs5U7DGsn0ppJCyPLssQkxAshzjBQp4B/Wd8PiGEoIQx21ZRGORk=","X-Received":"by 2002:a81:4f41:0:b0:5d3:e5:46b7 with SMTP id\n\td62-20020a814f41000000b005d300e546b7mr2303240ywb.4.1701335114400; \n\tThu, 30 Nov 2023 01:05:14 -0800 (PST)","MIME-Version":"1.0","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t\n\t<20231130085833.GC8402@pendragon.ideasonboard.com>","In-Reply-To":"<20231130085833.GC8402@pendragon.ideasonboard.com>","Date":"Thu, 30 Nov 2023 09:04:43 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28219,"web_url":"https://patchwork.libcamera.org/comment/28219/","msgid":"<20231130091007.GE8402@pendragon.ideasonboard.com>","date":"2023-11-30T09:10:07","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Nov 28, 2023 at 11:13:27AM +0100, Jacopo Mondi via libcamera-devel wrote:\n> On Tue, Nov 28, 2023 at 10:08:33AM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2023-11-27 16:43:37)\n> > > On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> > > >\n> > > > vendor: rpi\n> > > > controls:\n> > > > - MyExampleControl:\n> > > > type: string\n> > > > description: |\n> > > > Test for libcamera vendor-specific controls.\n> > > >\n> > > > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > > > applications to conditionally compile code if the specific vendor\n> > > > controls are present. For the python bindings, the control is available\n> > > > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > > > example applies similarly to properties.\n> > > >\n> > > > Existing libcamera controls defined in control_ids.yaml are given the\n> > > > \"libcamera\" vendor tag.\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> > > > As a drive-by, sort and redefine the output command line argument in\n> > > > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > > > consistency.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/control_ids.h.in | 2 +\n> > > > include/libcamera/meson.build | 15 ++-\n> > > > include/libcamera/property_ids.h.in | 2 +\n> > > > src/libcamera/control_ids.cpp.in | 4 +\n> > > > src/libcamera/control_ids.yaml | 1 +\n> > > > src/libcamera/meson.build | 5 +-\n> > > > src/libcamera/property_ids.cpp.in | 4 +\n> > > > src/libcamera/property_ids.yaml | 1 +\n> > > > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > > > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > > > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > > > utils/gen-controls.py | 120 +++++++++++++-----\n> > > > 12 files changed, 171 insertions(+), 70 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> > > > } /* namespace controls */\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > > index a24c50d66a82..2c8c0258c95e 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> > > >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> > >\n> > > If you add an emtpy line here already, you can avoid doing so in\n> > > \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> > >\n> > > > #endif\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 5827d7ecef49..ff74ce1deedb 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -6,6 +6,7 @@\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> > > > +vendor: libcamera\n> > > > controls:\n> > > > - AeEnable:\n> > > > type: bool\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..0771ac5c091f 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> > >\n> > > Same comment as above. Now I wonder if that's intentional then :)\n> > >\n> > > > #endif\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > index f35563842a5a..45f3609b4236 100644\n> > > > --- a/src/libcamera/property_ids.yaml\n> > > > +++ b/src/libcamera/property_ids.yaml\n> > > > @@ -4,6 +4,7 @@\n> > > > #\n> > > > %YAML 1.1\n> > > > ---\n> > > > +vendor: libcamera\n> > > > controls:\n> > > > - Location:\n> > > > type: int32_t\n> > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > > > index 9948c41e42b1..dfd7c179a883 100755\n> > > > --- a/src/py/libcamera/gen-py-controls.py\n> > > > +++ b/src/py/libcamera/gen-py-controls.py\n> > > > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > > > def generate_py(controls, mode):\n> > > > out = ''\n> > > >\n> > > > - for ctrl in controls:\n> > > > - name, ctrl = ctrl.popitem()\n> > > > -\n> > > > - if ctrl.get('draft'):\n> > > > - ns = 'libcamera::{}::draft::'.format(mode)\n> > > > - container = 'draft'\n> > > > - else:\n> > > > - ns = 'libcamera::{}::'.format(mode)\n> > > > - container = 'controls'\n> > > > + vendors_class_def = []\n> > > > + vendor_defs = []\n> > > > + vendors = []\n> > > > + for vendor, ctrl_list in controls.items():\n> > > > + for ctrls in ctrl_list:\n> > > > + name, ctrl = ctrls.popitem()\n> > > > +\n> > > > + if vendor not in vendors and vendor != 'libcamera':\n> > > > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > > > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > > > + vendors.append(vendor)\n> > > > +\n> > > > + if ctrl.get('draft'):\n> > > > + ns = 'libcamera::{}::draft::'.format(mode)\n> > > > + container = 'draft'\n> > > > + elif vendor != 'libcamera':\n> > > > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > > > + container = vendor\n> > > > + else:\n> > > > + ns = 'libcamera::{}::'.format(mode)\n> > > > + container = 'controls'\n> > > >\n> > > > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > >\n> > > > - enum = ctrl.get('enum')\n> > > > - if not enum:\n> > > > - continue\n> > > > + enum = ctrl.get('enum')\n> > > > + if not enum:\n> > > > + continue\n> > > >\n> > > > - cpp_enum = name + 'Enum'\n> > > > + cpp_enum = name + 'Enum'\n> > > >\n> > > > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > >\n> > > > - if mode == 'controls':\n> > > > - # Adjustments for controls\n> > > > - if name == 'LensShadingMapMode':\n> > > > - prefix = 'LensShadingMapMode'\n> > > > + if mode == 'controls':\n> > > > + # Adjustments for controls\n> > > > + if name == 'LensShadingMapMode':\n> > > > + prefix = 'LensShadingMapMode'\n> > > > + else:\n> > > > + prefix = find_common_prefix([e['name'] for e in enum])\n> > > > else:\n> > > > + # Adjustments for properties\n> > > > prefix = find_common_prefix([e['name'] for e in enum])\n> > > > - else:\n> > > > - # Adjustments for properties\n> > > > - prefix = find_common_prefix([e['name'] for e in enum])\n> > > >\n> > > > - for entry in enum:\n> > > > - cpp_enum = entry['name']\n> > > > - py_enum = entry['name'][len(prefix):]\n> > > > + for entry in enum:\n> > > > + cpp_enum = entry['name']\n> > > > + py_enum = entry['name'][len(prefix):]\n> > > >\n> > > > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > >\n> > > > - out += '\\t;\\n\\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> > > This part \"looks\" ok to me, but I'm not familiar with this code, so\n> > > just \"ack\"\n> > >\n> > > >\n> > > >\n> > > > def fill_template(template, data):\n> > > > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > > > def main(argv):\n> > > > # Parse command line arguments\n> > > > parser = argparse.ArgumentParser()\n> > > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > > > + help='Mode is either \"controls\" or \"properties\"')\n> > > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > > help='Output file name. Defaults to standard output if not specified.')\n> > > > parser.add_argument('input', type=str,\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,\n> > > > - help='Mode is either \"controls\" or \"properties\"')\n> > > > args = parser.parse_args(argv[1:])\n> > > >\n> > > > if args.mode not in ['controls', 'properties']:\n> > > > @@ -90,7 +104,10 @@ def main(argv):\n> > > > return -1\n> > > >\n> > > > data = open(args.input, 'rb').read()\n> > > > - controls = yaml.safe_load(data)['controls']\n> > > > +\n> > > > + controls = {}\n> > > > + vendor = yaml.safe_load(data)['vendor']\n> > > > + controls[vendor] = yaml.safe_load(data)['controls']\n> > > >\n> > > > data = generate_py(controls, args.mode)\n> > > >\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_(m, \"controls\");\n> > > > auto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > > > auto draft = py::class_(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..c1172cb26e7b 100755\n> > > > --- a/utils/gen-controls.py\n> > > > +++ b/utils/gen-controls.py\n> > > > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> > > >\n> > > >\n> > > > class Control(object):\n> > > > - def __init__(self, name, data):\n> > > > + def __init__(self, name, data, vendor):\n> > > > self.__name = name\n> > > > self.__data = data\n> > > > self.__enum_values = None\n> > > > self.__size = None\n> > > > + self.__vendor = vendor\n> > > >\n> > > > enum_values = data.get('enum')\n> > > > if enum_values is not None:\n> > > > @@ -89,6 +90,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.__vendor\n> > > > +\n> > > > @property\n> > > > def name(self):\n> > > > \"\"\"The control name (CamelCase)\"\"\"\n> > > > @@ -145,15 +151,18 @@ ${description}\n> > > > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > > > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> > > >\n> > > > - ctrls_doc = []\n> > > > - ctrls_def = []\n> > > > - draft_ctrls_doc = []\n> > > > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > > > + if vendor not in ctrls_doc:\n> > > > + ctrls_doc[vendor] = []\n> > > > + ctrls_def[vendor] = []\n> > > > +\n> > > > info = {\n> > > > 'name': ctrl.name,\n> > > > 'type': ctrl.type,\n> > > > @@ -161,11 +170,8 @@ ${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> > > >\n> > > > if ctrl.is_enum:\n> > > > enum_doc = []\n> > > > @@ -203,39 +209,68 @@ ${description}\n> > > >\n> > > > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > > >\n> > > > + vendor_ctrl_doc_sub = []\n> > > > + vendor_ctrl_template = string.Template('''\n> > > > +/**\n> > > > + * \\\\brief Namespace for ${vendor} controls\n> > > > + */\n> > > > +namespace ${vendor} {\n> > > > +\n> > > > +${vendor_controls_str}\n> > > > +\n> > > > +} /* namespace ${vendor} */''')\n> > > > +\n> > > > + for vendor in [v for v in ctrls_doc.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_def.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> > >\n> > > It would have been nicer if 'libcamera', 'draft' and vendor controls\n> > > would be handled through the same mechanism, but this would probably\n> > > require changing how the .cpp.in file is structured and to assign a\n> > > dedicated ::core namespace to libcamera controls which would make\n> > > handling them more verbose than necessary.\n> > >\n> > > I think this is fine even if a bit convoluted\n> >\n> > Absolutely. I don't think we should have to specify\n> > libcamera::core::brightness for instance, so stripping out (only) the\n> > core namespace is reasonable.\n\nActually, as mentioned in a separate reply in this thread, I'm tempted\nby controls::core::brightness instead of controls::brightness. I asked\nin that other mail for opinions, Kieran has already expressed his :-)\n\n> > But ... why is draft referenced here? Are draft controls not just\n> > treated like a vendor control?\n> \n> They will be broken out to a dedicated file and to a dedicated\n> namespace (like other vendor controls) in the next patches in the\n> series\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_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),\n> > > > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > > > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n> >\n> > Aha, I guess we don't want to define things like\n> > LIBCAMERA_HAS_DRAFT_VENDOR_CONTROLS so I can see why draft is still\n> > treated as a bit of a special case for now.\n> >\n> > With Jacopo's minors...\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > > +\n> > > > +enum {\n> > > > +${vendor_enums}\n> > > > +};\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> > > Same comment here as per the cpp file. I guess it's fine!\n> > >\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> > > > @@ -278,22 +336,26 @@ def main(argv):\n> > > >\n> > > > # Parse command line arguments\n> > > > parser = argparse.ArgumentParser()\n> > > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > > > + help='Mode of operation')\n> > > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > > help='Output file name. Defaults to standard output if not specified.')\n> > > > parser.add_argument('input', type=str,\n> > > > help='Input file name.')\n> > > > parser.add_argument('template', type=str,\n> > > > help='Template file name.')\n> > > > +\n> > > > args = parser.parse_args(argv[1:])\n> > > >\n> > > > data = open(args.input, 'rb').read()\n> > > > + vendor = yaml.safe_load(data)['vendor']\n> > > > controls = yaml.safe_load(data)['controls']\n> > > > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > > > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> > > >\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> > > Minors and the 'ack I trust you' on the py part, this looks good to\n> > > me! Thanks\n> > >\n> > > Reviewed-by: Jacopo Mondi ","headers":{"Return-Path":"","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 E0723C31E9\n\tfor ;\n\tThu, 30 Nov 2023 09:10:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 240C2629C3;\n\tThu, 30 Nov 2023 10:10:03 +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 4EC61629BB\n\tfor ;\n\tThu, 30 Nov 2023 10:10:01 +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 650FF6B8;\n\tThu, 30 Nov 2023 10:09:24 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701335403;\n\tbh=U2YgTQ/tXTRWvRk8MtxAqE/uOGU1CH4CfI/+BO8c2T0=;\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=OW6CKgn97sj7Dw2xh3aR8r//iy9MNlXQ4m4wRUM67UZksWp0Z+f+r3WZNo/1D7fR7\n\tyIO7ufapVB1fhIBCHJto5Fhj642FL7VnQzMmT/To/D3WjV+q4KXxwVWEMbCOQjFuhl\n\tYm/iwyovc8Ju2KthOuJseuj1ebHzb0Tx/V64yFJBKI/vNGdFK9MMwFArc9vYJRhZFL\n\teP/Gw6tHiaLi0mmcECZXe3N1nbgQWllNd+9yoT0PJObO0vur/ZXifhxK83+4vZ0H2l\n\tCklv8SSgGpgtPZefXcoqOE/HcY4P3rB9nX6Gy3czT0a6IDR85wbOYfGgxYudhy6zkG\n\tPoT2mBVFTIxLw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701335364;\n\tbh=U2YgTQ/tXTRWvRk8MtxAqE/uOGU1CH4CfI/+BO8c2T0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Fa0v6Hoh4QBPUBtvFSkaC5D4PICX11HO5tub3NdBqdgQVBhQlyTzG/YTs4mUDaqBI\n\tsdjE7kvTfDHHl1OV1nUrmj/UtwUjeSCHRLKFf7TvsdtoR1FaWrdmTAZudXWTdc0dQ2\n\tu6pLwCMLFiOxieYmW5uFx5pvtqJUTzrHCBFjTjsg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Fa0v6Hoh\"; dkim-atps=neutral","Date":"Thu, 30 Nov 2023 11:10:07 +0200","To":"Jacopo Mondi ","Message-ID":"<20231130091007.GE8402@pendragon.ideasonboard.com>","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t\n\t<170116611323.630990.17283607147485135126@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28220,"web_url":"https://patchwork.libcamera.org/comment/28220/","msgid":"<20231130093230.GF8402@pendragon.ideasonboard.com>","date":"2023-11-30T09:32:30","subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 30, 2023 at 09:04:43AM +0000, Naushir Patuck wrote:\n> On Thu, 30 Nov 2023 at 08:58, Laurent Pinchart wrote:\n> > On Mon, Nov 27, 2023 at 05:43:37PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > > On Mon, Nov 27, 2023 at 11:29:16AM +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 file, for example:\n> > > >\n> > > > vendor: rpi\n> > > > controls:\n> > > > - MyExampleControl:\n> > > > type: string\n> > > > description: |\n> > > > Test for libcamera vendor-specific controls.\n> > > >\n> > > > This will now generate a control id in the libcamera::controls::rpi\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_HAS_RPI_VENDOR_CONTROLS is also generated to allow\n> > > > applications to conditionally compile code if the specific vendor\n> > > > controls are present. For the python bindings, the control is available\n> > > > with libcamera.controls.rpi.MyExampleControl. The above controls\n> > > > example applies similarly to properties.\n> > > >\n> > > > Existing libcamera controls defined in control_ids.yaml are given the\n> > > > \"libcamera\" vendor tag.\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> > > > As a drive-by, sort and redefine the output command line argument in\n> > > > gen-controls.py and gen-py-controls.py to ('--output', '-o') for\n> > > > consistency.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/control_ids.h.in | 2 +\n> > > > include/libcamera/meson.build | 15 ++-\n> > > > include/libcamera/property_ids.h.in | 2 +\n> > > > src/libcamera/control_ids.cpp.in | 4 +\n> > > > src/libcamera/control_ids.yaml | 1 +\n> > > > src/libcamera/meson.build | 5 +-\n> > > > src/libcamera/property_ids.cpp.in | 4 +\n> > > > src/libcamera/property_ids.yaml | 1 +\n> > > > src/py/libcamera/gen-py-controls.py | 81 +++++++-----\n> > > > src/py/libcamera/py_controls_generated.cpp.in | 3 +\n> > > > .../libcamera/py_properties_generated.cpp.in | 3 +\n> > > > utils/gen-controls.py | 120 +++++++++++++-----\n> > > > 12 files changed, 171 insertions(+), 70 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> > > > } /* namespace controls */\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > > index a24c50d66a82..2c8c0258c95e 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -32,20 +32,21 @@ install_headers(libcamera_public_headers,\n> > > >\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_ids.h and property_ids.h and associated modes\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..be86548cf73f 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> > >\n> > > If you add an emtpy line here already, you can avoid doing so in\n> > > \" libcamera: controls: Use vendor tags for draft controls and properties\"\n> > >\n> > > > #endif\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 5827d7ecef49..ff74ce1deedb 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -6,6 +6,7 @@\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> > > > +vendor: libcamera\n> > > > controls:\n> > > > - AeEnable:\n> > > > type: bool\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..0771ac5c091f 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> > >\n> > > Same comment as above. Now I wonder if that's intentional then :)\n> > >\n> > > > #endif\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > index f35563842a5a..45f3609b4236 100644\n> > > > --- a/src/libcamera/property_ids.yaml\n> > > > +++ b/src/libcamera/property_ids.yaml\n> > > > @@ -4,6 +4,7 @@\n> > > > #\n> > > > %YAML 1.1\n> > > > ---\n> > > > +vendor: libcamera\n> > > > controls:\n> > > > - Location:\n> > > > type: int32_t\n> > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py\n> > > > index 9948c41e42b1..dfd7c179a883 100755\n> > > > --- a/src/py/libcamera/gen-py-controls.py\n> > > > +++ b/src/py/libcamera/gen-py-controls.py\n> > > > @@ -24,45 +24,59 @@ def find_common_prefix(strings):\n> > > > def generate_py(controls, mode):\n> > > > out = ''\n> > > >\n> > > > - for ctrl in controls:\n> > > > - name, ctrl = ctrl.popitem()\n> > > > -\n> > > > - if ctrl.get('draft'):\n> > > > - ns = 'libcamera::{}::draft::'.format(mode)\n> > > > - container = 'draft'\n> > > > - else:\n> > > > - ns = 'libcamera::{}::'.format(mode)\n> > > > - container = 'controls'\n> > > > + vendors_class_def = []\n> > > > + vendor_defs = []\n> > > > + vendors = []\n> > > > + for vendor, ctrl_list in controls.items():\n> > > > + for ctrls in ctrl_list:\n> > > > + name, ctrl = ctrls.popitem()\n> > > > +\n> > > > + if vendor not in vendors and vendor != 'libcamera':\n> > > > + vendors_class_def.append('class Py{}Controls\\n{{\\n}};\\n'.format(vendor))\n> > > > + vendor_defs.append('\\tauto {} = py::class_(controls, \\\"{}\\\");'.format(vendor, vendor, vendor))\n> > > > + vendors.append(vendor)\n> > > > +\n> > > > + if ctrl.get('draft'):\n> > > > + ns = 'libcamera::{}::draft::'.format(mode)\n> > > > + container = 'draft'\n> > > > + elif vendor != 'libcamera':\n> > > > + ns = 'libcamera::{}::{}::'.format(mode, vendor)\n> > > > + container = vendor\n> > > > + else:\n> > > > + ns = 'libcamera::{}::'.format(mode)\n> > > > + container = 'controls'\n> > > >\n> > > > - out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > > + out += f'\\t{container}.def_readonly_static(\"{name}\", static_cast(&{ns}{name}));\\n\\n'\n> > > >\n> > > > - enum = ctrl.get('enum')\n> > > > - if not enum:\n> > > > - continue\n> > > > + enum = ctrl.get('enum')\n> > > > + if not enum:\n> > > > + continue\n> > > >\n> > > > - cpp_enum = name + 'Enum'\n> > > > + cpp_enum = name + 'Enum'\n> > > >\n> > > > - out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > > + out += '\\tpy::enum_<{}{}>({}, \\\"{}\\\")\\n'.format(ns, cpp_enum, container, cpp_enum)\n> > > >\n> > > > - if mode == 'controls':\n> > > > - # Adjustments for controls\n> > > > - if name == 'LensShadingMapMode':\n> > > > - prefix = 'LensShadingMapMode'\n> > > > + if mode == 'controls':\n> > > > + # Adjustments for controls\n> > > > + if name == 'LensShadingMapMode':\n> > > > + prefix = 'LensShadingMapMode'\n> > > > + else:\n> > > > + prefix = find_common_prefix([e['name'] for e in enum])\n> > > > else:\n> > > > + # Adjustments for properties\n> > > > prefix = find_common_prefix([e['name'] for e in enum])\n> > > > - else:\n> > > > - # Adjustments for properties\n> > > > - prefix = find_common_prefix([e['name'] for e in enum])\n> > > >\n> > > > - for entry in enum:\n> > > > - cpp_enum = entry['name']\n> > > > - py_enum = entry['name'][len(prefix):]\n> > > > + for entry in enum:\n> > > > + cpp_enum = entry['name']\n> > > > + py_enum = entry['name'][len(prefix):]\n> > > >\n> > > > - out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > > + out += '\\t\\t.value(\\\"{}\\\", {}{})\\n'.format(py_enum, ns, cpp_enum)\n> > > >\n> > > > - out += '\\t;\\n\\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> > > This part \"looks\" ok to me, but I'm not familiar with this code, so\n> > > just \"ack\"\n> > >\n> > > >\n> > > >\n> > > > def fill_template(template, data):\n> > > > @@ -75,14 +89,14 @@ def fill_template(template, data):\n> > > > def main(argv):\n> > > > # Parse command line arguments\n> > > > parser = argparse.ArgumentParser()\n> > > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > > + parser.add_argument('--mode', '-m', type=str, required=True,\n> > > > + help='Mode is either \"controls\" or \"properties\"')\n> > > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > > help='Output file name. Defaults to standard output if not specified.')\n> > > > parser.add_argument('input', type=str,\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,\n> > > > - help='Mode is either \"controls\" or \"properties\"')\n> > > > args = parser.parse_args(argv[1:])\n> > > >\n> > > > if args.mode not in ['controls', 'properties']:\n> > > > @@ -90,7 +104,10 @@ def main(argv):\n> > > > return -1\n> > > >\n> > > > data = open(args.input, 'rb').read()\n> > > > - controls = yaml.safe_load(data)['controls']\n> > > > +\n> > > > + controls = {}\n> > > > + vendor = yaml.safe_load(data)['vendor']\n> > > > + controls[vendor] = yaml.safe_load(data)['controls']\n> > > >\n> > > > data = generate_py(controls, args.mode)\n> > > >\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_(m, \"controls\");\n> > > > auto draft = py::class_(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..f7b5ec8c635d 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_(m, \"properties\");\n> > > > auto draft = py::class_(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..c1172cb26e7b 100755\n> > > > --- a/utils/gen-controls.py\n> > > > +++ b/utils/gen-controls.py\n> > > > @@ -35,11 +35,12 @@ class ControlEnum(object):\n> > > >\n> > > >\n> > > > class Control(object):\n> > > > - def __init__(self, name, data):\n> > > > + def __init__(self, name, data, vendor):\n> > > > self.__name = name\n> > > > self.__data = data\n> > > > self.__enum_values = None\n> > > > self.__size = None\n> > > > + self.__vendor = vendor\n> > > >\n> > > > enum_values = data.get('enum')\n> > > > if enum_values is not None:\n> > > > @@ -89,6 +90,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.__vendor\n> > > > +\n> > > > @property\n> > > > def name(self):\n> > > > \"\"\"The control name (CamelCase)\"\"\"\n> > > > @@ -145,15 +151,18 @@ ${description}\n> > > > enum_values_start = string.Template('''extern const std::array ${name}Values = {''')\n> > > > enum_values_values = string.Template('''\\tstatic_cast(${name}),''')\n> > > >\n> > > > - ctrls_doc = []\n> > > > - ctrls_def = []\n> > > > - draft_ctrls_doc = []\n> > > > - draft_ctrls_def = []\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 = 'draft' if ctrl.is_draft else ctrl.vendor\n> > > > + if vendor not in ctrls_doc:\n> > > > + ctrls_doc[vendor] = []\n> > > > + ctrls_def[vendor] = []\n> > > > +\n> > > > info = {\n> > > > 'name': ctrl.name,\n> > > > 'type': ctrl.type,\n> > > > @@ -161,11 +170,8 @@ ${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> > > >\n> > > > if ctrl.is_enum:\n> > > > enum_doc = []\n> > > > @@ -203,39 +209,68 @@ ${description}\n> > > >\n> > > > ctrls_map.append('\\t{ ' + id_name + ', &' + ctrl.q_name + ' },')\n> > > >\n> > > > + vendor_ctrl_doc_sub = []\n> > > > + vendor_ctrl_template = string.Template('''\n> > > > +/**\n> > > > + * \\\\brief Namespace for ${vendor} controls\n> > > > + */\n> > > > +namespace ${vendor} {\n> > > > +\n> > > > +${vendor_controls_str}\n> > > > +\n> > > > +} /* namespace ${vendor} */''')\n> > > > +\n> > > > + for vendor in [v for v in ctrls_doc.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_def.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> > >\n> > > It would have been nicer if 'libcamera', 'draft' and vendor controls\n> > > would be handled through the same mechanism, but this would probably\n> > > require changing how the .cpp.in file is structured and to assign a\n> > > dedicated ::core namespace to libcamera controls which would make\n> > > handling them more verbose than necessary.\n> >\n> > I'm tempted by \"controls::core::\" actually. The code change can happen\n> > later, but let's discuss it already: any opinion from anyone ?\n> \n> I quite like this. It would also allow us to simplify the control\n> generation scripts. But it does make things more verbose to type...\n\nExactly my thoughts :-)\n\n> > > I think this is fine even if a bit convoluted\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_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),\n> > > > + 'vendor_controls_doc': '\\n'.join(vendor_ctrl_doc_sub),\n> > > > + 'vendor_controls_def': '\\n'.join(vendor_ctrl_def_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 ${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 = 'draft' if ctrl.is_draft else ctrl.vendor\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 +292,35 @@ 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_HAS_${vendor_def}_VENDOR_${mode}\n> > > > +\n> > > > +enum {\n> > > > +${vendor_enums}\n> > > > +};\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> > > Same comment here as per the cpp file. I guess it's fine!\n> > >\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> > > > @@ -278,22 +336,26 @@ def main(argv):\n> > > >\n> > > > # Parse command line arguments\n> > > > parser = argparse.ArgumentParser()\n> > > > - parser.add_argument('-o', dest='output', metavar='file', type=str,\n> > > > + parser.add_argument('--mode', '-m', type=str, required=True, choices=['controls', 'properties'],\n> > > > + help='Mode of operation')\n> > > > + parser.add_argument('--output', '-o', metavar='file', type=str,\n> > > > help='Output file name. Defaults to standard output if not specified.')\n> > > > parser.add_argument('input', type=str,\n> > > > help='Input file name.')\n> > > > parser.add_argument('template', type=str,\n> > > > help='Template file name.')\n> > > > +\n> > > > args = parser.parse_args(argv[1:])\n> > > >\n> > > > data = open(args.input, 'rb').read()\n> > > > + vendor = yaml.safe_load(data)['vendor']\n> > > > controls = yaml.safe_load(data)['controls']\n> > > > - controls = [Control(*ctrl.popitem()) for ctrl in controls]\n> > > > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls]\n> > > >\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> > > Minors and the 'ack I trust you' on the py part, this looks good to\n> > > me! Thanks\n> > >\n> > > Reviewed-by: Jacopo Mondi ","headers":{"Return-Path":"","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 67C36BDE6B\n\tfor ;\n\tThu, 30 Nov 2023 09:32:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7F35629C2;\n\tThu, 30 Nov 2023 10:32:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDB21629AF\n\tfor ;\n\tThu, 30 Nov 2023 10:32:24 +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 CC4BA8C1;\n\tThu, 30 Nov 2023 10:31:47 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701336745;\n\tbh=SEwR0mWv3H/uAFWr4xPPuPo6VyN7algs8zyM85/PMe4=;\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=iTyHOGThlbhVGt/P1eM1umIcTiFFkWyhhoVWj7I0Z/TkofD8q6r6LJdmlIdgO4S1t\n\tHBe7nukCucSbRz5tWJasUV2g+iGmbr2Cqfq1oVjxIPx0Krgk6JtsuzkWaOAHWw1+KU\n\tyFbF3hxYYvL6rvGVkWtO5EdlTX78bPnTx5k0QYIofXTGssjQ4FLtudtnLBkvH5zGr4\n\ti4INoev4KV/7UrCSESxvyjsU/eutkzR0ZR7qduEC+b5sKFBgvd3RYXfZy8f7G8B2zb\n\tEl61cpkqgJqy4rvXd747mzRo6+uy5nd2p2pzS3BWMtWSSfTE9/6T4DxAixZPoguhQs\n\tpfOp3NllXKsMA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1701336708;\n\tbh=SEwR0mWv3H/uAFWr4xPPuPo6VyN7algs8zyM85/PMe4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T+hhdpz2atPg66DwCW1MpBenKEwF6Qiqi6ETETOimMzqFMX5Df0Lt4RRnkEgN68B2\n\tBXOyfVQ1kOlm/MRszyB4arfNhQ2QLKMh5eFuHzIioPSXFNw9Ctsp5p9KaPnGL+7P52\n\t6Onp/fzT3QJ4xgm2r7l1EEM5gHwLeSKrYtPRcSR4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T+hhdpz2\"; dkim-atps=neutral","Date":"Thu, 30 Nov 2023 11:32:30 +0200","To":"Naushir Patuck ","Message-ID":"<20231130093230.GF8402@pendragon.ideasonboard.com>","References":"<20231124123713.22519-2-naush@raspberrypi.com>\n\t<20231127112916.26364-1-naush@raspberrypi.com>\n\t\n\t<20231130085833.GC8402@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] controls: Add vendor control/property\n\tsupport to generation scripts","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]