add concept of node namespace to rcl API #112

wjwwood · 2017-03-29T08:04:56Z

This not only exposes the node namespace in the API (via the rcl_node_init() and rcl_node_get_namespace() functions), but it also validates the node name and the new namespace arguments.

This also relies on ros2/rmw#93 to help validate the node namespace.

Connects to ros2/rmw#95

also validate node name and node namespace

Karsten1987 · 2017-03-29T16:52:12Z

rcl/include/rcl/node.h

+rcl_node_init(
+  rcl_node_t * node,
+  const char * name,
+  const char * name_space,


why not putting name_space into rcl_node_options_t ? We can expose this struct on a high level language.

I had two reasons for placing it like this.

The first was because I believe the namespace should be of elevated importance (similar to node name). Your same argument could be made for name, specifically that it should also be in the options struct, but to emulate the interface of ROS 1, node name and node namespace are high level concepts.

On the technical side, it is nice that the options structure remains a POD. As soon as I add a pointer (like a const char *) it is no longer "trivially copyable" (borrowing a term from C++).

That all being said, I don't feel strongly about it. I definitely agree that most options should end up in the structure, but in this instance I made a decision to leave it out.

Karsten1987 · 2017-04-05T00:47:04Z

rcl/include/rcl/node.h

+ * Even though this is a namespace, with additional relative namespace and/or
+ * a basename to follow, the namespace string passed here should not include
+ * a trailing forward slash.
+ * For an empty namespace, an empty string, i.e. "", should be used.


might be worth mentioning that NULL is considered an error here.

Good idea: dce05d6

Karsten1987 · 2017-04-05T01:00:01Z

rcl/src/rcl/node.c

+    return ret;
+  }
+  if (validation_result != RMW_NODE_NAME_VALID) {
+    const char * msg = rmw_node_name_validation_result_string(validation_result);


i think this was discussed in another PR already. But why not apply the same logic as above here?

// sets the error message inside ret = rmw_node_name_validation_result_string(validation_result); if (ret != RMW_RET_OK) // or similar for this use-case { RCL_GET_ERROR_MSG_SAFE... }

rmw_node_name_validation_result_string() returns the validation issue description as a string, because I didn't want to force it into being an error. Also, this way you can formulate the error however you want (possibly using the input topic name and the violation index to make a better error message).

Karsten1987 · 2017-04-05T01:07:25Z

rcl/test/rcl/test_node.cpp

+    EXPECT_EQ(RCL_RET_OK, ret);
+  }
+
+  // Node namespace with a trailing / which is not allowed.


I know this is not allowed by design, but I wonder if we should manually filter this constraint to be allowed. It looks to me like a rather easy check for the last character. Just an idea, nothing critical to this PR.

I'd rather leave this kind of flexibility to the top level API's (like rclcpp and rclpy). Each corner case makes it harder to describe and harder to test, so I'd like to keep rcl simple by avoiding them. That's just my personal preference though. I have no problem with allowing this exception to the rule here or in the higher level API's.

Karsten1987 · 2017-04-05T01:08:19Z

rcl/test/rcl/test_node.cpp

+    EXPECT_EQ(RCL_RET_OK, ret);
+  }
+
+  // Node namespace which is not absolute.


same thinking as above.

What if I add some "smoothing" here when I do the code which expands topics and stuff automatically (upcoming prs)? That way I can reuse those functions to automatically remove trailing / and add leading / if necessary.

sounds fine to me.

dirk-thomas · 2017-04-05T15:06:13Z

rcl/src/rcl/node.c

+  // Make sure the node name is valid before allocating memory.
+  int validation_result = 0;
+  size_t invalid_index = 0;
+  ret = rmw_validate_node_name(name, &validation_result, &invalid_index);


Since invalid_index isn't used it would be good to make the argument optional in the rmw_validate_node_name function.

See: ros2/rmw#95 (comment)

dirk-thomas · 2017-04-05T15:09:37Z

rcl/src/rcl/node.c

+  if (strlen(namespace_) > 0) {
+    validation_result = 0;
+    invalid_index = 0;
+    ret = rmw_validate_topic_name(namespace_, &validation_result, &invalid_index);


Since the namespace_ alone is not a FQN yet (it doesn't contain the topic name at the end, right?) doesn't this need a separate validation function (rmw_validate_namespace)?

It is either / or it is a valid FQN, so the topic name validation function is sufficient once I account for the / case. The code as written (and documented and tested) covers all the cases for namespace that I'm aware of. I don't think we need a separate namespace validation function myself.

A namespace can be empty. A topic name can't be. So both types have separate validation criteria, right? Therefore I would suggest to create separate validation functions for them. These implementation could share common functionality though if that makes sense.

See: ros2/rmw#95 (comment)

wjwwood · 2017-04-07T01:27:27Z

In 33e7a35 I started using rmw_validate_namespace().

@Karsten1987 I also made rcl_node_init() more flexible in 33e7a35 to allow: "", "/", and "foo/bar".

Also @Karsten1987 , another possible use case for utilities_strcat which takes an allocator:

394048d#diff-b1ed1b132e1e72fee905fe7bc3b4c62bR111

dirk-thomas

Beside the comment this LGTM.

dirk-thomas · 2017-04-07T16:45:49Z

rcl/src/rcl/node.c

+  if (validation_result != RMW_NAMESPACE_VALID) {
+    const char * msg = rmw_namespace_validation_result_string(validation_result);
+    if (!msg) {
+      msg = "unknown validation_result, this should not happen";


It would be good if the message included the value of validation_result.

Similar to the change in rmw: f0bec07

add concept of node namespace to rcl API

b889dd1

also validate node name and node namespace

wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Mar 29, 2017

wjwwood self-assigned this Mar 29, 2017

Karsten1987 reviewed Mar 29, 2017

View reviewed changes

name_space -> namespace_

d3e99fe

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 30, 2017

Karsten1987 reviewed Apr 5, 2017

View reviewed changes

Karsten1987 approved these changes Apr 5, 2017

View reviewed changes

doc clarification

dce05d6

dirk-thomas reviewed Apr 5, 2017

View reviewed changes

wjwwood mentioned this pull request Apr 7, 2017

Support node namespaces ros2/rmw#95

Merged

wjwwood added 2 commits April 6, 2017 18:25

make use of rmw_validate_namespace()

394048d

clear expected error messages in test_node.cpp

33e7a35

avoid unused invalid_index

33f084c

dirk-thomas approved these changes Apr 7, 2017

View reviewed changes

wjwwood added 3 commits April 7, 2017 15:26

include the validation result value when it is unknown

f0bec07

use _snprintf_s on windows

558b6e5

uncrustify

a87e9c0

wjwwood merged commit 64f1b16 into master Apr 8, 2017

wjwwood deleted the node_namespace branch April 8, 2017 09:04

wjwwood removed the in review Waiting for review (Kanban column) label Apr 8, 2017

ivanpauno pushed a commit that referenced this pull request Jan 2, 2020

update docs for beta 2 (#112)

c253937

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add concept of node namespace to rcl API #112

add concept of node namespace to rcl API #112

wjwwood commented Mar 29, 2017

Karsten1987 Mar 29, 2017

wjwwood Mar 30, 2017

Karsten1987 Apr 5, 2017

wjwwood Apr 5, 2017

Karsten1987 Apr 5, 2017

wjwwood Apr 5, 2017

Karsten1987 Apr 5, 2017

wjwwood Apr 5, 2017

Karsten1987 Apr 5, 2017

wjwwood Apr 5, 2017

Karsten1987 Apr 5, 2017

dirk-thomas Apr 5, 2017

wjwwood Apr 7, 2017

wjwwood Apr 7, 2017

dirk-thomas Apr 5, 2017

wjwwood Apr 5, 2017

dirk-thomas Apr 5, 2017

wjwwood Apr 7, 2017

wjwwood commented Apr 7, 2017

dirk-thomas left a comment

dirk-thomas Apr 7, 2017

wjwwood Apr 7, 2017

add concept of node namespace to rcl API #112

add concept of node namespace to rcl API #112

Conversation

wjwwood commented Mar 29, 2017

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

wjwwood commented Apr 7, 2017

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