-
Notifications
You must be signed in to change notification settings - Fork 640
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
Fixes pointed out by clang-tidy #310
Fixes pointed out by clang-tidy #310
Conversation
The ament checkers are also happy with this arrangement. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Also use C++-style reinterpret_cast instead of C-style cast. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
There are a few problems. First is that it was doing an integer divide, which seems like it would be always 0. Second, the number that was being used was the raw integer, not the distance. Finally, the order of operations meant that it wasn't actually implementing the algorithm shown in the VLP documentation. This commit fixes all of those problems. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
None of the implementations use it. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
We were seeing artifacts in the organize_cloud case that this removes. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
There are a lot of changes here, but most of them are cleanup or making things more nice. There are 2 important bugfixes that I'll call out separately:
Both of these could be considered to be backported to the ROS 1 version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me. Most of them are pretty straight-forward. Since this changes the intensity reading, have you tried it on a sensor? Did you notice anything obvious?
I did try it out on both the VLP16 and VLP32C I have, and nothing was obviously wrong in the RViz2 visualization. However, I'm honestly not entirely sure how to test that it is doing the "right" thing; any thoughts there? |
Now that I think about it, the only way would be using a target with a known reflectivity and I doubt you have one of those lying around. If intensity appears to still be working in general, I'm OK with merging. |
Good assumption :). Thanks for the review and merge! |
This is a bunch of small fixes pointed out when running clang-tidy against the codebase. Most of them are minor, though there is one fix in here that changes the calculation of the intensity to match the documentation. It's probably easiest to review this commit-by-commit, as I've split them up logically.