Reading from a serial port

Question

I'm receiving data from a serial port in C, using Serial Programming Guide for POSIX Operating Systems as a guide.

The data I receive should be always 10 bytes in length but I want to be sure that, if there is any error (more or less bytes received), reading will clear buffer before the next data arrive, so that there is always proper data in the buffer to be processed. I'm using select to monitor serial file descriptor and local socket. I figured out that I can clean serial buffer in case there was something left from previous transmission, when device is not sending for some period of time.

Question is: is this the right solution?

main.c:

loop_num=0;
si_processed=0;
while(TRUE) {
    /* copy fd_set for select */
    tmp_input=input;

    n = select(max_fd,&tmp_input,NULL,NULL,NULL/*&timeout*/);

    /* See if there was an error */
    if (n<0)
        perror("select failed");
    else {
        /* We have input */
        if (FD_ISSET(serial_fd, &input)) {
            if(!process_serial(serial_fd))
                loop_num++;
        }
        if (FD_ISSET(local_socket, &input))
            process_socket(local_socket);
    }
    if(loop_num>10) {
        /* clear buffer */
        si_processed=0;
        loop_num=0;
    }
    fflush(stdout);
    usleep(20000);
}

serial_port.c

char serial_buffer[256];

int process_serial(int serial_fd) {
    int bytes;
    int n,i;
    char tmp_buffer[32];

    ioctl(serial_fd, FIONREAD, &bytes);
    if(!bytes)
        return 0;

    n=read(serial_fd, tmp_buffer, sizeof(tmp_buffer));
    for(i=0;i<n;i++) {
        serial_buffer[si_processed+i]=tmp_buffer[i];
    }
    si_processed+=n;
    if(si_processed>=INPUT_BYTES_NUM/* defined as 10 */) {
        serial_buffer[si_processed]='\0';
        printf("read:%s\n",serial_buffer);
        si_processed=0;
        fflush(stdout);
    }
    return 1;
}

---

Some more info. I'm querying device every 5 seconds for data, and it should always respond with 10 bytes packets: so data should come fairly quickly into serial buffer, and then a few seconds of silence; sometimes however the device can send data on its own, I don't know when, but also as a 10 bytes packets.

I was doing some test with my program and minicom connected on the other side.

Until I was sending data like

1234567890

1234567890

program was reading what it supposed to read:

1234567890

Problem arises when I send on minicom:

12345678901234567890

Usually what was read was:

1234567890123456

so next, when I wrote on minicom proper data:

1234567890

program was reading

7890123456

and every next read was bad because of leftovers in buffer from previous read.

When I added clearing buffer after 1 or 2 seconds when nothing is read, then I'm sure any leftovers are cleared before next transmission.

I read about this VMIN settings but decided against because of this leftovers.

Also this data 1234567890 is just an example. In fact 10 bytes consist of:

• Byte #1 - always some constant to mark begining of transmission like 0xA5
• Byte #2 through #9 byte - data
• Byte #10 - control sum from all previous bytes.

So after I have 10 bytes packet received I can verify if it has proper data or is somehow corrupted: so if one transmission fails, I should be ok, especially when I can clear the buffer before next data coming in.

Answer 1

The code you posted contains no error-recovery at all. It's a bit off-topic on this site to ask how to implement a new feature (error-recovery); but I'll try.

It's not clear what your communication protocol looks like. It might be:

1. A continuous stream of bytes, to be split into packets of 10
2. 10-byte packets, with a measurable delay between packets, no reply expected
3. 10-byte packets, after which silence until there is a reply, perhaps with a retry if there no reply

I'll assume it isn't 1. because that has little opportunity for error-recovery: if a byte were ever lost then you wouldn't know where the boundary is between "packets", if it's a continuous stream of bytes, unless you use 'framing' and 'escape sequences' in the data.

So I'll assume that it's 2. or 3.. In either case, what's important is:

• There are 10 bytes received (probably received quickly with little or no delay between bytes)
• There is then a delay (no bytes received) after those 10 bytes.

Setting Read Timeouts looks interesting. It may be your best algorithm: set VMIN to say that you want to receive no more or less than exactly 10 bytes when you read.

However that may have a problem:

However, the timeout only applies to the first character read, so if for some reason the driver misses one character inside the N byte packet then the read call could block forever waiting for additional input characters.

You don't say why you want error-recovery after losing a character: perhaps you sometimes get overruns in the driver, or bit parity errors from the serial port?

"Setting Input Parity Options" talks about handling the parity errors:

1. Replace/mark the byte with an error byte

   If you do this then you can be more sure that you will get 10 bytes; but you can only do it if the protocol allows you to recognize an illegal byte value.

2. Or remove the error byte

   If you do this then you must be able to recover from getting 9 bytes sometimes.

The data I receive should be always 10 bytes in length but I want to be sure that, if there is any error (more or less bytes received), reading will clear buffer before the next data arrive, so that there is always proper data in the buffer to be processed.

Reading should empty the buffer, if your sizeof(tmp_buffer) is bigger is the number of bytes queued in the driver. However:

• It depends on the read mode (e.g. a read may block forever instead of clearing the buffer, if VMIN is set to non-zero)
• If there are, somehow, very many received bytes enqueued in the driver, then you may have to read repeatedly until the driver is empty.
• Immediately after you read, there may be more bytes in the driver: for example if the serial port is flow-controlled, reading from the driver allows the device to send again / send more.

Whether you can clear the buffer "before the next data arrive" is difficult to say: I don't know when the next data is supposed to arrive.

Depending on how you handle parity errors, maybe bytes are never lost. If bytes are sometimes lost then you may want to implement logic like:

read_10_bytes:
  read (waiting forever) until some bytes are returned
  if 10 bytes were read then return
  if less than 10 bytes were read, then:
    set an expiry timer
    loop doing the following
  read more bytes
  if you read 10 bytes before the timer expires, then clear the timer and return
  if the timer expires before you read 10 bytes, then discard the bad bytes and return

Instead of actually setting a timer, above, perhaps loop doing a blocking-read-with-timeout (which either reads the bytes, or expires).

As well as handling too few bytes (above), you could modify the above to check for receiving more than 10 bytes: after you receive 10 bytes, do another blocking-read-with-small-timeout to verify that there are no more bytes to receive. If there are extra bytes, then read them all before returning to the 'waiting-for-next-10-bytes' state (otherwise, these extra bytes will mess up your next 10-byte packet).

---

I think there's an obvious bug in the code. After you read 16 bytes, you completely clear your serial_buffer by zeroing si_processed, and you then think "success!" after each next 10 bytes you read.

Instead you should:

• If you read too much data, don't discard the extra bytes: instead use memmove to move them to the beginning of the buffer (assuming they're the start of the next packet), and then read the subsequent bytes into the vacant space after them.

Something like:

// after reading and processing a 10-byte packet
if (si_processed > 10) // already have the start of the next packet
{
     // move start of next packet to start of buffer
     memmove(serial_buffer, serial_buffer+10, si_processed - 10);
     // not 0: remember how many start bytes we already have
     si_processed -= 10;
}

• After you read any bytes, verify whether the start byte is your 0xA5 value, and discard the bytes if not.

Something like:

if (si_processed > 0) // have some bytes
{
    int i;
    for (i = 0; i < si_processed; ++i)
        if (serial_buffer[i] == 0xA5) // found start of packet
            break;
    if (i > 0)
    {
        // start of packet is not start of buffer
        // so discard bad bytes at the start of the buffer
        memmove(serial_buffer, serial_buffer+i, si_processed - i);
        si_processed -= i;
    }
}

Answer 2

You shouldn't need to call usleep() to throttle the loop. If you need usleep() prevent the loop from consuming 100% of the CPU, then something is wrong, since select() is supposed to be the gatekeeper that lets the loop continue when input is available to be processed. Perhaps you are failing to drain the socket or serial port completely in your input handlers, so that select() always returns immediately.

Answer 3

The final version, in case someone was interested.

main.c

  int            local_socket;
  int            serial_fd;
  int            max_fd;
  fd_set         input;
  fd_set         tmp_input;
  char *serial_output_buffer;
  serial_output_buffer=malloc(11 * sizeof(char));

  serial_fd=open_port();
  local_socket=open_local_socket();

  FD_ZERO(&input);
  FD_SET(serial_fd, &input);
  FD_SET(local_socket, &input);
  max_fd = (local_socket > serial_fd ? local_socket : serial_fd) + 1;

  si_processed=0;
  serial_output_buffer[10]='\0';
  while(TRUE) {
    tmp_input=input;

    n = select(max_fd,&tmp_input,NULL,NULL,NULL);

    /* See if there was an error */
    if (n<0)
        perror("select failed");
    else {
        /* We have input */
        if (FD_ISSET(serial_fd, &input)) {
            if(process_serial(serial_fd,serial_output_buffer)) {
                printf("read:%s\n",serial_output_buffer);
                fflush(stdout);
            }
        }
        if (FD_ISSET(local_socket, &input))
            process_socket(local_socket);
    }
    usleep(20000);
}
return 0;

serial_port.h

#ifndef SERIAL_PORT
#define SERIAL_PORT

int open_port();
int process_serial(int serial_fd,char *output);

int si_processed;

#endif

serial_port.c

char serial_buffer[256];


int process_serial(int serial_fd,char *output) {
   int bytes;
   int n,i;
   char tmp_buffer[256];

   ioctl(serial_fd, FIONREAD, &bytes);
   if(!bytes && si_processed<INPUT_BYTES_NUM) //proceed if data still in buffer
       return 0;

   n=read(serial_fd, tmp_buffer, sizeof(tmp_buffer));
   for(i=0;i<n;i++) {
      serial_buffer[si_processed+i]=tmp_buffer[i];
   }
   si_processed+=n;
   if(si_processed>=INPUT_BYTES_NUM) {
      for (i = 0; i < si_processed; ++i)
          if (serial_buffer[i] == '1') // found start of packet
                break;
      if (i > 0) {
        // start of packet is not start of buffer
        // so discard bad bytes at the start of the buffer

        memmove(serial_buffer, serial_buffer+i, si_processed - i);
        si_processed -= i;
      }
      if(si_processed>=INPUT_BYTES_NUM) {
        memmove(output, serial_buffer, 10);

        //move what left to the beginning
        memmove(serial_buffer,serial_buffer+10,si_processed-10);
        si_processed -= 10;
        return 1;
      }
   }
return 0;
}

int open_port(void) {
  int fd; /* File descriptor for the port */
  struct termios options;

  fd = open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY);

  if (fd == -1) {
  /*
  * Could not open the port.
  */
      error_exit("open_port: Unable to open /dev/ttyS0");
  }
  else
    fcntl(fd, F_SETFL, 0);

  /*
   * Get the current options for the port...
  */

  tcgetattr(fd, &options);

  /*
  * Set the baud rates to 19200...
   */

  cfsetispeed(&options, B38400);
  cfsetospeed(&options, B38400);

  /*
  * Enable the receiver and set local mode...
  */

  options.c_cflag |= (CLOCAL | CREAD);

  //set 8N1
  options.c_cflag &= ~PARENB;
  options.c_cflag &= ~CSTOPB;
  options.c_cflag &= ~CSIZE;
  options.c_cflag |= CS8;

  /*
  * Set the new options for the port...
   */

  tcsetattr(fd, TCSANOW, &options);


  return (fd);
}

Answer 4

I would modify your serial reading function in a few ways, firstly by changing its name to reflect what it does. I would also move the accumulation buffer into the function, making it and its position counter static (which means they stick around without losing their value between calls) . Also define the details of the packet in #defines.

More extensive changes are required to recover from a loss of synchronization to the data or on failure of the check-sum. If the check-sum fails, I don't think it is safe to discard the whole packet unless you can guarantee that the sentinel value (0xA5 or whatever) never occurs within the packet or its check-sum. As you don't mention it, I'm assuming we can't guarantee this, although it is clearly possible to arrange the data protocol to provide such a guarantee.

Without the guarantee, we need to check the check-sum within the reading function and to re-sync to the next sentinel on error.

I think this might be easier to achieve by making the file descriptor non-blocking and reading one byte at a time. As your data rate is slow, this is not a great overhead. And as your baud rate is slow (compared to processor speed) the chances are that your select will unblock after each received character.

You should also check for errors in the read call and return an error unless the read was interrupted (EINTR) - in which case the fd will still select available and the function will be called again - or no data was available (EAGAIN).

Here is some untested code to do this.

#define SENTINEL ((char)0xA5)
#define PACKET_SIZE 10

int read_serial(int fd, char *out)
{
    static char buf[PACKET_SIZE * 2];
    static int in;
    char ch;
    ssize_t err;

    while ((err = read(fd, &ch, 1)) > 0) { /* non-blocking read */
        buf[in++] = ch;
        if (buf[0] == SENTINEL) {
            if (in < PACKET_SIZE) {
                continue;
            }
            if (checksum_ok(buf)) {
                memcpy(out, buf+1, PACKET_SIZE-2); /* copy only the data */
                in = 0;
                return 1;
            }
        }
        /* resynchronise */
        const char *s = memchr(buf + 1, SENTINEL, (size_t) in);
        if (!s) {
            in = 0;
        } else {
            in -= s - buf;
            memmove(buf, s, in);
        }
    }
    return ((err == EINTR) || (err == EAGAIN)) ? 0 : -1;
}

Answer 5

You pass &tmp_input to select but then pass &input to the FD_ISSETs. These two fd_set objects are different. The input fd_set is set once, so FD_ISSET may well lie to you and your reads will fail.