Integration Candidate: 2020-09-02

[yammajamma]

Describe the contribution
Fix #815
Fix #823
Fix #851

Testing performed
Bundle CI - https://github.com/nasa/cFS/pull/136/checks

Expected behavior changes
PR #817 - In the next major CFE release, this code will be no longer supported at all. It should be removed early in the cycle
to avoid needing to maintain this compatibility code.

PR #857 - The CFE_ES_FindCDSInRegistry function had an unusual loop control structure with mixed types of signed and unsigned. This has the possibility of being infinite if the MaxNumRegEntries is zero due to the way the end condition is structured. Simplify to be like other loops and use unsigned int control variable.

PR #852 - Fixes the cast-align error (use the aligned Msg since it's available already).

HOTFIX-20200902 - Fix sb unit test setup issue.

HOTFIX 20200902 - Update documentation links for deprecated symbols.

HOTFIX 20200902 - Fix SB Test_CleanupApp_API AppID.

System(s) tested on
Ubuntu - CI

Additional context
nasa/cFS#136

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman, NASA-GSFC
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

FYI - @skliper and @yammajamma I have a fix for the documentation symbol name changes that gets rid of the doxygen warnings. Should I push as hotfix directly to integration-candidate?

[yammajamma]

@jphickey that should work

[jphickey]

See commit d47a869 for the fixes. Most changing old names to new (non-deprecated) names within documentation, but there were some items e.g. shell related event ID that the entire feature was deprecated.

[astrogeco]

@skliper and @jphickey we still have a unit test failure, see https://github.com/nasa/cFS/pull/136/checks:

8/76 Test #68: cfe-core_sb_UT ....................***Failed    0.02 sec

[skliper]

Started digging in, but seems like a behavior change... still need to nail down the difference.

[jphickey]

This is the same as it was last week ... in the Test_CleanupApp_API() test we are getting 4 events instead of 3.

The previous hotfix in commit 3baef94 changed it so it is now a "real" failure rather than a test teardown (TTF) failure which makes the count actually show up in the summary. However the failure is due to the event count difference and it is still a failure....

[skliper]

This is the same as it was last week...

Signature is the same but cause is different.

The previous problem is there were four events counted because it wasn't clearing the number of events between running the last subtest of Test_RcvMsg_API and running Test_CleanupApp_API. Now there really are 4 events being sent as part of Test_CleanupApp_API. Behavior change.

[jphickey]

I am taking a closer look ... I do see that that CFE_SB_DeletePipeWithAppId is being invoked several times, looks like a possible case of stale data in the table from a previous test case making the entry look "valid" ... so it is trying to delete but ultimately triggering event 46 (CFE_SB_DEL_PIPE_ERR1_EID)

[jphickey]

FWIW, this is happening on the first pass (line 3563 - which is commented to be the "bad" application ID - and isn't supposed to actually do anything AFAICT).

This is sort of confirming that there are remnants in the table which appear valid....

[jphickey]

I think I found it ... AppID "0" was both the valid and invalid AppID here in this test ...

Adding a call to UT_SetAppID(1); prior to this test case seems to fix it and turns the first call back into a no-op (which agrees with the comments).

In a near term change the AppID "0" will become always invalid so this is really just a patch to get things working for this IC.

[skliper]

I did check the code at 8b9a3db at the point where my hotfix was merged and it did reject the first call and delete on the 2nd... seems like there was some other logic change in the other commits that affected behavior? Not saying the current logic isn't correct, just surprised to see a change in behavior of a unit test.

EDIT - actually this was in a separate branch... oh well.

[jphickey]

Possibly related to the internal table management changes in #797 / #859 ....
This updated the code to use the output of CFE_ES_GetAppID() as the "real" AppID but this is returning 0 at this stage. The code seems to be expecting AppID 1.

[skliper]

I just found:

cFE/fsw/cfe-core/ut-stubs/ut_es_stubs.c

Line 66 in 4d3c809

#define CFE_UT_ES_DEFAULT_APPID ((uint32)1)

and this put my mind at ease.

[jphickey]

Right ... it is the UT_AppID here which overrides that:

cFE/fsw/cfe-core/unit-test/ut_support.c

Line 173 in 08f6eab

UT_SetDataBuffer(UT_KEY(CFE_ES_GetAppID), (uint8*)&UT_AppID, sizeof(UT_AppID), false);

Once I submit PRs for the rest of the AppID cleanup stuff I think this can actually go away for good.

[skliper]

I suspect this should be != AppID to get branch coverage. You need an entry in use but != AppID:

cFE/fsw/cfe-core/unit-test/sb_UT.c

Line 3557 in 68f871a

CFE_SB.PipeTbl[1].AppId = AppID;

EDIT - actually it looks like you won't get the error case anymore without doing this. Looks like you'd get two successful deletions (maybe, just inspection).

[jphickey]

Just as a sanity check I locally added another assert:

EVTSENT(CFE_SB_DEL_PIPE_ERR1_EID);

And this is also passing -- so it appears that we are getting the error case. Probably worth adding this to ensure we cover that event path (but that doesn't need to be a hotfix).

[astrogeco]

@yammajamma can you update the "Expected behavior changes" above with a summary of the hotfixes?