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.

Sign up for GitHub

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

Jump to bottom

Add support for TX2-90 series #41

Merged
merged 16 commits into from
Jan 31, 2020
Merged

Add support for TX2-90 series #41

merged 16 commits into from
Jan 31, 2020

Conversation

simonschmeisser
Copy link
Contributor

This adds support for all three model in the TX2-90 family

gonzalocasas
gonzalocasas approved these changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

Besides the naming remark, this looks good to me. I tested it a bit creating a moveit_config package for it, and playing both directly on Rviz and also with our pipeline (which makes many more assumptions) and it seems to behave just fine.

image

staubli_tx2-90_support/CMakeLists.txt Outdated Show resolved Hide resolved
gonzalocasas
gonzalocasas suggested changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

Even thou I agree with the PR, I guess the correct thing is to mark as "Request changes" due to the naming change request. ;)

gavanderhoorn
gavanderhoorn requested changes Nov 26, 2019
View reviewed changes
Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I've added some in-line comments.

staubli_tx2-90_support/urdf/tx2-90_macro.xacro Outdated Show resolved Hide resolved
staubli_tx2-90_support/urdf/tx2-90.xacro Outdated Show resolved Hide resolved
staubli_tx2-90_support/urdf/tx2-90_macro.xacro Outdated Show resolved Hide resolved
staubli_tx2-90_support/urdf/tx2-90_macro.xacro Outdated Show resolved Hide resolved
staubli_tx2-90_support/urdf/tx2-90_macro.xacro Outdated Show resolved Hide resolved
staubli_tx2-90_support/package.xml Outdated Show resolved Hide resolved
staubli_tx2-90_support/package.xml Outdated Show resolved Hide resolved
staubli_tx2-90_support/tests/roslaunch_test.xml Outdated Show resolved Hide resolved
staubli_tx2-90_support/config/joint_names_tx2-90l.yaml Outdated Show resolved Hide resolved
staubli_tx2-90_support/launch/load_tx2-90l.launch Outdated Show resolved Hide resolved
@gavanderhoorn
Copy link
Member

@simonschmeisser: I've only looked at the _macro.xacro for the TX2-90, but the same comments apply to the files for the other variants.

gavanderhoorn
gavanderhoorn requested changes Nov 29, 2019
View reviewed changes
Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Very nice. Thanks for the update.

Some minor comments.

staubli_tx2_90_support/package.xml Outdated Show resolved Hide resolved
staubli_tx2_90_support/package.xml Outdated Show resolved Hide resolved
gavanderhoorn
gavanderhoorn reviewed Jan 17, 2020
View reviewed changes
Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Just a few questions/comments.

Thanks again for the PR and for iterating.

staubli_tx2_90_support/package.xml Outdated Show resolved Hide resolved
staubli_tx2_90_support/package.xml Outdated Show resolved Hide resolved
staubli_tx2_90_support/urdf/tx2_90_macro.xacro Outdated Show resolved Hide resolved
staubli_tx2_90_support/urdf/tx2_90_macro.xacro Show resolved Hide resolved
@gavanderhoorn
Copy link
Member

Just noticed that link_2.stl and link_4.stl for the XL variant are 4MB+ each.

I've decimated them in Blender @simonschmeisser, but I don't appear to be able to push the commits to the tx2-90 branch.

@gavanderhoorn
Copy link
Member

Opened isys-vision#1.

@simonschmeisser
Merge pull request #1 from gavanderhoorn/pr41_decimate_links
137c836 
Decimate XL meshes for link 2 and 4.
@gavanderhoorn gavanderhoorn mentioned this pull request Jan 24, 2020
Merged
@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jan 31, 2020 •
edited
Loading

I've submitted some final changes for this PR @simonschmeisser. See isys-vision#2.

Again thanks for iterating 👍 I believe this is in a good shape now.

gavanderhoorn
gavanderhoorn approved these changes Jan 31, 2020
View reviewed changes
Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Thanks again.

@gavanderhoorn gavanderhoorn merged commit bd8c3f1 into ros-industrial:kinetic-devel Jan 31, 2020
@simonschmeisser
Copy link
Contributor Author

Thanks for reviewing!

@simonschmeisser simonschmeisser deleted the tx2-90 branch April 30, 2021 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonzalocasas gonzalocasas gonzalocasas requested changes

@gavanderhoorn gavanderhoorn gavanderhoorn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonschmeisser @gavanderhoorn @gonzalocasas