[WIP] Use grouped low level components for simulation #583

fmessmer · 2017-01-11T19:46:17Z

builds on top of #575
I then introduced a sim launch-argument and made use of it in cob_bringup/robot.xml

There I followed the scheme:
Within each machine-tag:

first there is a hardware-specific group unless="$(arg sim)", where hardware specific nodes are started
followed by a block of "togglable" component launch files where the sim argument is passed
ending in a block of nodes that can be used identically within hardware and simulation

Afterwards I removed from cob_configuration_controller_gazebo/default_controllers_robot.launch all the parts that are now covered by the new robot.xml file. What is left there is simulation-specific.

To use this in simulation ipa320/cob_simulation#131 is required

This is part of #437

fmessmer · 2017-01-12T14:16:47Z

grouping legacy components and applying new structure for cob3-2

fmessmer · 2017-01-12T14:17:07Z

@ipa-mdl @ipa-fmw @ipa-nhg @ipa-bnm @ipa-mig feedback welcome!

fmessmer · 2017-01-12T18:40:37Z

Still missing are the raw's
I also will compare rosnode list, rostopic list, rosservice list and rosparam list of simulation (indigo_dev vs. feature_branch) as well as simulation vs. hardware (where possible) in order to see whether I missed something.

Still, feel free to drop in feedback already!

mgruhler

Getting impatient? ;-)

I like this. Seems to reduce a lot of duplication.

(Note that I did not check all robot specific launch and xml files for now)

mgruhler · 2017-01-13T07:20:06Z

cob_bringup/components/legacy_base.launch

+			<arg name="raw_cmd_vel_topic"	 default="velocity_smoother/command"/>
+			<arg name="smooth_cmd_vel_topic"  default="twist_controller/command"/>
+			<arg name="robot_cmd_vel_topic"   default="twist_controller/command"/>
+			<arg name="odom_topic"			default="odometry_controller/odometry"/>


mixed tabs and spaces

mgruhler · 2017-01-13T07:21:07Z

cob_bringup/components/cob4_head_camera.launch

+	<arg name="name" default="head_cam"/>
+	<arg name="sim" default="false"/>
+
+   <include unless="$(arg sim)" file="$(find cob_bringup)/drivers/usb_camera_node.launch">


tabs vs spaces

mgruhler · 2017-01-13T07:21:23Z

cob_bringup/components/cob4_head_camera.launch

+		<arg name="camera_name" value="$(arg name)"/>
+		<arg name="colorimage_in" value="/$(arg name)/image_rect_color"/>
+		<arg name="colorimage_out" value="/$(arg name)_upright/image_rect_color"/>
+		</include>


mgruhler · 2017-01-13T07:22:18Z

cob_bringup/components/legacy_schunk_powercube.launch

+		<include file="$(find cob_bringup)/controllers/cob_trajectory_controller.launch">
+		  <arg name="robot" value="$(arg robot)"/>
+		  <arg name="component_name" value="$(arg component_name)"/>
+		</include>


tabs vs spaces

mgruhler · 2017-01-13T07:22:43Z

cob_bringup/components/legacy_schunk_powercube.launch

+		  <arg name="robot" value="$(arg robot)" />
+		  <arg name="component_name" value="$(arg component_name)" />
+		</include>
+		<include file="$(find cob_bringup)/controllers/cob_trajectory_controller.launch">


imo, it would improve readability to add an empty line between includes. what do you think?

mgruhler · 2017-01-13T07:24:55Z

cob_bringup/robots/base_solo.launch

-	</group>
+	<include file="$(find cob_bringup)/components/canopen_base.launch" unless="$(arg use_old_base_drive_chain)">
+		<arg name="robot" value="$(arg robot)"/>
+	</include>


shouldn't the group block below be exchanged with the legacy_base.launch file`?

mgruhler · 2017-01-13T07:27:55Z

cob_bringup/robots/cob3-6.xml

+			</include>
+			<!--include file="$(find cob_bringup)/tools/wifi_monitor.launch" >
+				<arg name="robot" value="$(arg robot)" />
+			</include-->


Is there a reason to comment out the wifi_monitor?

mathias-luedtke · 2017-01-13T09:28:25Z

I also will compare rosnode list, rostopic list, rosservice list and rosparam list of simulation (indigo_dev vs. feature_branch) as well as simulation vs. hardware (where possible) in order to see whether I missed something.

Please try roslaunch-dump it should dump the fully parsed launchfile with all entries sorted for comparison.

fmessmer · 2017-01-13T09:34:53Z

@ipa-mdl, I tried to figure out how to use your script...can you give a usage example?

mathias-luedtke · 2017-01-13T09:38:19Z

I tried to figure out how to use your script...can you give a usage example?

as you would run roslaunch..
./roslaunch-dump cob_bringup cob4-2.launch
(chmod +x roslaunch-dump first)

mathias-luedtke · 2017-01-13T09:55:46Z

~~The script does not handle $() expressions for now, will fix it.~~
Script works fine, had a faulty package in my catkin workspace..

fmessmer · 2017-01-13T17:39:06Z

For travis to be happy, https://github.com/ipa320/cob_common/pull/202 is required

I consider this restructuring done....for now...more iterations will follow...(see suggested steps below)
I guaranteed the exact same behavior of the hardware (to the best of my knowledge and belief) using @ipa-mdl's roslaunch-dump doing a meld-comparison between the result of
roslaunch-dump cob_bringup robot.launch robot:=cobX-X
for the ipa320/indigo_dev and this feature branch

Simulation is working for all robots and basic functionality tests (move around using dashboard) were successful

In case you guys agree with this approach, I would like to merge this quite soon, as it will almost be impossible to resolve merge conflicts in case other PRs get merged in the meantime...

fmessmer · 2017-01-13T20:47:38Z

In future PRs

sort nodes/includes (alphabetically?) so that inter-robot comparison is easier
harmonize robots, i.e. converge xml's again
verify/harmonize simulation vs. hardware
further group frequently used stuff, e.g. teleop, script_server,....into a command_tools.launch
move cob_controller_configuration_gazebo package to cob_simulation, then introduce "cob_bringup"-like structure within cob_bringup_sim, finally getting rid of cob_controller_configuartion_gazebo package by moving launch files and scripts to appropriate location

...among other things...tbd

mathias-luedtke · 2017-01-13T22:27:11Z

cob_bringup/components/cob4_head_camera.launch


-   <include file="$(find cob_bringup)/drivers/usb_camera_node.launch">
+   <include unless="$(arg sim)" file="$(find cob_bringup)/drivers/usb_camera_node.launch">


alignment was wrong

mathias-luedtke · 2017-01-13T22:44:21Z

cob_bringup/components/cam3d_r200_rgbd.launch

@@ -8,6 +8,7 @@

 	<include unless="$(arg sim)" file="$(find realsense_camera)/launch/r200_nodelet_rgbd.launch">
 		<arg name="camera" value="$(arg name)"/>
+		<arg name="num_worker_threads" default="$(arg num_worker_threads)" />


default -> value?

copy-paste...:wink:

mathias-luedtke

the woes of commit-based review ;)

floweisshardt · 2017-01-17T11:42:53Z

cob_bringup/components/cam3d_r200_rgbd.launch

+		<arg name="robot" value="$(arg robot)"/>
+		<arg name="yaml_name" value="$(arg name)"/>
+	</include>
+	<include file="$(find cob_bringup)/drivers/image_flip_nodelet.launch">


use flip argument

floweisshardt · 2017-01-17T11:44:56Z

cob_bringup/components/cob4_head_camera.launch

+		<arg name="robot" value="$(arg robot)"/>
+		<arg name="yaml_name" value="$(arg name)"/>
+	</include>
+	<include file="$(find cob_bringup)/drivers/image_flip.launch">


use flip argument

fmessmer · 2017-01-17T17:07:00Z

Will be rebased and squash-merged after #585 and updated #579 (by @ipa-mdl)

…launch

fmessmer · 2017-01-18T10:03:24Z

I rebased this feature branch on top of indigo_dev as #579 and #585 have been merged now.
@ipa-fmw @ipa-nhg @ipa-mdl please one final review...
...and then this PR should be Squash and merge into a single commit e.g. "group components and restructure"

fmessmer · 2017-01-18T13:11:04Z

tag eol-cob3 has been added

* refactored generic canopen&config into canopen_generic.launch * refactored base driver+config into canopen_base.launch * added components/cob4_head_camera.launch * added components/cam3d_openni2.launch * added components/cam3d_r200_rgbd.launch * introduce sim arg for components * use sim arg in robot.xml * remove nodes started within robot.xml from default_controllers_robot.launch * introducing legacy components * reorganize and sim toggle for more components * adjust cob4-1 to latest changes * use new structure for cob3-2 * use new structure for cob3-6 * use new structure for cob3-9 * use new structure for cob4-2 * use new structure for remaining cob4s * travis fixes * syntax styling * use new structure for raws * more travis fixes * harmonize old vs. new behavior cob4-1 * guarantee same hw behavior as before * add flip argument

fmessmer changed the title ~~Use grouped low level components for simulation~~ [WIP] Use grouped low level components for simulation Jan 11, 2017

This was referenced Jan 11, 2017

use cob_bringup robot.xml in simulation ipa320/cob_simulation#131

Merged

[DoNotMerge] concept for unification of hw/sim-independent bringup nodes #571

Closed

[WIP] group components #574

Closed

fmessmer force-pushed the grouped-low-level-components_simulation branch from 1c07fcf to 44319a0 Compare January 12, 2017 14:55

mgruhler suggested changes Jan 13, 2017

View reviewed changes

fmessmer force-pushed the grouped-low-level-components_simulation branch 3 times, most recently from 68e3400 to 4281c06 Compare January 13, 2017 15:41

fmessmer mentioned this pull request Jan 13, 2017

allow static sensorring 4am-robotics/cob_common#202

Merged

fmessmer changed the title ~~[WIP] Use grouped low level components for simulation~~ Use grouped low level components for simulation Jan 13, 2017

fmessmer force-pushed the grouped-low-level-components_simulation branch from e0cf511 to 5cfdc20 Compare January 13, 2017 20:36

mathias-luedtke suggested changes Jan 13, 2017

View reviewed changes

mathias-luedtke approved these changes Jan 13, 2017

View reviewed changes

fmessmer force-pushed the grouped-low-level-components_simulation branch from 5cfdc20 to 0a5f1d0 Compare January 13, 2017 22:58

floweisshardt requested changes Jan 17, 2017

View reviewed changes

floweisshardt approved these changes Jan 17, 2017

View reviewed changes

mgruhler approved these changes Jan 17, 2017

View reviewed changes

fmessmer changed the title ~~Use grouped low level components for simulation~~ [WIP] Use grouped low level components for simulation Jan 17, 2017

mathias-luedtke mentioned this pull request Jan 17, 2017

Grouped low level components #575

Closed

mathias-luedtke and others added 23 commits January 18, 2017 10:55

refactored base driver+config into canopen_base.launch

e8a7620

refactored generic canopen&config into canopen_generic.launch

3c7bfe8

added components/cob4_head_camera.launch

8635a7d

added components/cam3d_openni2.launch

26f1835

added components/cam3d_r200_rgbd.launch

0f2ef64

introduce sim arg for components

16f6b41

use sim arg in robot.xml

4ff9fd0

remove nodes started within robot.xml from default_controllers_robot.…

23a08d9

…launch

introducing legacy components

088925a

reorganize and sim toggle for more components

e8b9718

adjust cob4-1 to latest changes

05cb56e

use new structure for cob3-2

25c554d

use new structure for cob3-6

1147fd5

use new structure for cob3-9

8d1d749

use new structure for cob4-2

de2cd57

use new structure for remaining cob4s

673c128

travis fixes

99126d7

syntax styling

8455038

use new structure for raws

c77f221

more travis fixes

6cd4a60

harmonize old vs. new behavior cob4-1

99dc422

guarantee same hw behavior as before

5b3aa46

add flip argument

cd4b234

fmessmer force-pushed the grouped-low-level-components_simulation branch from 4bcb15d to cd4b234 Compare January 18, 2017 09:57

floweisshardt merged commit ad46472 into ipa320:indigo_dev Jan 18, 2017

fmessmer deleted the grouped-low-level-components_simulation branch January 18, 2017 13:13

fmessmer restored the grouped-low-level-components_simulation branch January 18, 2017 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use grouped low level components for simulation #583

[WIP] Use grouped low level components for simulation #583

fmessmer commented Jan 11, 2017 •

edited

Loading

fmessmer commented Jan 12, 2017

fmessmer commented Jan 12, 2017

fmessmer commented Jan 12, 2017

mgruhler left a comment

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mgruhler Jan 13, 2017

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

fmessmer commented Jan 13, 2017

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

fmessmer commented Jan 13, 2017 •

edited

Loading

fmessmer commented Jan 13, 2017 •

edited

Loading

mathias-luedtke Jan 13, 2017

mathias-luedtke Jan 13, 2017

fmessmer Jan 13, 2017

mathias-luedtke left a comment

floweisshardt Jan 17, 2017

floweisshardt Jan 17, 2017

fmessmer commented Jan 17, 2017

fmessmer commented Jan 18, 2017

fmessmer commented Jan 18, 2017


		<include file="$(find cob_bringup)/drivers/usb_camera_node.launch">
		<include unless="$(arg sim)" file="$(find cob_bringup)/drivers/usb_camera_node.launch">

[WIP] Use grouped low level components for simulation #583

[WIP] Use grouped low level components for simulation #583

Conversation

fmessmer commented Jan 11, 2017 • edited Loading

fmessmer commented Jan 12, 2017

fmessmer commented Jan 12, 2017

fmessmer commented Jan 12, 2017

mgruhler left a comment

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

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

mathias-luedtke commented Jan 13, 2017 • edited Loading

fmessmer commented Jan 13, 2017

mathias-luedtke commented Jan 13, 2017 • edited Loading

mathias-luedtke commented Jan 13, 2017 • edited Loading

fmessmer commented Jan 13, 2017 • edited Loading

fmessmer commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathias-luedtke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmessmer commented Jan 17, 2017

fmessmer commented Jan 18, 2017

fmessmer commented Jan 18, 2017

fmessmer commented Jan 11, 2017 •

edited

Loading

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

mathias-luedtke commented Jan 13, 2017 •

edited

Loading

fmessmer commented Jan 13, 2017 •

edited

Loading

fmessmer commented Jan 13, 2017 •

edited

Loading