-
Notifications
You must be signed in to change notification settings - Fork 218
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
Kr210 kinematics update #124
base: melodic-devel
Are you sure you want to change the base?
Kr210 kinematics update #124
Conversation
This looks like a nice cleanup. Thanks for that @jwhendy. 👍 I'm still a bit foggy on why that offset is needed, but I think I'll just have to re-read your previous explanation(s) to figure that out (again). I think @BrettHemes was more involved here, so perhaps he could also take a look? |
@gavanderhoorn I think this is the simplest I've managed to try and explain it: Here's one more shot... honestly the time between looking at this has me needing to re-remember as well :) It's down to a balance of three factors:
Basically, you can pick any two of the above but you can't have all three. I was under the assumption that re-using the CAD was a definite to include, and all of these questions have been around which to give up among the second two.
Lastly, I admit to being hung up on trying something clever like your suggestion on #98. At this point, I think the PR is held up on me figuring that out in order to avoid a separate, mostly redundant I think I'll just create a separate Edit: also, the guidance from @BrettHemes steered me toward adding the offset and preserving the manual convention, so that's what I did here. The part of this PR that needs review was what folks thought of the probably stupid attempt shown here at adding a parameter to specify the L150 vs. L150-2 without a separate |
I just split the L150 and L150-2 into separate
If you liked the One uncertainty: are dashes forbidden in file names? I have |
Okay, after fiddling with the usage of an arg on the
I think that's pretty reasonable and a nice compromise (all we give up is one additional non-zero joint offset in the case of the |
As with the others, (#117 and #118) I like this approach. When I get some time I will load them up and check them unless @gavanderhoorn gets to it first. Hopefully this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Dashes in file names to underscores (i.e., "-2" to "_2")
- "-2" Loads/tests don't use new changes
- Move "variants" file code directly into parent macro.xacro (or rename to avoid confusion)
@@ -0,0 +1,3 @@ | |||
<launch> | |||
<param name="robot_description" command="$(find xacro)/xacro --inorder '$(find kuka_kr210_support)/urdf/kr210l150-2.xacro'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line no loner works with most recent changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is non-existent now.
@@ -0,0 +1,3 @@ | |||
<launch> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this file to load_kr210l150_2.launch
(i.e., no dashes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is non-existent now.
@@ -0,0 +1,8 @@ | |||
<launch> | |||
<include file="$(find kuka_kr210_support)/launch/load_kr210l150-2.launch" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above... update to use new variant arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is non-existent now.
@@ -0,0 +1,10 @@ | |||
<?xml version="1.0" ?> | |||
<launch> | |||
<group ns="load_kr210l150-2__"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underscores
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is non-existent now.
<include file="$(find kuka_kr210_support)/launch/load_kr210l150-2.launch"/> | ||
</group> | ||
|
||
<group ns="test_kr210l150-2__"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dash to underscore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is non-existent now.
@@ -0,0 +1,22 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this code into the kr210l150_macro.xacro and then delete this file. The variants
naming is confusing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Committed/pushed. I had a tricky time figuring out how to get at the arg
from the top level file. From looking around xacro:property
seemed like the ticket, so that's what I used. Hopefully that's correct? I admit to not knowing precisely why arg
vs. param
vs. property
...
<!-- for the KR210 L150, use ""; for the KR210 L150-2, use "_2" -->
<xacro:arg name="variant" default="" />
<xacro:property name="variant" value="$(arg variant)"/>
@@ -1,5 +1,5 @@ | |||
<?xml version="1.0" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a corresponding kr210l150_2.xacro
file as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking not anymore? I apologize as I flopped back and forth on trying to use an argument, then quasi giving up and just making replicate macro/xacro, launch, and test files, then going back to an argument. So I figured the path was either:
- no
_2
argument, and duplicate everything. The only difference would be the definition ofjoint_a3
per variant - use the
_2
argument, but don't replicate all the files with afoo_2.bar
shadow
</joint> | ||
<xacro:kuka_kr210l150_a3 prefix="${prefix}" variant="$(arg variant)"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, just put code here (vs separate file)?
<!--degrees to radians--> | ||
<property name="deg" value="0.017453293"/> | ||
<xacro:include filename="$(find kuka_resources)/urdf/common_materials.xacro"/> | ||
<xacro:include filename="$(find kuka_kr210_support)/urdf/kr210_variants_macro.xacro"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove? see below.
I'll look at the reviews more closely tonight; much appreciated. One main question... I see comments on files with I didn't plan to have a I can look into moving the code into the main |
The load/test files for the -2 are still there and as it stands these are broken. If you delete them you can ignore the respective comments. |
I missed that last comment before reading all the reviews. Yes, I intended to do completely away with the Thanks for the insight to just move top level; I agree for a simple |
Hi folks, Any further changes since May 2018? What's the best way for me to proceed? Thanks! |
Sorry, I'm not sure how to answer. I believe for this, #117 and #118 just need time from @gavanderhoorn . I have not heard anything on these PRs since May 2018, minus similar inquiries. |
This surely still needs work, but is a first stab at #98 . Creating the PR so we can talk code instead of theory and pictures! This addresses the kinematics of the original L150 and also attempts to bring in the L150-2, which is the same physical model with different limits/zero position defined for
joint_a3
.