[libcamera-devel,v4] ipa: core.mojom: Rework core file documentation

13124 diff mbox series

The comment block at the beginning of the core.mojom file is meant to
provide an overview of how to use libcamera defined types in the definition
of mojom interfaces.

As the IPA/IPC interface definition mechanism evolved, the documentation
has not been updated accordingly.

Update the file comments to match the most recent IPA/IPC
interface definition and generation mechanism.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
v3->v4:
- Apply wording suggestions from Paul

v2-v3:
- Address Paul's comment:
  - s/[Mojo core|build system]/code generator/
  - Swap points in skipSerdes description to make clear the flag instruct
    the code generator not to generate a deserializer
  - Clarify that nested types can *solely* be used as map/array members

v1->v2:
- Address Paul's comment and clarify points that were not clear to me when I
  wrote v1 :)
  - (de)serializers implementations go in ipa_data_serializer.cpp
  - types used as array/map members do not require a mojom definition
  - remove duplications
  - s/the library/libcamera
- While at it, make all statements start with a capital letter and end without a
  full-stop.
---
---
 include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 27 deletions(-)

--
2.32.0

Hi Jacopo,

On Tue, Jul 27, 2021 at 09:50:23AM +0200, Jacopo Mondi wrote:
> The comment block at the beginning of the core.mojom file is meant to
> provide an overview of how to use libcamera defined types in the definition
> of mojom interfaces.
> 
> As the IPA/IPC interface definition mechanism evolved, the documentation
> has not been updated accordingly.
> 
> Update the file comments to match the most recent IPA/IPC
> interface definition and generation mechanism.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Looks good!

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> v3->v4:
> - Apply wording suggestions from Paul
> 
> v2-v3:
> - Address Paul's comment:
>   - s/[Mojo core|build system]/code generator/
>   - Swap points in skipSerdes description to make clear the flag instruct
>     the code generator not to generate a deserializer
>   - Clarify that nested types can *solely* be used as map/array members
> 
> v1->v2:
> - Address Paul's comment and clarify points that were not clear to me when I
>   wrote v1 :)
>   - (de)serializers implementations go in ipa_data_serializer.cpp
>   - types used as array/map members do not require a mojom definition
>   - remove duplications
>   - s/the library/libcamera
> - While at it, make all statements start with a capital letter and end without a
>   full-stop.
> ---
> ---
>  include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index b32f30939454..29ba35482b39 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -15,40 +15,54 @@ module libcamera;
>   *
>   * Attributes:
>   * - skipHeader - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a C++ header definition
> - *     generated
> + *   - Do not generate a C++ definition for the structure
> + *   - Any type used in a mojom interface definition must have a corresponding
> + *     definition in a mojom file for the code generator to accept it, except
> + *     for types solely used as map/array members for which a definition is not
> + *     required
> + *   - This attribute allows defining a symbol for the code generator that
> + *     corresponds to a libcamera type without duplicating its definition in the
> + *     generated C++ headers
>   * - skipSerdes - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a (de)serializer generated
> - *   - all fields need a (de)serializer to be defined, either hand-written
> - *     in ipa_data_serializer.h
> + *   - All types need a (de)serializer to be defined in order to be transported
> + *     over IPC. The (de)serializer can be:
> + *     - Manually implemented as a template specialization in
> + *       ipa_data_serializer.cpp in the libcamera sources
> + *     - Generated at build time for types defined in a mojom file
> + *   - This attribute instructs the build system that a (de)serializer is
> + *     available for the type and there's no need to generate one
>   * - hasFd - struct fields or empty structs only
> - *   - designate that this field or empty struct contains a FileDescriptor
> + *   - Designate that this field or empty struct contains a FileDescriptor
>   *
>   * Rules:
> - * - Any struct that is used in a struct definition in mojom must also be
> - *   defined in mojom
> - *   - If the struct has both a definition in a C++ header and a (de)serializer
> - *     in ipa_data_serializer.h, then the struct shall be declared as empty,
> - *     with both the [skipHeader] and [skipSerdes] attributes
> - *   - If the struct only has a definition in a C++ header, but no
> - *     (de)serializer, then the struct definition should have the [skipHeader]
> - *     attribute
> - * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.
> - *   - Avoid them, by defining them in a header in C++ and a (de)serializer in
> - *     ipa_data_serializer.h
> - * - If a struct is in an array/map inside a struct, then the struct that is
> - *   the member of the array/map does not need a mojom definition if it is
> - *   defined in a C++ header.
> - *   - This can be used to embed nested structures. The C++ double colon is
> - *     replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)
> - *   - The struct must still be defined in a header in C++ and a (de)serializer
> - *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is
> + *   available then the type shall be declared as empty with both attributes
> + *   associated and specified as: [skipHeader, skipSerdes]
> + *   - Example: [skipHeader, skipSerdes] ControlList {};
> + * - If the type is defined in libcamera but no (de)serializer is available
> + *   then the type definition in the core.mojom file should have the
> + *   [skipHeader] attribute only
> + *   - A (de)serializer will be generated for the type
> + * - If a type definition has [skipHeader], then the header where the type is
> + *   defined must be included in ipa_interface.h
> + * - Types that are solely used as array/map members do not require a mojom
> + *   definition if one exists in libcamera
> + * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom
> + *   - If used in mojom, the nested type shall be defined in a C++ header
> + *     and a (de)serializer shall be provided
> + *   - Nested types can only be used as array/map members
> + *   - When using the type, the C++ namespace separator :: is replaced with a
> + *     dot
> + *   - In example, to use the FrameBuffer::Plane type in mojom:
> + *     - Provide a definition of the FrameBuffer::Plane type in a C++ header
> + *     - Include the header in ipa_interface.h
> + *     - Provide a (de)serializer implementation ipa_data_serializer.cpp
> + *     - In mojom, reference the type as FrameBuffer.Plane and only as map/array
> + *       member
>   * - [skipHeader] and [skipSerdes] only work here in core.mojom.
> - * - If a struct definition has [skipHeader], then the header where the
> - *   struct is defined must be #included in ipa_interface.h
>   * - If a field in a struct has a FileDescriptor, but is not explicitly
>   *   defined so in mojom, then the field must be marked with the [hasFd]
> - *   attribute.
> + *   attribute
>   *
>   * \todo Generate documentation from Doxygen comments in .mojom files
>   * \todo Figure out how to keep the skipHeader structs in sync with their
> --
> 2.32.0
>

Hi Jacopo,

On 27/07/2021 08:50, Jacopo Mondi wrote:
> The comment block at the beginning of the core.mojom file is meant to
> provide an overview of how to use libcamera defined types in the definition
> of mojom interfaces.
> 
> As the IPA/IPC interface definition mechanism evolved, the documentation
> has not been updated accordingly.
> 
> Update the file comments to match the most recent IPA/IPC
> interface definition and generation mechanism.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> v3->v4:
> - Apply wording suggestions from Paul
> 
> v2-v3:
> - Address Paul's comment:
>   - s/[Mojo core|build system]/code generator/
>   - Swap points in skipSerdes description to make clear the flag instruct
>     the code generator not to generate a deserializer
>   - Clarify that nested types can *solely* be used as map/array members
> 
> v1->v2:
> - Address Paul's comment and clarify points that were not clear to me when I
>   wrote v1 :)
>   - (de)serializers implementations go in ipa_data_serializer.cpp
>   - types used as array/map members do not require a mojom definition
>   - remove duplications
>   - s/the library/libcamera
> - While at it, make all statements start with a capital letter and end without a
>   full-stop.
> ---
> ---

Lots of extra --- to watch out for when applying there ;-)

Sounds good.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  include/libcamera/ipa/core.mojom | 68 +++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index b32f30939454..29ba35482b39 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -15,40 +15,54 @@ module libcamera;
>   *
>   * Attributes:
>   * - skipHeader - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a C++ header definition
> - *     generated
> + *   - Do not generate a C++ definition for the structure
> + *   - Any type used in a mojom interface definition must have a corresponding
> + *     definition in a mojom file for the code generator to accept it, except
> + *     for types solely used as map/array members for which a definition is not
> + *     required
> + *   - This attribute allows defining a symbol for the code generator that
> + *     corresponds to a libcamera type without duplicating its definition in the
> + *     generated C++ headers
>   * - skipSerdes - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a (de)serializer generated
> - *   - all fields need a (de)serializer to be defined, either hand-written
> - *     in ipa_data_serializer.h
> + *   - All types need a (de)serializer to be defined in order to be transported
> + *     over IPC. The (de)serializer can be:
> + *     - Manually implemented as a template specialization in
> + *       ipa_data_serializer.cpp in the libcamera sources
> + *     - Generated at build time for types defined in a mojom file
> + *   - This attribute instructs the build system that a (de)serializer is
> + *     available for the type and there's no need to generate one
>   * - hasFd - struct fields or empty structs only
> - *   - designate that this field or empty struct contains a FileDescriptor
> + *   - Designate that this field or empty struct contains a FileDescriptor
>   *
>   * Rules:
> - * - Any struct that is used in a struct definition in mojom must also be
> - *   defined in mojom
> - *   - If the struct has both a definition in a C++ header and a (de)serializer
> - *     in ipa_data_serializer.h, then the struct shall be declared as empty,
> - *     with both the [skipHeader] and [skipSerdes] attributes
> - *   - If the struct only has a definition in a C++ header, but no
> - *     (de)serializer, then the struct definition should have the [skipHeader]
> - *     attribute
> - * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.
> - *   - Avoid them, by defining them in a header in C++ and a (de)serializer in
> - *     ipa_data_serializer.h
> - * - If a struct is in an array/map inside a struct, then the struct that is
> - *   the member of the array/map does not need a mojom definition if it is
> - *   defined in a C++ header.
> - *   - This can be used to embed nested structures. The C++ double colon is
> - *     replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)
> - *   - The struct must still be defined in a header in C++ and a (de)serializer
> - *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is
> + *   available then the type shall be declared as empty with both attributes
> + *   associated and specified as: [skipHeader, skipSerdes]
> + *   - Example: [skipHeader, skipSerdes] ControlList {};
> + * - If the type is defined in libcamera but no (de)serializer is available
> + *   then the type definition in the core.mojom file should have the
> + *   [skipHeader] attribute only
> + *   - A (de)serializer will be generated for the type
> + * - If a type definition has [skipHeader], then the header where the type is
> + *   defined must be included in ipa_interface.h
> + * - Types that are solely used as array/map members do not require a mojom
> + *   definition if one exists in libcamera
> + * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom
> + *   - If used in mojom, the nested type shall be defined in a C++ header
> + *     and a (de)serializer shall be provided
> + *   - Nested types can only be used as array/map members
> + *   - When using the type, the C++ namespace separator :: is replaced with a
> + *     dot
> + *   - In example, to use the FrameBuffer::Plane type in mojom:
> + *     - Provide a definition of the FrameBuffer::Plane type in a C++ header
> + *     - Include the header in ipa_interface.h
> + *     - Provide a (de)serializer implementation ipa_data_serializer.cpp
> + *     - In mojom, reference the type as FrameBuffer.Plane and only as map/array
> + *       member
>   * - [skipHeader] and [skipSerdes] only work here in core.mojom.
> - * - If a struct definition has [skipHeader], then the header where the
> - *   struct is defined must be #included in ipa_interface.h
>   * - If a field in a struct has a FileDescriptor, but is not explicitly
>   *   defined so in mojom, then the field must be marked with the [hasFd]
> - *   attribute.
> + *   attribute
>   *
>   * \todo Generate documentation from Doxygen comments in .mojom files
>   * \todo Figure out how to keep the skipHeader structs in sync with their
> --
> 2.32.0
>