Improved state machine API (#3)

* feat: Seperate SM & scheduler

* feat: Clean up scheduler API even more

Not working yet, just comitting to move machines!

* fix: Now it works!

* feat: Add docs

* feat: Remove state enum

* fix: format

* fix: format should NOT format build!

* chore: refactor state_info to just state

* fix: format

* fix: Update docs to reflect no enum

* fix: Get rid of superfluous wait

* fix: Const issue

---------

Co-authored-by: Niklas Vainio <niklasvainio8@gmail.com>

CMakeLists.txt

@@ -35,10 +35,12 @@ project(samwise C CXX ASM)
 pico_sdk_init()
 
 # Add executable. Default name is the project name, version 0.1
-file(GLOB sources CONFIGURE_DEPENDS "src/*.c")
-file(GLOB tasks CONFIGURE_DEPENDS "src/tasks/*.c")
-file(GLOB state_machine CONFIGURE_DEPENDS "src/state_machine/*.c")
-add_executable(samwise ${sources} ${tasks} ${state_machine})
+file(GLOB top CONFIGURE_DEPENDS "src/*.c")
+file(GLOB scheduler CONFIGURE_DEPENDS "src/scheduler/*.c")
+file(GLOB state_machine_tasks CONFIGURE_DEPENDS "src/state_machine/tasks/*.c")
+file(GLOB state_machine_states CONFIGURE_DEPENDS "src/state_machine/states/*.c")
+add_executable(samwise ${top} ${scheduler} ${state_machine_tasks}
+  ${state_machine_states})
 
 pico_set_program_name(samwise "samwise")
 pico_set_program_version(samwise "0.1")
@@ -57,5 +59,7 @@ target_include_directories(samwise PRIVATE
   ${CMAKE_CURRENT_LIST_DIR}/.. # for our common lwipopts or any other standard includes, if required
 )
 
+target_include_directories(samwise PRIVATE ${CMAKE_CURRENT_LIST_DIR}/src)
+
 pico_add_extra_outputs(samwise)
 

format_all.sh

@@ -1,4 +1,4 @@
 # Helper script to recursively clang-format every file
 echo Formatting all files...
-find . -name "*.cpp" -o -name "*.c" -o -name "*.h" | xargs -I {} clang-format -i {}
-echo Done!
+find src -name "*.cpp" -o -name "*.c" -o -name "*.h" | xargs -I {} clang-format -i {}
+echo Done!

src/init.c

@@ -10,7 +10,7 @@
 #include "init.h"
 #include "macros.h"
 #include "pico/stdlib.h"
-#include "state_machine/state_machine.h"
+#include "scheduler/scheduler.h"
 
 /**
  * Initialize all gpio pins to their default states.
@@ -41,7 +41,7 @@ bool init(slate_t *slate)
     /*
      * Initialize the state machine
      */
-    sm_init(slate);
-    
+    sched_init(slate);
+
     return true;
-}
+}

src/main.c

@@ -8,8 +8,8 @@
 #include "init.h"
 #include "macros.h"
 #include "pico/stdlib.h"
+#include "scheduler/scheduler.h"
 #include "slate.h"
-#include "state_machine/state_machine.h"
 
 /**
  * Statically allocate the slate.
@@ -45,20 +45,18 @@ int main()
     ASSERT(init(&slate));
     LOG_INFO("main: Initialized successfully!\n\n\n");
 
-    sleep_ms(5000);
-
     /*
      * Go state machine!
      */
     LOG_INFO("main: Dispatching the state machine...");
     while (true)
     {
-        sm_dispatch(&slate);
+        sched_dispatch(&slate);
     }
 
     /*
      * We should NEVER be here so something bad has happened.
      * @todo reboot!
      */
     ERROR("We reached the end of the code - this is REALLY BAD!");
-}
+}

src/state_machine/state_machine.c → src/scheduler/scheduler.c

@@ -6,37 +6,62 @@
  * scheduling and dispatching tasks on the satellite.
  */
 
-#include "state_machine.h"
-#include "../macros.h"
-#include "../slate.h"
+#include "scheduler.h"
+#include "macros.h"
 #include "pico/time.h"
-#include "state_machine_states.h"
+#include "slate.h"
+
+/*
+ * Include the actual state machine
+ */
+#include "state_machine/states/states.h"
+#include "state_machine/tasks/tasks.h"
+
+static size_t n_tasks = 0;
+static sched_task_t *all_tasks[num_states * MAX_TASKS_PER_STATE];
 
 /**
  * Initialize the state machine.
  *
  * @param slate     Pointer to the slate.
  */
-void sm_init(slate_t *slate)
+void sched_init(slate_t *slate)
 {
     /*
-     * Check that each state has a valid number of tasks.
+     * Check that each state has a valid number of tasks, and enumerate all
+     * tasks.
      */
     for (size_t i = 0; i < num_states; i++)
     {
-        ASSERT(all_states[i].num_tasks <= MAX_TASKS_PER_STATE);
+        ASSERT(all_states[i]->num_tasks <= MAX_TASKS_PER_STATE);
+        for (size_t j = 0; j < all_states[i]->num_tasks; j++)
+        {
+            bool is_duplicate = 0;
+            for (size_t k = 0; k < n_tasks; k++)
+            {
+                if (all_tasks[k] == all_states[i]->task_list[j])
+                    is_duplicate = 1;
+            }
+            if (!is_duplicate)
+            {
+                all_tasks[n_tasks] = all_states[i]->task_list[j];
+                n_tasks++;
+            }
+        }
     }
 
+    LOG_INFO("sched: Enumerated %d tasks", n_tasks);
+
     /*
      * Initialize all tasks.
      */
-    for (size_t i = 0; i < NUM_TASKS; i++)
+    for (size_t i = 0; i < n_tasks; i++)
     {
-        LOG_INFO("sm: Initializing task %s", all_tasks[i]->name);
+        LOG_INFO("sched: Initializing task %s", all_tasks[i]->name);
         all_tasks[i]->task_init(slate);
     }
 
-    for (size_t i = 0; i < NUM_TASKS; i++)
+    for (size_t i = 0; i < n_tasks; i++)
     {
         all_tasks[i]->next_dispatch =
             make_timeout_time_ms(all_tasks[i]->dispatch_period_ms);
@@ -45,11 +70,11 @@ void sm_init(slate_t *slate)
     /*
      * Enter the init state by default
      */
-    slate->current_state = state_init;
+    slate->current_state = initial_state;
     slate->entered_current_state_time = get_absolute_time();
     slate->time_in_current_state_ms = 0;
 
-    LOG_DEBUG("sm: Done initializing!");
+    LOG_DEBUG("sched: Done initializing!");
 }
 
 /**
@@ -58,17 +83,16 @@ void sm_init(slate_t *slate)
  *
  * @param slate     Pointer to the slate.
  */
-void sm_dispatch(slate_t *slate)
+void sched_dispatch(slate_t *slate)
 {
-    sm_state_t current_state = slate->current_state;
-    sm_state_info_t *current_state_info = all_states + slate->current_state;
+    sched_state_t *current_state_info = slate->current_state;
 
     /*
      * Loop through all of this state's tasks
      */
     for (size_t i = 0; i < current_state_info->num_tasks; i++)
     {
-        sm_task_t *task = current_state_info->task_list[i];
+        sched_task_t *task = current_state_info->task_list[i];
 
         /*
          * Check if this task is due and if so, dispatch it
@@ -78,7 +102,7 @@ void sm_dispatch(slate_t *slate)
             task->next_dispatch =
                 make_timeout_time_ms(task->dispatch_period_ms);
 
-            LOG_DEBUG("sm: Dispatching task %s", task->name);
+            LOG_DEBUG("sched: Dispatching task %s", task->name);
             task->task_dispatch(slate);
         }
     }
@@ -91,10 +115,10 @@ void sm_dispatch(slate_t *slate)
     /*
      * Transition to the next state, if required.
      */
-    const sm_state_t next_state = current_state_info->get_next_state(slate);
-    if (next_state != current_state)
+    sched_state_t * const next_state = current_state_info->get_next_state(slate);
+    if (next_state != current_state_info)
     {
-        LOG_INFO("sm: Transitioning to state %s", all_states[next_state].name);
+        LOG_INFO("sched: Transitioning to state %s", next_state->name);
 
         slate->current_state = next_state;
         slate->entered_current_state_time = get_absolute_time();

src/state_machine/state_machine.h → src/scheduler/scheduler.h

@@ -8,29 +8,18 @@
 
 #pragma once
 
-#include "../slate.h"
 #include "pico/types.h"
 
 #define MAX_TASKS_PER_STATE 10
 
-/**
- * A list of all states the state machine can be in. Each state dispatches a set
- * of tasks at regular intervals.
- */
-typedef enum sm_state
-{
-    state_init, /* Entered by default at boot */
-    state_running,
-
-    /* Auto-updates to the number of sates */
-    num_states
-
-} sm_state_t;
+struct samwise_slate;
+typedef struct samwise_slate slate_t;
 
 /**
- * Holds the info for a single task. A single task can belong to multiple states.
+ * Holds the info for a single task. A single task can belong to multiple
+ * states.
  */
-typedef struct sm_task
+typedef struct sched_task
 {
     /**
      * Friendly name for the task.
@@ -59,28 +48,28 @@ typedef struct sm_task
      */
     void (*task_dispatch)(slate_t *slate);
 
-} sm_task_t;
+} sched_task_t;
 
 /**
  * Holds the info for defining a state.
  */
-typedef struct sm_state_info
+typedef struct sched_state
 {
     /**
      * Friendly name for the state.
      */
     const char *name;
 
     size_t num_tasks;
-    sm_task_t *task_list[MAX_TASKS_PER_STATE];
+    sched_task_t *task_list[MAX_TASKS_PER_STATE];
 
     /**
      * Called each time the state dispatches.
      * @param slate     Pointer to the current satellite slate
      * @return The next state to transition to
      */
-    sm_state_t (*get_next_state)(slate_t *slate);
-} sm_state_info_t;
+    struct sched_state *(*get_next_state)(slate_t *slate);
+} sched_state_t;
 
-void sm_init(slate_t *slate);
-void sm_dispatch(slate_t *slate);
+void sched_init(slate_t *slate);
+void sched_dispatch(slate_t *slate);

src/slate.h

@@ -12,15 +12,16 @@
 #pragma once
 
 #include "pico/types.h"
+#include "scheduler/scheduler.h"
 
 typedef struct samwise_slate
 {
     /*
      * State machine info.
      */
-    uint32_t current_state;
+    sched_state_t *current_state;
     absolute_time_t entered_current_state_time;
     uint64_t time_in_current_state_ms;
 
     bool led_state;
-} slate_t;
+} slate_t;

src/state_machine/ADDING_STATES_AND_TASKS.md

@@ -0,0 +1,71 @@
+# Adding a new state
+
+## Add your state code to `states/mystate.c` and `states/mystate.h`
+
+Here you will define your transition condition. For example, if you always want
+to transition, you can write:
+
+```c
+sm_state_t my_state_get_next_state(slate_t *slate)
+{
+    return next_state;
+}
+
+sched_state_info_t my_state_info  = {
+     .name = "my state",
+     .num_tasks = 1,
+     .task_list = {&my_task, &blink_task},
+     .get_next_state = &my_task_get_next_state}
+```
+
+If you need to reference another state (e.g. to transition) or task you should
+include the top level list headers `state_machine/tasks.h` and
+`state_machine/states.h`, not the headers for each task and state.
+
+## Add your state to `states.h`
+
+First add your state to the extern list:
+
+```c
+extern sched_state_info_t init_state_info;
+extern sched_state_info_t running_state_info;
+extern sched_state_info_t my_state_info;
+```
+
+Then, add your state to the array:
+
+```c
+static sched_state_info_t* all_states[] = {
+    /* state_init */
+    &init_state_info,
+    /* state_running */
+    &running_state_info,
+    /* my_state */
+    &my_state_info};
+```
+
+# Adding a new task
+
+## Add your task code to `tasks/mytask.c` and `tasks/mytask.h`
+
+See the other tasks for details, most importantly you will need something like:
+
+```c
+sched_task_t my_task = {.name = "my task",
+                        .dispatch_period_ms = 1000,
+                        .task_init = &my_task_init,
+                        .task_dispatch = &my_task_dispatch,
+
+                        /* Set to an actual value on init */
+                        .next_dispatch = 0};
+```
+
+## Add your task to `tasks.h`
+
+```c
+extern sched_task_t print_task;
+extern sched_task_t blink_task;
+extern sched_task_t my_task;
+```
+
+Now you can use your task in states!