UbiquityRobotics / raspicam_node

ROS node for camera module of Raspberry Pi
BSD 3-Clause "New" or "Revised" License
292 stars 162 forks source link

The topic names shouldn't start with raspicam_node #63

Closed ghost closed 5 years ago

ghost commented 5 years ago

Hi,

Thank you very much for the work. I have only one problem, the topic names shouldn't start with raspicam_node. The topic names should be as minimal as possible, to allow using namespaces.

In my project I want the topics to be /robotname/image, /robotname/image/compressed... Normally this should be as easy as adding a namespace on the raspicam node, but currently I have to manually remap the topics which is not future proof. This is part of ROS good practices.

Thanks in advance.

rohbotics commented 5 years ago

I think this is something we could consider for future ROS distros we release into (like melodic), but is unlikely.

But why do you say remapping topics is not future proof? It is a perfectly standard ROS feature and will not introduce any breaking changes to raspicam.

ghost commented 5 years ago

Maybe "not future proof" is not exactly how I should have explained it. What I meant, is that the launch file now needs to know every topic which is published by the node and remap it. If I was using a namespace, should you add, remove, rename a topic in the future, all the topics will still be correctly mapped.

For raspicam_node, I don't understand why the topics are prefixed with raspicam_node/. The camera name could have been an option, or nothing at all.

But I perfectly understand the problem, changing this would be a breaking change.

What do you think about adding a parameter like "prefix" with a default value "raspicam_node/" (so that we could set no prefix or set it to the camera name in the launch file.

E.g.: <param name="prefix" value="$(arg camera_name)/"/>

And then using the namespace offers a lot of flexibility. Using namespace my_robot would produce topics like: my_robot/my_camera_name/...

rohbotics commented 5 years ago

That makes a lot of sense, I think a camera prefix parameter with a default of "raspicam_node" makes a lot of sense.

Can you submit a PR with this change?

ghost commented 5 years ago

I just had a look at the code, I thought you were hardcoding the "raspicam_node" in front of the topic names, but you are actually using the node handle with a private namespace for some reason.

ros::init(argc, argv, "raspicam_node"); ros::NodeHandle n("~");

I can't use the prefix parameter I suggested because of that. I need the node handle to get the param and I would need the param to initiate the node handle...

Removing the ~ would do the trick, then users could specify whatever namespace. E.g: ns="$(arg robot_name)/$(arg camera_name)"

But this would make raspicam_node not backward compatible, unless we put a default namespace in every launch file, but should a user have created his own launch file then it wouldn't work for him anymore.

I could think of other solutions, like removing the tilt and manually prefixing the topics, but that would make the code messy.

It's a pitty that the whole node handle is set as private while most the topics are meant to be publicly accessible over ROS.

rohbotics commented 5 years ago

I think the best solution is to use a private node handle for parameters, and a global one for topics.

I am not opposed to manually prefixing the topic name with the node name if the camera_prefix is not specified.

ghost commented 5 years ago

Using two different node handles is indeed a good idea. I started implement the prefix option but it's not a good solution. The problem is that by setting the topics prefix to "camera_x", we end up with:

Topics example: "/camera_x/image" Param exaple: "/raspicam_node/the_param"

This doesn't look very intuitive. The param should be "/camera_x/raspicam_node/the_param" imho, the camera is the main prefix, the param is private and is itself prefixed with the node name.

To achieve that, I ended up with a really simple solution. Simply introducing a new parameter called "private_topics" (defaults to true).

If the parameter is true: Topics example: "/raspicam_node/image" Param example: "/raspicam_node/the_param"

If the parameter is false: Topics example: "/image" Param example: "/raspicam_node/the_param"

Then using a namespace on the node makes everything clear.

With namespace "camera_x" and "private_topics" false: Topics example: "/camera_x/image" Param example: "/camera_x/raspicam_node/the_param"

Which is probably what we want.

PR coming as soon as I have done some more tests.

ghost commented 5 years ago

64 PR Created