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

prevent calling ros::init without name; use node_name command option instead of namespace #44

Conversation

furushchev
Copy link
Contributor

Currently launching naoqi_driver_node without --namespace option causes mis-registering node name to ros master.

e.g. when launching by roslaunch pepper_bringup pepper_full.launch (ros_naoqi_driver is executed without --namespace option and under ns="pepper_robot"), the node name of ros_naoqi_driver is shown as //pepper_robot and ROS master cannot resolve address of this illegal ros node name.

$ rostopic info /cmd_vel
Type: geometry_msgs/Twist

Publishers: 
 * /teleop_twist_joy (http://133.11.216.136:44116/)
 * /pepper_1444291883382908575 (http://133.11.216.136:54125/) (Note: This is from pepper-interface.l)

Subscribers: 
 * //pepper_robot (unknown address //pepper_robot)  # this is invalid for ROS

Looking more deeply, I found that the value of command line option --namespace(https://github.com/ros-naoqi/naoqi_driver/blob/master/src/external_registration.cpp#L45) is passed to ros::init (https://github.com/ros-naoqi/naoqi_driver/blob/master/src/ros_env.hpp#L92) whose name must NOT be empty, so I added exception handler.
Also I'd like to point out that argument name --namespace is inappropriate and it is better to rename it to --node_name because the value of this option is passed to ros::init.

Review on Reviewable

@furushchev furushchev changed the title prevent calling ros::init without name; use node_name command option … prevent calling ros::init without name; use node_name command option instead of namespace Oct 10, 2015
@k-okada
Copy link
Member

k-okada commented Oct 14, 2015

k-okada@9425caa is another implementation for this issue.

@vrabaud
Copy link
Contributor

vrabaud commented Oct 15, 2015

@furushchev furushchev force-pushed the use-node-name-instead-of-namespace branch from 9f36290 to 0166089 Compare October 17, 2015 09:54
@furushchev
Copy link
Contributor Author

@vrabaud Thanks for advice. I agree that using __name is better. I fixed.

("network_interface,i", po::value<std::string>()->default_value("eth0"), "set the network interface over which to connect");

all_desc.add_options()
("node_name,n", po::value<std::string>()->default_value("naoqi_driver"), "this node name");
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for that line anymore right ?

@vrabaud
Copy link
Contributor

vrabaud commented Oct 21, 2015

@furushchev , node_name is not a command line argument anymore right ? As we use the __name from ROS (that the launch file will implicitly set).
Therefore, there is no need for all_desc right ?

@Karsten1987
Copy link
Contributor

as far as I know, there is no real API for fetching the __name parameter without calling ros::init.

That being said, here is a little function which parses the argc/argv pair for the __name argument.

std::string filterName( int argc, char** argv )
{ 
  for (size_t i=0; i<argc; ++i)
  {
    std::string arg(argv[i]);
    size_t pos = arg.find(":=");
    if (arg.substr(0,pos) == "__name")
    {
      return arg.substr(pos+2);
    } 
  } 
  return "";
} 

@furushchev
Copy link
Contributor Author

@vrabaud Yes. It is actually hidden option, that's why I use 2 options_descriptions: desc for command line options (they are shown when with argument -h/--help), and all_desc (desc + node_name just for enabling external parser of __name)
Option node_name is not shown on --help.

@furushchev
Copy link
Contributor Author

ping

1 similar comment
@furushchev
Copy link
Contributor Author

ping

@k-okada
Copy link
Member

k-okada commented Oct 30, 2015

i think this PR contains two different things, to prevent error when no
namespace is given, and enable to change nodename (and use them for
namespace)
we should create two different PR for each of them.

◉ Kei Okada

On Fri, Oct 30, 2015 at 10:57 AM, Furushchev notifications@github.com
wrote:

ping


Reply to this email directly or view it on GitHub
#44 (comment)
.

@Karsten1987
Copy link
Contributor

The PR got merged, so there is an official ros method ros::getROSArg now to parse any ROS argument.
Have a look at here ros/ros_comm@5db2088

@vrabaud
Copy link
Contributor

vrabaud commented Dec 27, 2015

Does that PR still make sense ? Please rebase first.

@Karsten1987
Copy link
Contributor

ping?

@k-okada
Copy link
Member

k-okada commented Jan 8, 2016

i think we can use #58 for
this issue, using command option instead of namespace may big change so we
need more discussion.

◉ Kei Okada

On Fri, Jan 8, 2016 at 4:28 PM, Karsten Knese notifications@github.com
wrote:

ping?


Reply to this email directly or view it on GitHub
#44 (comment)
.

@vrabaud vrabaud closed this Jan 10, 2016
@furushchev furushchev deleted the use-node-name-instead-of-namespace branch January 10, 2016 13:54
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