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

Add support for ROS2 humble #47

Open
wants to merge 9 commits into
base: ros1-master
Choose a base branch
from

Conversation

nakai-omer
Copy link

Hi,

We have ported this lib to ROS2 humble, could probably work on older ROS2 versions as well, just the static transform arguments have changed in humble.
We tried to make only small changes, without major refactoring, in order to be able to easily migrate fixes from current implementation to this branch.

@nakai-omer nakai-omer mentioned this pull request Jul 13, 2022
@nakai-omer
Copy link
Author

it should be merged into a humble branch to match ros guidelines

@ryanpennings
Copy link

@nakai-omer I had to change Line 219 of laserscan_multi_merger.cpp otherwise the range was limited. See here. Just wondering if you have the same issue?

@nakai-omer
Copy link
Author

nakai-omer commented Aug 9, 2022

@ryanpennings Thanks for the feedback. In our implementation we use the transformLaserScanToPointCloud override which doesn't include the range_cutoff param at all. This to my understanding, should cause the range_max from the laserscan message to be used instead. Make sure the value of max range in your scan message is correct.

@samiamlabs
Copy link

I had the same limited range issue as @ryanpennings and the fix worked.

Also, I had to set QoS for the publisher to be reliable in order for the merged scan to show up in rviz2.

@nakai-omer
Copy link
Author

@samiamlabs Can you please check what is the max_range param from your laser scan message, and whether is matches your configured range_max in this package? I will merge this line fix if needed.

@samiamlabs
Copy link

I have already checked that. I could not find a transformLaserScanToPointCloud that matched the arguments you were using.

@nakai-omer
Copy link
Author

@samiamlabs Thanks for the update. I have added @ryanpennings fix to this PR.

@vineet131
Copy link

Hi maintainers and author @nakai-omer
Are there any other changes that are needed to be made on this branch? Can it be merged so we can have the option for ROS2?

@nakai-omer
Copy link
Author

@vineet131 From my point of view, it can be merged, it is working for us with ROS2 Humble.

@vineet131
Copy link

vineet131 commented Apr 15, 2023

@vineet131 From my point of view, it can be merged, it is working for us with ROS2 Humble.

Thank you! So maintainers @axelfurlan and @trigal can we have this PR merged?

Edit: Asking again including @iralab

@vineet131
Copy link

Hello authors @JackFrost67 @axelfurlan @trigal @iralab

Once again I wanted to ask if this PR can be accepted so that ROS2 upstream support for this repo can be obtained. It would be of help to us. Thank you.

@andrycapod
Copy link

Hello mantainers @JackFrost67 @axelfurlan @iralab any update about the possibility to merge the PR, for us it is working well in ROS Humble!

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.

5 participants