From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Contribution: Serial port access Date: Tue, 22 Apr 2008 20:45:49 +0300 Message-ID: References: Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: ger.gmane.org 1208886433 19578 80.91.229.12 (22 Apr 2008 17:47:13 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 22 Apr 2008 17:47:13 +0000 (UTC) Cc: emacs-devel@gnu.org To: Daniel Engeler Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Apr 22 19:47:45 2008 connect(): Connection refused Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1JoMZt-0001Gs-0z for ged-emacs-devel@m.gmane.org; Tue, 22 Apr 2008 19:46:49 +0200 Original-Received: from localhost ([127.0.0.1] helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JoMZD-0000Ch-FF for ged-emacs-devel@m.gmane.org; Tue, 22 Apr 2008 13:46:07 -0400 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JoMZA-0000CQ-1f for emacs-devel@gnu.org; Tue, 22 Apr 2008 13:46:04 -0400 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JoMZ9-0000C8-8v for emacs-devel@gnu.org; Tue, 22 Apr 2008 13:46:03 -0400 Original-Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JoMZ9-0000C2-5h for emacs-devel@gnu.org; Tue, 22 Apr 2008 13:46:03 -0400 Original-Received: from mtaout2.012.net.il ([84.95.2.4]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JoMZ8-000605-DX for emacs-devel@gnu.org; Tue, 22 Apr 2008 13:46:02 -0400 Original-Received: from HOME-C4E4A596F7 ([83.130.1.82]) by i_mtaout2.012.net.il (HyperSendmail v2004.12) with ESMTPA id <0JZQ00272NC03IG5@i_mtaout2.012.net.il> for emacs-devel@gnu.org; Tue, 22 Apr 2008 21:00:11 +0300 (IDT) In-reply-to: X-012-Sender: halo1@inter.net.il X-detected-kernel: by monty-python.gnu.org: Solaris 9.1 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:95792 Archived-At: > From: Daniel Engeler > Date: Mon, 21 Apr 2008 22:36:38 +0200 > > this is my first contribution to Emacs: Serial port access for GNU/ > Linux, Unix, and Windows. Thanks! This is a large contribution, so you will need to sign legal papers for it to be accepted. If you didn't sign them already (I don't see your assignment on file, but maybe you did it very recently), please contact Stefan or Yidong for the details. > All changes are fully documented, in every function and in the > documentation. Thanks. One thing that bothers me is that your changes include quite a few of unrelated fixes, such as typos in the manual and in the comments to existing code. Could you please separate these into another series of patches, to avoid confusion? > + @cindex serial terminal > + @findex serial-term Please avoid having several index entries that begin with the same substring and point to the same place in the manual. They bloat the index without making it any more useful. In this case, I'd change the first index entry into @cindex terminal, serial > + A serial port can be configured even more by clicking on ``8N1'' in > + the mode line. This probably requires related changes in the section that describes the mode line, as you are now introducing a new indicator there, right? > + @item type > + Symbol indicating the type of process: real, network, serial The 3 possible values should be in @code, as they are Lisp symbols. > + @cindex /dev/tty /dev/tty is a file, so it should have the @file markup. > + @cindex COM1 Same here. > + The serial port can be configured at run-time, without having to > + close and re-open it. The function @code{serial-process-configure} ^^ Please make sure you always leave 2 blanks after a period that ends a sentence. > + be "/dev/ttyS0" on Unix. On Windows, this could be "COM1", or > + "\\.\COM10" (double the quotes in strings). You mean "double the backslashes", right? > + SPEED is the speed of the serial port in bits per second. 9600 is a This should be @var{speed}, and no need to capitalize. > + @table @asis > + @item :port @var{port} @asis is not a good idea when your @item's are symbols. Please use @code or @samp. > + @var{PORT} (mandatory) is the path or name of the serial port. For ^^^^^^^^^^ No need to capitalize things inside @var, it looks ugly in print. > + example, this could be "/dev/ttyS0" on Unix. On Windows, this could > + be "COM1", or "\\.\COM10" for ports higher than COM9 (double the Please use @file instead of quoting the port names. > + (@var{decoding} . @var{encoding}), @var{decoding} is used for reading, Please put the cons cell in @code, as it is a Lisp expression. > + @item :speed, :bytesize, :parity, :stopbits, :flowcontrol Please name these one per line, and please use @itemx for all but the first one. And no need for the commas. > + Examples: > + > + (make-serial-process :port "/dev/ttyS0" :speed 9600) > + > + (make-serial-process :port "COM1" :speed 115200 :stopbits 2) > + > + (make-serial-process :port "\\\\.\\COM13" :speed 1200 :bytesize 7 :parity 'odd) > + > + (make-serial-process :port "/dev/tty.BlueConsole-SPP-1" :speed nil) These examples should be in an @example..@end example block. > + @table @asis > + @item :process @var{process}, :name @var{name}, :buffer @var{buffer}, :port @var{port} Again @code is better than @asis here. And also please use @item/@itemx to specify each argument on its own line. > + @code{nil}, the serial port is not configured any further, i.e. all Either put a comma after "i.e." or use @:, to tell TeX that this period does not end a sentence. > + Examples: > + > + (serial-process-configure :process "/dev/ttyS0" :speed 1200) > + > + (serial-process-configure > + :buffer "COM1" :stopbits 1 :parity 'odd :flowcontrol 'hw) > + > + (serial-process-configure :port "\\\\.\\COM13" :bytesize 7) Please use @example. > + (defun serial-port-is-file-p () > + "Guess whether serial ports are files on this system. > + Return t if this is a Unix-based system, where serial ports are > + files, such as /dev/ttyS0. > + Return nil if this is Windows or DOS, where serial ports have > + special identifiers such as COM1." > + (not (member system-type (list 'windows-nt 'ms-dos)))) I think you need to add `cygwin' to this list. Not that I understand completely why you needed this distinction in the first place. Is it because trying to access a non-existing port on Windows will hang Emacs, or is there any other reason? > + sprintf (tembuf, "(serial port %s", > + STRINGP (port) ? (char *) SDATA (port) : "?"); I'm not sure tembuf[] is large enough to hold an arbitrary file name, so this sprintf is unsafe. > + #ifdef WINDOWSNT > + #define SERIAL_PROCESS_CONFIGURE_WINDOWSNT > + #else /* not WINDOWSNT */ This kind of ugliness should go into src/s/ms-w32.h, no need to have it here. > + #ifdef SERIAL_PROCESS_CONFIGURE_WINDOWSNT > + HANDLE hnd; > + DCB dcb; > + COMMTIMEOUTS ct; > + #else > + struct termios attr; > + #endif The declaration in the #else clause will not compile cleanly (or not at all, depending on the compiler) if termios is unavailable. > + #ifndef SERIAL_PROCESS_CONFIGURE_WINDOWSNT > + #ifndef SERIAL_PROCESS_CONFIGURE_HAVE_TERMIOS > + error ("Cannot configure serial port"); > + #endif > + #endif I think the error message should better say something like "serial port configuration is not supported on this platform". Or maybe simply make the Lisp binding unavailable in such cases, so the function could never be invoked on those platforms. > + #ifdef SERIAL_PROCESS_CONFIGURE_WINDOWSNT > + /* Initialise timeouts for blocking read and blocking write. */ > + if (!GetCommTimeouts (hnd, &ct)) > + error ("GetCommTimeouts() failed"); > + ct.ReadIntervalTimeout = 0; > + ct.ReadTotalTimeoutMultiplier = 0; > + ct.ReadTotalTimeoutConstant = 0; > + ct.WriteTotalTimeoutMultiplier = 0; > + ct.WriteTotalTimeoutConstant = 0; > + if (!SetCommTimeouts (hnd, &ct)) > + error ("SetCommTimeouts() failed"); > + > + memset (&dcb, 0, sizeof (dcb)); > + dcb.DCBlength = sizeof (DCB); > + if (!GetCommState (hnd, &dcb)) > + error ("GetCommState() failed"); > + dcb.fBinary = TRUE; > + dcb.fNull = FALSE; > + dcb.fAbortOnError = FALSE; > + /* dcb.XonLim and dcb.XoffLim are set by GetCommState() */ > + dcb.ErrorChar = 0; > + dcb.EofChar = 0; > + dcb.EvtChar = 0; > + #else /* SERIAL_PROCESS_CONFIGURE_WINDOWSNT */ > + err = tcgetattr (p->outfd, &attr); This kind of system-dependent stuff should be isolated into a function, and each platform should implement its own version in a source file that is compiled only on that platform. > + #ifdef WINDOWSNT > + init_winsock (TRUE); > + #endif Why is this needed? We don't need Winsock for serial comms, do we? > + #ifdef WINDOWSNT > + hnd = CreateFile ((char*) SDATA (port), GENERIC_READ | GENERIC_WRITE, 0, > + 0, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0); > + if (hnd == INVALID_HANDLE_VALUE) > + error ("Could not open %s", (char*) SDATA (port)); > + fd = (int) _open_osfhandle ((int) hnd, 0); > + if (fd == -1) > + error ("Could not open %s", (char*) SDATA (port)); > + #else /* not WINDOWSNT */ > + fd = emacs_open ((char*) SDATA (port), > + O_RDWR Again, this should be a function with 2 different implementations, and the Windows implementation should be on w32.c, not here. Thanks again for working on this.