[riot-notifications] [RIOT-OS/RIOT] tests/xtimer_mutex_lock_timeout: New test (#12008)

MichelRottleuthner notifications at github.com
Wed Sep 25 17:47:55 CEST 2019


MichelRottleuthner requested changes on this pull request.

Thanks for keep bugging me.. and sorry for the delay ;) The changes you made are fine. But I still found some documentation things, see below. After that it should be ready for merge, so feel free to already suqash everything together as needed since the changes are small enough to still keep track.

> @@ -114,6 +172,77 @@ static int cmd_test_xtimer_mutex_lock_timeout_long_locked(int argc,
             puts("error mutex not locked");
         }
     }
+    /* to make the test easier to read */
+    printf("\n");
+
+    return 0;
+}
+
+/**
+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * This function will create a new thread with lower prio
+ * than the main thread (this function should be called from
+ * the main thread). The new thread will get a mutex and will
+ * lock it. This function (main thread) messages the other
+ * thread and calls xtimer_mutex_lock_timeout.

This part of the documentation is not valid anymore. i.e. the messaging before calling xtimer_mutex_lock_timeout was removed.

> +/**
+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * This function will create a new thread with lower prio
+ * than the main thread (this function should be called from
+ * the main thread). The new thread will get a mutex and will
+ * lock it. This function (main thread) messages the other
+ * thread and calls xtimer_mutex_lock_timeout.
+ * The other thread will then unlock the mutex. The main
+ * thread gets the mutex and wakes up. The timer will not
+ * trigger because the main threads gets the mutex.
+ *
+ * @param[in] argc  Number of arguments
+ * @param[in] argv  Array of arguments
+ *
+ * @return 0 on success

I think in this case it can be argued what counts as sucess and what not but the function *always* return 0 even in "error mutex not locked" condition.

> @@ -42,9 +49,58 @@ static const shell_command_t shell_commands[] = {
       cmd_test_xtimer_mutex_lock_timeout_long_unlocked, },
     { "mutex_timeout_long_locked", "locked mutex with long timeout",
       cmd_test_xtimer_mutex_lock_timeout_long_locked, },
+    { "mutex_timeout_long_locked_low",
+      "locked mutex from lower prio thread function with long timeout",

actually the description part of the command is confusing as its not really explaining what happens here. Somewhere in the test it should be explained in clear to understand language what the test checks, what kind of guarantee this implies and by that what it means if the test fails. Maybe for the short description in the command something like "checks if xtimer_mutex_lock_timeout can acquire a mutex that was previously locked by a lower prio thread" would be better to understand for a random dude looking at this test for the first time. Also the terms low and long are not very precise here. I guess you want to express something like *longer than needed* i.e. the timeout is big enough that we are absolutely sure the timeout should never actually be triggered if everything is working fine. And *too short* as in "the timeout is so small that we know it should always trigger if everything works.  

-- 
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/12008#pullrequestreview-293155915
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190925/688e1361/attachment-0001.htm>


More information about the notifications mailing list