Fixing review comments

nasa · May 22, 2024 · 8f32105 · 8f32105
1 parent 9ffdee8
commit 8f32105
Show file tree

Hide file tree

Showing 8 changed files with 22 additions and 198 deletions.
diff --git a/Fw/Comp/ActiveComponentBase.cpp b/Fw/Comp/ActiveComponentBase.cpp
@@ -93,7 +93,9 @@ namespace Fw {
         // cast void* back to active component
         ActiveComponentBase* component = static_cast<ActiveComponentBase*>(component_pointer);
 
-        // Each invocation of this function runs a single stage of the thread lifecycle.
+        // Each invocation of this function runs a single stage of the thread lifecycle. This has moved the thread
+        // while loop to the top level such that it can be replaced by something else (e.g. cooperative thread
+        // dispatcher) and is not intrinsic to this code.
         switch (component->m_stage) {
             // The first stage the active component triggers the "preamble" call before moving into the dispatching
             // stage of the component thread.
@@ -126,6 +128,8 @@ namespace Fw {
     void ActiveComponentBase::s_taskLoop(void* component_pointer) {
         FW_ASSERT(component_pointer != nullptr);
         ActiveComponentBase* component = static_cast<ActiveComponentBase*>(component_pointer);
+        // A non-cooperative task switching implementation is just a while-loop around the active component
+        // state-machine. Here the while loop is at top-level.
         while (component->m_stage != ActiveComponentBase::Lifecycle::DONE) {
             ActiveComponentBase::s_taskStateMachine(component);
         }

diff --git a/Os/Baremetal/File.cpp b/Os/Baremetal/File.cpp
diff --git a/Os/Baremetal/Task.cpp b/Os/Baremetal/Task.cpp
diff --git a/Os/CMakeLists.txt b/Os/CMakeLists.txt
@@ -89,13 +89,11 @@ if (FPRIME_USE_BAREMETAL_SCHEDULER)
             list(REMOVE_ITEM SOURCE_FILES "${ITER_ITEM}")
         endif()
     endforeach()
-    list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/File.cpp")
     list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/FileSystem.cpp")
     list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/IntervalTimer.cpp")
     list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/Mutex.cpp")
     list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/Queue.cpp")
     list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/SystemResources.cpp")
-    list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/Task.cpp")
 endif()
 register_fprime_module()
 require_fprime_implementation(Os/File)

diff --git a/Os/Posix/Task.cpp b/Os/Posix/Task.cpp
@@ -25,7 +25,7 @@ namespace Task {
     void* pthread_entry_wrapper(void* wrapper_pointer) {
         FW_ASSERT(wrapper_pointer != nullptr);
         Os::Task::TaskRoutineWrapper& wrapper = *reinterpret_cast<Os::Task::TaskRoutineWrapper*>(wrapper_pointer);
-            wrapper.run(&wrapper);
+        wrapper.run(&wrapper);
         return nullptr;
     }
 

diff --git a/Os/Task.cpp b/Os/Task.cpp
@@ -9,15 +9,13 @@ namespace Os {
 
 TaskInterface::Arguments::Arguments(const Fw::StringBase &name, const Os::TaskInterface::taskRoutine routine,
                                     void * const routine_argument, const FwSizeType priority,
-                                    const FwSizeType stackSize, const FwSizeType cpuAffinity,
-                                    const PlatformUIntType identifier) :
+                                    const FwSizeType stackSize, const FwSizeType cpuAffinity) :
     m_name(name),
     m_routine(routine),
     m_routine_argument(routine_argument),
     m_priority(priority),
     m_stackSize(stackSize),
-    m_cpuAffinity(cpuAffinity),
-    m_identifier(identifier)
+    m_cpuAffinity(cpuAffinity)
 {
     FW_ASSERT(routine != nullptr);
 }
@@ -80,14 +78,12 @@ Task::State Task::getState() {
 }
 
 Task::Status Task::start(const Fw::StringBase &name, const taskRoutine routine, void* const arg,
-                         const ParamType priority, const ParamType stackSize, const ParamType cpuAffinity,
-                         const ParamType identifier) {
+                         const ParamType priority, const ParamType stackSize, const ParamType cpuAffinity) {
     FW_ASSERT(routine != nullptr);
     return this->start(Task::Arguments(name, routine, arg,
                                        priority,
                                        stackSize,
-                                       cpuAffinity,
-                                       static_cast<PlatformUIntType>(identifier)));
+                                       cpuAffinity));
 }
 
 
@@ -168,7 +164,10 @@ TaskHandle* Task::getHandle() {
 }
 
 FwSizeType Task::getNumTasks() {
-    return Task::s_numTasks;
+    Task::s_taskMutex.lock();
+    FwSizeType num_tasks = Task::s_numTasks;
+    Task::s_taskMutex.unlock();
+    return num_tasks;
 }
 
 void Task::registerTaskRegistry(TaskRegistry* registry) {

diff --git a/Os/Task.hpp b/Os/Task.hpp
@@ -69,14 +69,12 @@ namespace Os {
                 //! \param routine_argument: (optional) argument to supply to the task routine
                 //! \param priority: (optional) priority of this task
                 //! \param stackSize: (optional) size of stack supplied to this task
-                //! \param cpuAffinity: (optional) cpu affinity of this task. TODO: fix this into an array
-                //! \param identifier: (optional) identifier for this task
+                //! \param cpuAffinity: (optional) cpu affinity of this task.
                 Arguments(const Fw::StringBase& name, const taskRoutine routine,
                           void* const routine_argument = nullptr,
                           const FwSizeType priority = TASK_DEFAULT,
                           const FwSizeType stackSize = TASK_DEFAULT,
-                          const FwSizeType cpuAffinity = TASK_DEFAULT,
-                          const PlatformUIntType identifier = static_cast<PlatformUIntType>(TASK_DEFAULT));
+                          const FwSizeType cpuAffinity = TASK_DEFAULT);
 
               public:
                 const Os::TaskString m_name;
@@ -85,7 +83,6 @@ namespace Os {
                 FwSizeType m_priority;
                 FwSizeType m_stackSize;
                 FwSizeType m_cpuAffinity;
-                PlatformUIntType m_identifier;
             };
 
             //! \brief default constructor
@@ -245,8 +242,8 @@ namespace Os {
 
         //! \brief start this task
         //!
-        //! Start this task with supplied name, task routine (run function), priority, stack, affinity, and task
-        //! identifier. These arguments are supplied into an Arguments class and that version of the function is called.
+        //! Start this task with supplied name, task routine (run function), priority, stack, and affinity. These
+        //! arguments are supplied into an Arguments class and that version of the function is called.
         //! It is illegal to supply a nullptr as routine.
         //!
         //! \param name: name of the task to start
@@ -255,13 +252,11 @@ namespace Os {
         //! \param priority: (optional) priority of this task
         //! \param stackSize: (optional) stack size of this task
         //! \param cpuAffinity: (optional) affinity of this task. Use `Task::start(Arguments&)` to supply affinity set.
-        //! \param identifier: (optional) identifier of this task
         //! \return: status of the start call
         DEPRECATED(Status start(const Fw::StringBase &name, const taskRoutine routine, void* const arg = nullptr,
                                 const ParamType priority = TASK_DEFAULT,
                                 const ParamType stackSize = TASK_DEFAULT,
-                                const ParamType cpuAffinity = TASK_DEFAULT,
-                                const ParamType identifier = TASK_DEFAULT), "Switch to Task::start(Arguments&)");
+                                const ParamType cpuAffinity = TASK_DEFAULT), "Switch to Task::start(Arguments&)");
 
         //! \brief start the task
         //!
@@ -279,6 +274,9 @@ namespace Os {
         void onStart() override;
 
         //! \brief invoke the task's routine
+        //~
+        //! This will invoke the task's routine passing this as the argument to that call. This is used as a helper when
+        //! running this task (e.g. repetitive cooperative calls).
         void invokeRoutine();
 
         //! \brief join calling thread to this thread
@@ -339,7 +337,6 @@ namespace Os {
         Mutex m_lock; //!< Guards state transitions
         TaskRoutineWrapper m_wrapper; //!< Concrete storage for task routine wrapper
 
-        PlatformIntType m_identifier = 0; //!< thread independent identifier
         bool m_registered = false; //!< Was this task registered
 
         // This section is used to store the implementation-defined file handle. To Os::File and fprime, this type is

diff --git a/Os/TaskCommon.cpp b/Os/TaskCommon.cpp