nasa/lc

The Core Flight System (cFS) Limit Checker (LC) application.

core Flight System (cFS) Limit Checker (LC)

Introduction

The Limit Checker (LC) is a core Flight System (cFS) application that is a plug in to the Core Flight Executive (cFE) component of the cFS.

The LC application monitors telemetry data points in a cFS system and compares the values against predefined threshold limits. When a threshold condition is encountered, an event message is issued and a Relative Time Sequence (RTS) command script may be initiated to respond/react to the threshold violation.

The LC application is written in C and depends on the cFS Operating System Abstraction Layer (OSAL) and cFE components. There is additional LC application specific configuration information contained in the application user's guide.

User's guide information can be generated using Doxygen (from top mission directory):

  make prep
  make -C build/docs/lc-usersguide lc-usersguide

Software Required

cFS Framework (cFE, OSAL, PSP)

An integrated bundle including the cFE, OSAL, and PSP can be obtained at https://github.com/nasa/cfs

About cFS

The cFS is a platform and project independent reusable software framework and set of reusable applications developed by NASA Goddard Space Flight Center. This framework is used as the basis for the flight software for satellite data systems and instruments, but can be used on other embedded systems. More information on the cFS can be found at http://cfs.gsfc.nasa.gov

lc's Issues

Make lc_tbl.h public

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
lc_tbl.h defines macros used in table generation and should be made public.

Requester Info
Dan Knutsen
NASA Goddard

LC Sets the TtoFValue when it transitions from STALE to FALSE

While testing LC against cFE 6.6, the lc_noaction test procedure failed because values were contained in the TtoFValue when they were not expected. The transition from STALE to FALSE seemed to set these values.

Imported from GSFCCFS-1075

Resolve static analysis issues relative to unit tests

Many actual code issues (uninitialized variables, buffer overruns, etc) identified via static analysis in the unit tests. All actual "bugs" should be resolved.

Imported from GSFCCFS-1936

Add CodeQL to repository

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Use CodeQL for continuous integration

Describe the solution you'd like
Add CodeQL workflow

Describe alternatives you've considered
None

Additional context
Add any other context about the feature request here.

Requester Info
Haven Carlson

Correct naming convention inconsistencies

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
The following identifiers used in LC do not correctly follow the naming conventions for CFE/CFS symbols:

• LC_SET_AP_PERMOFF_CC: should be LC_SET_AP_PERM_OFF_CC, because the CamelCase version is LC_SetAPPermOff_t, thus "Perm" and "Off" are separate words.
• LC_ACTION_NOT_USED: should be LC_APSTATE_ACTION_NOT_USED, because its used as an enum and the other labels all start with LC_APSTATE prefix
• LC_WATCH_NOT_USED: should be LC_DATA_WATCH_NOT_USED, for the same reason as LC_ACTION above.
• LC_NO_OPER: should be LC_OPER_NONE, same reason
• LC_NO_BITMASK: should be LC_BITMASK_NONE

Describe the solution you'd like
Rename these symbols for consistency.

Additional context
These name mismatches become relevant when using generated header files.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

LC_TableInit helper functions should be moved to their own file

LC_TableInit has several helper functions that it calls while running. These calls have many different variations and effects upon what happens in LC_TableInit. This makes it very difficult to unit test because there is too much variation in the helper functions to attempt to keep adequate track of what may or may not be happening. If these helper functions were moved to their own .c file they could be wrapped and stubbed for testing LC_TableInit. This would simplify the unit testing process.

Imported from GSFCCFS-1102

App Requirement Issues

1. LC3002.1.1 & LC3002.2.1 - Don’t see where event filter is able to be specified in Action Point or Watchpoint definition tables.
2. DS8000 – DS HK packet
3. FM4000 – FM HK packet
4. DS3000 - Requirement Incomplete

Imported from GSFCCFS-1917

LC: support 64-bit types

LC currently does not support 64-bit integers or floats (doubles).

Imported from GSFCCFS-768

Apps should use CFE_MSG_PTR macro instead of cast or local unwrapping

Apps typically cast to a CFE_MSG_Message_t or use *.Msg. Better to use abstracted CFE_MSG_PTR. NOTE - Not backwards compatible with Caelum so recommend not addressing in Draco.

Imported from GSFCCFS-1900

LC_SbInit casts every Status to unsigned int in every event message

LC_SbInit is casting the Status in every event send to an unsigned int. The cast appears unnecessary, CFE_EVS_SendEvent takes a variable length of arguments because it does a formatting of the string with the values sent AND the format in the send events is %08X (print 8 characters in upper case hex).

Imported from GSFCCFS-1099

Refactor LC_SampleAPs

The function LC_SampleAPs can be refactored - the "if(StartIndex == EndIndex)" condition can be removed.

Imported from GSFCCFS-1358

Items instantiated in header causes duplicate definitions and link errors

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The unit test header file lc_test_utils.h instantiates objects directly in the header file, which breaks if it is ever included in more than one C file.

To Reproduce
Build LC with unit tests enabled, get lots of linker errors:

usr/bin/ld: libcoverage-lc_internal-stubs.a(lc_test_utils.c.o):/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:39: multiple definition of `WDTable'; CMakeFiles/coverage-lc-lc_action-testrunner.dir/lc_action_tests.c.o:/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:39: first defined here
/usr/bin/ld: libcoverage-lc_internal-stubs.a(lc_test_utils.c.o):/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:40: multiple definition of `ADTable'; CMakeFiles/coverage-lc-lc_action-testrunner.dir/lc_action_tests.c.o:/home/joe/code/cfecfs/github/apps/lc/unit-test/utilities/lc_test_utils.h:40: first defined here

Expected behavior
Build should work?

Code snips

LC/unit-test/utilities/lc_test_utils.h

Lines 38 to 43 in 543cf6b

	/* Global table variables for table pointers contained in LC_OperData */
	LC_WDTEntry_t WDTable[LC_MAX_WATCHPOINTS];
	LC_ADTEntry_t ADTable[LC_MAX_ACTIONPOINTS];
	LC_WRTEntry_t WRTable[LC_MAX_WATCHPOINTS];
	LC_ARTEntry_t ARTable[LC_MAX_ACTIONPOINTS];

System observed on:
Ubuntu

Additional context
I was just trying to build LC "out of the box" - not modified in any way - and it failed badly. Not sure how this ever built or passed any validation testing with this the way it was.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

Add fsw/src to app target

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
Cannot build cert_testbed. target_include_directories(xx PUBLIC fsw/src) is needed to build tables, etc. Revise CMakeLists.txt

To Reproduce
Build in cert_testbed

Expected behavior
Error-free build

Code snips
If applicable, add references to the software.

System observed on:

• Ubuntu 20.04

Additional context
N/A

Reporter Info
Justin Figueroa, Vantage Systems

Variables declared mid-function

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
A few cases of variables declared mid-function left here.
Copy of nasa/CF#109

Expected behavior
All variables should be declared at the top of the function.

Reporter Info
Avi Weiss @thnkslprpt

LC: Consider making states into enums

In lc_msgdefs.h, LC states could be made into enums

Imported from GSFCCFS-1370

Static analysis workflow failure due to style warnings

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Strict cppcheck warnings cause static analysis workflow to fail, see https://github.com/nasa/LC/runs/6356575973?check_suite_focus=true

[fsw/src/lc_cmds.h:86] -> [fsw/src/lc_cmds.c:852]: (style, inconclusive) Function 'LC_ResetResultsAP' argument 3 names different: declaration 'ResetCmd' definition 'ResetStatsCmd'.
[fsw/src/lc_cmds.h:103] -> [fsw/src/lc_cmds.c:932]: (style, inconclusive) Function 'LC_ResetResultsWP' argument 3 names different: declaration 'ResetCmd' definition 'ResetStatsCmd'.
[fsw/src/lc_utils.c:234] -> [fsw/src/lc_utils.c:240]: (style) Variable 'Result' is reassigned a value before the old one has been used.

Describe the solution you'd like
Fix

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

Buffer overflow in unit tests when using default config

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The unit test code sets the EventText member with a call to strncpy and a hardcoded size here:

LC/unit-test/lc_action_tests.c

Line 233 in 779b903

strncpy(LC_OperData.ADTPtr[APNumber].EventText, "Event Message", 50);

However in the default platform config the size is only 32:

LC/fsw/inc/lc_platform_cfg.h

Line 228 in 779b903

#define LC_MAX_ACTION_TEXT 32

To Reproduce
Build and run using default/out-of-box config.

Expected behavior
Example configuration should not trigger buffer overflow

Additional context
Consider using sizeof() operator here, to adapt the strncpy call to the real size of the target buffer.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

Static analysis issues relative to flight code

Handful of static analysis issues in the "red" identified (non-Style issues). Need to resolve these.

Filter: -file:elf -file:ut -file:cfe -file:os -file:cf_ -file:_lab_app.c !(significance:style)

should resolve and/or disposition the higher ranked ones at minimum.

Note license restricts publishing issues.

Imported from GSFCCFS-1958

Remove use of compiler extension in LC table file definitions

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
The __attribute__((__used__)) is a GNU compiler extension flag and such constructs should be avoided in portable code.

Describe the solution you'd like
Remove

Additional context
This flag is only in there to avoid a warning due to the use of static - but this object should not be static to begin with, then everything works without the use of special flags.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

Should results tables be critical tables instead of stored to CDS

The Watchpoint Results Table and Actionpoint Results Table are currently saved to the CDS on the housekeeping request interval (see functions LC_UpdateTaskCDS and LC_HousekeepingReq). Should these tables be critical tables instead?

Imported from GSFCCFS-1103

LC does not support double precision watchpoints

LC does not currently support defining a watchpoint for a double precision value. Doubles are commonly used in GNC applications, and it would be helpful for LC to support monitoring those telemetry points, rather than requiring the upstream app to provide special telemetry for LC to monitor.

Combine consecutive, mutually-exclusive status checks

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
Mutually exclusive logic in these consecutive status checks in LC_CreateResultTables().

Code snips

LC/fsw/src/lc_app.c

Lines 513 to 519 in 2f177ae

	if (Result != CFE_SUCCESS)
	{
	CFE_EVS_SendEvent(LC_WRT_REGISTER_ERR_EID, CFE_EVS_EventType_ERROR, "Error registering WRT, RC=0x%08X",
	(unsigned int)Result);
	}
	if (Result == CFE_SUCCESS)

Expected behavior
Combine into an if/else.

Reporter Info
Avi Weiss @thnkslprpt

Add LC_SAMPLE_AP_ALL_MID to use a cmd w/ no parameters to process all watchpoints

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
LC_SAMPLE_AP_MID is out-of-family with most MIDs for doing simple processing (like SEND_HK, etc) in that it has parameters. This is fine until you try to send it from something like sch_lab which doesn't support command parameters.

Describe the solution you'd like
Add a simple LC_SAMPLE_AP_ALL_MID w/ no parameters (size 8)

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

Remove CFE_PSP_MemSet use for addresses in RAM

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Should just use memset/memcpy for addresses in RAM. The PSP functions serve no use in this context.

Describe the solution you'd like
Replace with memset/memcpy.

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

LC Transitions Active Action Points to Passive When Application is in Passive Mode

During a stakeholder rehearsal there were several APs that were commanded "active" while the LC application state was in "passive" mode. Before operations could command the application state to "active" mode, some of the APs that were activated and had "tripped" causing the AP to transition back to passive mode. The purpose of changing a "tripped" APs state from active to passive is to prevent an RTS from getting initiated more than once. In "passive" mode, LC performs all limit tests as in "active" mode, but no stored command sequences are invoked as the result of AP failures. Having the AP's state transition while the application is in passive mode will make enabling APs with a low threshold while LC is in passive mode very difficult. The rational for this design feature needs to be clearly understood and documented. The LC user's guides (both doxygen and word/pdf) do not make this design feature clear. If no rational exists this design feature should be removed from LC.

Imported from GSFCCFS-744

Move interface definition files to "inc" location

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
The interface definition files of all open source apps currently exist in the "src" directory.

Describe the solution you'd like
Create an "inc" dir to go with the "src" dir. Move the interface definitions into this location: "_msg.h", "_msgdefs.h", "_tbldefs.h", and "_events.h". Consider moving header files in both "platform_inc" and "mission_inc" to "inc"

Describe alternatives you've considered
Leaving as is.

Additional context
N/A

Requester Info
Justin Figueroa, Vantage Systems

Add break; for switch default case in LC_VerifyMsgLength()

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
default case of the switch block in LC_VerifyMsgLength() is missing a break;.
Purely a style/guidelines issue for consistency and future maintenance.

Code snips

LC/fsw/src/lc_utils.c

Lines 83 to 92 in 2f177ae

	default:
	/*
	** All other cases, increment error counter
	*/
	CFE_EVS_SendEvent(LC_LEN_ERR_EID, CFE_EVS_EventType_ERROR,
	"Invalid msg length: ID = 0x%08lX, CC = %d, Len = %d, Expected = %d",
	(unsigned long)CFE_SB_MsgIdToValue(MessageID), CommandCode, (int)ActualLength,
	(int)ExpectedLength);
	LC_AppData.CmdErrCount++;
	}

Expected behavior
All switch cases (including default) should be terminated by an unconditional break statement.

Reporter Info
Avi Weiss @thnkslprpt

Refactor LC_SampleAPReq and LC_HousekeepingReq

The functions LC_SampleAPReq and LC_Housekeeping Req could be refactored to use a loop instead of the multiple switch statements.

The final if condition in LC_SampleAPReq could also be refactored to reduce the nesting.

Imported from GSFCCFS-1368

Multiple return statements in LC_CreateTaskCDS and LC_TableInit

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Multiple returns are a coding style violation, and the implementation is challenging to follow (see #12 and others).

LC/fsw/src/lc_app.c

Lines 332 to 493 in bb91036

	int32 LC_TableInit(void)
	{
	int32 Result;
	/*
	** LC task use of Critical Data Store (CDS)
	**
	** Global application data (LC_AppData)
	** Watchpint results dump only table data
	** Actionpoint results dump only table data
	**
	** cFE Table Services use of CDS for LC task
	**
	** Watchpint definition loadable table data
	** Actionpoint definition loadable table data
	**
	** LC table initialization logic re CDS
	**
	** If LC cannot create all the CDS storage at startup, then LC
	** will disable LC use of CDS and continue.
	**
	** If LC cannot register definition tables as critical, then LC
	** will disable LC use of CDS and re-register tables as non-critical.
	**
	** If LC cannot register definition and results tables at startup,
	** then LC will terminate - table use is a required function.
	**
	** If LC can create all the CDS storage and register definition
	** tables as critical, then LC will write to CDS regardless of
	** whether LC was able to read from CDS at startup.
	**
	** If LC cannot restore everything from CDS at startup, then LC
	** will initialize everything - load default definition tables,
	** init results table contents, init global application data.
	*/
	/* lc_platform_cfg.h */
	#ifdef LC_SAVE_TO_CDS
	LC_OperData.HaveActiveCDS = true;
	#endif
	/*
	** Maintain a detailed record of table initialization results
	*/
	if (LC_OperData.HaveActiveCDS)
	{
	LC_OperData.TableResults |= LC_CDS_ENABLED;
	}
	/*
	** Create watchpoint and actionpoint result tables
	*/
	if ((Result = LC_CreateResultTables()) != CFE_SUCCESS)
	{
	return (Result);
	}
	/*
	** If CDS is enabled - create the 3 CDS areas managed by the LC task
	** (continue with init, but disable CDS if unable to create all 3)
	*/
	if (LC_OperData.HaveActiveCDS)
	{
	if (LC_CreateTaskCDS() != CFE_SUCCESS)
	{
	LC_OperData.HaveActiveCDS = false;
	}
	}
	/*
	** Create wp/ap definition tables - critical if CDS enabled
	*/
	if ((Result = LC_CreateDefinitionTables()) != CFE_SUCCESS)
	{
	return (Result);
	}
	/*
	** CDS still active only if we created 3 CDS areas and 2 critical tables
	*/
	if (LC_OperData.HaveActiveCDS)
	{
	LC_OperData.TableResults |= LC_CDS_CREATED;
	}
	/*
	** If any CDS area or critical table is not restored - initialize everything.
	** (might be due to reset type, CDS disabled or corrupt, table restore error)
	*/
	if (((LC_OperData.TableResults & LC_WRT_CDS_RESTORED) == LC_WRT_CDS_RESTORED) &&
	((LC_OperData.TableResults & LC_ART_CDS_RESTORED) == LC_ART_CDS_RESTORED) &&
	((LC_OperData.TableResults & LC_APP_CDS_RESTORED) == LC_APP_CDS_RESTORED) &&
	((LC_OperData.TableResults & LC_WDT_TBL_RESTORED) == LC_WDT_TBL_RESTORED) &&
	((LC_OperData.TableResults & LC_ADT_TBL_RESTORED) == LC_ADT_TBL_RESTORED))
	{
	LC_OperData.TableResults |= LC_CDS_RESTORED;
	/*
	** Get a pointer to the watchpoint definition table data...
	*/
	Result = CFE_TBL_GetAddress((void *)&LC_OperData.WDTPtr, LC_OperData.WDTHandle);
	if ((Result != CFE_SUCCESS) && (Result != CFE_TBL_INFO_UPDATED))
	{
	CFE_EVS_SendEvent(LC_WDT_GETADDR_ERR_EID, CFE_EVS_EventType_ERROR, "Error getting WDT address, RC=0x%08X",
	(unsigned int)Result);
	return (Result);
	}
	/*
	** Get a pointer to the actionpoint definition table data
	*/
	Result = CFE_TBL_GetAddress((void *)&LC_OperData.ADTPtr, LC_OperData.ADTHandle);
	if ((Result != CFE_SUCCESS) && (Result != CFE_TBL_INFO_UPDATED))
	{
	CFE_EVS_SendEvent(LC_ADT_GETADDR_ERR_EID, CFE_EVS_EventType_ERROR, "Error getting ADT address, RC=0x%08X",
	(unsigned int)Result);
	return (Result);
	}
	}
	else
	{
	Result = LC_LoadDefaultTables();
	if ((Result != CFE_SUCCESS) && (Result != CFE_TBL_INFO_UPDATED))
	{
	return (Result);
	}
	}
	/*
	** Create watchpoint hash tables -- also subscribes to watchpoint packets
	*/
	LC_CreateHashTable();
	/*
	** Display results of CDS initialization (if enabled at startup)
	*/
	if ((LC_OperData.TableResults & LC_CDS_ENABLED) == LC_CDS_ENABLED)
	{
	if ((LC_OperData.TableResults & LC_CDS_RESTORED) == LC_CDS_RESTORED)
	{
	CFE_EVS_SendEvent(LC_CDS_RESTORED_INF_EID, CFE_EVS_EventType_INFORMATION,
	"Previous state restored from Critical Data Store");
	}
	else if ((LC_OperData.TableResults & LC_CDS_UPDATED) == LC_CDS_UPDATED)
	{
	CFE_EVS_SendEvent(LC_CDS_UPDATED_INF_EID, CFE_EVS_EventType_INFORMATION,
	"Default state loaded and written to CDS, activity mask = 0x%08X",
	(unsigned int)LC_OperData.TableResults);
	}
	}
	else
	{
	CFE_EVS_SendEvent(LC_CDS_DISABLED_INF_EID, CFE_EVS_EventType_INFORMATION,
	"LC use of Critical Data Store disabled, activity mask = 0x%08X",
	(unsigned int)LC_OperData.TableResults);
	}
	return (CFE_SUCCESS);
	} /* LC_TableInit() */

LC/fsw/src/lc_app.c

Lines 735 to 855 in bb91036

	int32 LC_CreateTaskCDS(void)
	{
	int32 Result;
	uint32 DataSize;
	/*
	** Create CDS and try to restore Watchpoint Results Table (WRT) data
	*/
	DataSize = LC_MAX_WATCHPOINTS * sizeof(LC_WRTEntry_t);
	Result = CFE_ES_RegisterCDS(&LC_OperData.WRTDataCDSHandle, DataSize, LC_WRT_CDSNAME);
	if (Result == CFE_SUCCESS)
	{
	/*
	** Normal result after a power on reset (cold boot) - continue with next CDS area
	*/
	LC_OperData.TableResults |= LC_WRT_CDS_CREATED;
	}
	else if (Result == CFE_ES_CDS_ALREADY_EXISTS)
	{
	/*
	** Normal result after a processor reset (warm boot) - try to restore previous data
	*/
	LC_OperData.TableResults |= LC_WRT_CDS_CREATED;
	Result = CFE_ES_RestoreFromCDS(LC_OperData.WRTPtr, LC_OperData.WRTDataCDSHandle);
	if (Result == CFE_SUCCESS)
	{
	LC_OperData.TableResults |= LC_WRT_CDS_RESTORED;
	}
	}
	else
	{
	CFE_EVS_SendEvent(LC_WRT_CDS_REGISTER_ERR_EID, CFE_EVS_EventType_ERROR,
	"Error registering WRT CDS Area, RC=0x%08X", (unsigned int)Result);
	return (Result);
	}
	/*
	** Create CDS and try to restore Actionpoint Results Table (ART) data
	*/
	DataSize = LC_MAX_ACTIONPOINTS * sizeof(LC_ARTEntry_t);
	Result = CFE_ES_RegisterCDS(&LC_OperData.ARTDataCDSHandle, DataSize, LC_ART_CDSNAME);
	if (Result == CFE_SUCCESS)
	{
	/*
	** Normal result after a power on reset (cold boot) - continue with next CDS area
	*/
	LC_OperData.TableResults |= LC_ART_CDS_CREATED;
	}
	else if (Result == CFE_ES_CDS_ALREADY_EXISTS)
	{
	/*
	** Normal result after a processor reset (warm boot) - try to restore previous data
	*/
	LC_OperData.TableResults |= LC_ART_CDS_CREATED;
	Result = CFE_ES_RestoreFromCDS(LC_OperData.ARTPtr, LC_OperData.ARTDataCDSHandle);
	if (Result == CFE_SUCCESS)
	{
	LC_OperData.TableResults |= LC_ART_CDS_RESTORED;
	}
	}
	else
	{
	CFE_EVS_SendEvent(LC_ART_CDS_REGISTER_ERR_EID, CFE_EVS_EventType_ERROR,
	"Error registering ART CDS Area, RC=0x%08X", (unsigned int)Result);
	return (Result);
	}
	/*
	** Create CDS and try to restore Application (APP) data
	*/
	DataSize = sizeof(LC_AppData_t);
	Result = CFE_ES_RegisterCDS(&LC_OperData.AppDataCDSHandle, DataSize, LC_APPDATA_CDSNAME);
	if (Result == CFE_SUCCESS)
	{
	/*
	** Normal result after a power on reset (cold boot) - continue with next CDS area
	*/
	LC_OperData.TableResults |= LC_APP_CDS_CREATED;
	}
	else if (Result == CFE_ES_CDS_ALREADY_EXISTS)
	{
	/*
	** Normal result after a processor reset (warm boot) - try to restore previous data
	*/
	LC_OperData.TableResults |= LC_APP_CDS_CREATED;
	Result = CFE_ES_RestoreFromCDS(&LC_AppData, LC_OperData.AppDataCDSHandle);
	if ((Result == CFE_SUCCESS) && (LC_AppData.CDSSavedOnExit == LC_CDS_SAVED))
	{
	/*
	** Success - only if previous session saved CDS data at least once
	*/
	LC_OperData.TableResults |= LC_APP_CDS_RESTORED;
	/*
	** May need to override the restored application state
	*/
	#if LC_STATE_WHEN_CDS_RESTORED != LC_STATE_FROM_CDS
	LC_AppData.CurrentLCState = LC_STATE_WHEN_CDS_RESTORED;
	#endif
	}
	}
	else
	{
	CFE_EVS_SendEvent(LC_APP_CDS_REGISTER_ERR_EID, CFE_EVS_EventType_ERROR,
	"Error registering application data CDS Area, RC=0x%08X", (unsigned int)Result);
	return (Result);
	}
	return (CFE_SUCCESS);
	} /* LC_CreateTaskCDS() */

Describe the solution you'd like
Single entry/exit point from functions. Refactor to simplify flow.

Describe alternatives you've considered
None

Additional context
Could group w/ multiple issues or tackle by file.

Requester Info
Jacob Hageman - NASA/GSFC

Remove "return;" from last line of void functions

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
"return;" at last line in void function is redundant.
Copy of nasa/cFE#1540

Describe the solution you'd like
Remove

Reporter Info
@thnkslprpt

Update CLA Information

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the cFS README.md file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Have new CLAs given the change in nasa/cFS#491 with the combined CLA,

Describe the solution you'd like

• Update the instructions in each app's Contributing.md
• Delete old CLA pdfs
• Update PR and Issue templates as needed

Describe alternatives you've considered

None

Additional context
None

Requester Info
Gerardo E. Cruz-Ortiz

Fill in missing requirements rationale

There's a handful of missing requirements rationale. Add.

Imported from GSFCCFS-2019

LC Hash logic comments only applicable to V1 Message IDs

If the V1 Message ID isn't used, theoretically the hash could collide up to the entire length of the linked list.

At minimum the comments should reflect the possibility for more hash collisions, but might be worth reconsidering implementation or reporting collision depth.

Imported from GSFCCFS-1881

LC - more deterministic behavior

Currently LC will process messages when they are received, which is fine, generally. Also, currently, LC uses a single pipe for commands and for watchpoint telemetry. LC doesn't process action points until it receives a command message.

But in deterministic environments it may be better to have LC be commanded to read telemetry as well as be commanded to process action points. This will necessitate having a separate message pipe for commanding (which the main loop would block on) and a telemetry pipe (which would accumulate telemetry until the command to read.)

The default LC behavior should remain the same (process messages as they arrive.) This should be a compile-time option, or perhaps run-time command-able.

Imported from GSFCCFS-769

Revisit coverage, update to 100% code/branch or write issues where unreachable

Failure: Coverage lc lines 99.6% functions 100.0% branches 94.3%

Other coverage failures:
Failure: Coverage cs lines 99.9% functions 100.0% branches 99.1%
Failure: Coverage ds lines 99.6% functions 100.0% branches 97.7%
Failure: Coverage fm lines 93.8% functions 94.0% branches 89.1%
Failure: Coverage hs lines 100.0% functions 100.0% branches 98.1%
Failure: Coverage md lines 98.4% functions 100.0% branches 96.7%
Failure: Coverage sc lines 99.9% functions 100.0% branches 99.0%

Fix where possible, elsewise Issues should document all uncovered lines/branches and disposition (why it's ok as-is)

Imported from GSFCCFS-1935

LC "IsNAN" check relies on platform-defined behavior (non-standard)

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
With floating point watchpoints, the watch point checking code looks for the IEEE754 not-a-number (NaN) value before doing other comparisons.

However, the method used for checking this involves accessing the float value as a uint32, and checking the bits per IEEE754.

This type of action is not standardized by the C language, and results of doing this are platform-defined.

To Reproduce
N/A (it does work as intended on most compilers, it is just not standard or portable "by the book")

Expected behavior
Should only rely on behavior that is specified by ISO C.

Regarding NaN, the C standard does guarantee that a NaN is never equal to any other value, even itself. Therefore, the generally recommended, portable way to check for NaN is by checking that, e.g.:

if (value == value)
{
    /* value is valid */
}
else
{
    /* value is NaN */
}

Code snips
Code at issue is here:

LC/fsw/src/lc_watch.c

Lines 1071 to 1097 in 358fc48

	bool LC_Uint32IsNAN(uint32 Data)
	{
	bool Result = false;
	uint32 Exponent;
	uint32 Fraction;
	/*
	** Check if the exponent field is all 1's
	*/
	Exponent = Data & LC_IEEE_EXPONENT_MASK;
	if (Exponent == LC_IEEE_EXPONENT_MASK)
	{
	/*
	** If the fraction field is also non-zero,
	** it's a NAN
	*/
	Fraction = Data & LC_IEEE_FRACTION_MASK;
	if (Fraction > 0)
	{
	Result = true;
	}
	}
	return Result;
	}

System observed on:
Code Inspection when doing EDS implementation

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

Support platform-endian byte order in WDT

LC should support numerical telemetry data types that are platform-endian. While it would be possible to get the same effect with an #if macro block, it would make the table very hard to read.

Imported from GSFCCFS-770

LC_TableInit has an if/else if without an else clause and its behavior is undefined

So at the end of the LC_TableInit function there is an odd branching

if (LC_CDS_ENABLED)
{
if (LC_CDS_RESTORED)
{
restored event
}
else if (LC_CDS_UPDATED)
{
default event
}
// nothing else here !!!
}
else
{
CDS disabled event
}

So, the question is: Can we have a scenario where LC_CDS_ENABLED is TRUE, but both LC_CDS_RESTORED and LC_CDS_UPDATED are FALSE? And IF SO: What is the desired behavior here?? This is a situation where the lack of an else clause on an "else if" most definitively is cause for concern. If that scenario cannot exist, then it would seem an "else if" is not required. Unfortunately, due to the convoluted nature of this function and those that it calls, it is would be difficult to determine if the above scenario is a possibility.

Imported from GSFCCFS-1104

Inconsistent Event ID naming

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
Copy of nasa/cFE#2175
After finding that there were 9 different Event IDs to indicate the same thing (Invalid Message ID) in nasa/CF#262, I scrubbed the other common commands (e.g. Task Initialisation [INIT], NOOP, Reset Counters etc.) and found the same issue there - almost every component/app had their own variation of the Event ID name for the exact same event.

Expected behavior
Apply consistent Event ID names to the events which are common to all/most components and apps.

Code snips
Invalid Message ID:
CFE_EVS_ERR_MSGID_EID
CFE_SB_BAD_MSGID_EID
CFE_TIME_ID_ERR_EID
CS_MID_ERR_EID
TO_LAB_MSGID_ERR_EID
SAMPLE_APP_INVALID_MSGID_ERR_EID
BP_INVALID_MID_ERR_EID
SCH_MD_ERR_EID
CI_LAB_COMMAND_ERR_EID

Initialization:
CFE_TIME_INIT_EID
CFE_TBL_INIT_INF_EID
CFE_EVS_STARTUP_EID
CF_EID_INF_INIT
BP_INIT_APP_INFO_EID
SCH_INITSTATS_INF_EID
CI_LAB_STARTUP_INF_EID

NOOP:
CFE_TIME_NOOP_EID
CFE_TBL_NOOP_INF_EID
CFE_SB_CMD0_RCVD_EID
CF_EID_INF_CMD_NOOP
FM_NOOP_CMD_EID
CI_LAB_COMMANDNOP_INF_EID

Reset Counters:
CFE_TIME_RESET_EID
CFE_TBL_RESET_INF_EID
CFE_EVS_RSTCNT_EID
CFE_SB_CMD1_RCVD_EID
CF_EID_INF_CMD_RESET
SC_RESET_DEB_EID
HS_RESET_DBG_EID
FM_RESET_CMD_EID
HK_RESET_CNTRS_CMD_EID
MD_RESET_CNTRS_DBG_EID
CI_LAB_COMMANDRST_INF_EID

etc.

Reporter Info
Avi Weiss @thnkslprpt

Delete duplicate headers files in src

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
There are duplicate files "fsw/src/lc_msg.h" and "fsw/src/lc_msgdefs.h" that need to be removed.

To Reproduce
View fsw directory

Expected behavior
Single file of each

Code snips
If applicable, add references to the software.

System observed on:

• Ubuntu 22.04

Additional context
Bug introduced in #56

Reporter Info
Justin Figueroa, Vantage Systems

Replace quotes with angle brackets in #include statements within /inc

Describe the solution you'd like
Quotes should be replaced with angle brackets in #include statements that reside in the /inc location. This will ensure that the preprocessor selects the files pre-designated to override the default files contained within the open source cFS build release - as opposed to selecting those located in the same directory.

Requester Info
Dan Knutsen
NASA Goddard

LC has duplicate conditions leading to untestable branches

LC_ValidateWDT has duplicate conditions in its switch statement for both DataType and OperatorID. This leads to branches that cannot be covered by unit testing.

Imported from GSFCCFS-1731

EDIT:
Duplicate conditional in PUSH_RPN_DATA, since the limit is checked before the push and can never be exceeded

LC/fsw/src/lc_action.c

Line 298 in bb91036

#define PUSH_RPN_DATA(x) ((StackPtr >= LC_MAX_RPN_EQU_SIZE) ? (IllegalRPN = true) : (RPNStack[StackPtr++] = x))

Setting Done to true when IllegalOperand is true makes the checks redundant

LC/fsw/src/lc_action.c

Lines 416 to 423 in bb91036

	if ((EvalResult == LC_WATCH_ERROR) || (EvalResult == LC_WATCH_STALE))
	{
	IllegalOperand = true;
	}
	if (StackPtr == 0)
	{
	Done = true;
	}

LC/fsw/src/lc_action.c

Line 450 in bb91036

if ((Done == false) && (IllegalRPN == false) && (IllegalOperand == false))

LC/fsw/src/lc_action.c

Line 321 in bb91036

while ((Done == false) && (IllegalRPN == false) && (IllegalOperand == false))

Recommended resolutions:
Remove duplicate condions in LC_ValidateWDT, replace PUSH_RPN_DATA with simple push, don't set Done when IllegalOperand.

Redundant comments (/* end of function */, /* end if */ etc.) and clean up empty lines.

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
Copy of nasa/to_lab#68 and nasa/sample_app#111
There are quite a few redundant comments in the code, such as:

• /* end of function */-type comments
• /* end if */-type comments
• function header comments which include the function name

Another minor issue has to do with empty lines:
a) unnecessary empty lines (e.g. first line after the opening brace of a function/struct, or the last line before the closing brace - the latter apparently sometimes triggers the CI format checks).
b) missing empty lines between functions (i.e. closing brace of last function, then next function beginning on the immediately next line without an empty line in between)

The unnecessary empty lines (at the beginning or end of a function, for example) represent a low single-digit percentage of the cases (the vast majority of functions/structs do not have these extra empty lines), so there is an argument to remove them purely for consistency, not just due to them being redundant and triggering the CI format checks.

Expected behavior
Remove redundant comments to reduce clutter and inconsistency in the code, and improve readability.

Reporter Info
@thnkslprpt

Scrub use of () in return statements

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
Unnecessary parentheses around return values in LC
Copy of nasa/osal#866

Reporter Info
@thnkslprpt

Suggest reversing order in which AP/WP telemetry is stored

LC builds AP/WP status/results starting with most significant bits first. i.e. AP 1 state in most significant 2 bits, then AP 1 results, then AP 0 state, and AP 0 results in least significant bits. When doing an array of bit fields, would be nice to have AP 0 in the most significant bits.

Imported from GSFCCFS-1119

Improper use of unions in LC_GetSizedWPData

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The LC_GetSizedWPData function is not using the LC_MultiType_t union properly. It is writing to one member and then reading from another, different member of the same union. This is "type punning" and may not work in an optimized build.

The new version of CppCheck reports this issue.

To Reproduce
Run Cppcheck workflow to see issue.
No known way to actually produce a failure though, as most platforms will behave as the code expects it to, its just not guaranteed to work.

Expected behavior
Should not read from a different union member than was written to.

Code snips
This writes to Unsigned8 but then reads from Signed8:

LC/fsw/src/lc_watch.c

Lines 854 to 855 in f49a965

	TempBuffer.Unsigned8 = *WPDataPtr;
	ConvBuffer.Signed32 = TempBuffer.Signed8; /* Extend signed 8 bit value to 32 bits */

Additionally, Many cases write to Signed32 but only Unsigned32 is read here at the end:

LC/fsw/src/lc_watch.c

Line 922 in f49a965

*SizedDataPtr = ConvBuffer.Unsigned32;

System observed on:
N/A

Additional context
This code does work as intended but is not necessarily safe/portable across platforms in its current form, particularly when optimization is enabled.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

ADT table size limit

Hello,

I would like to know if it is possible to increase the WDT table size (LC_MAX_ACTIONPOINTS). How much I can extend the size of WDT table ? How many AP I can define? Is there any limit?

Thanks.

Remove remaining stray references to old event types

Checklist

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
While updating event type constants in nasa/cFE#2221 it was noticed that a few stray uses of the old types (CFE_EVS_ERROR, CFE_EVS_INFORMATION etc.) were still present in LC's commented-out tests.

Expected behavior
Update to new event type constants or remove these commented-out tests.

Code snips

LC/unit-test/lc_app_tests.c

Line 470 in 358fc48

(Ut_CFE_EVS_EventSent(LC_SUB_HK_REQ_ERR_EID, CFE_EVS_ERROR, "Error Subscribing to HK Request, MID=0x18A5,

LC/unit-test/lc_app_tests.c

Line 283 in 358fc48

/* UtAssert_True (Ut_CFE_EVS_EventSent(LC_INIT_INF_EID, CFE_EVS_INFORMATION, Message), Message); */

Reporter Info
Avi Weiss @thnkslprpt

Remove all mentions of ASIST or ITOS artifacts from documentation

Some of our doxygen docs still reference ASIST or ITOS artifacts.

See
cfs_mm repo: fsw/src/mm_msgdefs.h:l28

Imported from GSFCCFS-1929

All CMD/TLM message should put content in a "Payload" sub-structure

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I reviewed the README file to see if the feature is in the major future work.
• I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
To match the patterns used in CFE and other modules, all CMD/TLM message definitions should put the content (non-header) parts into a separate struct called "Payload".

Describe the solution you'd like
Separate message content into a sub structure called "Payload".

Additional context
This is benefit to tooling that can use the presence of this field to identify where the actual content starts (e.g. something like offsetof(MsgType, Payload) would work and be correct, as opposed to checking sizeof(CFE_MSG_CommandHeader_t) which may not actually reflect where the content starts due to possible compiler-added padding between them).

Requester Info
Joseph Hickey, Vantage Systems, Inc.