[{"id":32374,"web_url":"https://patchwork.libcamera.org/comment/32374/","msgid":"<20241125212655.GU19381@pendragon.ideasonboard.com>","date":"2024-11-25T21:26:55","subject":"Re: [PATCH 0/4] Add direction field to ControlId","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patches.\n\nOn Tue, Nov 26, 2024 at 12:29:59AM +0900, Paul Elder wrote:\n> This patch series add support for querying the ControlId for the\n> direction that it can be passed.\n> \n> This used to only be mentioned in the control id definitions as \"This\n> control can only be returned in metadata\" so this codifies it and allows\n> this information to be queried by applications.\n> \n> This is an ABI breaking change, so I really want to sneak it in before\n> the 0.4.0 release that's coming imminently...\n> \n> Patches 1 and 2 prepare control definitions and parsing, while patch 3\n> adds the actual support. Patch 4 enables visualization via cam.\n\nHere's a simplification for the use of the direction flags:\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex cd338ac0d653..9af903733439 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -245,7 +245,7 @@ public:\n \n \tControlId(unsigned int id, const std::string &name, const std::string &vendor,\n \t\t ControlType type, std::size_t size = 0,\n-\t\t const DirectionFlags &direction =\n+\t\t DirectionFlags direction =\n \t\t\t static_cast(Direction::In) |\n \t\t\t static_cast(Direction::Out),\n \t\t const std::map &enumStrMap = {});\n@@ -258,13 +258,11 @@ public:\n \tstd::size_t size() const { return size_; }\n \tbool isInput() const\n \t{\n-\t\treturn static_cast(\n-\t\t\tdirection_ & static_cast(Direction::In));\n+\t\treturn !!(direction_ & Direction::In);\n \t}\n \tbool isOutput() const\n \t{\n-\t\treturn static_cast(\n-\t\t\tdirection_ & static_cast(Direction::Out));\n+\t\treturn !!(direction_ & Direction::Out);\n \t}\n \tconst std::map &enumerators() const { return reverseMap_; }\n \n@@ -281,6 +279,8 @@ private:\n \tstd::map reverseMap_;\n };\n \n+LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n+\n static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n {\n \treturn lhs == rhs.id();\n@@ -308,9 +308,8 @@ public:\n \tusing type = T;\n \n \tControl(unsigned int id, const char *name, const char *vendor,\n-\t\tconst ControlId::DirectionFlags &direction =\n-\t\t\tstatic_cast(ControlId::Direction::In) |\n-\t\t\tstatic_cast(ControlId::Direction::Out),\n+\t\tControlId::DirectionFlags direction = ControlId::Direction::In\n+\t\t\t\t\t\t | ControlId::Direction::Out,\n \t\tconst std::map &enumStrMap = {})\n \t\t: ControlId(id, name, vendor, details::control_type>::value,\n \t\t\t details::control_type>::size, direction, enumStrMap)\ndiff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\nindex 30eb17e7f064..1f78de1aaaed 100644\n--- a/src/libcamera/controls.cpp\n+++ b/src/libcamera/controls.cpp\n@@ -402,7 +402,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n */\n ControlId::ControlId(unsigned int id, const std::string &name,\n \t\t const std::string &vendor, ControlType type,\n-\t\t std::size_t size, const DirectionFlags &direction,\n+\t\t std::size_t size, DirectionFlags direction,\n \t\t const std::map &enumStrMap)\n \t: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n \t direction_(direction), enumStrMap_(enumStrMap)\ndiff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\nindex bc1c655f1b9b..2fd20ec674f4 100644\n--- a/utils/codegen/controls.py\n+++ b/utils/codegen/controls.py\n@@ -122,8 +122,8 @@ class Control(object):\n \n @property\n def direction(self):\n- in_flag = 'static_cast(ControlId::Direction::In)'\n- out_flag = 'static_cast(ControlId::Direction::Out)'\n+ in_flag = 'ControlId::Direction::In'\n+ out_flag = 'ControlId::Direction::Out'\n \n if self.__direction == 'inout':\n return f'{in_flag} | {out_flag}'\n\n\nUnfortunately the static_cast in the ControlId constructor have to stay.\nAn alternative would be to require the direction argument, which may not\nbe a bad idea given that all controls should have a direction.\n\n> Paul Elder (4):\n> libcamera: controls: Populate direction field in control definitions\n> utils: codegen: controls.py: Parse direction information\n> libcamera: controls: Add support for querying direction information\n> apps: cam: Print control direction information\n> \n> include/libcamera/controls.h | 27 +++++++++++++++++-\n> src/apps/cam/camera_session.cpp | 10 +++++--\n> src/libcamera/control_ids.cpp.in | 4 +--\n> src/libcamera/control_ids_core.yaml | 12 ++++++++\n> src/libcamera/control_ids_draft.yaml | 7 +++++\n> src/libcamera/controls.cpp | 42 ++++++++++++++++++++++++++--\n> utils/codegen/controls.py | 21 ++++++++++++++\n> 7 files changed, 116 insertions(+), 7 deletions(-)","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 5ABC1C3274\n\tfor ;\n\tMon, 25 Nov 2024 21:27:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67CC16604A;\n\tMon, 25 Nov 2024 22:27:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 38C6D65F7E\n\tfor ;\n\tMon, 25 Nov 2024 22:27:05 +0100 (CET)","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 1D7254AD;\n\tMon, 25 Nov 2024 22:26:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oyBThQc0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732570003;\n\tbh=/LGFZD9DrXbQPPgMbCk6+XjlLGu3S2pYxGHz0uDmtSQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oyBThQc05sIuevR0jybRdqBdPaNGRWwE0Y9nXrbBkGOM+JlEDEF2kk9QUtVJ8G6Yb\n\thHxi3ugaTgMZq4AeIl7MQTSexpZzkdBCVIvK2WJrGlE7igVOTKNKVehMG2N7LGm4hH\n\tzYeQ0VREu4EFvFIdHWzqZZkFHbrYWF2dfc6EdrWc=","Date":"Mon, 25 Nov 2024 23:26:55 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 0/4] Add direction field to ControlId","Message-ID":"<20241125212655.GU19381@pendragon.ideasonboard.com>","References":"<20241125153003.3309066-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241125153003.3309066-1-paul.elder@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\" "}}]