[{"id":31575,"web_url":"https://patchwork.libcamera.org/comment/31575/","msgid":"<20241003202643.GD32537@pendragon.ideasonboard.com>","date":"2024-10-03T20:26:43","subject":"Re: [PATCH v1 3/8] utils: Add script to generate\n\tcontrol_ids_debug.yaml","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 06:19:21PM +0200, Stefan Klug wrote:\n> For flexible debugging it is helpful to minimize the roundtrip time.\n> This scipts parses the sourcetree and loos for usages of\n\ns/scipts/script/\ns/loos/looks/\n\n> \n> set(controls::debug::Something, ...)\n> \n> and adds (or removes) the controls as necessary from the yaml\n> description.\n\nIt's worth mentioning it (and in the script itself too) that this is\nmeant as a development tool, to ease updating the yaml file when making\nchanges to IPA modules.\n\n> \n> Signed-off-by: Stefan Klug \n> ---\n> utils/gen-debug-controls.py | 161 ++++++++++++++++++++++++++++++++++++\n> 1 file changed, 161 insertions(+)\n> create mode 100755 utils/gen-debug-controls.py\n> \n> diff --git a/utils/gen-debug-controls.py b/utils/gen-debug-controls.py\n> new file mode 100755\n> index 000000000000..6cb087e1e0ae\n> --- /dev/null\n> +++ b/utils/gen-debug-controls.py\n> @@ -0,0 +1,161 @@\n> +#!/usr/bin/env python3\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +# Copyright (C) 2024, Ideas on Board Inc.\n> +#\n> +# Author: Stefan Klug \n> +#\n> +# This script looks for occurrences of the debug metadata controls in the source\n> +# tree and updates src/libcamera/control_ids_debug.yaml accordingly\n> +\n> +import argparse\n> +import logging\n> +import os\n> +import re\n> +import sys\n> +from dataclasses import dataclass\n> +from pathlib import Path\n> +\n> +logger = logging.getLogger(__name__)\n> +logging.basicConfig(level=logging.INFO, format='%(levelname)s: %(message)s')\n> +\n> +try:\n> + import ruyaml\n> +except:\n> + logger.error(\n> + f'Failed to import ruyaml. Please install the ruyaml package.')\n\nThis is only available through pip and not packaged by Debian. Please\nuse the yaml module instead, I'd like to avoid adding another\ndependency.\n\nI suspect you may tell me that pyyaml doesn't preserve comments while\nruyaml does. Could we at least use ruamel then, which is packaged by\nDebian ? Or reconsider the need to preserve comments ? This tool is\nmeant to assist developers, so I think we can do a best effort and\nassume usage of 'git add -p' for the time being.\n\n> + sys.exit(1)\n> +\n> +\n> +@dataclass\n> +class FoundMatch:\n> + file: os.PathLike\n> + whole_match: str\n> + line: int\n> + type: str\n> + name: str\n> + size: str = None\n> +\n> +\n> +def get_control_name(control):\n> + k = list(control.keys())\n> + if len(k) != 1:\n> + raise Exception(f\"Can't handle control entry with {len(k)} keys\")\n> + return k[0]\n> +\n> +\n> +def find_debug_controls(dir):\n> + extensions = ['.cpp', '.h']\n> + files = [p for p in dir.rglob('*') if p.suffix in extensions]\n> +\n> + # The following regex was tested on\n> + # set>( controls::debug::something , static_cast(var) )\n> + # set<>( controls::debug::something , static_cast(var) )\n> + # set( controls::debug::something , static_cast (var) )\n> + exp = re.compile(r'set' # set function\n> + # possibly followed by template param\n> + r'(?:\\<((?:[^)(])*)\\>)?'\n> + # referencing a debug control\n> + r'\\(\\s*controls::debug::(\\w+)\\s*,.*\\)'\n> + )\n> + matches = []\n> + for p in files:\n> + with p.open('r') as f:\n> + for idx, line in enumerate(f):\n> + match = exp.search(line)\n> + if match:\n> + m = FoundMatch(file=p, line=idx, type=match.group(1),\n> + name=match.group(2), whole_match=match.group(0))\n> + if m.type is not None and m.type.startswith('Span'):\n> + # simple span type detection treating the last word inside <> as type\n> + r = re.match(r'Span<(?:.*\\s+)(.*)>', m.type)\n> + m.type = r.group(1)\n> + m.size = '[n]'\n> + matches.append(m)\n> + return matches\n> +\n> +\n> +def main(argv):\n> + parser = argparse.ArgumentParser(\n> + description='Automatically updates control_ids_debug.yaml',)\n\nI think you can drop the trailing comma.\n\n> + args = parser.parse_args(argv[1:])\n\nargs seems unused. I assume this is meant to print a help text. You can\ndrop the args variable and just call parse_args() then.\n\n> +\n> + yaml = ruyaml.YAML()\n> + root_dir = Path(__file__).resolve().parent.parent\n> + output = root_dir.joinpath('src/libcamera/control_ids_debug.yaml')\n\nHmmmm... I suppose there's no real use case for specifying the file as a\ncomment line argument.\n\nI would rename 'output' as it's both an input and an output file. Maybe\nctrl_file or something similar.\n\n> +\n> + matches = find_debug_controls(root_dir.joinpath('src'))\n> +\n> + doc = yaml.load(output)\n> +\n> + controls = doc['controls']\n> +\n> + # create a map of names in the existing yaml for easier updating\n\n # Create a map of names in the existing yaml for easier updating.\n\n> + controls_map = {}\n> + for control in controls:\n> + for k, v in control.items():\n> + controls_map[k] = v\n> +\n> + obsolete_names = list(controls_map.keys())\n> +\n> + for m in matches:\n> + if not m.type:\n> + p = m.file.relative_to(Path.cwd(), walk_up=True)\n> + logger.warning(\n> + f'{p}:{m.line + 1}: Failed to deduce type from {m.whole_match} ... skipping')\n> + continue\n> +\n> + p = m.file.relative_to(root_dir)\n> + desc = {'type': m.type,\n> + 'description': f'Debug control {m.name} found in {p}:{m.line}'}\n> + if m.size is not None:\n> + desc['size'] = m.size\n> +\n> + if m.name in controls_map:\n> + # Can't use == for modified check because of the special yaml dicts.\n> + update_needed = False\n> + if list(controls_map[m.name].keys()) != list(desc.keys()):\n> + update_needed = True\n> + else:\n> + for k, v in controls_map[m.name].items():\n> + if v != desc[k]:\n> + update_needed = True\n> + break\n> +\n> + if update_needed:\n> + logger.info(f\"Update control '{m.name}'\")\n> + controls_map[m.name].clear()\n> + controls_map[m.name].update(desc)\n> +\n> + obsolete_names.remove(m.name)\n> + else:\n> + logger.info(f\"Add control '{m.name}'\")\n> + insert_before = len(controls)\n> + for idx, control in enumerate(controls):\n> + if get_control_name(control).lower() > m.name.lower():\n> + insert_before = idx\n> + break\n> + controls.insert(insert_before, {m.name: desc})\n> +\n> + # Remove elements from controls without recreating the list (to keep comments etc.)\n\n # Remove elements from controls without recreating the list (to keep\n # comments etc.).\n\n> + idx = 0\n> + while idx < len(controls):\n> + name = get_control_name(controls[idx])\n> + if name in obsolete_names:\n> + logger.info(f\"Remove control '{name}'\")\n> + controls.pop(idx)\n> + else:\n> + idx += 1\n> +\n> + with output.open('w') as f:\n> + # ruyaml looses the copyright header\n> + f.write((\"# SPDX-License-Identifier: LGPL-2.1-or-later\\n\"\n> + \"#\\n\"\n> + \"# Copyright (C) 2024, Ideas on Board Oy\\n\"\n\nMaybe print the current year (or \"2024-current year\") to be more\nfuture-proof ?\n\n> + \"#\\n\"))\n> + yaml.dump(doc, f)\n> +\n> + return 0\n> +\n> +\n> +if __name__ == '__main__':\n> + sys.exit(main(sys.argv))","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 DF2C6BD80A\n\tfor ;\n\tThu, 3 Oct 2024 20:26:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8EAA6351F;\n\tThu, 3 Oct 2024 22:26:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3B8E60534\n\tfor ;\n\tThu, 3 Oct 2024 22:26:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 516E3593;\n\tThu, 3 Oct 2024 22:25:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hsqj7C8d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727987114;\n\tbh=6XV/82DTSkf/vJblRBOv1A9+Whgb05TDHmHaeBeY8qA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hsqj7C8dDgjrk2tE3r3He3FAFS73cFZcbOf/SV0rcFWaqx2IwlRODWfj+q+dkEE3j\n\tPEy1/FJSedGatMmYW9hINd1f/AOxCVrfYTVwDD4pmzmHY4fE8sNMykTNPBAVVVVsC7\n\tMPz9s7CWAxKbYnFHK262v4k99/qg8vpFdcbF0G5A=","Date":"Thu, 3 Oct 2024 23:26:43 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/8] utils: Add script to generate\n\tcontrol_ids_debug.yaml","Message-ID":"<20241003202643.GD32537@pendragon.ideasonboard.com>","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-4-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241002161933.247091-4-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31576,"web_url":"https://patchwork.libcamera.org/comment/31576/","msgid":"","date":"2024-10-03T20:50:02","subject":"Re: [PATCH v1 3/8] utils: Add script to generate\n\tcontrol_ids_debug.yaml","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Thu, Oct 03, 2024 at 11:26:43PM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 02, 2024 at 06:19:21PM +0200, Stefan Klug wrote:\n> > For flexible debugging it is helpful to minimize the roundtrip time.\n> > This scipts parses the sourcetree and loos for usages of\n> \n> s/scipts/script/\n> s/loos/looks/\n> \n> > \n> > set(controls::debug::Something, ...)\n> > \n> > and adds (or removes) the controls as necessary from the yaml\n> > description.\n> \n> It's worth mentioning it (and in the script itself too) that this is\n> meant as a development tool, to ease updating the yaml file when making\n> changes to IPA modules.\n> \n\nAck.\n\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > utils/gen-debug-controls.py | 161 ++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 161 insertions(+)\n> > create mode 100755 utils/gen-debug-controls.py\n> > \n> > diff --git a/utils/gen-debug-controls.py b/utils/gen-debug-controls.py\n> > new file mode 100755\n> > index 000000000000..6cb087e1e0ae\n> > --- /dev/null\n> > +++ b/utils/gen-debug-controls.py\n> > @@ -0,0 +1,161 @@\n> > +#!/usr/bin/env python3\n> > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > +# Copyright (C) 2024, Ideas on Board Inc.\n> > +#\n> > +# Author: Stefan Klug \n> > +#\n> > +# This script looks for occurrences of the debug metadata controls in the source\n> > +# tree and updates src/libcamera/control_ids_debug.yaml accordingly\n> > +\n> > +import argparse\n> > +import logging\n> > +import os\n> > +import re\n> > +import sys\n> > +from dataclasses import dataclass\n> > +from pathlib import Path\n> > +\n> > +logger = logging.getLogger(__name__)\n> > +logging.basicConfig(level=logging.INFO, format='%(levelname)s: %(message)s')\n> > +\n> > +try:\n> > + import ruyaml\n> > +except:\n> > + logger.error(\n> > + f'Failed to import ruyaml. Please install the ruyaml package.')\n> \n> This is only available through pip and not packaged by Debian. Please\n> use the yaml module instead, I'd like to avoid adding another\n> dependency.\n> \n\nIn ubuntu it is available as python3-ruyaml. ruamel seems quite\nunmaintained. I could support both. Or go back to yaml (see below).\n\n> I suspect you may tell me that pyyaml doesn't preserve comments while\n> ruyaml does. Could we at least use ruamel then, which is packaged by\n> Debian ? Or reconsider the need to preserve comments ? This tool is\n> meant to assist developers, so I think we can do a best effort and\n> assume usage of 'git add -p' for the time being.\n\nYes, the preserved comments were the main reason. But ruyaml also has\nline breaks that differ from the libcamera style. So maybe it doesn't\nmatter in the end. I'll test with yaml and see how it looks.\n\n> \n> > + sys.exit(1)\n> > +\n> > +\n> > +@dataclass\n> > +class FoundMatch:\n> > + file: os.PathLike\n> > + whole_match: str\n> > + line: int\n> > + type: str\n> > + name: str\n> > + size: str = None\n> > +\n> > +\n> > +def get_control_name(control):\n> > + k = list(control.keys())\n> > + if len(k) != 1:\n> > + raise Exception(f\"Can't handle control entry with {len(k)} keys\")\n> > + return k[0]\n> > +\n> > +\n> > +def find_debug_controls(dir):\n> > + extensions = ['.cpp', '.h']\n> > + files = [p for p in dir.rglob('*') if p.suffix in extensions]\n> > +\n> > + # The following regex was tested on\n> > + # set>( controls::debug::something , static_cast(var) )\n> > + # set<>( controls::debug::something , static_cast(var) )\n> > + # set( controls::debug::something , static_cast (var) )\n> > + exp = re.compile(r'set' # set function\n> > + # possibly followed by template param\n> > + r'(?:\\<((?:[^)(])*)\\>)?'\n> > + # referencing a debug control\n> > + r'\\(\\s*controls::debug::(\\w+)\\s*,.*\\)'\n> > + )\n> > + matches = []\n> > + for p in files:\n> > + with p.open('r') as f:\n> > + for idx, line in enumerate(f):\n> > + match = exp.search(line)\n> > + if match:\n> > + m = FoundMatch(file=p, line=idx, type=match.group(1),\n> > + name=match.group(2), whole_match=match.group(0))\n> > + if m.type is not None and m.type.startswith('Span'):\n> > + # simple span type detection treating the last word inside <> as type\n> > + r = re.match(r'Span<(?:.*\\s+)(.*)>', m.type)\n> > + m.type = r.group(1)\n> > + m.size = '[n]'\n> > + matches.append(m)\n> > + return matches\n> > +\n> > +\n> > +def main(argv):\n> > + parser = argparse.ArgumentParser(\n> > + description='Automatically updates control_ids_debug.yaml',)\n> \n> I think you can drop the trailing comma.\n> \n> > + args = parser.parse_args(argv[1:])\n> \n> args seems unused. I assume this is meant to print a help text. You can\n> drop the args variable and just call parse_args() then.\n\nCopy paste leftover. I always waited for args to come back, but there\nwere none :-)\n\n> \n> > +\n> > + yaml = ruyaml.YAML()\n> > + root_dir = Path(__file__).resolve().parent.parent\n> > + output = root_dir.joinpath('src/libcamera/control_ids_debug.yaml')\n> \n> Hmmmm... I suppose there's no real use case for specifying the file as a\n> comment line argument.\n\nJep. I wanted to keep usage as easy as possible.\n\n> \n> I would rename 'output' as it's both an input and an output file. Maybe\n> ctrl_file or something similar.\n\nack\n\n> \n> > +\n> > + matches = find_debug_controls(root_dir.joinpath('src'))\n> > +\n> > + doc = yaml.load(output)\n> > +\n> > + controls = doc['controls']\n> > +\n> > + # create a map of names in the existing yaml for easier updating\n> \n> # Create a map of names in the existing yaml for easier updating.\n> \n> > + controls_map = {}\n> > + for control in controls:\n> > + for k, v in control.items():\n> > + controls_map[k] = v\n> > +\n> > + obsolete_names = list(controls_map.keys())\n> > +\n> > + for m in matches:\n> > + if not m.type:\n> > + p = m.file.relative_to(Path.cwd(), walk_up=True)\n> > + logger.warning(\n> > + f'{p}:{m.line + 1}: Failed to deduce type from {m.whole_match} ... skipping')\n> > + continue\n> > +\n> > + p = m.file.relative_to(root_dir)\n> > + desc = {'type': m.type,\n> > + 'description': f'Debug control {m.name} found in {p}:{m.line}'}\n> > + if m.size is not None:\n> > + desc['size'] = m.size\n> > +\n> > + if m.name in controls_map:\n> > + # Can't use == for modified check because of the special yaml dicts.\n> > + update_needed = False\n> > + if list(controls_map[m.name].keys()) != list(desc.keys()):\n> > + update_needed = True\n> > + else:\n> > + for k, v in controls_map[m.name].items():\n> > + if v != desc[k]:\n> > + update_needed = True\n> > + break\n> > +\n> > + if update_needed:\n> > + logger.info(f\"Update control '{m.name}'\")\n> > + controls_map[m.name].clear()\n> > + controls_map[m.name].update(desc)\n> > +\n> > + obsolete_names.remove(m.name)\n> > + else:\n> > + logger.info(f\"Add control '{m.name}'\")\n> > + insert_before = len(controls)\n> > + for idx, control in enumerate(controls):\n> > + if get_control_name(control).lower() > m.name.lower():\n> > + insert_before = idx\n> > + break\n> > + controls.insert(insert_before, {m.name: desc})\n> > +\n> > + # Remove elements from controls without recreating the list (to keep comments etc.)\n> \n> # Remove elements from controls without recreating the list (to keep\n> # comments etc.).\n> \n> > + idx = 0\n> > + while idx < len(controls):\n> > + name = get_control_name(controls[idx])\n> > + if name in obsolete_names:\n> > + logger.info(f\"Remove control '{name}'\")\n> > + controls.pop(idx)\n> > + else:\n> > + idx += 1\n> > +\n> > + with output.open('w') as f:\n> > + # ruyaml looses the copyright header\n> > + f.write((\"# SPDX-License-Identifier: LGPL-2.1-or-later\\n\"\n> > + \"#\\n\"\n> > + \"# Copyright (C) 2024, Ideas on Board Oy\\n\"\n> \n> Maybe print the current year (or \"2024-current year\") to be more\n> future-proof ?\n\nNo problem.\n\nRegards,\nStefan\n\n> \n> > + \"#\\n\"))\n> > + yaml.dump(doc, f)\n> > +\n> > + return 0\n> > +\n> > +\n> > +if __name__ == '__main__':\n> > + sys.exit(main(sys.argv))\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 1158EC3257\n\tfor ;\n\tThu, 3 Oct 2024 20:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 210EF63526;\n\tThu, 3 Oct 2024 22:50:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D184560534\n\tfor ;\n\tThu, 3 Oct 2024 22:50:05 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c4d0:8e70:cd38:b51f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4ECDF593;\n\tThu, 3 Oct 2024 22:48:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lOLiq8RV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727988512;\n\tbh=8gOXZ2fHVf1zo3ZmRg4oQxqUzXWWzXqjtJz+iFLc29M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lOLiq8RVk+bFZmKmh2gfYmcPDkgNK18qpH7HF8R9VT1DzJHlokbphBA1PsljOKc87\n\tEjhQ7FPJ8oRPBIVepd7jNBiShHThvSlPHZ5IPJZO7VLsSqPq5qyLzUOL8hzGKVTFoG\n\tvMAym24LUiyyrR89FMv6Xy/2JyCDS11sWsj7KJdU=","Date":"Thu, 3 Oct 2024 22:50:02 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/8] utils: Add script to generate\n\tcontrol_ids_debug.yaml","Message-ID":"","References":"<20241002161933.247091-1-stefan.klug@ideasonboard.com>\n\t<20241002161933.247091-4-stefan.klug@ideasonboard.com>\n\t<20241003202643.GD32537@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241003202643.GD32537@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]