Add space nav2 #80

mkhansenbot · 2023-08-25T22:03:29Z

This adds a Dockerfile to build navigation2 on top of Space ROS. It also adds corresponding build.sh and run.sh to build and run the container. Finally, it adds a build step to the workflow to build the osrf/space_nav2 image

Initial files were provided on a branch by @mjeronimo

mkhansenbot · 2023-08-26T00:40:21Z

This is the first PR for: space-ros/space-ros#67

alexbuyval · 2023-08-27T14:13:35Z

@mkhansenbot I have just tested it locally. I have successfully build the docker image and run it.
However I noticed that 'src' folder was repeated twice in nav2 workspace path.
/home/spaceros-user/nav2_ws/src/src
I think if it was not done by intention you need to remove 'src' in line 46-47 of Dockerfile

RUN mkdir -p ${NAVIGATION2_WS}
WORKDIR ${NAVIGATION2_WS}

Anyway, is there any demos inside which should I test?

mkhansenbot · 2023-08-27T15:58:38Z

Good catch, I found the issue and will push a fix today. I'll also add a README with instructions.

I can currently run the mars_rover demo, along with Nav2 and SLAM, however, SLAM doesn't work yet because I need to add a lidar and encoders to the model to have it publish the /scan and odom topics. I'll submit a separate PR for that to the space-ros/simulation repo

alexbuyval · 2023-08-29T18:45:08Z

Hi @mkhansenbot
I have just rebuilt the docker and tested it. Everything looks good. Also I have successfully run all launch files you described in the readme. It looks that all nodes started correctly, but since we don't have a correct odometry there are warnings like this:
Timed out waiting for transform from base_link to odom to become available, tf error: Invalid frame ID "odom" passed to canTransform argument target_frame - frame does not exist
As I understand it is ok, right?

quarkytale · 2023-12-14T18:59:34Z

I think the odometry can be solved by adding a OdometryPublisher plugin, see ros_gz_sim_demos/worlds/vehicle.sdf. This tutorial might help as well https://gazebosim.org/docs/fortress/ros2_interop

mkhansenbot · 2024-01-21T18:04:11Z

This is now ready for review. I moved the demo specific files into a separate folder "nav2_demo" and moved the instructions for running the demo into the README in that folder.

Bckempa

Haven't built and tested yet but here are some preliminary comments

Bckempa · 2024-01-22T15:16:47Z

nav2_demo/Dockerfile

+LABEL org.label-schema.vendor="Open Robotics"
+LABEL org.label-schema.version=${VERSION}
+LABEL org.label-schema.url="https://github.com/space-ros"
+LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker-images"


Suggested change

LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker-images"

LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker"

Bckempa · 2024-01-22T15:22:33Z

nav2_demo/Dockerfile

+COPY mars_map* /home/spaceros-user/nav2_ws
+
+# Install nav2 config file for mars rover demo
+COPY nav2_params.yaml /home/spaceros-user/nav2_ws


I think we should be able to use the NAVIGATION2_WS variable (maybe as a build arg instead of env var) but I know variable expansion in COPY expressions has been difficult in the past so I'm going to test this myself before adding a suggested patch.

Bckempa · 2024-01-22T15:24:48Z

nav2_demo/README.md

+# Run the Mars Rover demo with Nav2 and SLAM toolbox
+
+## Terminal 1 - launch the mars_rover demo 
+Follow instructions on space-ros/docker/space-robots/README.md


Suggested change

Follow instructions on space-ros/docker/space-robots/README.md

Follow instructions in the [space-robots `README.md`](../space-robots/README.md)

Bckempa · 2024-01-22T15:29:50Z

nav2_demo/README.md

+Exec into the same space_nav2 container and launch Rviz2
+```
+docker exec -it -e DISPLAY=:0 osrf_space_nav2_demo bash
+source /opt/ros/humble/setup.bash


Are we installing Space ROS and Humble in the nav container?

Bckempa · 2024-01-22T15:34:03Z

navigation2/README.md

+To build the docker image, run:
+
+```
+$ ./build.sh


This isn't consistent with how shell scripts are listed in the demo container README, we should make it consistent.

Not sure what you mean by this, is it the leading $ that is different?

Bckempa · 2024-01-22T15:34:57Z

navigation2/README.md

+
+The new image is named **osrf/space_nav2:latest**.
+
+There is a run.sh script provided for convenience that will run the spaceros image in a container.


Do we need to mention that rocker is required here?

I can add it but I don't see it mentioned on the space_robots README, so we should be consistent. I added issue #128 to fix this.

Bckempa · 2024-01-22T15:37:46Z

navigation2/navigation2.repos

Since this is a demo, should we pin these to commits so they don't move out from under us?

Since this same issue exists for the MoveIt2 demo, I created an issue so we can resolve it for both. #127

Bckempa · 2024-01-22T15:38:58Z

nav2_demo/README.md

+## Terminal 2 - launch Nav2
+Start the space_nav2 container and launch the navigation2 nodes
+```
+run.sh


Do we need to note that this requires rocker?

Filed issue #128

Bckempa · 2024-01-22T15:39:21Z

navigation2/Dockerfile

+LABEL org.label-schema.vendor="Open Robotics"
+LABEL org.label-schema.version=${VERSION}
+LABEL org.label-schema.url="https://github.com/space-ros"
+LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker-images"


Suggested change

LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker-images"

LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker"

Bckempa · 2024-01-22T15:42:47Z

navigation2/Dockerfile

@xfiderek should we copy the exclude packages text files pattern here now or hold off incase we convert to Earthfile in the next release?

If you are asking in context of rosdep then I would double check if we need rosdep commands at all in here. I think that unless Nav2 requires some non-ROS dependencies then it should not be required ¹. @mkhansenbot have you by any chance tried to build this image without rosdep commands? If not maybe its worth trying / we can try together. Btw I see that list of skip keys here is a bit different than in spaceros base image. Is there a specific reason to that?

If it turns out that rosdep is needed I would already make use of text file, however I see that there is a divergence between:

"excluded-pkgs.txt"

skip keys in spaceros image

skip keys in this image

so unless we can address the divergence soon, we need to postpone it.

On the side note (not related to this PR), your question @Bckempa made me wondering and I think that we should have a better way to track what exactly is installed by rosdep and we should be saving such thing as an artifact in base image. What do you think? If yes, we can create item and include it after this release.

Footnotes

As far as I tested, rosdep in base image does not install any ROS-related dependencies. ↩

mkhansenbot · 2024-01-29T16:45:34Z

Please let me know what is needed to get this approved. I think I need to add an issue number to each commit and squash some commits to clean up the history, is that correct?

mkhansenbot · 2024-01-29T19:26:18Z

@Bckempa - I can make the small changes you suggested above when I add the issue number to the commits. I'll do this tonight

mkhansenbot · 2024-01-30T03:00:53Z

I squashed and added the issue to the commit messages. I also fixed the two issues that @Bckempa found in the README files

I think this is ready for review

Signed-off-by: Michael Jeronimo <[email protected]>

ivanperez-keera · 2024-01-30T08:28:14Z

@mkhansenbot I think there's a level of detailed being retained here that is unnecessary. I sent you an email with two proposals (one more aggressive, one middle ground). Please check and confirm. Thanks!

EDIT: Just documentation, my thought process is the following:

I think the way to explain this PR is in two or three commits, keeping the detailed history is not necessary.

Proposal one:

Alternative middle ground:

In the second proposal, other commits not listed there are just squashed with related commits (e.g., fixes to the README are squashed with the commit that introduces the README; it makes no sense to introduce it without fixes and fix it later).

mkhansenbot · 2024-01-30T15:33:07Z

I'm fine with your second proposal

ivanperez-keera · 2024-01-29T01:39:56Z

navigation2/README.md

@@ -0,0 +1,47 @@
+# Navigation2 Docker Image
+
+The Navigation2 Docker image uses the Space ROS docker image (*openrobotics/spaceros:latest*) as its base image. The Navigation2 Dockerfile installs all of the prerequisite system dependencies to build Navigation2


Should this refer to osrf/spaceros:latest instead?

Actually, I made a mistake in my fix below 664665f. It should be space-ros with a hyphen. My commit message spells it right but my actual commit spells it wrong.

ivanperez-keera · 2024-01-30T17:29:17Z

I'm waiting for this to build before I merge.

mkhansenbot mentioned this pull request Aug 26, 2023

Add Nav2 to Space ROS space-ros/space-ros#67

Closed

mkhansenbot force-pushed the add_space_nav2 branch from 60acc19 to 23ce28d Compare August 26, 2023 22:26

mkhansenbot marked this pull request as draft August 27, 2023 15:42

Bckempa added this to the humble-2024.01.0 milestone Dec 3, 2023

mkhansenbot force-pushed the add_space_nav2 branch from 0eeebe4 to a242b7d Compare January 21, 2024 17:55

mkhansenbot marked this pull request as ready for review January 21, 2024 17:56

Bckempa reviewed Jan 22, 2024

View reviewed changes

mkhansenbot force-pushed the add_space_nav2 branch from 355ca65 to 8cd6391 Compare January 23, 2024 01:11

ivanperez-keera force-pushed the add_space_nav2 branch from 8cd6391 to 81f4ecc Compare January 29, 2024 00:08

ivanperez-keera self-requested a review January 29, 2024 01:38

mkhansenbot mentioned this pull request Jan 30, 2024

Update READMEs to include rocker as a requirement for run.sh scripts #128

Closed

mkhansenbot force-pushed the add_space_nav2 branch 2 times, most recently from 8479383 to 8ee7b75 Compare January 30, 2024 02:59

mjeronimo and others added 7 commits January 30, 2024 00:04

Initial Dockerfile for nav2 on Space ROS (space-ros#126).

8efcb23

Signed-off-by: Michael Jeronimo <[email protected]>

First build of navigation2/Dockerfile (space-ros#126).

e2ede95

Add pipefail to build script (space-ros#126).

40066f2

Use rocker to run docker image in run script (space-ros#126).

1cf281b

Change container name to osrf/space_nav2 (space-ros#126).

0153703

Fix path to setup.bash in entrypoint (space-ros#126).

c3e2ef2

Install rviz2 to send goals to Nav2 (space-ros#126).

cf479a7

ivanperez-keera force-pushed the add_space_nav2 branch from 8ee7b75 to ebf3d7a Compare January 30, 2024 16:51

ivanperez-keera approved these changes Jan 30, 2024

View reviewed changes

mkhansenbot added 3 commits January 30, 2024 09:28

Add README with instructions for running Nav2 and SLAM (space-ros#126).

563bf73

Add workflow step to build navigation2 (space-ros#126).

5f0a3d3

Add nav2_demo docker files (space-ros#126).

b82ca0a

ivanperez-keera force-pushed the add_space_nav2 branch from ebf3d7a to b82ca0a Compare January 30, 2024 17:28

ivanperez-keera merged commit bf23e58 into space-ros:main Jan 30, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add space nav2 #80

Add space nav2 #80

mkhansenbot commented Aug 25, 2023

mkhansenbot commented Aug 26, 2023

alexbuyval commented Aug 27, 2023

mkhansenbot commented Aug 27, 2023

alexbuyval commented Aug 29, 2023

quarkytale commented Dec 14, 2023

mkhansenbot commented Jan 21, 2024

Bckempa left a comment

Bckempa Jan 22, 2024

Bckempa Jan 22, 2024

Bckempa Jan 22, 2024

Bckempa Jan 22, 2024

Bckempa Jan 22, 2024

mkhansenbot Jan 30, 2024

Bckempa Jan 22, 2024

mkhansenbot Jan 30, 2024

Bckempa Jan 22, 2024

mkhansenbot Jan 30, 2024

Bckempa Jan 22, 2024

mkhansenbot Jan 30, 2024

Bckempa Jan 22, 2024

Bckempa Jan 22, 2024

xfiderek Jan 22, 2024 •

edited

Loading

mkhansenbot commented Jan 29, 2024

mkhansenbot commented Jan 29, 2024

mkhansenbot commented Jan 30, 2024

ivanperez-keera commented Jan 30, 2024 •

edited

Loading

mkhansenbot commented Jan 30, 2024

ivanperez-keera Jan 29, 2024

ivanperez-keera Jan 29, 2024

ivanperez-keera Jan 29, 2024

ivanperez-keera commented Jan 30, 2024

	LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker-images"
	LABEL org.label-schema.vcs-url="https://github.com/space-ros/docker"

	Follow instructions on space-ros/docker/space-robots/README.md
	Follow instructions in the [space-robots `README.md`](../space-robots/README.md)


		The new image is named osrf/space_nav2:latest.

		There is a run.sh script provided for convenience that will run the spaceros image in a container.

		@@ -0,0 +1,47 @@
		# Navigation2 Docker Image

		The Navigation2 Docker image uses the Space ROS docker image (openrobotics/spaceros:latest) as its base image. The Navigation2 Dockerfile installs all of the prerequisite system dependencies to build Navigation2

Add space nav2 #80

Add space nav2 #80

Conversation

mkhansenbot commented Aug 25, 2023

mkhansenbot commented Aug 26, 2023

alexbuyval commented Aug 27, 2023

mkhansenbot commented Aug 27, 2023

alexbuyval commented Aug 29, 2023

quarkytale commented Dec 14, 2023

mkhansenbot commented Jan 21, 2024

Bckempa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xfiderek Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Footnotes

mkhansenbot commented Jan 29, 2024

mkhansenbot commented Jan 29, 2024

mkhansenbot commented Jan 30, 2024

ivanperez-keera commented Jan 30, 2024 • edited Loading

mkhansenbot commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanperez-keera commented Jan 30, 2024

xfiderek Jan 22, 2024 •

edited

Loading

ivanperez-keera commented Jan 30, 2024 •

edited

Loading