Rewrite nmea_socket_driver to use serversocket #92
Conversation
There was a problem hiding this comment.
I think this is a much cleaner implementation than the previous socket server. I haven't had a chance to test it completely, but I have a few preliminary comments.
Regarding Windows--this package doesn't officially support Windows, and I've never tested it on Windows. It's pure Python, so presumably it's one of the easier ROS 1 packages to use in Windows, but I'm not concerned if this breaks Windows support, since any Windows support was luck in the first place.
We do need to maintain Python 2.7 support in ROS Melodic unfortunately.
| "```\n" + | ||
| repr(line) + "\n\n" + | ||
| traceback.format_exc() + | ||
| "```") |
There was a problem hiding this comment.
I suggest you use format() here, i.e.:
rospy.logwarn("ValueError, likely due to missing fields in the NMEA "
"message. Please report this issue at "
"https://github.com/ros-drivers/nmea_navsat_driver"
", including the following:\n\n```\n{0}\n\n{1}```"
.format(repr(line), traceback.format_exc()))
|
Hi @evenator, did you have time to give this a shake? |
There was a problem hiding this comment.
Sorry I took so long to test this out.
I've tested it, and there don't seem to be any regressions. I'll merge this to master, and I think it can be included in the next release to existing supported ROS distributions because there are no breaking changes to the API. Thanks for your contribution!
|
@evenator Do you know when the next release will happen (or happened)? |
Use Python's SocketServer rather than low level socket APIs. This simplifies code and makes it easier to add TCP support in the future. The buffer_size parameter is no longer necessary because this is an internal detail of UDPServer.
Use Python's SocketServer rather than low level socket APIs. This simplifies code and makes it easier to add TCP support in the future. The buffer_size parameter is no longer necessary because this is an internal detail of UDPServer.
|
@rgov I backported this PR (and several others) to ROS Melodic in #95 and just opened PR ros/rosdistro#23858 to make a new release. |
This is deviating a little from #90. I recommend testing on Windows since
select()behaves slightly differently there, and on Python 2.7 if that is still supported.Rather than using low level socket APIs, this uses the Python SocketServer module. This simplifies the code by several lines and makes it easier to support TCP packets in the future.
The
buffer_sizeparameter is no longer necessary because this is an internal detail ofUDPServer.