Add documentation for generators and API #640

gbiggs · 2021-12-17T01:20:48Z

This PR adds documentation to the type support APIs and generators for C and C++.

The documentation needs review by someone with a good understanding of rosidl, to ensure it is correct and identify any missing documentation.

Signed-off-by: Geoffrey Biggs <[email protected]>

gbiggs · 2021-12-17T01:21:14Z

Should the code templates for the generated files also be documented so that the generated type support APIs get documented?

clalancette · 2021-12-21T15:37:05Z

rosidl_typesupport_introspection_c/rosidl_typesupport_introspection_c/cli.py

+    """
+    Generator class for generating the C implementation of the type support.


CI is failing because pep257 thinks this can all be on one line.

let's see if this is enough c41fbcd

hidmic

First pass. Thanks for doing this @gbiggs !

hidmic · 2021-12-21T20:49:05Z

rosidl_typesupport_introspection_c/bin/rosidl_typesupport_introspection_c

+
+    Parameters
+    ----------
+    argv : List[str]


@gbiggs meta: if we're adding type annotations in docstrings, we may as well type annotate the code itself here.

I've tried here 1b972df

hidmic · 2021-12-21T20:56:52Z

rosidl_typesupport_introspection_c/include/rosidl_typesupport_introspection_c/field_types.h

  rosidl_typesupport_introspection_c__ROS_TYPE_CHAR = 4,
+  /// Wide character, large enough to support Unicode code points.


@gbiggs nit: I think we should specify this is a 16 bit character, which doesn't necessarily match wchar_t size on every platform nor is enough for code points in every Unicode encoding.

hidmic · 2021-12-21T20:58:20Z

...pesupport_introspection_c/include/rosidl_typesupport_introspection_c/message_introspection.h

  const char * name_;
+  /// The type of the field as a value of the field types enum,
+  /// e.g. rosidl_typesupport_introspection_c__ROS_TYPE_FLOAT
  uint8_t type_id_;


@gbiggs meta, preexisting: I wonder why don't we simply name that enum type and use it here.

I've tried 739b377

hidmic · 2021-12-21T20:59:04Z

...pesupport_introspection_c/include/rosidl_typesupport_introspection_c/message_introspection.h

  size_t string_upper_bound_;
+  /// If the type_id_ value is rosidl_typesupport_introspection_c__ROS_TYPE_MESSAGE,
+  /// this points to an array describing the fields of the sub-interface.


@gbiggs nit: here we talk about a sub-interface, above we talk about an embedded message.

Consider sticking to one.

I did not find the place where we referenced it as "embedded message". Could you please point me to it?

hidmic · 2021-12-21T21:00:54Z

...pesupport_introspection_c/include/rosidl_typesupport_introspection_c/message_introspection.h

  const rosidl_message_type_support_t * members_;
+  /// True if this field is an array, false if it is a unary type. An array has the same value for


@gbiggs hmm what do you mean by unary type? The IDL spec talks about base types, and rosidl_parser talks about basic types.

Removed the reference to 'unary' type bd1c007

hidmic · 2021-12-21T21:23:33Z

...l_typesupport_introspection_cpp/include/rosidl_typesupport_introspection_cpp/field_types.hpp

@@ -21,30 +21,53 @@
 namespace rosidl_typesupport_introspection_cpp
 {

+/// Primitive (plain-old data) types supported by the ROS type system.
+
+/// IEEE-754 binary32 format floating point number.
 const uint8_t ROS_TYPE_FLOAT = rosidl_typesupport_introspection_c__ROS_TYPE_FLOAT;


@gbiggs meta, preexisting: this is weird. Why not defining an equivalent enum type?

I don't understand this one, skipped.

hidmic · 2021-12-21T21:24:17Z

...ort_introspection_cpp/include/rosidl_typesupport_introspection_cpp/message_introspection.hpp

  const char * name_;
+  /// The type of the field as a value of the field types enum,
+  /// e.g. rosidl_typesupport_introspection_c__ROS_TYPE_FLOAT


@gbiggs I'd think this field deals with rosidl_typesupport_introspection_cpp::ROS_TYPE_* values? Same below.

changed here c22fabc

hidmic · 2021-12-21T21:27:25Z

...introspection_cpp/include/rosidl_typesupport_introspection_cpp/message_type_support_decl.hpp

-// This is implemented in the shared library provided by this package.
+/// Returns a pointer to the type support structure provided by this library for introspecting
+/// interfaces.
+/// This is implemented in the shared library provided by this package.


@gbiggs meta: this isn't strictly provided by librosidl_typesupport_introspection_cpp.so itself but fully specialized in the lib<package_name>__rosidl_typesupport_introspection_cpp.so libraries that can be found in downstream packages. See here.

I tried to clarify it here 1b1d634

hidmic · 2021-12-21T21:59:05Z

rosidl_typesupport_introspection_cpp/rosidl_typesupport_introspection_cpp/cli.py

+    """
+    Generator class for generating the C++ implementation of the type support.
+
+    """


@gbiggs meta: I think we have to separate things a bit here. These cli.py modules simply make up a CLI for convenient access to the generators. They are not used during builds (yet, #560).

Improve description here 5eb8f06

hidmic · 2021-12-21T22:02:56Z

...ort_introspection_cpp/include/rosidl_typesupport_introspection_cpp/message_introspection.hpp

  const void * default_value_;
+  /// If is_array_ is true, a pointer to a function that gives the size of one member of the array.
+  /// Only used for nested interface types.


@gbiggs hmm, IIUC this function applies to array members (see here).

@gbiggs also, here and below, it may be worth mentioning that the first argument should be a pointer to the actual memory representation of the member.

Added a comment 2d013dc

j-rivero · 2022-01-16T23:03:38Z

I have the review changes ready but not sure if I can push them to this current PR from @gbiggs fork.

j-rivero · 2022-01-18T19:34:00Z

I have the review changes ready but not sure if I can push them to this current PR from @gbiggs fork.

Follow here #646

Add documentation for generators and API

ebc68ed

Signed-off-by: Geoffrey Biggs <[email protected]>

gbiggs requested a review from j-rivero December 17, 2021 01:21

gbiggs self-assigned this Dec 17, 2021

gbiggs added the documentation label Dec 17, 2021

clalancette reviewed Dec 21, 2021

View reviewed changes

hidmic self-requested a review December 21, 2021 15:37

hidmic reviewed Dec 21, 2021

View reviewed changes

j-rivero assigned j-rivero and unassigned gbiggs Jan 15, 2022

j-rivero mentioned this pull request Jan 18, 2022

add documentation for generators and API (take II) #646

Merged

j-rivero closed this Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation for generators and API #640

Add documentation for generators and API #640

gbiggs commented Dec 17, 2021

gbiggs commented Dec 17, 2021

clalancette Dec 21, 2021

j-rivero Jan 18, 2022

hidmic left a comment

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

hidmic Dec 21, 2021

hidmic Dec 21, 2021

j-rivero Jan 18, 2022

j-rivero commented Jan 16, 2022

j-rivero commented Jan 18, 2022

		"""
		Generator class for generating the C implementation of the type support.

		rosidl_typesupport_introspection_c__ROS_TYPE_CHAR = 4,
		/// Wide character, large enough to support Unicode code points.

		const rosidl_message_type_support_t * members_;
		/// True if this field is an array, false if it is a unary type. An array has the same value for

Add documentation for generators and API #640

Add documentation for generators and API #640

Conversation

gbiggs commented Dec 17, 2021

gbiggs commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-rivero commented Jan 16, 2022

j-rivero commented Jan 18, 2022