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

NASA Challenge_[franklinselva]_[Restructure the contents of demos to be modular and scalable] #31

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

franklinselva
Copy link

@franklinselva franklinselva commented Aug 14, 2024

Fixes space-ros/space-ros#178

Merge queue no. 1

This PR restructures the contents of the demo (from space-ros/simulations, space-ros/docker, space-ros/demos) under a single tree. The main changes include

  • Add canadarm_description package for simulation assets of canadarm2
  • Add curiosity_description package for simulation assets of curiosity mars rover
  • Add gazebo packages for canadarm and curiosity rover demos
  • Add docker based entrypoints for canadarm and curiosity rover demos
  • Add shell scripts for easier building and running of candarm and curiosity demos

This PR also restructure the workflow of the past demo in such a way the GUI apps are ran outside the docker. The reason for this change is to adapt different simulation tools and have a reproducible demo across the simulation tools.

@franklinselva franklinselva marked this pull request as ready for review August 15, 2024 07:02
@franklinselva
Copy link
Author

Note for reviewers: This is a large refactoring related to the referenced issue. I think it would be easier to review the work from clear slate. If there is any information needed, please let me know.

@franklinselva franklinselva changed the title Update demos to be modular and scalable NASA Challenge_[franklinselva]_[Restructure the contents of demos to be modular and scalable] Aug 15, 2024
@mkhansenbot
Copy link
Contributor

Overall I like the changes, but I do have some concern about using a Makefile to build the Docker images. We're not doing that elsewhere. We've already got both Docker and Earthly in Space ROS, so adding a 3rd build method seems unnecessary. Would you consider moving to either a docker-compose or Earthly build instead?

@mkhansenbot
Copy link
Contributor

Also, I'm planning to add a GH action build flow, see issue #35. If I get that in, I'm going to ask you to add build.sh files to wrap all the builds generically too.

@franklinselva
Copy link
Author

franklinselva commented Aug 28, 2024

Hi @mkhansenbot, Thanks for your review. I understand the use of Makefile as a runtime dependency is odd that is not in favor. I will make a direct replacement of the makefile with the shell script to build, clean and run the demos.

Would you consider moving to either a docker-compose or Earthly build instead?

I would not go for Earthly builds for demo since this will also be an addtional runtime dependency for working with the demos. At the moment, Earthly builds are only used as internal tool within collaborators and the scope of the build tool doesn't need to be expanded beyond this point. As mentioned here in the issue, using docker-compose for show casing different demos supported by shell scripts would be an good choice in my opinion. This will ensure, we still have the required entrypoints to the demos.

EDIT: I still see the space-ros docker images are expected to be pulled from registry and should not build when running the demos.

@franklinselva
Copy link
Author

franklinselva commented Aug 28, 2024

I have replaced the Makefiles with the build.sh.
There are still few changes expected in the shell script based on #35. I will need to hold the rebasing on queued PRs until this PR is resolved.

@EzraBrooks
Copy link
Member

Thanks for consolidating the simulation assets into one repository! This is a much more straightforward package layout.

franklinselva added a commit to franklinselva/spaceros-simulation that referenced this pull request Aug 31, 2024
Related to space-ros/space-ros#178
Related to space-ros#18

 - Moved the simulation assets to space-ros/demos#31
 - Repurpose the repository for the simulation assets of Nvidia's
Isaac Sim
franklinselva added a commit to franklinselva/spaceros-simulation that referenced this pull request Sep 9, 2024
Related to space-ros/space-ros#178
Related to space-ros#18

 - Moved the simulation assets to space-ros/demos#31
 - Repurpose the repository for the simulation assets of Nvidia's
Isaac Sim
resolves #178

- Group demos to its robot content
resolves space-ros/space-ros#178

 - Add canadarm_description
 - Fix model asset paths
 - Reverse control configuration for canadarm model
resolves space-ros/space-ros#178
 - Add curiosity_description
 - Strip third party dependencies of mars_rover
resolves space-ros/space-ros#178

 - Fix control parameters for curiosity_description
 - Move curiosity_path world to curiosity_gazebo
 - Add launch file for spinning of gazebo world
related to space-ros/space-ros#178
 - Cleaned up rover launch file for the demo
 - Updated comments for demo node
related to space-ros/space-ros#178

 - Add dockerfile for demo
 - Add makefile for build, run and clean automation
related to space-ros/space-ros#178

 - Add README.md for Curiosity Rover demo
 - Update Main README.md of the demos repository
resolves space-ros/space-ros#178

 - Add canadarm_description package
 - Move earth and iss  world models to canadarm_gazebo
related to space-ros/space-ros#178
 - Cleaned up canadarm launch file for the demo
 - Updated comments for demo node
 - Strip Moveit Redundant Node
 - Cleanup packages.xml
 - Remove ros control parameters for gazebo
related to space-ros/space-ros#178

 - Add README.md for Canadarm2 demo
 - Update Main README.md of the demos repository
Related to space-ros/space-ros#178

 - Update package.xml for canadarm2 packages
 - Update package.xml for curiosity_rover packages
Related to space-ros/space-ros#178

 - Remove makefiles for canadarm2 and curiosity rover demo
 - Add build.sh for building canadarm2 and curiosity rover demo
 - Add run.sh for running the demos
 - Update README.md of demos to the new process
@franklinselva
Copy link
Author

franklinselva commented Sep 10, 2024

@mkhansenbot Can you approve the changes for the CI run?

@franklinselva
Copy link
Author

Is it possible to include docker-compose for the CI run?

@mkhansenbot
Copy link
Contributor

@franklinselva - according to GitHub, Docker Compose v1 is no longer supported, can you change your script to use v2? https://github.blog/changelog/2024-04-10-github-hosted-runner-images-deprecation-notice-docker-compose-v1/

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.

Move demo/docker/simulation contents of demo packages within demo repository
3 participants