Add plugin support for generated C++ message headers #32

meyerj · 2017-08-07T20:37:19Z

This pull requests adds plugin support to gencpp. It is motivated by the plugin feature in Eigen to extend the MatrixBase class, but uses a slightly different approach. Please consider it as a proof-of-concept, a request for comments and a starting point for discussions.

It is basically a combination of two patches:

The possibility to provide user-defined message headers for all or a subset of messages and skip generation completely. Check https://github.com/orocos/kdl_msgs for an example use case, where we completely replace the generated struct with a C++11 template alias for a native KDL type.
The possibility to provide a user-defined plugin header. Given that the message type is foo_msgs/Bar, a header placed at include/foo_msgs/plugin/Bar.h inside the package's source folder can define macros to
- add custom constructors, member functions or even data members to generated structs (FOO_MSGS_MESSAGE_PLUGIN_CLASS_BODY)
- override the default set of constructors (FOO_MSGS_MESSAGE_BAR_PLUGIN_CONSTRUCTOR)
- override the default serializer (FOO_MSGS_MESSAGE_BAR_PLUGIN_SERIALIZER)
- override the default printer (FOO_MSGS_MESSAGE_BAR_PLUGIN_PRINTER)
- or simply include other useful headers

In any case, if none of the two additional files exists, the generated message header should be exactly the same as without the patch.

Of course as a developer you have to know what you are doing to not break the expected message API or compatibility with other languages. There clearly are some limitations and caveats. The approach is kind of complementary to what is called C++ type adaptation in the roscpp tutorials, where another, existing type is adapted to behave like a ROS message for publishing and subscribing, without identifying them. But in some cases extending or customizing an existing message type is more convenient than to adapt another type, especially because those extensions will immediately apply to nested instances, too.

Just to make my point - not necessarily as a serious suggestion - here are some examples that could be applied to messages similar to the ones in geometry_msgs:

add a Pose constructor that initializes it with a position and orientation (C++11 has list initialization as an alternative)
override the default constructor of a Quaternion with one that initializes the scalar component to 1 if desired (unit quaternion), or only for the Transform message
provide some basic math operators for vector- and matrix-like message types
provide operator[] for vectors or matrices
provide simple helper methods like setZero(), isZero(), isValid(), ...
modify the textual representation of messages for the console, e.g. adding the norm or RPY output for orientation quaternions

Or, in case of sensor_msgs, additional headers like image_encodings.h could be included by default from the plugin header file.

What I consider as less appropriate is to add conversion functions to and from other types that would add extra dependencies. A separate header file with free functions, like they already exist in various packages today, is the better choice for that purpose.

I assume a similar mechanism could be applied to other generators like genpy, too, for the sake of consistency.

dirk-thomas

I like the idea of both options (full replacement vs. macro based extensibility). While you can easily shoot yourself in the foot with this it provides advanced users with a very powerful option to extend the generated code.

This repo currently uses a single branch for all active ROS distros. I am not sure if such an invasive change should be applied to all ROS distros though (even though it intends to be backward compatible / not changing anything until explicitly being used). I would lean towards either only targeting future ROS distros or Kinetic a newer (as a compromise).

I also added a few comments inline.

dirk-thomas · 2017-10-24T21:59:02Z

cmake/gencpp-extras.cmake.em

-    )
-  list(APPEND ALL_GEN_OUTPUT_FILES_cpp ${GEN_OUTPUT_FILE})
+  # check if a user-provided header file exists
+  if(EXISTS ${PROJECT_SOURCE_DIR}/include/${ARG_PKG}/${MSG_SHORT_NAME}.h)


Nitpick: please wrap ${PROJECT_SOURCE_DIR}/include/${ARG_PKG}/${MSG_SHORT_NAME}.h in quotes.

dirk-thomas · 2017-10-24T22:01:40Z

scripts/msg.h.template

+  print('#include <%s/plugin/%s.h>'%(spec.package, spec.short_name));
+else:
+  have_plugin = False
+}@


Nitpick: rewrite as:

@{ import os has_plugin = os.path.exists('include/%s/plugin/%s.h' % (spec.package, spec.short_name)) }@

and then use it the same way as for the other macros:

@[if has_plugin]@ #include <@(spec.package)/plugin/@(spec.short_name).h> @[end if]@

dirk-thomas · 2017-10-24T22:06:59Z

scripts/msg.h.template

@@ -64,6 +75,9 @@ struct @(spec.short_name)_
  typedef @(spec.short_name)_<ContainerAllocator> Type;

 @# constructors (with and without allocator)
+@[if have_plugin]@
+#ifndef @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_CUSTOM_CONSTRUCTOR


The use case of initializing the fields with different values would imo be better addressed by specifying those default values in the .msg file (as it is possible in ROS 2). That approach ensures a consistent initialization across all languages. Therefore I would rather not have a CUSTOM_CONSTRUCTOR macro.

Or do you have other use cases in mind beside that?

I agree that specifying default values in the msg file would be preferable in most cases. But this feature is not available in ROS 1 to my best knowledge and custom constructors could go further, e.g. initialize values nested inside a message field. An example use case would be quaternions nested inside messages that represent orientation and rotation (like in geometry_msgs/Transform) which could be initialized to a unit quaternion in that context, while this is not necessarily true for arbitrary quaternions.

(just my 2 cents on this, in the hypothesis that we had default values in ROS1)
If it was possible, I do believe that users should be able specify default values for nested fields as for any other field type and generate a constructor accordingly. (so in your example geometry_msgs/Transform.msg would specify the default value of the rotation field to be a unit Quaternion and the constructor of the Transform message in every language would initialize the rotation field accordingly)

dirk-thomas · 2017-10-24T22:10:00Z

scripts/msg.h.template

@@ -210,6 +235,9 @@ namespace ros
 namespace serialization
 {

+@[if have_plugin]@
+#ifndef @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_CUSTOM_SERIALIZER


I think it would be beneficial if all macro names share a common prefix which includes the term "plugin", e.g. @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN_. Maybe the "custom" part can be skipped since by providing the macro it is kind of implied that a custom version is provided?

That would name this macro: @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN_SERIALIZER.

dirk-thomas · 2017-10-24T22:10:46Z

scripts/msg.h.template

@@ -237,6 +270,9 @@ namespace message_operations
 {

 @# Printer operation
+@[if have_plugin]@
+#ifndef @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_CUSTOM_PRINTER


Same as above: @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN_PRINTER.

dirk-thomas · 2017-10-24T22:13:25Z

scripts/msg.h.template

+@# Plugins
+@[if have_plugin]@
+#ifdef @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN
+  @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN


I think this macro name should contain something in the name which clarifies that it is "additional stuff in the class body".
Together with the feedback from another comment below maybe: @(spec.package.upper())_MESSAGE_@(spec.short_name.upper())_PLUGIN_CLASS_BODY.

meyerj · 2017-10-24T23:23:12Z

Thanks for your feedback, @dirk-thomas. I will try to address your comments as soon as possible.

We simply add this version of gencpp to a custom workspace if needed, and it does not have any side effects so far. So it would not be strictly necessary to have this patch in released versions of gencpp, but I thought it might be useful for other users, too.

See ros#32 (review).

meyerj · 2018-01-01T16:23:50Z

I finally addressed @dirk-thomas' comments and pushed an update version of the plugin patch. Feel free to create a separate branch for kinetic and newer and change the target branch of the PR. But as the patch has almost no risk of breaking existing packages and has no effect on generated message headers unless a plugin header would exist, I do not see a reason to not release it for indigo, too.

…ter the initial build Without this dependency, if a plugin header has been added after the initial build of a message package, it would not be picked up even if a cmake invocation is enforced with the `--force-cmake` argument or manually because cmake considers the generated header as up-to-date. The patch also adds some cmake output to indicate that a user-defined message or plugin header has been found in the package source.

dirk-thomas · 2018-01-29T19:58:44Z

I finally addressed @dirk-thomas' comments and pushed an update version of the plugin patch.

Great - thank you.

In 270a736 I just refactored the order to do the following which I think is easier to read:

#ifdef MACRO
  MACRO
#else
  ... normal code
#endif

dirk-thomas · 2018-01-29T20:26:22Z

@meyerj Can you please document the new feature in the wiki with a note that it is / will be available as of version 0.6.0 of the package. Thanks.

meyerj · 2018-01-29T21:12:28Z

Thanks for merging! Very helpful to have that feature merged upstream.

I'll update the ROS wiki soon. Which page would you prefer?

I think a new section in (*) would fit best.

dirk-thomas · 2018-01-29T21:19:40Z

Sounds good to me. I leave it up to your judgement. Please post a link to the diff here for future readers to easily find it. Thanks!

hamalMarino · 2018-02-14T08:03:14Z

New section added here: Customizing generated message headers for C++.

This customization of the CMake for copying over message headers is no longer necessary as of gencpp 0.6.0 (after ros/gencpp#32 has been merged). Signed-off-by: Hamal Marino <hamal.marino@intermodalics.eu>

dirk-thomas · 2018-02-14T23:30:42Z

Thank you for writing the docs.

I updated the version as of when this feature is available since the feature will become available in older ROS distros once a the new version of this package is released for them (since they all share the same branch).

peci1 · 2019-03-13T16:46:09Z

Thanks for the useful PR, I've already made use of it :)
What about adding MY_PKG_MESSAGE_MYVECTOR3_PLUGIN_FREE_FUNCTIONS that would be at (or near) the very end of the header file? That'd allow adding more traits and such things.

dirk-thomas · 2019-03-13T16:52:49Z

@peci1 To avoid that your comment gets lost I would suggest to create a new ticket rather than commenting on an already closed one.

What about adding MY_PKG_MESSAGE_MYVECTOR3_PLUGIN_FREE_FUNCTIONS that would be at (or near) the very end of the header file?

Depending on what you mean with free functions you might be able to just put them into the <pkgname>/plugin/<msgname>.h file.

Added plugin support

653b80c

dirk-thomas reviewed Oct 24, 2017

View reviewed changes

dirk-thomas mentioned this pull request Oct 30, 2017

roscpp: message constructors with values for fields (ros ticket #2723) ros/ros_comm#148

Closed

Changed plugin macro names according to PR review

31710d7

See ros#32 (review).

meyerj force-pushed the feature/plugin-support branch from 792046d to c612b51 Compare January 5, 2018 17:23

switch order of ifdef / ifndef

270a736

dirk-thomas added the enhancement label Jan 29, 2018

dirk-thomas merged commit 347ed33 into ros:indigo-devel Jan 29, 2018

meyerj deleted the feature/plugin-support branch January 29, 2018 21:12

hamalMarino mentioned this pull request Feb 15, 2018

Removed no longer necessary copying of message include files orocos/kdl_msgs#3

Merged

This was referenced May 3, 2018

boost_date_time_msgs: do not copy generated include files for gencpp (>= 0.6.0) orocos/boost_date_time_msgs#1

Merged

rt_string_msgs: do not copy generated include files for gencpp (>= 0.6.0) orocos/rt_string_msgs#2

Merged

peci1 mentioned this pull request Mar 13, 2019

Allow free functions in C++ message header plugins #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin support for generated C++ message headers #32

Add plugin support for generated C++ message headers #32

meyerj commented Aug 7, 2017 •

edited

Loading

dirk-thomas left a comment

dirk-thomas Oct 24, 2017

dirk-thomas Oct 24, 2017

dirk-thomas Oct 24, 2017

meyerj Oct 24, 2017

mikaelarguedas Oct 24, 2017

dirk-thomas Oct 24, 2017

dirk-thomas Oct 24, 2017

dirk-thomas Oct 24, 2017

meyerj commented Oct 24, 2017

meyerj commented Jan 1, 2018

dirk-thomas commented Jan 29, 2018 •

edited

Loading

dirk-thomas commented Jan 29, 2018

meyerj commented Jan 29, 2018

dirk-thomas commented Jan 29, 2018

hamalMarino commented Feb 14, 2018

dirk-thomas commented Feb 14, 2018 •

edited

Loading

peci1 commented Mar 13, 2019 •

edited

Loading

dirk-thomas commented Mar 13, 2019

Add plugin support for generated C++ message headers #32

Add plugin support for generated C++ message headers #32

Conversation

meyerj commented Aug 7, 2017 • edited Loading

dirk-thomas 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

meyerj commented Oct 24, 2017

meyerj commented Jan 1, 2018

dirk-thomas commented Jan 29, 2018 • edited Loading

dirk-thomas commented Jan 29, 2018

meyerj commented Jan 29, 2018

dirk-thomas commented Jan 29, 2018

hamalMarino commented Feb 14, 2018

dirk-thomas commented Feb 14, 2018 • edited Loading

peci1 commented Mar 13, 2019 • edited Loading

dirk-thomas commented Mar 13, 2019

meyerj commented Aug 7, 2017 •

edited

Loading

dirk-thomas commented Jan 29, 2018 •

edited

Loading

dirk-thomas commented Feb 14, 2018 •

edited

Loading

peci1 commented Mar 13, 2019 •

edited

Loading