This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

RTOS/TM4C1294NCPDT: Always checking return value of Mailbox_pend?

Part Number: TM4C1294NCPDT
Other Parts Discussed in Thread: SYSBIOS

Tool/software: TI-RTOS


CCS 6.1.3
tirtos_tivac_2_16_01_14(bios_6_45_02_31)
xdctools_3_32_00_06_core
ti-cgt-arm_15.12.1.LTS

On page 117 of SPRUEX3P(SYS/BIOS (TI-RTOS Kernel) v6.45 Use's Guide), an example is shown for using event together with mailbox:

readerTask()
{
  while (TRUE) {/* Wait for either ISR or Mailbox message */
    events = Event_pend(myEvent,
      Event_Id_NONE, /* andMask = 0 */
      Event_Id_00 + Event_Id_01, /* orMask */
      BIOS_WAIT_FOREVER); /* timeout */
      
      if (events & Event_Id_00) {
        /* Get the posted message.
        * Mailbox_pend() will not block since Event_pend()
        * has guaranteed that a message is available.
        * Notice that the special BIOS_NO_WAIT
        * parameter tells Mailbox that Event_pend()
        * was used to acquire the available message.
        */
        Mailbox_pend(mbox, &msgB, BIOS_NO_WAIT);
        
        processMsg(&msgB);
      }
      if (events & Event_Id_01) {
        processISR();
    }
  }
}


But during my debug, I believe I sometimes see return value of Mailbox_pend(mbox, &msgB, BIOS_NO_WAIT) is FALSE, and then msgB is invalid. I read the source code in "packages\ti\sysbios\knl\Mailbox.c", and I can't figure out why it returns FALSE when Event_Id_00 has been set and timeout is BIOS_NO_WAIT.

So could you please help make it clear? Shall I also check the return value in this case?

  • Is anything else setting Event_Id_00 (besides the Mailbox)?
  • Hi, Todd,

      I think I could reproduce this issue by modifing example code "event_EK_TM4C1294XL_TI_TivaTM4C1294NCPDT". Please see the attchment. "event_EK_TM4C1294XL_TI_TivaTM4C1294NCPDT_0" is the original code, and "event_EK_TM4C1294XL_TI_TivaTM4C1294NCPDT_1" is the modified.

      When I run the code, CIO will output 

    event_EK_TM4C1294XL_TI_TivaTM4C1294NCPDT_0.rarevent_EK_TM4C1294XL_TI_TivaTM4C1294NCPDT_1.rar

  • Hi Jianyi,

    Thanks for the example. I've reproduced the issue. Nothing obvious is jumping out. I'm still debugging it. I'll update you soon.

    Todd
  • Found it. There is a small race window. I'm testing out a fix for the kernel and a potential work-around for the application. I'll post it on Tues.
  • Hi Jianyi,

    When a Mailbox_post is called, this is what basically happens internally  

       1. grabs the free queue semaphore

       2. grabs a msg from the free queue

       3. copies the user msg contents to the free msg

       4. places the msg on the  data queue

       5. posts the data queue semaphore which will also call Event_post

    Here's the race condition you exposed...

    - The low priority task calls Mailbox_post, but is preempted by the Clock function before it completes step 5 above

    - Clock function adds 5 msgs to the mailbox (and thus readies the readerTask)

    - readerTask returns from Event_pend with Event_Id_02 set

    - readerTask successfully gets msg from Mailbox_pend. Internally Mailbox_pend knows there are more messages, so it sets the Event_Id_02 again to denote another msg is present.

    - readerTask returns from Event_pend with Event_Id_02 set

    ...

    The problem is that the readerTask actually gets 6 msgs now (5 from the Clock function and 1 from the writerTask). So the Mailbox is empty and the readerTask now blocks.

    So the writerTask can now finish step 5. So the readerTask wakes up because of step 5, ..but the mailbox is empty.

    We opened a bug report on this and will be addressed in a future release.

    So what can you do?

    1. You can add a GateMutexPri_enter/leave around the Mailbox_pend and Mailbox_post in the readerTask and writerTask. This allows the writerTask to complete step 5 before the readerTask processes any msgs.

    2. We have a preliminary fix (not reviewed or stressed tested) for the Semaphore_post() API. I've attached it, but you'd need to rebuild the SYS/BIOS kernel (instructions are in the TI-RTOS User Guide). 

    /*
     * Copyright (c) 2013, Texas Instruments Incorporated
     * All rights reserved.
     *
     * Redistribution and use in source and binary forms, with or without
     * modification, are permitted provided that the following conditions
     * are met:
     *
     * *  Redistributions of source code must retain the above copyright
     *    notice, this list of conditions and the following disclaimer.
     *
     * *  Redistributions in binary form must reproduce the above copyright
     *    notice, this list of conditions and the following disclaimer in the
     *    documentation and/or other materials provided with the distribution.
     *
     * *  Neither the name of Texas Instruments Incorporated nor the names of
     *    its contributors may be used to endorse or promote products derived
     *    from this software without specific prior written permission.
     *
     * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
     * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
     * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
     * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
     * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
     * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
     * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
     * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
     * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
     * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
     * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
     */
    
    /*
     *  ======== Semaphore.c ========
     *  Implementation of functions specified in Semaphore.xdc.
     */
    #include <xdc/std.h>
    #include <xdc/runtime/Error.h>
    #include <xdc/runtime/Log.h>
    #include <xdc/runtime/Assert.h>
    #include <xdc/runtime/Startup.h>
    
    #include <ti/sysbios/knl/Event.h>
    #include <ti/sysbios/BIOS.h>
    #include <ti/sysbios/knl/Queue.h>
    #include <ti/sysbios/hal/Hwi.h>
    #include <ti/sysbios/knl/Swi.h>
    #define ti_sysbios_knl_Task__internalaccess
    #include <ti/sysbios/knl/Task.h>
    #include <ti/sysbios/knl/Clock.h>
    
    #include "package/internal/Semaphore.xdc.h"
    
    /*
     *  Semaphore uses Clock, Task and Event APIs. Queue and Hwi are fully
     *  unconstrained. Clock uses Swi. Swi and Task APIs are safe before
     *  BIOS_start() except for the user hooks.  Clock and Event APIs are
     *  therefore also safe. No startup needed in Semaphore.
     */
    
    /*
     *  ======== Semaphore_Instance_init ========
     */
    Void Semaphore_Instance_init(Semaphore_Object *sem, Int count,
            const Semaphore_Params *params)
    {
        Queue_Handle pendQ;
        UInt hwiKey;
    
        pendQ = Semaphore_Instance_State_pendQ(sem);
    
        sem->mode = params->mode;
        sem->count = count;
    
        /* Make sure that supportsEvents is TRUE if params->event is not null */
        Assert_isTrue((Semaphore_supportsEvents == TRUE) ||
                   ((Semaphore_supportsEvents == FALSE) &&
                    (params->event == NULL)), Semaphore_A_noEvents);
    
    
        Queue_construct(Queue_struct(pendQ), NULL);
    
        if (Semaphore_supportsEvents && (params->event != NULL)) {
    
            sem->event = params->event;
            sem->eventId = params->eventId;
    
            hwiKey = Hwi_disable();
            if (count) {
                /*
                 *  In the unlikely case that a task is already
                 *  pending on the event object waiting for this
                 *  Semaphore...
                 */
                Hwi_restore(hwiKey);
                Semaphore_eventPost(sem->event, sem->eventId);
            }
            else {
                Semaphore_eventSync(sem->event, sem->eventId, 0);
                Hwi_restore(hwiKey);
            }
        }
        else {
            sem->event = NULL;
            sem->eventId = 1;
        }
    }
    
    /*
     *  ======== Semaphore_Instance_finalize ========
     */
    Void Semaphore_Instance_finalize(Semaphore_Object *sem)
    {
        Queue_Handle pendQ;
    
        pendQ = Semaphore_Instance_State_pendQ(sem);
        Queue_destruct(Queue_struct(pendQ));
    
        if (Semaphore_supportsEvents && (sem->event != NULL)) {
            Semaphore_eventSync(sem->event, sem->eventId, 0);
        }
    }
    
    /*
     *  ======== Semaphore_pendTimeout ========
     *  called by Clock when timeout for a semaphore expires
     */
    Void Semaphore_pendTimeout(UArg arg)
    {
        UInt hwiKey;
        Semaphore_PendElem *elem = (Semaphore_PendElem *)xdc_uargToPtr(arg);
    
        hwiKey = Hwi_disable();
    
        /* Verify that Semaphore_post() hasn't already occurred */
    
        if (elem->pendState == Semaphore_PendState_CLOCK_WAIT) {
    
            /* remove task's qElem from semaphore queue */
            Queue_remove((Queue_Elem *)elem);
    
            elem->pendState = Semaphore_PendState_TIMEOUT;
    
            /*
             *  put task back into readyQ
             *  No need for Task_disable/restore sandwich since this
             *  is called within Swi (or Hwi) thread
             */
            Task_unblockI(elem->tpElem.task, hwiKey);
        }
    
        Hwi_restore(hwiKey);
    }
    
    /*
     *  ======== Semaphore_pend ========
     */
    Bool Semaphore_pend(Semaphore_Object *sem, UInt32 timeout)
    {
        UInt hwiKey, tskKey;
        Semaphore_PendElem elem;
        Queue_Handle pendQ;
        Clock_Struct clockStruct;
    
        Log_write3(Semaphore_LM_pend, (IArg)sem, (UArg)sem->count, (IArg)((Int)timeout));
    
        /*
         *  Consider fast path check for count != 0 here!!!
         */
    
        /*
         *  elem is filled in entirely before interrupts are disabled.
         *  This significantly reduces latency.
         */
    
        /* add Clock event if timeout is not FOREVER nor NO_WAIT */
        if (BIOS_clockEnabled
                && (timeout != BIOS_WAIT_FOREVER)
                && (timeout != BIOS_NO_WAIT)) {
            Clock_addI(Clock_handle(&clockStruct), (Clock_FuncPtr)Semaphore_pendTimeout, timeout, (UArg)&elem);
            elem.tpElem.clock = Clock_handle(&clockStruct);
            elem.pendState = Semaphore_PendState_CLOCK_WAIT;
        }
        else {
            elem.tpElem.clock = NULL;
            elem.pendState = Semaphore_PendState_WAIT_FOREVER;
        }
    
        pendQ = Semaphore_Instance_State_pendQ(sem);
    
        hwiKey = Hwi_disable();
    
        /* check semaphore count */
        if (sem->count == 0) {
    
            if (timeout == BIOS_NO_WAIT) {
                Hwi_restore(hwiKey);
                return (FALSE);
            }
    
            Assert_isTrue((BIOS_getThreadType() == BIOS_ThreadType_Task),
                            Semaphore_A_badContext);
    
            /*
             * Verify that THIS core hasn't already disabled the scheduler
             * so that the Task_restore() call below will indeed block
             */
            Assert_isTrue((Task_enabled()),
                            Semaphore_A_pendTaskDisabled);
    
            /* lock task scheduler */
            tskKey = Task_disable();
    
            /* get task handle and block tsk */
            elem.tpElem.task = Task_self();
    
            /* leave a pointer for Task_delete() */
            elem.tpElem.task->pendElem = (Task_PendElem *)&(elem);
    
            Task_blockI(elem.tpElem.task);
    
            if ((Semaphore_supportsPriority) &&
               (((UInt)sem->mode & 0x2) != 0)) {    /* if PRIORITY bit is set */
                Semaphore_PendElem *tmpElem;
                Task_Handle tmpTask;
                UInt selfPri;
    
                tmpElem = Queue_head(pendQ);
                selfPri = Task_getPri(elem.tpElem.task);
    
                while (tmpElem != (Semaphore_PendElem *)pendQ) {
                    tmpTask = tmpElem->tpElem.task;
                    /* use '>' here so tasks wait FIFO for same priority */
                    if (selfPri > Task_getPri(tmpTask)) {
                        break;
                    }
                    else {
                        tmpElem = Queue_next((Queue_Elem *)tmpElem);
                    }
                }
    
                Queue_insert((Queue_Elem *)tmpElem, (Queue_Elem *)&elem);
            }
            else {
                /* put task at the end of the pendQ */
                Queue_enqueue(pendQ, (Queue_Elem *)&elem);
            }
    
            /* start Clock if appropriate */
            if (BIOS_clockEnabled &&
                    (elem.pendState == Semaphore_PendState_CLOCK_WAIT)) {
                Clock_startI(elem.tpElem.clock);
            }
    
            Hwi_restore(hwiKey);
    
            /* unlock task scheduler and block */
            Task_restore(tskKey);   /* the calling task will block here */
    
            /* Here on unblock due to Semaphore_post or timeout */
    
            hwiKey = Hwi_disable();
    
            if (Semaphore_supportsEvents && (sem->event != NULL)) {
                /* synchronize Event state */
                Semaphore_eventSync(sem->event, sem->eventId, sem->count);
            }
    
            /* remove Clock object from Clock Q */
            if (BIOS_clockEnabled && (elem.tpElem.clock != NULL)) {
                Clock_removeI(elem.tpElem.clock);
                elem.tpElem.clock = NULL;
            }
            
            elem.tpElem.task->pendElem = NULL;
    
            Hwi_restore(hwiKey);
    
            return ((Bool)(elem.pendState));
        }
        else {
            /*
             * Assert catches Semaphore_pend calls from Hwi and Swi
             * with non-zero timeout.
             */
            Assert_isTrue((timeout == BIOS_NO_WAIT) ||
                    ((BIOS_getThreadType() == BIOS_ThreadType_Task) ||
                    (BIOS_getThreadType() == BIOS_ThreadType_Main)),
                    Semaphore_A_badContext);
    
            --sem->count;
    
            if (Semaphore_supportsEvents && (sem->event != NULL)) {
                /* synchronize Event state */
                Semaphore_eventSync(sem->event, sem->eventId, sem->count);
            }
    
            /* remove Clock object from Clock Q */
            if (BIOS_clockEnabled && (elem.tpElem.clock != NULL)) {
                Clock_removeI(elem.tpElem.clock);
                elem.tpElem.clock = NULL;
            }
    
            Hwi_restore(hwiKey);
    
            return (TRUE);
        }
    }
    
    /*
     *  ======== Semaphore_post ========
     */
    Void Semaphore_post(Semaphore_Object *sem)
    {
        UInt tskKey, hwiKey;
        Semaphore_PendElem *elem;
        Queue_Handle pendQ;
    
        /* Event_post will do a Log_write, should we do one here too? */
        Log_write2(Semaphore_LM_post, (UArg)sem, (UArg)sem->count);
    
        pendQ = Semaphore_Instance_State_pendQ(sem);
    
        /* lock task scheduler */
        tskKey = Task_disable();
    
        hwiKey = Hwi_disable();
        
    
        if (Queue_empty(pendQ)) {
            if (((UInt)sem->mode & 0x1) != 0) {   /* if BINARY bit is set */
                sem->count = 1;
            }
            else {
                sem->count++;
                Assert_isTrue((sem->count != 0), Semaphore_A_overflow);
            }
    
            Hwi_restore(hwiKey);
    
            if (Semaphore_supportsEvents && (sem->event != NULL)) {
                Semaphore_eventPost(sem->event, sem->eventId);
            }
            /* lock task scheduler */
            Task_restore(tskKey);
    
            return;
        }
    
        
        /* dequeue tsk from semaphore queue */
        elem = (Semaphore_PendElem *)Queue_dequeue(pendQ);
    
        /* mark the Semaphore as having been posted */
        elem->pendState = Semaphore_PendState_POSTED;
    
        /* disable Clock object */
        if (BIOS_clockEnabled && (elem->tpElem.clock != NULL)) {
            Clock_stop(elem->tpElem.clock);
        }
    
        /* put task back into readyQ */
        Task_unblockI(elem->tpElem.task, hwiKey);
    
        Hwi_restore(hwiKey);
    
        Task_restore(tskKey);
    }
    
    /*
     *  ======== Semaphore_getCount ========
     */
    Int Semaphore_getCount(Semaphore_Object *sem)
    {
        return (sem->count);
    }
    
    /*
     *  ======== Semaphore_reset ========
     */
    void Semaphore_reset(Semaphore_Object *sem, Int count)
    {
        UInt hwiKey;
    
        hwiKey = Hwi_disable();
    
        sem->count = count;
    
        if (Semaphore_supportsEvents && (sem->event != NULL)) {
            /* synchronize Event state */
            Semaphore_eventSync(sem->event, sem->eventId, sem->count);
        }
    
        Hwi_restore(hwiKey);
    }
    
    /*
     *  ======== Semaphore_registerEvent ========
     */
    void Semaphore_registerEvent(Semaphore_Object *sem, Event_Handle event, UInt eventId)
    {
        if (Semaphore_supportsEvents) {
            UInt hwiKey = Hwi_disable();
            sem->event = event;
            sem->eventId = eventId;
            Hwi_restore(hwiKey);
        }
    }
    

    Todd

  • So another workaround might be checking return value of Mailbox_pend and processing the msg only if the return value if TRUE?

  • My solutions avoids the race condition, but nothing bad is really happening. You basically just have an extra Mailbox_pend call. So I agree that just checking the return code and acting accordingly is probably the best thing to do.