[{"id":25393,"web_url":"https://patchwork.libcamera.org/comment/25393/","msgid":"<20221012073216.6dhb6fne3uhziopz@uno.localdomain>","date":"2022-10-12T07:32:16","subject":"Re: [libcamera-devel] [PATCH v5 4/9] utils: ipc: Add support for\n\tFlags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Not sure I fully get why this patch does two things (scoped enums +\nflags) , but I guess it's  fine\n\nOn Tue, Oct 11, 2022 at 07:58:54PM +0900, Paul Elder via libcamera-devel wrote:\n> Add Flags<E> as a supported type in the IPA interface.\n>\n> It is used in mojom with the [flags] attribute. Any field or parameter\n> type E that is prefixed with the [flags] attribute will direct the code\n> generator to generate the type name \"Flags<E>\" and appropriate\n> serialization/deserialization code for Flags<E> instead of for E.\n>\n> It is usable and has been tested in struct members, function input and\n> output parameters, and Signal parameters. This does not add support for\n> returning Flags as direct return values.\n>\n> Additionally, the [scopedEnum] attribute can be used on enum\n> definitions, which will instruct the code generator to convert it to an\n> enum class instead of a raw enum.\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n> ---\n> Changes in v5:\n> - fix serialization of scoped enums that are struct members\n>   - IsScoped doesn't catch enums that are struct members; only the enum\n>     definition itself, so we have a separate IsEnumScoped now for struct\n>     members\n>\n> Changes in v4:\n> - make flags also passed directly (as opposed to const references)\n>\n> Changes in v3.1:\n> - add documentation of [scopedEnum] and [flags] to core.mojom\n>   - it's not the best place, but better somewhere than nowhere for now\n>\n> Changes in v3:\n> - change flags attribute from [Flags] to [flags]\n> - make it so that enum input parameters are passed directly (as opposed\n>   to const references)\n> - to turn enum definitions in mojom to enum classes, use the\n>   [scopedEnum] attribute (as opposed to the [Flags] attribute in v2)\n>   - correspondingly, the function in the mojom generator python script\n>     is separated out from IsFlags() into IsScoped()\n> - clean up IsFlags()\n> - simplify GetFullNameForElement() for flags\n>\n> Question for reviewers (in v2): Should we use a different attribute name to\n> specify that an enum should be an enum class?\n>\n> Answer (in v3): Yes; that attribute name is [scopedEnum]\n>\n> Changes in v2:\n> - Use mojom attribute to specify that a field should be Flags<E>,\n>   instead of using a magic type name format\n> ---\n>  include/libcamera/ipa/core.mojom              |  9 +++++\n>  include/libcamera/ipa/ipa_interface.h         |  1 +\n>  .../definition_functions.tmpl                 |  2 +-\n>  .../module_ipa_interface.h.tmpl               |  2 +-\n>  .../module_ipa_proxy.h.tmpl                   |  2 +-\n>  .../libcamera_templates/proxy_functions.tmpl  | 10 +++--\n>  .../libcamera_templates/serializer.tmpl       |  6 +++\n>  .../generators/mojom_libcamera_generator.py   | 37 +++++++++++++++++--\n>  8 files changed, 60 insertions(+), 9 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index 74f3339e..ef28ff2d 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -33,6 +33,15 @@ module libcamera;\n>   *     available for the type and there's no need to generate one\n>   * - hasFd - struct fields or empty structs only\n>   *   - Designate that this field or empty struct contains a SharedFD\n> + * - scopedEnum - enum definitions\n> + *   - Designate that this enum should be an enum class, as opposed to a pure\n> + *     enum\n> + * - flags - struct fields or function parameters that are enums\n> + *   - Designate that this enum type E should be Flags<E> in the generated C++\n> + *     code\n> + *   - For example, if a struct field is defined as `[flags] ErrorFlag f;`\n> + *     (where ErrorFlag is defined as an enum elsewhere in mojom), then the\n> + *     generated code for this field will be `Flags<ErrorFlag> f`\n>   *\n>   * Rules:\n>   * - If the type is defined in a libcamera C++ header *and* a (de)serializer is\n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\n> index 50ca0e7b..8afcfe21 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -13,6 +13,7 @@\n>  #include <map>\n>  #include <vector>\n>\n> +#include <libcamera/base/flags.h>\n>  #include <libcamera/base/signal.h>\n>\n>  #include <libcamera/controls.h>\n> diff --git a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> index 94bb4918..8b8509f3 100644\n> --- a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> @@ -9,7 +9,7 @@\n>   # \\param enum Enum object whose definition is to be generated\n>   #}\n>  {%- macro define_enum(enum) -%}\n> -enum {{enum.mojom_name}} {\n> +enum{{\" class\" if enum|is_scoped}} {{enum.mojom_name}} {\n>  {%- for field in enum.fields %}\n>  \t{{field.mojom_name}} = {{field.numeric_value}},\n>  {%- endfor %}\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl\n> index 415ec283..160601f7 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl\n> @@ -69,7 +69,7 @@ public:\n>  {%- for method in interface_event.methods %}\n>  \tSignal<\n>  {%- for param in method.parameters -%}\n> -\t\t{{\"const \" if not param|is_pod}}{{param|name}}{{\" &\" if not param|is_pod}}\n> +\t\t{{\"const \" if not param|is_pod}}{{param|name}}{{\" &\" if not param|is_pod and not param|is_enum}}\n>  \t\t{{- \", \" if not loop.last}}\n>  {%- endfor -%}\n>  > {{method.mojom_name}};\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index c308dd10..ed270f5c 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -46,7 +46,7 @@ public:\n>  {%- for method in interface_event.methods %}\n>  \tSignal<\n>  {%- for param in method.parameters -%}\n> -\t\t{{\"const \" if not param|is_pod}}{{param|name}}{{\" &\" if not param|is_pod}}\n> +\t\t{{\"const \" if not param|is_pod}}{{param|name}}{{\" &\" if not param|is_pod and not param|is_enum}}\n>  \t\t{{- \", \" if not loop.last}}\n>  {%- endfor -%}\n>  > {{method.mojom_name}};\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index cbcfb64a..2be65d43 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -62,7 +62,9 @@\n>  {%- else %}\n>  \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n>  {%- endif %}\n> -{%- if param|is_enum %}\n> +{%- if param|is_flags %}\n> +\t\tIPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}\n> +{%- elif param|is_enum %}\n>  \t\tIPADataSerializer<uint32_t>::serialize(static_cast<uint32_t>({{param.mojom_name}})\n>  {%- else %}\n>  \t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> @@ -105,7 +107,9 @@\n>   #}\n>  {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}\n>  {{\"*\" if pointer}}{{param.mojom_name}} =\n> -{%- if param|is_enum %}\n> +{%- if param|is_flags %}\n> +IPADataSerializer<{{param|name_full}}>::deserialize(\n> +{%- elif param|is_enum %}\n>  static_cast<{{param|name_full}}>(IPADataSerializer<uint32_t>::deserialize(\n>  {%- else %}\n>  IPADataSerializer<{{param|name}}>::deserialize(\n> @@ -133,7 +137,7 @@ IPADataSerializer<{{param|name}}>::deserialize(\n>  {%- if param|needs_control_serializer %}\n>  \t&controlSerializer_\n>  {%- endif -%}\n> -){{\")\" if param|is_enum}};\n> +){{\")\" if param|is_enum and not param|is_flags}};\n>  {%- endmacro -%}\n>\n>\n> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> index 77bae36f..323e1293 100644\n> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -34,6 +34,10 @@\n>  \t\tstd::tie({{field.mojom_name}}, std::ignore) =\n>  \t{%- if field|is_pod %}\n>  \t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> +\t{%- elif field|is_flags %}\n> +\t\t\tIPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});\n> +\t{%- elif field|is_enum_scoped %}\n> +\t\t\tIPADataSerializer<uint{{field|bit_width}}_t>::serialize(static_cast<uint{{field|bit_width}}_t>(data.{{field.mojom_name}}));\n>  \t{%- elif field|is_enum %}\n>  \t\t\tIPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}});\n>  \t{%- endif %}\n> @@ -96,6 +100,8 @@\n>  \t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n>  \t\t{%- if field|is_pod %}\n>  \t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n> +\t\t{%- elif field|is_flags %}\n> +\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});\n>  \t\t{%- else %}\n>  \t\tret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));\n>  \t\t{%- endif %}\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> index 753bfc73..6c176aba 100644\n> --- a/utils/ipc/generators/mojom_libcamera_generator.py\n> +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> @@ -74,6 +74,8 @@ def GetDefaultValue(element):\n>          return element.default\n>      if type(element.kind) == mojom.Kind:\n>          return '0'\n> +    if IsFlags(element):\n> +        return ''\n>      if mojom.IsEnumKind(element.kind):\n>          return f'static_cast<{element.kind.mojom_name}>(0)'\n>      if isinstance(element.kind, mojom.Struct) and \\\n> @@ -184,7 +186,7 @@ def MethodParameters(method):\n>      params = []\n>      for param in method.parameters:\n>          params.append('const %s %s%s' % (GetNameForElement(param),\n> -                                         '&' if not IsPod(param) else '',\n> +                                         '' if IsPod(param) or IsEnum(param) else '&',\n>                                           param.mojom_name))\n>      for param in MethodParamOutputs(method):\n>          params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n> @@ -220,9 +222,28 @@ def IsControls(element):\n>  def IsEnum(element):\n>      return mojom.IsEnumKind(element.kind)\n>\n> +# Only works the enum definition, not types\n> +def IsScoped(element):\n> +    attributes = getattr(element, 'attributes', None)\n> +    if not attributes:\n> +        return False\n> +    return 'scopedEnum' in attributes\n> +\n> +\n> +def IsEnumScoped(element):\n> +    if not IsEnum(element):\n> +        return False\n> +    return IsScoped(element.kind)\n> +\n\nI guess Python coding style wants two empy lines between function\ndeclarations ? At least this is how the current implementation looks\nlike\n\nClearly a minor:\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n\n>  def IsFd(element):\n>      return mojom.IsStructKind(element.kind) and element.kind.mojom_name == \"SharedFD\"\n>\n> +def IsFlags(element):\n> +    attributes = getattr(element, 'attributes', None)\n> +    if not attributes:\n> +        return False\n> +    return 'flags' in attributes\n> +\n>  def IsMap(element):\n>      return mojom.IsMapKind(element.kind)\n>\n> @@ -251,9 +272,11 @@ def ByteWidthFromCppType(t):\n>          raise Exception('invalid type')\n>      return str(int(_bit_widths[key]) // 8)\n>\n> -\n\n>  # Get the type name for a given element\n>  def GetNameForElement(element):\n> +    # Flags\n> +    if IsFlags(element):\n> +        return f'Flags<{GetFullNameForElement(element.kind)}>'\n>      # structs\n>      if (mojom.IsEnumKind(element) or\n>          mojom.IsInterfaceKind(element) or\n> @@ -302,7 +325,8 @@ def GetNameForElement(element):\n>  def GetFullNameForElement(element):\n>      name = GetNameForElement(element)\n>      namespace_str = ''\n> -    if mojom.IsStructKind(element):\n> +    if (mojom.IsStructKind(element) or\n> +        mojom.IsEnumKind(element)):\n>          namespace_str = element.module.mojom_namespace.replace('.', '::')\n>      elif (hasattr(element, 'kind') and\n>               (mojom.IsStructKind(element.kind) or\n> @@ -311,6 +335,10 @@ def GetFullNameForElement(element):\n>\n>      if namespace_str == '':\n>          return name\n> +\n> +    if IsFlags(element):\n> +        return GetNameForElement(element)\n> +\n>      return f'{namespace_str}::{name}'\n>\n>  def ValidateZeroLength(l, s, cap=True):\n> @@ -407,10 +435,13 @@ class Generator(generator.Generator):\n>              'is_array': IsArray,\n>              'is_controls': IsControls,\n>              'is_enum': IsEnum,\n> +            'is_enum_scoped': IsEnumScoped,\n>              'is_fd': IsFd,\n> +            'is_flags': IsFlags,\n>              'is_map': IsMap,\n>              'is_plain_struct': IsPlainStruct,\n>              'is_pod': IsPod,\n> +            'is_scoped': IsScoped,\n>              'is_str': IsStr,\n>              'method_input_has_fd': MethodInputHasFd,\n>              'method_output_has_fd': MethodOutputHasFd,\n> --\n> 2.30.2\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0F7C8BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 12 Oct 2022 07:32:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E86062D89;\n\tWed, 12 Oct 2022 09:32:19 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DCE1603D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 Oct 2022 09:32:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id CB07E20012;\n\tWed, 12 Oct 2022 07:32:17 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665559939;\n\tbh=uKKhECSDcVXfh6ICPTaaUa5CArEXMN4ueUaBDNUEgGI=;\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=OLOp3RVgt0XD/BIOLlCi5moO52V62lQrHHHmn1CyUMtwiDhcTZG3T/9e2CRLjfhmx\n\tt7RnkyfGubNqGQSrb/I0uWrUod+30Zpjjp80uZ4GSnq8mulcP6+94Vi9FoHjB+u+le\n\tfTXcbfRh/9kBYUmI7ZSH5JzPVp/vLO4o0TBM80It6Aj7rtD4QgiOMPTTdLCcUSoqma\n\t7dTkOuXtdqUBL4XKYpftaRo/TToB8cfn8RlAHn6lHmqOXJwfmkSu5zrjoCAyW9D4sL\n\tCp/YAHNKuE5ml0IkvsBmLul07qdWpSEmWbTajoIFzN0Z1ewCTLFxmc8crcREOKteXc\n\tuj30hU2ictM4g==","Date":"Wed, 12 Oct 2022 09:32:16 +0200","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20221012073216.6dhb6fne3uhziopz@uno.localdomain>","References":"<20221011105859.457567-1-paul.elder@ideasonboard.com>\n\t<20221011105859.457567-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221011105859.457567-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 4/9] utils: ipc: Add support for\n\tFlags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]