-
Notifications
You must be signed in to change notification settings - Fork 531
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
base: humble
Are you sure you want to change the base?
Multi-Robot SLAM in ROS 2 #727
Conversation
…nd separate classes for multirobot stuff
…bots in loop_closure_assistant file
…randomized colors. Loop closure connections between paths of different robots are defaulted to blue
…robot slam section to readme file
…r than an array of SPHERE markers. This should improve performance
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 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:
I think I prefer it being a couple hundred line change in-line over the completely separated nodes, but I could be convinced otherwise... |
Done!
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?
Resolving this would be necessary for a proper multi-robot SLAM solution. |
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'.
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).
This is virtually a non-issue since other robots are no different than people, other vehicles, etc in a dynamic scene :-) |
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? |
Lets start with just a link. I can look at the diff and see what seems like the best path forward |
Are there any updates on the status of this PR? |
@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 |
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. |
Sorry for the delay, ROSCon, ROS-I and so forth has kept me busy as of late and returning back to normal. @Daniel-Meza can you follow up on the comment thread in #592 I started on a meeting between yourself, me, and @acachathuranga to find a shared path forward here? I like your work here and appreciate your time! This is all great work :-) |
Notes from our conversation:
|
Basic Info
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.
Description of documentation updates required from your changes
odom_frames
,base_frames
, andlaser_topics
define the respective information from each robot.Source code, launch and configuration files are all contained within dedicated multirobot directories.
Future work that may be required in bullet points
Any assistance/suggestions on these is appreciated!