Fix #274, NumOfChildTasks not decremented #485

dmknutsen · 2020-01-21T22:57:55Z

Describe the contribution
Fixes #274, NumOfChildTasks not decremented

Testing performed
Steps taken to test the contribution:

Nominal build process.
Ran unit tests.

Expected behavior changes
NumOfChildTasks will now be decremented when a child task is deleted.

System(s) tested on
Oracle VM VirtualBox
OS: ubuntu-19.10
Version: cFE 6.7.3.0; OSAL 5.0.3.0; PSP 1.4.1.0

Contributor Info - All information REQUIRED for consideration of pull request
Dan Knutsen
GSFC/NASA

jphickey

I'm not sure this is sufficient to cover the issue. There are a few things here:

Only covers child tasks that explicitly call CFE_ES_DeleteChildTask(). Does not cover tasks that exit themselves using e.g. CFE_ES_ExitChildTask().
It also assumes and does not confirm that CFE_ES_DeleteChildTask() is called by the another task within the same app. But the API description does not say this is a requirement - It just says it deletes a task specified by the task ID. It would be wrong to assume this.

I suggest an entirely different approach here. The issue is that it might not be possible to catch ALL cases of a task exiting so it is very challenging to keep a counter accurate here.

The only place that NumOfChildTasks is actually used is part of the CFE_ES_GetAppInfo() call which is generally only invoked from the QueryOne/QueryAll ground commands. It would be fairly easy to remove the counter from the global table entirely, which thereby solves the problem of keeping it up to date, and simply scan the app table as part of the CFE_ES_GetAppInfoInternal() implementation itself; just loop through the app table and count the number of entries that have their MainTaskId equal to the AppId being queried. Of course this is slower but this is an infrequent call so I would think the complexity avoided by computing the value on-demand is better than optimizing a rarely-called function.

jphickey · 2020-01-22T15:52:20Z

fsw/cfe-core/src/es/cfe_es_api.c

+                if (Result != CFE_SUCCESS)
+                {
+                   CFE_ES_SysLogWrite_Unsync("CFE_ES_DeleteChildTask: Error calling CFE_ES_GetAppIDInternal for Task '%d'. RC = 0x%08X\n",(int)TaskId,(unsigned int)Result);
+                   ReturnCode = CFE_ES_ERR_CHILD_TASK_DELETE;


Should NOT return an error here. So long as the OS_TaskDelete returned success, that means the task is actually deleted. This is just an error with record-keeping. Fine to write to the syslog to document the inconsistency event, but the return code to the caller should still indicate the task was successfully deleted.

jphickey · 2020-01-22T15:57:08Z

fsw/cfe-core/src/es/cfe_es_api.c

@@ -1224,16 +1225,34 @@ int32 CFE_ES_DeleteChildTask(uint32 OSTaskId)
             if ( OSReturnCode == OS_SUCCESS )
             {
                /*
+                ** Get the AppID of the calling Application
+                */
+                Result = CFE_ES_GetAppIDInternal(&AppId);


Note that CFE_ES_GetAppIDInternal() returns the AppID of the calling context. So this assumes that this function will only ever invoked by the same App that created the child task in the first place. But this restriction is not documented in the API description of the CFE_ES_DeleteChildTask() function.

If it is the intent to restrict this to the same app that owns the child task, then this should also be verified prior to doing the delete (i.e. make it an error to delete a child task associated with a different app) and note that restriction in the API documentation.

skliper · 2020-01-22T18:16:46Z

CCB 20200122 - Discussed, preference is to calculate on request and remove the counter.

jphickey

Changes to the FSW code look OK to me, but recommending a few changes to the UT test case.

Also as part of this I noticed duplicate storage of the tasks names which I noted in #502

jphickey · 2020-01-30T14:58:46Z

fsw/cfe-core/unit-test/es_UT.c

+    CFE_ES_Global.TaskTable[18].RecordUsed = true;
+    CFE_ES_Global.TaskTable[18].AppId = 15;
+    CFE_ES_Global.AppTable[15].AppState = CFE_ES_AppState_RUNNING;
+    CFE_ES_Global.AppTable[15].TaskInfo.MainTaskId = 25;


Test cases that write to specific fixed entries in the table should focus on the lower numbered entries (ideally just 0-4 if possible).

The size of the actual TaskTable is dependent on the OS_MAX_TASKS setting, and generally needs to be at least 5 to run CFE core tasks in the first place, but no guarantee it of anything beyond that.

jphickey · 2020-01-30T15:02:47Z

fsw/cfe-core/unit-test/es_UT.c

+
+    CFE_ES_GetAppInfo(&AppInfo,CFE_ES_Global.TaskTable[1].AppId);    
+    uint32 NumORegTasksTemp = CFE_ES_Global.RegisteredTasks;
+    uint32 NumOChildTastTemp1 = AppInfo.NumOfChildTasks;


Wouldn't the "NumOfChildTasks" here be a predictable/consistent number based on the previous TaskTable config? That is, I think we should know exactly how many child task entries are associated with the AppId because it was just set up earlier in the test sequence. If we do have a specific number which is the correct answer based on an explicitly set config, then we should assert for that specific number.

Also as a general style thing I think we try to avoid instantiating local variables in the middle of the function.

jphickey

Just a correction to the previous review, mistakenly submitted as "approve" when it should have been "request changes". Just the UT bits noted in the previous review.

jphickey

Still not quite... see comment.

fsw/cfe-core/unit-test/es_UT.c

jphickey

Real close now....

fsw/cfe-core/unit-test/es_UT.c

jphickey

Looks good now!!

No longer keeps a running count of child tasks

skliper · 2020-02-03T16:55:08Z

Reset soft to master and checked in again.

ghost

I like the approach of calculating this on the fly when needed.

astrogeco · 2020-02-12T17:21:45Z

CCB 20200212 - Approved

skliper approved these changes Jan 22, 2020

View reviewed changes

jphickey requested changes Jan 22, 2020

View reviewed changes

dmknutsen force-pushed the issue_274 branch from 8ed0dca to 7fff455 Compare January 29, 2020 21:14

skliper requested review from jphickey and skliper January 30, 2020 14:49

jphickey approved these changes Jan 30, 2020

View reviewed changes

jphickey requested changes Jan 30, 2020

View reviewed changes

dmknutsen requested a review from jphickey January 30, 2020 18:47

jphickey requested changes Jan 30, 2020

View reviewed changes

fsw/cfe-core/unit-test/es_UT.c Outdated Show resolved Hide resolved

skliper changed the title ~~Fixes #274, NumOfChildTasks not decremented~~ Fix #274, NumOfChildTasks not decremented Jan 30, 2020

dmknutsen force-pushed the issue_274 branch from f04243d to b8ff70f Compare January 30, 2020 21:44

dmknutsen requested a review from jphickey January 30, 2020 21:47

jphickey requested changes Jan 30, 2020

View reviewed changes

fsw/cfe-core/unit-test/es_UT.c Outdated Show resolved Hide resolved

dmknutsen requested a review from jphickey January 31, 2020 16:13

jphickey approved these changes Jan 31, 2020

View reviewed changes

skliper approved these changes Feb 3, 2020

View reviewed changes

dmknutsen force-pushed the issue_274 branch from 1af8e70 to 936fb45 Compare February 3, 2020 16:15

Fix nasa#274, Calculate child tasks when reported

8558411

No longer keeps a running count of child tasks

skliper force-pushed the issue_274 branch from 936fb45 to 8558411 Compare February 3, 2020 16:53

astrogeco added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 11, 2020

ghost approved these changes Feb 11, 2020

View reviewed changes

astrogeco added the CCB - 20200213 label Feb 11, 2020

astrogeco added this to the 6.8.0 milestone Feb 12, 2020

astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Feb 13, 2020

astrogeco added ic 20200226 and removed ic 20200226 labels Feb 25, 2020

astrogeco added the IC - 20200226 label Feb 25, 2020

astrogeco changed the base branch from master to ic-20200226 February 25, 2020 19:24

astrogeco merged commit dccf3b2 into nasa:ic-20200226 Feb 25, 2020

astrogeco mentioned this pull request Feb 26, 2020

IC - 20200226 #530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #274, NumOfChildTasks not decremented #485

Fix #274, NumOfChildTasks not decremented #485

dmknutsen commented Jan 21, 2020

jphickey left a comment

jphickey Jan 22, 2020

jphickey Jan 22, 2020

skliper commented Jan 22, 2020

jphickey left a comment

jphickey Jan 30, 2020

jphickey Jan 30, 2020

jphickey left a comment

jphickey left a comment

jphickey left a comment

jphickey left a comment

skliper commented Feb 3, 2020

ghost left a comment

astrogeco commented Feb 12, 2020

Fix #274, NumOfChildTasks not decremented #485

Fix #274, NumOfChildTasks not decremented #485

Conversation

dmknutsen commented Jan 21, 2020

jphickey left a comment

Choose a reason for hiding this comment

jphickey Jan 22, 2020

Choose a reason for hiding this comment

jphickey Jan 22, 2020

Choose a reason for hiding this comment

skliper commented Jan 22, 2020

jphickey left a comment

Choose a reason for hiding this comment

jphickey Jan 30, 2020

Choose a reason for hiding this comment

jphickey Jan 30, 2020

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

skliper commented Feb 3, 2020

ghost left a comment

Choose a reason for hiding this comment

astrogeco commented Feb 12, 2020