Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Levi-Armstrong · 2015-07-31T03:43:58Z

The abb_irb6600_support contained the irb6640 not the irb6600. A duplicate of the irb6640 will remain in the abb_irb6600_support package until Jade

gavanderhoorn · 2015-07-31T13:06:17Z

abb_irb6600_support/package.xml

@@ -38,4 +38,8 @@
  <run_depend>joint_state_publisher</run_depend>
  <run_depend>robot_state_publisher</run_depend>
  <run_depend>rviz</run_depend>
+
+  <export>
+    <deprecated> All files related to the irb6640 will be removed in Jade, use the abb_irb6640_support package going forward. </deprecated>


'please use the ..' :) ?

gavanderhoorn · 2015-07-31T13:07:25Z

Should we perhaps put this package in abb_experimental first? Or are we assuming things are ok because it's essentially a copy of what was in abb_irb6600_support all this time?

gavanderhoorn · 2015-07-31T13:08:07Z

abb_irb6640_support/launch/test_irb6640_185_280.launch

+        <param name="use_gui" value="true" />
+        <node name="joint_state_publisher" pkg="joint_state_publisher" type="joint_state_publisher" />
+        <node name="robot_state_publisher" pkg="robot_state_publisher" type="robot_state_publisher" />
+        <node name="rviz" pkg="rviz" type="rviz" args="-d $(find industrial_robot_client)/config/robot_state_visualize.rviz" required="true" />


Indentation

gavanderhoorn · 2015-07-31T13:21:14Z

Regarding the subdirs of meshes: for Fanucs it is often the case that variants can share a lot of meshes (especially long/short variants), with only a single / few link(s) that are actually different. Is that something we could exploit here as well?

It's a minor thing, but would cut down the size of the package, as well as reduce the amount of work of adding support for a new variant as we don't have to convert all the meshes every time.

If we can, then the reach should probably not be included in the name of the meshes subdir.

If the payload influences the geometry of the robot as well, then sharing may not be as convenient anymore.

gavanderhoorn · 2015-07-31T13:22:36Z

abb_irb6640_support/CHANGELOG.rst

@@ -0,0 +1,51 @@
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


This package, although mostly a copy (content wise) of another, has not yet been released, so it seems strange to include the changelog of another pkg here.

Levi-Armstrong · 2015-07-31T14:41:55Z

Should we perhaps put this package in abb_experimental first? Or are we assuming things are ok because it's essentially a copy of what was in abb_irb6600_support all this time?

I assumed because it was just a copy and a rename to keep it in the abb repository. After reviewing the link offsets, I believe they are not correct per the product specification. Some of the links are only off my a few millimeters but I believe that this should eventual be corrected.

I see three option:
option 1: Hold off until I have time to fix the geometry files.
option 2: Go ahead and merge once everything corrected minus the geometry and create an issue.
option 3: Submit this PR against the abb_experimental and create an issue to fix the geometry.

I am leaning toward option 3, what do you think?

gavanderhoorn · 2015-08-02T17:03:57Z

@Levi-Armstrong wrote:

I assumed because it was just a copy and a rename to keep it in the abb repository.

yeah, that would make the most sense, as it is really a copy, content-wise.

After reviewing the link offsets, I believe they are not correct per the product specification. Some of the links are only off my a few millimeters but I believe that this should eventual be corrected.

Yes, we should certainly fix that. The offsets could be the result of using the SolidWorks to URDF plugin without manually defining and / or correcting the link origins.

'luckily' we have a copy of the old structure in the irb6600_support pkg, so users looking for bw-compatibility could keep using that.

I am leaning toward option 3, what do you think?

Option 3 would be the quickest way to move forward. Option 1 seems like the option which would cause the least amount of PRs introducing new pkgs and correcting things, which is also nice.

We could go with 3: it's against an _experimental repository, so we have time there to fix things. Afterwards, we can then migrate the pkg to the abb repository.

gavanderhoorn · 2015-09-05T15:58:02Z

I think this pkg looks ok, but I've one thing I'd like to check: do the 7 variants of the IRB 6640 share any link geometry (so everything up to link_4 is identical fi)?

If so, could we introduce meshes/irb6640/{collsion,visual} folders with the common parts, and then suitable meshes/irb6640_x_y/{collsion,visual} folders with just the differences? With 7 variants it should be worth the effort.

gavanderhoorn · 2015-09-05T16:12:20Z

@Levi-Armstrong wrote:

After reviewing the link offsets, I believe they are not correct per the product specification. Some of the links are only off my a few millimeters but I believe that this should eventual be corrected.

Both link_6 and tool0 look like they are off by more than a few millimetres. tool0 is floating in the air at quite a distance from the flange.

gavanderhoorn · 2015-09-05T16:32:56Z

Btw, should abb_irb6640_moveit_config be changed to load files from the new abb_irb6640_support, instead of the old abb_irb6600_support?

Levi-Armstrong · 2015-09-07T04:59:18Z

@gavanderhoorn: Since there were several issues with the urdf, I went ahead and downloaded the model and created the urdf from scratch. It now matches the product specification and it now includes .dae files for the visual mesh.

Btw, should abb_irb6640_moveit_config be changed to load files from the new abb_irb6640_support, instead of the old abb_irb6600_support?

I was not sure on this, but know that I have thought about it is unlikely that anyone is using this other than to visualize the model. If they were going to create their own setup they create it using the robot support package; so I think it would be best to go ahead and update the abb_irb6640_moveit_config. Do you agree?

gavanderhoorn · 2015-09-07T08:14:59Z

abb_irb6600_support/urdf/irb6640.xacro

+  <xacro:abb_irb6640 prefix=""/>
+
+</robot>
+


remove empty lines

gavanderhoorn · 2015-09-07T08:37:57Z

@Levi-Armstrong wrote:

@gavanderhoorn: Since there were several issues with the urdf, I went ahead and downloaded the model and created the urdf from scratch. It now matches the product specification and it now includes .dae files for the visual mesh.

nice. Lots of detail there. The colours aren't as 'popping' as they used to be, but that is more a rendering issue I believe.

re: moveit config: I'm not sure. Updating the MoveIt config really would make this a non-bw-compatible PR. Especially since the piston & cylinder are now non-fixed joints. That's why I asked.

On the other hand: what is there right now is really wrong, so we might just take the opportunity to fix things up completely.

Two other minor things:

could you add a readme to the source_data/meshes dir explaining where you got the originals from and what steps you took to convert them to Collada
could you (re)save the .blend files with compression enabled? Could save something like 60% on file sizes

Levi-Armstrong · 2015-09-27T03:52:02Z

@gavanderhoorn: Now that the source_data directory has been removed is it ready to be merged?

gavanderhoorn · 2015-09-28T12:46:00Z

Have you checked the MoveIt config? With these updated files I get some unexpected collisions between link_cylinder and link_2.

Also: the piston seems to be completely ignored. This is probably not really important, but looks strange.

Levi-Armstrong · 2015-10-13T16:19:13Z

@gavanderhoorn, I disabled the collision between link_cylinder and link_2 it should never be in collision. I was showing collision because the convex hull of link_2 simplified the relief in the back of link_2.

I also noticed that when dragging the robot it is not updating the mimic links but when planning and executing they get updated. Do you think this is an error within the Motion Planning plugin for RVIZ?

gavanderhoorn · 2015-10-26T15:57:10Z

I also noticed that when dragging the robot it is not updating the mimic links but when planning and executing they get updated. Do you think this is an error within the Motion Planning plugin for RVIZ?

Not sure whether it is really an error, but I've noticed this with other robots with parallel links as well. I believe mimic and / or virtual joints are not updated during dragging.

Some minor things:

from the way default_warehouse_db.launch is modified, I believe you used the latest released version of the MoveIt Setup Assistant, correct? That reverts 3757daa. If we want to be nice, we should regenerate the pkg using a source checkout of the MSA.
package manifest of abb_irb6640_moveit_config should be reverted to how it was (before update)
MoveIt config looses its acceleration limits (but I'm not too sure how important we think this is)
RViz config is overwritten

Other than those things this looks ok now.

shaun-edwards · 2016-01-11T23:43:40Z

Can this be merged?

Levi-Armstrong · 2016-01-12T03:56:05Z

@gavanderhoorn, Since the visual and collision model were updated, where the piston and cylinder is now non-fixed and the link offsets were corrected; would it be safe to say this is a new package and leave the updated moveit_config?

gavanderhoorn · 2016-01-12T13:35:22Z

I'm not sure I follow @Levi-Armstrong? "would it be safe to say this is a new package and leave the updated moveit_config" ?

Levi-Armstrong · 2016-01-12T14:44:13Z

The comment was in reference to the statement below. Is it ok to leave the updated moveit_config package since several things about the robot were corrected/updated? Also the other package will be around until Jade to provide time for users to transition to the new package for those current using the old package.

Some minor things:

from the way default_warehouse_db.launch is modified, I believe you used the latest released version of the MoveIt Setup Assistant, correct? That reverts 3757daa. If we want to be nice, we should regenerate the pkg using a source checkout of the MSA.
package manifest of abb_irb6640_moveit_config should be reverted to how it was (before update)
MoveIt config looses its acceleration limits (but I'm not too sure how important we think this is)
RViz config is overwritten

Levi-Armstrong · 2016-02-04T04:20:57Z

@gavanderhoorn, Is this OK to merge based on my previous comment?

gavanderhoorn · 2016-02-04T18:36:08Z

Updating the MoveIt pkg was fine. I just thought it'd be nice to keep the original manifest (package.xml) as that was overwritten by the MSA, while not containing anything that might need to be updated per se.

The reference to 3757daa was there because we added the ability to configure the location of the mongodb database by hand in #60, but the newly generated moveit pkg removes it again (as you probably used the latest released MSA). That would be a regression for this particular package, as it has been released (1.2.0) with that functionality in place.

The RViz config is minor.

@gavanderhoorn, Is this OK to merge based on my previous comment?

As the maintainer I'll leave it to you to decide whether the above issues should be fixed before merging.

Levi-Armstrong · 2017-01-10T15:46:19Z

@gavanderhoorn I have made the remaining requested changes:

from the way default_warehouse_db.launch is modified, I believe you used the latest released version of the MoveIt Setup Assistant, correct? That reverts 3757daa. If we want to be nice, we should regenerate the pkg using a source checkout of the MSA.

package manifest of abb_irb6640_moveit_config should be reverted to how it was (before update)

MoveIt config looses its acceleration limits (but I'm not too sure how important we think this is)

RViz config is overwritten

Levi-Armstrong · 2017-01-13T17:43:23Z

@gavanderhoorn I got the travis ci errors resolved. Can you please review when available?

Levi-Armstrong · 2017-01-17T05:34:15Z

@gavanderhoorn Friendly ping.

Jmeyer1292 · 2017-01-20T00:06:18Z

@Levi-Armstrong This PR needs to be rebased against the current version of the repository. Otherwise, though, +1 from me. This nomenclature issue has caused problems for me in the past. Would be nice to get this merged as we move toward kinetic.

Levi-Armstrong · 2017-01-20T17:12:08Z

@gavanderhoorn I believe all issues have been addressed and is ready to be merge.

gavanderhoorn · 2017-01-20T17:53:04Z

abb_irb6640_moveit_config/launch/demo.launch

-  <include file="$(find abb_irb6640_moveit_config)/launch/default_warehouse_db.launch" if="$(arg db)">
-    <arg name="moveit_warehouse_database_path" value="$(arg db_path)"/>
-  </include>
+  <include file="$(find abb_irb6640_moveit_config)/launch/default_warehouse_db.launch" if="$(arg db)"/>

 </launch>


This seems to revert this file back to one where the DB location isn't configurable. This will lead to problems when this package gets released, as the default installation location /opt/ros/$distro/.. is not world writable. See #60.

gavanderhoorn · 2017-01-20T17:57:18Z

Looks ok. MoveIt still plans. The mimic joint works (piston can do some strange things, but that is expected given the limitations it has). Unfortunately no 6640 around to test this on (yet).

gavanderhoorn · 2017-01-20T17:58:37Z

If you address the two comments about Daniel's email address and the demo.launch revert, this can be merged.

Could you also please insert an empty line between the first and second line of the commit message? That would make things slightly easier to read (on the command line fi).

- The abb_irb6600_support contained the irb6640 not the irb6600. A duplicate of the irb6640 will remain in the abb_irb6600_support package until Jade

Levi-Armstrong · 2017-01-20T21:13:11Z

@gavanderhoorn, I have made the requested changes.

Levi-Armstrong force-pushed the indigo-devel branch from b18ec2e to b2a4c88 Compare July 31, 2015 03:47

Levi-Armstrong mentioned this pull request Jul 31, 2015

Add ABB IRB6600 225kg 2.55m ros-industrial/abb_experimental#25

Closed

gavanderhoorn reviewed Jul 31, 2015
View reviewed changes

Levi-Armstrong force-pushed the indigo-devel branch 2 times, most recently from ca992d5 to 8e5ecff Compare July 31, 2015 15:09

Levi-Armstrong force-pushed the indigo-devel branch from bd1c650 to bd0ee58 Compare September 7, 2015 04:53

Levi-Armstrong force-pushed the indigo-devel branch from bd0ee58 to e3a2fed Compare September 7, 2015 05:13

gavanderhoorn reviewed Sep 7, 2015
View reviewed changes

abb_irb6600_support/urdf/irb6640.xacro

<xacro:abb_irb6640 prefix=""/>

</robot>

Copy link

Member

gavanderhoorn Sep 7, 2015

Choose a reason for hiding this comment

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

remove empty lines

Levi-Armstrong force-pushed the indigo-devel branch from 9baaf93 to 5e13088 Compare February 4, 2016 05:02

Levi-Armstrong force-pushed the indigo-devel branch from 38aa314 to 9c9c532 Compare January 10, 2017 15:44

Levi-Armstrong force-pushed the indigo-devel branch 2 times, most recently from aceb8de to c033510 Compare January 20, 2017 17:08

gavanderhoorn mentioned this pull request Jan 20, 2017

Everything was successful: your build exited with 1 - but Travis still says 'failed' ros-industrial/industrial_ci#111

Closed

gavanderhoorn reviewed Jan 20, 2017

View reviewed changes

Levi-Armstrong force-pushed the indigo-devel branch from c033510 to 43e31b4 Compare January 20, 2017 21:07

Create a copy of abb_irb6600_support and rename to abb_irb6640_support

4c160a3

- The abb_irb6600_support contained the irb6640 not the irb6600. A duplicate of the irb6640 will remain in the abb_irb6600_support package until Jade

Levi-Armstrong force-pushed the indigo-devel branch from 43e31b4 to 4c160a3 Compare January 20, 2017 21:11

gavanderhoorn approved these changes Jan 20, 2017

View reviewed changes

gavanderhoorn merged commit d734892 into ros-industrial:indigo-devel Jan 20, 2017

Levi-Armstrong mentioned this pull request Jan 21, 2017

moveit_cfg: 6640 ctrlr mngr launch file empty #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Levi-Armstrong commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

gavanderhoorn commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

gavanderhoorn commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

Levi-Armstrong commented Jul 31, 2015

gavanderhoorn commented Aug 2, 2015

gavanderhoorn commented Sep 5, 2015

gavanderhoorn commented Sep 5, 2015

gavanderhoorn commented Sep 5, 2015

Levi-Armstrong commented Sep 7, 2015

gavanderhoorn Sep 7, 2015

gavanderhoorn commented Sep 7, 2015

Levi-Armstrong commented Sep 27, 2015

gavanderhoorn commented Sep 28, 2015

Levi-Armstrong commented Oct 13, 2015

gavanderhoorn commented Oct 26, 2015

shaun-edwards commented Jan 11, 2016

Levi-Armstrong commented Jan 12, 2016

gavanderhoorn commented Jan 12, 2016

Levi-Armstrong commented Jan 12, 2016

Levi-Armstrong commented Feb 4, 2016

gavanderhoorn commented Feb 4, 2016

Levi-Armstrong commented Jan 10, 2017

Levi-Armstrong commented Jan 13, 2017

Levi-Armstrong commented Jan 17, 2017

Jmeyer1292 commented Jan 20, 2017

Levi-Armstrong commented Jan 20, 2017

gavanderhoorn Jan 20, 2017

gavanderhoorn commented Jan 20, 2017

gavanderhoorn commented Jan 20, 2017

Levi-Armstrong commented Jan 20, 2017

Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Conversation

Levi-Armstrong commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

Choose a reason for hiding this comment

gavanderhoorn commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

Choose a reason for hiding this comment

gavanderhoorn commented Jul 31, 2015

gavanderhoorn Jul 31, 2015

Choose a reason for hiding this comment

Levi-Armstrong commented Jul 31, 2015

gavanderhoorn commented Aug 2, 2015

gavanderhoorn commented Sep 5, 2015

gavanderhoorn commented Sep 5, 2015

gavanderhoorn commented Sep 5, 2015

Levi-Armstrong commented Sep 7, 2015

gavanderhoorn Sep 7, 2015

Choose a reason for hiding this comment

gavanderhoorn commented Sep 7, 2015

Levi-Armstrong commented Sep 27, 2015

gavanderhoorn commented Sep 28, 2015

Levi-Armstrong commented Oct 13, 2015

gavanderhoorn commented Oct 26, 2015

shaun-edwards commented Jan 11, 2016

Levi-Armstrong commented Jan 12, 2016

gavanderhoorn commented Jan 12, 2016

Levi-Armstrong commented Jan 12, 2016

Levi-Armstrong commented Feb 4, 2016

gavanderhoorn commented Feb 4, 2016

Levi-Armstrong commented Jan 10, 2017

Levi-Armstrong commented Jan 13, 2017

Levi-Armstrong commented Jan 17, 2017

Jmeyer1292 commented Jan 20, 2017

Levi-Armstrong commented Jan 20, 2017

gavanderhoorn Jan 20, 2017

Choose a reason for hiding this comment

gavanderhoorn commented Jan 20, 2017

gavanderhoorn commented Jan 20, 2017

Levi-Armstrong commented Jan 20, 2017