nasa/hk

The Core Flight System (cFS) Housekeeping (HK) application.

core Flight System (cFS) Housekeeping Application (HK)

Introduction

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

The HK application is used for building and sending combined telemetry messages (from individual system applications) to the software bus for routing. Combining messages is performed in order to minimize downlink telemetry bandwidth. Combining certain data from multiple messages into one message eliminates the message headers that would be required if each message was sent individually. Combined messages are also useful for organizing certain types of data. This application may be used for data types other than housekeeping telemetry. HK provides the capability to generate multiple combined packets (a.k.a. output packets) so that data can be sent at different rates (e.g. a fast, medium and slow packet).

The HK application is written in C and depends on the cFS Operating System Abstraction Layer (OSAL) and cFE components. There is additional HK 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/hk-usersguide hk-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

hk's Issues

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

Several printf spec string format conversions are incorrect

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
Several printf format conversions in HK are incorrect

To Reproduce
Build on system with different type mapping for int32, etc.

Expected behavior
Build should succeed

System observed on:
RTEMS 4.11

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

HK missing CFE_ES_PerfLogEntry(HK_APPMAIN_PERF_ID) calls after CFE_SB_ReceiveBuffer in HK_AppMain

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
Looks like HK is missing a couple of CFE_ES_PerfLogEntry(HK_APPMAIN_PERF_ID); calls after the CFE_SB_ReceiveBuffer in void HK_AppMain(void).

To Reproduce
Code inspection

Expected behavior
Every time the app wakes up, it should have an CFE_ES_PerfLogEntry call.
Every time the app wakes is about to block, it should have an CFE_ES_PerfLogExit call.

Code snips

    while (CFE_ES_RunLoop(&HK_AppData.RunStatus) == true)
    {
        /*
        ** Performance Log Exit Stamp.
        */
        CFE_ES_PerfLogExit(HK_APPMAIN_PERF_ID);

        /*
        ** Pend on the arrival of the next Software Bus message.
        */
        Status = CFE_SB_ReceiveBuffer(&BufPtr, HK_AppData.CmdPipe, HK_SB_TIMEOUT);

        if (Status == CFE_SUCCESS)
        {
            /*
            ** Performance Log Entry Stamp.
            */
            CFE_ES_PerfLogEntry(HK_APPMAIN_PERF_ID);

            /* Perform Message Processing */
            HK_AppPipe(BufPtr);
        }
        else if (Status == CFE_SB_TIME_OUT)
        {
            /* Check for copy table load and runtime dump request. This is
             * generally done during the housekeeping cycle.  If we are
             * getting routine messages at a rate of less than 1Hz we do
             * the routine maintenance here. */
            if (HK_CheckStatusOfTables() != HK_SUCCESS)
            {
                HK_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
            }
        }
        else
        {
            CFE_EVS_SendEvent(HK_RCV_MSG_ERR_EID, CFE_EVS_EventType_ERROR,
                              "HK_APP Exiting due to CFE_SB_RcvMsg error 0x%08X", (unsigned int)Status);

            /* Write to syslog in case there is a problem with event services */
            CFE_ES_WriteToSysLog("HK_APP Exiting due to CFE_SB_RcvMsg error 0x%08X\n", (unsigned int)Status);

            HK_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
        }
    } /* end while */

System observed on:

• Hardware: any
• OS: any
• Versions: I'm looking at the draco rc4 release, but it looks like this is HK v2.5.1

Additional context
Add any other context about the problem here.

Reporter Info
Keegan Moore
NASA/GSFC

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.

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

Remove dependency on CFE_PLATFORM_TLM_MID_BASE in tests

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.

Describe the bug
The HK app depends on CFE_PLATFORM_TLM_MID_BASE which may or may not exist in the user-defined config

To Reproduce
Build HK with latest mainline

Expected behavior
Should not depend on this symbol

System observed on:
Debian

Additional context
Unit test issue, does not affect FSW

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

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

HK `int32` return codes and variables should be converted to `CFE_Status_t`

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
HK has quite a few return codes (as well as local status/return variables which hold CFE return codes) that can be easily converted over to the CFE_Status_t typedef.

Expected behavior
Use the more expressive CFE_Status_t and improve consistency with cFS.

Reporter Info
Avi Weiss @thnkslprpt

Static analysis workflow 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.
Style warnings in strict cppcheck static analysis workflow causing workflow failure:

[fsw/src/hk_utils.c:47] -> [fsw/src/hk_utils.c:59]: (style) Variable 'StartOfCopyTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:49] -> [fsw/src/hk_utils.c:60]: (style) Variable 'StartOfRtTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:371] -> [fsw/src/hk_utils.c:378]: (style) Variable 'StartOfRtTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:437] -> [fsw/src/hk_utils.c:439]: (style) Variable 'HKStatus' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:457] -> [fsw/src/hk_utils.c:461]: (style) Variable 'Status' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:482] -> [fsw/src/hk_utils.c:485]: (style) Variable 'Status' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:556] -> [fsw/src/hk_utils.c:560]: (style) Variable 'Status' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:601] -> [fsw/src/hk_utils.c:606]: (style) Variable 'StartOfCopyTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:603] -> [fsw/src/hk_utils.c:607]: (style) Variable 'StartOfRtTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:640] -> [fsw/src/hk_utils.c:645]: (style) Variable 'StartOfCopyTable' is reassigned a value before the old one has been used.
[fsw/src/hk_utils.c:642] -> [fsw/src/hk_utils.c:646]: (style) Variable 'StartOfRtTable' 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

Add check within HK_ValidateHkCopyTable to verify max table/combined packet size

Checklist (Please check before submitting)

• [ x] I reviewed the Contributing Guide.
• [ x] I reviewed the README file to see if the feature is in the major future work.
• [ x] 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.
It is possible to for user's to build HK copy tables/combined packets that violate project specific requirements in regards to max packet size.

Describe the solution you'd like
Add logic inside of HK_ValidateHkCopyTable to determine the size of the resulting combined packet and ensure that it is less than the platform/project defined max value.

Requester Info
Dan Knutsen
NASA Goddard

Split "platform_cfg" into external and internal components

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.
Currently, all configurable items for the app are contained in a single hk_platform_cfg.h header file. This contains macro definitions that affect both the internal operation of the application (such as operational limits) as well as the external interface in CMD/TLM and table files.

Describe the solution you'd like
Split this header into two components, one that contains only public items (i.e. those that affect CMD/TLM/Table definitions) and one that contains private/internal items that are only used within the local application code and do not affect the interface.

Additional context
Mainly a scoping concern, separate files for separate scopes. These files could also be generated in the future, but scope still needs to be consistent.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

Default InputOffset in HK Copy Table Needs Update for New Tlm Hdr Size

Checklist (Please check before submitting)

• [z] 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 default copy table looks at byte offset 12 of the cFE messages. The new Tlm secondary header padding for 64-bit alignment causes the start of data to be at 16 bytes into the packet.

To Reproduce
Steps to reproduce the behavior:

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Expected behavior
A clear and concise description of what you expected to happen.

Code snips
Currently:

    /*   0 */ {
        CFE_SB_MSGID_WRAP_VALUE(CFE_EVS_HK_TLM_MID),
        12,
        CFE_SB_MSGID_WRAP_VALUE(HK_COMBINED_PKT1_MID),
        12,
        4,
    },
    /*   1 */
    {
        CFE_SB_MSGID_WRAP_VALUE(CFE_TIME_HK_TLM_MID),
        12,
        CFE_SB_MSGID_WRAP_VALUE(HK_COMBINED_PKT1_MID),
        16,
        4,
    },
    /*   2 */
    {
        CFE_SB_MSGID_WRAP_VALUE(CFE_SB_HK_TLM_MID),
        12,
        CFE_SB_MSGID_WRAP_VALUE(HK_COMBINED_PKT1_MID),
        20,
        4,
    },
    /*   3 */
    {
        CFE_SB_MSGID_WRAP_VALUE(CFE_ES_HK_TLM_MID),
        12,
        CFE_SB_MSGID_WRAP_VALUE(HK_COMBINED_PKT1_MID),
        24,
        4,
    },
    /*   4 */
    {
        CFE_SB_MSGID_WRAP_VALUE(CFE_TBL_HK_TLM_MID),
        12,
        CFE_SB_MSGID_WRAP_VALUE(HK_COMBINED_PKT1_MID),
        28,
        4,
    },

The input offset for each should be 16, and the output offset should start at 16 (16, 20, 24, 28, 32).

System observed on:

• Hardware: x64 (but should work for any)
• OS: PC-Linux (but should work for any)
• Versions cFE Draco rc4

Additional context

Additionally (optionally), the current table reads the first 4 bytes out of each packet. I think it would make more sense to only read the command count and command error count (the first 2 bytes) of each packet. If this also gets implemented, remember to update the numBytes accordingly.

Reporter Info
Keegan Moore
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

Simplify HK_SendCombinedHKCmd and remove stray printfs

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.
Unnecessary variable:

HK/fsw/src/hk_app.c

Lines 397 to 406 in fbb231e

	void HK_SendCombinedHKCmd(const CFE_SB_Buffer_t *BufPtr)
	{
	CFE_SB_MsgId_t WhichCombinedPacket = CFE_SB_INVALID_MSG_ID;
	HK_Send_Out_Msg_t *CmdPtr = (HK_Send_Out_Msg_t *)BufPtr;
	WhichCombinedPacket = CmdPtr->OutMsgToSend;
	HK_SendCombinedHkPacket(WhichCombinedPacket);
	} /* end of HK_SendCombinedHKCmd() */

Stray printfs (leftover debugging?):

HK/fsw/src/hk_utils.c

Line 156 in fbb231e

printf("after first loop\n");

HK/fsw/src/hk_utils.c

Line 497 in fbb231e

printf("Calling HK_ProcessNewCopyTable\n");

Describe the solution you'd like
Clean/remove.

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

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

Add HK_UNDEFINED_ENTRY check to hk_verify.h

hk_verify.h recommendation:

HK_UNDEFINED_ENTRY is #define to 0 in hk_utils.h. I would consider, since any uninitialized entries in the table will be zeroed out, to put:
#if HK_UNDEFINED_ENTRY != 0
#error HK_UNDEFINED_ENTRY must be set to 0
#endif

Imported from GSFCCFS-986

Use Generated Stubs

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.
HK unit testing currently uses a set of stubs for its internal units that are not generated by the tool

Describe the solution you'd like
Use the generated stubs directly whenever possible, as this makes future maintenance easier - when an API changes, just re-run the generator tool to update the stubs.

Additional context
This requires some additional separation of items - global variable stubs should be in a separate compilation unit, as the tool does not generate these.

Requester 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

Change HK_INPUTMID_SUBSCRIBED to 1

In hk_utils.h suggest setting HK_INPUTMID_SUBSCRIBED to 1 instead of 0xFF.

Finding in code review.

Imported from GSFCCFS-987

MemPoolHandle should be of type CFE_ES_MemHandle_t, not uint32

See

HK/fsw/src/hk_app.h

Line 78 in cf59498

uint32 MemPoolHandle;/**< \brief HK mempool handle for output pkts */

This causes HK to exit while running on a 64 bit platform, as an 8byte pointer is written to this and overwrites the adjacent RunStatus field.

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

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 HK
Copy of nasa/osal#866

Reporter Info
@thnkslprpt

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 redundant use of "dummy" in test code

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
Redundant use of the word "dummy" in some variables/text in the test code. All test variables code are 'dummy' variables - no need to label it as such.

Code snips

HK/unit-test/hk_app_tests.c

Line 1095 in 71fe47a

CFE_SB_Buffer_t DummyBuf;

HK/unit-test/hk_app_tests.c

Line 1223 in 71fe47a

CFE_MSG_CommandHeader_t DummyMsg;

Expected behavior
Simplifies test code.

Reporter Info
Avi Weiss @thnkslprpt

Compiler warning in HK unit tests

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 sufficient compiler options enabled, I get the following warning:

apps/hk/unit-test/hk_app_tests.c:1020:5: error: ‘Buf’ may be used uninitialized [-Werror=maybe-uninitialized]
 1020 |     HK_ResetCountersCmd(&Buf);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~

To Reproduce
Enable full warnings and at least -O1 optimization level

Expected behavior
Should build cleanly

System observed on:
Debian (GCC 12.2)

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

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

HK Count track times packet is not found

In file hk_utils.c function HK_SendCombinedHkPacket recommendation:

HK app hk packet could keep track of number of times packet is not found. HK_AppData.MissingDataCtr could be repurposed for this.

Finding from code review

Imported from GSFCCFS-991

Remove UT TODOs and clean up related tests

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.

HK/unit-test/hk_app_tests.c

Lines 1192 to 1208 in fbb231e

	void Test_HK_SendCombinedHKCmd(void)
	{
	/* Arrange */
	CFE_SB_Buffer_t DummyBuf;
	uint8 call_count_HK_SendCombinedHkPacket;
	/* TODO set return value of CFE_SB_GetUserData and check input to
	HK_SendCombinedHkPacket */
	/* Act */
	HK_SendCombinedHKCmd(&DummyBuf);
	call_count_HK_SendCombinedHkPacket = UT_GetStubCount(UT_KEY(HK_SendCombinedHkPacket));
	/* Assert */
	UtAssert_INT32_EQ(call_count_HK_SendCombinedHkPacket, 1);
	}

Describe the solution you'd like
Remove TODO and clean

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

Uninitialized variable in unit tests

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
Compiling the latest HK app with the latest GCC version (12.x) produces a warning about uninitialized variable in the unit test:

[ 93%] Building C object apps/hk/unit-test/CMakeFiles/coverage-hk-hk_app-testrunner.dir/hk_app_tests.c.o
/home/joe/code/cfecfs/github/apps/hk/unit-test/hk_app_tests.c: In function ‘Test_HK_HousekeepingCmd’:
/home/joe/code/cfecfs/github/apps/hk/unit-test/hk_app_tests.c:1235:5: error: ‘DummyMsg’ may be used uninitialized [-Werror=maybe-uninitialized]
 1235 |     HK_HousekeepingCmd(&DummyMsg);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/joe/code/cfecfs/github/apps/hk/unit-test/hk_app_tests.c:24:
/home/joe/code/cfecfs/github/apps/hk/unit-test/../fsw/src/hk_app.h:181:6: note: by argument 1 of type ‘const CFE_MSG_CommandHeader_t *’ {aka ‘const struct CFE_MSG_CommandHeader *’} to ‘HK_HousekeepingCmd’ declared here
  181 | void HK_HousekeepingCmd(const CFE_MSG_CommandHeader_t *Msg);
      |      ^~~~~~~~~~~~~~~~~~

To Reproduce
Add HK to latest CFS bundle, build using default config.

Expected behavior
Should build cleanly

System observed on:
Debian (Latest version, with GCC 12.2)

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

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

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

Multiple returns in `HK_AppInit()` and `HK_TableInit()` can be refactored out

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
The only places multiple returns remain in HK are in HK_AppInit() and HK_TableInit(). This is unnecessary.

Expected behavior
Refactor to a single exit point.

Reporter Info
Avi Weiss @thnkslprpt

Change HK_UNDEFINED_ENTRY to an actual undefined value

The current value of HK_UNDEFINED_ENTRY in hk_utils.h is 0, which is a potentially valid entry value. We should consider changing the definition to something that is actually an undefinable value, for example 0xFFFF.

Imported from GSFCCFS-1076

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

New Cppcheck errors: '[unreadVariable]'

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
Latest version of Cppcheck is issuing the following failures for HK:

fsw/src/hk_utils.c:57:46: style: Variable 'LastByteAccessed' is assigned a value that is never used. [unreadVariable]
    int32                   LastByteAccessed = 0;
                                             ^
fsw/src/hk_utils.c:126:55: style: Variable 'Loop2' is assigned a value that is never used. [unreadVariable]
    int32                   Loop2                     = 0;
                                                      ^
fsw/src/hk_utils.c:127:55: style: Variable 'MidOfThisPacket' is assigned a value that is never used. [unreadVariable]
    CFE_SB_MsgId_t          MidOfThisPacket           = CFE_SB_INVALID_MSG_ID;
                                                      ^
fsw/src/hk_utils.c:128:55: style: Variable 'SizeOfThisPacket' is assigned a value that is never used. [unreadVariable]
    int32                   SizeOfThisPacket          = 0;
                                                      ^
fsw/src/hk_utils.c:129:55: style: Variable 'FurthestByteFromThisEntry' is assigned a value that is never used. [unreadVariable]
    int32                   FurthestByteFromThisEntry = 0;
                                                      ^
fsw/src/hk_utils.c:130:55: style: Variable 'NewPacketAddr' is assigned a value that is never used. [unreadVariable]
    CFE_SB_Buffer_t *       NewPacketAddr             = 0;
                                                      ^
fsw/src/hk_utils.c:131:55: style: Variable 'Result' is assigned a value that is never used. [unreadVariable]
    int32                   Result                    = CFE_SUCCESS;
                                                      ^
fsw/src/hk_utils.c:273:46: style: Variable 'Loop2' is assigned a value that is never used. [unreadVariable]
    int32                   Loop2            = 0;
                                             ^
fsw/src/hk_utils.c:274:46: style: Variable 'MidOfThisPacket' is assigned a value that is never used. [unreadVariable]
    CFE_SB_MsgId_t          MidOfThisPacket  = CFE_SB_INVALID_MSG_ID;
                                             ^
fsw/src/hk_utils.c:277:46: style: Variable 'Result' is assigned a value that is never used. [unreadVariable]
    int32                   Result           = CFE_SUCCESS;
                                             ^
##[error]Process completed with exit code 255.

To Reproduce
Run the current version of Cppcheck on the current main branch HK source code.

Expected behavior
Cppcheck should pass without raising any errors.

Reporter Info
Avi @thnkslprpt

Apps should check/verify msg BEFORE calling handler

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.
This remains an area with coding pattern discrepancies between CFE core and CFS apps, and also different between CFS apps to some degree as well.

The CFE core and sample app (which is supposed to be the example of "best practice") do validation on the message before calling the handler. For example:

        case SAMPLE_APP_NOOP_CC:
            if (SAMPLE_APP_VerifyCmdLength(&SBBufPtr->Msg, sizeof(SAMPLE_APP_NoopCmd_t)))
            {
                SAMPLE_APP_Noop((SAMPLE_APP_NoopCmd_t *)SBBufPtr);
            }

This is different from HK, which does a similar check, but done inside each handler, for example:

HK/fsw/src/hk_app.c

Lines 448 to 453 in 71fe47a

	void HK_ResetCtrsCmd(const CFE_SB_Buffer_t *BufPtr)
	{
	size_t ExpectedLength = sizeof(HK_ResetCountersCmd_t);
	if (HK_VerifyCmdLength(BufPtr, ExpectedLength) == HK_BAD_MSG_LENGTH_RC)
	{

Describe the solution you'd like
CFS Apps should follow the best practices/patterns set forth in the framework code. (there are reasons for the pattern being recommended practice)

Additional context
The pattern recommended in the framework (checking before calling, as done in sample_app) has several advantages:

1. Each command handler function has a unique type-safe prototype, that cannot be interchanged with another handler without triggering a type mismatch compiler error.
2. All typecasting/conversions are confined to one place, and it is nearby to the place that the verification is done - which eases maintainability because check and conversion are all in close proximity and any mismatches will be more visible.
3. It spreads out the cyclomatic complexity. In the non-recommended pattern, there is a case where the length check fails, and the entire handler is essentially skipped. This adds to the cyclomatic complexity of every handler. In the recommended pattern, those checks are done prior to the invocation of the handler, so the handler can focus solely on its intended purpose - doing the command itself.

Requester Info
Joseph Hickey, Vantage Systems, Inc.