strands-project / strands_systems

all system configs
0 stars 16 forks source link

Switch to openni2_launch for cameras. #114

Closed cburbridge closed 8 years ago

cburbridge commented 8 years ago

This might effect other components if they have not been released since the review. Notably metric maps @RaresAmbrus ?

marc-hanheide commented 8 years ago

Do we have a list of packages effected? From the review having maybe?

cburbridge commented 8 years ago

Charging works fine, as do metric maps... let's merge this?

marc-hanheide commented 8 years ago

we should make that merge soon, I suppose. Did you test people perception and V4R stuff? If that works as well I'm happy to merge it.

cburbridge commented 8 years ago

It works with V4R, @cdondrup please can you confirm that this does not break people perception?

cdondrup commented 8 years ago

I will test it again but it worked well during the review. Just have to change one of the default topic names.

marc-hanheide commented 8 years ago

I'm just extra cautious as I need the robot to be working for BBC breakfast on Monday ;-)

marc-hanheide commented 8 years ago

@cdondrup ready for merge?

cdondrup commented 8 years ago

Let me quickly test this on linda. Slipped my mind I have to admit.

cdondrup commented 8 years ago

A few minor things:

Long story short, for the people perception to work, I just need to change the default depth topic in the launch files once this is merge. So please go ahead after the other issues are resolved.

lucasb-eyer commented 8 years ago

Yeah, we started running with openni2 yesterday and no obvious problems so far. I also noticed the change in box-color and was wondering - good to know why!

The head-orientation node will probably become just slightly worse, but I'm close to completely replacing it anyways.

cdondrup commented 8 years ago

See https://github.com/strands-project/strands_perception_people/pull/174

nilsbore commented 8 years ago

I might have to change some topics in the logging, possibly switching from openni_wrapper to openni2_launch for re-creating the point clouds from the saved images.

cdondrup commented 8 years ago

Any updates on this?

bfalacerda commented 8 years ago

can we merge this, I just had some problems because of it not being in

marc-hanheide commented 8 years ago

I'm still unclear if all related changes have been done. But maybe now is a good time to enforce it...

bfalacerda commented 8 years ago

the new version (still unreleased) of strands_movebase needs openni2_launch. Not sure about all the other camera stuff

nilsbore commented 8 years ago

I'm all for merging this and clear up everything needed as we go along. As @bfalacerda said, strands_movebase kinda depends on this being merged.

@cburbridge Why is there a need for a separate openni2.launch file in this package?

cburbridge commented 8 years ago

It is so that we have machine tags compatible with the previous strands_cameras.launch.

marc-hanheide commented 8 years ago

So, good to merge?

nilsbore commented 8 years ago

We've been running it on Rosie (and it works), though I can't get the machine tags to work for some reason. I have the same problem with other launch files though.

marc-hanheide commented 8 years ago

so... I'm about to re-relases scitos_apps, also including this commit: https://github.com/strands-project/scitos_apps/commit/df7aea03f367933eb8171b8930cabce60349bc0a:

    ROS_INFO("subscribing to camera topics");
    image_transport::ImageTransport it(*nh);
    subim = it.subscribe("head_xtion/rgb/image_mono", 1, imageCallback);
-   subdepth = it.subscribe("head_xtion/depth/image_rect", 1, depthCallback);
+   subdepth = it.subscribe("head_xtion/depth/image_rect_raw", 1, depthCallback);
    subcamera = nh->subscribe("head_xtion/rgb/camera_info", 1,cameraInfoCallBack);

so, it will now also assume the new topic names. This needs to be addresses in the code, I suppose. I won't accep-t the re-release until somebody confirms I should...

marc-hanheide commented 8 years ago

This is the PR for re-releasing scitos_apps as requested by @cburbridge: https://github.com/strands-project/rosdistro/pull/579

marc-hanheide commented 8 years ago

Can @cburbridge, @cdondrup, @lucasb-eyer or @Pandoro, and @nilsbore comment, please? I really lost track what we did change and what not. And if I merge https://github.com/strands-project/rosdistro/pull/579 at least the topic for charging will have changed in the released version. I don't want create an unstable released system. So, please let me know where we stand on this.

cdondrup commented 8 years ago

I just tested the charging using the openni2 on Linda. By default it subscribes to:

/head_xtion/rgb/image_mono
/head_xtion/depth/image_rect
/head_xtion/rgb/camera_info

and works as intended. Hence, the commit should be removed.

cburbridge commented 8 years ago

I used this for a while and with the currently released docking code without problem, so agree this review-time commit "Fixing Chaos" should not be released.

nilsbore commented 8 years ago

We have also been using openni2_launch with the meta rooms and for chest navigation, works as intended. We have not been running docking though.

marc-hanheide commented 8 years ago

ok see https://github.com/strands-project/scitos_apps/pull/151 I will re-release once merged.

ferdianjovan commented 8 years ago

Can we merge this now?

marc-hanheide commented 8 years ago

Can people please comment on this to finally get round it, asking @gestom @Jailander @lucasb-eyer @Pandoro @nilsbore. Please confirm.

lucasb-eyer commented 8 years ago

We've been running with openni2 (not this specific PR) since my last comment, so from Aachen's side this should be fine.

nilsbore commented 8 years ago

I'm all for this! Need this for new chest cam navigation.

marc-hanheide commented 8 years ago

Right, and I want to things break now rather than later. So here we go.

hawesie commented 8 years ago

On 27 Jan 2016, at 15:45, Marc Hanheide notifications@github.com wrote:

Right, and I want to things break now rather than later. So here we go.

:)