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

Official support for ROS2 #5825

Closed
ruffsl opened this issue Feb 10, 2020 · 25 comments
Closed

Official support for ROS2 #5825

ruffsl opened this issue Feb 10, 2020 · 25 comments

Comments

@ruffsl
Copy link
Contributor

ruffsl commented Feb 10, 2020

Hello RealSense team, I'd like to ask if we could have official support for RealSense with ROS2. I'm opening the floor here, as the ROS2 RealSense package over on the intel org doesn't seem to be watched by many of the core RealSense maintainers. As you may know, the RealSense device is quite popular with the robotics community, made accessible to robotics via the current ROS1 wrapper package.

I myself, as a robotics researcher, am using it for sensor data acquisition for a visual inertial odometry + SLAM pipeline for semantically annotated navigation. For performance reasons though, I've migrated to ROS2 to take advantage of shared memory transport, composition of multiple nodes per process, and greater quality of service features. I'm perhaps not alone in doing so as well: #5033 (comment)

In particular, I think it would be beneficial to migrate the intel/ros2_intel_realsense github repo over to the RealSense org, to help bolster maintainership and visibility. Currently, I alone have about six of the newest PRs and two open tickets that are at most two months old without any maintainer response. Most are trivial, but it's been bothersome to continually rebase all of these out of mainline changes.

cc @AndreasAZiegler @RealSenseCustomerSupport

@ruffsl
Copy link
Contributor Author

ruffsl commented May 1, 2020

It's been about two more months with no response. I think we may have to start thinking about alternative maintainership strategies to get the relevant realsense packages released into ROS2 Foxy.

It'd be a shame for support to languish given the amount of realsense hardware used with ROS:
https://discourse.ros.org/t/subt-challenge-urban-circuit-overview/13635

@SteveMacenski
Copy link

SteveMacenski commented May 4, 2020

@dorodnic any help here would be great, even just to say that we're not going to get any. Leaving the ROS community hanging on the flagship realsense product is unacceptable IMO. The lack of support and development of librealsense and realsense-ros is making the product line close to non-usable, especially adding in the complexity of librealsense. I have started to recommend ROS users move to the Astra Mini cameras as a result (https://orbbec3d.com/astra-mini-series/).

We want to like the realsense ❤️ but something really needs to change here. This has become ridiculous. How can there be 300+ tickets for a sensor driver when we can create an entire autonomous navigation system with < 70. And closing tickets from lack of activity automatically doesn't count as resolving problems.

@AndreasAZiegler
Copy link

As I already wrote in another ticket on this repo, I think it's quite a pity that there is no official ROS2 support for a sensor that is so heavily used within the robotics community. I already heard and read from a couple of companies that they want to move to ROS2 (mostly for stability) and most of them actually use a RealSense sensor. Therefore, IMHO it's quite a bad business decision (from an engineer point of view) to not support ROS2.

@RealSenseSupport
Copy link
Collaborator

Thank you for your interest in ROS2. Our focus is on ROS1 and other wrappers which are the vast mainstay of our customer base. This being the case, these other development projects have higher priority. We do work with many developers and academic researchers and understand the value of using the most recent software. We support over 15 OS’s, wrappers and languages across all our products in a single SDK. We believe this is the best depth SDK coverage in the market. On ROS2, we have logged it as future development request. We do not have clear timing when this would occur and wanted to acknowledge this community is being heard inside of our organization. It is these posts which make us consider our next roadmap items.”

@ruffsl
Copy link
Contributor Author

ruffsl commented May 8, 2020

Our focus is on ROS1 and other wrappers which are the vast mainstay of our customer base.

Sounds like a chicken-or-the-egg effect. Customers can't update if Intel doesn't update as well.

We support over 15 OS’s, wrappers and languages across all our products in a single SDK.

Indeed, and thanks for that! The PRs above however merely update existing support, being quite minimal and quick to review, but have been unacknowledged for a prolonged amount of time.

We do not have clear timing when this would occur and wanted to acknowledge this community is being heard inside of our organization. It is these posts which make us consider our next roadmap items.”

Would you then be willing to add external community maintainers so the rest of the community can move forward with migration and release of the ROS2 wrappers? Otherwise we'll need to fork to release the wrapper into a modern LTS Ubuntu and ROS distro. It'd be better to pool our efforts together.

@AndreasAZiegler
Copy link

AndreasAZiegler commented May 11, 2020

@ruffsl I like the idea to add external community maintainers to put efforts together. Forks often result in redundant work.

@RealSenseSupport As far as I understand there will only be one more ROS1 release and afterwards support for ROS1 is not longer given and the community is forced to move to ROS2. Therefore it would be wise, IMHO, to already move more towards ROS1.

@ruffsl
Copy link
Contributor Author

ruffsl commented May 11, 2020

As far as I understand there will only be one more ROS1 release and afterwards support for ROS1 is not longer given and the community is forced to move to ROS2.

The final release for ROS1, code-named Noetic, is planned for May 2020. More details can be found in the official announcement here: https://discourse.ros.org/t/planning-future-ros-1-distribution-s/6538

Therefore it would be wise, IMHO, to already move more towards ROS1.

Think you meant to type ROS2 😉 . But yes, starting the migration process now would be much better than procrastinating on the matter, and having to rush development as EOL for ROS1 approaches.

@SteveMacenski
Copy link

And also supporting the large number of customers moving to ROS2, some of which are blocked by realsense support.

@doronhi
Copy link
Contributor

doronhi commented Jul 9, 2020

Hi,
We consider supporting ROS2, a very important task of ours.
I don't have a specific release date yet but work for supporting RealSense with ROS2 is already underway. The idea is to keep it as close as possible to the ROS1 wrapper for maximal code sharing.

@SteveMacenski
Copy link

I'd general advise against trying to support both ROS1 and ROS2 with a common codebase, its going to require alot of fundamental rearchitecture to make use of components and lifecycle nodes. Plus the ROS1 drivers could use a refactor / make over so using this as an opportunity to learn from that in ROS2 would be beneficial to everyone.

(PS, the T265 publishing of TF isn't optional, that should be a param so it can be fused into other sources)

@ruffsl
Copy link
Contributor Author

ruffsl commented Jul 9, 2020

I concur with @SteveMacenski , while it may seem like a noble goal to unify the ROS1 and ROS2 wrappers into one code base, the client library APIs are substantially different given the growing number of optimizations and features in ROS2 that will never get back ported to ROS1, like zero copy shared memory transport. Given the final release of ROS1 has been relegated to maintenance mode and is on schedule for EOL in the next 1.5 years, I think it would be wise to keep the two different wrapper code base separate. Realsense already has an abstraction layer given the SDK library, attempting to abstract between that and two vary different client libraries that share little but the "ROS" trademark would be wasted labor, especially given the growing urgency for a migration path.

@doronhi , at present, I think the current realsence ROS2 wrapper that was refactored last year is a nice fresh start, as it makes good use of fundamental architecture like composed nodes and inherited sensor classes for sharing code across device models. It just needs some attention from the repo maintainers to review and merge the PRs fixes that have been piling up in limbo.

make over so using this as an opportunity to learn from that in ROS2 would be beneficial to everyone.

This would also be an excellent opportunity to fix the realsense wrapper to conform to REP standards in the ROS community, addressing them in the ROS2 wrapper code base without changing or breaking ROS1 apps the use the legacy ROS1 wrapper.

https://www.ros.org/reps/rep-0105.html

@ruffsl
Copy link
Contributor Author

ruffsl commented Jul 15, 2020

FYI, removal of realsense related packages from ROS Dashing, Foxy, and Rolling is expected due to unresponsive maintainers:
ros/rosdistro#25814

nuclearsandwich added a commit to ros/rosdistro that referenced this issue Jul 16, 2020
Reverts #24521

This release has not built on Dashing but previous releases have.
Since the maintainers currently have other priorities[[1]] reverting to a
working release is the best course of action available.

[1]: IntelRealSense/librealsense#5825 (comment)
nuclearsandwich added a commit to ros/rosdistro that referenced this issue Jul 16, 2020
#25837)

Reverts #24521

This release has not built on Dashing but previous releases have.
Since the maintainers currently have other priorities[[1]] reverting to a
working release is the best course of action available.

[1]: IntelRealSense/librealsense#5825 (comment)
@doronhi
Copy link
Contributor

doronhi commented Aug 4, 2020

We plan to release an official version that supports ROS2 in about 2 month from now.
@ruffsl and @SteveMacenski , thank you very much for your remarks. I read them and took them into heart.
The current ROS2 wrapper is based on an old version of the ROS1 wrapper. Therefor I took the fastest route and started converting the latest ROS1 wrapper, using pieces of code from the existing ROS2 wrapper to a new ROS2-eloquent version.
The new version is currently maintained here: https://github.com/IntelRealSense/realsense-ros/tree/eloquent

Plus the ROS1 drivers could use a refactor / make over

I fully accept that. I started by having a working version which I plan to refactor. One could argue that it would have been better to start from scratch but one also need a learning curve when stepping into the ROS2 environment. I have my ideas about refactoring but if you have some concrete ideas I would love to hear them.

This would also be an excellent opportunity to fix the realsense wrapper to conform to REP standards in the ROS community, addressing them in the ROS2 wrapper code base without changing or breaking ROS1 apps the use the legacy ROS1 wrapper.

I totally agree. I'll do my research on the REP standards but any pointers and observations you already have regarding the ROS1 version will help immensely and is more then welcome.

I'm sure the current version does not take full advantage of the new ROS2 capabilities and I would love some pointers here as well.

@ruffsl
Copy link
Contributor Author

ruffsl commented Aug 5, 2020

using pieces of code from the existing ROS2 wrapper to a new ROS2-eloquent version.

Consider targeting ROS2 Foxy to start with, rather than an older non-LTS release.

I have my ideas about refactoring but if you have some concrete ideas I would love to hear them.

You may want to checkout the migration page for ideas, as well as the list of tutorials, such as using intra-process-communication, node composition, staying real-time friendly:

I'll do my research on the REP standards but any pointers and observations you already have regarding the ROS1 version will help immensely and is more then welcome.

@SteveMacenski may have more recent advice on writing new ROS2 sensor packages,
But here are some REP references to start with:

If you're uncertain, I'd suggest posting to answers.ros.org and pinging folks like @tfoote .

I'm sure the current version does not take full advantage of the new ROS2 capabilities and I would love some pointers here as well.

You may want to look at my PRs from the top of the ticket for suggestions. E.g: Options to preserve or synchronize of sensor hardware timestamps for data frames would be nice for CV related robotics applications, exposing more of the SDK options via dynamic parameters in ROS2 would help with changing configuration during runtime.

@doronhi
Copy link
Contributor

doronhi commented Aug 5, 2020

Thanks for the pointers @ruffsl , I'll check them all.
The problem I have with foxy is that it targets Ubuntu 20.04 while librealsense2 officially only supports Ubuntu 18.04. That has to do mainly with some kernel patches still in development. Naturally foxy is the goal.

@ruffsl
Copy link
Contributor Author

ruffsl commented Aug 5, 2020

The problem I have with foxy is that it targets Ubuntu 20.04 while librealsense2 officially only supports Ubuntu 18.04

Perhaps you should discuss with @dorodnic , as librealsense2 looks to be staged for next foxy sync:

image

@SteveMacenski
Copy link

SteveMacenski commented Aug 5, 2020

Consider targeting ROS2 Foxy to start with, rather than an older non-LTS release.

Please do. If you plan on releasing it in 2 months, exactly 30 days later it will not be supported anymore. Eloquent goes out of support in November. Please focus on Foxy as the primary distribution people will be using, by a large margin. Having Dashing support wouldn't be bad either though.

Beyond what Ruffin listed, look up lifecycle nodes and make them lifecycle if possible, which should be possible. Especially how flaky the D435's are in mass volumes on a computer, we need a way to reset them (and also do that firmware reset thing needed to get a good bringup) if data stops processing. So I'd recommend lifecycle and exposing a service to do that firmware reset thing, or just do the firmware reset thing on startup automatically. I setup all my robots to do that because its such a problem.

Make sure you also add a parameter for the T265 and others for publish_tf so that can be disabled to publish the odometry to TF. That should always be optional so that the odometry output of T265 can be funneled into a sensor fusion framework like robot_localization with other sensors to output TF there.

By in large, the most irritating parts of realsense_ros has been a result of 'not knowing the standards'. Ruffin's links will help with that but also there are standard features like the publish_tf and frame names that might not be called out in the standards. You have to look at 3-4 other analog drivers and find the patterns / write down the features they have and names they use to use here. The other irritation is around not making fixes when there are issues ;-)

ROS2-y things you should make sure to do:

  • Derive from Node so it can be a component and follow best practices
  • Linters / CI setup
  • Librealsense released to apt
  • Librealsense and wrapper able to be installed in docker, in a colcon workspace without installing librealsense on your actual system separately
  • Get rid of the kernal patch stuff, its been enough years, there should be a work around or it should be in the OS
  • Dont try to make one magic node to rule them all. It makes that 1 object far too complex and poorly designed. The T265 is sufficiently different, stick that in its own node so you dont need the T265-specific code muddling up what should be a simple 300 line depth camera driver
  • I don't know how much the L515 differs from the D4xx series, but make a judgement on that one too.
  • Have someone at least part time responsible for the ROS2 repo to fix issues and merge PRs.

Realsense cameras are another case of 'I really want to like this!' but we just need reliable support, a good driver, and fixing issues as they come up, and resolving the interaction problems / headaches with librealsense. Given the number of robots running ROS and Realsense cameras, I'd think that would be in the best interests of Intel as well.

@shoemakerlevy9
Copy link

@doronhi I would also agree with focusing on ROS 2 Foxy. If the Foxy binaries worked with Intel RealSense Binaries by October that would go a really long way towards convincing our customer (www.roverrobotics.com) to make the switch. 95% of our customers are still using ROS 1 and the Intel RealSense driver often comes up as a reason not to switch.

@doronhi
Copy link
Contributor

doronhi commented Aug 8, 2020

Thank you @SteveMacenski . Your notes give me valuable information. I took them to heart and already started studying some of the subjects you mention. Hopefully they will help delivering this product in the very high level costumers rightly expect.

@doronhi
Copy link
Contributor

doronhi commented Sep 9, 2020

While working on the ROS2 wrapper, I came across an issue while trying to set "dynamic reconfigure" parameters. Is dictionary type supported?

For example, the parameter rgb_camera/power_line_frequency has 3 available option:
Disabled - 0
50 Hz - 1
60 Hz - 2
Auto - 3

In ROS1, using ddynamic_reconfigure I had a function: registerEnumVariable.
Using rqt_reconfigure in ROS1, it looks like that:
image

Is there a parallel in ROS2?
And on that note, is there an option for rqt in ROS2 to collapse parameters into groups, the way it's done in rqt_reconfigure in ROS1?

@SteveMacenski
Copy link

SteveMacenski commented Sep 9, 2020

Dynamic reconfigure doesn't exist in ROS2. There's parameter events where you can set dynamic parameters, but the library "dynamic reconfigure" no longer exists.

@doronhi
Copy link
Contributor

doronhi commented Sep 28, 2020

Thanks @SteveMacenski , but I think I was unclear about the issue. By now I released a first version of the ROS2 wrapper for librealsense2. Contrary to all your suggestions which I accept and believe in, the first version was released for Eloquent and is heavily based on the ROS1 version. This was caused by some constraints I had and hopefully will change in the near future.
The new ROS2 version registers to parameter events the way the ROS1 version works with dynamic_reconfigure parameters.
You can watch and modify them, on ROS2, using rqt with "parameter reconfigure" plugin.
I can have parameters of type bool, integer and double but what I can't figure out is how to represent options that can have one of several available states only. I gave the "power_line_frequency" parameter as an example. For that it seems a "dictionary" type of parameter would be needed. Since I couldn't find one I wonder what is the ROS2 way to address this situation.

@SteveMacenski
Copy link

Yeah, that doesn't exist quite yet - but me + ROS2 people would be more than happy to have a PR implement that. I think you're best off with an int with documented enum values. Or have your enums be strings that are parsed for specific enum values. Ex. OPTION_1 OPTION_2 OPTION_3 are string type param inputs that would then be parsed to their respective enum values in the parameters callback.

Frankly, any ROS2 wrapper for this is an improvement. Though its well known in the robotics community that the existing drivers have a number of problems and generally force companies to rewrite them internally (which defeats the point of an open driver).
I'd strongly suggest moving forward being able to start from a blank slate with a requirements list. It doesn't need to happen overnight though.

@doronhi
Copy link
Contributor

doronhi commented Dec 3, 2020

Thanks @SteveMacenski . Thank you for your points and insights. I agree with you completely.
Foxy is on the way. Expected release by the end of December 2020 - still not the blank-slate version.
As the ROS2-eloquent version was release a while ago, I think this issue can be closed now.

@doronhi doronhi closed this as completed Dec 3, 2020
@MartyG-RealSense
Copy link
Collaborator

Hi everyone,

The first ROS2 Foxy wrapper (3.1.3) is now available to match with librealsense 2.41.0.

https://github.com/IntelRealSense/realsense-ros/releases/tag/3.1.3

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

No branches or pull requests

8 participants