From dd9c7c44c998de7b9dc7a7e320c49f371153cc3d Mon Sep 17 00:00:00 2001 From: Cavin McKinley <30472644+mcknly@users.noreply.github.com> Date: Mon, 24 Jun 2024 13:46:52 -0500 Subject: [PATCH] Services refactor (#17) * Unify service arrays into a simple struct array (#14) * Group service arrays into simple struct * Use size_t and fixed typo * Added some additional documentation to support new service descriptors structure, bump BBOS version to 0.2 * Minor comments & formatting --------- Co-authored-by: nbes4 <80783337+nbes4@users.noreply.github.com> --- cli/node_bin.c | 22 +++++----- services/CMakeLists.txt | 1 + services/services.c | 55 +++++++++++++++++++++++++ services/services.h | 82 +++++++++++++++++++------------------- services/taskman_service.c | 11 +++-- version.h | 2 +- 6 files changed, 114 insertions(+), 59 deletions(-) create mode 100644 services/services.c diff --git a/cli/node_bin.c b/cli/node_bin.c index c89b2e2..51f40ad 100644 --- a/cli/node_bin.c +++ b/cli/node_bin.c @@ -226,14 +226,14 @@ static void service_exec_callback(struct ush_object *self, struct ush_file_descr shell_print(err_msg); } else { - // Iterate through service_strings array to find the matching service index + // Iterate through service_descriptors array to find the matching service index int i; - for (i = 0; i < (sizeof(service_strings)/sizeof(service_strings[0])); i++) { - if (strcmp(argv[2], service_strings[i]) == 0) { - service_functions[i](); // call the function pointer at the same index of the matched service string + for(i = 0; i < service_descriptors_length; i++) { + if (strcmp(argv[2], service_descriptors[i].name) == 0) { + service_descriptors[i].service_func(); // call the function pointer for the matching service break; } - if (i == ((sizeof(service_strings)/sizeof(service_strings[0]))-1)) { + if (i == (service_descriptors_length - 1)) { sprintf(err_msg, "%s is not an available service, try 'service list'", argv[2]); shell_print(err_msg); break; @@ -276,15 +276,17 @@ static void service_exec_callback(struct ush_object *self, struct ush_file_descr "Available Services\tStatus\r\n" "------------------------------------\r\n" USH_SHELL_FONT_STYLE_RESET; - char *service_list_msg = pvPortMalloc(strlen(service_list_header) + (sizeof(service_strings) * (configMAX_TASK_NAME_LEN + 16))); + char *service_list_msg = pvPortMalloc(strlen(service_list_header) + + (service_descriptors_length * + (configMAX_TASK_NAME_LEN + 16))); // add 16 bytes per line for service state and whitespace TaskHandle_t service_taskhandle; char service_state[12]; strcpy(service_list_msg, service_list_header); // interate through available services, get their states from RTOS - for (i = 0; i < (sizeof(service_strings)/sizeof(service_strings[0])); i++) { - service_taskhandle = xTaskGetHandle(service_strings[i]); + for (i = 0; i < service_descriptors_length; i++) { + service_taskhandle = xTaskGetHandle(service_descriptors[i].name); if (service_taskhandle == NULL) { strcpy(service_state, "not started"); } @@ -306,9 +308,9 @@ static void service_exec_callback(struct ush_object *self, struct ush_file_descr } // copy current service name into table - strcpy(service_list_msg + strlen(service_list_msg), service_strings[i]); + strcpy(service_list_msg + strlen(service_list_msg), service_descriptors[i].name); // add an extra tab to short service names to make the table look better - if (strlen(service_strings[i]) < (configMAX_TASK_NAME_LEN - 8)) { + if (strlen(service_descriptors[i].name) < (configMAX_TASK_NAME_LEN - 8)) { strcpy(service_list_msg + strlen(service_list_msg), "\t"); } // copy current service state into table diff --git a/services/CMakeLists.txt b/services/CMakeLists.txt index 7a5e23b..2df3d0b 100644 --- a/services/CMakeLists.txt +++ b/services/CMakeLists.txt @@ -1,5 +1,6 @@ # add source files to the top-level project target_sources(${PROJ_NAME} PRIVATE + services.c service_queues.c cli_service.c usb_service.c diff --git a/services/services.c b/services/services.c new file mode 100644 index 0000000..080113a --- /dev/null +++ b/services/services.c @@ -0,0 +1,55 @@ +/****************************************************************************** + * @file services.c + * + * @brief Defines the service descriptors for each system service declared in + * services.h. These service descriptors are used to associate a service + * name string with a service function pointer, as well as indicate if + * the service should be started at boot. The individual service function + * implementations and FreeRTOS APIs for registering the associated tasks + * are contained in the source files for the respective services. Queues + * used to pass data between services are implemented in service_queues.h. + * + * @author @nbes4 (github) + * + * @date 06-18-2024 + * + * @copyright Copyright (c) 2024 @nbes4 (github) + * Released under the MIT License + * + * SPDX-License-Identifier: MIT + ******************************************************************************/ + +#include "services.h" + +// see services.h for more information on creating services +// note: use xstr() to convert the SERVICE_NAME_ #define from services.h to a usable string +// (e.g. xstr(SERVICE_NAME_HEARTBEAT)) +const service_desc_t service_descriptors[] = { + { + .name = xstr(SERVICE_NAME_USB), + .service_func = usb_service, + .startup = true + }, + { + .name = xstr(SERVICE_NAME_CLI), + .service_func = cli_service, + .startup = true + }, + { + .name = xstr(SERVICE_NAME_STORMAN), + .service_func = storman_service, + .startup = true + }, + { + .name = xstr(SERVICE_NAME_WATCHDOG), + .service_func = watchdog_service, + .startup = true + }, + { + .name = xstr(SERVICE_NAME_HEARTBEAT), + .service_func = heartbeat_service, + .startup = false + } +}; + +const size_t service_descriptors_length = sizeof(service_descriptors)/sizeof(service_desc_t); \ No newline at end of file diff --git a/services/services.h b/services/services.h index cfeb1d9..b98b492 100644 --- a/services/services.h +++ b/services/services.h @@ -33,12 +33,12 @@ * any time via 'taskmanager', the base service. * * If a new service is added, a SERVICE_NAME must be defined for it, - * the service implementation and FreeRTOS task launcher must be + * and the service implementation and FreeRTOS task launcher must be * created in a separate source file (see "heartbeat_service.c" for - * an example), its function pointer needs to be added to the - * 'service_functions[]' array, string name added to 'service_strings[]' - * IN THE SAME ORDER, and then it can be added to 'startup_services[]' - * if it should run at boot. + * an example). The service's "descriptor" then needs to be added + * to the service_descriptors[] array in services.c, which associates + * the service's main function pointer with its name string, and + * defines whether the service should run at boot. * * The service schedule within FreeRTOS is determined by 3 parameters: * Priority, Repeat, and Delay. Upon any given scheduler tick, the OS @@ -49,6 +49,12 @@ * based on the DELAY parameter. This essentially means a task's run * percentage (within a priority level) is given by REPEAT/DELAY. * + * Pro tip: uncomment add_definitions(-DSCHED_TEST_DELAY) in the + * top-level CMakeLists.txt to burn extra cycles in each service, and + * then use the 'bin/top' CLI command to show FreeRTOS task runtime + * percentages to help tune the scheduler! Don't forget to comment + * this back out after testing! + * * Note that by default, 1 OS tick is 1 ms. * This can be changed in FreeRTOSConfig.h, see 'configTICK_RATE_HZ' *******************************************************************************/ @@ -101,6 +107,8 @@ // FreeRTOS stack sizes for the services - "stack" in this sense is dedicated heap memory for a task. // local variables within a service/task use this stack space. // If pvPortMalloc is called within a task, it will allocate directly from shared FreeRTOS heap. +// Use 'bin/ps' command to show a service's min stack (memory usage high water mark) +// to determine if too much/too little has been allocated. #define STACK_TASKMAN 512 #define STACK_CLI 1024 #define STACK_USB 1024 @@ -114,7 +122,8 @@ *************************/ // below are functions for launching the services. -// pointers to these are added to service_functions[]. +// pointers to these are added to a service's .service_func descriptor item in +// services.c /** * @brief Start the taskmanager service. @@ -203,44 +212,33 @@ BaseType_t heartbeat_service(void); /************************ - * Service Arrays + * Service Descriptors *************************/ -// function ptr array of available service functions above for launching with taskmanager. -// note that taskmanager itself is not in this list (it is a base service). -// the corresponding string in service_strings[] below will be used to match the service. -// service_functions[] and service_strings[] need to be in the same order! -typedef BaseType_t (*ServiceFunc_t)(void); -static const ServiceFunc_t service_functions[] = { - cli_service, - usb_service, - storman_service, - watchdog_service, - heartbeat_service -}; - -// string versions of service names, used when comparing against user input. -// use xstr() to convert the SERVICE_NAME_ #define to a usable string. -// the corresponding index number in the service_functions[] array above will be the -// function ptr to launch the service. -// service_functions[] and service_strings[] need to be in the same order! -static const char *service_strings[] = { - xstr(SERVICE_NAME_CLI), - xstr(SERVICE_NAME_USB), - xstr(SERVICE_NAME_STORMAN), - xstr(SERVICE_NAME_WATCHDOG), - xstr(SERVICE_NAME_HEARTBEAT) -}; - -// startup services - launched automatically by taskmanager at boot. -// these strings should match with what is in service_strings[] for the intended service. -// these can be in any order, the corresponding FreeRTOS tasked will be launched in this order. -static const char *startup_services[] = { - "usb", - "cli", - "storagemanager", - "watchdog" -}; +// service function pointer typedef +typedef BaseType_t (*service_func_t)(void); + +// service descriptor structure to hold the service name, service function pointer and startup flag +typedef struct service_desc_t { + // string version of the service name, used when comparing against user input + const char * const name; + + //Defines wether or not this service should be automatically launched by taskmanager at boot + const bool startup; + + //Function pointer to the service that creates the respective FreeRTOS task - declared in services.h + service_func_t service_func; +} service_desc_t; + +// holds all the services that can be launched with taskmanager. +// these can be in any order, the corresponding FreeRTOS tasks will be launched in this order. +// note: taskmanager itself is not in this array (it is a base service). +// edit services.c to add your own services! +extern const service_desc_t service_descriptors[]; + +// number of service descriptors in the array is used to iterate through services +// for startup and interaction +extern const size_t service_descriptors_length; #endif /* SERVICES_H */ \ No newline at end of file diff --git a/services/taskman_service.c b/services/taskman_service.c index 73f0ca2..b0d4f76 100644 --- a/services/taskman_service.c +++ b/services/taskman_service.c @@ -74,15 +74,14 @@ static void prvTaskManagerTask(void *pvParameters) // launch startup services cli_uart_puts(timestamp()); cli_uart_puts("Starting all bootup services...\r\n"); - int i, j; - for (i = 0; i < (sizeof(startup_services)/sizeof(startup_services[0])); i++) { - for (j = 0; j < (sizeof(service_strings)/sizeof(service_strings[0])); j++) { - if (strcmp(service_strings[j], startup_services[i]) == 0) { - service_functions[j](); - } + int i; + for (i = 0; i < service_descriptors_length; i++) { + if(service_descriptors[i].startup) { + service_descriptors[i].service_func(); } } + // At this point the system is fully running. Print a message cli_uart_puts(timestamp()); cli_uart_puts("All startup services launched.\r\n"); diff --git a/version.h b/version.h index ac0e95f..f9cf35a 100644 --- a/version.h +++ b/version.h @@ -19,7 +19,7 @@ // maintain BreadboardOS version so it can be tracked along with custom project #define BBOS_NAME "breadboard-os" // do not change #define BBOS_VERSION_MAJOR 0 // changed only for major release update -#define BBOS_VERSION_MINOR 1 // changed only for minor release update +#define BBOS_VERSION_MINOR 2 // changed only for minor release update // custom project name and version is defined in top level CMakeLists.txt // look for PROJ_NAME and PROJ_VER