From 69b0940dd9249adc4d4d2e998a610a4354c63153 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Tue, 1 Jun 2021 13:15:18 -0400 Subject: [PATCH] Fix #1544, add time get reference error bit Use one of the unused time state bits to indicate if an error has occurred where CFE_TIME_GetReference was not able to get a consistent copy of the reference state data. In a functional system this should never occur - there should be at most one retry, which only happens in the event there was a burst of updates (more than 4) concurrently with reading the structure. The previous implementation did not report or handle the condition at all, this at least sets a TLM status bit and returns a reference struct filled with all zeros. --- modules/time/fsw/inc/cfe_time_msg.h | 4 +- modules/time/fsw/src/cfe_time_api.c | 8 ++++ modules/time/fsw/src/cfe_time_utils.c | 21 +++++++++ modules/time/fsw/src/cfe_time_utils.h | 4 +- modules/time/ut-coverage/time_UT.c | 66 ++++++++++++++++++++++++++- 5 files changed, 99 insertions(+), 4 deletions(-) diff --git a/modules/time/fsw/inc/cfe_time_msg.h b/modules/time/fsw/inc/cfe_time_msg.h index 9aad58e12..3924fe4ec 100644 --- a/modules/time/fsw/inc/cfe_time_msg.h +++ b/modules/time/fsw/inc/cfe_time_msg.h @@ -717,7 +717,9 @@ #define CFE_TIME_FLAG_ADDTCL 0x0080 /**< \brief Time Client Latency is applied in a positive direction */ #define CFE_TIME_FLAG_SERVER 0x0040 /**< \brief This instance of Time Services is a Time Server */ #define CFE_TIME_FLAG_GDTONE 0x0020 /**< \brief The tone received is good compared to the last tone received */ -#define CFE_TIME_FLAG_UNUSED 0x001F /**< \brief Reserved flags - should be zero */ +#define CFE_TIME_FLAG_REFERR \ + 0x0010 /**< \brief GetReference read error, will be set if unable to get a consistent ref value */ +#define CFE_TIME_FLAG_UNUSED 0x000F /**< \brief Reserved flags - should be zero */ /** \} */ /*************************************************************************/ diff --git a/modules/time/fsw/src/cfe_time_api.c b/modules/time/fsw/src/cfe_time_api.c index f6eb1246e..3c2ea1d6b 100644 --- a/modules/time/fsw/src/cfe_time_api.c +++ b/modules/time/fsw/src/cfe_time_api.c @@ -284,6 +284,14 @@ uint16 CFE_TIME_GetClockInfo(void) StateFlags |= CFE_TIME_FLAG_GDTONE; } + /* + ** Check if CFE_TIME_GetReference ever failed to get a good value + */ + if (CFE_TIME_Global.GetReferenceFail) + { + StateFlags |= CFE_TIME_FLAG_REFERR; + } + return (StateFlags); } diff --git a/modules/time/fsw/src/cfe_time_utils.c b/modules/time/fsw/src/cfe_time_utils.c index 764cd0acd..aee970c9d 100644 --- a/modules/time/fsw/src/cfe_time_utils.c +++ b/modules/time/fsw/src/cfe_time_utils.c @@ -619,6 +619,27 @@ void CFE_TIME_GetReference(CFE_TIME_Reference_t *Reference) --RetryCount; } + /* + * This should really never happen: if RetryCount reaches its limit, it means something is + * continously changing the time structure to the point where this task was not able to + * get a consistent copy. The only way this could happen is if some update task got into + * a continuous loop, or if the memory itself has gone bad and reads inconsistently. But + * if the latter is the case, the whole system has undefined behavior. + */ + if (RetryCount == 0) + { + /* set the flag that indicates this failed */ + CFE_TIME_Global.GetReferenceFail = true; + + /* + * Zeroing out the structure produces an identifiable output, in particular + * this sets the ClockSetState to CFE_TIME_SetState_NOT_SET, which the CFE_TIME_CalculateState() + * will in turn translate to CFE_TIME_ClockState_INVALID in TLM. + */ + memset(Reference, 0, sizeof(*Reference)); + return; + } + /* ** Compute the amount of time "since" the tone... */ diff --git a/modules/time/fsw/src/cfe_time_utils.h b/modules/time/fsw/src/cfe_time_utils.h index 232940f16..ec08f7aa6 100644 --- a/modules/time/fsw/src/cfe_time_utils.h +++ b/modules/time/fsw/src/cfe_time_utils.h @@ -257,9 +257,9 @@ typedef struct bool IsToneGood; /* - ** Spare byte for alignment + ** Flag that indicates if "CFE_TIME_GetReference()" ever failed to get a valid time */ - bool Spare; + bool GetReferenceFail; /* ** Local 1Hz wake-up command packet (not related to time at tone)... diff --git a/modules/time/ut-coverage/time_UT.c b/modules/time/ut-coverage/time_UT.c index aac8c2d00..34201c3c5 100644 --- a/modules/time/ut-coverage/time_UT.c +++ b/modules/time/ut-coverage/time_UT.c @@ -124,6 +124,36 @@ int32 ut_time_CallbackCalled; void OS_SelectTone(int16 Signal) {} #endif +/* + * A hook functions for CFE_PSP_GetTime that updates the Reference State. + * This mimmics what would happen if a time update occurred at the moment + * another task was reading the time. + */ +int32 UT_TimeRefUpdateHook(void *UserObj, int32 StubRetcode, uint32 CallCount, const UT_StubContext_t *Context) +{ + volatile CFE_TIME_ReferenceState_t *RefState; + uint32 * UpdateCount = UserObj; + uint32 i; + + /* + * NOTE: in order to trigger a read retry, this actually needs to do CFE_TIME_REFERENCE_BUF_DEPTH + * updates, such that the buffer being read is overwritten. + */ + if (*UpdateCount > 0) + { + for (i = 0; i < CFE_TIME_REFERENCE_BUF_DEPTH; ++i) + { + RefState = CFE_TIME_StartReferenceUpdate(); + RefState->AtToneLatch.Seconds = 1 + CallCount; + RefState->ClockSetState = CFE_TIME_SetState_WAS_SET; + CFE_TIME_FinishReferenceUpdate(RefState); + } + --(*UpdateCount); + } + + return StubRetcode; +} + void UtTest_Setup(void) { /* Initialize unit test */ @@ -492,6 +522,7 @@ void Test_GetTime(void) CFE_TIME_Global.OneHzDirection = CFE_TIME_AdjustDirection_SUBTRACT; RefState->DelayDirection = CFE_TIME_AdjustDirection_SUBTRACT; CFE_TIME_Global.IsToneGood = false; + CFE_TIME_Global.GetReferenceFail = false; CFE_TIME_FinishReferenceUpdate(RefState); ActFlags = CFE_TIME_GetClockInfo(); StateFlags = 0; @@ -517,10 +548,11 @@ void Test_GetTime(void) CFE_TIME_Global.OneTimeDirection = CFE_TIME_AdjustDirection_ADD; CFE_TIME_Global.OneHzDirection = CFE_TIME_AdjustDirection_ADD; CFE_TIME_Global.IsToneGood = true; + CFE_TIME_Global.GetReferenceFail = true; StateFlags = CFE_TIME_FLAG_CLKSET | CFE_TIME_FLAG_FLYING | CFE_TIME_FLAG_SRCINT | CFE_TIME_FLAG_SIGPRI | CFE_TIME_FLAG_SRVFLY | CFE_TIME_FLAG_CMDFLY | CFE_TIME_FLAG_ADD1HZ | CFE_TIME_FLAG_ADDADJ | - CFE_TIME_FLAG_ADDTCL | CFE_TIME_FLAG_GDTONE; + CFE_TIME_FLAG_ADDTCL | CFE_TIME_FLAG_GDTONE | CFE_TIME_FLAG_REFERR; #if (CFE_PLATFORM_TIME_CFG_SERVER == true) StateFlags |= CFE_TIME_FLAG_SERVER; @@ -529,6 +561,8 @@ void Test_GetTime(void) ActFlags = CFE_TIME_GetClockInfo(); snprintf(testDesc, UT_MAX_MESSAGE_LENGTH, "Expected = 0x%04X, actual = 0x%04X", StateFlags, ActFlags); UT_Report(__FILE__, __LINE__, ActFlags == StateFlags, "CFE_TIME_GetClockInfo", testDesc); + + CFE_TIME_Global.GetReferenceFail = false; } /* @@ -2009,6 +2043,7 @@ void Test_GetReference(void) { CFE_TIME_Reference_t Reference; volatile CFE_TIME_ReferenceState_t *RefState; + uint32 UpdateCount; UtPrintf("Begin Test Get Reference"); @@ -2050,6 +2085,35 @@ void Test_GetReference(void) */ UT_Report(__FILE__, __LINE__, Reference.CurrentMET.Seconds == 25 && Reference.CurrentMET.Subseconds == 0, "CFE_TIME_GetReference", "Local clock > latch at tone time"); + + /* Use a hook function to test the behavior when the read needs to be retried */ + /* This just causes a single retry, the process should still succeed */ + UT_InitData(); + memset((void *)CFE_TIME_Global.ReferenceState, 0, sizeof(CFE_TIME_Global.ReferenceState)); + CFE_TIME_Global.GetReferenceFail = false; + UpdateCount = 1; + UT_SetHookFunction(UT_KEY(CFE_PSP_GetTime), UT_TimeRefUpdateHook, &UpdateCount); + UT_SetBSP_Time(20, 0); + UT_SetBSP_Time(20, 100); + CFE_TIME_GetReference(&Reference); + + /* This should not have set the flag, and the output should be valid*/ + UtAssert_UINT32_EQ(CFE_TIME_Global.GetReferenceFail, false); + UtAssert_UINT32_EQ(Reference.CurrentMET.Seconds, 19); + UtAssert_UINT32_EQ(Reference.CurrentMET.Subseconds, 429497); + UtAssert_UINT32_EQ(Reference.ClockSetState, CFE_TIME_SetState_WAS_SET); + + /* With multiple retries, it should fail */ + UpdateCount = 1000000; + CFE_TIME_GetReference(&Reference); + + /* This should have set the flag, and the output should be all zero */ + UtAssert_UINT32_EQ(CFE_TIME_Global.GetReferenceFail, true); + UtAssert_UINT32_EQ(Reference.CurrentMET.Seconds, 0); + UtAssert_UINT32_EQ(Reference.CurrentMET.Subseconds, 0); + UtAssert_UINT32_EQ(Reference.ClockSetState, CFE_TIME_SetState_NOT_SET); + + CFE_TIME_Global.GetReferenceFail = false; } /*