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

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


@fjmolinas commented on this pull request.

Initial nitpicks...

> @@ -0,0 +1,23 @@
+# name of your application 
+APPLICATION = thread_duell

Semantics but why "duell" wouldn't it be "duel"?

> @@ -0,0 +1,23 @@
+# name of your application 
+APPLICATION = thread_duell
+
+# 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)/../..
+
+USEMODULE += xtimer
+# USEMODULE += sched_round_robin

Why is this comented out?

> +
+# 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)/../..
+
+USEMODULE += xtimer
+# USEMODULE += sched_round_robin
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+DEVELHELP ?= 1
+
+# CFLAGS += -DSCHED_RR_TIMER=100000

Why comment it out?

> +void sched_register_runqueue_cb(sched_runqueue_callback_t callback);
+
+/**
+ * @brief   round robin a runqueue
+ *
+ * @param   prio      the priority of the runqueue to round robin
+ */
+void sched_round_robin_runqueue(uint8_t prio);
+
+/**
+ * @brief   count the number of threads in a runqueue up to 2
+ *
+ * @param[in]   prio      the priority of the runqueue to get the length from
+ * @returns     number of threads in a runqueue up to 2
+ */
+uint_fast8_t sched_runqueue_len2(uint8_t prio);

Why counting up to two?

> +ifeq (1,$(ZTIMER))
+  USEMODULE += ztimer_usec
+else
+  USEMODULE += xtimer
+endif

Can't we just let it use `ztimer`?

> +ifeq (1,$(ZTIMER))
+  USEMODULE += ztimer_usec
+else
+  USEMODULE += xtimer
+endif
+
+ifeq (1,$(RR))
+  USEMODULE += sched_round_robin
+endif
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+DEVELHELP ?= 1
+
+# CFLAGS += -DSCHED_RR_TIMEOUT=100000

Why is this commented?

> + *
+ */
+#ifndef SCHED_ROUND_ROBIN_H
+#define SCHED_ROUND_ROBIN_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#if !defined(SCHED_RR_TIMEOUT) || defined(DOXYGEN)
+/**
+ * @brief   Time between round robin calls in µs
+ *
+ * @details Defaults to 1ms
+ */
+#define SCHED_RR_TIMEOUT 1000

If this is a configuration can you add `CONFIG_`

> + *
+ * @details Defaults to 1ms
+ */
+#define SCHED_RR_TIMEOUT 1000
+#endif
+
+#if !defined(SCHED_RR_MASK) || defined(DOXYGEN)
+/**
+ * @brief   Masks off priorities that should not be scheduled default: 0 is masked
+ *
+ * @details Priority 0 (highest) should always be masked.
+ *          Threads with that priority may not be programmed
+ *          with the possibility of being scheduled in mind.
+ *          Parts of this scheduler assume 0 current_rr_priority is uninitialised.
+ */
+#define SCHED_RR_MASK (1 << 0)

Same as above

-- 
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-602005747
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210928/437f6797/attachment-0001.htm>


More information about the notifications mailing list