EVS "CFE_PLATFORM_EVS_LOG_ON" option unit test failure

[jphickey]

Describe the bug
The Event Services subsystem has a broken compile-time platform option called CFE_PLATFORM_EVS_LOG_ON. The description says: "The CFE_PLATFORM_EVS_LOG_ON configuration parameter must be defined to enable EVS event logging"

If UT is disabled, then CFE core itself actually seems to build and run OK. However, certain risky things are not clear in the code that:

• The EVS_SharedDataMutexID will be left uninitialized
• The EVS_LogPtr will be left as NULL

The code that accesses these seems to be mostly protected by checking the separate CFE_EVS_GlobalData.EVS_TlmPkt.Payload.LogEnabled member boolean in the outgoing telemetry packet. This seems like a weak design, in particular because the telemetry packet is supposed to be informational, not an active control structure.

To Reproduce
Disable the CFE_PLATFORM_EVS_LOG_ON option, and build with ENABLE_UNIT_TESTS=TRUE. CFE EVS unit test fails to build with a compiler error.

System observed on:
Ubuntu 18.04 LTS 64 bit.

Additional context
Unless there is a specific requirement for CFE_PLATFORM_EVS_LOG_ON as it stands today, my recommendation would be to deprecate this option and keep it always on, which reduces the testing matrix, and makes the FSW code more consistent. Platform config options that actually add/remove #ifdef code should be avoided, as this has proven to be a testing/support issue time and time again.

In this case, only the code that initializes the structures is compiled out. All the code that reads/writes to it is still compiled in, but skipped via a runtime test. So this isn't saving much in the way of code/text space.

If the goal of this option is to save data space memory, then mostly the same effect can be achieved by keeping the log very small, by setting CFE_PLATFORM_EVS_LOG_MAX to a very low number, such as 1. In this mode the log structure uses only 176 bytes of memory on an x86-64 machine, down from 3368 bytes with the default size of 20.

And the unit tests still build and pass with the max set to 1, and it reduces the amount of conditionally-compiled code and variances on the FSW side.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

For CCB discussion - can we remove/deprecate the CFE_PLATFORM_EVS_LOG_ON option, consider it always enabled, and just recommend the user set CFE_PLATFORM_EVS_LOG_MAX to a very low number if memory is of concern.

[CDKnightNASA]

For CCB discussion - can we remove/deprecate the CFE_PLATFORM_EVS_LOG_ON option, consider it always enabled, and just recommend the user set CFE_PLATFORM_EVS_LOG_MAX to a very low number if memory is of concern.

Not having dug into the code, what would be the behavior if LOG_MAX is defined to 0?

[jphickey]

That creates a different compile time failure, because it is used to size an array. The lower limit is 1.

We could conditionally compile based on #if CFE_PLATFORM_EVS_LOG_MAX > 0 instead, but then we'd still have the testing matrix issues and a config option that isn't validated by any current testing. I don't see it as worth the complexity to save 176 bytes.

Interestingly, due to the way the "reset area" is allocated, unless you also manually reduced this size when turning off the EVS log feature, the full size is allocated anyway. See #610.

[jphickey]

Another possible option would be to keep the option but only use it to initialize the runtime boolean. (CFE_EVS_GlobalData.EVS_TlmPkt.Payload.LogEnabled), and get rid of all other #ifdef blocks.

This would be a lower impact change, but it solves the issue of having these fields present in the global but left uninitialized, which IMO is risky. They should always be initialized.

[CDKnightNASA]

Lame hack: MAX = 0, array[MAX+1]

[skliper]

@acudmore - CCB discussed, agreed removing this option is low impact and not a requirement to be optional

[skliper]

Should we just CFE_PSP_Panic (similar to ES) for reset area check, limp along if OS_MutSemCreate fails, and remove use of CFE_EVS_GlobalData.EVS_TlmPkt.Payload.LogEnabled for control? Just set to true in this implementation, if/when EVS becomes a module other implementations could be done that don't log. Would allow for removal of some events, branches.

Or could add a CFE_EVS_GlobalData.LogEnabled used for control and to set the HK info. That could keep the same behavior, just avoid use of HK payload data for control.