From ccf9215f0ac65a2bba333759f75897923a0b770a Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Mon, 12 Apr 2021 11:01:38 -0400 Subject: [PATCH] Fix #957, move async console option Puts the "async" option into the shared layer instead of the impl layer. This allows both options to be coverage tested and also allows a bit more of the logic to be common instead of duplicated in the 3 implementations. This also adds back an osconfig option to allow the user to elect this mode at configuration time. --- default_config.cmake | 25 +++++++++++++++++++ osconfig.h.in | 1 + src/os/posix/inc/os-impl-console.h | 1 - src/os/posix/src/os-impl-console.c | 22 ++++++---------- src/os/rtems/inc/os-impl-console.h | 1 - src/os/rtems/src/os-impl-console.c | 25 +++++++------------ src/os/shared/inc/os-shared-printf.h | 9 +++---- src/os/shared/src/osapi-printf.c | 22 +++++++++++++++- src/os/vxworks/inc/os-impl-console.h | 1 - src/os/vxworks/src/os-impl-console.c | 20 +++++---------- .../shared/src/coveragetest-printf.c | 23 ++++++++++++----- .../vxworks/adaptors/inc/ut-adaptor-console.h | 5 ---- .../vxworks/adaptors/src/ut-adaptor-console.c | 5 ---- .../vxworks/src/coveragetest-console.c | 18 +++++++------ 14 files changed, 101 insertions(+), 77 deletions(-) diff --git a/default_config.cmake b/default_config.cmake index f71dafbc5..2ba2c0d1d 100644 --- a/default_config.cmake +++ b/default_config.cmake @@ -180,6 +180,31 @@ set(OSAL_CONFIG_DEBUG_PRINTF FALSE CACHE BOOL "Controls inclusion of OS_DEBUG statements in the code" ) +# +# OS_CONFIG_CONSOLE_ASYNC +# ---------------------------------- +# +# Controls whether the console device writes (OS_printf) will be deferred +# to a separate utility task or handled directly by the calling task. +# +# If set FALSE, the utility task WILL NOT be spawned, and all OS_printf() +# calls will be synchronously written to the console device. +# +# If set TRUE, an extra utility task WILL be spawned, and the data from +# all OS_printf() calls will be written to an output queue which is then +# transferred to the console device by the utility task. +# +# When this is TRUE (default), it may improve real time performance by not +# requiring the caller to delay on a potentially slow console device output. +# +# However decoupling in this manner requires creation of an extra task and +# stack to handle the output, and a side effect is that the OS_printf() output +# can become decoupled from the event/task where it actually occurred, or +# messages might appear in a different order than they originally occurred. +# +set(OSAL_CONFIG_CONSOLE_ASYNC TRUE + CACHE BOOL "Controls spawning of a separate utility task for OS_printf" +) ############################################# # Resource Limits for the OS API diff --git a/osconfig.h.in b/osconfig.h.in index a7f947b18..fc1710b2d 100644 --- a/osconfig.h.in +++ b/osconfig.h.in @@ -44,6 +44,7 @@ #cmakedefine OSAL_CONFIG_INCLUDE_SHELL #cmakedefine OSAL_CONFIG_DEBUG_PRINTF #cmakedefine OSAL_CONFIG_DEBUG_PERMISSIVE_MODE +#cmakedefine OSAL_CONFIG_CONSOLE_ASYNC #cmakedefine OSAL_CONFIG_BUGCHECK_DISABLE #cmakedefine OSAL_CONFIG_BUGCHECK_STRICT diff --git a/src/os/posix/inc/os-impl-console.h b/src/os/posix/inc/os-impl-console.h index ac0140053..73faaedb6 100644 --- a/src/os/posix/inc/os-impl-console.h +++ b/src/os/posix/inc/os-impl-console.h @@ -36,7 +36,6 @@ /* Console device */ typedef struct { - bool is_async; sem_t data_sem; } OS_impl_console_internal_record_t; diff --git a/src/os/posix/src/os-impl-console.c b/src/os/posix/src/os-impl-console.c index 672770fc7..0dff3e863 100644 --- a/src/os/posix/src/os-impl-console.c +++ b/src/os/posix/src/os-impl-console.c @@ -68,16 +68,9 @@ void OS_ConsoleWakeup_Impl(const OS_object_token_t *token) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); - if (local->is_async) - { - /* post the sem for the utility task to run */ - sem_post(&local->data_sem); - } - else - { - /* output directly */ - OS_ConsoleOutput_Impl(token); - } + /* post the sem for the utility task to run */ + sem_post(&local->data_sem); + } /* end OS_ConsoleWakeup_Impl */ /*---------------------------------------------------------------- @@ -121,18 +114,19 @@ static void *OS_ConsoleTask_Entry(void *arg) int32 OS_ConsoleCreate_Impl(const OS_object_token_t *token) { OS_impl_console_internal_record_t *local; + OS_console_internal_record_t * console; pthread_t consoletask; int32 return_code; OS_VoidPtrValueWrapper_t local_arg = {0}; - local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); + console = OS_OBJECT_TABLE_GET(OS_console_table, *token); + local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); if (token->obj_idx == 0) { - return_code = OS_SUCCESS; - local->is_async = OS_CONSOLE_ASYNC; + return_code = OS_SUCCESS; - if (local->is_async) + if (console->IsAsync) { if (sem_init(&local->data_sem, 0, 0) < 0) { diff --git a/src/os/rtems/inc/os-impl-console.h b/src/os/rtems/inc/os-impl-console.h index b015783a9..1ad387fa3 100644 --- a/src/os/rtems/inc/os-impl-console.h +++ b/src/os/rtems/inc/os-impl-console.h @@ -36,7 +36,6 @@ /* Console device */ typedef struct { - bool is_async; sem_t data_sem; } OS_impl_console_internal_record_t; diff --git a/src/os/rtems/src/os-impl-console.c b/src/os/rtems/src/os-impl-console.c index 7aa2622e3..08e83c9b3 100644 --- a/src/os/rtems/src/os-impl-console.c +++ b/src/os/rtems/src/os-impl-console.c @@ -67,7 +67,6 @@ /* Console device */ typedef struct { - bool is_async; rtems_id data_sem; int out_fd; } OS_impl_console_internal_record_t; @@ -93,16 +92,9 @@ void OS_ConsoleWakeup_Impl(const OS_object_token_t *token) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); - if (local->is_async) - { - /* post the sem for the utility task to run */ - rtems_semaphore_release(local->data_sem); - } - else - { - /* output directly */ - OS_ConsoleOutput_Impl(token); - } + /* post the sem for the utility task to run */ + rtems_semaphore_release(local->data_sem); + } /* end OS_ConsoleWakeup_Impl */ /*---------------------------------------------------------------- @@ -143,20 +135,21 @@ static void OS_ConsoleTask_Entry(rtems_task_argument arg) int32 OS_ConsoleCreate_Impl(const OS_object_token_t *token) { OS_impl_console_internal_record_t *local; + OS_console_internal_record_t * console; int32 return_code; rtems_name r_name; rtems_id r_task_id; rtems_status_code status; - local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); + local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); + console = OS_OBJECT_TABLE_GET(OS_console_table, *token); if (OS_ObjectIndexFromToken(token) == 0) { - return_code = OS_SUCCESS; - local->is_async = OS_CONSOLE_ASYNC; - local->out_fd = OSAL_CONSOLE_FILENO; + return_code = OS_SUCCESS; + local->out_fd = OSAL_CONSOLE_FILENO; - if (local->is_async) + if (console->IsAsync) { OS_DEBUG("%s(): Starting Async Console Handler\n", __func__); /* diff --git a/src/os/shared/inc/os-shared-printf.h b/src/os/shared/inc/os-shared-printf.h index b1cce4fcc..e221016ae 100644 --- a/src/os/shared/inc/os-shared-printf.h +++ b/src/os/shared/inc/os-shared-printf.h @@ -50,6 +50,7 @@ typedef struct volatile size_t ReadPos; /**< Offset of next byte to read */ volatile size_t WritePos; /**< Offset of next byte to write */ uint32 OverflowEvents; /**< Number of lines dropped due to overflow */ + bool IsAsync; } OS_console_internal_record_t; @@ -89,17 +90,15 @@ int32 OS_ConsoleCreate_Impl(const OS_object_token_t *token); void OS_ConsoleOutput_Impl(const OS_object_token_t *token); /*---------------------------------------------------------------- - Function: OS_ConsoleOutput_Impl + Function: OS_ConsoleWakeup_Impl Purpose: Console output data notification This is a notification API that is invoked whenever there is new data available in the console output buffer. - For a synchronous console service, this may call - OS_ConsoleWrite_Impl() directly. For an async console - service, this should wakeup the actual console servicing - thread. + It is only used of the console is configured for async operation, + and it should wakeup the actual console servicing thread. ------------------------------------------------------------------*/ void OS_ConsoleWakeup_Impl(const OS_object_token_t *token); diff --git a/src/os/shared/src/osapi-printf.c b/src/os/shared/src/osapi-printf.c index a21f55b36..677fc8c3c 100644 --- a/src/os/shared/src/osapi-printf.c +++ b/src/os/shared/src/osapi-printf.c @@ -56,6 +56,16 @@ #include "os-shared-idmap.h" #include "os-shared-printf.h" +/* + * The choice of whether to run a separate utility task + * comes from osal compile-time config + */ +#ifdef OSAL_CONFIG_CONSOLE_ASYNC +#define OS_CONSOLE_IS_ASYNC true +#else +#define OS_CONSOLE_IS_ASYNC false +#endif + /* reserve buffer memory for the printf console device */ static char OS_printf_buffer_mem[(sizeof(OS_PRINTF_CONSOLE_NAME) + OS_BUFFER_SIZE) * OS_BUFFER_MSG_DEPTH]; @@ -99,6 +109,7 @@ int32 OS_ConsoleAPI_Init(void) */ console->BufBase = OS_printf_buffer_mem; console->BufSize = sizeof(OS_printf_buffer_mem); + console->IsAsync = OS_CONSOLE_IS_ASYNC; return_code = OS_ConsoleCreate_Impl(&token); @@ -234,7 +245,16 @@ int32 OS_ConsoleWrite(osal_id_t console_id, const char *Str) * This is done while still locked, so it can support * either a synchronous or asynchronous implementation. */ - OS_ConsoleWakeup_Impl(&token); + if (console->IsAsync) + { + /* post the sem for the utility task to run */ + OS_ConsoleWakeup_Impl(&token); + } + else + { + /* output directly */ + OS_ConsoleOutput_Impl(&token); + } OS_ObjectIdRelease(&token); } diff --git a/src/os/vxworks/inc/os-impl-console.h b/src/os/vxworks/inc/os-impl-console.h index f87c9203f..862225646 100644 --- a/src/os/vxworks/inc/os-impl-console.h +++ b/src/os/vxworks/inc/os-impl-console.h @@ -37,7 +37,6 @@ typedef struct { VX_COUNTING_SEMAPHORE(cmem); - bool is_async; SEM_ID datasem; TASK_ID taskid; } OS_impl_console_internal_record_t; diff --git a/src/os/vxworks/src/os-impl-console.c b/src/os/vxworks/src/os-impl-console.c index 9fc250814..bc5dd1664 100644 --- a/src/os/vxworks/src/os-impl-console.c +++ b/src/os/vxworks/src/os-impl-console.c @@ -74,19 +74,12 @@ void OS_ConsoleWakeup_Impl(const OS_object_token_t *token) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, *token); - if (local->is_async) + /* post the sem for the utility task to run */ + if (semGive(local->datasem) == ERROR) { - /* post the sem for the utility task to run */ - if (semGive(local->datasem) == ERROR) - { - OS_DEBUG("semGive() - vxWorks errno %d\n", errno); - } - } - else - { - /* output directly */ - OS_ConsoleOutput_Impl(token); + OS_DEBUG("semGive() - vxWorks errno %d\n", errno); } + } /* end OS_ConsoleWakeup_Impl */ /*---------------------------------------------------------------- @@ -142,10 +135,9 @@ int32 OS_ConsoleCreate_Impl(const OS_object_token_t *token) if (OS_ObjectIndexFromToken(token) == 0) { - return_code = OS_SUCCESS; - local->is_async = OS_CONSOLE_ASYNC; + return_code = OS_SUCCESS; - if (local->is_async) + if (console->IsAsync) { OS_DEBUG("%s(): Starting Async Console Handler\n", __func__); diff --git a/src/unit-test-coverage/shared/src/coveragetest-printf.c b/src/unit-test-coverage/shared/src/coveragetest-printf.c index e27780d1d..6592cd941 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-printf.c +++ b/src/unit-test-coverage/shared/src/coveragetest-printf.c @@ -64,7 +64,6 @@ void Test_OS_printf(void) * void OS_printf_disable(void); * void OS_printf_enable(void); */ - uint32 CallCount = 0; /* catch case where OS_printf called before init */ OS_SharedGlobalVars.PrintfConsoleId = OS_OBJECT_ID_UNDEFINED; @@ -80,17 +79,29 @@ void Test_OS_printf(void) UtAssert_True(OS_console_table[0].WritePos == 0, "WritePos (%lu) >= 0", (unsigned long)OS_console_table[0].WritePos); - /* normal case */ + /* normal case - sync mode */ + OS_console_table[0].IsAsync = false; OS_printf_enable(); - OS_printf("UnitTest3"); - CallCount = UT_GetStubCount(UT_KEY(OS_ConsoleWakeup_Impl)); - UtAssert_True(CallCount == 1, "OS_ConsoleWakeup_Impl() call count (%lu) == 1", (unsigned long)CallCount); - UtAssert_True(OS_console_table[0].WritePos >= 9, "WritePos (%lu) >= 9", + OS_printf("UnitTest3s"); + UtAssert_STUB_COUNT(OS_ConsoleWakeup_Impl, 0); + UtAssert_STUB_COUNT(OS_ConsoleOutput_Impl, 1); + UtAssert_True(OS_console_table[0].WritePos >= 10, "WritePos (%lu) >= 10", + (unsigned long)OS_console_table[0].WritePos); + + /* normal case - async mode */ + OS_console_table[0].IsAsync = true; + OS_console_table[0].WritePos = 0; + OS_printf("UnitTest3a"); + UtAssert_STUB_COUNT(OS_ConsoleWakeup_Impl, 1); + UtAssert_STUB_COUNT(OS_ConsoleOutput_Impl, 1); + UtAssert_True(OS_console_table[0].WritePos >= 10, "WritePos (%lu) >= 10", (unsigned long)OS_console_table[0].WritePos); /* print a long string that does not fit in the 16-char buffer */ OS_printf_enable(); OS_printf("UnitTest4BufferLengthExceeded"); + UtAssert_True(OS_console_table[0].OverflowEvents == 1, "OverflowEvents (%lu) == 1", + (unsigned long)OS_console_table[0].OverflowEvents); /* test writing with a non-empty console name */ strncpy(OS_console_table[0].device_name, "ut", sizeof(OS_console_table[0].device_name) - 1); diff --git a/src/unit-test-coverage/vxworks/adaptors/inc/ut-adaptor-console.h b/src/unit-test-coverage/vxworks/adaptors/inc/ut-adaptor-console.h index 73890d11b..83a1b47b1 100644 --- a/src/unit-test-coverage/vxworks/adaptors/inc/ut-adaptor-console.h +++ b/src/unit-test-coverage/vxworks/adaptors/inc/ut-adaptor-console.h @@ -39,9 +39,4 @@ extern size_t const UT_Ref_OS_impl_console_table_SIZE; */ extern void UT_ConsoleTest_TaskEntry(int arg); -/** - * Force the "is_async" field to a given state for coverage testing - */ -extern void UT_ConsoleTest_SetConsoleAsync(osal_index_t local_id, bool is_async); - #endif /* UT_ADAPTOR_CONSOLE_H */ diff --git a/src/unit-test-coverage/vxworks/adaptors/src/ut-adaptor-console.c b/src/unit-test-coverage/vxworks/adaptors/src/ut-adaptor-console.c index c69adb7ee..8909b537b 100644 --- a/src/unit-test-coverage/vxworks/adaptors/src/ut-adaptor-console.c +++ b/src/unit-test-coverage/vxworks/adaptors/src/ut-adaptor-console.c @@ -39,8 +39,3 @@ void UT_ConsoleTest_TaskEntry(int arg) { OS_VxWorks_ConsoleTask_Entry(arg); } - -void UT_ConsoleTest_SetConsoleAsync(osal_index_t local_id, bool is_async) -{ - OS_impl_console_table[local_id].is_async = is_async; -} diff --git a/src/unit-test-coverage/vxworks/src/coveragetest-console.c b/src/unit-test-coverage/vxworks/src/coveragetest-console.c index 1865b4afa..0746f74fc 100644 --- a/src/unit-test-coverage/vxworks/src/coveragetest-console.c +++ b/src/unit-test-coverage/vxworks/src/coveragetest-console.c @@ -43,18 +43,13 @@ void Test_OS_ConsoleWakeup_Impl(void) */ OS_object_token_t token = UT_TOKEN_0; - /* no return code - check for coverage */ - UT_ConsoleTest_SetConsoleAsync(0, true); + /* this just gives the sem, only called in async mode */ OS_ConsoleWakeup_Impl(&token); UtAssert_True(UT_GetStubCount(UT_KEY(OCS_semGive)) == 1, "semGive() called in async mode"); + /* Failure only causes a debug message to be generated, no error handling here */ UT_SetDefaultReturnValue(UT_KEY(OCS_semGive), -1); OS_ConsoleWakeup_Impl(&token); - - UT_ConsoleTest_SetConsoleAsync(0, false); - OS_console_table[0].WritePos = 1; - OS_ConsoleWakeup_Impl(&token); - UtAssert_True(UT_GetStubCount(UT_KEY(OS_ConsoleOutput_Impl)) == 1, "OS_ConsoleOutput_Impl() called in sync mode"); } void Test_OS_ConsoleCreate_Impl(void) @@ -63,8 +58,15 @@ void Test_OS_ConsoleCreate_Impl(void) memset(&token, 0, sizeof(token)); + /* Verify coverage when configured for sync mode */ + OS_console_table[0].IsAsync = false; + OSAPI_TEST_FUNCTION_RC(OS_ConsoleCreate_Impl(&token), OS_SUCCESS); + UtAssert_STUB_COUNT(OCS_taskSpawn, 0); /* Task _was not_ spawned */ + + /* Verify coverage when configured for async mode */ + OS_console_table[0].IsAsync = true; OSAPI_TEST_FUNCTION_RC(OS_ConsoleCreate_Impl(&token), OS_SUCCESS); - UtAssert_True(UT_GetStubCount(UT_KEY(OCS_taskSpawn)) == 1, "taskSpawn() called"); + UtAssert_STUB_COUNT(OCS_taskSpawn, 1); /* Task _was_ spawned */ UT_SetDefaultReturnValue(UT_KEY(OCS_semCInitialize), OCS_ERROR); OSAPI_TEST_FUNCTION_RC(OS_ConsoleCreate_Impl(&token), OS_SEM_FAILURE);