scanse / sweep-sdk

Sweep SDK
MIT License
90 stars 85 forks source link

Unplug exception #103

Closed sevenbitbyte closed 7 years ago

sevenbitbyte commented 7 years ago

Issues

When the cable becomes unplugged the device_read function goes into an infinite loop causing the caller to hang and the process to consume 100% CPU cycles. This failure mode can be more gracefully handled by detecting EOF.

Scope of changes

Modified Linux serial support to throw an exception if an EOF condition is detected.

Known Limitations

The error handling facilities are not very graceful and if the caller does not handle the exception, it result in a crash in upstream programs which may not have previously been expected(but neither was a hang). I have not yet been able to fully recover the connection after handling the exception but now you atleast get a clear error message.

sevenbitbyte commented 7 years ago

I've code reviewed the windows implementation and believe that this failure mode will result in an exception on windows(nearly same as how my patch works) so no windows patch should be needed. But if someone can verify that would be great.

dcyoung commented 7 years ago

Thanks for the catch! I'm not sure if the perror() is necessary, but otherwise this looks good to me. @daniel-j-h any reason not to merge this ASAP?

I've tested on windows, and indeed an exception is thrown... Error: reading from serial device failed.

sevenbitbyte commented 7 years ago

I agree the perror should not be required but from the ROS support layer I was not getting any error messages without doing it and I didn't fully understand your error bubbling approach @dcyoung

sevenbitbyte commented 7 years ago

Is there a way we can get these exception messages to get printed to standard error when used under ROS?

MikeGitb commented 7 years ago

@sevenbitbyte: My mistake. I forgot to upstream a fix for this.

https://github.com/scanse/sweep-sdk/pull/105 should fix this

sevenbitbyte commented 7 years ago

Ah makes sense.

dcyoung commented 7 years ago

Merged! Thanks @sevenbitbyte