CFE RELOAD and RESTART commands handled (almost) identically

[jphickey]

Describe the bug
The documentation of the CFE_ES_RESTART_APP_CC specifically says here that it is not reloaded from the disk file:

cFE/fsw/cfe-core/src/inc/cfe_es_msg.h

Line 324 in 5ac3f0a

** This command does \b NOT reload the application from the onboard

But in the way the code has evolved, both RELOAD and RESTART control requests end up going through CFE_ES_CleanUpApp() function:

cFE/fsw/cfe-core/src/es/cfe_es_apps.c

Lines 961 to 968 in 5ac3f0a

	case CFE_ES_RunStatus_SYS_RESTART:
	/*
	** Kill the app
	*/
	Status = CFE_ES_CleanUpApp(AppID);
	if ( Status == CFE_SUCCESS )
	{

cFE/fsw/cfe-core/src/es/cfe_es_apps.c

Lines 997 to 1003 in 5ac3f0a

	case CFE_ES_RunStatus_SYS_RELOAD:
	/*
	** Kill the app
	*/
	Status = CFE_ES_CleanUpApp(AppID);
	if ( Status == CFE_SUCCESS )

Notably, the CFE_ES_CleanUpApp() function will, in fact, unload the module via OS_ModuleUnload(), and the subsequent CFE_ES_AppCreate() function will load it again from disk. Also important that the AppID might change too as part of this process, which may or may not be expected?

Expected behavior
Should make the documentation and code match one way or another:

• If we want a true "restart" without reload, as the documentation for CFE_ES_RESTART_APP_CC says, we need to update this to NOT completely unload the module.
• OR If the current implementation is OK then I'd say it isn't sufficiently different from CFE_ES_RELOAD_APP_CC to warrant the existence of a separate command.

Additional context
I noticed this inconsistency while doing implementation of #28. I can put in a fix for this issues as part of the same (upcoming) PR, just need CCB concurrence on which way to go - do we make it work as described, or we describe the way it works.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Checking the requirements -- cES1007 doesn't say one way or the other:

Upon receipt of a Command, the cFE shall Restart the Command specified Application.

But functional requirement cES1007.2 says:

If the original cFE Application file is not found then the cFE shall reject the Command, increment the invalid Command counter, and generate an event message.

Which seems weird, if the intent is that the restart command doesn't reload the file.

The code also fails this requirement, as the "restart" command doesn't check for the existence of the file, only the "reload" command does this, even though they currently both unload/reload the code module from disk.

[jphickey]

ping @skliper , @acudmore , @jwilmot --- any thoughts on this "restart" behavior?

Does the internal JIRA have more details on cES1007 than the CSV file does? Does it or any other GSFC requirements better define the "restart" as including an unload + load or NOT doing an unload + reload?

[skliper]

No futher details exist in the internal requirements management system. But I read the difference as being RELOAD is intended to be a different file name (app name and file name parameters in command), where-as RESTART uses the same information as when originally started (only takes app name).

cES1007 - RESTART rationale:

Need to be able to restart an Application. A restart involves deleting it (cleaning up) and then starting it again. This is similar to starting the cFE Application from a file system. When an Application is restarted, the only command parameter required is the application name. All other parameters including the filename are the same as the original cFE Application Create command. The restart is intended for error recovery such as an exception, and should not be used to start a new version of an Application. If a Critical Data Store Area is allocated for the Application, it is preserved, and the Application may re-connect to the Critical Data Store Area when it is running again.

cES1008 - RELOAD ratonale:

This command enables the ground to replace an Application with only one command. This is required for applications such as a Command Uplink Application, which must be replaced with one command. The specified cFE Application file may be from any valid cFE.

For cES1007.2 - we recently updated the rationale and requirement as part of #424 to reflect what the code does:

Can't restart the Application if the original file has been removed.  The command is aborted during the attempt to load, after the application has been deleted.

Edit - just updated the rationale that used to say the app would continue without a restart

[skliper]

Looks to me like the header file documentation is wrong.

[jphickey]

OK - changing the doxygen comments to reflect what the code does is certainly easier than updating the code to do what the comment says.

The rationale seems lacking because, at least as written, RELOAD can cover the RESTART use-case as well as its own. Having a whole separate command to do the same thing (just minus the file name) doesn't seem necessary. But since it already exists I suppose we can leave it.

[skliper]

...doesn't seem necessary.

Agreed, reload with empty filename could use original or similar. Maybe a quick refactor under the hood to avoid code duplication? If it's easy. Processing really should be the same code I'd think.

[jphickey]

It is already consolidated where possible. Although it is nearly identical -- there are a dedicated/different set of event IDs -- same meaning, different numbers, and the text says "restart" or "reload" within the message too. So there is dedicated code to handle the command, and dedicated code to send completion events, but everything in the middle uses common functions.

Again, although the event meanings are also identical, they have different IDs and different text.

So to further clean it up/consolidate it would be matter of removing "restart" entirely but I'm not necessarily advocating for that right now as it impacts requirements and ground system etc. It is definitely easier to just update the doxygen comments and leave the two commands in place.