search

ros-industrial / abb

ROS-Industrial ABB support (http://wiki.ros.org/abb)
146 stars 154 forks source link

Create a copy of abb_irb6600_support and rename to abb_irb6640_support #89

Closed Levi-Armstrong closed 7 years ago

Levi-Armstrong commented 9 years ago
gavanderhoorn commented 9 years ago

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 commented 9 years ago

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.

Levi-Armstrong commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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

Levi-Armstrong commented 9 years ago

@gavanderhoorn: I believe I have fixed everything mentioned above. I also found out why the color was not as nice, it turns out you must export the Collada file from Blender as shadeless. I think it was importing the file with shading and then the rviz rendering was applying its own shading on top causing it to appear much darker.

Levi-Armstrong commented 9 years ago

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

gavanderhoorn commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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:

Other than those things this looks ok now.

shaun-edwards commented 8 years ago

Can this be merged?

Levi-Armstrong commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

gavanderhoorn commented 8 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

Levi-Armstrong commented 7 years ago

@gavanderhoorn Friendly ping.

Jmeyer1292 commented 7 years ago

@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 commented 7 years ago

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

gavanderhoorn commented 7 years ago

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 commented 7 years ago

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

Levi-Armstrong commented 7 years ago

@gavanderhoorn, I have made the requested changes.