Skip to content

Refactor structure of launch files reduce duplication when adding new simulations#28

Closed
glpuga wants to merge 13 commits intomainfrom
glpuga/mujoco_exp
Closed

Refactor structure of launch files reduce duplication when adding new simulations#28
glpuga wants to merge 13 commits intomainfrom
glpuga/mujoco_exp

Conversation

@glpuga
Copy link
Copy Markdown
Collaborator

@glpuga glpuga commented Mar 3, 2025

  • Refactor structure of launch files reduce duplication when adding new simulations
  • Improve dockerfile to add ccache, avoid redundant image rebuilds and add build file persistance.

The gazebo simulation is now launched with:

ros2 launch ar4_gazebo_bringup main.launch.py
graph TD
      A[**ar4_moveit_config** <br> main.launch.py]
      B[**ar4_common** <br> main.launch.py]
      C[**ar4_description** <br> main.launch.py]
      D[**ar4_gazebo_sim** <br> main.launch.py]
      E[**ar4_gazebo_bringup** <br> main.launch.py]
      F[**ar4_hardware_interface** <br> main.launch.py]
      G[**ar4_realbot_bringup** <br> main.launch.py]

      A --> F
      C --> F

      H[**ar4_isaac_bringup** <br> main.launch.py <br> ...not yet available...]
      I[**...** <br> main.launch.py]

      B --> H
      I --> H

      J[**ar4_mujoco_bringup** <br> main.launch.py <br> ...not yet available...]
      K[**...** <br> main.launch.py]

      B --> J
      K --> J


      A --> B

      C --> D
      C --> A

      B --> E
      D --> E

      B --> G
      F --> G
Loading

glpuga added 8 commits March 3, 2025 16:09
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
…persistance

Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
Signed-off-by: Gerardo Puga <glpuga@gmail.com>
@glpuga glpuga force-pushed the glpuga/mujoco_exp branch from 0745a16 to bc5448c Compare March 3, 2025 19:09
@glpuga glpuga requested a review from stevendes March 3, 2025 19:12
Copy link
Copy Markdown
Member

@stevendes stevendes left a comment

Choose a reason for hiding this comment

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

@glpuga I left some early comments, this change will make the repo incompatible with the isaac_sim changes, just to keep that in mind, that branch has AR4 + MoveIt already added.

Is a test with the real robot necessary with these changes too?

Comment thread ar4_common/README.md Outdated
Comment thread ar4_common/package.xml Outdated
Comment thread ar4_gazebo_sim/LICENSE
@@ -0,0 +1,29 @@
BSD 3-Clause License
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What advantage do we have in having ar4_gazebo_sim and ar4_gazebo_bringup as separate packages instead of just one ar4_gazebo?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Arguably, it would be totally possible have just one, so you can say this is mostly personal preference.

However, out of experience I find that keeping them separate allows to have a launch file that will only launch the simulation (or hardware) parts without the common parts, which comes handy when debugging the hardware/simulation layer.

Bringup packages end up being only a front-end for the packages needed for any particular configuration, and in the case of simulation, it's the single fail-safe place to set the use_sim_time param for the whole set without risking omissions.

Regarding naming, packages starting a robot (or a simulation of one) must end in "bringup" to give a hint to the user of their function.

Comment thread ar4_realbot_bringup/launch/main.launch.py Outdated
Comment thread ar4_realbot_bringup/launch/main.launch.py Outdated
@glpuga
Copy link
Copy Markdown
Collaborator Author

glpuga commented Mar 4, 2025

@glpuga I left some early comments, this change will make the repo incompatible with the isaac_sim changes, just to keep that in mind, that branch has AR4 + MoveIt already added.

Indeed, I know. And potentially a Mujoco simulation coming (either by someone, or a test one I might try later today), but I think there's advantage both in reuse and clarity in the structure proposed here, and it'll get harder to refactor later.

Regarding the isaac_sim branch, looking at the changes, there should be no conflict with this branch in the robot description because I only added a single file in there, and changed no other. The ar4_isaac package does not depend on anything I changed, so it can be merged with no conflict. We might have small conflicts on the README file and in the docker-compose file, though.

We can keep this PR on hold until we merge the isaac_sim branch, then I'll rebase this, fix conflicts and refactor the isaac_sim package in the same structure as this in this same PR.

We can then test also the hardware launch files before finally merging.

Thanks for the review.

@glpuga
Copy link
Copy Markdown
Collaborator Author

glpuga commented Mar 18, 2025

Closing as this will be replaced with #29

@glpuga glpuga closed this Mar 18, 2025
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.

2 participants