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 and add MARA files #24

Closed
wants to merge 6 commits into from

Conversation

vmayoral
Copy link

Requesting @davetcoleman's review

@vmayoral
Copy link
Author

Obviously, this PR needs to be pointed to a new ros2 branch as usual

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Feb 26, 2019
with moveit_resources ported moveit/moveit_resources#24
this PR re-enables the testing and while allowing the
submodule to still compile
@vmayoral
Copy link
Author

@rhaschke?

@rhaschke
Copy link
Contributor

I don't have time to look into any PR before March 9th.
Reading that in the title that you want to add more robot file (MARA?) to this repo, I can already tell that I don't like the idea. This repo is for unit testing primarily and should be kept as small as possible.
@davetcoleman Any other opinions?

@vmayoral
Copy link
Author

@rhaschke I understand you want to keep it small but I can guess that most of the initial testing for MoveIt! 2 port will be with MARA. Shouldn't that justify adding it to the ros2 branch of it?


# devel space file
set(MOVEIT_TEST_RESOURCES_DIR ${CMAKE_CURRENT_SOURCE_DIR})
configure_file("include/${PROJECT_NAME}/config.h.in" "${CATKIN_DEVEL_PREFIX}/include/${PROJECT_NAME}/config.h")
Copy link
Member

Choose a reason for hiding this comment

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

@rhaschke i forget what this cmake magic was suppose to do. is it still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's needed to define MOVEIT_TEST_RESOURCES_DIR within cpp.
Should not be removed!

@@ -3,10 +3,11 @@ MoveIt Resources

This repository includes various resources (URDFs, meshes, moveit_config packages) needed for MoveIt! testing.

[![Build Status](https://travis-ci.org/ros-planning/moveit_resources.png?branch=master)](https://travis-ci.org/ros-planning/moveit_resources)
[![Build Status](https://travis-ci.org/AcutronicRobotics/moveit_resources.png?branch=master)](https://travis-ci.org/AcutronicRobotics/moveit_resources)
Copy link
Member

Choose a reason for hiding this comment

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

please do not change the Travis badges to Acutronic

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[![Build Status](https://travis-ci.org/AcutronicRobotics/moveit_resources.png?branch=master)](https://travis-ci.org/AcutronicRobotics/moveit_resources)
[![Build Status](https://travis-ci.org/ros-planning/moveit_resources.png?branch=master)](https://travis-ci.org/ros-planning/moveit_resources)

README.md Show resolved Hide resolved
@@ -0,0 +1,8 @@
# mara arm Gazebo
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# mara arm Gazebo
# MARA URDFs

Why gazebo?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@ahcorde, can you help with this please?

Copy link
Member

Choose a reason for hiding this comment

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

please accept my code suggestion (its easy!)

@@ -0,0 +1,4 @@
<?xml version="1.0"?>
<launch>
<!-- <param name="robot_description" command="$(find xacro)/xacro --inorder '$(find mara_description)/urdf/mara_nogripper.xacro'" /> -->
Copy link
Member

Choose a reason for hiding this comment

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

this should not be commented out

Copy link
Author

Choose a reason for hiding this comment

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

@ahcorde, ping

Copy link
Member

Choose a reason for hiding this comment

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

ping

@@ -0,0 +1,47 @@
# table.obj
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we need a table for unit tests. please re-add this in the future if you actually have a use-case for this

Copy link
Author

Choose a reason for hiding this comment

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

@ahcorde, can you help removing this please?

f 4/3/5 8/2/5 6/4/5
s 6
f 7/1/6 1/2/6 5/3/6
f 5/3/6 1/2/6 3/4/6
Copy link
Member

Choose a reason for hiding this comment

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

the next file is a png file... that is def not needed for tests

@v4hn
Copy link
Contributor

v4hn commented Feb 28, 2019 via email

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 5, 2019
with moveit_resources ported moveit/moveit_resources#24
this PR re-enables the testing while allowing the
submodule to still compile
@ahcorde
Copy link

ahcorde commented Mar 9, 2019

I think this PR should point to ros-planning:ros2 not to master, Am I right?.

Anyways I will remove the table.

@ahcorde
Copy link

ahcorde commented Mar 9, 2019

I just commit some changes. Leaving only the description of mara without additional hardware. Like fanuc or panda

@vmayoral
Copy link
Author

have all your concerns been addressed now @davetcoleman?

CMakeLists.txt Outdated
set(MOVEIT_TEST_RESOURCES_DIR ${CMAKE_INSTALL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION})
configure_file("include/${PROJECT_NAME}/config.h.in" "${CATKIN_DEVEL_PREFIX}/include/${PROJECT_NAME}/.config_install.h")
set(MOVEIT_TEST_RESOURCES_DIR "share/${PROJECT_NAME}")
configure_file("include/${PROJECT_NAME}/config.h.in" "include/${PROJECT_NAME}/.config_install.h")
Copy link
Member

Choose a reason for hiding this comment

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

@rhaschke this looks good to me, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't ament support devel and install space anymore?
This change, hardcoding the install location (in share), looks like breaking devel-space usage.

@@ -3,10 +3,11 @@ MoveIt Resources

This repository includes various resources (URDFs, meshes, moveit_config packages) needed for MoveIt! testing.

[![Build Status](https://travis-ci.org/ros-planning/moveit_resources.png?branch=master)](https://travis-ci.org/ros-planning/moveit_resources)
[![Build Status](https://travis-ci.org/AcutronicRobotics/moveit_resources.png?branch=master)](https://travis-ci.org/AcutronicRobotics/moveit_resources)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[![Build Status](https://travis-ci.org/AcutronicRobotics/moveit_resources.png?branch=master)](https://travis-ci.org/AcutronicRobotics/moveit_resources)
[![Build Status](https://travis-ci.org/ros-planning/moveit_resources.png?branch=master)](https://travis-ci.org/ros-planning/moveit_resources)

@@ -0,0 +1,8 @@
# mara arm Gazebo
Copy link
Member

Choose a reason for hiding this comment

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

please accept my code suggestion (its easy!)

@@ -0,0 +1,4 @@
<?xml version="1.0"?>
<launch>
<!-- <param name="robot_description" command="$(find xacro)/xacro --inorder '$(find mara_description)/urdf/mara_nogripper.xacro'" /> -->
Copy link
Member

Choose a reason for hiding this comment

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

ping

@davetcoleman
Copy link
Member

@ahcorde agreed it should target ros2 branch, I'll switch it

@davetcoleman davetcoleman changed the base branch from master to ros2 March 13, 2019 01:01
davetcoleman pushed a commit to moveit/moveit2 that referenced this pull request Apr 5, 2019
* Port robot_model submodule of moveit_core to ROS 2

* Clean submodule CMakeLists.txt

Remove unnecesary commands listed already one level higher

* robot_model, simplify CMakeLists.txt

Following guidelines provided at
#8 (comment)

* Fix testing and comment it out for now

moveit_resources hasn't been ported just yet

* robot_model, cleanup code

* Revert changes, reenable properties version

* robot_model re-enable tests

with moveit_resources ported moveit/moveit_resources#24
this PR re-enables the testing while allowing the
submodule to still compile

* robot_model submodule, fix logging

* robot_model use LOGGER

Follow consensus about logging in moveit_core
submodules

* Remove robot_model log.h

* robot_model, apply clang format

* moveit_core, robot_model: remove setDefaultIKAttempts
@henningkayser
Copy link
Member

Closing in favor of #26

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.

8 participants