Fixing camera_info_manager topic so that it conforms to the standard namespace

ros-drivers / gscam

ROS Camera driver for GStreamer-based video streams.

136 stars 172 forks source link

Fixing camera_info_manager topic so that it conforms to the standard namespace #16

Open jbohren opened 10 years ago

jbohren commented 10 years ago

This bug made it into the releases, but should really be fixed.

According to the ROS camera spec, the camera info topic should be on camera/set_camera_info, but instead with gscam, it's just set_camera_info. This patch fixes that, but it might break code (mostly calibration scripts) for people who have been using gscam already.

jack-oquin commented 10 years ago

While many older drivers still use the "camera" namespace, the camera prefix is not recommended practice any longer. See: REP-0135.

That REP clearly deprecates adding an "extra" level of namespace to the topic names. Services are not explicitly mentioned. I would still interpret them as working similarly.

In my opinion, this change breaks the API and should not be done in Hydro. Even changing it in Indigo should be done as a "tick-tock" update, with the old behavior supported but deprecated for an entire release cycle.

That is why camera1394 still does not comply with REP-0135. It's just too much hassle and too disruptive to make that kind of change.

jbohren commented 10 years ago

Hm. So should the fix actually be to remove the camera sub-namespace from the other topics?

jack-oquin commented 10 years ago

I don't know, haven't used this driver.

Removing the extra namespace would at least follow recommended practice, but it's tricky to do that correctly.

Either way, the compatibility issue seriously needs to be addressed. We don't want to put out a Hydro update that makes people's systems suddenly start failing.

jbohren commented 10 years ago

It's an easy fix to remove the namespace, we just have to create two image transport publishers, and emit a warning when someone subscribed to the deprecated one.

jack-oquin commented 10 years ago

Good idea.

That's probably a reasonable solution for deprecation in Indigo, with removal in J-turtle. I would not recommend changing Hydro.

I assume this driver is fairly widely used.

jbohren commented 10 years ago

Good idea.

That's probably a reasonable solution for deprecation in Indigo, with removal in J-turtle. I would not recommend changing Hydro.

That sounds good. I'll update the PR accordingly.

I assume this driver is fairly widely used.

Yeah, even on OS X now! #15

jbohren commented 10 years ago

Ok, @jack-oquin take a look at that. If it looks good, I'll branch hydro-devel out from the current master, merge this into master, and then rename master to indigo-devel

jack-oquin commented 10 years ago

It's not for me to say. You are one of the maintainers. It looks OK to me.

Since you asked: :smile:

I would only issue the deprecation warning once per subscription. Maybe that is what you are doing, I can't tell without reading the code in more detail.
For my repositories, I develop in "master" and only create "hydro-devel" if I need to release a new change to Hydro after making Indigo changes like this. Until then, I have a release tag that permanently marks the latest Hydro version. But, if you anticipate releasing further Hydro changes, you may as well go ahead and branch now.
Since there are API differences, the Indigo release should increment the minor number (to 0.2.0 in this case).
Because I am a forgetful fellow, I'd also open an issue to remove the deprecated topic, with J-turtle the target.

zacwitte commented 5 years ago

Could we merge this PR please? I have multiple cameras and end up having to remap all the topics to remove the extra /camera namespace that's hard-coded.