search

robotology / yarp-ros

YARP support for ROS1.
1 stars 1 forks source link

No error when tcpros is not compiled, but yarp node is being used to publish to ROS topic #9

Open Tobias-Fischer opened 6 years ago

Tobias-Fischer commented 6 years ago

Hi, We were playing around today to set up the joint state publisher using the ControlBoardWrapper as described in http://www.yarp.it/classyarp_1_1dev_1_1ControlBoardWrapper.html

It took us a while to realize that we did not get an output on the ROS topic because we did not have the ENABLE_yarpcar_tcpros flag enabled. The ROS topic was created just fine, and rostopic info showed the proper type etc.; there was just no output.

It would be great if there was a warning / error message somewhere if that happens.

Best, Tobi

randaz81 commented 6 years ago

Hi, Tobias, thank you for reporting this issue. Please note that similar problems may happen also if you have not properly set the ip addresses of the machines in your /etc/hosts files or if yarp is bound to 127.0.1.1 instead of the ip address of your network interface. This is another issue frequently reported by users.

Nicogene commented 6 years ago

Related to robotology/yarp#722 and robotology/yarp#1679 .

Tobias-Fischer commented 6 years ago

Hi @Nicogene, Thanks for the heads up! Unfortunately I don't think this is fixed by https://github.com/robotology/yarp/pull/1679 as this PR only addresses the yarpserver but not the client machines. In my case, the machine running yarpserver --ros had everything installed correctly, but another machine (pc104 ....) which was supposed to send messages to the ROS topic was missing tcpros. Is there any way of catching this?

EDIT: One idea - when initializing a yarp::os::Node, one could do a similar check as in the PR above and issue a warning of the carriers do not exist. I'm happy to create a PR if that sounds viable.

Nicogene commented 6 years ago

Hi @Tobias-Fischer ! yes you are right :confused:

One idea - when initializing a yarp::os::Node, one could do a similar check as in the PR above and issue a warning of the carriers do not exist. I'm happy to create a PR if that sounds viable.

Yes it could be a nice idea! My only concern is that the overhead introduced by the call of yarp::os::Carriers::listCarriers in the initialization of every yarp::os::Node :thinking: @randaz81 what do you think about it?

traversaro commented 6 years ago

Related issue on the nature of yarp::os::Node : https://github.com/robotology/yarp/issues/1501 .

barbalberto commented 6 years ago

@Tobias-Fischer: On the client should not be necessary. Each carrier is searched for as a plugin, and if the plugin is not found an error is already printed. For example, this is what I get if I don't have the tcpros carrier:

yarp connect /w /r tcpros
Cannot load plugin from shared library (yarp_tcpros)
(yarp_tcpros: cannot open shared object file: No such file or directory)
yarp: Failed to find library support for carrier tcpros
yarp: Could not find carrier "tcpros"
Failure: no way to make connection /w->tcpros://r

or if I try to use a non existing carrier:

yarp connect /w /r vsdvsd
[ERROR]Cannot find "vsdvsd" plugin (not built in, and no .ini file found for it)
[ERROR]Check that YARP_DATA_DIRS leads to at least one directory with plugins/vsdvsd.ini or share/yarp/plugins/vsdvsd.ini in it
Cannot load plugin from shared library ()
(yarp_vsdvsd: cannot open shared object file: No such file or directory)
yarp: Failed to find library support for carrier vsdvsd
yarp: Could not find carrier "vsdvsd"
Failure: no way to make connection /w->vsdvsd://r

So that check is already implemented at the connection level. Can you provide more details and an example about your failure case?

Tobias-Fischer commented 6 years ago

Hi @barbalberto, I wanted to publish the state information of the iCub to ROS via the ControlBoardWrapper. I am not familiar enough to know how the connections between yarp and ROS are established, but there is no yarp connect command needed. Hence I do not think one can observe the error you point out in this specific case, but please do let me know if I just overlooked it.

drdanz commented 6 years ago

One idea - when initializing a yarp::os::Node, one could do a similar check as in the PR above and issue a warning of the carriers do not exist. I'm happy to create a PR if that sounds viable.

This could be a good idea, but Node and Publisher/Subscriber are supposed to work even if ROS is not enabled, therefore we should add some kind of check to see if the server is running with --ros or not. Perhaps, we could check if the port /ros exists...

drdanz commented 6 years ago

My only concern is that the overhead introduced by the call of yarp::os::Carriers::listCarriers in the initialization of every yarp::os::Node :thinking:

I don't think this is a big issue, since Node is initialized only once in each program...

Nicogene commented 6 years ago

therefore we should add some kind of check to see if the server is running with --ros or not. Perhaps, we could check if the port /ros exists...

:+1: But we have to be sure that the /ros port is our guy, through not allowing to open an /ros port by other executables (port name validation, robotology/yarp-ros#15 robotology/yarp#1508) or through setting a property to port (like the nameserver port)

drdanz commented 6 years ago

This is strongly related to the issues with the known issues of the /ros port (deleted on yarp clean, replaced if someone registers a new /ros port, etc).

I'd start adding a property to the /ros port, and this check on Node, and fix these issues later.

An alternative could be adding some specific command to the nameserver protocol... i.e. in node init isROSEnabled, reply true or false.

This check could also replace all the ROS configuration in the .ini files, forcing to a single point where you can choose whether to enable ROS or not...

barbalberto commented 6 years ago

@Tobias-Fischer The example was just to have a simple test. The procedure underneath is the same, you should see those those error raised by the ControlBoardWrapper. If it not the case let us know