[riot-notifications] [RIOT-OS/RIOT] cpu/esp32/freertos: fix semaphore take (#11239)

Gunar Schorcht notifications at github.com
Fri Mar 22 17:23:32 CET 2019


gschorcht requested changes on this pull request.



> +
+#include "semaphore_test.h"
+
+/*
+ * @brief amount of times the tested function will be called
+ */
+#define TEST_SEMAPHORE_FOR_COUNTER 20
+
+/**
+ * @brief   tests the take function for a freertos mutex semaphore
+ *
+ * @return pdPASS when the test is passed, pdFail otherwise
+ */
+int semaphore_test_mutex_take(void)
+{
+    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();

This is a memory leak since `xSemaphoreCreateMutex` mallocs a mutex object. If it is not freed before return, it keeps allocated.

> + *
+ * @return pdPASS when the test is passed, pdFail otherwise
+ */
+int semaphore_test_mutex_take(void)
+{
+    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: mutex semaphore not created");
+    }
+    /*
+    Freertos Documentation:
+    "pdPASS: Returned only if the call to xSemaphoreTake()
+    was successful in obtaining the semaphore."
+    */
+    /* first call should be successful */
+    if (xSemaphoreTake(testing_semaphore,0) == pdFAIL) {

Use always a space after `,` here and below.

> +    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: mutex semaphore not created");
+    }
+    /*
+    Freertos Documentation:
+    "pdPASS: Returned only if the call to xSemaphoreTake()
+    was successful in obtaining the semaphore."
+    */
+    /* first call should be successful */
+    if (xSemaphoreTake(testing_semaphore,0) == pdFAIL) {
+        puts("error in Take");
+        return pdFAIL;
+    }
+    /* after the fist call every call should fail*/
+    for(size_t i = 0; i < TEST_SEMAPHORE_FOR_COUNTER; i++)

Why 20 times? One further try to lock the mutex should be enough to test the functionality that is tested here.

> + */
+int semaphore_test_recursive_mutex_take(void)
+{
+    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateRecursiveMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: recursive mutex semaphore not created");
+    }
+    /*
+    Freertos Documentation:
+    "pdPASS: Returned only if the call to xSemaphoreTakeRecursive()
+    was successful in obtaining the semaphore.",
+    "once a recursive mutex has been successfully ‘taken’ by a task, 
+    further calls to xSemaphoreTakeRecursive() made by the 
+    same task will also be successful."
+    */
+    for(size_t i = 0; i < TEST_SEMAPHORE_FOR_COUNTER; i++)

Place the opening curly brace in same line as the for statement.

> +    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: mutex semaphore not created");
+    }
+    /*
+    Freertos Documentation:
+    "pdPASS: Returned only if the call to xSemaphoreTake()
+    was successful in obtaining the semaphore."
+    */
+    /* first call should be successful */
+    if (xSemaphoreTake(testing_semaphore,0) == pdFAIL) {
+        puts("error in Take");
+        return pdFAIL;
+    }
+    /* after the fist call every call should fail*/
+    for(size_t i = 0; i < TEST_SEMAPHORE_FOR_COUNTER; i++)

Place the opening curly brace in same line as the for statement.

> + * @brief amount of times the tested function will be called
+ */
+#define TEST_SEMAPHORE_FOR_COUNTER 20
+
+/**
+ * @brief   tests the take function for a freertos mutex semaphore
+ *
+ * @return pdPASS when the test is passed, pdFail otherwise
+ */
+int semaphore_test_mutex_take(void)
+{
+    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: mutex semaphore not created");
+    }
+    /*

Multiple line comments have to be as following
```
/*
 * comment
 * comment
 */

> +{
+    SemaphoreHandle_t testing_semaphore = xSemaphoreCreateMutex();
+    if (testing_semaphore == NULL) {
+        puts("test failed: mutex semaphore not created");
+    }
+    /*
+    Freertos Documentation:
+    "pdPASS: Returned only if the call to xSemaphoreTake()
+    was successful in obtaining the semaphore."
+    */
+    /* first call should be successful */
+    if (xSemaphoreTake(testing_semaphore,0) == pdFAIL) {
+        puts("error in Take");
+        return pdFAIL;
+    }
+    /* after the fist call every call should fail*/

Use a space before the `*/`.

> @@ -0,0 +1,21 @@
+/*

The file has no header guards and no C binding section. Please refer the output of [TravisCI](https://travis-ci.org/RIOT-OS/RIOT/builds/509975119?utm_source=github_status&utm_medium=notification)

> +            break
+    
+    child.sendline("mutex_semaphore")
+    child.expect("starting test: mutex semaphore")
+    child.expect("OK")
+    child.expect_exact("> ")
+
+    child.sendline("recursive_mutex_semaphore")
+    child.expect_exact("starting test: recursive mutex semaphore")
+    child.expect("OK")
+    child.expect_exact("> ")
+
+
+
+if __name__ == "__main__":
+    sys.exit(run(testfunc))

Additional new line at the end of file.

> +};
+
+/**
+ * @brief   shell command to test freertos mutex semaphore
+ *
+ * @param[in] argc  Number of arguments
+ * @param[in] argv  Array of arguments
+ *
+ * @return 0 on success
+ */
+static int cmd_test_mutex(int argc, char **argv)
+{
+    (void) argc;
+    (void) argv;
+    puts("starting test: mutex semaphore");
+    if (semaphore_test_mutex_take() == pdPASS) {

Even though this function and the command are easy to use, they don't cover all possible test cases. Maybe, there should be separate functions for creating semaphores, locking/unlocking them, and deleting them. Only with separated functions, you will be able to test functionalities from different threads.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11239#pullrequestreview-217840769
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190322/62a091ca/attachment-0001.html>


More information about the notifications mailing list