Use of CFE_EVS_SendEventWithAppID in APIs doesn't follow preferred model

[skliper]

Is your feature request related to a problem? Please describe.
CFE_EVS_SendEventWitAppID is used in many of the cFE core APIs. This leads to possible EID collisions (core to app), cross dependencies for filter management, and doesn't really follow preferred pattern.

Describe the solution you'd like
Apps should manage their events, APIs should return status for apps to handle.

Describe alternatives you've considered
Manage EIDs to prevent collisions, still not great since they are compiled in so fairly fragile

Additional context
Code review

Requester Info
Jacob Hageman - NASA/GSFC

[jphickey]

Do we need to restrict this or just document it better?

I think the use-case is justified, It is used when a function in one app is called by another app (of which there are many examples, such as CFE_ES_RegisterCDS), and the function needs to generate one of the ES-defined event IDs (e.g. CFE_ES_CDS_REGISTER_ERR_EID). But the calling context is that of the app that called the function, not ES, so it uses this function to (properly) associate the event ID with the ES app, not the app ID that called the function, which wouldn't be correct.

The same problem can easily occur in user-defined apps too. Even if the functions are put into a library, if one wants to send an event, one has to associate that event with an appID.

I do consider this to be a "specialized" API - but I think its a valid tool to have in the tool box. It has to be used properly, but it's not wrong to have it.

[jwilmot]

Joe,
Thanks. That is the point of the API. It should be retained.

[skliper]

I thought we discouraged sending of events from libraries/APIs, in favor of providing return status that the app could then handle within it's context. From what I see there's nothing deconflicting them when sent from APIs, I thought we had an open issue to that effect but couldn't find it. Just as a random example, not actually looking if these are sent by APIs but:

grep -r EID * | grep " 6$"
apps/to_lab/fsw/src/to_lab_events.h:#define TO_TLMOUTSOCKET_ERR_EID  6
apps/sample_app/fsw/src/sample_app_events.h:#define SAMPLE_APP_LEN_ERR_EID           6
apps/ci_lab/fsw/src/ci_lab_events.h:#define CI_LAB_COMMANDRST_INF_EID   6
cfe/docs/cFE Application Developers Guide.md:#define SAMPLE_LEN_ERR_EID     6
cfe/modules/core_private/fsw/inc/cfe_core_resourceid_basevalues.h:    CFE_RESOURCEID_SB_PIPEID_RESOURCE_BASE_OFFSET = OS_OBJECT_TYPE_USER + 6
cfe/modules/sb/fsw/inc/cfe_sb_events.h:#define CFE_SB_SUB_ARG_ERR_EID 6
cfe/modules/time/fsw/inc/cfe_time_events.h:#define CFE_TIME_DIAG_EID 6
cfe/modules/evs/fsw/inc/cfe_evs_events.h:#define CFE_EVS_ERR_EVTIDNOREGS_EID 6
cfe/modules/es/fsw/inc/cfe_es_events.h:#define CFE_ES_START_INF_EID 6

[jwilmot]

ES knows what the issues are and defines the event/error codes in context of ES. This approach decouples the ES implementation from the calling app and potentially reduces code duplication. It is a judgement call on when to use events in an API so your questioning is valid. For this case I think it's better that ES send the event since ES detected and defined the error condition being reported.

[skliper]

Well, if we do want API's to send events (which would also keep the message contents consistent), then it seems like we need to deconflict EIDs since it's the only way to tell them apart on a short message (if type is the same). Maybe assign/reserve bits to uniquely identify API events, either in the type or event? If it was type we might be able to filter in useful ways?

[jphickey]

It is already de-conflicted (sort of) because the combination of (appID + eventID) is what is unique. My only concern is that the "appID" here is not a fixed/consistent ID, so you might boot once and get one appID, then reboot and get a different appID. But either way, event IDs need to be combined with app IDs, then they are unique.

This is the exact reason why, depending on the use case of the function, it needs to ensure that the event ID is indeed associated with the correct app ID.

I also concur that it is often more appropriate for the implementer (e.g. ES) to handle the events in many cases, for instance going back to the CDS example, the ES provides the implementation, the ES knows exactly what went wrong and has full context info to assemble the message. It is impossible to encode all the detail into a simple return code. (i.e. events include the CDS name, caller name, plus any other detail).

The pattern should be that the event gets generated by the spot that has the best context info to describe the problem....

That being said, I do think we might want to consider notion of app IDs assigned at compile-time, which can provide a consistent identifier that doesn't change after every reboot/reload, to supplement the runtime app ID. Maybe the app name itself could serve this but it is too long to put into binary structures many times.

[skliper]

It is already de-conflicted...

How so? If the SB API defines an EID of 6, how can you tell that apart from an app EID of 6, or an ES API EID of 6 (or any other random library that is used that defines EIDs used in APIs)? Where is the appID in a SHORT message? How would you filter any of these differently, since the filter is based on EID? Wouldn't the filter logic collide (if you set to filter all but the first, only the first EID of 6 would get through)? All you get in a short message is the CFE_EVS_PacketID structure:

cFE/modules/evs/fsw/inc/cfe_evs_msg.h

Lines 1235 to 1248 in e80aae9

	typedef struct CFE_EVS_PacketID
	{
	char AppName[CFE_MISSION_MAX_API_LEN]; /**< \cfetlmmnemonic \EVS_APPNAME
	\brief Application name */
	uint16 EventID; /**< \cfetlmmnemonic \EVS_EVENTID
	\brief Numerical event identifier */
	uint16 EventType; /**< \cfetlmmnemonic \EVS_EVENTTYPE
	\brief Numerical event type identifier */
	uint32 SpacecraftID; /**< \cfetlmmnemonic \EVS_SCID
	\brief Spacecraft identifier */
	uint32 ProcessorID; /**< \cfetlmmnemonic \EVS_PROCESSORID
	\brief Numerical processor identifier */
	} CFE_EVS_PacketID_t;

[skliper]

ping @tngo67, @klystron78 for participation/awareness

[jphickey]

In the short message the App Name is serving to de-conflict. One must look at the app name in order to decipher event ID.

(FWIW, I thought it was sending app ID, this is actually better - app name will at least be consistent from run to run, so this is not bad, it is just verbose/large in size)

[skliper]

I'm still not following. How do you tell the difference between an event created by sample app with an EID defined within sample app with a value of 6 vs an event created by sample app calling an API that uses an EID it defines as a value of 6? What am I missing? We don't do anything I'm aware of to deconflict EIDs between apps/libs/modules. I am aware there are external systems that do deconflict across a distribution, but cFE doesn't make this easy since the EIDs are defined internally (modules/es/fsw/inc/cfe_es_events.h for example).

[skliper]

From splinter - I definitely had it confused, I assumed "AppName" was the calling context, where instead it's the EID namespace context. So really what happens is if sample_app calls an ES API that generates an EID of 6, the "AppName" that is sent down in telemetry is ES (NOT sample_app). Therefore for short messages, it's currently impossible to tell the calling context from a short event for events created by APIs (you wouldn't know if sample_app or some other app caused it), nor can you filter based on calling context since the filter is within the context of ES.

[skliper]

Remaining action/options - Could improve documentation and/or change AppName to better reflect that it is the EID context not the calling context. Could enhance reporting to include calling context. Could add filtering options to additionally filter based on context. Also still pending feedback from reviewers who recommended removing this... seems like it's still the recommended pattern and a required capability.

[astrogeco]

CCB:2021-04-21 DISCUSSED

• It is somewhat of a misnomer but it is in the EID context
• Looking for comments from @tngo67 and @klystron78
• Can't filter events based on an App and silence the rest.
• Probably need a new feature to add filtering options
• Event is owned by the library so the app can't interact with the system
• For Caelum: Change the app name to eid context make the context obvious
• Looking for suggestions on renaming PktID See Overloaded "PacketID" use in CFE_EVS_PacketID #1397

EDIT (skliper): Just going to leave as-is for now, future work to clean up/improve.

[skliper]

Marking as invalid and closing this issue since there is a use case, future work to improve naming/reporting/filtering.