[{"id":18375,"web_url":"https://patchwork.libcamera.org/comment/18375/","msgid":"<20210727073055.GB2167@pyrite.rasen.tech>","date":"2021-07-27T07:30:55","subject":"Re: [libcamera-devel] [PATCH v3] ipa: core.mojom: Rework core file\n\tdocumentation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 26, 2021 at 04:35:49PM +0200, Jacopo Mondi wrote:\n> The comment block at the beginning of the core.mojom file is meant to\n> provide an overview of how to use libcamera defined types in the definition\n> of mojom interfaces.\n> \n> As the IPA/IPC interface definition mechanism evolved, the documentation\n> has not been updated accordingly.\n> \n> Update the file comments to match the most recent IPA/IPC\n> interface definition and generation mechanism.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> v2-v3:\n> - Address Paul's comment:\n> - s/[Mojo core|build system]/code generator/\n> - Swap points in skipSerdes description to make clear the flag instruct\n> the code generator not to generate a deserializer\n> - Clarify that nested types can *solely* be used as map/array members\n> \n> v1->v2:\n> - Address Paul's comment and clarify points that were not clear to me when I\n> wrote v1 :)\n> - (de)serializers implementations go in ipa_data_serializer.cpp\n> - types used as array/map members do not require a mojom definition\n> - remove duplications\n> - s/the library/libcamera\n> - While at it, make all statements start with a capital letter and end without a\n> full-stop.\n> ---\n> \n> include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------\n> 1 file changed, 41 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index b32f30939454..2e1835c6ec84 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -15,40 +15,54 @@ module libcamera;\n> *\n> * Attributes:\n> * - skipHeader - structs only, and only in core.mojom\n> - * - designate that this struct shall not have a C++ header definition\n> - * generated\n> + * - Do not generate a C++ definition for the structure\n> + * - Any type used in a mojom interface definition must have a corresponding\n> + * definition in a mojo file for the code generator to accept it, except for\n> + * types solely used as map/array members for which a definition is not\n> + * required\n> + * - This attribute allows defining a symbol for the code generator that\n> + * corresponds to a libcamera type without duplicating its definition in the\n> + * generated C++ headers\n> * - skipSerdes - structs only, and only in core.mojom\n> - * - designate that this struct shall not have a (de)serializer generated\n> - * - all fields need a (de)serializer to be defined, either hand-written\n> - * in ipa_data_serializer.h\n> + * - All types need a (de)serializer to be defined in order to be transported\n> + * over the IPA protocol. The (de)serializer can be:\n\ns/IPA protocol/IPC/ ? Maybe it's an implementation detail?\n\n> + * - Manually implemented as a template specialization in\n> + * ipa_data_serializer.cpp in the libcamera sources\n> + * - Generated at build time for types defined in a mojo interfaces\n\ns/mojo interfaces/mojom files/ ? An interface in mojo is a collection of\nfunction definitions.\n\n> + * - This attribute instructs the build system that a (de)serializer is\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 FileDescriptor\n> + * - Designate that this field or empty struct contains a FileDescriptor\n> *\n> * Rules:\n> - * - Any struct that is used in a struct definition in mojom must also be\n> - * defined in mojom\n> - * - If the struct has both a definition in a C++ header and a (de)serializer\n> - * in ipa_data_serializer.h, then the struct shall be declared as empty,\n> - * with both the [skipHeader] and [skipSerdes] attributes\n> - * - If the struct only has a definition in a C++ header, but no\n> - * (de)serializer, then the struct definition should have the [skipHeader]\n> - * attribute\n> - * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.\n> - * - Avoid them, by defining them in a header in C++ and a (de)serializer in\n> - * ipa_data_serializer.h\n> - * - If a struct is in an array/map inside a struct, then the struct that is\n> - * the member of the array/map does not need a mojom definition if it is\n> - * defined in a C++ header.\n> - * - This can be used to embed nested structures. The C++ double colon is\n> - * replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)\n> - * - The struct must still be defined in a header in C++ and a (de)serializer\n> - * implemented in ipa_data_serializer.h, as it cannot be defined in mojom\n> + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is\n> + * available then the type shall be declared as empty with both attributes\n> + * associated and specified as: [skipHeader, skipSerdes]\n> + * - Example: [skipHeader, skipSerdes] ControlList {};\n> + * - If the type is defined in libcamera but no (de)serializer is available\n> + * then the type definition in the core.mojom file should have the\n> + * [skipHeader] attribute only\n> + * - A (de)serializer will be generated for the type\n> + * - If a type definition has [skipHeader], then the header where the type is\n> + * defined must be included in ipa_interface.h\n> + * - Types that are solely contained in an array/map do not require a mojom\n\ns/contained/used in/ ? Or \"used as array/map members\"? Not sure which is\nbest.\n\n> + * definition if one exists in libcamera\n> + * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom\n> + * - If used in mojom, the nested type shall be defined in a C++ header\n> + * and a (de)serializer shall be provided\n> + * - Nested types can only be used as part of array/map containers\n\ns/containers/members/ I think is more clear?\n\n\nMuch better! I think we just need to get the wording down and it'll be\nready to ship.\n\nPaul\n\n> + * - When using the type, the C++ namespace separator :: is replaced with a\n> + * dot\n> + * - In example, to use the FrameBuffer::Plane type in mojom:\n> + * - Provide a definition of the FrameBuffer::Plane type in a C++ header\n> + * - Include the header in ipa_interface.h\n> + * - Provide a (de)serializer implementation ipa_data_serializer.cpp\n> + * - In mojom, reference the type as FrameBuffer.Plane and only as map/array\n> + * member\n> * - [skipHeader] and [skipSerdes] only work here in core.mojom.\n> - * - If a struct definition has [skipHeader], then the header where the\n> - * struct is defined must be #included in ipa_interface.h\n> * - If a field in a struct has a FileDescriptor, but is not explicitly\n> * defined so in mojom, then the field must be marked with the [hasFd]\n> - * attribute.\n> + * attribute\n> *\n> * \\todo Generate documentation from Doxygen comments in .mojom files\n> * \\todo Figure out how to keep the skipHeader structs in sync with their\n> --\n> 2.32.0\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 B01EBC0109\n\tfor ;\n\tTue, 27 Jul 2021 07:31:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EE41687BF;\n\tTue, 27 Jul 2021 09:31:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAFEB687BA\n\tfor ;\n\tTue, 27 Jul 2021 09:31:03 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B3FDEE;\n\tTue, 27 Jul 2021 09:31:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fQDfOynQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627371063;\n\tbh=s3sCQRZxPn8pxa31/jzk/OgTOd6EmZJ+d6YdMSR/9hw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fQDfOynQ3WWCRDCWOAZB7d30GPYjNVLcQjI++EgwqN5Y99on/SxoHabA3Z6pIEU4j\n\tmv/UzcmehoYQjySX3c5DI9nxCtciAKMWGONcTVyaCqBQYgTQ4KkwqLEe0TZjVs0ls8\n\tkkZPJE4d8/JtAHfNyqHKNolKbctVpXhWp+A9FnVs=","Date":"Tue, 27 Jul 2021 16:30:55 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210727073055.GB2167@pyrite.rasen.tech>","References":"<20210726143549.23509-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210726143549.23509-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3] ipa: core.mojom: Rework core file\n\tdocumentation","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]