search

zivid / zivid-ros

Official ROS driver for Zivid 3D cameras
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

Zivid nodelet can't attach to a manager in the global namespace #84

Closed danielcranston closed 3 weeks ago

danielcranston commented 1 year ago

Summary

Hi,

I think it's great that the driver itself enforces REP-135, but I noticed a situation where this enforcement is too strict.

It has to do with the nodelet version of the driver. The method the driver uses to determine if it was started in the global namespace (the thing that REP-135 prohibits) is ros::this_node::getNamespace().

For nodelets, this resolves to the namespace of the nodelet manager, not the Zivid driver nodelet (at least in ROS Melodic). This means that, a Zivid driver nodelet, brought up in its own namespace but attached to a manager in the global namespace, will fail to start (claiming it doesn't satisfy REP-135, but in fact it does).

I made a commit with a proposed solution here: https://github.com/danielcranston/zivid-ros/commit/473bf4eb9ac7727c58f4302349b87b673e21bff9

How to reproduce

To test, you can use this minimal launchfile

<launch>
    <arg name="ns" default="/" />

    <node pkg="nodelet" type="nodelet" name="nodelet_manager" args="manager" output="screen" />
    <node ns="$(arg ns)" pkg="nodelet" type="nodelet" name="latch" args="load zivid_camera/nodelet /nodelet_manager" output="screen" />
</launch>

Notice how the manager is in the global namespace and the Zivid driver nodelet is put in $(arg ns).

roslaunch test.launch ns:=zivid fails on master, but succeeds on https://github.com/danielcranston/zivid-ros/commit/473bf4eb9ac7727c58f4302349b87b673e21bff9. Starting the nodelet (or node) in the global namespace still has the same (intended) behavior as before.

I guess one could make the argument that nodelet managers shouldn't be put in the global namespace, but I don't interpret anything in REP-135 that would discourage it, and I think it's a pretty common practice to do so.

Thanks for reading.

apartridge commented 3 weeks ago

Hi @danielcranston , thanks for your report and fix. I have added a PR for it here https://github.com/zivid/zivid-ros/pull/99 and we will aim to merge it in soon.

apartridge commented 3 weeks ago

Solved with https://github.com/zivid/zivid-ros/pull/99