-
Notifications
You must be signed in to change notification settings - Fork 187
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
base: ros2
Are you sure you want to change the base?
Conversation
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). |
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)? |
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 |
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 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:
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.
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.
🎉 These do a lot for usability so I'm a fan! |
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. |
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 |
@LotfiZ Yeah, using TF as in input implies that |
Addresses #75
Other changes from ros1 version:
laser_scan_matcher
describing topics and parametersPoseWithCovarianceStamped
kf_dist_linear
parameter tomin_travel_distance
kf_dist_angular
tomin_travel_heading