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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jbohren
Copy link
Member

@jbohren jbohren commented May 21, 2014

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
Copy link
Member

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
Copy link
Member Author

jbohren commented May 21, 2014

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

@jack-oquin
Copy link
Member

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
Copy link
Member Author

jbohren commented May 21, 2014

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
Copy link
Member

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
Copy link
Member Author

jbohren commented May 21, 2014

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

This adds a new image_transport topic set without the additional
`/camera/` namespace.

If one subscribes to the topics with `/camera/` in them, it will
issue a warning over rosconsole.
@jbohren
Copy link
Member Author

jbohren commented May 21, 2014

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
Copy link
Member

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

Since you asked: 😄

  • 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
Copy link

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.

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.

3 participants