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

Port to ROS2 #1162

Closed
mkhansenbot opened this issue Jan 16, 2019 · 19 comments
Closed

Port to ROS2 #1162

mkhansenbot opened this issue Jan 16, 2019 · 19 comments
Assignees
Labels

Comments

@mkhansenbot
Copy link

Our team has ported Navigation to ROS2: github.com/ros-planning/navigation2

We would really like to be able to use cartographer in ROS2 also to generate maps. I've been trying to get this fork to build: github.com/ros2/cartographer_ros but having issues, and think it would be better if it were a collaborative effort anyway.

Please respond and let me know if you plan to add a ros2 branch in the near future. If you need more information or help from me, please let me know.

@MichaelGrupp
Copy link
Contributor

disclaimer: I'm a contributor, not a maintainer of cartographer_ros.

I've been trying to get this fork to build: github.com/ros2/cartographer_ros but having issues

I don't know much about the ROS2 fork that you linked, but I can safely say it's very outdated in terms of upstream commits (older than a year). We did several important changes in cartographer_ros throughout the last year that are missing from it. The current upstream state can be considered stable, while the old 0.3 version that the ROS2 repo was forked from is definitely not. So, from my side 👍 for better collaboration.

add a ros2 branch in the near future

Although I haven't tried ROS2 myself yet, I like the idea. Just as with the fork above, my concern is however how to keep this in sync with the ROS1 master branch, which is under active development.

I would be interested in how difficult it would be to support both ROS versions in parallel in the same repository.

I think this would be a great topic for an RFC, which you could submit here: https://github.com/googlecartographer/rfcs

Also, today at 5pm CET (9am PDT) a "Cartographer Open House" will happen, maybe you can join it and mention your idea. https://github.com/googlecartographer/cartographer#open-house

@mkhansenbot
Copy link
Author

We believe we have a working implementation now of ros2 cartographer. Do I need to submit an RFC to get a ros2 branch created so we can submit a PR? We're not changing functionality or adding features, so I'm not sure what more an RFC will say that I haven't said in this issue. Please let me know how to proceed.

@MichaelGrupp
Copy link
Contributor

MichaelGrupp commented Jan 28, 2019

I mentioned the RFC process as a way to discuss the integration/maintenance process before starting an implementation, because I'm not sure whether maintaining a separate ROS2 branch is the right approach. But of course, if you already have a branch you can open a PR to get feedback.

@MichaelGrupp
Copy link
Contributor

MichaelGrupp commented Jan 28, 2019

@cschuet @gaschler @pifon2a @sebastianklose What do you think?

@sebastianklose
Copy link
Contributor

@mkhansen-intel Thanks for looking into this. If you have a branch ready, I think it would be best to put this up for review. Based on the changes we can then decide if we a RFC is needed to decide how we maintain it.

@routiful
Copy link

routiful commented Feb 1, 2019

I've opened PR into ros2 repo cartographer with ROS2.
I had been developed the code based on release-1.0 tag.

cartographer : ros2/cartographer#7
cartographer_ros ros2#25

If you want to test this, please following emanual
http://emanual.robotis.com/docs/en/platform/turtlebot3/ros2/#ros2

@ahuizxc
Copy link

ahuizxc commented Feb 15, 2019

I've opened PR into ros2 repo cartographer with ROS2.
I had been developed the code based on release-1.0 tag.

cartographer : ros2/cartographer#7
cartographer_ros ros2#25

If you want to test this, please following emanual
http://emanual.robotis.com/docs/en/platform/turtlebot3/ros2/#ros2

Hi Lim, I have added the occupancy node to cartographer_ros with ROS2 crystal, and test it in my turtlebot3. I wonder if we done the same work? thanks

@mkhansenbot
Copy link
Author

@sebastianklose - Can we get the ROS2 branch created so @routiful can submit his changes in a PR to that branch? Right now this exists in a fork of the OSRF ROS2 fork. It would really be best for all if we could upstream it all here. You can still have the RFC discussion of course. Right now we just need a branch created, as in #1172

@mkhansenbot
Copy link
Author

@sebastianklose - are you the right person to create a ros2 branch so a PR can be submitted? If not, who is the right person?

@ojura
Copy link
Contributor

ojura commented Mar 22, 2019

@mkhansen-intel, AFAIK the Cartographer team has never had any branches except a single master branch, PRs to which are carefully reviewed. I think they would most prefer if the ROS2 bringup was able to follow the same process and be merged to the main branch.

I could be wrong though, perhaps they might decide to open a special ros2 branch.

@MichaelGrupp
Copy link
Contributor

MichaelGrupp commented Mar 22, 2019

As I understood the PR was meant as a first step to get a grasp of the diff that the changes that were already implemented introduce. So more like an early draft PR that probably won't get merged immediately. This can be opened against master.

The discussion for a separate branching model or a ROS1/2 hybrid (if possible, or sth else) is still open.

RFC (+draft PR) -> discussion -> decision -> implementation/refinement -> integration is still the sane way to go in my opinion, as we did/do for other changes too.

@mkhansenbot
Copy link
Author

@ojura - the changes for ROS2 are incompatible with ROS, so if they get merged to the master branch, the master branch won't build in ROS anymore. I don't think that is what is wanted by anyone. Most other repos have added a ROS2 branch for this reason.

@MichaelGrupp - we don't want these changes merged to master, therefore we don't want to submit a PR to master. We can do that but it would need to be re-submitted to a ros2 branch before merging.

It feels to me like I'm having to push really hard on this to make it happen. If none of the maintainers care about ROS2, then maybe a fork is the right solution, in which case I'll close this issue.

@MichaelGrupp
Copy link
Contributor

if none of the maintainers care about ROS2

I really don't think that's the case. There is just not much experience with ROS2 in this project yet, so there are a lot of unknown variables.

Official ROS2 support would be a great addition, but the development roadmap is unclear to me.

the changes for ROS2 are incompatible with ROS
[...] other repos have added a ROS2 branch for this reason.

For example, this is something at least I didn't know for sure.

But even if we have a technically working ROS2 branch now:

  • do we want to develop for both ROS versions?
  • ...or is bleeding edge only on ROS1, and someone backports to ROS2 ?
  • what about documentation? (I guess the ROS1 docs won't be valid for ROS2?)
  • ...

Sorry If I'm annoying, but the advantage of an RFC is that such things are collected and will finally get documented in a simple document that can be referenced easily, instead of a thread like here. It's a very lean process that would be definitely helpful to get started towards ROS2.

@ojura
Copy link
Contributor

ojura commented Mar 22, 2019

@mkhansen-intel don't get us wrong, we would love to see progress on ROS2, and it would suck if you had to fork. @MichaelGrupp and I are not maintainers but rather contributors, and we're just explaining how things usually go here. Based on the prior development history and how Google did things so far, this project absolutely does not do branches and only has a single master branch, kind of like Linus. (Again, this is not necessarily the maintainers stance about this - perhaps they will be open to creating a ROS2 branch, I can't speak on their behalf). PRs have the role of feature and development branches, and they are merged only after review from maintainers and if they pass CI.

Discussion for larger-scale changes goes on in the RFC repository (https://github.com/googlecartographer/rfcs). And as Michael said, the plan for adopting ROS2 surely deserves an RFC. Besides the points Michael raised, since cartographer_ros is a relatively thin wrapper around the Cartographer library, why do you think it's impossible to have the same branch work for ROS2 and ROS1? Surely we can have separate code paths where necessary, and/or abstract away the differences between ROS1 and ROS2? I would love to see this discussed in more detail in an RFC. And you can open a ROS2 PR to go alongside the RFC so we can discuss the changes.

Also: the maintainers are a bit tardy in responding lately, please arm yourself with patience :)

@ruffsl
Copy link

ruffsl commented Mar 22, 2019

Based on the prior development history and how Google did things so far, this project absolutely does not do branches and only has a single master branch, kind of like Linus.

Besides the points Michael raised, since cartographer_ros is a relatively thin wrapper around the Cartographer library, why do you think it's impossible to have the same branch work for ROS2 and ROS1? Surely we can have separate code paths where necessary, and/or abstract away the differences between ROS1 and ROS2?

There are many differences that necessitate separate code bases, think Death by a thousand paper cuts. Aside from the obvious changes, like client API, depreciation of nodelets:

  • Different and incompatible build frameworks, catkin vs ament
  • Different launch systems, ros1 xml launch files, vs ros2 python launch files
  • Divergent features in message IDL
  • No centralized parameter server

If this wrapper is so thin, perhaps a new repo is sensible , googlecartographer/cartographer_ros2?
We could evaluate what still could still be shared between the two version of ros wrappers?

@ojura
Copy link
Contributor

ojura commented Mar 22, 2019

We could evaluate what still could still be shared between the two version of ros wrappers?

Agree. Discussing differences like these is exactly why we first need a well written RFC to get us all up to speed. For example, I had to google what exactly is IDL because I am not familiar with ROS2. I feel that you feel it's unnecessary bureaucracy and a waste of time, I thought so at first, but it turns out that it really is a good way to make decisions, make the motivation behind certain things clear and get everyone informed.

Anyway, I feel I'm stepping out of bounds here a bit, since I'm not a maintainer, but I can tell you that a well written rfc has the greatest chance of getting a reaction from the maintainers that you want :)

@AndreasAZiegler
Copy link

I would really like a ROS2 version of the Cartographer too. Due to the fact that ROS1 and ROS2 are incompatible, I also think it would be best to have a cartographer_ros2 repository.

After reading through this issue, I feel like nothing gonna happen due to bureaucracy, which would be a pity. Specially as ROS2 is making a lot of progress. @mkhansen-intel Could you open such a RFC, to push Cartographer ROS2 further?

@MichaelGrupp
Copy link
Contributor

See here for a new discussion on ROS2: #1536

Closing this one.

@mkhansenbot
Copy link
Author

I'm glad to see this is happening, I can't contribute at this time but will keep an eye on #1536 going forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants