CCNYRoboticsLab / scan_tools

ROS Laser scan tools
300 stars 187 forks source link

ROS2 port for laser_scan_matcher and scan_to_cloud_converter #81

Open malban opened 2 years ago

malban commented 2 years ago

Addresses #75

Other changes from ros1 version:

cst0 commented 2 years ago

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 commented 2 years ago

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 https://github.com/AndreaCensi/csm/issues/33)?

malban commented 2 years ago

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 commented 2 years ago

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

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

malban commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.