[En-Nut-Discussion] "Critical" section in UartAvrInput of uartavr.c

[Ernst Stippl]

Hi!

I am trying to design a device driver for an custom RS232 addon board and
looked thru various drivers to learn how device drivers look like in Nut/OS,
including uartavr.c

There, I found UartAvrInput

/*!
 * \brief Wait for input.
 *
 * This function checks the input buffer for any data. If
 * the buffer is empty, the calling \ref xrThread "thread" 
 * will be blocked until at least one new character is 
 * received or a timeout occurs.
 *
 * \param dev Indicates the UART device.
 *
 * \return 0 on success, -1 on timeout.
 */
int UartAvrInput(NUTDEVICE * dev)
{
    int rc = 0;
    IFSTREAM *ifs = dev->dev_icb;
    UARTDCB *dcb;

    NutEnterCritical();
    if (ifs->if_rd_idx == ifs->if_rx_idx) {
        dcb = dev->dev_dcb;
        /*
         * Changing if into a while loop fixes a serious bug: 
         * Previous receiver events without any waiting thread 
         * set the event handle to the signaled state. So the
         * wait returns immediately. Unfortunately the calling
         * routines rely on a filled buffer when we return 0.
         * Thanks to Mike Cornelius, who found this bug.
         */
        do {
            rc = NutEventWait(&dcb->dcb_rx_rdy, dcb->dcb_rtimeout);
        } while (rc == 0 && ifs->if_rd_idx == ifs->if_rx_idx);
    }
    NutExitCritical();

    return rc;
} 


I am puzzled by the fact that the critical secion extends over the call to
NutEventWait(...)

As far as I could see, NutEnterCritical disables interrupts (atom.h), which
is OK for this service, but what happens when another Thread is selcted to
run? And in case the SREG is reloded for another Thread when it becomes
running (which I assume) why have interrupts been disabled when calling
NutEventWait anyway?

On the other hand, may this implementation be a possible cause for missed
interrupts (i.e missed characters) ?

I thought about putting both comparison expressions within their own
function and surround them with Enter/Exit-Critical, i.e.:


/*!
 * \brief Wait for input.
 *
 * This function checks the input buffer for any data. If
 * the buffer is empty, the calling \ref xrThread "thread" 
 * will be blocked until at least one new character is 
 * received or a timeout occurs.
 *
 * NutEventWait returns 0 on event receive and -1 on timeout

 * \param dev Indicates the UART device.
 *
 * \return 0 on success, -1 on timeout.
 */


int MAXSPI_Comp_idx (NUTDEVICE * dev)
//
//  returns 1 on equal idx's, 0 on unequal
//
{
int rc = 0;				// preset result code to unequal
IFSTREAM *ifs = dev->dev_icb;		// define ptr to IFS 

NutEnterCritical();			// set critical
if (ifs->if_rd_idx == ifs->if_rx_idx) // compare
    rc = 1;				// set result code to equal
NutExitCritical();			// reset critical
return rc;				// return comparison result

}   // end MaxSpiComp_idx


int MaxSpiInput(NUTDEVICE * dev)
{
    int rc = 0;
    //IFSTREAM *ifs = dev->dev_icb;		// ES
    MAXDCB *dcb;

    //NutEnterCritical();			// ES
    if ((MAXSPI_Comp_idx(dev)) == 1) {		// if there are no char in
Buffer
        dcb = dev->dev_dcb;			// lets wait for PostEvent
        do {					// to happen
            rc = NutEventWait(&dcb->dcb_rx_rdy, dcb->dcb_rtimeout);
        } while ((rc == 0) && (MAXSPI_Comp_idx(dev) == 1));
    }
    //NutExitCritical();			// ES
						// otherwise: OK-return 
    return rc;
}


I am unsure if this change makes any sense, but my prototype device driver
and appl had less "hangs" after this change.

What do the more-Ethernut-experienced think about this?

regards
ernst