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

[Harald Kipp]

Hello Ernst,

Somewhere in the context switch (too lazy to search the code)
an 'rti' is used to return to the newly activated thread. This
command will enable interrupts. This means, that NutEventWait()
will implicitly enable interrupts.

Taking this into account, NutEnterCritical() will work on the
first comparision, but the second one inside the loop is still
unprotected.

Another question comes up: Does it have to be protected at all?
IMO, no, because if_rx_idx is an 8-bit value, which is
incremented in the receive interrupt. Verifing two 8-bit values
can be considered atomic, because the CPU will read each value in
a single cycle.

More comments on your suggested change: Did you check, how much
additional CPU cycles are required to execute this code? I'd
at least vote for an inline call. However, not all compilers
may support this feature, so we have to use a preprocessor macro.
On the other hand, extensive use of preprocessor macros may create
less readable code. Thus, they should be used with care and reasons
for using them should be documented very well.

Finally, uartavr.c is the old version. It's quite simple and fast
and thus still kept in the distribution. usartavr.c is much more
advanced, but difficult to follow. The reason for the extraordinary
way of including .c files is not very well explained, which makes
it less attractive as a template for new drivers.

Harald

At 21:49 08.01.2005 +0100, you wrote:
>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
>
>
>_______________________________________________
>En-Nut-Discussion mailing list
>En-Nut-Discussion at egnite.de
>http://www.egnite.de/mailman/listinfo.cgi/en-nut-discussion