Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespace override with ROS2_NAMESPACE environment variable #993

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from

Conversation

alpylmz
Copy link

@alpylmz alpylmz commented Jul 14, 2022

This PR addresses the issue ros2/ros2#1259. The environment variable is chosen as ROS2_NAMESPACE. A design issue that needs to be discussed is if the ROS2_NAMESPACE variable will override all namespaces defined or will be added to the namespace. For example,

ROS2_NAMESPACE=/bar, node's namespace is /foo, should the overall namespace be /bar, or /bar/foo.

rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Show resolved Hide resolved
rcl/src/rcl/node.c Show resolved Hide resolved
@@ -121,13 +122,27 @@ rcl_node_init(
rcl_context_t * context,
const rcl_node_options_t * options)
{

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change?

@alpylmz
Copy link
Author

alpylmz commented Aug 11, 2022

I just had some time to work on this, and I realized that the remap code is executed in every service and topic creation at the runtime. We could add the namespacing after remap's, but that means adding the same code to 5 files, client.c, node_resolve_name.c, publisher.c, service.c, subscription.c.
I think a much cleaner way would be leaving this pull request almost as is, changing the launch code to not to use remaps for namespacing but use a function that we may implement at this repo(to add additional namespaces in any kind to node), and merge both of the pull requests. What about this?

@alpylmz
Copy link
Author

alpylmz commented Oct 16, 2022

This PR stayed for a long time than it really should be. Is there anyone who can comment on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants