Potential race condition in CommandTask

[tygamvrelis]

Describe the bug
At the end of the initialization sequence in the CommandTask thread, we signal the other threads in the system (which are all a higher priority than CommandTask). We then block on a signal from the RX Task. The issue is if, a context switch to a higher-priority thread happens before we can block on this signal, we will miss it when it is set.

To Reproduce
N/A as we are currently getting lucky with our timing. This was identified after #126 was pulled in.

[tygamvrelis]

@rfairley and I discussed yesterday, and we figure that the issue would be solved if the CommandTask thread blocks on a binary semaphore initialized with a count of 0 instead of a signal.

Case 1: CommandTask thread blocks before a context switch

• When a new packet is received, the RX thread will give the semaphore, causing the CommandTask to be put into the ready state. This is the desired behaviour

Case 2: CommandTask thread is interrupted by a context switch before it can block

• In this case, when the RX thread gives the semaphore, it will increase the semaphore's count to 1. When the CommandTask eventually wakes up (due to all threads of higher priority being blocked), it will take the semaphore and since its count is positive when it is taken, the CommandTask thread will continue executing without being blocked, as desired

[rfairley]

One option for implementing the semaphore is FreeRTOS's xTaskNotifyGive() macro for when using task notifications as binary semaphores. It has to be used with ulTaskNotifyTake(), which either returns immediately if the task notification value is nonzero or waits for the value to be nonzero. If in Case 2 where CommandTask got interrupted before blocking, RxThread would set the notification value to nonzero, so when CommandTask "takes" the notification, it'd see it is nonzero, and continue as it should (where previously it would have missed the notification if with xTaskNotifyWait).

With CMSIS the option is osSemaphoreWait/Take which is fine, but the FreeRTOS "lightweight" semaphore option might give a performance benefit.

[rfairley]

Did some digging into this. I first looked at CMSIS again, because osThreadId is the type used for the task handles in freertos.cpp rather than the FreeRTOS TaskHandle_t. Because of this, it is preferable to use CMSIS functions (we have a maintenance task in #121 for this).

The CMSIS-RTOS documentation (for version 1.02 which is the version given in our cmsis_os.h file) states "When these signal flags are already set, the function returns instantly": https://siliconlabs.github.io/Gecko_SDK_Doc/CMSIS/RTOS/html/group___c_m_s_i_s___r_t_o_s___signal_mgmt.html#ga38860acda96df47da6923348d96fc4c9

The underlying function that cmsis_os.c uses for osSignalWait() is xTaskNotifyWait(), the one we were worried about. Giving the documentation a closer look, https://www.freertos.org/xTaskNotifyWait.html under Parameters: > xTicksToWait, the part "if a notification is not already pending when xTaskNotifyWait() is called" supports what the CMSIS documentation has. The code in tasks.c supports this as well.

It looks like we needn't have worried about xTaskNotifyWait() after all, though in the documentation it could have been clearer that the function doesn't block if a notification (of the correct signal) has already been received. Since CMSIS is the preferable API, and we are using osSignalSet/Wait already, it is safe to close the issue.

As a side note, with regard to the binary semaphore-style xTaskNotifyGive(), turns out there's not too much that's special about it as it just does xTaskNotify but with eAction passed as eIncrement - and the corresponding ulTaskNotifyTake() operates similarly to xTaskNotifyWait(). So we are good using osSignalSet() for our purposes 👍 .

[rfairley]

Closing this - our use of the CMSIS API does what we want it to.

[tygamvrelis]

Good to have that sorted out—thanks Robert

…

On Dec 15, 2018, at 17:09, Robert Fairley ***@***.***> wrote: Closed #129. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.