Refactor the ROS 2 code to be more modern #126

clalancette · 2020-01-22T19:07:08Z

This is a major refactor of the robot_state_publisher code. There are a few goals here:

Finish up the port, including adding support for most of the parameters that were hard-coded before.
Heavily cleanup the code to make it easier to understand.
Switch to have robot_description provided as a parameter instead of a command-line argument.
Allow the robot_description to be updated on the fly (via doing a "set" on the parameter).
Allow robot_state_publisher to be composed.
Add additional documentation on how to launch it.

With all of the above, this is a large API and ABI break, so is only appropriate for Foxy and forwards. Once this is ready to go in, there will be another PR necessary to https://github.com/ros2/demos/blob/857c436e76bc6b40d32f096f6989bf51bb73f359/dummy_robot/dummy_robot_bringup/launch/dummy_robot_bringup.launch.py#L28 to make it use the new parameter.

Reviews and thoughts on this welcome!

Signed-off-by: Chris Lalancette <[email protected]>

The new implementation will be dynamic, so downstream consumers shouldn't need to subclass it. Signed-off-by: Chris Lalancette <[email protected]>

We'll use this during the constructor and later on during the parameter setup. Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

This includes some example launch files. Signed-off-by: Chris Lalancette <[email protected]>

mjcarroll

A few drive-by comments.

CMakeLists.txt

include/robot_state_publisher/robot_state_publisher.hpp

src/robot_state_publisher.cpp

include/robot_state_publisher/robot_state_publisher.hpp

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron

Generally looks good! Some questions and suggestions below.

CMakeLists.txt

README.md

CMakeLists.txt

jacobperron · 2020-01-23T22:15:55Z

test/two_links_moving_joint-launch.py

+        output='screen',
+    )
+
+    urdf_file = os.path.join(os.path.dirname(__file__), 'two_links_moving_joint.urdf')


Correct me if I'm wrong, but it looks like this is the only difference with the other launch_testing file. Can we consolidate and make the urdf file name a launch argument?

It's not a bad idea. I'll follow-up a little later on this.

So I tried to do this, but I don't think it is possible (or at least, I don't know how to do it with the launch_testing API). The problem is that inside the launch file, I need to do something like:

urdf_path = os.path.join(os.path.dirname(__file__), LaunchConfiguration('urdf_file').expand()) with open(urdf_path) as infp: robot_desc = infp.read() params = {'robot_description': robot_desc}

Unfortunately, the expand() method on LaunchConfiguration just doesn't exist. There is perform(), but that needs some sort of context that I don't know how to get. Without that, I can't read in the robot_description that I need. So I'm going to give up on this for now.

Any ideas on how to accomplish this are welcome!

I see the problem. Here's a similar question with not quite the answer we're looking for. I feel like it should be possible; e.g. wrap the LaunchConfiguration substitution in another Substitution that does the file read. I don't have bandwidth atm to look into, so I'm happy to move forward with this PR as-is and if we figure it out we can revisit it.

test/test_two_links_moving_joint.cpp

src/robot_state_publisher.cpp

jacobperron · 2020-01-23T22:32:25Z

src/robot_state_publisher.cpp

+    geometry_msgs::msg::TransformStamped tf_transform = kdlToTransform(seg.second.segment.pose(0));
+    rclcpp::Time now = this->now();
+    if (!use_tf_static_) {
+      now = now + rclcpp::Duration(std::chrono::milliseconds(500));


Why are we adding 500ms to the current time here?

I honestly have no idea. This is ported directly from

robot_state_publisher/src/robot_state_publisher.cpp

Line 117 in fe661c1

tf_transform.header.stamp += ros::Duration(0.5);

; I'll have to look into it later.

I did a bit of archaeology in it, and found #46 . In there, Jackie says that she considered this a hack, to be removed, but kept it for backwards-compatibility reasons. It looks like she thought that we should just always default to use_tf_static, and remove the ability to shut it off.

That being said, I know we've had some complaints about this in the past, as splitting the tf tree between tf and tf_static can make it more difficult to get a full view of the tf tree. So I'm on the fence about this. My inclination is to leave it as-is, but I'm definitely open to hearing other opinions about it.

I tend to agree with removing the use_tf_static option and always publishing fixed frames as static (i.e. /tf_static), on the grounds that the split topics /tf and /tf_static are agreed on as a standard. IMO, tools that interact with the TF package (e.g. viewing the tree) should handle data coming from both channels.

Moving forward, I'd suggest deprecating the usage of use_tf_static; defaulting to true and warning the user if they set it to false.

My two cents.

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron

LGTM with green CI

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-01-27T15:07:40Z

CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-01-27T15:10:12Z

After doing the changes to dummy_robot_bringup in ros2/demos#426 , there are 2 backwards compatibility concerns:

The name of the standalone node switched from robot_state_publisher to robot_state_publisher_node.
The URDF now has to be passed as a parameter, not a command-line argument, to the standalone node.

The first one is easy enough to fix, if we want to. For the second one, we could make it backwards compatible (with a warning) if I hand-wrote the standalone node and did argument processing in there. @jacobperron @sloretz thoughts on how important backwards compatibility is here?

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron · 2020-01-27T17:00:59Z

If there's not a strong argument for renaming the node, I'd vote for changing it back to robot_state_publisher. This should make it more straight forward for people not only coming from Eloquent, but also ROS 1.

Regarding the second point. You don't necessarily have to write the standalone node by hand. I ran into a similar situation when refactoring the image_tools demos and we added this line so that non-ROS CLI args are passed to the node component (accessed with the NodeOptions arguments(), for example).

mjcarroll · 2020-01-27T17:06:19Z

Regarding the second point. You don't necessarily have to write the standalone node by hand. I ran into a similar situation when refactoring the image_tools demos and we added this line so that non-ROS CLI args are passed to the node component (accessed with the NodeOptions arguments(), for example).

Can we get this in the component tutorials/documentation somewhere? I think this is a pattern that people would like to know about.

jacobperron · 2020-01-27T17:21:46Z

Can we get this in the component tutorials/documentation somewhere? I think this is a pattern that people would like to know about.

Sure. At the time there was discussion as to whether we were actually abusing the NodeOptions API, which was originally intended for ROS-args. IMO, our usage is still a little awkward, since the rclcpp_components node is stripping out the ROS-args and only forwarding the non-ROS args. Maybe we should change the behaviour so that rclcpp_components doesn't strip away the ROS-args, or we can leave it as-is and document the special case for components. I can open a ticket to change the behaviour if I get a +1.

clalancette · 2020-01-27T17:36:15Z

If there's not a strong argument for renaming the node, I'd vote for changing it back to robot_state_publisher. This should make it more straight forward for people not only coming from Eloquent, but also ROS 1.

Yeah, agreed. Will make that change.

Regarding the second point. You don't necessarily have to write the standalone node by hand. I ran into a similar situation when refactoring the image_tools demos and we added this line so that non-ROS CLI args are passed to the node component (accessed with the NodeOptions arguments(), for example).

Oh, nice. I'll play around with this, and make something that has backwards compatibility with the old stuff.

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-01-27T17:49:31Z

All right, backwards compatibility implemented. I'm pretty close here, there is one last thing not working. On Windows, the trick I'm using to pass the test executable name into the launch file doesn't work for some reason. I'm still debugging that.

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-01-27T21:11:37Z

And tests fixed on Windows. Here's another CI try:

CI:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

clalancette · 2020-01-27T22:41:35Z

All right, all CI is green except for Windows Debug. But the warning there already exists from the nightly: https://ci.ros2.org/view/nightly/job/nightly_win_deb/1509/

Unless there are further objections, I'm going to go ahead and merge this tomorrow morning. Thanks again to @jacobperron and @mjcarroll for reviews!

CMakeLists.txt

Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2020-01-28T01:57:32Z

CI:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

jacobperron

LGTM!

clalancette requested review from jacobperron and sloretz January 22, 2020 19:07

clalancette added 8 commits January 22, 2020 19:07

Major refactor.

da4cbeb

Signed-off-by: Chris Lalancette <[email protected]>

Make robot_state_publisher composable.

d2f025d

Signed-off-by: Chris Lalancette <[email protected]>

Update the documentation a bit.

93bb6f9

Signed-off-by: Chris Lalancette <[email protected]>

Mark things as explicit and final.

bbcd58f

The new implementation will be dynamic, so downstream consumers shouldn't need to subclass it. Signed-off-by: Chris Lalancette <[email protected]>

Refactor to have a "setup" function.

bd0ab39

We'll use this during the constructor and later on during the parameter setup. Signed-off-by: Chris Lalancette <[email protected]>

Fix silly bug while computing the publish interval.

d06780b

Signed-off-by: Chris Lalancette <[email protected]>

Implement the ability to change the parameters on the fly.

2160a53

Signed-off-by: Chris Lalancette <[email protected]>

More updates.

81288b0

This includes some example launch files. Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the ros2-refactor branch from 4b2f66d to 81288b0 Compare January 22, 2020 19:08

clalancette mentioned this pull request Jan 22, 2020

tf_prefix no longer used in ROS2 node #125

Closed

mjcarroll reviewed Jan 22, 2020

View reviewed changes

include/robot_state_publisher/robot_state_publisher.hpp Outdated Show resolved Hide resolved

clalancette added 2 commits January 22, 2020 19:57

Fixes from review.

3fced14

Signed-off-by: Chris Lalancette <[email protected]>

Add testing to robot_state_publisher.

def1bad

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron reviewed Jan 23, 2020

View reviewed changes

clalancette added 4 commits January 24, 2020 16:22

Fixes from review.

cf7fe91

Signed-off-by: Chris Lalancette <[email protected]>

Switch to modern launch_testing ReadyToTest.

9a6c78e

Signed-off-by: Chris Lalancette <[email protected]>

Guard against negative publish_frequencies.

db17433

Signed-off-by: Chris Lalancette <[email protected]>

Get rid of redundant declaration.

9bf4e83

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron approved these changes Jan 24, 2020

View reviewed changes

clalancette mentioned this pull request Jan 27, 2020

Why is there a leading slash? #128

Closed

More fixes.

a0eb5cf

Signed-off-by: Chris Lalancette <[email protected]>

clalancette mentioned this pull request Jan 27, 2020

Switch dummy_robot_bringup to use parameter for rsp. ros2/demos#426

Merged

Fix test build on Windows.

51bf732

Signed-off-by: Chris Lalancette <[email protected]>

clalancette added 2 commits January 27, 2020 17:46

robot_state_publisher_node -> robot_state_publisher

be95c29

Signed-off-by: Chris Lalancette <[email protected]>

Remove debugging code.

ffdb9a8

Signed-off-by: Chris Lalancette <[email protected]>

clalancette added 2 commits January 27, 2020 20:45

Make the error message much nicer on Windows.

0ec9f43

Signed-off-by: Chris Lalancette <[email protected]>

Fix tests on Windows.

532fc87

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron reviewed Jan 27, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Switch to using TARGET_FILE to find the binary targets on Windows.

14ad8a5

Signed-off-by: Chris Lalancette <[email protected]>

jacobperron approved these changes Jan 28, 2020

View reviewed changes

clalancette merged commit 4be6ed9 into ros2 Jan 28, 2020

clalancette deleted the ros2-refactor branch January 28, 2020 14:58

clalancette restored the ros2-refactor branch January 28, 2020 15:00

clalancette deleted the ros2-refactor branch January 28, 2020 15:49

This was referenced Apr 2, 2020

ROS 2: Restore original publish frequency and parameters from ROS 1 #117

Closed

ROS 2: Port the robot_state_publisher tests #116

Closed

ROS 2: Switch back to parameter for description? #119

Closed

ROS 2: Properly parse command-line options #118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the ROS 2 code to be more modern #126

Refactor the ROS 2 code to be more modern #126

clalancette commented Jan 22, 2020

mjcarroll left a comment

jacobperron left a comment

jacobperron Jan 23, 2020

clalancette Jan 24, 2020

clalancette Jan 24, 2020

jacobperron Jan 24, 2020

jacobperron Jan 23, 2020

clalancette Jan 24, 2020

jacobperron Jan 24, 2020

clalancette Jan 24, 2020

jacobperron Jan 24, 2020 •

edited

Loading

jacobperron left a comment

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

jacobperron commented Jan 27, 2020 •

edited

Loading

mjcarroll commented Jan 27, 2020

jacobperron commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 28, 2020

jacobperron left a comment

Refactor the ROS 2 code to be more modern #126

Refactor the ROS 2 code to be more modern #126

Conversation

clalancette commented Jan 22, 2020

mjcarroll left a comment

Choose a reason for hiding this comment

jacobperron 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

Choose a reason for hiding this comment

jacobperron Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

jacobperron commented Jan 27, 2020 • edited Loading

mjcarroll commented Jan 27, 2020

jacobperron commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 27, 2020

clalancette commented Jan 28, 2020

jacobperron left a comment

Choose a reason for hiding this comment

jacobperron Jan 24, 2020 •

edited

Loading

jacobperron commented Jan 27, 2020 •

edited

Loading