cra-ros-pkg / robot_localization

robot_localization is a package of nonlinear state estimation nodes. The package was developed by Charles River Analytics, Inc. Please ask questions on answers.ros.org.
http://www.cra.com
Other
1.36k stars 880 forks source link

Migrating to ROS2 #265

Closed ayrton04 closed 5 years ago

ayrton04 commented 8 years ago

I'd like for this package to be ported to ROS2. This leads me to two questions:

  1. Should the package undergo a redesign before moving to ROS2? The number of use cases is clearly growing, and it's becoming harder to support all of the proposed changes within the existing framework (or at least to support them cleanly). Alternatively, we can do the redesign for ROS1 and the move the entire thing to ROS2.
  2. I really think the package needs to be renamed. I won't bother to go into the history of the package name, but it's clearly not a great description of what the package actually provides. Some ideas:
    • Robot Incremental Pose Estimation ("RIPE")
    • RObot State Estimation ("ROSE" - a little close to "ROS" perhaps?)

Or some mix of the two that would lead to acronyms like "RISE" and "ROPE." In any case, I'd welcome other ideas.

ayrton04 commented 8 years ago

I have decided to call the ROS2 package fuse. The (empty) repo is here:

https://github.com/ayrton04/fuse

gavanderhoorn commented 8 years ago

A bit late, but some semi-official guidelines on naming packages may be found in REP-144.

You're of course free to choose a name to your liking, but looking at that REP, 'fuse' seems to be a bit too general / non-descriptive.

ayrton04 commented 8 years ago

Nah, not late at all. I'm still in super-preliminary development, so the name isn't set in stone by any means. I'll think about this, and will also welcome suggestions!

msy22 commented 7 years ago

For what it's worth, I like the current package name, simply because it's a description of what the package does in layman's terms - it localizes the robot.

Names involving terms like "state estimation" or "incremental pose estimate" refer to how the kalman filters work: incrementally estimating the next state using the current state (plus sensor data/proces noise etc). It's more specific and that's great, but it pre-supposes that the user knows how a kalman filter works and understands that the "state" of a robot is its current pose+orientation. From trawling ros.answers it seems that a lot of ROS users are hobbyists/university students (like me) and pretty fresh to a lot of this jargon/terminology.

Another interesting perspective is that it seems that for many ROS users, English is a second language. The words "robot" and "localization" have meaning independent of the sentence they're in and probably survive translation a bit better. Phrases like "Robot Incremental Pose Estimation" rely on the whole phrase to make sense. So between that and the above you need the meaning and the context for the package name to make sense.

I know very little about other languages and I've completely over-analyzed this, but I figure that there is a lot of value in simplicity.

ayrton04 commented 7 years ago

So my primary concern is that many people would argue that localization is a bit of a misnomer. Localization itself typically refers to using a map to estimate the robot's pose. However, you could make the argument that the package itself can localize you within a map, even if it doesn't use the map itself to do so (a pose estimate is a pose estimate, regardless of whether it's correct!). It also can fuse poses from proper localization packages such as amcl with other sensor data to provide a more accurate global state estimate.

In any case, I'm not against leaving the name as-is by any means, especially as it means there can be continuity between ROS 1 and ROS 2. I just wanted to let the community weigh in. Thanks for the feedback!

msy22 commented 7 years ago

Ah, I see. That's a fair argument. In that case you're right and some of your suggestions above are more descriptive. It will be interesting to see what the final name is, and as always I really appreciate your consistent engagement with the community!

mkhansenbot commented 6 years ago

@ayrton04 - Hi Tom, what is the current status of the migration to ROS2? I see there is a branch here for it, but haven't tried to build it. Is it functional? I'm leading an effort for the ROS2 navigation system here: https://github.com/ros-planning/navigation2

I know this isn't technically part of the navigation itself, but I think it is a key extension. Let me know if you want to be involved in the ROS2 navigation effort, and what the status here is.

Thanks, Matt

ayrton04 commented 6 years ago

Hi Matt,

No, the branch isn’t functional yet. I got the core EKF and UKF to build, but the RosFilter class is ludicrously massive (it really needs to be refactored), and it’s taken a long time to work through it. I also had to take a hiatus to add parameter array support to rclcpp, because we need it for the sensor configs.

The real reason for the delayed development, though, is that we (Locus) are considering releasing a new state estimation package for ROS 1 and ROS 2, and so my efforts here are a bit in limbo. Even if we do go that route, though, I will wrap up the ROS2 port to ease the potential transition.

I’ll try to keep this space updated with any developments on that front.

Thanks!

-Tom

mkhansenbot commented 6 years ago

Hi Tom, Thanks for the update. If you release a new state estimation package, will that be compatible with this one? I'd like to know more, especially if it is something we should consider while working on the ROS2 Nav system. We may want to sync with you for tutorials & demos, etc.

Matt

ayrton04 commented 6 years ago

If by "compatible," you mean "will its interfaces be the same?" then yes. We'll support the same message input and output types, and I would assume we'd do a few more. Our goal would be to allow for it to be a drop-in replacement. However, it will obviously have very different looking parameters, though I'd be likely to make a conversion tool to ease that.

I'll dust off my ROS2 dev space and keep prodding at r_l in the meantime.

mkhansenbot commented 6 years ago

Yes, that's what I meant by compatible, thanks for answering. I just wanted to know if we needed to be aware of interface changes. Thanks!

ayrton04 commented 6 years ago

OK, I have this building and running, though I am having trouble with building the ROS 1 bridge so as to produce data to test it. But if anyone is interested, you can run with your config file by doing this:

ros2 run robot_localization se_node __params:=/path/to/your/ekf_config.yaml

If anyone happens to have time to give it a shot and wants to share results, that would be great.

mkhansenbot commented 6 years ago

@ayrton04 - I just saw this update that you have the ros2 branch building. We'll take a look at it and see if we can test it with the ROS2 Navigation stack.

ayrton04 commented 6 years ago

Thanks, @mkhansen-intel. I have been so far unsuccessful in working with the ROS1 bridge so as to actually play real robot data back through it. If it doesn't work right away, I wouldn't burn too much time on it.

Rohita83 commented 5 years ago

As per suggestion in https://discourse.ros.org/t/robot-localization-porting-to-ros2/6379/6, i am just Re-posting here below.

@ayrton04, For r_l initial testing, we have pulled r_l source from ros2 branch of the r_l repo and also pulled test folder from kinetic-devel. We are currently working on migrating test cases and we have ported/executed only “FilterBaseTest” testcase successfully but for remaining testcases, we have observed few things below.

Testcases are dependent on .test (launch) file that needs rostest/rosparam/rosbag etc pkgs that used in ros1 and not available in ros2.
For “filter_base_diagnostics_timestamps” testcase, diagnostics code is commented in r_l source.
For “robot_localization_estimator” and “ros_robot_localization_listener” testcase, estimator and listener related header/class are not available in r_l source.
“node_bag_pose_tester” testcase is also depend on tag (under .test file) for fetching parameter values from multiple .test file.

We are heading towards migration using below steps. As per migration guide, .test(launch) files should be converted into .py (launch.py) files using launch pkg in ros2. But we are facing issues in loading yaml file with this launch file as there is no ros2 load param feature available.

Also, in CMakelists.txt, .test files are added and those are getting executed with ros1 gtest. But in ros2 when we convert these .test into .py, we are not able to invoke those files from CMakelist.txt.

If anyone have any information/suggestion on the same, it will help us to move faster.

Rohita83 commented 5 years ago

@Tom_Moore, We are pleased to inform that we have migrated most of the robot localization tests into ROS2. Thanks to you , Matt Hansen and ros/discourse community for support.

But there are certain limitations as of now. 1)As rosbag is not available in ros2, we need to use ros1 bridge for few test cases. 2)rostest is not avaliable in ros2 and all .test files are converted into launch.py that can be executed as: ros2 launch 3)Because of above , as of now, colcon test does not work.

So there are two ways to run these test cases which are explained in README.md. 1) Test cases are passed by launching it independently with specific steps 2) Automatic bash scripts are created to run these test cases. Please note that to use automated scripts, we need to take care of naming conventions and source path which is explained in README.md.

README.md to test is uploaded under test folder on the same git path below. Please find the git path "https://github.com/Rohita83/robot_localization"

Do let us know for any suggestions.

Thanks,

Rohita83 commented 5 years ago

Hi All,

We have successfully built and tested the robot localization code on the ros2 crystal as well. Please find the updated source path below which also includes ros2_migration_readme.md for more details . Any improvements/suggestions are always welcome.

Branch Name: ros2-crystal-devel URL Path: https://github.com/Rohita83/robot_localization/tree/ros2-crystal-devel

ayrton04 commented 5 years ago

Branches are now bouncy-devel and crystal-devel on the main repo. I think we can consider this ticket solved. Thanks for everything, @Rohita83!

Rohita83 commented 5 years ago

Branches are now bouncy-devel and crystal-devel on the main repo. I think we can consider this ticket solved. Thanks for everything, @Rohita83!

Thanks @ayrton04 for accepting the robot_localization code changes. What is the next step to get it onto ROS2 repo (bouncy/crystal).