cpu/esp32/freertos: fix semaphore take #11239

JulianHolzwarth · 2019-03-22T15:38:34Z

Contribution description

Involved parts: cpu/esp32/freertos/semphr.c and tests/freertos

Added test to test freertos. At the moment only tests for esp32-wroom-32 board the mutex semaphore creation and xSemaphoreTake() and the reucursive mutex semaphore creation and xSemaphoreTakeRecursive().
The future goal for this test is to test the complete freertos compatibility layer and to have the test for a general freertos compatibility layer that does not only work for esp32. I am currently working on a general freertos compatibility layer and trying to implement a test for it first, while doing it i noticed a bug.

Bug fix for cpu/esp32/freertos/semphr.c::xSemaphoreTakeRecursive() and cpu/esp32/freertos/semphr.c::xSemaphoreTake() return value when xTicksToWait is 0. The bug is that the functions return pdTRUE when it failed and pdFALSE when it was succesful.
But they should return (freertos documentation):

"pdPASS Returned only if the call to xSemaphoreTakeRecursive() was successful in obtaining the semaphore."

"pdFAIL Returned if the call to xSemaphoreTakeRecursive() did not successfully obtain the semaphore."

"pdPASS Returned only if the call to xSemaphoreTake() was successful in obtaining the semaphore"

"pdFAIL Returned if the call to xSemaphoreTake() did not successfully obtain the semaphore."

Testing procedure

only whitelisted for esp32-wroom-32

To test run BOARD=esp32-wroom-32 make -C tests/freertos/ flash test
To see the the bug run the command before the return value fix commit (there is one for each function)

gschorcht · 2019-03-22T15:48:37Z

@JulianHolzwarth Thanks for figuring out that bug. Makes sense. Obviously, these functions were never used by SDK library functions even though they require that the symbols are defined for linking.

tests/freertos/semaphore_test.c

tests/freertos/semaphore_test.h

tests/freertos/tests/01-run.py

tests/freertos/main.c

gschorcht · 2019-03-22T16:29:02Z

I wonder if it wouldn't be better to bundle the test application test/freertos with your future freertos library. This PR should only contain the bug fix for the ESP32 version. The correctness of this PR can be deduced without a test from the source code.

JulianHolzwarth · 2019-03-22T18:22:19Z

I will work on it Wednesday .

Added test for freertos. Only for BOARD:esp32-wroom-32 at the moment. At the moment it only tests xSemaphoreTake(), xSemaphoreTakeRecursive() and the creation of the semaphore. It creates the semaphore and calls take for it multiple times and checks whether the return value is correct. Take should return (quote freertos documention): "pdPASS Returned only if the call to xSemaphoreTakeRecursive() was successful in obtaining the semaphore." "pdFAIL Returned if the call to xSemaphoreTakeRecursive() did not successfully obtain the semaphore." The same for xSemaphoreTake(). The future goal for this test is to test the complete freertos compatibility layer and to have the test for a general freertos compatibility layer that does not only work for esp32.

xSemaphoreTake() returned before the fix: pdFALSE(equal to pdFAIL) when the call was successful in obtaining the semaphore and pdTRUE(equal to pdPASS) when the call did not successfully obtain the semaphore. According to freertos documentation: "pdPASS Returned only if the call to xSemaphoreTake() was successful in obtaining the semaphore" "pdFAIL Returned if the call to xSemaphoreTake() did not successfully obtain the semaphore." Fixed it to return the correct value.

xSemaphoreTakeRecursive() returned before the fix: pdFALSE(equal to pdFAIL) when the call was successful in obtaining the semaphore and pdTRUE(equal to pdPASS) when the call did not successfully obtain the semaphore. According to freertos documentation: "pdPASS Returned only if the call to xSemaphoreTakeRecursive() was successful in obtaining the semaphore" "pdFAIL Returned if the call to xSemaphoreTakeRecursive() did not successfully obtain the semaphore." Fixed it to return the correct value.

This reverts commit 0a7a86d.

JulianHolzwarth · 2019-04-17T14:11:12Z

I reverted the test commit. Are you ok with this one?

xSemaphoreTake() and xSemaphoreTakeRecursive() assert when timeout is not 0 or portMAX_DELAY because it is not implemented. Return value fix: cpu/esp32/freertos: fix semaphore take RIOT-OS#11239

miri64 · 2019-07-30T14:04:15Z

Ping @gschorcht?

gschorcht · 2019-07-30T15:13:58Z

cpu/esp32/freertos/semphr.c

@@ -94,7 +94,7 @@ BaseType_t xSemaphoreTake (SemaphoreHandle_t xSemaphore,
        case queueQUEUE_TYPE_MUTEX:
        {
            if (xTicksToWait == 0) {
-                return (mutex_trylock(mutex) == 0) ? pdTRUE : pdFALSE;
+                return (mutex_trylock(mutex) == 1) ? pdPASS : pdFAIL;


@JulianHolzwarth Don't change the return values. There is no reason for it. According to API reference, the return value of xSemaphoreTake is pdTRUE or pdFALSE. Even though the values are the same, we should use them as specified to avoid misleadings in future implementations of the FreeRTOS adaptation layer.

I was looking at the Reference Manual. It gave me more information about the function.
https://www.freertos.org/wp-content/uploads/2018/07/FreeRTOS_Reference_Manual_V10.0.0.pdf
(Page 244 ff.)

Linked from here:
https://www.freertos.org/Documentation/RTOS_book.html

If you like PdTrue more I will change it.
How shall we proceed?

The latest official release http://www.freertos.org/a00104.html has return pdPASS;
and
https://github.com/aws/amazon-freertos/blob/edc41fb223b8f9529162651a7c0459b10b450164/freertos_kernel/queue.c#L1414
has the same.
(The other return is return errQUEUE_EMPTY)

That's strange. Obviously, the official offline documentation does not fit the official online documentation. And even more strange, the documentation in semphr.h does not fit the implementatoin in queue.c.

The question is how errQUEUE_EMPTY is defined.

This might become a pitfall. By default, these values are defined:

#define errQUEUE_EMPTY ( ( BaseType_t ) 0 ) #define pdFALSE ( ( BaseType_t ) 0 ) #define pdFAIL ( pdFALSE )

Defining something like an error code which is nothing else or the same as pdFALSE is quite strange, especially since other error codes are real values:

#define errQUEUE_BLOCKED ( -4 ) #define errQUEUE_YIELD ( -5 )

If someone concludes from the online documentation that xSemaphoreTake returns something like a boolean, he could try the following:

if (!xSemaphoreTake(...)) { ... }

Although it would work for errQUEUE_EMPTY, it would not work for the other error codes.

gschorcht · 2019-07-30T15:14:21Z

cpu/esp32/freertos/semphr.c

@@ -147,7 +147,7 @@ BaseType_t xSemaphoreTakeRecursive (SemaphoreHandle_t xSemaphore,
    rmutex_t*  rmutex = &((_rmutex_t*)xSemaphore)->rmutex;

    if (xTicksToWait == 0) {
-        ret = (rmutex_trylock(rmutex) == 0) ? pdTRUE : pdFALSE;
+        ret = (rmutex_trylock(rmutex) == 1) ? pdPASS : pdFAIL;


The return values should not be changed, see above.

gschorcht

Finally, we can change the return values because they are mapped to pdTRUE and pdFALSE anyway. So there will be no difference.

jcarrano changed the title ~~Esp32/freertos/fix/semaphore take~~ cpu/esp32/freertos: fix semaphore take Mar 22, 2019

jcarrano requested a review from gschorcht March 22, 2019 15:39

jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Mar 22, 2019

gschorcht requested changes Mar 22, 2019

View reviewed changes

JulianHolzwarth force-pushed the esp32/freertos/fix/semaphoreTake branch from 2fc56dd to ce3de87 Compare March 22, 2019 18:01

JulianHolzwarth added a commit to JulianHolzwarth/RIOT that referenced this pull request Mar 27, 2019

new test, copy from cpu/esp32/freertos: fix semaphore take RIOT-OS#11239

7463442

JulianHolzwarth force-pushed the esp32/freertos/fix/semaphoreTake branch from 44e53fc to 04cbb4f Compare March 27, 2019 14:40

JulianHolzwarth added 3 commits March 27, 2019 15:54

JulianHolzwarth force-pushed the esp32/freertos/fix/semaphoreTake branch from 04cbb4f to e1d4551 Compare March 27, 2019 14:55

Revert "tests/freertos: add test for compat layer"

be9063c

This reverts commit 0a7a86d.

JulianHolzwarth mentioned this pull request May 3, 2019

[TRACKING] FreeRTOS Adaption Layer #11485

Open

4 tasks

gschorcht requested changes Jul 30, 2019

View reviewed changes

gschorcht approved these changes Jul 31, 2019

View reviewed changes

gschorcht merged commit dd6a5cd into RIOT-OS:master Jul 31, 2019

JulianHolzwarth deleted the esp32/freertos/fix/semaphoreTake branch July 31, 2019 14:51

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/esp32/freertos: fix semaphore take #11239

cpu/esp32/freertos: fix semaphore take #11239

JulianHolzwarth commented Mar 22, 2019

gschorcht commented Mar 22, 2019 •

edited

Loading

gschorcht commented Mar 22, 2019

JulianHolzwarth commented Mar 22, 2019

JulianHolzwarth commented Apr 17, 2019

miri64 commented Jul 30, 2019

gschorcht Jul 30, 2019 •

edited

Loading

JulianHolzwarth Jul 30, 2019 •

edited

Loading

JulianHolzwarth Jul 30, 2019

gschorcht Jul 30, 2019

gschorcht Jul 30, 2019 •

edited

Loading

gschorcht Jul 30, 2019

gschorcht left a comment

cpu/esp32/freertos: fix semaphore take #11239

cpu/esp32/freertos: fix semaphore take #11239

Conversation

JulianHolzwarth commented Mar 22, 2019

Contribution description

Testing procedure

gschorcht commented Mar 22, 2019 • edited Loading

gschorcht commented Mar 22, 2019

JulianHolzwarth commented Mar 22, 2019

JulianHolzwarth commented Apr 17, 2019

miri64 commented Jul 30, 2019

gschorcht Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

JulianHolzwarth Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

JulianHolzwarth Jul 30, 2019

Choose a reason for hiding this comment

gschorcht Jul 30, 2019

Choose a reason for hiding this comment

gschorcht Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

gschorcht Jul 30, 2019

Choose a reason for hiding this comment

gschorcht left a comment

Choose a reason for hiding this comment

gschorcht commented Mar 22, 2019 •

edited

Loading

gschorcht Jul 30, 2019 •

edited

Loading

JulianHolzwarth Jul 30, 2019 •

edited

Loading

gschorcht Jul 30, 2019 •

edited

Loading