[riot-notifications] [RIOT-OS/RIOT] sys/sched_rr: Add a round robin scheduler module (#16126)

Francisco notifications at github.com
Tue Sep 28 10:46:45 CEST 2021


@fjmolinas commented on this pull request.

Some more comments...

> +    while ((xtimer_now_usec() - start) < us){
+        /*do nothing*/
+    }

Isn't this the same as `xtimer_spin`?

> +    /*be nice give the CPU some time to do other things or rest*/
+    xtimer_usleep(us);
+}
+
+/*
+ * the following are threads that count and wait with different strategies and
+ * print their current count in steps.
+ * the ration of active (doing hard work like checking the timer)
+ * to passive (wait to be informed when a certain time is there) waiting
+ * is determined by there value given to the thread.
+ * the restless threads do never pause.
+ */
+
+void * thread_nicebreaks(void * d)
+{
+    xtimer_usleep(200 * 1000);  /*always be nice at start*/

```suggestion
    xtimer_usleep(200 * US_PER_MS);  /*always be nice at start*/
```

> +            printf("%s: %" PRIu32 ", %" PRIu32 "\n", name, w, work);
+#else
+            printf("T-Pid %i:%" PRIu32 ", %" PRIu32 "\n", pid, w, work);
+#endif
+            step = w;
+        }
+        bad_wait(work * WORK_SCALE);
+        w += work;
+    }
+}
+
+int main(void)
+{
+    {
+        static char stack[THREAD_STACKSIZE_DEFAULT];
+        static uint32_t workness = 3;   /* 0-10 workness*/

Is this a word?

> +        if (w - step >= PRINT_STEPS) {
+#ifdef DEVELHELP
+            printf("%s: %" PRIu32 ", %" PRIu32 "\n", name, w, work);
+#else
+            printf("T-Pid %i:%" PRIu32 ", %" PRIu32 "\n", pid, w, work);
+#endif
+            step = w;
+        }
+        bad_wait(work * WORK_SCALE);
+        w += work;
+    }
+}
+
+int main(void)
+{
+    {

why these brackets?

> +    for (;;) {
+        if (w - step >= PRINT_STEPS) {
+#ifdef DEVELHELP
+            printf("%s: %" PRIu32 ", %" PRIu32 "\n", name, w, work);
+#else
+            printf("T-Pid %i:%" PRIu32 ", %" PRIu32 "\n", pid, w, work);
+#endif
+            step = w;
+        }
+        bad_wait(work * WORK_SCALE);
+        w += work;
+        bad_wait(rest * WORK_SCALE);
+    }
+}
+
+void * thread_restless_yielding(void * d)

This does not seem to be used...

> @@ -0,0 +1,34 @@
+# name of your application
+APPLICATION = thread_duel
+
+# If no BOARD is found in the environment, use this default:
+BOARD ?= native
+
+# This has to be the absolute path to the RIOT base directory:
+RIOTBASE ?= $(CURDIR)/../..
+
+#this defaults to build with round_robin using ztimer

```suggestion
# This defaults to build with round_robin using ztimer
```

> +this is a thread duel application
+    to show or not to show
+RIOTS multi-threading abilities

Can you explain more here? Right now when looking at the test and example I'm not sure what to expect...

> @@ -0,0 +1,12 @@
+# Copyright (c) 2021 TUBA Freiberg
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+#
+
+config MODULE_SCHED_ROUND_ROBIN
+    bool "round robin scheduling support"
+    depends on MODULE_XTIMER

Currently, we are going with `ztimer` as the default backend for new modules.

> +    uint32_t w = 0;
+    uint32_t work = *((uint32_t *) d);
+    if (work > 10) {
+        work = 5;
+    }
+    uint32_t rest = (10 - work);
+    uint32_t step = 0;

This logic is not evident for me just from looking at it, can you give more details (comments)?

> +    uint32_t w = 0;
+    uint32_t work = *((uint32_t *) d);
+    if (work > 10) {
+        work = 5;
+    }
+    /*uint32_t rest = (10 - work);*/
+    uint32_t step = 0;

Same as above.

> +#ifdef DEVELHELP
+            printf("%s: %" PRIu32 ", %" PRIu32 "\n", name, w, work);
+#else
+            printf("T-Pid %i:%" PRIu32 ", %" PRIu32 "\n", pid, w, work);
+#endif
+            step = w;
+        }
+        bad_wait(work * WORK_SCALE);
+        w += work;
+        thread_yield();
+    }
+}
+
+void * thread_restless(void * d)
+{
+    xtimer_usleep(200 * 1000);  /*always be nice at start*/

```suggestion
    xtimer_usleep(200 * 1US_PER_MS);  /*always be nice at start*/
```

> +            else {
+                return;
+            }

?

-- 
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/16126#pullrequestreview-765133776
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210928/2069ca0c/attachment-0001.htm>


More information about the notifications mailing list