Fixed badly set bitrate causing sweep object creation to hang on Linux #117
Conversation
libsweep/src/unix/serial.cc
Outdated
| @@ -26,6 +26,15 @@ static speed_t get_baud(int32_t bitrate) { | |||
| throw error{"Only baud rate 115200 is supported at this time."}; | |||
| return -1; | |||
| } | |||
|
|
|||
| // translate human readable bitrate to termios bitrate | |||
| if (bitrate == 115200) | |||
There was a problem hiding this comment.
This conditional branch is redundant given the if (bitrate != 115200) above, which results in an error. Maybe just
// if defined, translate human readable bitrate to termios bitrate
#ifdef B115200
return B115200
#endif
return bitrate;There was a problem hiding this comment.
Well ok, I was supposing that more bitrates would be supported in the future.
libsweep/src/unix/serial.cc
Outdated
| // translate human readable bitrate to termios bitrate | ||
| if (bitrate == 115200) | ||
| { | ||
| #ifdef B115200 |
There was a problem hiding this comment.
Could you clang format as well. Format file included in the main repo. If not, let me know and I'll clean it up before merging.
There was a problem hiding this comment.
I'm not use to it but I can figure out how it works
|
Thanks for this second PR. Even if a more robust and elegant portability fix can't be resolved just yet, the library is currently broken.... so I'd like to get this merged ASAP!! |
|
Done |
|
I'll do one more final check on mac and get this merged! Thanks so much. |
|
Urgh thanks for the deep dive here and fixing the bug. I think we hit this last week when playing with Haskell bindings in #115 but didn't look further (since struggling with Haskell was already enough for the evening 😅). |
| @@ -148,7 +153,7 @@ void device_read(device_s serial, void* to, int32_t len) { | |||
| } else { | |||
| throw error{"reading from serial device failed"}; | |||
| } | |||
| } else if(ret == 0){ | |||
| } else if (ret == 0) { | |||
There was a problem hiding this comment.
I think this should check for feof (?)
| // translate human readable bitrate to termios bitrate | ||
| #ifdef B115200 | ||
| return B115200; | ||
| #endif |
There was a problem hiding this comment.
This is a good quick-fix but we should refactor the function (or how we allow for bitrates here).
We take a bitrate argument but in case we hit the branch here the argument is never used.
|
The first fix I made was actually including a condition testing the bitrate before setting a constant, but this one was not merged. |
This bug was causing sweep object creation to hang when trying to read the returned value from serial port. ==> Issue #72
Bitrate of the serial communication was badly set during the creation of the sweep object when using the libsweep library on Linux.
cfsetispeed(...) and cfsetospeed(...) both take a termios bitrate constant and not the bitrate directly. So should the get_baud(115200) call return the B115200 value defined in termios.h.
The fix seems to have a limitation when compiling on some MAC OS X computers. Termios constants like B115200 doesn't seem to be always defined. That's why the code uses a #ifdef as a workaround for MAC.
Thanks.