search

nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
408 stars 200 forks source link

Unit Test Stub Function UT_GetActualCmdCodeField Not Restored with CCSDS Secondary Header (GSFC DCR 23036) #90

Closed skliper closed 4 years ago

skliper commented 4 years ago

A change was made to the format of the cFE CCSDS command secondary header to enforce the header to be in big endian in cFE 6.4.1. The cFE unit test stub function UT_GetActualCmdCodeField was updated accordingly.

The change to the cFE CCSDS command secondary header was restored to the original definition of the header in cFE 6.4.1. The cFE unit test stub function UT_GetActualCmdCodeField however, was not restored. The original code for this function is attached.

skliper commented 4 years ago

Imported from trac issue 59. Created by sstrege on 2015-05-27T16:51:49, last modified: 2016-08-11T15:56:14

skliper commented 4 years ago

Trac comment by glimes on 2015-06-02 13:15:13:

I will drop the new function into place and make a branch. ... TODO: update the tarball in #81

CCB has further discussion about endianness and alignment issues raised by this code snip. Joe has an idea, and I will run from there.

skliper commented 4 years ago

Trac comment by glimes on 2015-06-03 11:47:59:

Checked the version in the "development" branch to see if it needed updating, and it does: the current version there uses a single byte access (which is good) but is hard wired to be correct for little-endian. Compiling for big-endian requries editing the file and changing which of two lines is commented out.

Per Joe:

cFE 6.4.0 attempted to make both primary + secondary as big-endian cFE 6.4.1 backtracked this for the secondary header ONLY, so the secondary header is now "native-endian" again as it was pre-6.4.0.

The normal way to get the Command Function code is to pass the secondary header to the CCSDS_RD_FC() function, which allows ccsds.h to control the implementation.

This unit test function is used by the CFE SB unit test to verify that the "correct" function code is being extracted from a packet by the normal API. So calling the normal API here would "verify" very little code.

Without using the ccsds.h provided structures and macros, the correct way to fetch the data is to follow the definition of the field. That is, fetch the 16-bit unsigned value from the two bytes at the correct offset, then extract the appropriate bitfield.

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-03 11:55:21:

Regarding the "best" option for 6.4.2 ---

My current vote would be the "shift and mask" option you proposed, i.e.: {{{ CFE_SB_CmdHdr_t CmdPtr =(CFE_SB_CmdHdr_t )MsgPtr;

/*
 * This code assumes that the command code is seven bits
 * starting eight bits up in the Command member. This would
 * detect bugs in the CCSDS access macros (presuming this code
 * is more likely to be correct than ccsds.h ... not likely).
 */
return (MsgPtr->Sec.Command >> 8) & 0x7F;

}}}

Note that the {{{UT_GetActualPktLenField}}} function (right above this one in the source file) accesses the Primary header using the structure defined in SB with a shift.

That's the only reason I proposed this, just to be the same. Otherwise, any of the three implementations is fine by me.

The only value to NOT using structures/macros defined in ccsds.h is if they are wrong and don't line up with the CCSDS definition, the UT will not catch it because it will always match if it uses the same accessor macro. Therefore there is some value to having a separate impl in UT that only uses raw words.

But this is minor/unlikely, and the other UT code is already using the structure accessors, so I say just do the same here.

skliper commented 4 years ago

Trac comment by glimes on 2015-06-03 14:07:15:

The commit based on master does not merge neatly into development, as the change is a forklift-replacement of the function, which differs in the two trees; the merge picks up an adjacent change and leaves behind a "land mine" where any subsequent merging between master and developent could end up reverting that change.

I have applied the change separately to the master and development branches as baseline, then merged the one from master into the hotfix.

Current set of Git commits (subject to change if testing does not go well):

[changeset:50a30b1] trac-59-unit-test-cmd-code-access contains the original change, based from the master branch.

[changeset:b078242] ic-642-trac-59 merges this change into the current ic-hotfix-6-4-2 branch for testing. Note that Bamboo will attempt to build this branch, and will fail because Bamboo uses the development OSAL and this requires master OSAL.

[changeset:0394145] ic-trac-59 applies the same fix (replacing the function) to the development branch. Bamboo has built this branch in combination with the development versions from the related projects. Bamboo status is GREEN, and there are no unit test in quarantine. Detailed compiler "stderr" logs show the unfortunate but not unexpected collection of warnings. ''Ticket #67 updated to remind me to turn compiler warnings into "test errors" for display in Bamboo, as is done in OSAL.''

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-03 16:11:04:

In this situation I would propose fast-forwarding the "ic-hotfix-6-4-2" branch to include this additional merge.

Either that, or I can delete the "ic-hotfix-6-4-2" branch, and the "ic-642-trac-59" can replace it.

Just don't want to have any confusion as to what branch reflects the proposed 6.4.2 release.

Agreed?

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-03 16:40:28:

Reviewing commit [changeset:50a30b1] -- it appears that the {{{UT_GetActualPktLenField}}} (the function immediately prior to the function in question) has been removed. Is this intended?

skliper commented 4 years ago

Trac comment by glimes on 2015-06-04 10:33:52:

Removing the LEN access function was not intended; probably dropped while I was dancing around, trying to get a single commit that could merge both ways.

Will have a new set of commits shortly.

skliper commented 4 years ago

Trac comment by glimes on 2015-06-04 11:04:53:

Revised set of Git commits:

[changeset:6767792] trac-59-unit-test-cmd-code-access contains the original change, based from the master branch.

[changeset:678076f] ic-642-trac-59 merges this change into the current ic-hotfix-6-4-2 branch for testing.

[changeset:0394145] ic-trac-59 applies the same fix (replacing the function) to the development branch.

Trying to best capture the "check the real bits" intent of the function, and to make it insensitive to further changes to the ccsds.h structure fields, I went with this:

{{{ /* * Return the actual command code / uint8 UT_GetActualCmdCodeField(CFE_SB_MsgPtr_t MsgPtr) { /*

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-04 11:23:40:

Looks good based on commit [changeset:6767792]. Removing dependencies on {{{ccsds.h}}} and going with the raw word access does have benefit here.

Also checked out the two merge branches and these also look good.

I recommend accepting this into development and master.

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-04 11:44:36:

As I assume these will be included in the release I have taken the liberty of updating the "ic" branches accordingly: {{{ic-2015-05-20}}} and {{{ic-hotfix-6-4-2}}} have been updated to include this.

Further tracking/testing of the 6.4.2 release should be done in ticket #81.

skliper commented 4 years ago

Trac comment by sstrege on 2015-06-09 16:54:44:

Looks good to me. Has the the tarball in #81 been updated with this change?

skliper commented 4 years ago

Trac comment by glimes on 2015-06-10 11:28:31:

I neglected to explicitly record all of the Commit ID numbers!

Commit [changeset:6767792] trac-59-unit-test-cmd-code-access has the final version of this fix for the master branch.

Commit [changeset:0394145] ic-trac-59 has the final version of this fix for the development branch.

Two separate commits were required to assure that later merges did not do the wrong thing with an adjacent unrelated change.

Commit [changeset:678076f] ic-hotfix-6-4-2 merges this fix into the CFE 6.4.2 Hotfix.

Commit [changeset:fd9a114] ic-2015-05-20 merges this fix into the development baseline.

... and yes, this is included in the tarball attached to #81.

skliper commented 4 years ago

Trac comment by sstrege on 2016-08-11 15:56:01:

Merged to development on Wed Jun 3 08:41:47 2015 -0700