[{"id":18202,"web_url":"https://patchwork.libcamera.org/comment/18202/","msgid":"<20210716031430.GE2395@pyrite.rasen.tech>","date":"2021-07-16T03:14:30","subject":"Re: [libcamera-devel] [PATCH] 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 Thu, Jul 15, 2021 at 05:32:00PM +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\nIt's a good clean up!\n\n> \n> Update the file comment to match the most recent of th IPA/IPC\n\ns/comment/comments/\n\ns/of th//\n\n> interface definition and generation mechanism.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/core.mojom | 60 +++++++++++++++++++-------------\n> 1 file changed, 35 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index b32f30939454..d2017369b597 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -15,37 +15,47 @@ 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 Mojo core to accept it\n\nShould we already mention at this point about the exception if the type\nis only used as an array/map member?\n\n> + * - this attribute allows to define a symbol for the Mojo core that\n\ns/to define/defining/\n\n> + * corresponds to a library-defined type without duplicating its definition\n\nShould we say \"libcamera\" instead of \"library\"?\n\n> + * in the 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> + * - do not generate a (de)serializer for the structure\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> + * - manually implemented in the core library as in example the\n> + * ControlSerializer class that handles libcamera control-related types\n\nThe manual implementation of the (de)serializer is the\nIPADataSerializer, in ipa_data_serializer.cpp. ControlSerializer doesn't\ndo this; it's *used* by IPADataSerializer and\nIPADataSerializer.\n\nWhich means that this point should be merged into the next one.\n\n> + * - provided as a template specialization as done in ipa_data_serializer.h\n> + * for POD types and C++ containers\n\nipa_data_serializer.h only contains the generic (de)serializers for\nvectors and maps. The specialized ones go in ipa_data_serializer.cpp.\n\n> + * - generated at build time for types defined in a mojo interfaces\n> + * definition\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\nYeah, this point makes it really clear. Although it has the same intent\nas \"do not generate a (de)serializer for the structure\" above; maybe it\ncould be moved to a subpoint of that?\n\n> * - hasFd - struct fields or empty structs only\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 library C++ header and a (de)serializer is\n\nI think something to emphasize that both conditions must be met would be\nnice. Like \"*and*\" or \"is both defined\"\n\n> + * available, either manually written in the library or in\n> + * ipa_data_serializer.h, then the struct shall be declared as empty,\n\nAgain, \"manually written in the library\" = IPADataSerializer\nspecialization defined in ipa_data_serializer.cpp.\n\n> + * with both the [skipHeader] and [skipSerdes] attributes associated\n\nWhen both attributes are defined it's written as [skipHeader,\nskipSerdes]. I'm not sure if it's better to write the attributes\nindividually here without the square brackets, or if you want to group\nthem together like how you'd write it in code, or keep it as-is.\n\n> + * - If the type is defined in the library but no (de)serializer is available\n> + * then the type definition in the core.mojom file should have the\n> + * [skipHeader] attribute only\n\nWould it be better to clarify/remind as a subpoint that in this case a\n(de)serializer will be generated?\n\n> + * - If a type definition has [skipHeader], then the header where the type is\n> + * defined must be included in ipa_interface.h\n> + * - Nested types (e.g. FrameBuffer::Plane) cannot be directly defined in mojom\n> + * - Avoid them, by defining the nested type in a C++ header and provide a\n> + * (de)serializer\n> + * - The C++ namespace separator :: is replaced with a dot\n> + * - In example, to refer to FrameBuffer::Plane provide a definition of the\n> + * Plane type in a C++ header to be included, provide a deserializer and\n> + * reference it as FrameBuffer.Plane\n\nIt's a bit hard to read the points I think...\n1 - provide a definition of FrameBuffer::Plane in a C++ header\n2 - include this header in ipa_interface.h\n3 - provide a (de)serializer (in ipa_data_serializer.cpp)\n4 - in mojom, reference the type as FrameBuffer.Plane\n\nAlso, FrameBuffer.Plane (and any other nested types) is only usable as\narray/map members as it cannot be defined in mojom.\n\n\nThanks,\n\nPaul\n\n> + * - Types that are contained in an array/map do not require a mojom definition\n> + * if one exists in the library.\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> -- \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 69984C3226\n\tfor ;\n\tFri, 16 Jul 2021 03:14:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93C7768539;\n\tFri, 16 Jul 2021 05:14:41 +0200 (CEST)","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 6A2EE6027B\n\tfor ;\n\tFri, 16 Jul 2021 05:14:39 +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 CA9CF3F0;\n\tFri, 16 Jul 2021 05:14:37 +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=\"PrLhf2/O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626405279;\n\tbh=/35kajvg1EYy9mLUA8p6DcaTiuAsqoI0pwZ5/IlwGQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PrLhf2/O0CqkTiyxIcmTNpax+k3EHPLoXs5sR5yV4KAqnvBT6KoB87Hq4o7mqAvn5\n\tY6ibsKDlFVO1Hw8ZAl60ckfksRSRPqxnaEj7VPxUKJv9+bx+sRx7OCFRwURuhTJOuI\n\tvdOAyvgHJuc/7XzCk04kNOJ/w9+HKqx05MqS83tQ=","Date":"Fri, 16 Jul 2021 12:14:30 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210716031430.GE2395@pyrite.rasen.tech>","References":"<20210715153200.63805-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210715153200.63805-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}}]