search

ros-drivers / rosserial

A ROS client library for small, embedded devices, such as Arduino. See: http://wiki.ros.org/rosserial
508 stars 527 forks source link

Made tcp_port a local parameter #562

Closed amalpavithran closed 3 years ago

amalpavithran commented 3 years ago

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

mikepurvis commented 3 years ago

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

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

mikepurvis commented 3 years ago

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

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

Looks perfect, thanks.