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

Multi-Robot SLAM in ROS 2 #727

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

Conversation

Daniel-Meza
Copy link

Basic Info

Info Please fill out this column
Ticket(s) this addresses #76
Primary OS tested on Ubuntu 22.04
Robotic platform tested on Turtlebot3 Burger (hardware and simulation)

Description of contribution in a few bullet points

This is a ROS 2 Humble port of the ROS 1 implementation from #541, extending SLAM Toolbox to support multi-robot mapping.

  • Localization and mapping using information from multiple sensors (one laser scanner per robot)
  • Loop closure across paths from multiple robots

Description of documentation updates required from your changes

  • Currently, both synchronous and asynchronous mapping is supported.
  • To operate, all robots must be part of the same transformation tree.
  • They must start close to each other; the first robot to connect establishes the map frame origin. Relative poses for the other robots are determined from the SLAM Toolbox solver.
  • Parameters odom_frames, base_frames, and laser_topics define the respective information from each robot.
  • Pose graph colors are randomly assigned for each robot (nodes and edges).

Source code, launch and configuration files are all contained within dedicated multirobot directories.

Future work that may be required in bullet points

  • Localization and lifelong mapping are pending. Currently, not a priority for me but would be good to have.
  • There is a poblem where robots identify each other as osbtacles. Especially when they're not moving upon first starting the mapping procedure.

Any assistance/suggestions on these is appreciated!

@SteveMacenski
Copy link
Owner

SteveMacenski commented Sep 10, 2024

Localization and lifelong mapping are pending. Currently, not a priority for me but would be good to have.

I wouldn't implement them. Localization is better suited on a per-robot basis anyway and the lifelong is experimental that should probably just be removed. I'd say this is done

Can you include a link back to the multirobot configuration file in your README section to point to an example?

I see both images/multirobot/pose_graph_colors.png and images/multirobot/pose_graph_colors_2.png, can the first be removed if its unused?


Motivational question before the review: Is there a reason these are duplicated files instead of having the couple hundred line changes inline in the main code? The previous PRs adding multirobot support didn't have that many changes to the core files. It looks like I wasn't able to merge them in because the authors either didn't respond or fix the items I needed fixed (or were on ROS 1 which I couldn't test)

I am honestly of 2 minds about it:

  1. I can more easily just merge this in with some quick testing since it is independent of existing capabilities so it won't break anyone, but also can't be used without knowing about the new nodes
  2. I can less easily merge this in because I can't see the few changes you made w.r.t. the original files, so I'll have to track down every line of the 2,400 line PR to validate it since I don't know what the few changes are.

I think I prefer it being a couple hundred line change in-line over the completely separated nodes, but I could be convinced otherwise...

@Daniel-Meza
Copy link
Author

Can you include a link back to the multirobot configuration file in your README section to point to an example?

Done!

I see both images/multirobot/pose_graph_colors.png and images/multirobot/pose_graph_colors_2.png, can the first be removed if its unused?

Done!


As for your motivational question...

The main reason was robustness ansd safety. Since this work was a "learn as I go" task, I wanted to avoid breaking anything in the application while modifying the original files.

Additionally, keeping the multi-robot functionality separate, at least in my mind, maintains it as an extended feature rather than a core capability. Like lifelong mapping, this approach makes it trivial to update the original files when new commits are made.


Lastly, do you have any suggestions on how (and where) to handle this issue?

There is a poblem where robots identify each other as osbtacles. Especially when they're not moving upon first starting the mapping procedure.

Resolving this would be necessary for a proper multi-robot SLAM solution.

@SteveMacenski
Copy link
Owner

SteveMacenski commented Sep 12, 2024

The main reason was robustness ansd safety. Since this work was a "learn as I go" task, I wanted to avoid breaking anything in the application while modifying the original files.

Would it be too much effort to have another branch where you make the few changes you made to these files in-line with the original software? It might help me make that judgement call if I think there's anything you've done that makes me think 'yeah, maybe its better to keep these separated'.

Additionally, keeping the multi-robot functionality separate, at least in my mind, maintains it as an extended feature rather than a core capability.

If its mostly allowing an array of information to stream in and fixing a few frames, I think its not much to have separated (and adds double the maintainer time to fix the multi-robot version when new features / bug fixes are added to the main software, since its duplicated).

There is a poblem where robots identify each other as osbtacles. Especially when they're not moving upon first starting the mapping procedure.

This is virtually a non-issue since other robots are no different than people, other vehicles, etc in a dynamic scene :-)

@Daniel-Meza
Copy link
Author

Would it be too much effort to have another branch where you make the few changes you made to these files in-line with the original software? It might help me make that judgement call if I think there's anything you've done that makes me think 'yeah, maybe its better to keep these separated'.

I’ve created the new branch with the changes integrated into the original files. For now, I’ve commented out the compilation of the "lifelong mapping" and "localization" components, as they require additional adjustments to align with the updated source code. However, this should give you a clear overview of the modifications made for the multirobot functionality.

Should I open a separate pull request? Or is there a cleaner way to integrate the new branch into this thread?

@SteveMacenski
Copy link
Owner

Should I open a separate pull request? Or is there a cleaner way to integrate the new branch into this thread?

Lets start with just a link. I can look at the diff and see what seems like the best path forward

@Daniel-Meza
Copy link
Author

@Amronos
Copy link

Amronos commented Oct 16, 2024

Are there any updates on the status of this PR?

@SteveMacenski
Copy link
Owner

SteveMacenski commented Oct 16, 2024

@Daniel-Meza sorry for the delay. ROSCon/ROS-I/Actuate conferences and talks have really delayed some of my reviews.

I think your diff looks good against the main servers. I have some review comments for changes, but largely that should sail through with a couple of iterations! I'll just test myself that the single robot case still works and if you can show the 2+ robot situation works, I'm happy with that

@Daniel-Meza
Copy link
Author

No problem at all. Feel free to send over your comments and I'll incorporate them as best as I can.

Also, I don't feel qualified to speak on other multi-robot slam implementations so I'll limit my contributions to this work. However, I'm happy to make necessary adjustments to ensure those who have made further progress can contribute their work to this project.

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