Please review: Patch for non-Linux serial set_baud_rate()
Hi, I happened to stumble across some code which didn't look quite right in dlls/ntdll/serial.c. However, I have no expertise in serial comms or termios. Also, I have no means to test the change I propose. On the other hand, the change seems clearly right and conforms to what I learn from man pages, etc. So, I'm hoping somebody who is more familiar with this functionality can review the attached patch before I formally submit it. Another approach that seems better from my reading of the man pages is to switch to using cfsetspeed() [or cfsetospeed() and cfsetispeed()] instead of having separate branches based on defined(CBAUD). But maybe that has portability problems worse than the current approach. Thanks, Ken
On Jun 30, 2010, at 11:47 AM, Ken Thomases wrote:
I happened to stumble across some code which didn't look quite right in dlls/ntdll/serial.c. However, I have no expertise in serial comms or termios. Also, I have no means to test the change I propose. On the other hand, the change seems clearly right and conforms to what I learn from man pages, etc.
On re-reading, I realize I didn't say what the nature of the problem and change are: set_baud_rate() needs to poke at a termios structure and enter the desired baud rate into its fields. However, the baud rate is represented differently on Linux than on other platforms. This is detected with an "#ifdef CBAUD" test. On Linux, the baud rate goes into the c_cflag field; elsewhere, it goes into dedicated c_ospeed and c_ispeed fields. At some point, somebody extended the non-Linux branch to support higher baud rates, to match capability that the Linux branch had. However, they seem to have just copied the Linux-appropriate code and pasted it into the non-Linux branch. So, the code is stuffing the baud rate into the c_cflag field and not the c_ospeed field. In the non-Linux branch, the high baud rate cases of the switch statement don't match the lower baud rate cases. My proposed change sets the baud rate via the c_ospeed field for all cases on the non-Linux branch. Cheers, Ken
Le 30/06/2010 20:26, Ken Thomases a écrit :
On Jun 30, 2010, at 11:47 AM, Ken Thomases wrote:
I happened to stumble across some code which didn't look quite right in dlls/ntdll/serial.c. However, I have no expertise in serial comms or termios. Also, I have no means to test the change I propose. On the other hand, the change seems clearly right and conforms to what I learn from man pages, etc.
On re-reading, I realize I didn't say what the nature of the problem and change are:
set_baud_rate() needs to poke at a termios structure and enter the desired baud rate into its fields. However, the baud rate is represented differently on Linux than on other platforms. This is detected with an "#ifdef CBAUD" test. On Linux, the baud rate goes into the c_cflag field; elsewhere, it goes into dedicated c_ospeed and c_ispeed fields.
At some point, somebody extended the non-Linux branch to support higher baud rates, to match capability that the Linux branch had. However, they seem to have just copied the Linux-appropriate code and pasted it into the non-Linux branch. So, the code is stuffing the baud rate into the c_cflag field and not the c_ospeed field. In the non-Linux branch, the high baud rate cases of the switch statement don't match the lower baud rate cases.
My proposed change sets the baud rate via the c_ospeed field for all cases on the non-Linux branch.
Cheers, Ken
looks ok to me A+ -- Eric Pouech "The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)
participants (2)
-
Eric Pouech -
Ken Thomases