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

Support for universal and ball joint in sdf #13

Merged
merged 10 commits into from
Jun 20, 2022
Merged

Support for universal and ball joint in sdf #13

merged 10 commits into from
Jun 20, 2022

Conversation

quarkytale
Copy link
Contributor

Mapped universal and ball joint in sdf to be floating joints in urdf.

Questions:

  1. Should the base be galactic or ros2?
  2. Initially I added comments beside the change that // Will need transforms published to function correctly, but that's true for all joints in that list. Not sure if I should indicate that here since there'll be an example world (WIP) to demo the usage.

@quarkytale
universal/ball -> floating joints
c25c7c6 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale quarkytale requested a review from sloretz June 8, 2022 00:30
@quarkytale quarkytale mentioned this pull request Jun 8, 2022
Merged
@sloretz
Copy link
Contributor

sloretz commented Jun 8, 2022

Should the base be galactic or ros2?

Please target the ros2 branch. ROS projects always do development on the latest branch, and then backport as needed.

@quarkytale quarkytale changed the base branch from galactic to ros2 June 13, 2022 20:11
@chapulina chapulina mentioned this pull request Jun 13, 2022
Merged
@quarkytale
suggested changes
5050350 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale quarkytale requested a review from sloretz June 13, 2022 20:14
sloretz
sloretz requested changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

The logic looks good to me. Mind updating the tests so CI passes with the new behavior?

@quarkytale
added joint tests
6140c00 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
fixed tests
549a963 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
Copy link
Contributor Author

Tests should be passing now! The gearbox joint has a kinematic loop, which is expected. (I think a custom TF publisher will handle that or should I just make it invalid?)

@quarkytale
cleaning
6c17e8a 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
chapulina
chapulina approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just have a couple of minor suggestions, but this looks good to me otherwise. I'd wait for an approval from @sloretz before merging.

After it's merged, we'll need to open a backport to galactic.

sdformat_urdf/src/sdformat_urdf.cpp Show resolved Hide resolved
sdformat_urdf/test/joint_tests.cpp
ASSERT_EQ(nullptr, joint->limits);
ASSERT_EQ(nullptr, joint->safety);
ASSERT_EQ(nullptr, joint->calibration);
ASSERT_EQ(nullptr, joint->mimic);
Copy link
Contributor

@chapulina chapulina Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think lines 40~44 can be EXPECT_EQ, because we're not trying to access those pointers in the test anyway. This way, if in the future one of them fails, we will know whether the others fail too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, though this could already be improved about the existing tests in this file so I wouldn't block on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can work on this in a separate PR :)

Copy link
Contributor Author

@quarkytale quarkytale Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Opened #16 for improving tests.

@chapulina chapulina requested a review from sloretz June 17, 2022 18:18
sloretz
sloretz approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM. @chapulina has good points, but since they're existing issues I won't block on it.

I don't think we need a run of ci.ros2.org since this is outside of the usual repos file and doesn't have a quality level declaration. I also have no idea what would happen if we tried to build this on Windows.

sdformat_urdf/src/sdformat_urdf.cpp Show resolved Hide resolved
sdformat_urdf/test/joint_tests.cpp
ASSERT_EQ(nullptr, joint->limits);
ASSERT_EQ(nullptr, joint->safety);
ASSERT_EQ(nullptr, joint->calibration);
ASSERT_EQ(nullptr, joint->mimic);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, though this could already be improved about the existing tests in this file so I wouldn't block on it.

quarkytale and others added 4 commits June 17, 2022 14:52
@quarkytale
check for joint axis and added test
a2b3dc3 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
newline
8f1f206 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
updating CMakeLists
5b2fdf7 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale
missing space
b69f03c 
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale quarkytale merged commit 8b7909b into ros2 Jun 20, 2022
@quarkytale quarkytale deleted the quarkytale/joint_support branch June 20, 2022 19:51
@quarkytale
Copy link
Contributor Author

@Mergifyio backport galactic

@mergify mergify bot mentioned this pull request Jun 20, 2022
Merged
@mergify
Copy link

mergify bot commented Jun 20, 2022

backport galactic

✅ Backports have been created

quarkytale added a commit that referenced this pull request Jun 20, 2022
@quarkytale
Merge pull request #14 from ros/mergify/bp/galactic/pr-13
f8868a8 
Support for universal and ball joint in sdf (backport #13)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@quarkytale @sloretz @chapulina