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

ROS2 port for laser_scan_matcher and scan_to_cloud_converter #81

Open
wants to merge 2 commits into
base: ros2
Choose a base branch
from

Conversation

malban
Copy link

@malban malban commented Jul 28, 2022

Addresses #75

Other changes from ros1 version:

  • Added README file to laser_scan_matcher describing topics and parameters
  • Added parameters for scaling covariance
  • Removed pose publishers other than PoseWithCovarianceStamped
  • Removed parameter to enabled pose publishing and replaced with checking subscriber count instead
  • Made applicable parameters dynamically reconfigurable
  • Renamed kf_dist_linear parameter to min_travel_distance
  • Renamed kf_dist_angular to min_travel_heading

@cst0
Copy link
Collaborator

cst0 commented Jul 29, 2022

This looks great, thanks for doing this... I'll try this out with some other laser hardware for the sake of being thorough and will report back (today or maybe tomorrow).

@130s
Copy link
Collaborator

130s commented Jul 29, 2022

Looks like a huge work! I'm not (yet) familiar with ROS2 so I defer reviews to others. If @cst0 approves I'll be good with merging.

Is this independently mergeable from the upstream change (tracked in AndreaCensi/csm#33)?

@malban
Copy link
Author

malban commented Jul 29, 2022

Is this independently mergeable from the upstream change

Yes, it should be agnostic to whether csm is using GLS or eigen, though I would prefer that the csm release for ros2 debians be based on eigen to avoid transitively creating a GPL dependency for laser_scan_matcher

@cst0
Copy link
Collaborator

cst0 commented Jul 30, 2022

Overall I think this is great, but after testing I do have some more thoughts.

I tested this PR in a clean environment using this Dockerfile. This gave me a shell in this clean environment capable of interacting with the rest of my ROS nodes, allowing me to run the nodes for testing.

Unfortunately, that test did show a dependency problem which may cause a delay here: it appears that the csm dependency cannot be installed via rosdep for ROS2 distributions, since that has not yet been added to the rosdep distributions.yaml file. Building from source works, but requires that a non-master branch be used (I used malban/pkg-config, there may be others that work; try commenting/uncommenting these lines in the dockerfile).

My takeaway from this is that it's necessary to prioritize the merge over at the CSM repo before updating the ROS2 dependencies.yaml, which will then unblock things over here.

That aside, regarding the rest of this PR:

  • rename kf_dist_linear, kf_dist_angular
  • remove pose publishers other than PoseWithCovarianceStamped

These are API changes, but 1) moving from ROS1 to ROS2 is already an API change and 2) the content being removed isn't helpful anyway. So I think it's a positive change.

  • Added parameters for scaling covariance

I don't fully understand the utility of this: covariance is a defined measure of similarity between two random variables... if arbitrary multiplications are allowed, it's no longer a true covariance measurement.

  • Added README file to laser_scan_matcher describing topics and parameters
  • Made applicable parameters dynamically reconfigurable

🎉 These do a lot for usability so I'm a fan!

@malban
Copy link
Author

malban commented Jul 30, 2022

I don't fully understand the utility of this: covariance is a defined measure of similarity between two random variables... if arbitrary multiplications are allowed, it's no longer a true covariance measurement.

That's a good question, the general answer is:

When fusing the pose estimate from the scan matching with other localization inputs in something like an EKF, the covariance will determine how much effect the scan match pose has. Allowing the covariance to be scaled provides a mechanism to tune how much influence the scan match has. Ideally, all the covariances would be based on some physical properties and uncertainties, but it's often done more heuristically in practice, so allowing it to be scaled with a parameter is helpful.

Also, since the scaling is uniform, it won't effect the "co" part of covariance between x and y.

More specifically, the covariance calculation is described here: https://www.researchgate.net/publication/224705616_An_accurate_closed-form_estimate_of_ICP'S_covariance

While this works well, it's only an estimate and won't take into account sensor biases. It's usually the case that sensor error isn't zero-mean, which can result in the estimated covariance being overly confident, sometimes by a lot.

@LotfiZ
Copy link

LotfiZ commented Aug 31, 2022

Really great work here @malban, however i have some confusion regarding the tf. If we provide an initial guess, from an odometry node for example, by the tf (odom->base) mechanism, and in same time we publish same tf provided by scan matcher, there will be tf's confusion here or i missed something here ? Thank you

@malban
Copy link
Author

malban commented Aug 31, 2022

@LotfiZ Yeah, using TF as in input implies that publish_tf can't be used. I can add in a check to make sure both use_tf and publish_tf are not used at the same time.

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.

4 participants