How to modify safely component library functions?

OK, not knowing a good way to modify library I have decided to work around the issue using original code and make all changes in my own functions.

Here is what I have done:

1. I used OSTaskDel() to delay my task for one tick in the event there was nothing in the circular buffer. Then I read again and compared number of ticks elapsed with the timeout. If I could wait longer I did, if not - I returned with timeout. Here is the code for the version 1

INT32S ReadUARTwithTimeout(struct _reent * r, INT32S uart, const char* uart_name, INT8U * ch, INT8U ms_timeout)
{
  INT32S result, need_to_close_uart=1;
  INT8U ticks = OS_TICKS_PER_SEC * ((INT32U)ms_timeout+500L/OS_TICKS_PER_SEC) / 1000;
  if(uart == 0)
    uart = _open_r(r, uart_name, O_RDONLY | O_NONBLOCK | O_NOCTTY, 0); 
  else
    need_to_close_uart = 0;
  
  if(uart == 0)
    {
    need_to_close_uart = 0;
    printf("Unable to open UART in the ReadUARTwithTimeout(%s)\n", uart_name);
    }
  else
    {
    INT32U start_time, current_time;
    start_time = OSTimeGet();
    while ((result = _read_r(r, uart, ch, 1)) == 0)
      {
      OSTimeDly(1); // delay task one tick
      current_time = OSTimeGet();
      // current_time might be smaller than start_time
      // when the tick counter overlaps, but we will
      // still get correct value working with INT32U types.
      if(current_time-start_time >= ticks)
        break;
      }
    }
  if(need_to_close_uart)
    _close_r(r, uart);
  return result;
}

This version of the function worked fine, but I did not like quite long and undetermined delay caused with lack of synchronisaton between uart receiver and the system timer interrupt. With a system clock in a range of 10ms the time was too much unpredictable. I could shorten the system clock interval but I felt it is not the way to go anyway... So I wrote 2nd version.

2. Second version is written under the inspiration of reading altera driver code. I tried to use the same system flags the original code in blocking mode uses to wait for received character. Instead waiting on flag indifinitelly, like the original code does, I added a timeout in my code. Here is the modified function:

 # include "altera_avalon_uart.h"# define ALT_UART_READ_RDY  0x1
extern alt_fd alt_fd_list;
INT32S ReadUARTwithTimeout(struct _reent * r, INT32S uart, const char* uart_name, INT8U * ch, INT8U ms_timeout)
{
  INT32S result, need_to_close_uart=1;
  INT8U ticks = OS_TICKS_PER_SEC * ((INT32U)ms_timeout+500L/OS_TICKS_PER_SEC) / 1000;
  if(uart == 0)
    uart = _open_r(r, uart_name, O_RDONLY | O_NONBLOCK | O_NOCTTY, 0); 
  else
    need_to_close_uart = 0;
  
  if(uart == 0)
    {
    need_to_close_uart = 0;
    printf("Unable to open UART in the ReadUARTwithTimeout(%s)\n", uart_name);
    }
  else
    {
    // Get access to UART code internal guts... :-)
    alt_fd* fd = &alt_fd_list;
    alt_avalon_uart_dev* dev = (alt_avalon_uart_dev*) fd->dev;
    result = _read_r(r, uart, ch, 1);
    if(result == 0)
      { // we need to wait on UART_READ_RDY flag for character
      result = ALT_FLAG_PEND (dev->events, 
                      ALT_UART_READ_RDY,
                      OS_FLAG_WAIT_SET_ANY + OS_FLAG_CONSUME,
                      ticks);
      if(result == OS_NO_ERR)
        {
        // The desired bits have been set within the specified 'timeout'
        result = _read_r(r, uart, ch, 1);
        }
      else     // The bit(s) have not been set in the specified 'timeout'
        result = 0;
      }
    }
  if(need_to_close_uart)
    _close_r(r, uart);
  return result;
}

The function worked great and I was amazed how predictable code can be compared to the 1st version... No uncertainity from the OSTaskDel()!

Unfortunatelly - there is a problem... From time to time I get an unexpected error. I debugged the code and I have found that the ALT_UART_READ_RDY system flag is set prematurely or without the reason at all! I am getting OS_NO_ERR result from ALT_FLAG_PEND indicating the flag was set without timeout but consecutive read() returns 0 like there was no character waiting! I have checked errno value after read() returned and it was not even EWOULDBLOCK. I am puzzled, but inspection of the altera RX IRQ handler revealed the reason for the false wake up call from the OS. It would be in the case of receiving a characeter with a parity or frame error.

Here is a fragment of altera_avalon_uart.c code showing RX IRQ handler:

static void alt_avalon_uart_rxirq (alt_avalon_uart_dev* dev,
                                   alt_u32              status)
{
  alt_u32 next;
  /*
   * In a multi-threaded environment, set the read event flag to indicate
   * that there is data ready. This is only done if the circular buffer was
   * previously empty.
   */
  if (dev->rx_end == dev->rx_start)
  {
    ALT_FLAG_POST (dev->events, ALT_UART_READ_RDY, OS_FLAG_SET);
  }
  /* Determine which slot to use next in the circular buffer */
  next = (dev->rx_end + 1) & ALT_AVALON_UART_BUF_MSK;
  /* Transfer data from the device to the circular buffer */
  dev->rx_buf = IORD_ALTERA_AVALON_UART_RXDATA(dev->base);
  /* If there was an error, discard the data */
  if (status & (ALTERA_AVALON_UART_STATUS_PE_MSK | 
                  ALTERA_AVALON_UART_STATUS_FE_MSK))
  {
    return;
  }
  dev->rx_end = next;
  next = (dev->rx_end + 1) & ALT_AVALON_UART_BUF_MSK;
  /*
   * If the cicular buffer was full, disable interrupts. Interrupts will be
   * re-enabled when data is removed from the buffer.
   */
  if (next == dev->rx_start)
  {
    dev->ctrl &= ~ALTERA_AVALON_UART_CONTROL_RRDY_MSK;
    IOWR_ALTERA_AVALON_UART_CONTROL(dev->base, dev->ctrl); 
  }   
}

As you can see, the task is awaken with a call to ALT_FLAG_POST before the received character is even checked for error and potentially discarded. I do not reject the posibility of receiving errors on my board but I tried to set the breakpoint in the RX IRW handler on the line with return; after detecting the parity/frame error but debugger never stopped there...

3. Because of this mistery, I have decided to hack my code in a way to detect a false wake up call from the OS and give it another chance http://forum.niosforum.com/work2/style_emoticons/<#EMO_DIR#>/smile.gif

Here is "improved" code:

#include "altera_avalon_uart.h"# define ALT_UART_READ_RDY  0x1
extern alt_fd alt_fd_list;
INT32S ReadUARTwithTimeout(struct _reent * r, INT32S uart, const char* uart_name, INT8U * ch, INT8U ms_timeout)
{
  INT32S result, need_to_close_uart=1;
  INT8U ticks = OS_TICKS_PER_SEC * ((INT32U)ms_timeout+500L/OS_TICKS_PER_SEC) / 1000;
  if(uart == 0)
    uart = _open_r(r, uart_name, O_RDONLY | O_NONBLOCK | O_NOCTTY, 0); 
  else
    need_to_close_uart = 0;
  
  if(uart == 0)
    {
    need_to_close_uart = 0;
    printf("Unable to open UART in the ReadUARTwithTimeout(%s)\n", uart_name);
    }
  else
    {
    // Get access to UART code internal guts... :-)
    alt_fd* fd = &alt_fd_list;
    alt_avalon_uart_dev* dev = (alt_avalon_uart_dev*) fd->dev;
    result = _read_r(r, uart, ch, 1);
    if(result == 0)
      { // we need to wait on UART_READ_RDY flag for character
      result = ALT_FLAG_PEND (dev->events, 
                      ALT_UART_READ_RDY,
                      OS_FLAG_WAIT_SET_ANY + OS_FLAG_CONSUME,
                      ticks);
      if(result == OS_NO_ERR)
        {
        // The desired bits have been set within the specified 'timeout'
        result = _read_r(r, uart, ch, 1);
        if(result == 0)
          { // False wake up call from the OS!!! Wait again.
          result = ALT_FLAG_PEND (dev->events, 
                          ALT_UART_READ_RDY,
                          OS_FLAG_WAIT_SET_ANY + OS_FLAG_CONSUME,
                          ticks);
          if(result == OS_NO_ERR)
            // The desired bits have been set within the specified 'timeout'
            result = _read_r(r, uart, ch, 1);
            // If result == 0 here than give up
            // 2nd False wake up call from the OS!!!
          else // The bit(s) have not been set in the specified 'timeout'
            result = 0;
          }
        }
      else     // The bit(s) have not been set in the specified 'timeout'
        result = 0;
      }
    }
  if(need_to_close_uart)
    _close_r(r, uart);
  return result;
}

My function in version 3. works fine, but unfortunatelly I have unwanted random delay of about 200-300us added from time to time - probably due to the bug in the Altera code. It is much better than the function version 1 but still not what I would like to have.

Of course - it could be a problem with my code, but in this case I do not see it and I would appreciate if you could point it out to me.

Thanks!