Serial port library for unix systems in C using callback model

Question

I wrote a blocking serial port library, Blocking serial port C library, but prefer non-blocking so wrote this library where user can specify callbacks. So it is event driven.

The code assumes C99 or better. How could this be improved?

The example is fairly custom to working in the UK with the British Telecom Caller ID system: https://www.btplc.com/SINet/sins/pdf/242v2p5.pdf

serial_port.h:

/* Serial port library for unix platform.
   Uses unix select method and callbacks are used to notify events.
   Events supported are connection and reads.
   User must provide connected and/or read callback function(s) to be
   notified of these events.
   Author: Angus Comber
*/
#ifndef SERIAL_PORT_H_
#define SERIAL_PORT_H_

#define READ_BUFFERSIZE 100

#include <stdlib.h>  // size_t

/* opaque serial port object */
struct serial_port;
typedef struct serial_port serial_port_t;

// user defined callbacks
typedef void (*connect_callback)(serial_port_t* port, void* userdata);
typedef void (*read_callback)(serial_port_t* port, char* buffer, int* length, void* userdata);

/* port_start opens portname at baudrate.
   Returns immediately with -1 error code on failure to open port. Interrogate 
   errno for more information. Alternatively, if unable to allocate memory for
   the port object returns -2 error.
   if the port setup proceeds successfully, port_start blocks until the process is closed.
   Arguments:
    - portname - port identifier for modem
    - baudrate - speed for connection
    - connectcb - user defined callback on a successful connection being established
    - readcb - user defined callback on reading data on port
    - userdata - user defined data to pass to callback functions (can be NULL)
*/
int port_start(const char* portname, const int baudrate, connect_callback connectcb, read_callback readcb, void* userdata);

/* returns characters written or -1 on error */
int port_write(serial_port_t* port, const char* data, size_t length);
/* returns characters read or -1 on error */
int port_read(serial_port_t* port, char* buffer, int buffer_size);
/* close port and free allocated resources */
int port_close(serial_port_t* port);

#endif // SERIAL_PORT_H_

serial_port.c:

/* serial port class for unix platform using callbacks */
#include "serial_port.h"

#include <unistd.h>  // posix api
#include <fcntl.h>   // file control operations
#include <termios.h> // terminal

struct serial_port {
  int fd;
  int baudrate;
  struct termios restore_tty;
  struct termios current_tty;
  connect_callback connectcb;
  read_callback readcb;
  void* userdata;
};

static int set_speed(struct termios tty, int speed) {
    speed_t sp;
    switch(speed) {
        case 1200: sp = B1200; break;
        case 1800: sp = B1800; break;
        case 2400: sp = B2400; break;
        case 4800: sp = B4800; break;
        case 9600: sp = B9600; break;
        case 19200: sp = B19200; break;
        case 38400: sp = B38400; break;
        case 57600: sp = B57600; break;
        case 115200: sp = B115200; break;
        default: return -1; // unsupported
    }

    int baudset = cfsetospeed(&tty, sp);
    baudset = cfsetispeed(&tty, sp);
    return baudset;
}

static int get_term(int fd, struct termios* ptty) {
    /* Upon successful completion, 0 shall be returned. Otherwise, -1
     shall be returned and errno set to indicate the error. */
    return tcgetattr(fd, ptty);
}

/* configure tty */
static int set_terminal(int fd, struct termios* ptty) {
    ptty->c_cflag |= (CLOCAL | CREAD);    /* ignore modem controls */
    ptty->c_cflag &= ~CSIZE;
    ptty->c_cflag |= CS8;         /* 8-bit characters */
    ptty->c_cflag &= ~PARENB;     /* no parity bit */
    ptty->c_cflag &= ~CSTOPB;     /* only need 1 stop bit */
    ptty->c_cflag &= ~CRTSCTS;    /* no hardware flowcontrol */

    /* setup for non-canonical mode */
    ptty->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR | IGNCR | ICRNL | IXON);
    ptty->c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
    ptty->c_oflag &= ~OPOST;

    /* fetch bytes as they become available */
    ptty->c_cc[VMIN] = 1;
    ptty->c_cc[VTIME] = 1;

    if (tcsetattr(fd, TCSANOW, ptty) != 0) {
        // Error from tcsetattr- use strerror(errno)
        return -1;
    }
    return 0;
}

int port_start(const char* portname, const int baudrate, connect_callback connectcb, read_callback readcb, void* userdata) {
    // open in non-blocking mode and notify as reads and write complete via callbacks
    int fd = open(portname, O_RDWR | O_NOCTTY | O_NONBLOCK);
    if (fd == -1) {
        return -1;
    }
    serial_port_t* port = malloc(sizeof(serial_port_t));
    if (port) {
        port->fd = fd;
        port->baudrate = baudrate;
        port->connectcb = connectcb;
        port->readcb = readcb;
        port->userdata = userdata;

        // cache prior terminal settings for restore later
        if(get_term(port->fd, &port->current_tty) == 0) {
        // cache previous tty settings
            port->restore_tty = port->current_tty;
        } else {
            return -1;
        }
        // set port speed
        if(set_speed(port->current_tty, port->baudrate) == -1) {
            // error setting tty speed
            return -1;
        }
        // set terminal attribs
        if(set_terminal(port->fd, &port->current_tty) < 0) {
            // error configuring port
            return -1;
        }
    } else {
        return -2;
    }

    if(port->connectcb) {
        connectcb(port, userdata);
    }

    fd_set readset;

    // keep looping forever
    while(1 == 1) {
        FD_ZERO(&readset); /* clear the set */
        FD_SET(fd, &readset); /* add our file descriptor to the set */

        // wait for data to be read (infinite timeout)
        int rv = select(fd + 1, &readset, NULL, NULL, NULL);
        if(rv > 0) {  // we have activity
            // only working with one fd so no need for for loop
            for (int i = fd; i < fd + 1; ++i) {
                if(FD_ISSET(fd, &readset)) {
                    if(readcb) {
                        char data[READ_BUFFERSIZE];
                        int len = READ_BUFFERSIZE;
                        int bytes_read = port_read(port, data, len);
                        readcb(port, data, &bytes_read, userdata);
                    }
            }
        }
        // if an error occurred, exit
        } else {
            return -1;
        }
    }  // while
}

int port_write(serial_port_t* port, const char* data, size_t length) {
    return write(port->fd, data, length);
}

int port_read(serial_port_t* port, char* buffer, int buffer_size) {
    return read(port->fd, buffer, buffer_size);
}

int port_close(serial_port_t* port) {
    if (tcsetattr(port->fd, TCSANOW, &port->restore_tty) < 0) {
        // error restoring attributes
    }
    int result = close(port->fd);
    free(port);
    return result;
}

main.c:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>  // strerror
#include <errno.h>   // errno
#include <ctype.h>   // isprint

#include "serial_port.h"

enum stage { RESET, SETCOUNTRY, SETCALLERID, WAITFORCALL };

/* getline_string function args:
   buffer - buffer to fill with line
   size   - ptr to size of line or -1 if no line extracted
   source - source bytes
   end    - end of byte stream
   returns remaining string stream or NULL if at end of string stream
*/
static const char* getline_stringstream(char* line, int* size, const char* source, const char* end) {
    const char* s = source;
    char* t = line;
    *size = 0;

    // eat any prepended newlines
    while (s != end && *s && (*s == '\r' || *s == '\n'))
        s++;

    while (s != end && *s && *s != '\r' && *s != '\n') {
        *t++ = *s++;
        (*size)++;
    }
    *t = '\0';  // null terminate string
    return *size > 0 ? s : NULL;
}

/* on successful connection we print report header */
void connected(serial_port_t* port, void* userdata) {
    printf("%-6s%-6s%s\n", "Date", "Time", "Caller ID");
    port_write(port, "AT\r\n", 4);
}

/* use simple state machine to setup modem and when finished, output caller ID */
void readdata(serial_port_t* port, char* buffer, int* length, void* userdata) {
  enum stage* step = userdata;
  switch(*step) {
  case RESET:
    port_write(port, "ATZ\r\n", 5);  // reset modem
    *step = SETCOUNTRY;
    break;
  case SETCOUNTRY:
    port_write(port, "AT+GCI=B4\r\n", 11);  // set country to UK
   *step = SETCALLERID;
  break;
  case SETCALLERID:
    port_write(port, "AT+VCID=1\r\n", 11);  // enables formatted caller report
  *step = WAITFORCALL;
  break;
  case WAITFORCALL:
    {
      //parse call data in to caller id etc
      static char callerid[50] = {0};
      static char caller_date[20] = {0};
      static char caller_time[20] = {0};

      int size;
      char line[100];  // we assume no lines > 100 chars
      const char* rest = buffer;
      char* end = buffer + *length;
      while ((rest = getline_stringstream(line, &size, rest, end)) != NULL) {
          if(strncmp(line, "NMBR", 4) == 0 && strlen(line) > 7) {
              strcpy(callerid, &line[7]);
          }
          if(strncmp(line, "DATE", 4) == 0 && strlen(line) >= 11) {
              sprintf(caller_date, "%c%c/%c%c", line[9], line[10], line[7], line[8]);
          }
          if(strncmp(line, "TIME", 4) == 0 && strlen(line) >= 11) {
              sprintf(caller_time, "%c%c:%c%c", line[7], line[8], line[9], line[10]);
          }
      }
      if(strlen(callerid) > 0 && strlen(caller_date) > 0 && strlen(caller_time) > 0) {
          printf("%-6s%-6s%s\n", caller_date, caller_time, callerid);
          callerid[0] = caller_date[0] = caller_time[0] = '\0';
      }
    }
  break;
  default:
    fprintf(stderr, "something unexpected happened, aborting...\n");
    fflush(stdout);
    exit(1);  // something bad happened - exiting
  }
}

// test serial library with modem
int main(int argc, char* argv[]) {

    if(argc != 3) {
        printf("Usage: %s <port name> <baudrate>\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    const char* portname = argv[1];
    const int baudrate = atoi(argv[2]);
    enum stage step = RESET;

    // program will block here forever
    if(port_start(portname, baudrate, connected, readdata, &step) == -1) {
        fprintf(stderr, "An error occurred starting serial port connection on %s, error: %s\n", portname, strerror(errno));
    }
}

Example output below.

acomber@mail:~/Documents/projects/modem/serial/serial_port_select_lib$ sudo ./prog /dev/ttyACM0 9600
Date  Time  Caller ID
08/07 10:43 07766112233
08/07 10:47 P
08/07 10:48 07766123123

In the output below, the 2nd call at 10:47, had caller id hidden and so the character P is the string to indicate PRIVATE.

Answer 1

typedef _t

POSIX reserves identifiers ending in _t. You should maybe use _s (s for struct) for the typedef:

struct Foobar {
    void    *foo;
    int32_t bar;
};
typedef struct Foobar foobar_s;

For your typedefs for function types, I would use _f (f for function) to help easily differentiate the type from a normal identifier.

Or not use typedef at all :)

---

encapsulate into a function

The following code deserves a function:

    // eat any prepended newlines
    while (s != end && *s && (*s == '\r' || *s == '\n'))
        s++;

The following too:

  case WAITFORCALL:
    {
      //parse call data in to caller id etc
      static char callerid[50] = {0};
      static char caller_date[20] = {0};
      static char caller_time[20] = {0};

      int size;
      char line[100];  // we assume no lines > 100 chars
      const char* rest = buffer;
      char* end = buffer + *length;
      while ((rest = getline_stringstream(line, &size, rest, end)) != NULL) {
          if(strncmp(line, "NMBR", 4) == 0 && strlen(line) > 7) {
              strcpy(callerid, &line[7]);
          }
          if(strncmp(line, "DATE", 4) == 0 && strlen(line) >= 11) {
              sprintf(caller_date, "%c%c/%c%c", line[9], line[10], line[7], line[8]);
          }
          if(strncmp(line, "TIME", 4) == 0 && strlen(line) >= 11) {
              sprintf(caller_time, "%c%c:%c%c", line[7], line[8], line[9], line[10]);
          }
      }
      if(strlen(callerid) > 0 && strlen(caller_date) > 0 && strlen(caller_time) > 0) {
          printf("%-6s%-6s%s\n", caller_date, caller_time, callerid);
          callerid[0] = caller_date[0] = caller_time[0] = '\0';
      }
    }
  break;

---

Safe usage of malloc

Malloc is easily misused.

• sizeof(type) vs sizeof(*foo):

foo = malloc(sizeof(*foo) * nmemb); is better because if you ever change the type of foo, this call will still be valid, while if not, you would have to change every line where malloc is called with foo. If you forget any of those lines, good luck.

---

Error checking:

// set port speed
if (set_speed(port->current_tty, port->baudrate) == -1) {
        // error setting tty speed
        return -1;
}

This is unsafe: if you ever add another error code to set_speed, and forget to update this line, good luck!

Solution: check for any non-zero values:

// set port speed
if (set_speed(port->current_tty, port->baudrate)) {
        // error setting tty speed
        return -1;
}

---

fflush

fprintf(stderr, "something unexpected happened, aborting...\n");
fflush(stdout);
exit(1);  // something bad happened - exiting

This doesn't make any sense:

• when the program exits, stdout is automatically fflushed (actually, all streams, I think).
• If you wanted to say fflush(stderr); it's not needed: stderr is either unbuffered or line-buffered.

---

EXIT_FAILURE

Use exit(EXIT_FAILURE); when you don't want a specific error code.

---

const non-pointer parameters (just no)

int port_start(const char* portname, const int baudrate, connect_callback connectcb, read_callback readcb, void* userdata);

const int baudrate adds clutter to the code, and absolutely no difference to the user of the function. A function can't modify its arguments. Only pointers can point to modifiable data, and therefore need the const qualifier if they don't modify the data pointed to.

Use int baudrate instead.

---

inline

This is a very good candidate for inline:

int port_write(serial_port_t* port, const char* data, size_t length) {
    return write(port->fd, data, length);
}

It provides a free optimization with no downsides for one line functions (unless you want to keep its contents secret):

serial_port.h:

inline
int     port_write(struct Serial_Port* port, const char* data, size_t length);

inline
int     port_write(struct Serial_Port* port, const char* data, size_t length)
{
        return write(port->fd, data, length);
}

serial_port.c:

extern
int     port_write(struct Serial_Port* port, const char* data, size_t length);