From f9414bda39c0c2b70143be75262ca96038df43f4 Mon Sep 17 00:00:00 2001 From: Jacob Hageman Date: Thu, 18 Mar 2021 13:52:35 -0400 Subject: [PATCH] Fix #846, Minor clean up and clarification in comments/naming --- src/os/inc/osapi-dir.h | 2 +- src/os/inc/osapi-select.h | 4 ++++ src/os/inc/osapi-task.h | 1 + src/os/portable/os-impl-bsd-select.c | 31 ++++++++++++++------------- src/os/portable/os-impl-posix-dirs.c | 2 +- src/os/shared/src/osapi-common.c | 12 +---------- src/os/shared/src/osapi-idmap.c | 2 +- src/os/shared/src/osapi-printf.c | 5 +---- src/os/shared/src/osapi-queue.c | 6 +++--- src/os/shared/src/osapi-select.c | 15 +++++++++++++ src/os/vxworks/src/os-impl-console.c | 1 + src/os/vxworks/src/os-impl-tasks.c | 2 +- src/os/vxworks/src/os-impl-timebase.c | 4 ++-- 13 files changed, 48 insertions(+), 39 deletions(-) diff --git a/src/os/inc/osapi-dir.h b/src/os/inc/osapi-dir.h index a9c594dcc..d29a1f196 100644 --- a/src/os/inc/osapi-dir.h +++ b/src/os/inc/osapi-dir.h @@ -121,7 +121,7 @@ int32 OS_mkdir(const char *path, uint32 access); /** * @brief Removes a directory from the file system. * - * Removes a directory from the structure. + * Removes a directory from the structure. * The directory must be empty prior to this operation. * * @param[in] path The directory to remove diff --git a/src/os/inc/osapi-select.h b/src/os/inc/osapi-select.h index fefc18cd0..0d4ced4b3 100644 --- a/src/os/inc/osapi-select.h +++ b/src/os/inc/osapi-select.h @@ -34,6 +34,10 @@ * This is part of the select API and is manipulated using the * related API calls. It should not be modified directly by applications. * + * Note: Math is to determine uint8 array size needed to represent + * single bit OS_MAX_NUM_OPEN_FILES objects, + 7 rounds up + * and 8 is the size of uint8. + * * @sa OS_SelectFdZero(), OS_SelectFdAdd(), OS_SelectFdClear(), OS_SelectFdIsSet() */ typedef struct diff --git a/src/os/inc/osapi-task.h b/src/os/inc/osapi-task.h index acc7e1304..bc753a534 100644 --- a/src/os/inc/osapi-task.h +++ b/src/os/inc/osapi-task.h @@ -144,6 +144,7 @@ int32 OS_TaskInstallDeleteHandler(osal_task_entry function_pointer); * @brief Delay a task for specified amount of milliseconds * * Causes the current thread to be suspended from execution for the period of millisecond. + * This is a scheduled wait (clock_nanosleep/rtems_task_wake_after/taskDelay), not a "busy" wait. * * @param[in] millisecond Amount of time to delay * diff --git a/src/os/portable/os-impl-bsd-select.c b/src/os/portable/os-impl-bsd-select.c index dbfb19622..c74edb31e 100644 --- a/src/os/portable/os-impl-bsd-select.c +++ b/src/os/portable/os-impl-bsd-select.c @@ -72,7 +72,7 @@ * * returns: Highest numbered file descriptor in the output fd_set *-----------------------------------------------------------------*/ -static int OS_FdSet_ConvertIn_Impl(fd_set *os_set, OS_FdSet *OSAL_set) +static int OS_FdSet_ConvertIn_Impl(fd_set *OS_set, OS_FdSet *OSAL_set) { size_t offset; size_t bit; @@ -94,7 +94,7 @@ static int OS_FdSet_ConvertIn_Impl(fd_set *os_set, OS_FdSet *OSAL_set) osfd = OS_impl_filehandle_table[id].fd; if (osfd >= 0 && OS_impl_filehandle_table[id].selectable) { - FD_SET(osfd, os_set); + FD_SET(osfd, OS_set); if (osfd > maxfd) { maxfd = osfd; @@ -109,18 +109,19 @@ static int OS_FdSet_ConvertIn_Impl(fd_set *os_set, OS_FdSet *OSAL_set) return maxfd; } /* end OS_FdSet_ConvertIn_Impl */ -/*---------------------------------------------------------------- - * Function: OS_FdSet_ConvertOut_Impl +/*----------------------------------------------------------------*/ +/** + * \brief Convert a POSIX fd_set structure into an OSAL OS_FdSet + * which can then be returned back to the application. * - * Purpose: Local helper routine, not part of OSAL API. + * Local helper routine, not part of OSAL API. * - * Convert a POSIX fd_set structure into an OSAL OS_FdSet - * which can then be returned back to the application. + * This un-sets bits in OSAL_set that are set in the OS_set * - * This actually un-sets any bits in the "Input" parameter - * which are also set in the "output" parameter. + * \param[in] OS_set The fd_set from select + * \param[in, out] OSAL_set The OS_FdSet updated by this helper *-----------------------------------------------------------------*/ -static void OS_FdSet_ConvertOut_Impl(fd_set *output, OS_FdSet *Input) +static void OS_FdSet_ConvertOut_Impl(fd_set *OS_set, OS_FdSet *OSAL_set) { size_t offset; size_t bit; @@ -128,9 +129,9 @@ static void OS_FdSet_ConvertOut_Impl(fd_set *output, OS_FdSet *Input) uint8 objids; int osfd; - for (offset = 0; offset < sizeof(Input->object_ids); ++offset) + for (offset = 0; offset < sizeof(OSAL_set->object_ids); ++offset) { - objids = Input->object_ids[offset]; + objids = OSAL_set->object_ids[offset]; bit = 0; while (objids != 0) { @@ -138,9 +139,9 @@ static void OS_FdSet_ConvertOut_Impl(fd_set *output, OS_FdSet *Input) { id = OSAL_INDEX_C((offset * 8) + bit); osfd = OS_impl_filehandle_table[id].fd; - if (osfd < 0 || !FD_ISSET(osfd, output)) + if (osfd < 0 || !FD_ISSET(osfd, OS_set)) { - Input->object_ids[offset] &= ~(1 << bit); + OSAL_set->object_ids[offset] &= ~(1 << bit); } } ++bit; @@ -179,7 +180,7 @@ static int32 OS_DoSelect(int maxfd, fd_set *rd_set, fd_set *wr_set, int32 msecs) } else { - /* eliminates a false warning about possibly uninitialized use */ + /* Zero for consistency and to avoid possible confusion if not cleared */ memset(&ts_end, 0, sizeof(ts_end)); } diff --git a/src/os/portable/os-impl-posix-dirs.c b/src/os/portable/os-impl-posix-dirs.c index 05b246f6d..87467960d 100644 --- a/src/os/portable/os-impl-posix-dirs.c +++ b/src/os/portable/os-impl-posix-dirs.c @@ -82,7 +82,7 @@ int32 OS_DirCreate_Impl(const char *local_path, uint32 access) if (errno == EEXIST) { - /* it exists, but not necessarily a directory */ + /* Success if already exists and is a directory */ if (stat(local_path, &st) == 0 && S_ISDIR(st.st_mode)) { return_code = OS_SUCCESS; diff --git a/src/os/shared/src/osapi-common.c b/src/os/shared/src/osapi-common.c index 2848228c8..17158bf49 100644 --- a/src/os/shared/src/osapi-common.c +++ b/src/os/shared/src/osapi-common.c @@ -255,21 +255,11 @@ void OS_ApplicationExit(int32 Status) } } /* end OS_ApplicationExit */ -/*--------------------------------------------------------------------------------------- - Name: OS_CleanUpObject - - Purpose: Implements a single API call that can delete ANY object - Will dispatch to the correct delete implementation for that object type - - Returns: None - ----------------------------------------------------------------------------------------*/ - /*---------------------------------------------------------------- * * Function: OS_CleanUpObject * - * Purpose: Local helper routine, not part of OSAL API. + * Purpose: Local helper routine that can delete ANY object, not part of OSAL API. * *-----------------------------------------------------------------*/ void OS_CleanUpObject(osal_id_t object_id, void *arg) diff --git a/src/os/shared/src/osapi-idmap.c b/src/os/shared/src/osapi-idmap.c index 1e2496662..cf286595a 100644 --- a/src/os/shared/src/osapi-idmap.c +++ b/src/os/shared/src/osapi-idmap.c @@ -971,7 +971,7 @@ int32 OS_ObjectIdGetBySearch(OS_lock_mode_t lock_mode, osal_objtype_t idtype, OS /* * The "ConvertToken" routine will return with the global lock * in a state appropriate for returning to the caller, as indicated - * by the "check_mode" parameter. + * by the "lock_mode" parameter. */ return_code = OS_ObjectIdConvertToken(token); } diff --git a/src/os/shared/src/osapi-printf.c b/src/os/shared/src/osapi-printf.c index f3bcac68c..a21f55b36 100644 --- a/src/os/shared/src/osapi-printf.c +++ b/src/os/shared/src/osapi-printf.c @@ -281,10 +281,7 @@ void OS_printf(const char *String, ...) } else if (OS_SharedGlobalVars.PrintfEnabled) { - /* - * Call vsnprintf() to determine the actual size of the - * string we are going to write to the buffer after formatting. - */ + /* Format and determine the size of string to write */ va_start(va, String); actualsz = vsnprintf(msg_buffer, sizeof(msg_buffer), String, va); va_end(va); diff --git a/src/os/shared/src/osapi-queue.c b/src/os/shared/src/osapi-queue.c index f50278f8e..e8f8c4f46 100644 --- a/src/os/shared/src/osapi-queue.c +++ b/src/os/shared/src/osapi-queue.c @@ -87,7 +87,7 @@ int32 OS_QueueAPI_Init(void) * See description in API and header file for detail * *-----------------------------------------------------------------*/ -int32 OS_QueueCreate(osal_id_t *queue_id, const char *queue_name, osal_blockcount_t queue_depth, size_t data_size, +int32 OS_QueueCreate(osal_id_t *queue_id, const char *queue_name, osal_blockcount_t queue_depth, size_t max_size, uint32 flags) { int32 return_code; @@ -97,7 +97,7 @@ int32 OS_QueueCreate(osal_id_t *queue_id, const char *queue_name, osal_blockcoun /* validate inputs */ OS_CHECK_POINTER(queue_id); OS_CHECK_APINAME(queue_name); - OS_CHECK_SIZE(data_size); + OS_CHECK_SIZE(max_size); ARGCHECK(queue_depth <= OS_QUEUE_MAX_DEPTH, OS_QUEUE_INVALID_SIZE); /* Note - the common ObjectIdAllocate routine will lock the object type and leave it locked. */ @@ -110,7 +110,7 @@ int32 OS_QueueCreate(osal_id_t *queue_id, const char *queue_name, osal_blockcoun OS_OBJECT_INIT(token, queue, queue_name, queue_name); queue->max_depth = queue_depth; - queue->max_size = data_size; + queue->max_size = max_size; /* Now call the OS-specific implementation. This reads info from the queue table. */ return_code = OS_QueueCreate_Impl(&token, flags); diff --git a/src/os/shared/src/osapi-select.c b/src/os/shared/src/osapi-select.c index f74453366..5d346eb66 100644 --- a/src/os/shared/src/osapi-select.c +++ b/src/os/shared/src/osapi-select.c @@ -141,6 +141,11 @@ int32 OS_SelectFdAdd(OS_FdSet *Set, osal_id_t objid) return_code = OS_ObjectIdToArrayIndex(OS_OBJECT_TYPE_OS_STREAM, objid, &local_id); if (return_code == OS_SUCCESS) { + /* + * Sets the bit in the uint8 object_ids array that corresponds + * to the local_id where local_id >> 3 determines the array element, + * and the mask/shift sets the bit within that element. + */ Set->object_ids[local_id >> 3] |= 1 << (local_id & 0x7); } @@ -166,6 +171,11 @@ int32 OS_SelectFdClear(OS_FdSet *Set, osal_id_t objid) return_code = OS_ObjectIdToArrayIndex(OS_OBJECT_TYPE_OS_STREAM, objid, &local_id); if (return_code == OS_SUCCESS) { + /* + * Clears the bit in the uint8 object_ids array that corresponds + * to the local_id where local_id >> 3 determines the array element, + * and the mask/shift clears the bit within that element. + */ Set->object_ids[local_id >> 3] &= ~(1 << (local_id & 0x7)); } @@ -194,5 +204,10 @@ bool OS_SelectFdIsSet(OS_FdSet *Set, osal_id_t objid) return false; } + /* + * Returns boolean for if the bit in the uint8 object_ids array that corresponds + * to the local_id is set where local_id >> 3 determines the array element, + * and the mask/shift checks the bit within that element. + */ return ((Set->object_ids[local_id >> 3] >> (local_id & 0x7)) & 0x1); } /* end OS_SelectFdIsSet */ diff --git a/src/os/vxworks/src/os-impl-console.c b/src/os/vxworks/src/os-impl-console.c index f5d44dc69..9fc250814 100644 --- a/src/os/vxworks/src/os-impl-console.c +++ b/src/os/vxworks/src/os-impl-console.c @@ -119,6 +119,7 @@ int OS_VxWorks_ConsoleTask_Entry(int arg) OS_ObjectIdRelease(&token); } + /* Return OK since called from taskSpawn, error is reported in debug message */ return OK; } /* end OS_ConsoleTask_Entry */ diff --git a/src/os/vxworks/src/os-impl-tasks.c b/src/os/vxworks/src/os-impl-tasks.c index 2f6ebfed4..e05524f33 100644 --- a/src/os/vxworks/src/os-impl-tasks.c +++ b/src/os/vxworks/src/os-impl-tasks.c @@ -166,7 +166,7 @@ int32 OS_TaskCreate_Impl(const OS_object_token_t *token, uint32 flags) * NOTE: Allocation of the stack requires a malloc() of some form. * This is what taskSpawn() effectively does internally to create * stack. If the system malloc() is unacceptable here then this - * could be replaced with a statically-allocated OSAL stack buffer. + * could be replaced with a locally scoped statically allocated buffer. * * ALSO NOTE: The stack-rounding macros are normally supplied from * vxWorks.h on relevant platforms. If not provided then it is diff --git a/src/os/vxworks/src/os-impl-timebase.c b/src/os/vxworks/src/os-impl-timebase.c index b390f15f2..7a774b10c 100644 --- a/src/os/vxworks/src/os-impl-timebase.c +++ b/src/os/vxworks/src/os-impl-timebase.c @@ -319,8 +319,8 @@ int32 OS_VxWorks_TimeBaseAPI_Impl_Init(void) /* * Finally compute the Microseconds per tick - * This must further round again to the nearest microsecond, so it is undesirable to use - * this for time computations if the result is not exact. + * This must further round again to the nearest microsecond (using the + 500 / 1000), + * so it is undesirable to use this for time computations if the result is not exact. */ OS_SharedGlobalVars.MicroSecPerTick = (OS_ClockAccuracyNsec + 500) / 1000;