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

Add namespace support for the new version of Rosbridge_suite #842

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

FlorianLebecque
Copy link

@FlorianLebecque FlorianLebecque commented Feb 5, 2025

Public API Changes
Add an optional namespace parameter to the Ros class

Description
The roslibjs is broken when setting a namespace to the rosbriduge_suite since this PR on rosbridge_suite.
This change allows dev to set a namespace on the Ros object. This parameter is optional and by default an empty string. When populated, act as a prefix for the different ROSAPI services.

const ros = new ROSLIB.Ros({
    url: props.url
});

ros.namespace = "NAME_SPACE_IN_ROS/";

this allows the Roslibjs to call "NAME_SPACE_IN_ROS/rosapi/Topics" instead of "rosapi/Topics"

@MatthijsBurgh
Copy link
Contributor

The ROS class doesn't have a namespace attribute. So this should be set in the constructor. Also tests should be added so it works with absolute, relative and private namespaces. And I think it requires more parsing than just prefixing the topics/services with the namespace. As you want to be robust against a missing slash at the end of the namespace.

@EzraBrooks
Copy link
Contributor

I'd also appreciate the use of template strings instead of string concatenation here for readability :)

@FlorianLebecque
Copy link
Author

FlorianLebecque commented Feb 7, 2025

I've done change to add the namespace to the constructor.
I've added better logic for 'parsing' of the namespace. this should make it more robust to missing '/' and leading '/'

Unfortunately, I have no idea how to test it, I'am not familiar with javascript testing. Do you have any ressource I could use to add it?

Since testing it requires having multiple rosbridge_suite with different namespaces running on different port for the test.

test/namespaces.test.js Outdated Show resolved Hide resolved
src/core/Ros.js Outdated Show resolved Hide resolved
src/core/Param.js Outdated Show resolved Hide resolved
Add missing string template
Add missing EOF newline
@MatthijsBurgh
Copy link
Contributor

@FlorianLebecque check CI errors

@FlorianLebecque
Copy link
Author

@MatthijsBurgh How can I setup the test to connect to different rosbridge_suite websockets ? To test different namespace configuration ?

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.

3 participants