Split jsp and jsp gui #31

clalancette · 2019-12-19T22:41:04Z

This PR splits joint_state_publisher into two packages: joint_state_publisher and joint_state_publisher_gui. The joint_state_publisher package contains both code to be imported (the JointStatePublisher class) along with a script that just runs the basic, non-GUI functionality of JointStatePublisher. The joint_state_publisher_gui package contains the PyQt UI that allows one to control the movable joint via a UI, and depends on the joint_state_publisher package and JointStatePublisher class.

Doing this is desirable for a few reasons, mainly to slim down joint_state_publisher and remove the Qt dependencies from the robot metapackage (see ros/metapackages#28 for details).

I am unsure where to target this PR. This is definitely a breaking change in that users who just install/launch joint_state_publisher will no longer have GUI functionality available. So I would usually target noetic with this. However, I think that removing the GUI dependencies from the robot metapackage in kinetic and melodic is important, so I'm currently targeting this at kinetic-devel. I'm definitely open to discussing this further.

This also fixes #21 .

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This will be moved to a separate package. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

matthew-reynolds

I think you're right to target Kinetic & Melodic, though I think we'd be better off with a smoother migration as described in my comment. Then for Noetic we can drop the forwarding altogether.

Couple other comments:

Since you're restructuring and creating a bunch of new files, is this a good opportunity to add license header comments? It appears the other former robot_model packages all have the expected 3-clause BSD license w/ Willow Garage as the copyright holder.

Don't think the screenshot.png needs to be copied over to the new package.

matthew-reynolds · 2019-12-20T03:22:04Z

joint_state_publisher/src/joint_state_publisher/__init__.py

+        use_gui = get_param('use_gui')
+        if use_gui is not None:
+            rospy.logerr('GUI is no longer supported by this package; install and run joint_state_publisher_gui')
+            sys.exit(1)


I strongly believe we should approach this the same way we're working on removing the GUI dependencies from actionlib in ros/actionlib#157.

That is, rather than failing outright, attempt to minimize the breaking change by searching for and launching joint_state_publisher_gui if possible, with a warning telling users about the change. In the case that the joint_state_publisher_gui package isn't found, print an error and exit(1) as you've done here.

README.md

joint_state_publisher_gui/CMakeLists.txt

joint_state_publisher_gui/package.xml

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2019-12-20T13:58:22Z

I think you're right to target Kinetic & Melodic, though I think we'd be better off with a smoother migration as described in my comment. Then for Noetic we can drop the forwarding altogether.
That is, rather than failing outright, attempt to minimize the breaking change by searching for and launching joint_state_publisher_gui if possible, with a warning telling users about the change. In the case that the joint_state_publisher_gui package isn't found, print an error and exit(1) as you've done here.

Stuck between a rock and a hard place here. If we do this, then by all rights joint_state_publisher should have a dependency on joint_state_publisher_gui. Since joint_state_publisher_gui already has a dependency on joint_state_publisher, we end up with a circular dependency. On the other hand, if we don't do this, then we end up with potentially surprising results as you point out. I guess if we have a graceful fallback option, we can just live with the incorrect dependency hierarchy. I can take a look at doing that after the holidays.

Couple other comments:

Since you're restructuring and creating a bunch of new files, is this a good opportunity to add license header comments? It appears the other former robot_model packages all have the expected 3-clause BSD license w/ Willow Garage as the copyright holder.

Good point. I've added the license stuff in 1db983d.

Don't think the screenshot.png needs to be copied over to the new package.

Actually, I think it is the other way around; joint_state_publisher itself no longer needs it (since it is a screenshot of the GUI). So I've moved the screenshot over to the GUI package.

Thanks for the review.

matthew-reynolds · 2019-12-20T14:11:27Z

I guess if we have a graceful fallback option, we can just live with the incorrect dependency hierarchy

Yeah that's pretty much what I'm suggesting. Rather than an exec_depend on joint_state_publisher_gui, we have a sort of "soft dependency" where we instruct the user to install the package. Definitely not a clean dependency hierarchy, but it smooths out the migration. Especially since I suspect the "soft dependency" will resolve for most users, since if they're using the GUI they're likely on a desktop metapackage install anyways.

Actually, I think it is the other way around

Good call 👍

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2020-01-02T19:00:36Z

All right, I've now updated it so that joint_state_publisher has a "soft" dependency on joint_state_publisher_gui, and will attempt to launch the latter if use_gui is set to True. This mimics backwards-compatible behavior with what exists in Kinetic and Melodic. Assuming we merge this PR, I intend to make a new noetic branch where we remove this workaround.

@sloretz @matthew-reynolds Ready for another round of review.

joint_state_publisher/scripts/joint_state_publisher

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

matthew-reynolds

Thanks for that change, looks great. import sys could be moved within the if use_gui: block, but then we've got imports in 3 different places, so I like what you've done by leaving it at the top.

Found one last question, see below.

joint_state_publisher/package.xml

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

We get it implicitly through the catkin dependency. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz

Partial review, coming back to this later.

sloretz · 2020-01-06T22:41:11Z

joint_state_publisher/README.md

+* `use_smallest_joint_limits` (bool) - Whether to honor `<safety_controller>` tags in the URDF.  Defaults to True.
+* `source_list` (array of strings) - Each string in this array represents a topic name.  For each string, create a subscription to the named topic of type `sensor_msgs/JointStates`.  Publication to that topic will update the joints named in the message.  Defaults to an empty array.
+* `zeros` (dictionary of string -> float) - A dictionary of joint_names to initial starting values for the joint.  Defaults to an empty dictionary, in which case 0.0 is assumed as the zero for all joints.
+* `dependent_joints` (dictionary of string -> dictionary of 'parent', 'factor', 'offset') - A dictionary of joint_names to the joints that they mimic; compare to the '<mimic>' tag in URDF.  A joint listed here will mimic the movements of the 'parent' joint, subject to the 'factor' and 'offset' provided.  The 'parent' name must be provided, while the 'factor' and 'offset' parameters are optional (they default to 1.0 and 0.0, respectively).  Defaults to the empty dictionary, in which case only joints that are marked as '<mimic>' in the URDF are mimiced.


In the preview, '<mimic>' renders as ''. Use backticks instead?

Will try to fix that.

Fixed in 3e59f5a

joint_state_publisher/src/joint_state_publisher/__init__.py

joint_state_publisher_gui/scripts/joint_state_publisher_gui

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This can happen if something is published to the joint_state_publisher source_cb, so in that case we need to update the GUI. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2020-01-08T16:35:55Z

@sloretz Thanks for the partial review, I've fixed the issues you've identified. Ready for complete review/second round of review.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz

LGTM with one nit. Thanks for keeping the amount of unrelated cleanup to a minimum. It made diffing/reviewing pretty easy.

joint_state_publisher_gui/src/joint_state_publisher_gui/__init__.py

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2020-01-13T18:15:58Z

Thanks! Merging now that the buildfarm is turning over.

matthew-reynolds · 2020-01-28T13:57:43Z

@clalancette @sloretz Do you plan on updating REP 142/150 to add joint_state_publisher_gui to the desktop metapackage? Happy to provide a PR if this is your plan.

clalancette · 2020-01-31T13:36:09Z

@clalancette @sloretz Do you plan on updating REP 142/150 to add joint_state_publisher to the desktop metapackage? Happy to provide a PR if this is your plan.

~~As far as I can tell, REP-142 doesn't need to be updated. It doesn't include joint_state_publisher in the robot metapackage, and I don't think we should change that this late in its lifetime.~~

~~REP-150 is obviously out-of-sync with what is in https://github.com/ros/metapackages; I'll submit a PR to update that soon.~~

Arg, never mind, I was confused. I forgot about the soft dependency thing.

So it is still the case that REP-142 doesn't have joint_state_publisher at all, so I don't think we need to do anything there. REP-150 should indeed be updated to add joint_state_publisher_gui to the desktop metapackage; I'll open a PR for that soon.

clalancette · 2020-01-31T14:25:15Z

ros/metapackages#30 and ros-infrastructure/rep#230

matthew-reynolds · 2020-01-31T14:58:54Z

REP-142 doesn't have joint_state_publisher at all, so I don't think we need to do anything there

jsp is pulled in via robot_model (in the robot metapackage) and urdf_tutorial (in the desktop_full metapackage). Seems risky to not add jsp_gui to desktop since previously every install from robot upwards contained the gui.

REP-150 should indeed be updated to add joint_state_publisher_gui to the desktop metapackage

👍

matthew-reynolds · 2020-01-31T15:02:06Z

I suppose we should also give a scan through the dependant packages on jsp and update them accordingly (ie. urdf_tutorial uses the jsp gui, but depends only on jsp). I will get started on this.

gavanderhoorn · 2020-02-25T10:44:48Z

We're getting some questions on ROS Answers about this split (344992 fi).

Should this change be announced on Discourse to reduce the surprise?

clalancette · 2020-02-26T14:03:55Z

@gavanderhoorn Thanks for fielding those ROS answers queries.

I'll go ahead and make an announcement on Discourse.

ros-discourse · 2020-02-26T14:14:22Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/split-of-joint-state-publisher-and-joint-state-publisher-gui/12997/1

ros-discourse · 2021-02-15T11:10:59Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/15

clalancette added 3 commits December 19, 2019 20:45

Update the README.md to have topics and parameters in use.

368215b

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove GUI parts from joint_state_publisher.

102c872

This will be moved to a separate package. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add in the GUI package.

e297479

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette requested a review from sloretz December 19, 2019 22:41

clalancette mentioned this pull request Dec 19, 2019

Port to ROS 2 #30

Merged

matthew-reynolds reviewed Dec 20, 2019

View reviewed changes

Fixes from review, including new LICENSE files.

1db983d

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Reinstate the use_gui option.

8093f3a

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

matthew-reynolds suggested changes Jan 2, 2020

View reviewed changes

joint_state_publisher/scripts/joint_state_publisher Outdated Show resolved Hide resolved

Fixes from review.

f1ed8c9

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

matthew-reynolds reviewed Jan 2, 2020

View reviewed changes

joint_state_publisher/package.xml Outdated Show resolved Hide resolved

clalancette added 3 commits January 2, 2020 21:49

Add <buildtool_depend>python-catkin-pkg for jsp_gui.

61dcd3c

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove build_depend on python-catkin-pkg.

516826a

We get it implicitly through the catkin dependency. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Honor the num_rows variable in joint_state_publisher_gui.

4d4bafc

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz reviewed Jan 6, 2020

View reviewed changes

sloretz mentioned this pull request Jan 7, 2020

REP 150: Add actionlib_tools to the desktop metapackage ros-infrastructure/rep#220

Closed

clalancette added 2 commits January 8, 2020 16:32

Try to fix README XML tag.

3e59f5a

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add the ability to update the GUI based on external events.

2ef311d

This can happen if something is published to the joint_state_publisher source_cb, so in that case we need to update the GUI. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Fix one more <mimic> tag.

d3beb88

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz approved these changes Jan 11, 2020

View reviewed changes

joint_state_publisher_gui/src/joint_state_publisher_gui/__init__.py Show resolved Hide resolved

Add license boilerplate to joint_state_publisher_gui.

12bd0e5

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz approved these changes Jan 13, 2020

View reviewed changes

clalancette merged commit d5b5c05 into kinetic-devel Jan 13, 2020

clalancette deleted the split-jsp-and-jsp-gui branch January 13, 2020 18:17

meyerj mentioned this pull request Feb 6, 2020

joint_state_publisher in metapackage robot depends on Qt ros/metapackages#28

Closed

fmessmer mentioned this pull request Feb 26, 2020

fix joint_state_publisher_gui repo split ipa320/cob_robots#805

Merged

gavanderhoorn mentioned this pull request Feb 27, 2020

Proposal: remove rviz run dependency from robot support packages ros-industrial/ros_industrial_issues#50

Open

Tiryoh mentioned this pull request Feb 29, 2020

Error and Warning message at running display.launch rt-net/raspimouse_description#7

Closed

tony2guo mentioned this pull request Mar 26, 2020

add joint_state_publisher_gui ros/urdf_tutorial#40

Merged

nicholasadr mentioned this pull request May 30, 2020

Add joint_state_publisher_gui crigroup/osr_course_pkgs#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split jsp and jsp gui #31

Split jsp and jsp gui #31

clalancette commented Dec 19, 2019

matthew-reynolds left a comment

matthew-reynolds Dec 20, 2019

clalancette commented Dec 20, 2019

matthew-reynolds commented Dec 20, 2019

clalancette commented Jan 2, 2020

matthew-reynolds left a comment

sloretz left a comment

sloretz Jan 6, 2020

clalancette Jan 8, 2020

clalancette Jan 8, 2020

clalancette commented Jan 8, 2020

sloretz left a comment

clalancette commented Jan 13, 2020

matthew-reynolds commented Jan 28, 2020 •

edited

Loading

clalancette commented Jan 31, 2020 •

edited

Loading

clalancette commented Jan 31, 2020

matthew-reynolds commented Jan 31, 2020

matthew-reynolds commented Jan 31, 2020 •

edited

Loading

gavanderhoorn commented Feb 25, 2020 •

edited

Loading

clalancette commented Feb 26, 2020

ros-discourse commented Feb 26, 2020

ros-discourse commented Feb 15, 2021

Split jsp and jsp gui #31

Split jsp and jsp gui #31

Conversation

clalancette commented Dec 19, 2019

matthew-reynolds left a comment

Choose a reason for hiding this comment

matthew-reynolds Dec 20, 2019

Choose a reason for hiding this comment

clalancette commented Dec 20, 2019

matthew-reynolds commented Dec 20, 2019

clalancette commented Jan 2, 2020

matthew-reynolds left a comment

Choose a reason for hiding this comment

sloretz left a comment

Choose a reason for hiding this comment

sloretz Jan 6, 2020

Choose a reason for hiding this comment

clalancette Jan 8, 2020

Choose a reason for hiding this comment

clalancette Jan 8, 2020

Choose a reason for hiding this comment

clalancette commented Jan 8, 2020

sloretz left a comment

Choose a reason for hiding this comment

clalancette commented Jan 13, 2020

matthew-reynolds commented Jan 28, 2020 • edited Loading

clalancette commented Jan 31, 2020 • edited Loading

clalancette commented Jan 31, 2020

matthew-reynolds commented Jan 31, 2020

matthew-reynolds commented Jan 31, 2020 • edited Loading

gavanderhoorn commented Feb 25, 2020 • edited Loading

clalancette commented Feb 26, 2020

ros-discourse commented Feb 26, 2020

ros-discourse commented Feb 15, 2021

matthew-reynolds commented Jan 28, 2020 •

edited

Loading

clalancette commented Jan 31, 2020 •

edited

Loading

matthew-reynolds commented Jan 31, 2020 •

edited

Loading

gavanderhoorn commented Feb 25, 2020 •

edited

Loading