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

new stls/frames to allow for only one offset per joint #118

Open
wants to merge 9 commits into
base: melodic-devel
Choose a base branch
from

Conversation

jwhendy
Copy link

@jwhendy jwhendy commented Mar 14, 2018

I utilized the lbr_iiwa_14_r820 xacro file when doing PR #117 and noticed it had a couple instances of multiple joint offsets, which was what I understood required attention in #31 so I'm attempting to fix here as well.

@jwhendy jwhendy mentioned this pull request Mar 14, 2018
4 tasks
@gavanderhoorn
Copy link
Member

@jwhendy: just wanted to let you know that we're not ignoring you. Reviewing this should be done carefully, and so takes a bit of time.

I will get to it if @BrettHemes doesn't do so sooner.

Copy link
Member

@BrettHemes BrettHemes left a comment

Choose a reason for hiding this comment

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

Nice! This is much cleaner than the existing definition.

From my inspection:

  • Meshes looks good / no holes or inverted normals
  • Joints limits and handedness look good
  • Offsets match documentation (Spez_LBR_iiwa_de.pdf)
  • URDF is optimal and nicely human-readable
  • Nice use of radians() 👍
  • Meshes are a bit large

Looks good to me except I question your choice of red however 😉
Maybe check out material defines in kuka_experimental/kuka_resources/urdf/common_materials.xacro and another robot that uses these in visual/collision elements

👍 from me if you fix the colors and decimate the meshes a bit

[edit]: mesh sizes

@gavanderhoorn
Copy link
Member

I know we're not always the fastest to respond @jwhendy, so I/we can't (and won't) complain (:)) but what is the status here?

@jwhendy
Copy link
Author

jwhendy commented May 4, 2018

Ha. I'm not the fastest either as it would happen! I think we're here:

  • I need to decimate a bit (easy)
  • update colors (easy)
  • per the email from Kuka on robot model for lbr_iiwa_7_r800 #117 and your comment, maybe split the end effector (flange) into a separate .xacro and include? That's slightly harder (mostly due to my inexperience), but feels like the right approach. I'll grab the pneumatic touch flange (since it's on the iiwa we have at work) and update this PR with that and the base flange.

I think if I do that on #117 then both of these will be ready to rock.

Does that sound good?

@jwhendy
Copy link
Author

jwhendy commented May 19, 2018

I think we're good! Colors are updated and the meshes have been decimated. Flanges now live as a file flange_foo.stl and are loaded as link_7 by a new xacro, lbr_iiwa_flanges_macro.xacro, which defaults to the basic flange. Users can also pass flange_type:=flange_pneumatic_touch to load that one.

I would have loaded more but 1) SolidWorks kept crashing trying to import and 2) I only have the pneumatic touch flange and am not sufficiently motivated... there's a lot for this robot!

I think the framework is sound, so anyone can contribute a flange as they need/see fit.

@BrettHemes
Copy link
Member

I personally like this approach. If we decide on this, we should make sure to document it somewhere so users can figure it out. I saw the comment in the xacro but somewhere more visible would help.

@BrettHemes
Copy link
Member

I just checked this out and I can't find anything wrong with it. I will, however, wait for @gavanderhoorn to give it a once over as he is much better at finding issues than I am 😉

Also, I propose that the (shared) flange stl's get moved to a general folder one higher for sharing between the 14 and 7 variants but this can happen with #117 when it gets pulled.

@jwhendy
Copy link
Author

jwhendy commented May 19, 2018

@BrettHemes I like the idea of shared flanges as well, assuming they are identical. And yes, I think that would be easier once they both exist in indigo-devel at the same time. I'd be happy to handle that.

@gavanderhoorn
Copy link
Member

Guys, I'm not ignoring this, just have a lot to do.

If @BrettHemes is ok then I'm inclined to believe him.

I just checked this out and I can't find anything wrong with it. I will, however, wait for @gavanderhoorn to give it a once over as he is much better at finding issues than I am 😉

Well, it's true that is one of my talents, but it will take a bit of time before I can take a look (probably not before the 11th).

I'm travelling this Sunday though, so that may give me /some/ time while waiting at the airport.

@simonschmeisser simonschmeisser changed the base branch from indigo-devel to melodic-devel September 23, 2021 15:05
@simonschmeisser simonschmeisser mentioned this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants