RQT Parameters for Ros2 #31

gonzodepedro · 2019-05-31T21:05:39Z

No description provided.

Signed-off-by: Gonzalo de Pedro <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

Signed-off-by: Gonzalo de Pedro <[email protected]>

cottsay · 2019-05-31T21:14:30Z

Let's create a dashing branch for these changes. I'll create the branch from the current master - if you could re-create this PR against that branch so the changes can be reviewed, that would be great.

gonzodepedro · 2019-06-03T19:41:26Z

Done

package.xml

cottsay · 2019-06-03T19:47:11Z

package.xml

@@ -23,17 +23,18 @@
 	<author>Isaac Saito</author>
 	<author>Ze'ev Klapow</author>

-	<buildtool_depend>catkin</buildtool_depend>
+	<buildtool_depend>ament_cmake</buildtool_depend>


I don't see anything fancy happening in the CMakeLists.txt - is there a reason not to just make this an ament_python package?

I agree. Changed to 'ament_python"

This didn't happen. An ament_python package doesn't include a CMakeLists.txt.

We'll also need a more robust setup.py like this:

from setuptools import setup package_name = 'rqt_reconfigure' setup( name=package_name, version='1.0.1', package_dir={'': 'src'}, packages=[package_name], data_files=[ ('share/ament_index/resource_index/packages', ['resource/' + package_name]), ('share/' + package_name, ['package.xml', 'plugin.xml']), ('share/' + package_name + '/resource', [ 'resource/editor_bool.ui', 'resource/editor_enum.ui', 'resource/editor_number.ui', 'resource/editor_string.ui', 'resource/node_selector.ui', 'resource/paramedit_pane.ui', 'resource/param_main.ui', 'resource/param_timeline.ui', 'resource/singlenode_parameditor.ui', 'resource/text_filter_widget.ui', ]), ], zip_safe=True, author='Isaac Saito, Ze\'ev Klapow', maintainer='Scott K Logan', maintainer_email='[email protected]', keywords=['ROS'], classifiers=[ 'Intended Audience :: Developers', 'License :: OSI Approved :: BSD License', 'Programming Language :: Python', 'Topic :: Software Development', ], description=( 'This rqt plugin succeeds former dynamic_reconfigure\'s GUI ' + '(reconfigure_gui), and provides the way to view and edit the parameters ' + 'that are accessible via dynamic_reconfigure.' ), license='BSD', entry_points={ 'console_scripts': [ 'rqt_reconfigure = rqt_reconfigure.main:main', ], }, )

dirk-thomas · 2019-06-06T17:52:43Z

The current patch mixes a lot of unrelated changes. E.g. whitespace / formatting changes should be done separately - they could land in master - and then this PR will focus on changes related to ROS 2. The same goes for other non-ROS 2 changes like changing the permission flags of the test files.

cottsay

I'd like to find a way to keep the log statements the same across ROS 1 and 2. I'll try to make a PR to the ROS 1 branch soon. Hopefully this will minimize the drift.

package.xml

test/test_parameters_service.py

Signed-off-by: Gonzalo de Pedro <[email protected]>

src/rqt_reconfigure/param_editors.py

src/rqt_reconfigure/node_selector_widget.py

cottsay · 2019-07-11T01:27:32Z

plugin.xml

@@ -1,5 +1,5 @@
 <library path="src">
-  <class name="Param" type="rqt_reconfigure.param_plugin.ParamPlugin" base_class_type="rqt_gui_py::Plugin">
+  <class name="ParamPlugin" type="rqt_reconfigure.param_plugin.ParamPlugin" base_class_type="rqt_gui_py::Plugin">


I'm curious about this change - is it to conform to a pattern? Should it be made in ROS 1 as well?

Signed-off-by: Gonzalo de Pedro <[email protected]>

This will make the package act like it did before - run by invoking `ros2 run rqt_reconfigure rqt_reconfigure` instead of only `rqt_reconfigure`.

Signed-off-by: Scott K Logan <[email protected]>

cottsay

I'm still curious about the change to plugin.xml, and there is still a lot of alignment we could do, but I think it is OK to merge this change now. All of my tests are coming back green.

Thanks for working with me to keep this package maintainable.

dirk-thomas · 2019-07-23T17:38:40Z

src/rqt_reconfigure/filter_children_model.py

-            _child_index = curr_qmindex.child(0, 0)
-            if ((not _child_index.isValid()) and
+            child_index = curr_qmindex.child(0, 0)
+            if ((not child_index.isValid()) and


Please either revert unnecessary / unrelated changes like this or apply the same diff on the ROS 1 branch.

dirk-thomas · 2019-07-23T17:39:27Z

src/rqt_reconfigure/node_selector_widget.py

@@ -328,8 +330,11 @@ def _update_nodetree_pernode(self):

                # Instantiate QStandardItem. Inside, dyn_reconf client will
                # be generated too.
+


Please revert unnecessary / unrelated changes like this to keep the diff between the branches minimal.

dirk-thomas · 2019-07-23T17:40:23Z

src/rqt_reconfigure/param_api.py

@@ -0,0 +1,136 @@
+# Software License Agreement (BSD License)
+#
+# Copyright (c) 2012, Willow Garage, Inc.


If this file is new the copyright should be OSRF.

dirk-thomas · 2019-07-23T17:41:03Z

src/rqt_reconfigure/param_api.py

+
+    def __init__(self, node, remote_node_name, param_change_callback=None):
+        """
+        """


Please either remove the empty docblock or add actual documentation to it.

dirk-thomas · 2019-07-23T17:42:01Z

src/rqt_reconfigure/param_client_widget.py

+        # TODO: .ui file needs to be back into usage in later phase.
+        # ui_file = os.path.join(rp.get_path('rqt_reconfigure'),
+        #                        'resource', 'singlenode_parameditor.ui')
+        # loadUi(ui_file, self)


What about this TODO?

dirk-thomas · 2019-07-23T17:42:16Z

src/rqt_reconfigure/param_client_widget.py

+
+        # Labels should not stretch
+        # self.grid.setColumnStretch(1, 1)
+        # self.setLayout(self.grid)


Why is this commented out?

dirk-thomas · 2019-07-23T17:43:33Z

src/rqt_reconfigure/param_editors.py

+                                 ).triggered.connect(self._set_to_min)
+
+            # TODO: Fix that the naming of _paramval_lineEdit instance is not
+            #       consistent among Editor's subclasses.


What about this TODO?

dirk-thomas · 2019-07-23T17:44:49Z

src/rqt_reconfigure/param_widget.py

        # TODO: .ui file needs to replace the GUI components declaration
        #       below. For unknown reason, referring to another .ui files
        #       from a .ui that is used in this class failed. So for now,
        #       I decided not use .ui in this class.
        #       If someone can tackle this I'd appreciate.
-        _hlayout_top = QHBoxLayout(self)
-        _hlayout_top.setContentsMargins(QMargins(0, 0, 0, 0))
+        hlayout_top = QHBoxLayout(self)


What is the reason to rename all these variables?

Please either revert unnecessary / unrelated changes like this or apply the same diff on the ROS 1 branch.

dirk-thomas · 2019-07-23T17:45:35Z

src/rqt_reconfigure/paramedit_widget.py

        ))

    def _node_disabled(self, node_grn):
        logging.debug('paramedit_w _node_disabled grn={}'.format(node_grn))
+        # self._checkbox.setChecked(value) # TODO(Gonzo) signal doesn't work


What about this comment / TODO?

Follow up PR to address comments made on already committed PR #31 Signed-off-by: Gonzalo de Pedro <[email protected]> Signed-off-by: Scott K Logan <[email protected]>

Gonzalo de Pedro and others added 6 commits May 28, 2019 14:07

Migration to ROS2

745c449

Signed-off-by: Gonzalo de Pedro <[email protected]>

Changes to fix bugs

ae96072

Signed-off-by: Gonzalo de Pedro <[email protected]>

Changes to readonly, step, and string edit

39b7fe3

Signed-off-by: Gonzalo de Pedro <[email protected]>

Stabilize migration to ROS2.

47f8500

Signed-off-by: Michel Hidalgo <[email protected]>

Minor fixes

01579da

Signed-off-by: Gonzalo de Pedro <[email protected]>

Do not show nodes that don't publish any parameter

2f3657c

Signed-off-by: Gonzalo de Pedro <[email protected]>

gonzodepedro self-assigned this May 31, 2019

gonzodepedro closed this Jun 3, 2019

gonzodepedro reopened this Jun 3, 2019

gonzodepedro changed the base branch from master to dashing June 3, 2019 19:40

cottsay requested changes Jun 3, 2019

View reviewed changes

Fixes based on review

ba517ea

Reverted non relevant changes

fba0086

gonzodepedro requested a review from cottsay June 19, 2019 18:14

cottsay requested changes Jun 25, 2019

View reviewed changes

package.xml Show resolved Hide resolved

package.xml Outdated Show resolved Hide resolved

test/test_parameters_service.py Outdated Show resolved Hide resolved

Gonzalo de Pedro added 2 commits June 27, 2019 15:59

Addressed comments

bd97bf6

Signed-off-by: Gonzalo de Pedro <[email protected]>

Removed comment

cf4daf0

Signed-off-by: Gonzalo de Pedro <[email protected]>

gonzodepedro requested a review from cottsay June 28, 2019 18:50

cottsay requested changes Jul 11, 2019

View reviewed changes

Fixed no range on parameters description

0cb61f2

Signed-off-by: Gonzalo de Pedro <[email protected]>

gonzodepedro requested a review from cottsay July 17, 2019 15:50

cottsay added 5 commits July 18, 2019 16:38

Merge branch 'dashing' into dashing-devel

fcc8092

Switch away from entry points for command line

97c71c7

This will make the package act like it did before - run by invoking `ros2 run rqt_reconfigure rqt_reconfigure` instead of only `rqt_reconfigure`.

Add ament tests

46c47f4

Merge branch 'dashing' into dashing-devel

3d5245f

Revert some unnecessary superficial changes

f38aaa8

Signed-off-by: Scott K Logan <[email protected]>

cottsay approved these changes Jul 20, 2019

View reviewed changes

gonzodepedro merged commit c72ab76 into dashing Jul 22, 2019

cottsay deleted the dashing-devel branch July 23, 2019 14:49

dirk-thomas reviewed Jul 23, 2019

View reviewed changes

wjwwood mentioned this pull request Jul 23, 2019

ROS 2 Eloquent Elusor ros2/ros2#734

Closed

34 tasks

gonzodepedro mentioned this pull request Jul 24, 2019

Dashing devel followup #38

Merged

cottsay pushed a commit that referenced this pull request Jul 31, 2019

Dashing devel followup (#38)

56eb484

Follow up PR to address comments made on already committed PR #31 Signed-off-by: Gonzalo de Pedro <[email protected]> Signed-off-by: Scott K Logan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RQT Parameters for Ros2 #31

RQT Parameters for Ros2 #31

gonzodepedro commented May 31, 2019

cottsay commented May 31, 2019

gonzodepedro commented Jun 3, 2019

cottsay Jun 3, 2019

gonzodepedro Jun 5, 2019

cottsay Jun 25, 2019

gonzodepedro Jun 27, 2019

dirk-thomas commented Jun 6, 2019

cottsay left a comment

cottsay Jul 11, 2019

cottsay left a comment

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

dirk-thomas Jul 23, 2019

		@@ -328,8 +330,11 @@ def _update_nodetree_pernode(self):

		# Instantiate QStandardItem. Inside, dyn_reconf client will
		# be generated too.

RQT Parameters for Ros2 #31

RQT Parameters for Ros2 #31

Conversation

gonzodepedro commented May 31, 2019

cottsay commented May 31, 2019

gonzodepedro commented Jun 3, 2019

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

dirk-thomas commented Jun 6, 2019

cottsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottsay 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

Choose a reason for hiding this comment