Remove topic partitions by rohitsalem · Pull Request #285 · ros2/rmw_connext

rohitsalem · 2018-04-13T21:23:45Z

connects to ros2/ros2#476
To use the topic/service name directly without using the partitions.

mikaelarguedas · 2018-04-18T21:49:52Z

rmw_connext_shared_cpp/src/node.cpp

    participant_qos.user_data.value[sizeof(prefix) - 1 + name_length] = ';';
  }

+  // Setting the contentfilter_property_max_length to 1024


Can you please modify the comment to describe why it's needed and how 1024 has been picked?

Yes, I'm still investigating the exact number that should be used for the property, will update once I have a reasonable one.

for reference: https://community.rti.com/static/documentation/connext-dds/5.3.1/doc/api/connext_dds/api_cpp/structDDS__DomainParticipantResourceLimitsQosPolicy.html#aef1ff851202f5777d31528a52d528e4f

From the RTPS spec:

ContentFilterProperty_t: Mapping of the type typedef string<256> String256; typedef sequence<string> StringSequence; struct ContentFilterProperty_t { String256 contentFilteredTopicName; String256 relatedTopicName; String256 filterClassName; string filterExpression; StringSequence expressionParameters; };

@mikaelarguedas do you suggest putting the link in the code comment as well?

yes I think it makes sense to have the link in the comment as well

mikaelarguedas · 2018-04-18T21:49:55Z

rmw_connext_shared_cpp/src/node.cpp

  }

+  // Setting the contentfilter_property_max_length to 1024
+  participant_qos.resource_limits.contentfilter_property_max_length = 1024;


As it been confirm that we can still talk to native connext participants if we modify this default value ?

Still investigating that to confirm along with the exact length for the property.

Should we place this back in progress then if you're still working on it ?

I've checked native connext replier and requester, changed the contentfilter_property_max_length of requester to be 1024, replier to be 256 and vice-versa. The replier and requester worked as expected.

mikaelarguedas · 2018-04-26T20:32:38Z

As we found out that the service length has to be shorter with Connext. Can you post here what the error message is if we pass a service name within "ROS maximum length" but too long for Connext?
It may be worth add a check here to return a sensible error message in the case where it's too long

rohitsalem · 2018-04-26T21:41:31Z

Here is the error message when I run ros2 run demo_nodes_cpp add_two_ints_client when the service_name is for example repeated character a but still within "ROS maximum length"

rsalem@warner:~/work/ros2_ws$ ros2 run demo_nodes_cpp add_two_ints_client
RTI Data Distribution Service Evaluation License issued to OSRF dthomas@osrfoundation.org For non-production use only.
Expires on 05-may-2018 See www.rti.com for more information.
CorrelationCFTBuilder::create_correlation_cft:ERROR: Bad parameter: topic name is too long: rr/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaReply
connext::details::EntityUntypedImpl::initialize:!failed (see previous errors)

>>> [rcutils|error_handling.c:155] rcutils_set_error_state()
This error state is being overwritten:

  'C++ exception during construction of Requester, at /home/rsalem/work/ros2_ws/build_isolated/example_interfaces/rosidl_typesupport_connext_cpp/example_interfaces/srv/dds_connext/add_two_ints__type_support.cpp:100'

with this new error message:

  'failed to create requester, at /home/rsalem/work/ros2_ws/src/ros2/rmw_connext/rmw_connext_cpp/src/rmw_client.cpp:141'

rcutils_reset_error() should be called after error handling to avoid this.
<<<
terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  could not create client: failed to create requester, at /home/rsalem/work/ros2_ws/src/ros2/rmw_connext/rmw_connext_cpp/src/rmw_client.cpp:141, at /home/rsalem/work/ros2_ws/src/ros2/rcl/rcl/src/rcl/client.c:174

rohitsalem · 2018-04-26T22:07:10Z

regarding the check for the length of the service_name, I think It should be added in _process_service_name function, but still as of now what should the max limit for service_name in connext should be? When I checked the native connext Requester Replier example, the maximum service name that I was able to use was of 185 characters, If we have to impose a limit here I think it should be 185U - 8U (to account for the ros prefixes)

mikaelarguedas · 2018-04-26T22:51:08Z

Actually as the service length is not checked properly at the moment for any rmw implementation (we check the length before adding Request or Reply to the topic names and don't account for it), let's keep that for another set of PRs to make sure these ones get reviewed / merged in a timely manner.

Can you open an issue on rmw to relocate the discussion there ?

mikaelarguedas

lgtm, one question about adding the Request/Reply suffixes ourselves though

mikaelarguedas · 2018-04-28T00:57:47Z

rmw_connext_cpp/src/rmw_client.cpp

@@ -129,12 +127,16 @@ rmw_create_client(
  // This has to be evaluated whether or not to provide a
  // Subscriber/Publisher object directly with preset partitions.


Nit: no preset partitions anymore

@Karsten1987 this is a TODO, can it be removed or should it be modified?

mikaelarguedas · 2018-04-28T01:02:14Z

rmw_connext_cpp/src/process_topic_and_service_names.cpp

+    // concat the ros_service_*_prefix and Request/Reply suffixes with the service_name
+    char * request_concat_str = rcutils_format_string(
+      allocator,
+      "%s%s%s", ros_service_requester_prefix, service_name, "Request");


hmmm Do we need to add Request and Reply ourselves now ? Isn't Connext adding them automatically when we call their service API ?

Yes, There are two ways connext allows us to do this:

if we use set service_name, then connext does add the Request and Reply for us.

If we use set the request_topic_name and reply_topic_name, connext just uses whatever we give to it. (although it adds the GUID to Reply topic) see: here
Here we are not passing the service name but instead passing the request_topic_str and reply_topic_str directly

Karsten1987 · 2018-04-28T01:19:53Z

rmw_connext_cpp/src/process_topic_and_service_names.cpp

+    char * response_concat_str = rcutils_format_string(
+      allocator,
+      "%s%s%s", ros_service_response_prefix, service_name, "Reply");
+    if (!response_concat_str) {


i think request_concat_str is leaked here. If it was previously allocated, but response_concat_str fails here, only the request string will be deallocated.

@Karsten1987 I think I got the logic wrong. So should it be if the response_concat_str fails here, the request_concat_str for which memory was allocated previously is deallocated? I see that's what was there previously

Karsten1987 · 2018-04-28T01:27:17Z

rmw_connext_cpp/src/process_topic_and_service_names.cpp

-      }
-      *request_partition_str = DDS_String_dup(request_concat_str);
-      *response_partition_str = DDS_String_dup(response_concat_str);
+  if (!avoid_ros_namespace_conventions) {


Consider to simply set the ros prefix to an empty string and run the same logic when concatenating the topic string. Because if I understand your code correctly, the only difference is whether you append the ros prefix or not, right?
This makes the logic for cleaning up the allocated strings simpler.

string _ros_prefix = ros_prefix; if (avoid_ros_namespace_conventions) { _ros_prefix = ""; } topic = (%s%s%s, _ros_prefix, topic_name, "[Request|Reply]"

rohitsalem added the in progress Actively being worked on (Kanban column) label Apr 13, 2018

rohitsalem mentioned this pull request Apr 13, 2018

Remove topic partitions ros2/ros2#476

Closed

rohitsalem force-pushed the remove_topic_partitions branch from f5e9b23 to ae38b7a Compare April 18, 2018 20:52

rohitsalem added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 18, 2018

rohitsalem requested a review from Karsten1987 April 18, 2018 20:54

mikaelarguedas reviewed Apr 18, 2018

View reviewed changes

rohitsalem added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) labels Apr 18, 2018

rohitsalem mentioned this pull request Apr 26, 2018

service_name length checks for different rmw_implementations ros2/rmw#136

Open

mikaelarguedas reviewed Apr 28, 2018

View reviewed changes

Karsten1987 reviewed Apr 28, 2018

View reviewed changes

rohitsalem and others added 7 commits May 14, 2018 11:44

remove partitions from topic names

db52387

Remove partitions from service_names

43fbbf6

Setting the contentfilter_property_max_length to 1024

d1badce

explanation for new content_filter_property_max_length limit

45ed820

Added detailed explanation for the changed property max length

e328c0a

ease up string concatenation

44d7122

cleanup obsolete comment

89024a5

Karsten1987 force-pushed the remove_topic_partitions branch from 58f06d4 to 89024a5 Compare May 14, 2018 18:44

Karsten1987 approved these changes Jun 7, 2018

View reviewed changes

Karsten1987 merged commit 26fee35 into master Jun 7, 2018

Karsten1987 deleted the remove_topic_partitions branch June 7, 2018 21:12

wjwwood removed the in review Waiting for review (Kanban column) label Jun 7, 2018

mikaelarguedas mentioned this pull request Feb 21, 2019

[rmw_connext_cpp] Cannot create secure nodes with current usage of partitions #281

Closed

		@@ -129,12 +127,16 @@ rmw_create_client(
		// This has to be evaluated whether or not to provide a
		// Subscriber/Publisher object directly with preset partitions.

Conversation

rohitsalem commented Apr 13, 2018

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!

mikaelarguedas commented Apr 26, 2018

Uh oh!

rohitsalem commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitsalem commented Apr 26, 2018

Uh oh!

mikaelarguedas commented Apr 26, 2018

Uh oh!

mikaelarguedas 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!

rohitsalem May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rohitsalem commented Apr 26, 2018 •

edited

Loading

rohitsalem May 3, 2018 •

edited

Loading