[riot-notifications] [RIOT-OS/RIOT] tests/xtimer_mutex_lock_timeout: add simple case test (#11679)

Gaëtan Harter notifications at github.com
Wed Jun 12 16:34:28 CEST 2019


cladmi requested changes on this pull request.

I agree with the functional part but still have some remarks with documentation/naming. Please see inline.

> @@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2019 Freie Universitaet Berlin,

```suggestion
 * Copyright (C) 2019 Freie Universität Berlin,
```

> @@ -0,0 +1,33 @@
+#!/usr/bin/env python3
+
+#  Copyright (C) 2019 Freie Universitaet Berlin,

```suggestion
#  Copyright (C) 2019 Freie Universität Berlin,
```

> +    /* timeout at one millisecond (1000 us) to make sure it does not spin. */
+    if (xtimer_mutex_lock_timeout(&test_mutex, 1000) == 0) {
+        puts("OK");
+    }
+    else {
+        puts("error: mutex timed out");
+    }
+
+    return 0;
+}
+
+/**
+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * the mutex is locked before the function call and
+ * the timeout is greater than XTIMER_BACKOFF (no spinning)

Please update in the direction, "the timeout will occur while waiting on the mutex" it would explain more the case without the need to mention spinning.

> +/**
+ * @brief   List of command for this application.
+ */
+static const shell_command_t shell_commands[] = {
+    { "mutex_timeout_n_spin_unlocked", "unlocked mutex and without spinning",
+      cmd_test_xtimer_mutex_lock_timeout_greater_backoff, },
+    { "mutex_timeout_n_spin_locked", "locked mutex and without spinning",
+      cmd_test_xtimer_mutex_lock_timeout_greater_backoff_locked, },
+    { NULL, NULL, NULL }
+};
+
+/**
+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * the mutex is not locked before the function call and
+ * the timeout is greater than XTIMER_BACKOFF (no spinning)

Please update in the direction, "the timeout callback will be removed before triggering" it would explain more the case.

> + * @brief       testing xtimer_mutex_lock_timeout function
+ *
+ *
+ * @author      Julian Holzwarth <julian.holzwarth at fu-berlin.de>
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include "shell.h"
+#include "xtimer.h"
+
+/**
+ * Foward declarations
+ */
+static int cmd_test_xtimer_mutex_lock_timeout_greater_backoff(int argc,

Maybe update the function names to not mention 'backoff' but more what is tested, so testing with a free mutex and long timeout. Would match the documentation updates.

Same for the other function.

>  typedef struct {
     mutex_t *mutex;
     thread_t *thread;
-    int timeout;
+    volatile int timeout;

Agreed with the change, the value is modified in another context.

> + *
+ * @param[in] argc  Number of arguments
+ * @param[in] argv  Array of arguments
+ *
+ * @return 0 on success
+ */
+static int cmd_test_xtimer_mutex_lock_timeout_greater_backoff(int argc,
+                                                              char **argv)
+{
+    (void)argc;
+    (void)argv;
+    puts("starting test: xtimer mutex lock timeout");
+    mutex_t test_mutex = MUTEX_INIT;
+
+    /* timeout at one millisecond (1000 us) to make sure it does not spin. */
+    if (xtimer_mutex_lock_timeout(&test_mutex, 1000) == 0) {

Please put the `1000` as a macro with the comment before so it can be re-used in both functions.

-- 
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/11679#pullrequestreview-248794125
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190612/cb3e0284/attachment-0001.html>


More information about the notifications mailing list