Re: Contribution: Serial port access

[Eli Zaretskii]

> From: Daniel Engeler <address@hidden>
> 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 @address@hidden 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.