[pr2eus_tutorials] fix pr2_tabletop.launch for remote PC #459

shmpwk · 2021-05-18T05:55:31Z

This PR is intended to reduce the bandwidth.
It compresses prosilica image, rgb image, depth image and point cloud
in the pr2eus_tutorials.

708yamaguchi

@shmpwk

Thank you very much for your work!

How about integrating pr2_tabletop_remote.launch into pr2_tabletop.launch?

<launch>
  <arg name="remote" default="false" />
  ...
</launch>

Then, write to README.md about the remote option.
https://github.com/jsk-ros-pkg/jsk_pr2eus/tree/master/pr2eus_tutorials#using-a-real-robot

Then, I will check this launch on real robot.

shmpwk · 2021-07-21T03:37:05Z

Thank you for your advice! So like this? I'm not sure the grammer "unless" and "if".

708yamaguchi · 2021-07-21T03:53:17Z

Yes, nice.
You can see the overview of if and unless attributes here.
https://answers.ros.org/question/208696/conditional-remap/

I think we can use <group> tag for following lines.

<group if=$(arg remote)>
 ...
</group>

https://github.com/jsk-ros-pkg/jsk_pr2eus/pull/459/files#diff-36099b04ef7b0fe95ec8ace80185317d01dfa265f637614eb64819bb0f7baa57R51-R66

As another point, I found that we may need to prepare pr2_tabletop.rviz and pr2_tabletop_remote.rviz because image and pointcloud names are different if we use remote arg.

shmpwk · 2021-07-21T04:39:26Z

Thank you for your comment.
I add <group> tag and rviz for remote PC.
I don't confirm it works until end of this week, sorry.

708yamaguchi · 2021-07-21T04:45:15Z

OK, at last, please update README.md and remove pr2_tabletop_remote.launch

shmpwk · 2021-07-21T04:56:36Z

OK done.

708yamaguchi · 2021-07-21T05:56:56Z

Could you also write your remote usage here?

If you try using compressed topic from the real PR2,
\```
# on local machine
roslaunch pr2eus_tutorials pr2_tabletop.launch remote:=true
\```

https://github.com/jsk-ros-pkg/jsk_pr2eus/tree/master/pr2eus_tutorials#using-a-real-robot

Then, I think this PR is perfect.

I will check this PR today.

708yamaguchi · 2021-07-21T16:55:39Z

pr2eus_tutorials/launch/pr2_tabletop.launch

@@ -5,11 +5,25 @@
  <arg name="manager" default="pr2_tabletop_object_detector_nodelet_manager"/>
  <arg name="machine" default="c2" />


If you use this launch file from remote, machine is localhost?

Fix with 791eb86

708yamaguchi · 2021-07-21T16:56:32Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <arg name="PROSILICA_IMAGE" value="/prosilica/image_raw" /> 
+  <arg name="PROSILICA_IMAGE_REMOTE" value="/prosilica_remote/image_raw" /> 
+  <arg name="POINT_CLOUD" value="/kinect_head/depth_registered/throttled/points" />
+  <arg name="POINT_CLOUD_REMOTE" value="/kinect_head_remote/depth_registered/points" />

  <include file="$(find pr2_machine)/$(env ROBOT).machine" unless="$(arg load_machine)" />


We do not need this line if we use remote?

Thank you, you mean summing up remote topics and the other topics with <group> tag is more readable ?

Or you mention <include file="$(find pr2_machine)/$(env ROBOT).machine" unless="$(arg load_machine)" /> ?

Or you mention ?

Yes, I mention this line.

Ok, fix by 791eb86

708yamaguchi · 2021-07-21T16:57:28Z

pr2eus_tutorials/launch/pr2_tabletop.launch

-        machine="$(arg machine)">
-    <remap from="~points" to="/kinect_head/depth_registered/throttled/points" />
-    <remap from="~point" to="/kinect_head/rgb/throttled/image_rect_color/screenpoint" />
+  <group if="$(arg remote)">


I think <group if="$(arg remote)"> should include xxx_decompress nodes and depth_image_proc nodes, not screenpoint.

What a mess!!! Fix with 7fbc86e and 7fda6c2

708yamaguchi · 2021-07-21T17:01:25Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  </group>
+
+  <node name="prosilica_decompress" pkg="image_transport" type="republish"
+        args="compressed in:=$(arg PROSILICA_IMAGE) out:=$(arg PROSILICA_IMAGE_REMOTE)" />


in and out should be in remap tag
args="compressed raw"

example:
https://github.com/jsk-ros-pkg/jsk_recognition/blob/b5657ed28937616d2abf3d3f967c69ccf7a85f28/jsk_pcl_ros/launch/openni2_remote.launch#L71-L76

http://wiki.ros.org/image_transport

Thank you for the review. You mean using remap tag enables more readable ? Putting them together with args is not formal ?

You mean using remap tag enables more readable ? Putting them together with args is not formal ?

Yes, I think using remap tag is readable and easy to understand.

args can do many roles (e.g. rosparam, remap, special keys, ...).
So I think using specific tag is easy to understand.
https://wiki.ros.org/Remapping%20Arguments

I see, thank you for the reference. Fix with 78880de

708yamaguchi · 2021-07-21T17:02:31Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <node name="prosilica_decompress" pkg="image_transport" type="republish"
+        args="compressed in:=$(arg PROSILICA_IMAGE) out:=$(arg PROSILICA_IMAGE_REMOTE)" />
+  <node name="rgb_decompress" pkg="image_transport" type="republish"
+        args="compressed in:=$(arg RGB_IMAGE) out:=$(arg RGB_IMAGE_REMOTE)" />


in and out should be in remap tag
args="compressed raw"

example:
https://github.com/jsk-ros-pkg/jsk_recognition/blob/b5657ed28937616d2abf3d3f967c69ccf7a85f28/jsk_pcl_ros/launch/openni2_remote.launch#L71-L76

http://wiki.ros.org/image_transport

Fix by 78880de

708yamaguchi · 2021-07-21T17:03:02Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <node name="rgb_decompress" pkg="image_transport" type="republish"
+        args="compressed in:=$(arg RGB_IMAGE) out:=$(arg RGB_IMAGE_REMOTE)" />
+  <node name="depth_decompress" pkg="image_transport" type="republish"
+        args="compressedDepth in:=$(arg DEPTH_IMAGE) out:=$(arg DEPTH_IMAGE_REMOTE)" />


in and out should be in remap tag
args="compressedDepth raw"

example:
https://github.com/jsk-ros-pkg/jsk_recognition/blob/b5657ed28937616d2abf3d3f967c69ccf7a85f28/jsk_pcl_ros/launch/openni2_remote.launch#L80-L85

http://wiki.ros.org/image_transport

Fix by 78880de

708yamaguchi · 2021-07-21T17:06:25Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <arg name="PROSILICA_IMAGE" value="/prosilica/image_raw" /> 
+  <arg name="PROSILICA_IMAGE_REMOTE" value="/prosilica_remote/image_raw" /> 
+  <arg name="POINT_CLOUD" value="/kinect_head/depth_registered/throttled/points" />
+  <arg name="POINT_CLOUD_REMOTE" value="/kinect_head_remote/depth_registered/points" />


If you use kinect_head and kinect_head_remote many times, you can use tag
(If you do not like the group tag, I think it's ok not to use the group tag)

example:
https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/jsk_pcl_ros/launch/openni2_remote.launch

I agree with you.

Fix with 8183683

shmpwk · 2021-07-26T15:48:03Z

Thank you for thorough review @708yamaguchi -san, I fixed code you mentioned. I will confirm it works with real PR2 this week end. Please wait for a while.

knorth55 · 2021-07-26T16:20:34Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <arg name="load_machine" default="false" unless="$(arg remote)" />
+  <arg name="load_machine" default="true" if="$(arg remote)" />
+  <arg name="machine" default="c2" unless="$(arg remote)" />
+  <arg name="machine" default="localhost" if="$(arg remote)" />


default -> value

Thank you for your review.
Fix with 391e0bd

pr2_tabletop.launch is called by pr2_tabletop_sim.launch which is gazebo simulator tabletop tutorial. So I've changed the code back from value to default. ( 408fcec )

this is because you add remote arg in this PR.
in that case, you should change pr2_tabletop_sim.launch to use remote arg.
otherwise, machine arg and remote arg will conflict, which may be confusing.

I suspect not.
When I execute pr2_tabletop_sim.launch, machine arg and remote arg aren't conflicted.
This is because machine arg in pr2_tabletop.launch is always overwritten by that in pr2_tabletop_sim.launch no matter which machine arg is.
Also, settingremote arg true in pr2_tabletop_sim.launch might makes users confused since remote access is not related to gazebo simulation.

そういう意味ではなく，ややこしいのでremoteを追加するんだったら，そちらに統一，そうじゃないのであればmachineを使う，という風に統一してください．
あとの人がこんがらがるので．

pr2_tabletop_sim.launch から pr2_tabletop.launch を呼び出すときに，remote arg を true にして良いということでしょうか．
その場合，procilica, rgb, depth をdecompress してpointcloudを再構成しますでしょうか．
私としては，pr2_tabletop_sim.launch では，machine arg を localhostに，load_machine arg を true にするという意味では，ご指摘なさった部分は，remote arg が true となるのですが，
procilica, rgb, depth をdecompress してpointcloudを再構成する必要性はないと思っており，この意味ではremote arg は false に相当すると考えています．
その場合，remote arg とは別に，use_sim arg を新たに作って場合分けするべきでしょうか．

@knorth55 さんに修正して頂き，use_sim ではなくsim arg を新たに作って場合わけをすることになりました．

shmpwk · 2021-07-31T12:25:12Z

I confirmed it works with real PR2 both via ssh and on remote PC.
Following the pr2eus_tutorials, I also confirmed that by clicking object on rviz or on image_view2, PR2 tries to pick up an object.

708yamaguchi · 2021-08-01T12:27:52Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <arg name="PROSILICA_IMAGE" value="/prosilica/image_raw" /> 
+  <arg name="PROSILICA_IMAGE_REMOTE" value="/prosilica_remote/image_raw" /> 
+  <arg name="POINT_CLOUD" value="/$(arg camera_name)/depth_registered/throttled/points" />
+  <arg name="POINT_CLOUD_REMOTE" value="/$(arg camera_name)_remote/depth_registered/points" />


<arg name="DEPTH_IMAGE" value="/$(arg camera_name)/depth_registered/image_rect" />

should be

<arg name="DEPTH_IMAGE" value="/$(arg camera_name)/depth_registered/throttled/image_rect" />

?

Yes, thank you.
Fix by bc63138 .

708yamaguchi · 2021-08-01T12:28:48Z

I left 1 comment, but this PR works nicely with ssh mode and remote mode.

Thank you for your work!

shmpwk · 2021-08-02T01:05:53Z

Again I confirmed it works with gazebo mode, ssh mode and remote mode.

knorth55 · 2021-08-02T02:59:23Z

pr2eus_tutorials/launch/pr2_tabletop.launch

+  <arg name="load_machine" default="false" unless="$(arg remote)" />
+  <arg name="load_machine" default="true" if="$(arg remote)" />
+  <arg name="machine" default="c2" unless="$(arg remote)" />
+  <arg name="machine" default="localhost" if="$(arg remote)" />


this is because you add remote arg in this PR.
in that case, you should change pr2_tabletop_sim.launch to use remote arg.
otherwise, machine arg and remote arg will conflict, which may be confusing.

shmpwk · 2021-08-02T10:50:54Z

Thank you for the thorough revise @knorth55.
By 812bf41 , args have first been classified in detail.
I confirmed the revised program works well with gazebo mode, ssh mode and remote mode.

708yamaguchi · 2021-08-09T14:10:00Z

pr2eus_tutorials/launch/pr2_tabletop.launch

-  <arg name="PROSILICA_IMAGE_REMOTE" value="/prosilica_remote/image_raw" /> 
-  <arg name="POINT_CLOUD" value="/$(arg camera_name)/depth_registered/throttled/points" />
-  <arg name="POINT_CLOUD_REMOTE" value="/$(arg camera_name)_remote/depth_registered/points" />
+  <arg name="robot" value="$(optenv ROBOT sim)" if="$(eval arg('remote') or arg('sim'))" /> 


You use eval arg, but eval can be used in kinetic or newer.
http://wiki.ros.org/roslaunch/XML

Our PR2 is indigo, so I think we cannot use this launch file with SSH mode.
(I am sorry if my understanding is wrong)

that's true.

The solution I only come up with is use if arg or unless arg instead of eval arg?

The solution I only come up with is use if arg or unless arg instead of eval arg?

I don't have any other ideas either....

By the way, do we really need load_machine arg?
If we need to use load_machine, please update README.md.

<include file="$(find pr2_machine)/$(arg robot).machine" if="$(arg load_machine)" />

Sorry to be late.

By the way, do we really need load_machine arg?

No need anymore. I deleted it by 557aa9b .

You use eval arg, but eval can be used in kinetic or newer.

I change if arg or unless arg instead of eval arg by 5687bb5.

I've never seen this writing style before, but it's interesting.

I hope this PR works well.

Yes, I confirmed it works well with gazebo mode, ssh mode and remote mode.

compress topic

1a7abc3

shmpwk force-pushed the topic-compress branch from 86e246d to 1a7abc3 Compare June 17, 2021 12:20

shmpwk changed the title ~~compress images and points~~ [pr2eus_tutorials] add pr2_tabletop_remote.launch for remote PC Jun 17, 2021

Merge branch 'master' into topic-compress

337c099

k-okada requested review from 708yamaguchi and knorth55 July 21, 2021 00:39

k-okada added the PR/WaitForReview label Jul 21, 2021

708yamaguchi reviewed Jul 21, 2021

View reviewed changes

shmpwk and others added 2 commits July 21, 2021 12:11

change prosilica_decompress output topic name

103876c

for remote PC

615e322

fix

830a7ed

fix rviz for remote PC

401883f

shmpwk added 3 commits July 21, 2021 13:53

edit readme for remote PC

9c10e4a

merge pr2_tabletop_remote.launch into pr2_tabletop.launch

4032c4e

fix

9269486

708yamaguchi reviewed Jul 21, 2021

View reviewed changes

shmpwk added 2 commits July 25, 2021 16:46

edit readme

23d3761

fix

791eb86

shmpwk added 4 commits July 25, 2021 23:09

rm group tag

7fbc86e

add group tag

7fda6c2

sum up kinect_head with tag

8183683

use remap tag

78880de

knorth55 requested changes Jul 26, 2021

View reviewed changes

shmpwk added 6 commits July 27, 2021 07:32

change arg tag attributes from default to value

391e0bd

fix readme

97c69a1

fix readme

0e5576c

change arg tag attributes from valie to default

408fcec

use throttled topic for image_view2

c0cd1d2

edit readme at image_view2

426c792

708yamaguchi reviewed Aug 1, 2021

View reviewed changes

fix depth image topic name

bc63138

708yamaguchi approved these changes Aug 2, 2021

View reviewed changes

knorth55 requested changes Aug 2, 2021

View reviewed changes

set args in more detail in advance

812bf41

shmpwk requested review from 708yamaguchi and knorth55 August 9, 2021 13:55

708yamaguchi reviewed Aug 9, 2021

View reviewed changes

shmpwk force-pushed the topic-compress branch from eabc88c to 812bf41 Compare August 11, 2021 03:15

shmpwk added 2 commits August 11, 2021 12:22

not use eval use if unless

5687bb5

rm load_machine

557aa9b

shmpwk changed the title ~~[pr2eus_tutorials] add pr2_tabletop_remote.launch for remote PC~~ [pr2eus_tutorials] fix pr2_tabletop.launch for remote PC Aug 11, 2021

knorth55 approved these changes Aug 11, 2021

View reviewed changes

k-okada merged commit ca2d919 into jsk-ros-pkg:master Nov 19, 2021

		@@ -5,11 +5,25 @@
		<arg name="manager" default="pr2_tabletop_object_detector_nodelet_manager"/>
		<arg name="machine" default="c2" />

[pr2eus_tutorials] fix pr2_tabletop.launch for remote PC #459

[pr2eus_tutorials] fix pr2_tabletop.launch for remote PC #459

Conversation

shmpwk commented May 18, 2021 • edited Loading

708yamaguchi left a comment

Choose a reason for hiding this comment

shmpwk commented Jul 21, 2021

708yamaguchi commented Jul 21, 2021

shmpwk commented Jul 21, 2021

708yamaguchi commented Jul 21, 2021

shmpwk commented Jul 21, 2021

708yamaguchi commented Jul 21, 2021

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

708yamaguchi Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

708yamaguchi Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

708yamaguchi Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

708yamaguchi Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

708yamaguchi Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk commented Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk commented Jul 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

708yamaguchi commented Aug 1, 2021

shmpwk commented Aug 2, 2021

Choose a reason for hiding this comment

shmpwk commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

shmpwk Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmpwk commented May 18, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

708yamaguchi Jul 21, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

708yamaguchi Jul 21, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

708yamaguchi Jul 26, 2021 •

edited

Loading

shmpwk Jul 26, 2021 •

edited

Loading

708yamaguchi Jul 21, 2021 •

edited

Loading

708yamaguchi Jul 21, 2021 •

edited

Loading

shmpwk Jul 25, 2021 •

edited

Loading

shmpwk commented Jul 26, 2021 •

edited

Loading

shmpwk Jul 26, 2021 •

edited

Loading

shmpwk Jul 31, 2021 •

edited

Loading

shmpwk Aug 2, 2021 •

edited

Loading

shmpwk Aug 11, 2021 •

edited

Loading

shmpwk Aug 11, 2021 •

edited

Loading