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

Migration to jazzy #83

Merged
merged 5 commits into from
Sep 16, 2024
Merged

Migration to jazzy #83

merged 5 commits into from
Sep 16, 2024

Conversation

JesusSilvaUtrera
Copy link
Collaborator

@JesusSilvaUtrera JesusSilvaUtrera commented Sep 2, 2024

🎉 New feature

Closes #71

Summary

I have changed the docker files to suport Jazzy instead of humble and tried out the simulation using Harmonic.

Test it

Build and run the image, then just source the workspace and try out the simulation as suggested on the README.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)

@JesusSilvaUtrera
Copy link
Collaborator Author

The simulation is working just fine, but there is a problem with the SLAM: I tried it out to see if it works fine, but it seems to not be finding the YAML file for configuration.

I have checked this on the new workspace that I created for the source code of Andino, and the file is being added correctly when I build this workspace:

ubuntu@themis:~$ cd ros_deps_ws/install/andino_slam/share/andino_slam/config/
ubuntu@themis:~/ros_deps_ws/install/andino_slam/share/andino_slam/config$ ls
slam_toolbox_online_async.yaml

So I don't know why it's not finding it (if you see the Dockerfile this workspace is being sourced by the .bashrc file on every terminal).

I tried using an unified workspace for all but it's not working really fine. I will continue trying to debug this when I have more time, but JIC you have any thought @francocipollone

@francocipollone
Copy link
Collaborator

francocipollone commented Sep 4, 2024

The simulation is working just fine, but there is a problem with the SLAM: I tried it out to see if it works fine, but it seems to not be finding the YAML file for configuration.

I have checked this on the new workspace that I created for the source code of Andino, and the file is being added correctly when I build this workspace:

ubuntu@themis:~$ cd ros_deps_ws/install/andino_slam/share/andino_slam/config/
ubuntu@themis:~/ros_deps_ws/install/andino_slam/share/andino_slam/config$ ls
slam_toolbox_online_async.yaml

So I don't know why it's not finding it (if you see the Dockerfile this workspace is being sourced by the .bashrc file on every terminal).

I tried using an unified workspace for all but it's not working really fine. I will continue trying to debug this when I have more time, but JIC you have any thought @francocipollone

The problem relies on the fact that for Jazzy the slam toolbox node changes a bit:
https://github.com/SteveMacenski/slam_toolbox/blob/jazzy/launch/online_async_launch.py

Basically, it is now a lifecycle node. So when we are running the node the node gets stalled (you can play with the ros2 lifecycle cli). So what they did in their launch file was to add some events to advance the node.
We should do the same. Basically replicate what they have there.

Naturally you will ask why don't we just include the launch file into this launch file using IncludeLaunchDescription? You can try but I think that you can't include a launch file with some EmitEvents, you can try.

PS: You can also try directly the launch file from slam toolbox to understand the case: ros2 launch slam_toolbox online_async_launch.py

@francocipollone
Copy link
Collaborator

@JesusSilvaUtrera Take a look

@JesusSilvaUtrera
Copy link
Collaborator Author

@JesusSilvaUtrera Take a look

Ohhh I see, I was researching a bit about the slam_toolbox but didn't see that they changed the node in Jazzy.

Thanks for this Franco, I will try it out when I have time this week.

@JesusSilvaUtrera
Copy link
Collaborator Author

JesusSilvaUtrera commented Sep 5, 2024

@francocipollone I have tried it out, and including the file does the trick, so no need to replicate the behavior, I even tried out creating a map:
my_map.zip

With this, we should be fine to merge this, so PTAL and approve the PR if you think it's okay.

Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Looks good! Let's do something before proceeding. Let's update/rebase Jazzy branch with all the humble latest stuff. There were several changes that we should verify that they are still working in this PR.

Comment on lines +97 to +98
# chown -R "$USER" ${REPOSITORY_FOLDER_PATH}/.build
# chown -R "$USER" ${REPOSITORY_FOLDER_PATH}/.install
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to double check this cause I had an issue locally with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the issue is when building the workspace, I think if you delete the build and install directories does the trick, I didn't find a workaround for that yet due to the ubuntu user.

@JesusSilvaUtrera
Copy link
Collaborator Author

@francocipollone I have tried it out after merging the latest updates from humble and it still works fine, so PTAL to see if we can merge it for now (maybe I can add on the docker README a warning about the issue with the build and install directories for now until we find a wrokaround).

Copy link
Member

@jballoffet jballoffet left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @francocipollone, thanks @JesusSilvaUtrera!

@JesusSilvaUtrera JesusSilvaUtrera merged commit 030c8f3 into jazzy Sep 16, 2024
2 checks passed
@JesusSilvaUtrera JesusSilvaUtrera deleted the jsilva/migration_to_Jazzy branch September 16, 2024 15:25
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