ros-drivers / ros2_ouster_drivers

ROS2 Drivers for the Ouster OS-0, OS-1, and OS-2 Lidars
https://ouster.com/
Apache License 2.0
139 stars 79 forks source link

`lidar_ip` and `computer_ip` need better naming and/or documentation #44

Closed rotu closed 4 years ago

rotu commented 4 years ago

lidar_ip and computer_ip have similar names and are documented analogously to each other. This makes the following caveats unexpected:

SteveMacenski commented 4 years ago

From the wiki on hostnames:

... hostname is translated into an IP address via the local hosts file, or the Domain Name System (DNS) resolver.

Anywhere that a IP can be used, a hostname can also be used. That's more of a networking thing. That's the role of /etc/hosts which I'm sure you've played with.

Same on this on, PRs are welcome - overall I find that intuitive so clearly I'm not the right person to try to make it explicit :frowning:

SteveMacenski commented 4 years ago

Are there alternative names you would suggest?

rotu commented 4 years ago

Ah you're right about udp_ip being able to take a hostname! I was confused because the documentation specifically says "Returns the ip address to which the sensor sends UDP traffic." and because there is a long (15s) wait after reinitialize before data starts getting transmitted again during which the lidar spits out messages about communication problems.

I would maybe add a comment to udp_ip that says it can be 255.255.255.255 to broadcast to all hosts. Or I'd be okay with closing this issue if that's not a use case you think this should support.

SteveMacenski commented 4 years ago

so that computer_ip can be 255.255.255.255? Sure, that sounds reasonable - feel free to submit a PR in that parameter's table entry

SteveMacenski commented 4 years ago

47 supersedes with need for documentation

RFRIEDM-Trimble commented 3 years ago

The naming that linux uses is addr, not ip, when a parameter can take a hostname or ip address. https://man7.org/linux/man-pages/man3/getaddrinfo.3.html