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

Made tcp_port a local parameter #562

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

amalpavithran
Copy link
Contributor

The tcp_port parameter being a global parameter prevents the launch of multiple serial nodes on different port numbers using a launch file

The tcp_port parameter being a global parameter prevents the launch of multiple serial nodes on different port numbers using a launch file
@mikepurvis
Copy link
Member

I agree with this in principle, but it's probably also a breaking change. Could you consider implementing this using a fallback where it looks in the private namespace and if the param isn't there it also checks the legacy name?

Forr consistency the same thing should probably done with the fork_server parameter, though I suppose there's little reason that would ever differ between instances.

@amalpavithran
Copy link
Contributor Author

Have made the necessary changes do have a look and let me know if anymore changes are required 😄

@mikepurvis
Copy link
Member

Looks great, thanks! There's no need for a fallback in the initial get_param case since we'll only hit that if we know it exists, and a comment explaining that the construct is there to not break legacy users would be helpful. But if you don't get to that, I'll merge anyway in a bit.

@amalpavithran
Copy link
Contributor Author

Yeah I was also having doubts about the initial get_param then felt that I will just leave it in there as it wasn't causing any issues. Have pushed a commit for that now with the comments you have requested. Do modify the comments as I feel I might not have been able to covey what you have requested.

@mikepurvis
Copy link
Member

Looks perfect, thanks.

@mikepurvis mikepurvis merged commit c169ae2 into ros-drivers:noetic-devel Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants