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

Add namespace capabilities to moveit_commander #835

Merged
merged 11 commits into from Apr 12, 2018

Conversation

Projects
None yet
4 participants
@willcbaker
Copy link
Contributor

willcbaker commented Apr 11, 2018

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)
@@ -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)

This comment has been minimized.

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

This comment has been minimized.

@davetcoleman

davetcoleman Apr 11, 2018

Member

please add standard BSD license with your name and short description

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

This comment has been minimized.

@davetcoleman

davetcoleman Apr 11, 2018

Member

please add standard BSD license with your name and short description

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

This comment has been minimized.

@davetcoleman

davetcoleman Apr 11, 2018

Member

please add standard BSD license with your name and short description

def tearDown(self):
pass

def check_target_setting(self, expect, *args):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

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):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

This comment has been minimized.

@willcbaker

willcbaker Apr 11, 2018

Author Contributor

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.

self.group.set_joint_value_target(target)
return self.group.plan()

def test_validation(self):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

def tearDown(self):
pass

def check_target_setting(self, expect, *args):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

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):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

def tearDown(self):
pass

def check_target_setting(self, expect, *args):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

same as above

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

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

This comment has been minimized.

@willcbaker

willcbaker Apr 11, 2018

Author Contributor

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

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

Makes sense

"Setting failed for %s, values: %s" % (type(args[0]), res))

def test_target_setting(self):
n = self.group.get_variable_count()

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

same as above

self.group.set_joint_value_target(target)
return self.group.compute_plan()

def test_validation(self):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

same as above

self.group.set_joint_value_target(target)
return self.group.plan()

def test_validation(self):

This comment has been minimized.

@mlautman

mlautman Apr 11, 2018

Member

same as above

Will Baker added some commits Apr 11, 2018

Will Baker
Will Baker
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Apr 12, 2018

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        
Will Baker

@davetcoleman davetcoleman merged commit 4598f66 into ros-planning:kinetic-devel Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Fix ros-planning#842
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 ros-planning#835.
@@ -348,7 +348,7 @@ def new(self, status, attr):

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

This comment has been minimized.

@rhaschke

rhaschke Apr 26, 2018

Contributor

@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?

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

bool setStateValueTarget(const std::string& state_str)

This comment has been minimized.

@rhaschke

rhaschke Apr 26, 2018

Contributor

@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!

This comment has been minimized.

@willcbaker

willcbaker May 31, 2018

Author Contributor

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.

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

std::string getJointValueTarget()

This comment has been minimized.

@rhaschke

rhaschke Apr 26, 2018

Contributor

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

This comment has been minimized.

@willcbaker

willcbaker May 31, 2018

Author Contributor

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
Copy link
Contributor

rhaschke left a comment

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

{
node_handle_ = ros::NodeHandle(ns);

This comment has been minimized.

@rhaschke

rhaschke Apr 26, 2018

Contributor

This local variable shouldn't have a trailing underscore.

This comment has been minimized.

@willcbaker

willcbaker May 31, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

rhaschke commented Apr 26, 2018

@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

This comment has been minimized.

Copy link
Member

davetcoleman commented May 14, 2018

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

@rhaschke rhaschke referenced this pull request May 14, 2018

Open

Fix PR #835 #893

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.