Fix TaskRunner::addTask registering every task twice

fprime-baremetal/Os/TaskRunner/CMakeLists.txt

@@ -1,11 +1,27 @@
 ####
 # F prime CMakeLists.txt:
 #
-# SOURCE_FILES: combined list of source and autocoding files
-# MOD_DEPS: (optional) module dependencies
+# SOURCES: source files
+# DEPENDS: module dependencies
 #
 ####
 # Fw/Time/TimeInterval.hpp is included by Os/Task.hpp
-set(MOD_DEPS Fw_Types Os)
-set(SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/TaskRunner.cpp")
-register_fprime_module()
+register_fprime_module(
+    SOURCES
+        "${CMAKE_CURRENT_LIST_DIR}/TaskRunner.cpp"
+    DEPENDS
+        Fw_Types
+        Os
+)
+
+register_fprime_ut(
+    TaskRunnerTest
+    SOURCES
+        "${CMAKE_CURRENT_LIST_DIR}/test/ut/TaskRunnerTests.cpp"
+    DEPENDS
+        Os
+        STest
+        fprime-baremetal_Os_TaskRunner
+    CHOOSES_IMPLEMENTATIONS
+        Os_Task_Baremetal
+)

fprime-baremetal/Os/TaskRunner/TaskRunner.cpp

@@ -25,17 +25,24 @@ TaskRunner::TaskRunner() {
 TaskRunner::~TaskRunner() {}
 
 void TaskRunner::addTask(Task* task) {
-    FW_ASSERT(task->isCooperative());  // Cannot register uncooperative tasks
     FW_ASSERT(task != nullptr);        // Cannot register a null task
+    FW_ASSERT(task->isCooperative());  // Cannot register uncooperative tasks
 
-    FW_ASSERT(this->m_index < Os::Baremetal::TASK_CAPACITY, static_cast<FwAssertArgType>(this->m_index),
+    // removeTask() keeps the table packed, so the first empty slot is the task count
+    FwSizeType count = 0;
+    for (FwSizeType i = 0; i < Os::Baremetal::TASK_CAPACITY; i++) {
+        if (this->m_task_table[i] != nullptr) {
+            count++;
+        } else {
+            break;
+        }
+    }
+    FW_ASSERT(count < Os::Baremetal::TASK_CAPACITY, static_cast<FwAssertArgType>(count),
               static_cast<FwAssertArgType>(Os::Baremetal::TASK_CAPACITY));
-    this->m_task_table[this->m_index] = task;
-    this->m_index++;
 
     // Find the insertion point that keeps the table sorted in descending priority order.
-    FwSizeType insert_pos = this->m_index;
-    for (FwSizeType i = 0; i < this->m_index; i++) {
+    FwSizeType insert_pos = count;
+    for (FwSizeType i = 0; i < count; i++) {
         if (task->getPriority() > this->m_task_table[i]->getPriority()) {
             insert_pos = i;
             break;
@@ -44,11 +51,10 @@ void TaskRunner::addTask(Task* task) {
 
     // Shift every entry from insert_pos onward one slot to the right to make room, then place the new task.
     // This avoids duplicating/overwriting existing entries.
-    for (FwSizeType i = this->m_index; i > insert_pos; i--) {
+    for (FwSizeType i = count; i > insert_pos; i--) {
         this->m_task_table[i] = this->m_task_table[i - 1];
     }
     this->m_task_table[insert_pos] = task;
-    this->m_index++;
 }
 
 void TaskRunner::removeTask(Task* task) {

fprime-baremetal/Os/TaskRunner/docs/sdd.md

@@ -0,0 +1,54 @@
+# Os::Baremetal::TaskRunner
+
+## 1. Introduction
+
+The `Os::Baremetal::TaskRunner` runs cooperative tasks on systems without an operating system scheduler. A cooperative
+task does one unit of work each time it is run and then returns, letting the task runner run the next task. Baremetal
+`Os::Task` implementations register themselves with the task runner when they are started, and the system's main loop
+calls `TaskRunner::run()` repeatedly.
+
+## 2. Requirements
+
+The requirements for `Os::Baremetal::TaskRunner` are as follows:
+
+Requirement | Description | Verification Method
+----------- | ----------- | -------------------
+TR-001 | The `Os::Baremetal::TaskRunner` shall register each started task exactly once | Unit Test
+TR-002 | The `Os::Baremetal::TaskRunner` shall run tasks in descending priority order, and tasks of equal priority in the order they were started | Unit Test
+TR-003 | The `Os::Baremetal::TaskRunner` shall run one unit of work of one task per call to `run()`, giving each registered task one turn per round | Unit Test
+TR-004 | The `Os::Baremetal::TaskRunner` shall run one unit of work of each registered task per call to `runAll()` | Unit Test
+TR-005 | The `Os::Baremetal::TaskRunner` shall register tasks started after it has begun running without dropping or duplicating registered tasks | Unit Test
+TR-006 | The `Os::Baremetal::TaskRunner` shall stop running a task once the task is removed | Unit Test
+TR-007 | The `Os::Baremetal::TaskRunner` shall register up to `TASK_CAPACITY` tasks, and assert when more are started | Unit Test
+
+## 3. Design
+
+### 3.1 Interfaces
+
+Method | Usage
+------ | -----
+`addTask(Task*)` | Called by `Os::Task::start()` to register a task
+`removeTask(Task*)` | Called by `Os::Task::~Task()`, and by the task runner when a task exits, to unregister a task
+`run()` | Run one unit of work of the next running task
+`runAll()` | Run one unit of work of each running task
+`stop()` | Stop running tasks
+`getSingleton()` | Get the task runner that baremetal tasks register with
+
+### 3.2 Functional Description
+
+Registered tasks are kept in a task table sorted by descending priority. Only running tasks are run: a task is running
+when its `Os::Task` state is `STARTING` or `RUNNING`. Tasks in other states, such as suspended or joined tasks, stay in
+the table but are skipped. `run()` walks the table in a round-robin, starting from where the previous call left off, and
+runs the next running task. `runAll()` walks the table from the start and runs each running task once. A task is removed
+from the table when its `Os::Task` is destroyed, or when its state becomes `EXITED` while its routine is being run.
+
+### 3.3 Algorithms
+
+The task table is a fixed array of `TASK_CAPACITY` task pointers. Registered tasks are packed at the start of the array
+and followed by `nullptr` entries, so the number of registered tasks is the index of the first `nullptr` entry.
+
+`addTask()` finds the first entry with a lower priority than the new task, shifts that entry and the ones after it one
+slot to the right, and places the new task there. This keeps the table sorted, and keeps tasks of equal priority in the
+order they were started.
+
+`removeTask()` shifts every entry after the removed task one slot to the left.

fprime-baremetal/Os/TaskRunner/test/ut/TaskRunnerTests.cpp

@@ -0,0 +1,182 @@
+// ======================================================================
+// \title fprime-baremetal/Os/TaskRunner/test/ut/TaskRunnerTests.cpp
+// \brief tests for Os::Baremetal::TaskRunner
+// ======================================================================
+#include <gtest/gtest.h>
+#include <Os/Task.hpp>
+#include <STest/Pick/Pick.hpp>
+#include <STest/Random/Random.hpp>
+#include <algorithm>
+#include <fprime-baremetal/Os/TaskRunner/TaskRunner.hpp>
+#include <memory>
+#include <string>
+#include <vector>
+
+int main(int argc, char** argv) {
+    ::testing::InitGoogleTest(&argc, argv);
+    STest::Random::seed();
+    return RUN_ALL_TESTS();
+}
+
+namespace {
+
+constexpr FwSizeType ITERATIONS = 100;  //!< Random task sets tried by each test
+constexpr U32 MAX_TASKS = 40;           //!< Tasks started at once, so two sets fit within TASK_CAPACITY
+constexpr U32 MAX_PRIORITY = 255;       //!< Largest priority picked, which fits any FwTaskPriorityType
+
+//! Record of a task's runs, passed to the task routine as its argument
+struct TaskRecord {
+    FwSizeType id;
+    std::vector<FwSizeType>* log;
+};
+
+void recordRun(void* argument) {
+    TaskRecord* record = static_cast<TaskRecord*>(argument);
+    record->log->push_back(record->id);
+}
+
+//! Starts baremetal tasks, which register themselves with the TaskRunner singleton, and records each run of their
+//! routines. Task ids are assigned in start order. Tasks are destroyed, and so removed from the runner, by clearTasks()
+//! and when the fixture is torn down.
+class TaskRunnerTest : public ::testing::Test {
+  protected:
+    void TearDown() override { this->clearTasks(); }
+
+    //! Start a task and return its id
+    FwSizeType startTask(FwTaskPriorityType priority) {
+        const FwSizeType id = this->m_tasks.size();
+        this->m_records.emplace_back(new TaskRecord{id, &this->m_log});
+        this->m_tasks.emplace_back(new Os::Task());
+        const std::string name = "Task" + std::to_string(id);
+        Os::Task::Status status = this->m_tasks.back()->start(
+            Os::Task::Arguments(Fw::String(name.c_str()), recordRun, this->m_records.back().get(), priority));
+        EXPECT_EQ(status, Os::Task::Status::OP_OK);
+        // Starting a baremetal task runs its routine once, which is not a scheduled run
+        this->m_log.clear();
+        return id;
+    }
+
+    //! Start tasks with the given priorities, in order
+    void startTasks(const std::vector<FwTaskPriorityType>& priorities) {
+        for (FwTaskPriorityType priority : priorities) {
+            this->startTask(priority);
+        }
+    }
+
+    //! Destroy all tasks and clear the run log
+    void clearTasks() {
+        // Destroy tasks before their records
+        this->m_tasks.clear();
+        this->m_records.clear();
+        this->m_log.clear();
+    }
+
+    //! Pick between 1 and MAX_TASKS random priorities. The priorities are drawn from a randomly sized range, so small
+    //! ranges produce many tasks of equal priority.
+    static std::vector<FwTaskPriorityType> pickPriorities() {
+        const U32 count = STest::Pick::lowerUpper(1, MAX_TASKS);
+        const U32 upper = STest::Pick::lowerUpper(0, MAX_PRIORITY);
+        std::vector<FwTaskPriorityType> priorities;
+        for (U32 i = 0; i < count; i++) {
+            priorities.push_back(static_cast<FwTaskPriorityType>(STest::Pick::lowerUpper(0, upper)));
+        }
+        return priorities;
+    }
+
+    //! Ids of the given priorities sorted by descending priority, with ties kept in start order
+    static std::vector<FwSizeType> expectedOrder(const std::vector<FwTaskPriorityType>& priorities) {
+        std::vector<FwSizeType> order;
+        for (FwSizeType id = 0; id < priorities.size(); id++) {
+            order.push_back(id);
+        }
+        std::stable_sort(order.begin(), order.end(),
+                         [&priorities](FwSizeType a, FwSizeType b) { return priorities[a] > priorities[b]; });
+        return order;
+    }
+
+    static Os::Baremetal::TaskRunner& runner() { return Os::Baremetal::TaskRunner::getSingleton(); }
+
+    std::vector<FwSizeType> m_log;
+    std::vector<std::unique_ptr<TaskRecord>> m_records;
+    std::vector<std::unique_ptr<Os::Task>> m_tasks;
+};
+
+}  // namespace
+
+TEST_F(TaskRunnerTest, RunAllRunsEachTaskOnceInPriorityOrder) {
+    for (FwSizeType iteration = 0; iteration < ITERATIONS; iteration++) {
+        this->clearTasks();
+        const std::vector<FwTaskPriorityType> priorities = pickPriorities();
+        this->startTasks(priorities);
+        runner().runAll();
+        ASSERT_EQ(this->m_log, expectedOrder(priorities));
+    }
+}
+
+TEST_F(TaskRunnerTest, RunGivesEachTaskOneTurnPerRound) {
+    for (FwSizeType iteration = 0; iteration < ITERATIONS; iteration++) {
+        this->clearTasks();
+        const std::vector<FwTaskPriorityType> priorities = pickPriorities();
+        this->startTasks(priorities);
+        for (FwSizeType round = 0; round < 2; round++) {
+            this->m_log.clear();
+            for (FwSizeType i = 0; i < priorities.size(); i++) {
+                runner().run();
+            }
+            ASSERT_EQ(this->m_log.size(), priorities.size());
+            for (FwSizeType id = 0; id < priorities.size(); id++) {
+                ASSERT_EQ(std::count(this->m_log.begin(), this->m_log.end(), id), 1) << "task " << id;
+            }
+        }
+    }
+}
+
+TEST_F(TaskRunnerTest, AddTaskAfterRunKeepsExistingTasks) {
+    for (FwSizeType iteration = 0; iteration < ITERATIONS; iteration++) {
+        this->clearTasks();
+        std::vector<FwTaskPriorityType> priorities = pickPriorities();
+        this->startTasks(priorities);
+        // Advance the run cursor, then start more tasks
+        const U32 runs = STest::Pick::lowerUpper(1, static_cast<U32>(2 * priorities.size()));
+        for (U32 i = 0; i < runs; i++) {
+            runner().run();
+        }
+        const std::vector<FwTaskPriorityType> more = pickPriorities();
+        this->startTasks(more);
+        priorities.insert(priorities.end(), more.begin(), more.end());
+        runner().runAll();
+        ASSERT_EQ(this->m_log, expectedOrder(priorities));
+    }
+}
+
+TEST_F(TaskRunnerTest, DestroyedTaskIsNoLongerRun) {
+    for (FwSizeType iteration = 0; iteration < ITERATIONS; iteration++) {
+        this->clearTasks();
+        const std::vector<FwTaskPriorityType> priorities = pickPriorities();
+        this->startTasks(priorities);
+        const FwSizeType destroyed = STest::Pick::lowerUpper(0, static_cast<U32>(priorities.size() - 1));
+        this->m_tasks[destroyed].reset();
+        runner().runAll();
+        std::vector<FwSizeType> expected = expectedOrder(priorities);
+        expected.erase(std::find(expected.begin(), expected.end(), destroyed));
+        ASSERT_EQ(this->m_log, expected);
+    }
+}
+
+TEST_F(TaskRunnerTest, AddTaskUpToCapacity) {
+    std::vector<FwTaskPriorityType> priorities;
+    for (FwSizeType i = 0; i < Os::Baremetal::TASK_CAPACITY; i++) {
+        priorities.push_back(static_cast<FwTaskPriorityType>(STest::Pick::lowerUpper(0, MAX_PRIORITY)));
+    }
+    this->startTasks(priorities);
+    runner().runAll();
+    ASSERT_EQ(this->m_log, expectedOrder(priorities));
+}
+
+TEST_F(TaskRunnerTest, AddTaskBeyondCapacityAsserts) {
+    for (FwSizeType i = 0; i < Os::Baremetal::TASK_CAPACITY; i++) {
+        this->startTask(static_cast<FwTaskPriorityType>(STest::Pick::lowerUpper(0, MAX_PRIORITY)));
+    }
+    ASSERT_DEATH(this->startTask(static_cast<FwTaskPriorityType>(STest::Pick::lowerUpper(0, MAX_PRIORITY))),
+                 "TaskRunner\\.cpp");
+}