Add namespace capabilities to moveit_commander #835

willcbaker · 2018-04-11T20:07:09Z

Description

Add namespace compatibility and tests to moveit commander

Checklist

Required: Code is auto formatted using clang-format
Extended the tutorials / documentation, if necessary reference
Include a screenshot if changing a GUI
Optional: Created tests, which fail without this PR reference
Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

davetcoleman · 2018-04-11T21:03:52Z

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

@@ -349,7 +379,7 @@ class MoveGroupInterfaceWrapper : protected py_bindings_tools::ROScppInitializer
    std::map<std::string, double> positions = getNamedTargetValues(name);
    std::map<std::string, double>::iterator iterator;

-    for (iterator = positions.begin(); iterator != positions.end(); iterator++)
+    for (iterator = positions.begin(); iterator != positions.end(); ++iterator)


davetcoleman · 2018-04-11T21:23:18Z

moveit_commander/test/python_moveit_commander.py

@@ -0,0 +1,68 @@
+#!/usr/bin/env python
+


please add standard BSD license with your name and short description

davetcoleman · 2018-04-11T21:23:28Z

moveit_commander/test/python_moveit_commander_ns.py

@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+


please add standard BSD license with your name and short description

davetcoleman · 2018-04-11T21:23:56Z

moveit_ros/planning_interface/test/python_move_group_ns.py

@@ -0,0 +1,68 @@
+#!/usr/bin/env python
+


please add standard BSD license with your name and short description

mlautman · 2018-04-11T21:27:17Z

moveit_commander/test/python_moveit_commander.py

+    def tearDown(self):
+        pass
+
+    def check_target_setting(self, expect, *args):


rename something like test__group__set_joint_value_target to make it easier to for future devs to understand what this test does

mlautman · 2018-04-11T21:29:24Z

moveit_commander/test/python_moveit_commander.py

+        self.assertTrue(np.all(np.asarray(res) == np.asarray(expect)),
+                        "Setting failed for %s, values: %s" % (type(args[0]), res))
+
+    def test_target_setting(self):


I think I understand this test but it took me a few minutes to parse what you are doing here. You are testing all the different ways to assign joint values correct? Changing test_target_setting to something like test__group__set_joint_value_target__methods will go a long way helping future devs debug issues if these tests ever break

I've just copy and pasta'd the tests from here:
https://github.com/ros-planning/moveit/blob/kinetic-devel/moveit_ros/planning_interface/test/python_move_group.py
to test basic functionality.. its definitely more duplicated testing than intended.

mlautman · 2018-04-11T21:31:15Z

moveit_commander/test/python_moveit_commander.py

+        self.group.set_joint_value_target(target)
+        return self.group.plan()
+
+    def test_validation(self):


rename something like test__group__execute_path_validation to make it easier to understand what this test does

mlautman · 2018-04-11T21:32:18Z

moveit_commander/test/python_moveit_commander_ns.py

+    def tearDown(self):
+        pass
+
+    def check_target_setting(self, expect, *args):


rename something like test__group__set_joint_value_target to make it easier to understand what this test does

mlautman · 2018-04-11T21:36:02Z

moveit_commander/test/python_moveit_commander_ns.py

+        self.assertTrue(np.all(np.asarray(res) == np.asarray(expect)),
+                        "Setting failed for %s, values: %s" % (type(args[0]), res))
+
+    def test_target_setting(self):


rename something like test__group__set_joint_value_target__methods to make it easier to understand what this test does

mlautman · 2018-04-11T21:42:29Z

moveit_ros/planning_interface/test/python_move_group_ns.py

+    def tearDown(self):
+        pass
+
+    def check_target_setting(self, expect, *args):


same as above

Also, why the copy-paste? Is there a reason why you test these functions three times?

the magic is all in the initialization of the test; was just ensuring "functionality" aka the robot can plan and execute as demonstrated in MoveGroupCommander tests. so I believe something like these tests should be tested against in each case

Makes sense

mlautman · 2018-04-11T21:44:15Z

moveit_ros/planning_interface/test/python_move_group_ns.py

+                        "Setting failed for %s, values: %s" % (type(args[0]), res))
+
+    def test_target_setting(self):
+        n = self.group.get_variable_count()


same as above

mlautman · 2018-04-11T21:44:25Z

moveit_ros/planning_interface/test/python_move_group_ns.py

+        self.group.set_joint_value_target(target)
+        return self.group.compute_plan()
+
+    def test_validation(self):


same as above

mlautman · 2018-04-11T21:44:58Z

moveit_commander/test/python_moveit_commander_ns.py

+        self.group.set_joint_value_target(target)
+        return self.group.plan()
+
+    def test_validation(self):


same as above

davetcoleman · 2018-04-12T19:18:48Z

The tests are failing. I've re-run the test locally and they also fail for me:

[moveit_ros_planning_interface:make] Traceback (most recent call last):                                                                                                                                                                       
[moveit_ros_planning_interface:make]   File "/home/dave/ros/current/ws_moveit/src/moveit/moveit_ros/planning_interface/test/movegroup_interface.py", line 5, in <module>                                                                      
[moveit_ros_planning_interface:make]     group = MoveGroupInterface("manipulator", "robot_description", rospy.get_namespace())                                                                                                                
[moveit_ros_planning_interface:make] Boost.Python.ArgumentError: Python argument types in                                                                                                                                                     
[moveit_ros_planning_interface:make]     MoveGroupInterface.__init__(MoveGroupInterface, str, str, str)                                                                                                                                       
[moveit_ros_planning_interface:make] did not match C++ signature:                                                                                                                                                                             
[moveit_ros_planning_interface:make]     __init__(_object*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)                 
[moveit_ros_planning_interface:make] [Testcase: testcleanup_tests] ... ok

Adds a optional namespace argument to the python side of planning_scene_interface and passes it to the wrapped C++, which expects a namespace after moveit#835.

rhaschke · 2018-04-26T16:00:30Z

moveit_ros/visualization/src/moveit_ros_visualization/moveitjoy_module.py

@@ -348,7 +348,7 @@ def new(self, status, attr):

 class MoveitJoy:
    def parseSRDF(self):
-        ri = RobotInterface("/robot_description")
+        ri = RobotInterface("/robot_description", "")


@willcbaker #842 shows a similar issue like the one fixed here.
Why did you added an empty namespace here, instead of using an empty default in the RobotInterface constructor?

rhaschke · 2018-04-26T16:03:35Z

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

@@ -109,6 +110,15 @@ class MoveGroupInterfaceWrapper : protected py_bindings_tools::ROScppInitializer
    return setJointValueTarget(js_msg);
  }

+  bool setStateValueTarget(const std::string& state_str)


@willcbaker I didn't realize that you just recently added this function.
This is highly dangerous, as it allows setting of joints that are not part of the JointModelGroup!

Exposing a publicly available c++ move group method
http://docs.ros.org/jade/api/moveit_ros_planning_interface/html/classmoveit_1_1planning__interface_1_1MoveGroup.html#ad35630dd853a734de3580390facf2c9b
iirc it was (is?) the only way to set a desired multi dof joint state.

rhaschke · 2018-04-26T16:04:35Z

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

@@ -118,6 +128,14 @@ class MoveGroupInterfaceWrapper : protected py_bindings_tools::ROScppInitializer
    return l;
  }

+  std::string getJointValueTarget()


Same here. Why do you want to expose the internal RobotState?

It's just a copy of the state, not quite exposing the internal rs. This compliments above; yes this might be possible to instead use a list like get Joint Value Target with joint states but does not have a clear explicit way to handle multi dof joints

rhaschke

@willcbaker Please take a look at my post-merge comments on this PR, particularly in regard of #861.

rhaschke · 2018-04-26T16:06:34Z

moveit_ros/planning_interface/planning_scene_interface/src/planning_scene_interface.cpp

  {
+    node_handle_ = ros::NodeHandle(ns);


This local variable shouldn't have a trailing underscore.

Correct, I think this was a rebase artifact as I had previously stored this namespace as a private variable in the class for some functionality that did was not targeted for this PR

rhaschke · 2018-04-26T22:16:04Z

@willcbaker This change-set caused several test regressions on the ROS build farm:
http://build.ros.org/job/Kdev__moveit__ubuntu_xenial_amd64/257/
http://build.ros.org/job/Ldev__moveit__ubuntu_xenial_amd64/184/

Please have a look and fix them asap. Otherwise, we should revert those changes.
Obviously Travis doesn't catch these test failures. @davetcoleman Do you have an idea why?

The reason for this was that Travis used an updated moveit_resources from source, while the build farm uses the released version.

davetcoleman · 2018-05-14T02:32:14Z

no i don't know why the build farm failed to catch this!

Will Baker added 8 commits April 11, 2018 10:42

Add python goodies

63b8afa

Update planning interface tests for namespace arg

f640008

Add namespace tests

4a04ecf

Update moveit commander for namespacing

1b56d4f

Add moveit commander tests

cc60eaa

Add movegroup test in namespace

388da27

Update param scopes in template and launch files

627782b

Code formatting

0bcc54b

willcbaker mentioned this pull request Apr 11, 2018

Feature/markers and states #836

Merged

davetcoleman reviewed Apr 11, 2018

View reviewed changes

mlautman suggested changes Apr 11, 2018

View reviewed changes

Will Baker added 2 commits April 11, 2018 17:26

Add BSD License and name

5334cb5

Add description

b9fb86d

Fix joy test

48fc9ce

davetcoleman approved these changes Apr 12, 2018

View reviewed changes

davetcoleman merged commit 4598f66 into moveit:kinetic-devel Apr 12, 2018

This was referenced Apr 16, 2018

Move group with namespace #779

Closed

multiple move_group nodes in different name spaces #757

Closed

BryceStevenWilley mentioned this pull request Apr 26, 2018

Issue while running move_group_python_interface_tutorial.py #842

Closed

BryceStevenWilley added a commit to BryceStevenWilley/moveit that referenced this pull request Apr 26, 2018

Fix moveit#842

665178c

Adds a optional namespace argument to the python side of planning_scene_interface and passes it to the wrapped C++, which expects a namespace after moveit#835.

rhaschke reviewed Apr 26, 2018

View reviewed changes

rhaschke mentioned this pull request Apr 26, 2018

test regression on ROS build farm #865

Closed

rhaschke mentioned this pull request May 14, 2018

Fix PR #835 #893

Open

rhaschke mentioned this pull request May 31, 2018

allow optional namespace args #929

Merged

rhaschke mentioned this pull request Jun 13, 2019

Add get_jacobian_matrix to moveit_commander #1501

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace capabilities to moveit_commander #835

Add namespace capabilities to moveit_commander #835

willcbaker commented Apr 11, 2018 •

edited

Loading

davetcoleman Apr 11, 2018

davetcoleman Apr 11, 2018

davetcoleman Apr 11, 2018

davetcoleman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

willcbaker Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

willcbaker Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

mlautman Apr 11, 2018

davetcoleman commented Apr 12, 2018 •

edited

Loading

rhaschke Apr 26, 2018

rhaschke Apr 26, 2018 •

edited

Loading

willcbaker May 31, 2018

rhaschke Apr 26, 2018

willcbaker May 31, 2018

rhaschke left a comment

rhaschke Apr 26, 2018

willcbaker May 31, 2018

rhaschke commented Apr 26, 2018 •

edited

Loading

davetcoleman commented May 14, 2018

Add namespace capabilities to moveit_commander #835

Add namespace capabilities to moveit_commander #835

Conversation

willcbaker commented Apr 11, 2018 • edited Loading

Description

Checklist

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

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

davetcoleman commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

rhaschke Apr 26, 2018 • edited Loading

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

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhaschke commented Apr 26, 2018 • edited Loading

davetcoleman commented May 14, 2018

willcbaker commented Apr 11, 2018 •

edited

Loading

davetcoleman commented Apr 12, 2018 •

edited

Loading

rhaschke Apr 26, 2018 •

edited

Loading

rhaschke commented Apr 26, 2018 •

edited

Loading