-
Notifications
You must be signed in to change notification settings - Fork 525
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
WIP: Explicit session lifecycle for the serial server. #228
Conversation
@@ -45,81 +45,63 @@ | |||
namespace rosserial_server | |||
{ | |||
|
|||
class SerialSession : public Session<boost::asio::serial_port> | |||
class SerialSession | |||
{ | |||
public: | |||
SerialSession(boost::asio::io_service& io_service, std::string port, int baud) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realistically, this is no longer a "Session", it's now a session manager. However, for the sake of not breaking API for other nodes which embed rosserial_server, it will remain as it is.
5f8525a
to
1eb322b
Compare
Despite the large size of this, I don't think it's a terribly risky change, and resolves some significant crashes for us. Going to merge to jade-devel as-is, and then bloom for Kinetic. If there are no reported issues, then Jade will be bloomed eventually too. |
if (client_version == PROTOCOL_VER1) { | ||
msg_checksum += checksum(stream.getLength() - 1); | ||
} | ||
// msg_checksum += checksum(stream.getLength() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
1eb322b
to
aaa1d7c
Compare
* io_service thread to avoid a concurrency nightmare. | ||
*/ | ||
void ros_spin_timeout(const boost::system::error_code& error) { | ||
ros_callback_queue_.callAvailable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the kernel of the change— periodically calling ROS callbacks on the ASIO thread, rather than from a separate one.
This is a long overdue change which will resolve some crashes when USB serial devices return error states in the face or noise or other interruptions. As part of this change, the support for VER1 protocol (pre-Hydro) has been dropped, to simplify the overall flow.
aaa1d7c
to
53332ec
Compare
Note that this change is fully backward compatible— the API for "embedding" rosserial_server is unchanged, it's simply that running |
LGTM other than previous fix-it comments. |
This is a long overdue change which will resolve some crashes when USB serial devices return error states in the face or noise or other interruptions.
This is a work in progress— once support for the socket server is in place, this will represent a resolution to #172.
FYI: @mitchellwills @mikeodr