Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests fail to build when MESSAGE_FORMAT_IS_CCSDS_VER_2 is enabled #618

Closed
johnphamngc opened this issue Apr 15, 2020 · 2 comments · Fixed by #674 or #692
Closed

Unit tests fail to build when MESSAGE_FORMAT_IS_CCSDS_VER_2 is enabled #618

johnphamngc opened this issue Apr 15, 2020 · 2 comments · Fixed by #674 or #692
Assignees
Milestone

Comments

@johnphamngc
Copy link

johnphamngc commented Apr 15, 2020

Describe the bug
Unit tests fail to build when MESSAGE_FORMAT_IS_CCSDS_VER_2 is enabled

To Reproduce
Enable MESSAGE_FORMAT_IS_CCSDS_VER_2 in *mission_cfg.h
Run make CFLAGS="-m32 -Wno-format" SIMULATION=native ENABLE_UNIT_TESTS=true

Expected behavior
Compilation succeeds, however build actually fails due to missing #include

Code snips
This can be fixed by doing the following

--- a/fsw/cfe-core/ut-stubs/ut_sb_stubs.c
+++ b/fsw/cfe-core/ut-stubs/ut_sb_stubs.c
@@ -34,6 +34,8 @@
 */
 #include <string.h>
 #include "cfe.h"
+#include "cfe_platform_cfg.h"
+#include "../sb/cfe_sb_msg_id_util.h"
 #include "utstubs.h"

However, subsequently a checksum error is encountered when running the tests.
This was worked around by doing the following, but unsure if the workaround is correct.

--- a/fsw/cfe-core/unit-test/sb_UT.c
+++ b/fsw/cfe-core/unit-test/sb_UT.c
@@ -9456,7 +9456,7 @@ void Test_CFE_SB_ChecksumUtils(void)
 #ifndef MESSAGE_FORMAT_IS_CCSDS_VER_2
     ExpRtnFrmGet = 0x2f;
 #else
-    ExpRtnFrmGet = 0x65;
+    ExpRtnFrmGet = 0x61;
 #endif

     /* Validation expected to return true */

System observed on:

  • Hardware: N/A
  • OS: RHEL7
  • Versions cFE 6.7.12

Reporter Info
John N Pham, Northrop Grumman

@skliper skliper added the bug label Apr 15, 2020
@johnphamngc
Copy link
Author

It looks like Test_CFE_SB_ChecksumUtils has further issues w/ CCSDSv2, as it assumes SID = MsgID in multiple cases, and doesn't account for the fact that ApidQSystemId is populated w/ the spacecraft ID by CFE_SB_InitMsg (which is a user configurable value, causing ExpRtnFrmGet to be wrong if the spacecraft ID is changed from the defaults).

@jphickey
Copy link
Contributor

I mistakenly filed a duplicate ticket about this because I just discovered it myself. Looks like there are a few issues in here.

@skliper skliper added this to the 6.8.0 milestone Apr 30, 2020
@jphickey jphickey self-assigned this May 5, 2020
jphickey added a commit to jphickey/cFE that referenced this issue May 5, 2020
The CFE_SB_GetMsgId/SetMsgId functions, among others, were implemented
as replicas of the actual FSW code.  This creates a dependency on the
actual definition of MsgId used by the mission.

This makes the stub and actual stub.  Stubs not actually
read/write to the message in any way, they just manipulate a
local (stored in the UT framework) out-of-band buffer to hold
the metadata about the message.

This revealed a few other minor issues in test cases where they
were depending on values sitting in globals (fixed).
jphickey added a commit to jphickey/cFE that referenced this issue May 6, 2020
The CFE_SB_GetMsgId/SetMsgId functions, among others, were implemented
as replicas of the actual FSW code.  This creates a dependency on the
actual definition of MsgId used by the mission.

This makes the stub and actual stub.  Stubs not actually
read/write to the message in any way, they just manipulate a
local (stored in the UT framework) out-of-band buffer to hold
the metadata about the message.

This revealed a few other minor issues in test cases where they
were depending on values sitting in globals (fixed).
astrogeco added a commit that referenced this issue May 8, 2020
Fix #618, stubs must not depend on real msgid implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment