From cc10f7c80274895d3e33fcf8af3405ae472d8a48 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 30 Dec 2020 17:59:38 -0500 Subject: [PATCH] Fix #429, update OSAL code to use time accessors Do not access members of OS_time_t directly, instead use conversion/accessor inline functions to get the desired value. --- src/os/inc/osapi-clock.h | 258 +++++++++++++++++- src/os/inc/osapi-file.h | 11 +- src/os/portable/os-impl-posix-files.c | 15 +- src/os/portable/os-impl-posix-gettime.c | 9 +- src/os/shared/src/osapi-clock.c | 2 +- .../timer-add-api-test/timer-add-api-test.c | 12 +- src/tests/timer-test/timer-test.c | 10 +- .../portable/src/coveragetest-posix-gettime.c | 10 +- .../oscore-test/ut_oscore_misc_test.c | 18 +- src/unit-tests/ostimer-test/ut_ostimer_test.c | 4 +- src/ut-stubs/osapi-utstub-clock.c | 7 +- 11 files changed, 300 insertions(+), 56 deletions(-) diff --git a/src/os/inc/osapi-clock.h b/src/os/inc/osapi-clock.h index f664d3582..55fec1c14 100644 --- a/src/os/inc/osapi-clock.h +++ b/src/os/inc/osapi-clock.h @@ -28,14 +28,26 @@ #include "osconfig.h" #include "common_types.h" -/** @brief OSAL time */ +/** + * @brief OSAL time interval structure + * + * This is used to represent a basic time interval. + * + * When used with OS_GetLocalTime/OS_SetLocalTime, this represents the + * interval from the OS's epoch point, typically 01 Jan 1970 00:00:00 UTC + * on systems that have a persistent real time clock (RTC), or the system + * boot time if there is no RTC available. + * + * Applications should not directly access fields within this structure, + * as the definition may change in future versions of OSAL. Instead, + * applications should use the accessor/conversion methods defined below. + */ typedef struct { uint32 seconds; uint32 microsecs; } OS_time_t; - /** @defgroup OSAPIClock OSAL Real Time Clock APIs * @{ */ @@ -66,7 +78,247 @@ int32 OS_GetLocalTime(OS_time_t *time_struct); * * @return Set local time status, see @ref OSReturnCodes */ -int32 OS_SetLocalTime(OS_time_t *time_struct); +int32 OS_SetLocalTime(const OS_time_t *time_struct); + + +/*-------------------------------------------------------------------------------------*/ +/* + * Accessor / Unit Conversion routines for OS_time_t + * + * These routines allow the user to simply interpret OS_time_t intervals into + * in normalized units of whole seconds, milliseconds, microseconds, or nanoseconds. + */ +/*-------------------------------------------------------------------------------------*/ + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get interval from an OS_time_t object normalized to whole number of seconds + * + * Extracts the number of whole seconds from a given OS_time_t object, discarding + * any fractional component. + * + * This may also replace a direct read of the "seconds" field from + * the OS_time_t object from previous versions of OSAL, where the + * structure was defined with separate seconds/microseconds fields. + * + * @sa OS_TimeGetMicrosecondsPart() + * + * @param[in] tm Time interval value + * @returns Whole number of seconds in time interval + */ +static inline int64 OS_TimeGetTotalSeconds(OS_time_t tm) +{ + return (tm.seconds); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get interval from an OS_time_t object normalized to millisecond units + * + * Note this refers to the complete interval, not just the fractional part. + * + * @param[in] tm Time interval value + * @returns Whole number of milliseconds in time interval + */ +static inline int64 OS_TimeGetTotalMilliseconds(OS_time_t tm) +{ + return (((int64)tm.seconds * 1000) + (tm.microsecs / 1000)); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get interval from an OS_time_t object normalized to microsecond units + * + * Note this refers to the complete interval, not just the fractional part. + * + * @param[in] tm Time interval value + * @returns Whole number of microseconds in time interval + */ +static inline int64 OS_TimeGetTotalMicroseconds(OS_time_t tm) +{ + return (((int64)tm.seconds * 1000000) + tm.microsecs); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get interval from an OS_time_t object normalized to nanosecond units + * + * Note this refers to the complete interval, not just the fractional part. + * + * @note There is no protection against overflow of the 64-bit return value. + * Applications must use caution to ensure that the interval does not exceed the + * representable range of a signed 64 bit integer - approximately 140 years. + * + * @param[in] tm Time interval value + * @returns Whole number of microseconds in time interval + */ +static inline int64 OS_TimeGetTotalNanoseconds(OS_time_t tm) +{ + return (((int64)tm.seconds * 1000000000) + (tm.microsecs * 1000)); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get subseconds portion (fractional part only) from an OS_time_t object + * + * Extracts the fractional part from a given OS_time_t object. + * Units returned are in ticks, not normalized to any standard time unit. + * + * @param[in] tm Time interval value + * @returns Fractional/subsecond portion of time interval in ticks + */ +static inline int64 OS_TimeGetFractionalPart(OS_time_t tm) +{ + return (tm.microsecs); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get 32-bit normalized subseconds (fractional part only) from an OS_time_t object + * + * Extracts the fractional part from a given OS_time_t object normalized + * to units of (1 / (2^32)) sec. This is a base-2 fixed-point fractional value + * with the point left-justified in the 32-bit value (i.e. left of MSB). + * + * This is (mostly) compatible with the CFE "subseconds" value, where 0x80000000 represents + * exactly one half second, and 0 represents a full second. + * + * @param[in] tm Time interval value + * @returns Fractional/subsecond portion of time interval as 32-bit fixed point value + */ +static inline uint32 OS_TimeGetSubseconds32Part(OS_time_t tm) +{ + /* + * This computation avoids a 32-bit left shift which may not be implemented. + * + * It also must round up, otherwise this may result in a value one + * less than the original when converted back to usec again. + */ + return (((OS_TimeGetFractionalPart(tm) << 26) + 15624) / 15625); +} + + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get milliseconds portion (fractional part only) from an OS_time_t object + * + * Extracts the fractional part from a given OS_time_t object normalized + * to units of milliseconds. + * + * @sa OS_TimeGetTotalSeconds() + * + * @param[in] tm Time interval value + * @returns Number of milliseconds in time interval + */ +static inline uint32 OS_TimeGetMillisecondsPart(OS_time_t tm) +{ + return OS_TimeGetFractionalPart(tm) / 1000; +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get microseconds portion (fractional part only) from an OS_time_t object + * + * Extracts the fractional part from a given OS_time_t object normalized + * to units of microseconds. + * + * This function may be used to adapt applications initially implemented + * using an older OSAL version where OS_time_t was a structure containing + * a "seconds" and "microsecs" field. + * + * This function will obtain a value that is compatible with the "microsecs" field of + * OS_time_t as it was defined in previous versions of OSAL, as well as the "tv_usec" + * field of POSIX-style "struct timeval" values. + * + * @sa OS_TimeGetTotalSeconds() + * + * @param[in] tm Time interval value + * @returns Number of microseconds in time interval + */ +static inline uint32 OS_TimeGetMicrosecondsPart(OS_time_t tm) +{ + return OS_TimeGetFractionalPart(tm); +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Get nanoseconds portion (fractional part only) from an OS_time_t object + * + * Extracts the only number of nanoseconds from a given OS_time_t object. + * + * This function will obtain a value that is compatible with the "tv_nsec" field + * of POSIX-style "struct timespec" values. + * + * @sa OS_TimeGetTotalSeconds() + * + * @param[in] tm Time interval value + * @returns Number of nanoseconds in time interval + */ +static inline uint32 OS_TimeGetNanosecondsPart(OS_time_t tm) +{ + return OS_TimeGetFractionalPart(tm) * 1000; +} + +/** + * @brief Convert a number of seconds + nanoseconds into an OS_time_t interval + * + * @param[in] seconds Whole number of seconds + * @param[in] nanoseconds Number of nanoseconds (fractional part only) + * @returns The input arguments represented as an OS_time_t interval + */ +static inline OS_time_t OS_TimeInterval(int64 seconds, int32 nanoseconds) +{ + OS_time_t result; + result.seconds = seconds; + result.microsecs = nanoseconds / 1000; + return result; +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Computes the sum of two time intervals + * + * @param[in] time1 The first interval + * @param[in] time2 The second interval + * + * @return The sum of the two intervals (time1 + time2) + */ +static inline OS_time_t OS_TimeAdd(OS_time_t time1, OS_time_t time2) +{ + OS_time_t result = time1; + result.seconds += time2.seconds; + result.microsecs += time2.microsecs; + if (result.microsecs >= 1000000) + { + ++result.seconds; + result.microsecs -= 1000000; + } + return result; +} + +/*-------------------------------------------------------------------------------------*/ +/** + * @brief Computes the difference between two time intervals + * + * @param[in] time1 The first interval + * @param[in] time2 The second interval + * + * @return The difference of the two intervals (time1 - time2) + */ +static inline OS_time_t OS_TimeSubtract(OS_time_t time1, OS_time_t time2) +{ + OS_time_t result = time1; + result.seconds -= time2.seconds; + result.microsecs -= time2.microsecs; + if (result.microsecs >= 1000000) + { + --result.seconds; + result.microsecs += 1000000; + } + return result; +} + + /**@}*/ #endif diff --git a/src/os/inc/osapi-file.h b/src/os/inc/osapi-file.h index fea9f7519..011da6872 100644 --- a/src/os/inc/osapi-file.h +++ b/src/os/inc/osapi-file.h @@ -27,6 +27,7 @@ #include "osconfig.h" #include "common_types.h" +#include "osapi-clock.h" /** @defgroup OSFileAccess OSAL File Access Option Defines @@ -63,9 +64,9 @@ typedef struct */ typedef struct { - uint32 FileModeBits; - int32 FileTime; - size_t FileSize; + uint32 FileModeBits; + OS_time_t FileTime; + size_t FileSize; } os_fstat_t; /** @@ -96,8 +97,8 @@ enum #define OS_FILESTAT_READ(x) ((x).FileModeBits & OS_FILESTAT_MODE_READ) /** @brief Access file stat size field */ #define OS_FILESTAT_SIZE(x) ((x).FileSize) -/** @brief Access file stat time field */ -#define OS_FILESTAT_TIME(x) ((x).FileTime) +/** @brief Access file stat time field as a whole number of seconds */ +#define OS_FILESTAT_TIME(x) (OS_TimeGetTotalSeconds((x).FileTime)) /** * @brief Flags that can be used with opening of a file (bitmask) diff --git a/src/os/portable/os-impl-posix-files.c b/src/os/portable/os-impl-posix-files.c index cb8d7df96..2c921d2ce 100644 --- a/src/os/portable/os-impl-posix-files.c +++ b/src/os/portable/os-impl-posix-files.c @@ -144,7 +144,20 @@ int32 OS_FileStat_Impl(const char *local_path, os_fstat_t *FileStats) } FileStats->FileSize = st.st_size; - FileStats->FileTime = st.st_mtime; + + /* + * NOTE: Traditional timestamps are only a whole number of seconds (time_t) + * POSIX.1-2008 expands this to have a full "struct timespec" with nanosecond + * resolution. + * + * For maximum compatibilty this is just returning the low-res timestamp from + * st_mtime, as the specific POSIX conformance level is not known, and OSAL should + * not have a hard dependency on POSIX.1-2008 (yet). + * + * However the OSAL API structure supports higher resolution time stamps and this + * may be expanded in the future by adding the fractional seconds value here. + */ + FileStats->FileTime = OS_TimeInterval(st.st_mtime, 0); /* note that the "fst_mode" member is already zeroed by the caller */ if (S_ISDIR(st.st_mode)) diff --git a/src/os/portable/os-impl-posix-gettime.c b/src/os/portable/os-impl-posix-gettime.c index 59175d0a0..507c23560 100644 --- a/src/os/portable/os-impl-posix-gettime.c +++ b/src/os/portable/os-impl-posix-gettime.c @@ -73,9 +73,8 @@ int32 OS_GetLocalTime_Impl(OS_time_t *time_struct) if (Status == 0) { - time_struct->seconds = time.tv_sec; - time_struct->microsecs = time.tv_nsec / 1000; - ReturnCode = OS_SUCCESS; + *time_struct = OS_TimeInterval(time.tv_sec, time.tv_nsec); + ReturnCode = OS_SUCCESS; } else { @@ -100,8 +99,8 @@ int32 OS_SetLocalTime_Impl(const OS_time_t *time_struct) int32 ReturnCode; struct timespec time; - time.tv_sec = time_struct->seconds; - time.tv_nsec = (time_struct->microsecs * 1000); + time.tv_sec = OS_TimeGetTotalSeconds(*time_struct); + time.tv_nsec = OS_TimeGetNanosecondsPart(*time_struct); Status = clock_settime(OSAL_GETTIME_SOURCE_CLOCK, &time); diff --git a/src/os/shared/src/osapi-clock.c b/src/os/shared/src/osapi-clock.c index 09dbf110d..1aec8bfc8 100644 --- a/src/os/shared/src/osapi-clock.c +++ b/src/os/shared/src/osapi-clock.c @@ -65,7 +65,7 @@ int32 OS_GetLocalTime(OS_time_t *time_struct) * See description in API and header file for detail * *-----------------------------------------------------------------*/ -int32 OS_SetLocalTime(OS_time_t *time_struct) +int32 OS_SetLocalTime(const OS_time_t *time_struct) { /* Check inputs */ OS_CHECK_POINTER(time_struct); diff --git a/src/tests/timer-add-api-test/timer-add-api-test.c b/src/tests/timer-add-api-test/timer-add-api-test.c index 2ec048fd4..5ff9541b4 100644 --- a/src/tests/timer-add-api-test/timer-add-api-test.c +++ b/src/tests/timer-add-api-test/timer-add-api-test.c @@ -130,7 +130,7 @@ void TestTimerAddApi(void) OS_GetLocalTime(&EndTime); - for (i = NUMBER_OF_TIMERS-1; i >= 0; --i) + for (i = NUMBER_OF_TIMERS - 1; i >= 0; --i) { TimerStatus[i] = OS_TimerDelete(TimerID[i]); } @@ -144,15 +144,7 @@ void TestTimerAddApi(void) /* * Time limited test */ - microsecs = 1000000 * (EndTime.seconds - StartTime.seconds); - if (EndTime.microsecs < StartTime.microsecs) - { - microsecs -= StartTime.microsecs - EndTime.microsecs; - } - else - { - microsecs += EndTime.microsecs - StartTime.microsecs; - } + microsecs = OS_TimeGetTotalMicroseconds(OS_TimeSubtract(EndTime, StartTime)); /* Make sure the ratio of the timers are OK */ for (i = 0; i < NUMBER_OF_TIMERS; i++) diff --git a/src/tests/timer-test/timer-test.c b/src/tests/timer-test/timer-test.c index 4d86367d1..79ed1a43c 100644 --- a/src/tests/timer-test/timer-test.c +++ b/src/tests/timer-test/timer-test.c @@ -189,15 +189,7 @@ void TimerTestCheck(void) /* * Time limited test - check and exit */ - microsecs = 1000000 * (EndTime.seconds - StartTime.seconds); - if (EndTime.microsecs < StartTime.microsecs) - { - microsecs -= StartTime.microsecs - EndTime.microsecs; - } - else - { - microsecs += EndTime.microsecs - StartTime.microsecs; - } + microsecs = OS_TimeGetTotalMicroseconds(OS_TimeSubtract(EndTime, StartTime)); /* Make sure the ratio of the timers are OK */ for (i = 0; i < NUMBER_OF_TIMERS && i < OS_MAX_TIMERS; i++) diff --git a/src/unit-test-coverage/portable/src/coveragetest-posix-gettime.c b/src/unit-test-coverage/portable/src/coveragetest-posix-gettime.c index 8dc31a951..e69cb830a 100644 --- a/src/unit-test-coverage/portable/src/coveragetest-posix-gettime.c +++ b/src/unit-test-coverage/portable/src/coveragetest-posix-gettime.c @@ -35,9 +35,8 @@ void Test_OS_GetLocalTime_Impl(void) * Test Case For: * int32 OS_GetLocalTime_Impl(OS_time_t *time_struct) */ - OS_time_t timeval; - timeval.seconds = 1; - timeval.microsecs = 1; + OS_time_t timeval = {0}; + OSAPI_TEST_FUNCTION_RC(OS_GetLocalTime_Impl, (&timeval), OS_SUCCESS); UT_SetDefaultReturnValue(UT_KEY(OCS_clock_gettime), -1); @@ -50,9 +49,8 @@ void Test_OS_SetLocalTime_Impl(void) * Test Case For: * int32 OS_SetLocalTime_Impl(const OS_time_t *time_struct) */ - OS_time_t timeval; - timeval.seconds = 1; - timeval.microsecs = 1; + OS_time_t timeval = {0}; + OSAPI_TEST_FUNCTION_RC(OS_SetLocalTime_Impl, (&timeval), OS_SUCCESS); UT_SetDefaultReturnValue(UT_KEY(OCS_clock_settime), -1); diff --git a/src/unit-tests/oscore-test/ut_oscore_misc_test.c b/src/unit-tests/oscore-test/ut_oscore_misc_test.c index f54ca186e..af5dfe62f 100644 --- a/src/unit-tests/oscore-test/ut_oscore_misc_test.c +++ b/src/unit-tests/oscore-test/ut_oscore_misc_test.c @@ -290,8 +290,8 @@ void UT_os_getlocaltime_test() for (i = 0; i < 5; i++) { UT_OS_LOG("OS_GetLocalTime() - #3 Nominal "); - UT_OS_LOG("[Expecting output after API call to increase over time: %ld.%ld]\n", (long)time_struct.seconds, - (long)time_struct.microsecs); + UT_OS_LOG("[Expecting output after API call to increase over time: %ld.%ld]\n", + (long)OS_TimeGetTotalSeconds(time_struct), (long)OS_TimeGetMicrosecondsPart(time_struct)); OS_TaskDelay(20); OS_GetLocalTime(&time_struct); @@ -379,23 +379,21 @@ void UT_os_setlocaltime_test() for (i = 0; i < 5; i++) { UT_OS_LOG("OS_SetLocalTime() - #3 Nominal "); - UT_OS_LOG("[Expecting output before API call to increase over time: %ld.%ld]\n", (long)time_struct.seconds, - (long)time_struct.microsecs); + UT_OS_LOG("[Expecting output before API call to increase over time: %ld.%ld]\n", + (long)OS_TimeGetTotalSeconds(time_struct), (long)OS_TimeGetMicrosecondsPart(time_struct)); OS_TaskDelay(20); OS_GetLocalTime(&time_struct); } } - memset(&time_struct, 0x00, sizeof(time_struct)); - time_struct.seconds = 20000; - time_struct.microsecs = 123; + time_struct = OS_TimeInterval(20000, 123000); res = OS_SetLocalTime(&time_struct); if (res == OS_SUCCESS) { - UT_OS_LOG("OS_SetLocalTime() - #3 Nominal [New time set at %ld.%ld]\n", (long)time_struct.seconds, - (long)time_struct.microsecs); + UT_OS_LOG("OS_SetLocalTime() - #3 Nominal [New time set at %ld.%ld]\n", (long)OS_TimeGetTotalSeconds(time_struct), + (long)OS_TimeGetMicrosecondsPart(time_struct)); res = OS_GetLocalTime(&time_struct); if (res == OS_SUCCESS) @@ -404,7 +402,7 @@ void UT_os_setlocaltime_test() { UT_OS_LOG("OS_SetLocalTime() - #3 Nominal "); UT_OS_LOG("[Expecting output after API call to increase over time: %ld.%ld]\n", - (long)time_struct.seconds, (long)time_struct.microsecs); + (long)OS_TimeGetTotalSeconds(time_struct), (long)OS_TimeGetMicrosecondsPart(time_struct)); OS_TaskDelay(20); OS_GetLocalTime(&time_struct); diff --git a/src/unit-tests/ostimer-test/ut_ostimer_test.c b/src/unit-tests/ostimer-test/ut_ostimer_test.c index 6b6716f37..dcbe25d10 100644 --- a/src/unit-tests/ostimer-test/ut_ostimer_test.c +++ b/src/unit-tests/ostimer-test/ut_ostimer_test.c @@ -78,7 +78,7 @@ void UT_os_timercallback(osal_id_t timerId) static int32 loopCnt = 0, res = 0; static uint32 prevIntervalTime = 0; static uint32 currIntervalTime = 0; - static OS_time_t currTime = {0, 0}, endTime = {0, 0}; + static OS_time_t currTime = {0}, endTime = {0}; if (OS_ObjectIdEqual(timerId, g_timerId)) { @@ -94,7 +94,7 @@ void UT_os_timercallback(osal_id_t timerId) OS_GetLocalTime(&endTime); - currIntervalTime = 1000000 * (endTime.seconds - currTime.seconds) + endTime.microsecs - currTime.microsecs; + currIntervalTime = OS_TimeGetTotalMicroseconds(OS_TimeSubtract(endTime, currTime)); if (currIntervalTime >= prevIntervalTime) deltaTime = currIntervalTime - prevIntervalTime; diff --git a/src/ut-stubs/osapi-utstub-clock.c b/src/ut-stubs/osapi-utstub-clock.c index 37686f582..45dd6221e 100644 --- a/src/ut-stubs/osapi-utstub-clock.c +++ b/src/ut-stubs/osapi-utstub-clock.c @@ -52,9 +52,8 @@ int32 OS_GetLocalTime(OS_time_t *time_struct) if (status == OS_SUCCESS && UT_Stub_CopyToLocal(UT_KEY(OS_GetLocalTime), time_struct, sizeof(*time_struct)) < sizeof(*time_struct)) { - count = UT_GetStubCount(UT_KEY(OS_GetLocalTime)); - time_struct->microsecs = 10000 * (count % 100); - time_struct->seconds = 1 + (count / 100); + count = UT_GetStubCount(UT_KEY(OS_GetLocalTime)); + *time_struct = OS_TimeInterval(1 + (count / 100), 10000000 * (count % 100)); } return status; @@ -66,7 +65,7 @@ int32 OS_GetLocalTime(OS_time_t *time_struct) * Stub function for OS_SetLocalTime() * *****************************************************************************/ -int32 OS_SetLocalTime(OS_time_t *time_struct) +int32 OS_SetLocalTime(const OS_time_t *time_struct) { UT_Stub_RegisterContext(UT_KEY(OS_SetLocalTime), time_struct);