ros-industrial / motoman

ROS-Industrial Motoman support (http://wiki.ros.org/motoman)
145 stars 192 forks source link

Copying/moving SIA5D moveit_config from ros-industrial/motoman_experimental #350

Open tdl-ua opened 4 years ago

tdl-ua commented 4 years ago

Hello,

the moveit_config package for the SIA5D is currently in the ros-industrial/motoman_experimental repo. I've been using the package several times on a physical SIA5F manipulator and think the package would be a valuable addition to the motoman repository to comple the support SIA5D/F manipulators without having to overlay ros-industrial/motoman_experimental.

I can create a PR containing an exact copy of the SIA5D moveit_config package as currently found in the ros-industrial/motoman_experimental repo if this is something worth considering.

gavanderhoorn commented 4 years ago

re: migrating the package: yes, that would make sense.

In the past we've always maintained that MoveIt configurations are inherently application specific and therefore have been hesitant to add more here (and in other repositories), as users should really create one themselves to make sure it's completely up-to-date.

However, the current version of the MSA produces configurations which are not exactly working OotB, so having them here might make sense again.

re: submitting a PR: that would be appreciated, however, it would be good if we could migrate the Git history as well as the files.

There is a way to do that, but it involves some Git commands which you may not be familiar with.

tdl-ua commented 4 years ago

It makes sense to encourage users to make their own config packages. Now it also makes sense why there are so little moveit_config packages in this repo.

I can give the migration including the Git history a try. Shouldn't be too difficult because the package is contained within one folder I think.

gavanderhoorn commented 3 years ago

@tdl-ua: is this still something you would be interested in?

If so, I could provide you with some commands which would perform the "move" (in quotes, as it's really not a regular move, but a copy and delete).

tdl-ua commented 3 years ago

@gavanderhoorn: for sure, it is on my todo list.

Would be great if you could provide commands that ensure proper migration of the package.

gavanderhoorn commented 3 years ago

Provided the directory it is in has never changed, the following command should format a patch containing all commits ever done to that directory in the "source repository":

git log --pretty=email --patch-with-stat --reverse --full-index --binary -- 'motoman_sia5d_moveit_config' > ../motoman_sia5d_moveit_config-history.patch

then in the "destination repository" you could import this as follows:

git checkout -b importing_sia5d_moveit_cfg origin/kinetic-devel
git am --committer-date-is-author-date < ../motoman_sia5d_moveit_config-history.patch

Note that this is a relatively primitive way of doing this, so you may run into some issues.

In the end we get a branch (importing_sia5d_moveit_cfg) which contains the commits against motoman_sia5d_moveit_config in motoman_experimental with their original commit date (so some commits will be years in the past).

This obviously disconnects those commits from their history in motoman_experimental, but at least their chronological ordering is preserved, as well as their place relative to when things happened in ros-industrial/motoman itself.

EricMarcil commented 3 years ago

I though we wanted to limit the size of the motoman repo which is why we've stopped including the MoveIt support package. In most cases, the MoveIt package needs to be modified anyway to include end-user specific environment. If you ask me, we should move the few move-it package still in motoman to motoman_experimental.

gavanderhoorn commented 3 years ago

@EricMarcil wrote:

I though we wanted to limit the size of the motoman repo which is why we've stopped including the MoveIt support package.

MoveIt configurations are a few hundred KB generally. They're a collection of .launch and .yaml files.

Robot support packages are several MBs, each.

In most cases, the MoveIt package needs to be modified anyway to include end-user specific environment.

Yes, I also mentioned that in https://github.com/ros-industrial/motoman/issues/350#issuecomment-667198355.

If you ask me, we should move the few move-it package still in motoman to motoman_experimental.

The reason I believe there is some value in moving the SIA5 one here is that the current MoveIt Setup Assistant does not generate configurations which are really usable out-of-the-box. Especially the Gazebo side of things is really confusing, and there are some files which -- depending on how you configure things -- are actually incorrect.

So having a nr of MoveIt configurations here in this repository, perhaps even just as an example of how a configuration could be setup when used with motoman_driver, would be valuable.

I share your concerns about the size of the repository, but making sure not to include large .stls probably helps more.

From a maintenance perspective though, we should certainly not start providing MoveIt configurations for all supported robot variants.

tdl-ua commented 3 years ago

Just tried to apply the patch to my local copy of the motoman repository (freshly fetched and after creating and checking out a branch called importing_sia5d_moveit_cfg that is even with origin/kinetic-devel) and it failed. Below is what the command git am --committer-date-is-author-date < ../motoman_sia5d_moveit_config-history.patch returned:

Applying: Created sia5d moveit config package. IK solution is currently broken
.git/rebase-apply/patch:233: trailing whitespace.
  <!-- Launch the warehouse with a default database location -->  
.git/rebase-apply/patch:262: trailing whitespace.

.git/rebase-apply/patch:266: trailing whitespace.
    <param name="/use_gui" value="false"/> 
.git/rebase-apply/patch:269: trailing whitespace.

.git/rebase-apply/patch:275: trailing whitespace.
    <arg name="allow_trajectory_execution" value="true"/>  
warning: squelched 24 whitespace errors
warning: 29 lines add whitespace errors.
Applying: Add planning and execution for real robot
Applying: Remove call to rosservice '/robot_enable'
Applying: Remove call to rosservice '/robot_enable' (#36)
error: patch failed: motoman_sia5d_moveit_config/launch/moveit_planning_execution.launch:30
error: motoman_sia5d_moveit_config/launch/moveit_planning_execution.launch: patch does not apply
Patch failed at 0004 Remove call to rosservice '/robot_enable' (#36)
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I then also tried to apply the patch using --ignore-whitespace because some of the above warnings indicate issues with whitespaces but no luck (same printout as above).

I'm a little out of my depth here so any suggestions as to what can be done to apply this patch without errors are welcome.

If it turns out to be a major headache to copy the package including the history (which I can not really judge), I think it would not be a major loss to abandon this effort as the package is still available on the motoman_experimental repository.