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

Port to ROS 2 #30

Merged
merged 24 commits into from Mar 10, 2020
Merged

Port to ROS 2 #30

merged 24 commits into from Mar 10, 2020

Conversation

clalancette
Copy link
Collaborator

@clalancette clalancette commented Dec 12, 2019

This PR is a port of joint_state_publisher to ROS 2. I did the initial port, but got a bunch of help from people along the way (thanks @gonzodepedro, @schornakj, @emersonknapp, @sloretz, and @pbeeson).

Besides the port to ROS 2, this PR does a number of other things to improve the code:

  1. It adds a README describing the subscribed topics, published topics, and parameters.
  2. If the GUI is in use, and the GUI is closed, this now quits the program (which I think is a little more intuitive).
  3. Allows the joint_state_publisher to get its URDF either from the command-line, or from the /robot_description topic. In the latter case, this node will actually do nothing until something is received on /robot_description, at which point it will start publishing. If both the command-line option is specified and the /robot_description topic is available, the command-line option takes precedence.

I've done quite some testing on this to make sure all of the options work. In my testing with a simple URDF, it does seem to work fairly well. There are a few TODOs left:

  1. Despite the fact that we allow_undeclared_parameters, there doesn't seem to be a good way to get at undeclared parameters in the rclpy API. Thus, we can't really support dependent_joints and zeros until we figure that out. That will be a PR for another time (maybe with some core rclpy changes as well). I actually found a way to do this, so this should work.
  2. While rclpy.Rate is available in Eloquent, I wanted to make sure this was backwards-compatible with Dashing as well. So I left the TODO about switching to Rate in there. This no longer applies since we switched to a timer.

This also fixes #20

@sloretz
Copy link
Contributor

sloretz commented Dec 13, 2019

Despite the fact that we allow_undeclared_parameters, there doesn't seem to be a good way to get at undeclared parameters in the rclpy API. Thus, we can't really support dependent_joints and zeros until we figure that out. That will be a PR for another time (maybe with some core rclpy changes as well).

It looks like this can be made to work with automatically_declare_parameters_from_overrides. I'm having trouble understanding the meaning of the options to the parameters API, but it seems automatically_declare_parameters_from_overrides declares any parameter that is passed via yaml (Not just parameters via parameter_overrides), and allow_undeclared_parameters doesn't seem to be necessary.

/**:
  ros__parameters:
    dependent_joints:
      joint_D: {parent: joint_A, factor: 3 }
      joint_E: {parent: joint_B }
      joint_F: {parent: joint_C, factor: -1 }
import rclpy
from rclpy.node import Node


if __name__ == '__main__':
    rclpy.init()
    node = Node('mock_jsp', automatically_declare_parameters_from_overrides=True)
    print(node.get_parameters_by_prefix('dependent_joints'))
    rclpy.shutdown()
$ python3 mockjsp.py --ros-args --params-file ./jsp_params.yaml --
{'joint_D.parent': <rclpy.parameter.Parameter object at 0x7fd4103f9780>, 'joint_D.factor': <rclpy.parameter.Parameter object at 0x7fd4103f9be0>, 'joint_E.parent': <rclpy.parameter.Parameter object at 0x7fd4103a1390>, 'joint_F.parent': <rclpy.parameter.Parameter object at 0x7fd4156cdc88>, 'joint_F.factor': <rclpy.parameter.Parameter object at 0x7fd407c74160>}

@clalancette
Copy link
Collaborator Author

@sloretz You are totally right, that works. Thanks! I've added back in support for dependent joints and zeros in 475e12f

@sloretz sloretz changed the base branch from ros2 to kinetic-devel December 13, 2019 17:51
@sloretz sloretz changed the base branch from kinetic-devel to ros2 December 13, 2019 17:51
@clalancette
Copy link
Collaborator Author

FYI that this is probably on hold until we decide what to do about #31. Once we've merged that, I'll likely rebase/rewrite this on top of that.

@clalancette
Copy link
Collaborator Author

I've now redone this on top of #31 . Still needs to wait until that is merged, but once that is in we can easily rebase and review this.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Collaborator Author

All right, the ROS 1 stuff has been merged into noetic-devel. I've updated the ros2 branch to be the same as noetic-devel, and rebased this branch on top of that. @sloretz This one should be ready for a review.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Collaborator Author

@sloretz Thanks for the great review! Things are now updated as you recommend in almost all cases. Please take another look when you get a chance.

joint_state_publisher/README.md Outdated Show resolved Hide resolved
joint_state_publisher/README.md Outdated Show resolved Hide resolved
joint_state_publisher/README.md Outdated Show resolved Hide resolved
joint_state_publisher/README.md Outdated Show resolved Hide resolved
joint_state_publisher/README.md Outdated Show resolved Hide resolved
joint_state_publisher/test/test_xmllint.py Outdated Show resolved Hide resolved
joint = self.free_joints[parent]

if has_position and 'position' in joint:
msg.position[i] = float(joint['position']) * factor + offset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an IndexError here when I publish the same URDF twice on the /robot_description topic. It looks like self.joint_list has duplicate joint names, but num_joints was calculated from free_joints and dependent_joints which can't have duplicates because they're dictionaries, so msg.position is half the length. Maybe the variables that are set in __init__ need to be set in self.configure_robot instead so they get cleared when a new description is published?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was absolutely a problem. Fixing it turned out to be...complicated. Commit 735f2a7 successfully fixes it, along with making the layout way more dynamic and useful (in my opinion). It also simplifies the code a bit, at the expense of adding a new PyQt layout type (FlowLayout), and putting the Center/Randomize buttons at the top. I think that is a pretty good tradeoff for a dynamic, reflowable UI, but please take a look and let me know what you think.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This means that updates from the robot_description topic are
properly shown in the UI now.  While I was in here, I also
changed it so that the UI "flows" as you resize the window.
This should be much more usable on smaller screens.  It
also simplifies the code quite a bit.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Collaborator Author

@sloretz All of your above points should be fixed now. Ready for another round, thanks!

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz and others added 8 commits March 6, 2020 14:21
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@clalancette clalancette merged commit cc7e876 into ros2 Mar 10, 2020
@clalancette clalancette deleted the ros2-devel branch March 10, 2020 18:00
@clalancette clalancette restored the ros2-devel branch March 10, 2020 18:27
@clalancette clalancette deleted the ros2-devel branch May 19, 2020 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants