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

Add convert_metric nodes to depth_registered.launch.xml #13

Merged
merged 4 commits into from Oct 14, 2014

Conversation

@kbogert
Copy link
Contributor

kbogert commented Sep 14, 2014

Fixes #11

@jbohren

This comment has been minimized.

Copy link
Member

jbohren commented Oct 3, 2014

+1

@piyushk

This comment has been minimized.

Copy link
Contributor

piyushk commented Oct 3, 2014

Sorry for the delay. I've been away for the summer.

I'll merge the PR next week when I can run some tests. Is it absolutely essential to have this PR merged into Hydro? I would prefer to make updates only to the Indigo release if possible.

@kbogert

This comment has been minimized.

Copy link
Contributor Author

kbogert commented Oct 4, 2014

This bug is annoying since there's no immediately apparent explanation for why the image topic isn't published when the registered depth image is used, but merging into indigo is fine.

@piyushk

This comment has been minimized.

Copy link
Contributor

piyushk commented Oct 8, 2014

Could you redirect the PR to Indigo? Additionally, if you have the ability to test in Indigo, please run a test before submitting the PR. I don't have Indigo setup here.

Thanks!

@kbogert

This comment has been minimized.

Copy link
Contributor Author

kbogert commented Oct 14, 2014

After all the trouble I went through to try and get a turtlebot2 and kinect working with Indigo, I would appreciate it if you would consider merging this PR as it looks like we'll be sticking with hydro for the foreseeable future.

@jbohren

This comment has been minimized.

Copy link
Member

jbohren commented Oct 14, 2014

@kbogert Out of curiosity, why are you staying on Hydro for now?

@kbogert

This comment has been minimized.

Copy link
Contributor Author

kbogert commented Oct 14, 2014

We run a number of turtlebot 1 & 2's with laptops currently with hydro on 12.04. After spending the better part of today with a test laptop first trying to get the indigo turtlebot packages to compile and then fighting with openni/sensorkinect (see #14) there's no way I'm doing an upgrade until things work out of the box like they do with hydro.

@piyushk

This comment has been minimized.

Copy link
Contributor

piyushk commented Oct 14, 2014

@kbogert I understand your concern. We're not moving away from Hydro as well for the same reason :). I'm just a bit scared about breaking openni_launch for anyone, since Hydro has been stable for a while. Since this PR is only adding new nodes, I think it's reasonable to introduce it into Hydro with minor version revision.

I've made the following changes to your pull request and submitted a PR to kbogert/hydro-devel (kbogert#1). It makes the following changes:

  1. There is an error with your current PR, since depth_registered/image_rect_raw does not exist. There are separate s/w and h/w processing pipelines, and I've changed the PR to add convert metric nodes in each pipeline:
  • depth_registered/hw_registered/image_rect_raw -> depth_registered/hw_registered/image_rect
  • depth_registered/sw_registered/image_rect_raw -> depth_registered/sw_registered/image_rect

Unfortunately, we cannot map these to the same topic, or else the driver starts throwing out a bunch of warnings. Is this acceptable?

  1. Fixed some minor indentation issues.

If the changes seem fine with you, please accept the PR and ping this thread again. We can then merge these changes into Indigo in a separate commit.

…tric

fixed rect convert metric to conform to both s/w and h/w pipelines. fixe...
piyushk added a commit that referenced this pull request Oct 14, 2014
Add convert_metric nodes to depth_registered.launch.xml
@piyushk piyushk merged commit 37dd0b8 into ros-drivers:hydro-devel Oct 14, 2014
@kbogert

This comment has been minimized.

Copy link
Contributor Author

kbogert commented Oct 14, 2014

Thank you! One thing to note is that I'm only able to test the hw_registered nodes. The software ones don't seem to work for me, as it seems when the registered processing is enabled the unregistered nodes (which the sw_registered uses) aren't ever published.

@piyushk

This comment has been minimized.

Copy link
Contributor

piyushk commented Oct 14, 2014

Thanks for testing! That's correct and expected behavior. I tested the sw_registered topics with registered processing disabled, and it seems to be running as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.