Socket driver #32
Socket driver #32
Conversation
As suggested by - ros-drivers#16 (comment) - ros-drivers#16 (comment)
According to the list at http://www.catb.org/gpsd/NMEA.html#_talker_ids
|
Retargeted to master. |
There was a problem hiding this comment.
Sorry for my tardy response on this. ROS isn't my day job anymore, and most of these PRs actually predate me taking over maintenance of the repo.
Overall, this looks like a good improvement to the repo, although I don't have any way to test it personally.
I requested a few changes. If you can make those changes, I think we can merge this PR and probably even back port it to supported releases.
scripts/nmea_socket_driver
Outdated
| local_ip = rospy.get_param('~ip', '0.0.0.0') | ||
| local_port = rospy.get_param('~port', 10110) | ||
| buffer_size = rospy.get_param('~buffer_size', 4096) | ||
| timeout = rospy.get_param('~timeout', 2) |
There was a problem hiding this comment.
Please add a units suffix, e.g. if this is in seconds, call it ~timeout_s or ~timeout_sec
scripts/nmea_socket_driver
Outdated
|
|
||
| try: | ||
| # Create a socket | ||
| _socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) |
There was a problem hiding this comment.
Why is the name of _socket prefixed with an underscore? Usually, this indicates a global variable.
There was a problem hiding this comment.
Because there is also a module imported as socket and mixing the two would seriously mess things up :).
I don't know of a naming convention that uses a _ prefix for globals, but there is a convention for postfixing to avoid conflicts with builtins and keyword, so I'll change it to that.
scripts/nmea_socket_driver
Outdated
| data, remote_address = _socket.recvfrom(buffer_size) | ||
| except socket.error, exc: | ||
| rospy.logerr("Caught exception socket.error during recvfrom: %s" % exc) | ||
| break |
There was a problem hiding this comment.
Should this try to reconnect here, rather than breaking out and killing the node?
src/libnmea_navsat_driver/driver.py
Outdated
| self.vel_pub = rospy.Publisher('vel', TwistStamped, queue_size=1) | ||
| self.time_ref_pub = rospy.Publisher('time_reference', TimeReference, queue_size=1) | ||
| self.vel_pub = rospy.Publisher('fix_velocity', TwistStamped, queue_size=1) | ||
| self.time_ref_pub = rospy.Publisher('time', TimeReference, queue_size=1) |
There was a problem hiding this comment.
Unfortunately, these two API changes would break users' existing systems. Please keep the ROS API the same so we can release your new functionality without breaking existing functionality.
src/libnmea_navsat_driver/driver.py
Outdated
| return "%s/%s" % (prefix, frame_id) | ||
| else: | ||
| return frame_id | ||
| return frame_id |
src/libnmea_navsat_driver/parser.py
Outdated
| @@ -126,7 +126,8 @@ def convert_deg_to_rads(degs): | |||
|
|
|||
| def parse_nmea_sentence(nmea_sentence): | |||
| # Check for a valid nmea sentence | |||
| if not re.match('(^\$GP|^\$GN|^\$GL).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | |||
|
|
|||
| if not re.match('^\$(GP|GN|GL|GA|II|IN|GB|BD|QZ).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | |||
There was a problem hiding this comment.
Is this change related to your new node? It seems like it might be a good addition, but it's not necessary for the new node. Please remove it from this PR and put it in its own PR if you'd like it to be considered.
|
Also please add the new node to the list of installed scripts in the CMakeLists.txt file. |
|
Reviews processed. Please note that this PR is building on an older PR (not by me) that had some unprocessed review comments, so not all the code in this PR is mine. |
src/libnmea_navsat_driver/parser.py
Outdated
| @@ -126,7 +126,8 @@ def convert_deg_to_rads(degs): | |||
|
|
|||
| def parse_nmea_sentence(nmea_sentence): | |||
| # Check for a valid nmea sentence | |||
| if not re.match('(^\$GP|^\$GN|^\$GL).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | |||
|
|
|||
| if not re.match('^\$(GP|GN).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | |||
There was a problem hiding this comment.
It looks like this is a regression, because the parser would no longer support $GL* messages.
|
Done. There is some git archeology in 92d156c concerning that regression. |
There was a problem hiding this comment.
Thanks for your contribution. I've tested this out by writing a quick UDP forwarder for my serial device, and this node seems to work.
I'll merge this to master. Because it doesn't break any existing APIs or add any new dependencies, I think it's a good candidate to be backported to existing releases.
|
Could the UDP forwarder be added as well, as a test tool? |
Add a NMEA socket driver node, which is like the existing serial driver node, but instead of attaching to a TTY handle from a serial port, it listens to a UDP port for NMEA sentences.
This PR addresses/fixes comments on #16