Provide direct serialization of ROS2 messsage to ROS1 streams #381

dbking77 · 2022-10-19T01:12:03Z

Short Description

Provide functions to potentially save some CPU when converting ROS messages by allow "direct" serialization of ROS2 message classes to ROS1 streams.

Long Description

Currently, when ros1_bridge bridges a ROS2 topics to a ROS1 topics, it requires three steps

Deserialize ROS2 stream into a ROS2 message
Use convert_2_to_1 to copy members from ROS2 object to ROS1 message
Serialize ROS1 into ROS1 stream.

Using new feature removes a conversion step

Deserialize ROS2 stream into a ROS2 message
Serialize ROS2 into ROS1 stream.

Here's what the convert_2_to_1 function looks like geometry_msgs::Vector3

template<>
void Factory< ... >::convert_2_to_1(
  const geometry_msgs::msg::Vector3 & ros2_msg,
  geometry_msgs::Vector3 & ros1_msg)
{
  ros1_msg.x = ros2_msg.x;
  ros1_msg.y = ros2_msg.y;
  ros1_msg.z = ros2_msg.z;
}

Here's what the direct serialization function looks like (same internal function is used to implement read, write, and length)

template<>
template<typename STREAM_T, typename ROS2_MSG_T>
void
Factory<..>::msg_2_to_1_stream(
  STREAM_T& stream,
  ROS2_MSG_T& ros2_msg)
{
  stream.next(ros2_msg.x);
  stream.next(ros2_msg.y);
  stream.next(ros2_msg.z);
}

Auto-Generated Code Examples

PoseStamped

template<>
template<typename STREAM_T, typename ROS2_MSG_T>
void
Factory<...>::msg_2_to_1_stream(
  STREAM_T& stream,
  ROS2_MSG_T& ros2_msg)
{
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::msg_2_to_1_stream(stream, ros2_msg.header);

  Factory<
    geometry_msgs::Pose,
    geometry_msgs::msg::Pose
  >::msg_2_to_1_stream(stream, ros2_msg.pose);
}

sensor_msgs::Image

Message Definition

[sensor_msgs/Image]:
std_msgs/Header header
uint32 height
uint32 width
string encoding
uint8 is_bigendian
uint32 step
uint8[] data

Streaming Code

template<>
template<typename STREAM_T, typename ROS2_MSG_T>
void
Factory<...>::msg_2_to_1_stream(
  STREAM_T& stream,
  ROS2_MSG_T& ros2_msg)
{
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::msg_2_to_1_stream(stream, ros2_msg.header);
  stream.next(ros2_msg.height);
  stream.next(ros2_msg.width);
  stream.next(ros2_msg.encoding);
  stream.next(ros2_msg.is_bigendian);
  stream.next(ros2_msg.step);
  streamVectorSize(stream, ros2_msg.data);
  streamPrimitiveVector(stream, ros2_msg.data);
}

sensor_msgs::Imu

Imu message definition

std_msgs/Header header
geometry_msgs/Quaternion orientation
float64[9] orientation_covariance
geometry_msgs/Vector3 angular_velocity
float64[9] angular_velocity_covariance
geometry_msgs/Vector3 linear_acceleration
float64[9] linear_acceleration_covariance

Streaming code

template<>
template<typename STREAM_T, typename ROS2_MSG_T>
void
Factory<
  sensor_msgs::Imu,
  sensor_msgs::msg::Imu
>::msg_2_to_1_stream(STREAM_T& stream,
                     ROS2_MSG_T& ros2_msg)
{
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::msg_2_to_1_stream(stream, ros2_msg.header);

  Factory<
    geometry_msgs::Quaternion,
    geometry_msgs::msg::Quaternion
  >::msg_2_to_1_stream(stream, ros2_msg.orientation);

  streamPrimitiveVector(stream, ros2_msg.orientation_covariance);

  Factory<
    geometry_msgs::Vector3,
    geometry_msgs::msg::Vector3
  >::msg_2_to_1_stream(stream, ros2_msg.angular_velocity);

  streamPrimitiveVector(stream, ros2_msg.angular_velocity_covariance);

  Factory<
    geometry_msgs::Vector3,
    geometry_msgs::msg::Vector3
  >::msg_2_to_1_stream(stream, ros2_msg.linear_acceleration);
  streamPrimitiveVector(stream, ros2_msg.linear_acceleration_covariance);
}

gbiggs · 2022-11-03T23:01:10Z

Please rebase your PR to incorporate the changes from #377.

dbking77 · 2022-11-05T15:34:14Z

Please rebase your PR to incorporate the changes from #377.

Done

gbiggs

The function names in this PR all need re-thinking. Many of them do not say what the function actually does, and some appear to be opposite of what the function does. This makes the code quite hard to read and understand.

include/ros1_bridge/builtin_interfaces_factories.hpp

gbiggs · 2022-11-06T23:30:34Z

include/ros1_bridge/builtin_interfaces_factories.hpp

+Factory<
+ std_msgs::Duration,
+ builtin_interfaces::msg::Duration
+>::msg_2_to_1_stream(


Is this function going in the opposite direction to the function above? If so the name is probably incorrect.

Please correct me if I'm wrong, this seems like separate functions for read(IStream), write(OStream) and length(LStream) functionality, nothing to do with ROS 2 to 1 direction? The description mentions use of "same internal function", this is the backbone function overloading?
(applies to other comments below too)

ROS1 offers an all-in-one function to provide read(), write(), and length() without needing to implement each independently. I'm doing the same with this function.

Here's a tutorial that discusses creation of custom serializers for ROS1
http:https://wiki.ros.org/roscpp/Overview/MessagesSerializationAndAdaptingTypes#Serializer_Specialization

Here's a link to the ROS1 code that implements the "all-in-in" serialization:
https://github.com/ros/roscpp_core/blob/72ce04f8b2849e0e4587ea4d598be6ec5d24d8ca/roscpp_serialization/include/ros/serialization.h#L74

I agree the name is confusing the context of serialization direction. Some other ideas?

msg_all_in_one_stream

msg_2_and_1_stream

msg_2_from_or_to_1_stream

I'm open to any other suggestions for this function name?

I think it would be best to follow the convention of the other functions in ros1_bridge and use names starting with convert_.

Made naming change in 3rd commit

include/ros1_bridge/builtin_interfaces_factories.hpp

include/ros1_bridge/convert_builtin_interfaces.hpp

gbiggs · 2022-11-06T23:35:36Z

include/ros1_bridge/factory.hpp

+ /**
+ * @brief Reads (deserializes) a ROS2 class directly from a ROS1 stream
+ */
+ static void read_2_to_1_stream(ros::serialization::IStream & in_stream, ROS2_T & msg);


This function is named incorrectly.

Should I also change argument ordering for write_2_to_1_stream (above)?

static void write_2_to_1_stream(const ROS2_T & msg, ros::serialization::OStream & out_stream); static void read_1_to_2_stream(ros::serialization::IStream & in_stream, ROS2_T & msg);

Follow the convention of the existing conversion functions in ros1_bridge.

Made change to naming in 3rd commit

resource/interface_factories.cpp.em

CMakeLists.txt

quarkytale · 2022-11-07T09:30:41Z

include/ros1_bridge/builtin_interfaces_factories.hpp

+Factory<
+ std_msgs::Duration,
+ builtin_interfaces::msg::Duration
+>::msg_2_to_1_stream(


Please correct me if I'm wrong, this seems like separate functions for read(IStream), write(OStream) and length(LStream) functionality, nothing to do with ROS 2 to 1 direction? The description mentions use of "same internal function", this is the backbone function overloading?
(applies to other comments below too)

package.xml

dbking77 · 2022-11-08T20:20:46Z

@gbiggs It looks like some/all the of integration tests were failing because the rosmaster wasn't coming up?

gbiggs · 2022-11-09T00:39:53Z

The convert_all_in_one_stream function name is no better than the previous function names. You are relying on the types of the function arguments for the correct function to be called. This is fragile and difficult to understand and maintain. Please don't imitate the ROS 1 API naming scheme. Follow the ros1_bridge naming scheme and have explicitly separate functions for converting from ROS 1 to ROS 2 and the reverse.

dbking77 · 2022-11-09T18:11:13Z

The convert_all_in_one_stream function name is no better than the previous function names. You are relying on the types of the function arguments for the correct function to be called. This is fragile and difficult to understand and maintain. Please don't imitate the ROS 1 API naming scheme. Follow the ros1_bridge naming scheme and have explicitly separate functions converting from ROS 1 to ROS 2 and the reverse.

convert_all_in_one_stream is an "internal" function that is used as the implementation the other functions. I think this is a better approach than having 3 versions of the template generation code in interface_factories.cpp.em. I'm my experience the template generation code is nearly impossible to read, change, or debug. If any python code is incorrect, the error message doesn't even provide an line number for the issue.

I'm not trying to emulate the ROS1 naming scheme, I'm am using a strategy from ROS1 to avoid repeating code 3x times.

convert_all_in_one_stream is not meant to be used externally. I can make it private and change the name to internal_stream_translate_helper or something (Please provide suggestions?)

The write and read functions have been changes to exactly match the naming convention of ros1_bridge. The argument ordering have been changed to be "correct".

convert_2_to_1(ros2_msg, out_stream)
convert_1_to_2(in_stream, ros2_msg)

gbiggs · 2022-11-09T23:33:09Z

convert_all_in_one_stream is not meant to be used externally. I can make it private and change the name to internal_stream_translate_helper or something (Please provide suggestions?)

I think that would be better.

dbking77 · 2022-11-10T21:02:59Z

@gbiggs I made the name change, but couldn't make the function private, because a whole bunch of other Factory types need to use it.

dbking77 · 2022-11-15T18:13:01Z

@gbiggs is it possible to trigger a full test run?

dbking77 · 2022-11-21T22:28:26Z

@gbiggs ping

gbiggs · 2022-11-22T03:45:23Z

CI:

gbiggs · 2022-11-25T01:47:47Z

Please fix the build failures.

Signed-off-by: Derek King <[email protected]>

dbking77 · 2022-11-27T08:11:15Z

I was able to reproduce this build failure by compiling with optimization turned on

colcon build --packages-select ros1_bridge --cmake-force-configure --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo

It looks like the RelWithDebInfo build causes the internal functions to get inlined inside the generated *.cpp files so there are no symbols for them. I can't define the function specializations in factory.hpp because I can't have a function specialization in a non-specialized class type. Since header files are not generated, its would be difficult add generation of type-specific headers.

My work-around is to just use operator overloading for the different stream types instead of templating and template specialization.
The generated code now has 3 functions, instead of just 1 templated function

For example the auto-generated C++ code for PoseStamped now looks like:

template<>
void
Factory<
  geometry_msgs::PoseStamped,
  geometry_msgs::msg::PoseStamped
>::internal_stream_translate_helper(
  ros::serialization::OStream & stream,
  geometry_msgs::msg::PoseStamped const & ros2_msg)
{

  // write non-array field
  // write sub message field
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::internal_stream_translate_helper(stream, ros2_msg.header);

  // write non-array field
  // write sub message field
  Factory<
    geometry_msgs::Pose,
    geometry_msgs::msg::Pose
  >::internal_stream_translate_helper(stream, ros2_msg.pose);
}


template<>
void
Factory<
  geometry_msgs::PoseStamped,
  geometry_msgs::msg::PoseStamped
>::internal_stream_translate_helper(
  ros::serialization::IStream & stream,
  geometry_msgs::msg::PoseStamped  & ros2_msg)
{

  // write non-array field
  // write sub message field
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::internal_stream_translate_helper(stream, ros2_msg.header);

  // write non-array field
  // write sub message field
  Factory<
    geometry_msgs::Pose,
    geometry_msgs::msg::Pose
  >::internal_stream_translate_helper(stream, ros2_msg.pose);
}


template<>
void
Factory<
  geometry_msgs::PoseStamped,
  geometry_msgs::msg::PoseStamped
>::internal_stream_translate_helper(
  ros::serialization::LStream & stream,
  geometry_msgs::msg::PoseStamped const & ros2_msg)
{

  // write non-array field
  // write sub message field
  Factory<
    std_msgs::Header,
    std_msgs::msg::Header
  >::internal_stream_translate_helper(stream, ros2_msg.header);

  // write non-array field
  // write sub message field
  Factory<
    geometry_msgs::Pose,
    geometry_msgs::msg::Pose
  >::internal_stream_translate_helper(stream, ros2_msg.pose);
}

I'm now divided on whether I should have internal_stream_translate_helper based on overloading, or I should just have 3 different code generations routines for the different convert and length functions. I'll prototype this out to see if it seems better or worse than this approach.

@gbiggs Would it be possible to trigger a CI build on the farm just to see if this makes it through?

gbiggs · 2022-11-27T23:38:50Z

Here you go:

dbking77 · 2022-12-03T21:11:43Z

@gbiggs Looks like stuff compiled, but most/all live tests are failing because they can't access the rosmaster ?

   11: [ros1_talker_ros2_listener_across_dynamic_bridge__dynamic_bridge-2] [ERROR] [1669537320.951368727]: [registerPublisher] Failed to contact master at [localhost:11311].  Retrying...
  11: [ros1_talker_ros2_listener_across_dynamic_bridge__talker-3] [ERROR] [1669537321.152250959]: [registerPublisher] Failed to contact master at [localhost:11311].  Retrying...

gbiggs · 2022-12-04T22:54:42Z

That seems more likely to be a problem with CI rather than this change. Let's try again.

methylDragon · 2022-12-08T23:43:54Z

It looks like just linting issues @gbiggs https://ci.ros2.org/job/ci_packaging_linux/536/testReport/

methylDragon · 2022-12-08T23:44:43Z

@dbking77 could you fix those linting issues? :>

Signed-off-by: Derek King <[email protected]>

dbking77 · 2022-12-13T06:25:27Z

@gbiggs Fixed linting issues

dbking77 · 2022-12-15T15:46:11Z

@gbiggs Can you re-trigger? apt-get update failed

Invoking "sudo apt-get update"
Error: The process '/usr/bin/sudo' failed with exit code 100

gbiggs · 2022-12-21T00:28:35Z

dbking77 · 2023-01-10T18:42:18Z

@gbiggs Looks like build/test is failing because of missing release file for Jammy debians. It could just be a problem with http:https://packages.ros.org/ros/ubuntu mirror, but maybe is a problem in general?

   E: The repository 'http:https://packages.ros.org/ros/ubuntu jammy Release' does not have a Release file.
  W: http:https://packages.ros.org/ros2/ubuntu/dists/jammy/InRelease: Key is stored in legacy trusted.gpg keyring (/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details.

gbiggs · 2023-01-11T23:46:27Z

Where are you seeing that output?

dbking77 · 2023-01-13T00:18:16Z

THe CI / ros1_bridge (pull_request) : https://github.com/ros2/ros1_bridge/actions/runs/3682873863/jobs/6230902918

gbiggs · 2023-01-13T00:32:24Z

That automatic check hasn't been re-run; the manual CI run I did above passes so we're happy anyway.

dbking77 · 2023-01-14T03:37:15Z

Is there anything else I need to do for this to get merged?

gbiggs · 2023-01-16T00:55:15Z

CI passed, despite the automatic check failing, so merging this.

clalancette assigned quarkytale Nov 3, 2022

clalancette requested a review from quarkytale November 3, 2022 13:53

dbking77 force-pushed the dbking77/direct_serialization3 branch from bc435c7 to 555a8f7 Compare November 5, 2022 15:33

gbiggs reviewed Nov 7, 2022

View reviewed changes

quarkytale reviewed Nov 7, 2022

View reviewed changes

dbking77 force-pushed the dbking77/direct_serialization3 branch from 72d9149 to e98cd30 Compare November 8, 2022 20:19

dbking77 force-pushed the dbking77/direct_serialization3 branch from 7914a7a to dd4febe Compare November 11, 2022 19:01

gbiggs approved these changes Nov 22, 2022

View reviewed changes

dbking77 added 5 commits November 27, 2022 05:49

Generated functions for (de)serializing ROS2 messages to ROS1 streams.

1bef590

Signed-off-by: Derek King <[email protected]>

Partial set of fixes from review.

32d27e1

Signed-off-by: Derek King <[email protected]>

Rename functions based on discussion from review

603bd27

Signed-off-by: Derek King <[email protected]>

Whitespace fix.

6dc2222

Signed-off-by: Derek King <[email protected]>

Name change for conversion helper fucntion.

68a4ddf

Signed-off-by: Derek King <[email protected]>

dbking77 force-pushed the dbking77/direct_serialization3 branch from dd4febe to 68a4ddf Compare November 27, 2022 05:59

dbking77 requested a review from methylDragon as a code owner November 27, 2022 05:59

Use explicit function overloading instead of template for stream types.

9bb38e1

Signed-off-by: Derek King <[email protected]>

Fix factory.hpp crustify errors.

b0bbb7b

Signed-off-by: Derek King <[email protected]>

gbiggs merged commit c2fa9ed into ros2:master Jan 16, 2023

Provide direct serialization of ROS2 messsage to ROS1 streams #381

Provide direct serialization of ROS2 messsage to ROS1 streams #381

Conversation

dbking77 commented Oct 19, 2022 • edited Loading

Short Description

Long Description

Auto-Generated Code Examples

PoseStamped

sensor_msgs::Image

sensor_msgs::Imu

gbiggs commented Nov 3, 2022

dbking77 commented Nov 5, 2022 • edited Loading

gbiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbking77 Nov 7, 2022 • edited Loading

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

dbking77 commented Nov 8, 2022

gbiggs commented Nov 9, 2022

dbking77 commented Nov 9, 2022 • edited Loading

gbiggs commented Nov 9, 2022

dbking77 commented Nov 10, 2022

dbking77 commented Nov 15, 2022

dbking77 commented Nov 21, 2022

gbiggs commented Nov 22, 2022 • edited Loading

gbiggs commented Nov 25, 2022

dbking77 commented Nov 27, 2022

gbiggs commented Nov 27, 2022

dbking77 commented Dec 3, 2022

gbiggs commented Dec 4, 2022

methylDragon commented Dec 8, 2022

methylDragon commented Dec 8, 2022

dbking77 commented Dec 13, 2022

dbking77 commented Dec 15, 2022

gbiggs commented Dec 21, 2022

dbking77 commented Jan 10, 2023

gbiggs commented Jan 11, 2023

dbking77 commented Jan 13, 2023

gbiggs commented Jan 13, 2023

dbking77 commented Jan 14, 2023

gbiggs commented Jan 16, 2023

dbking77 commented Oct 19, 2022 •

edited

Loading

dbking77 commented Nov 5, 2022 •

edited

Loading

dbking77 Nov 7, 2022 •

edited

Loading

dbking77 commented Nov 9, 2022 •

edited

Loading

gbiggs commented Nov 22, 2022 •

edited

Loading