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

Fix serial parameter not taken into account. #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bapsti
Copy link

@Bapsti Bapsti commented May 17, 2022

Consideration of the serial parameter does not work, which is problematic when using multiple sensors.
These modifications take this parameter into account and adjust the default configuration of rviz.

@dave992
Copy link
Member

dave992 commented May 18, 2022

Thank you for the PR. it is much appreciated! It has been some time since I worked on this, but remember there being some issues with the serial number indeed. I think one of them I try to fix in cdd39cc in #24.

One question I have:

  • What is the reason behind the changes to the default Rviz configuration and why do you think this should be included?

As for the launch file changes, I agree with adding the parameter(s) to load_driver.launch, but would then also like to see it as a possible arg to the launch file itself. This will allow using the launch file in your own application with the possibility of defining specific names without the need to change this package itself.

As there is currently an open PR that also targets the launch files I think this might be better suited to be added there, or as it is a separate issue via another PR. This makes sure the PR is targeted at the serial parameter bug fix.

@Bapsti
Copy link
Author

Bapsti commented May 19, 2022

Hi,
In fact changes in the default Rviz configuration file and in launch files are not directly related to the serial parameter bug fix. They are just handy modifications that allow to visualize sensor data at startup without the need to change the pointcloud topic in Rviz.
It might indeed be a good idea to put that in an other PR.

@dave992
Copy link
Member

dave992 commented May 25, 2022

Thanks for the updated PR, I'll try to test it today or next week to see if it works as expected with one of our sensors.

@dave992
Copy link
Member

dave992 commented Jul 25, 2022

I have not yet forgotten about this, but didn't find the time to dive into this yet as the laser line sensor is being used currently.

@andreflorindo
Copy link

Faced today the very same problem and this pull request does solve the issue with the serial parameter. Tried out this afternoon and indeed it works as expected. Thanks @Bapsti for the feedback.
@dave992 feel free to approve this pull request.

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.

3 participants