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

Add Demos for SDF #427

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Add Demos for SDF #427

merged 10 commits into from
Dec 4, 2024

Conversation

Amronos
Copy link
Contributor

@Amronos Amronos commented Oct 17, 2024

Adds demos for SDF. Linked to #ros2_control/1763.
These demos should help in testing the PR.


A couple of things I want an opinion on:

  1. Should using URDF or SDF be an argument of the launch file?
  2. What should be the name of the launch file?
  3. I have put the SDF files in a new sdf directory is that okay or should something else be done?

To test the diff_drive demo for SDF:
ros2 launch gz_ros2_control_demos diff_drive_example_sdf.launch.py
ros2 run gz_ros2_control_demos example_diff_drive
The simulation should show a behaviour similar to the diff_drive demo for URDF.

Signed-off-by: Aarav Gupta <[email protected]>
@Amronos Amronos requested a review from ahcorde as a code owner October 17, 2024 16:15
@Amronos Amronos marked this pull request as draft October 18, 2024 10:42
@Amronos
Copy link
Contributor Author

Amronos commented Nov 8, 2024

@ahcorde Can I get an initial review?

@@ -0,0 +1,107 @@
# Copyright 2022 Open Source Robotics Foundation, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2022 Open Source Robotics Foundation, Inc.
# Copyright 2024 Open Source Robotics Foundation, Inc.

Comment on lines 31 to 40
robot_description_content = Command(
[
PathJoinSubstitution([FindExecutable(name='xacro')]),
' ',
PathJoinSubstitution(
[FindPackageShare('gz_ros2_control_demos'),
'sdf', 'test_diff_drive.xacro.sdf']
),
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need to do that, the sdf file doesn't have any xacro tag. You should read the file as it's

Copy link
Collaborator

Choose a reason for hiding this comment

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

you should be able to use something like from launch.substitutions import FileContent

@Amronos
Copy link
Contributor Author

Amronos commented Nov 30, 2024

@ahcorde I decided it would be better for the description format to be an argument of the launch file. I did need to use an OpaqueFunction for this (please tell me if there is a simple way), have a look at ed7f047.
Another option would be not using SDF at all in the launch files and instead just having them be present in the sdf directory.

@Amronos Amronos requested a review from ahcorde November 30, 2024 09:23
@bmagyar
Copy link
Member

bmagyar commented Nov 30, 2024

If the purpose of the demo is to have a urdf and sdf description separately demonstrated but in the same use case, I think the separate argument makes sense.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you mind to add some docs to the README.md ?

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There are conflicts, do you mind to fix it ?

@Amronos Amronos requested a review from ahcorde December 2, 2024 11:39
@Amronos
Copy link
Contributor Author

Amronos commented Dec 2, 2024

If everything looks good now, I will also add SDF versions for the other demos.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

While I was compiling the documentation of this PR I noticed that the setup is wrong. This PR should configure the docs file properly.

also If you run the documentation command there are some errors

doc/index.rst Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@Amronos Amronos requested a review from ahcorde December 2, 2024 13:16
doc/index.rst Outdated Show resolved Hide resolved
@ahcorde ahcorde marked this pull request as ready for review December 2, 2024 14:07
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There are some issues:

gz_ros2_control_demos/launch/ackermann_drive_example.launch.py:39:21: E128 continuation line under-indented for visual indent
gz_ros2_control_demos/launch/ackermann_drive_example.launch.py:40:21: E128 continuation line under-indented for visual indent
gz_ros2_control_demos/launch/ackermann_drive_example.launch.py:91:9: E222 multiple spaces after operator
gz_ros2_control_demos/launch/diff_drive_example.launch.py:39:21: E128 continuation line under-indented for visual indent
gz_ros2_control_demos/launch/diff_drive_example.launch.py:40:21: E128 continuation line under-indented for visual indent

Signed-off-by: Aarav Gupta <[email protected]>
@Amronos Amronos requested a review from ahcorde December 3, 2024 11:18
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

is the robot moving ? I can't move the diff_drive or ackermann examples

@Amronos
Copy link
Contributor Author

Amronos commented Dec 3, 2024

is the robot moving ? I can't move the diff_drive or ackermann examples

Yes, the robot does move for me upon running ros2 run gz_ros2_control_demos example_diff_drive for diff_drive or ros2 run gz_ros2_control_demos example_ackermann_drive for ackermann.
Make sure you have the sdformat_urdf package installed and are using a version of hardware_interface that has this commit.

@Amronos Amronos requested a review from ahcorde December 3, 2024 13:21
@ahcorde
Copy link
Collaborator

ahcorde commented Dec 4, 2024

The issue that I mentioned is unrelated to this PR, we can merge this.

@ahcorde ahcorde merged commit d2b2f0d into ros-controls:rolling Dec 4, 2024
1 check passed
@Amronos Amronos deleted the sdf-demos branch December 4, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants