Bridging services by haueck · Pull Request #36 · ros2/ros1_bridge

haueck · 2016-11-22T13:02:05Z

dirk-thomas · 2016-12-06T23:36:06Z

I am sorry that my latest PR #44 made this conflict. I will rebase your patch against master and comment here when it is ready.

dhood

This is great - thank you for this PR! The custom mapping works well and I can get service calls working in both directions. I've been testing it out a bit and I have a couple of issues:

unsupported server-clients cause the bridge to crash, e.g. if you run ROS 2 parameter_events example you get

terminate called after throwing an instance of 'std::runtime_error'
  what():  No template specialization for the service: ros2:rcl_interfaces/DescribeParameters

using a ROS 1 client and a ROS 2 server results in two requests being received, for both Connext and Fast RTPS (running e.g. rosrun rospy_tutorials add_two_ints_client 3 4 and add_two_ints_server gives:

dhood@osrf-esteve:~ [~]$ add_two_ints_server__rmw_connext_cpp 
RTI Data Distribution Service EVAL License issued to OSRF dhood@osrfoundation.org For non-production use only.
Expires on 07-Jan-2017 See www.rti.com for more information.
Incoming request
a: 3 b: 4
Incoming request
a: 3 b: 4
^Csignal_handler(2)

removing a ROS 1 server - ROS 2 client bridge (e.g. rosrun roscpp_tutorials add_two_ints_server and add_two_ints_client_async__rmw_connext_cpp) crashes for Connext with:

Removed 2 to 1 bridge for service add_two_ints
terminate called after throwing an instance of 'std::runtime_error'
  what():  rcl_wait() failed: read condition handle is null, at /home/dhood/ros2_ws/install_isolated/rmw_connext_shared_cpp/include/rmw_connext_shared_cpp/shared_functions.hpp:440, at /home/dhood/ros2_ws/src/ros2/rcl/rcl/src/rcl/wait.c:581
Aborted (core dumped)

if you use a ROS 1 server - ROS 2 client bridge (as in 3) with Fast RTPS, then remove it, then try to use it again, the client never re-connects (might be related to the topics never going away for fastrtps):

dhood@osrf-esteve:~ [~]$ add_two_ints_client_async
service not available, waiting again...
service not available, waiting again...
service not available, waiting again...
service not available, waiting again...

dhood · 2016-12-07T03:50:26Z

doc/index.rst

 Example mapping rules file
 --------------------------

-Example package mapping rule::


this is appropriate formatting for .rst (I know it looks like a type if it were .md). this applies to the lines below as well

dhood · 2016-12-07T05:16:27Z

ros1_bridge/__init__.py

    resources = ament_index_python.get_resources(resource_type)
    for package_name, prefix_path in resources.items():
-        resource, _ = ament_index_python.get_resource(resource_type, package_name)
+        resource = ament_index_python.get_resource(resource_type, package_name)


this change reverts #35 which was in place because of an update to the ament_index API. If it's not working for you locally how it was, you probably have to update your ament/ament_index checkout to master. similarly at https://github.com/ros2/ros1_bridge/pull/36/files#diff-3db064ef68c7605e1de4bc963ee60609R186

dhood · 2016-12-07T05:29:14Z

src/dynamic_bridge.cpp

+      service_bridges_1_to_2.find(name) == service_bridges_1_to_2.end())
+    {
+      auto factory = ros1_bridge::get_service_factory("ros1", details.at("package"), details.at("name"));
+      service_bridges_2_to_1[name] = factory->service_bridge_2_to_1(ros1_node, ros2_node, name);


this is throwing a runtime error that causes the bridge to crash if it's not a supported service

haueck · 2016-12-09T13:03:40Z

Thanks for the review!

Ad. 1. Fixed, factory is not throwing anymore as it is a normal situation when there is no bridge for a service

Ad. 2. This client written in Python makes two calls add_two_ints_client

Ad. 3. I added error handling when removing a bridge but I cannot verify it because I cannot build ROS2 with RTI Connext DDS. What Connext version do you use? I use 5.2.3 for open source projects and it fails during linking. When I used a trial version it was fine but it has expired.

Ad. 4. It is probably related to this issue. If the bridge finds a ROS1 server it creates a ROS2 server. If ROS1 server is removed, the bridge removes its own ROS2 server. But this server does not disappear and the bridge thinks that it found a new ROS2 server... so it prepares for routing from ROS1 and ROS2 and will never route again from ROS2 to ROS1.

dirk-thomas · 2016-12-09T16:44:57Z

Ad. 3. I added error handling when removing a bridge but I cannot verify it because I cannot build ROS2 with RTI Connext DDS. What Connext version do you use? I use 5.2.3 for open source projects and it fails during linking. When I used a trial version it was fine but it has expired.

We can build ROS 2 against the professional version (which requires a license) as well as against the evaluation version (which can be downloaded from their website). Maybe you can post more information what exactly fails for you in order to resolve the problem.

- printing errors to stderr - replaced boost library with std - fixed indentation in templates - support for a manual service matching - simplified an example in README

- Service factory does not raise errors anymore - Handling exceptions while creating/removing bridges - Fixed syntax in index.rst

haueck · 2016-12-12T11:15:12Z

My branch is rebased + one more commit with some updates after rebase.

haueck · 2016-12-12T11:46:51Z

Here is a log file from building ROS2 with RTI Connext DDS for open source projects: gist/rti_connext_dds.log

dirk-thomas · 2016-12-12T17:18:26Z

README.md

+
+While building, ros1_bridge looks for all installed ROS and ROS2 services.
+Found services are matched by comparing package name, service name and fields
+in a request and a response. If all names are the same in ROS and ROS2 service,


Please add a newline after every sentence.

dirk-thomas · 2016-12-12T17:19:33Z

include/ros1_bridge/factory.hpp

    ROS1_T & ros1_msg);
 };

+template <class R1_TYPE, class R1_REQ, class R1_RES, class R2_TYPE, class R2_REQ, class R2_RES>


Please remove the template arguments for request / response. They can be derived with e.g. R1_TYPE::Request.

Please use the same template names as for pub / sub: e.g. ROS1_T.

dirk-thomas · 2016-12-12T17:21:19Z

include/ros1_bridge/factory.hpp

+private:
+
+  void translate_1_to_2(const R1_REQ&, R2_REQ&);
+  void translate_1_to_2(const R1_RES&, R2_RES&);


These should be able to reuse the conversion functions already being generated for messages. The request and response are normal messages - the only difference is that they are in the srv namespace rather then msg.

They do reuse those generated functions. They call convert*() functions for all non-primitive data types.

If you mean, that we could use that template generating convert*() to generate functions converting requests and responses, it is possible but not that trivial. I would need to rewrite this python script and I do not have time to do it now.

Yes, currently the implementation of these functions iterate over the fields. I think we can get rid of this of we expand the already existing message templates for the request / response. I will look into it...

dirk-thomas · 2016-12-12T17:21:55Z

include/ros1_bridge/factory.hpp

+  void translate_1_to_2(const R1_REQ&, R2_REQ&);
+  void translate_1_to_2(const R1_RES&, R2_RES&);
+  void translate_2_to_1(R1_REQ&, const R2_REQ&);
+  void translate_2_to_1(R1_RES&, const R2_RES&);


Please swap the argument order to match the name of the functions. Might become obsolete with the comment above.

I swapped the arguments.

dirk-thomas · 2016-12-12T17:22:53Z

include/ros1_bridge/factory.hpp

+    R1_TYPE srv;
+    translate_2_to_1(srv.request, *request);
+    if (client.call(srv))
+    {


Please place the { and } within functions not on a separate line to match the code style of the rest of the file.

dirk-thomas · 2016-12-12T17:32:43Z

ros1_bridge/__init__.py

+        for export in pkg.exports:
+            if export.tagname != 'ros1_bridge':
+                continue
+            if 'service_mapping_rules' not in export.attributes:


Please avoid a second different export for these mappings. They should be specified in a single mapping file together with the message mapping.

dirk-thomas · 2016-12-12T17:33:19Z

ros1_bridge/__init__.py

+        'ros1_service_name',
        'ros2_package_name',
        'ros2_message_name',
+        'ros2_service_name',


A single mapping shouldn't be able to have a message name as well as a service name.

I changed conditionals so it is either message mapping or service mapping, never both at the same time.

After all, I created two separate classes: MessageMappingRule and ServiceMappingRule.

dirk-thomas · 2016-12-12T17:34:54Z

ros1_bridge/__init__.py

+                        "type": ros2_type.rstrip("[]"),
+                        "name": ros2_name,
+                    }
+                })


This custom code shouldn't be necessary. Messages can already be converted between ROS 1 and ROS 2. There shouldn't be a need to reimplement this.

As I mentioned before, it is possible to unify some functions in this script but it might not be that easy and I do not have time to do it now.

dirk-thomas · 2016-12-12T17:35:52Z

src/dynamic_bridge.cpp

+      if (factory) {
+        try {
+          service_bridges_2_to_1[name] = factory->service_bridge_2_to_1(ros1_node, ros2_node, name);
+          // printf("Created 2 to 1 bridge for service %s\n", name.data());


These messages should be printed (same as for pub / sub) so that the user sees what is happening.

dirk-thomas · 2016-12-12T17:38:23Z

test/test_ros1_bridge.py.in

+    self.assertEqual(None, self.bridge.poll(), "Bridge exited prematurely")
+
+if __name__ == '__main__':
+    unittest.main()


Please use 4 space indentation for Python code (see PEP8).

dirk-thomas · 2016-12-13T00:40:21Z

Building the package also results in compiler warning about unused variables (start1, start2, goal1, goal2).

dirk-thomas · 2016-12-13T00:51:28Z

README.md

+# Shell C:
+. <ros-install-dir>/setup.bash
+export ROS_MASTER_URI=http://localhost:11311
+<ros-install-dir>/lib/roscpp_tutorials/add_two_ints_server


This line should use rosrun to start the node.

dirk-thomas · 2016-12-13T01:00:53Z

It works for me when starting a ROS 1 server and a ROS 2 client. But when I stop the ROS 1 server and start it again, the ROS 2 client doesn't find the server anymore.

dirk-thomas · 2016-12-13T01:04:39Z

I committed a fix for the merge conflict.

dirk-thomas · 2016-12-13T01:06:38Z

@haueck Please let me know when you will have time to address the comments. Depending on the timeline we might merge the patch as is in order to get it in before the beta freeze and the comments need to be addressed in a follow up PR.

dirk-thomas · 2016-12-13T06:24:36Z

I will merge the current state for now. Please create a follow up PR soon.

haueck · 2016-12-13T13:47:11Z

@dirk-thomas, thank you for the review!

Building the package also results in compiler warning about unused variables (start1, start2, goal1, goal2).

I didn't use variables with those names, no idea what it is.

It works for me when starting a ROS 1 server and a ROS 2 client. But when I stop the ROS 1 server and start it again, the ROS 2 client doesn't find the server anymore.

@dhood already asked about it:

Ad. 4. It is probably related to this issue. If the bridge finds a ROS1 server it creates a ROS2 server. If ROS1 server is removed, the bridge removes its own ROS2 server. But this server does not disappear and the bridge thinks that it found a new ROS2 server... so it prepares for routing from ROS1 and ROS2 and will never route again from ROS2 to ROS1.

I think I will finish rework tomorrow.

dirk-thomas · 2016-12-13T16:28:18Z

Thank you for the quick update!

dhood · 2016-12-13T23:24:40Z

FYI the unused variable warnings referred to come from the generated files e.g.:

/home/dhood/ros2_ws/build_isolated/ros1_bridge/generated/nav_msgs_factories.cpp: In member function ‘void ros1_bridge::ServiceFactory<R1_TYPE, R1_REQ, R1_RES, R2_TYPE, R2_REQ, R2_RES>::translate_2_to_1(R1_REQ&, const R2_REQ&) [with R1_TYPE = nav_msgs::GetPlan; R1_REQ = nav_msgs::GetPlanRequest_<std::allocator<void> >; R1_RES = nav_msgs::GetPlanResponse_<std::allocator<void> >; R2_TYPE = nav_msgs::srv::GetPlan; R2_REQ = nav_msgs::srv::GetPlan_Request_<std::allocator<void> >; R2_RES = nav_msgs::srv::GetPlan_Response_<std::allocator<void> >]’:
/home/dhood/ros2_ws/build_isolated/ros1_bridge/generated/nav_msgs_factories.cpp:589:9: warning: unused variable ‘start1’ [-Wunused-variable]
   auto& start1 = req1.start;

* Added support for bridging services * Added tests and fixes after the review - printing errors to stderr - replaced boost library with std - fixed indentation in templates - support for a manual service matching - simplified an example in README * Fixed issues after a review - Service factory does not raise errors anymore - Handling exceptions while creating/removing bridges - Fixed syntax in index.rst * Updated services to use separate compilation units

* Removed separate file for mapping services * Other minor changes

haueck · 2016-12-14T14:41:29Z

New pull requests: #48 ros2/examples#153

Rework after the review (#36)

Karsten1987 · 2017-02-16T18:09:18Z

The PR has leads to a crash of the bridge whenever any topic happens to be of length 4.
In particular, it's this line: https://github.com/ros2/ros1_bridge/blob/master/src/dynamic_bridge.cpp#L651

Consider the following example:

std::string first = "scan";
std::string second = "sensor_msgs/LaserScan";
std::string suffix = "reply";
	
size_t suffix_found = first.rfind(suffix);  // return std::string::npos (-1)
printf("%u\n", suffix_found);  // size_t (-1) = 4294967295
printf("%u\n", first.size()); // 4
printf("%u\n", suffix.size()); // 5
printf("%u\n", first.size() - suffix.size());  // size_t(-1) = 4294967295
	
// This expression will evaluate to true, even though the suffix was never found.
if (first.rfind(suffix) == (first.size() - suffix.size())) {
    std::string & t = second;
    // CRASH HERE!
    std::string name(first.begin(), first.end() - suffix.size()); 
} else {
  ...
}

One possible solution is to explicitly filter out std::string::npos in the case the suffix Reply is not found.

* Removed separate file for mapping services * Other minor changes

haueck · 2017-02-17T08:43:21Z

@Karsten1987, thank you for this information! I've created a fix for the issue #58

haueck mentioned this pull request Nov 22, 2016

support for request / reply #33

Closed

dirk-thomas added the enhancement New feature or request label Dec 6, 2016

dhood reviewed Dec 7, 2016

View reviewed changes

dirk-thomas mentioned this pull request Dec 8, 2016

support for bridging services #45

Closed

Rafal Kozik added 3 commits December 12, 2016 08:57

Added support for bridging services

c559000

Added tests and fixes after the review

1dc208f

- printing errors to stderr - replaced boost library with std - fixed indentation in templates - support for a manual service matching - simplified an example in README

Fixed issues after a review

4d938fd

- Service factory does not raise errors anymore - Handling exceptions while creating/removing bridges - Fixed syntax in index.rst

haueck force-pushed the bridging_services branch from 78c93d7 to 4d938fd Compare December 12, 2016 08:15

Updated services to use separate compilation units

91bddce

dirk-thomas reviewed Dec 12, 2016

View reviewed changes

haueck mentioned this pull request Dec 13, 2016

Added a service mapping rule ros2/examples#136

Merged

dirk-thomas reviewed Dec 13, 2016

View reviewed changes

Merge branch 'master' into bridging_services

c71c8af

dirk-thomas added the in review Waiting for review (Kanban column) label Dec 13, 2016

dirk-thomas merged commit 3a4dd43 into ros2:master Dec 13, 2016

dirk-thomas removed the in review Waiting for review (Kanban column) label Dec 13, 2016

haueck pushed a commit to haueck/ros1_bridge that referenced this pull request Dec 14, 2016

Rework after the review (ros2#36)

9b09e85

* Removed separate file for mapping services * Other minor changes

haueck pushed a commit to haueck/ros1_bridge that referenced this pull request Dec 14, 2016

Rework after the review (ros2#36)

5039283

* Removed separate file for mapping services * Other minor changes

dirk-thomas added a commit that referenced this pull request Dec 14, 2016

Merge pull request #48 from haueck/bridging_services_fixes

a649c4f

Rework after the review (#36)

Karsten1987 mentioned this pull request Feb 16, 2017

dynamic_bridge crashes when trying to send PointCloud2 Messages #56

Closed

haueck pushed a commit to haueck/ros1_bridge that referenced this pull request Feb 17, 2017

Rework after the review (ros2#36)

39d3a9d

* Removed separate file for mapping services * Other minor changes

dirk-thomas mentioned this pull request Sep 12, 2017

remove check for header fields request_type and response_type #91

Merged

dirk-thomas mentioned this pull request Apr 17, 2018

--print-pairs prints only messages and not services #115

Closed

Conversation

haueck commented Nov 22, 2016

Uh oh!

dirk-thomas commented Dec 6, 2016

Uh oh!

dhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haueck commented Dec 9, 2016

Uh oh!

dirk-thomas commented Dec 9, 2016

Uh oh!

haueck commented Dec 12, 2016

Uh oh!

haueck commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Dec 13, 2016

Uh oh!