Mismatch between MSG API and test case in "TestMsgId" functional test

[jphickey]

Describe the bug
There is a test case in the "TestMsgId" set which passes in CFE_SB_INVALID_MSG_ID to CFE_MSG_SetMsgId(), and expects CFE_MSG_BAD_ARGUMENT return value:

cFE/modules/cfe_testcase/src/message_id_test.c

Line 48 in 64a6a59

UtAssert_INT32_EQ(CFE_MSG_SetMsgId(&msg, CFE_SB_INVALID_MSG_ID), CFE_MSG_BAD_ARGUMENT);

However:

• The API does not document that it returns CFE_MSG_BAD_ARGUMENT in response to an invalid MsgId value (in fact it does not say anything about validating the input MsgId at all)
• The implementation is not actually checking if its a valid MsgId anyway. It is checking if it is > CFE_PLATFORM_SB_HIGHEST_VALID_MSGID, which is a different concept.

Although this is currently "passing" - it is only by chance, because CFE_SB_INVALID_MSG_ID has the value of -1, which when converted to an unsigned int, will be greater than CFE_PLATFORM_SB_HIGHEST_VALID_MSGID (unless the latter is set to 0xFFFFFFFF).

To Reproduce
Run this test against an alternate MSG module implementation (i.e. one that has different criteria) and/or change the SB definition of "CFE_SB_INVALID_MSG_ID". The test will now fail.

Expected behavior
Test case should still pass, even when run against an alternate MSG implementation. Should not depend on "chance" values that it does not control.

Code snips
Actual implementation is here (same basic check in v1/v2):

cFE/modules/msg/fsw/src/cfe_msg_msgid_v1.c

Line 67 in 64a6a59

if (MsgPtr == NULL || msgidval > CFE_PLATFORM_SB_HIGHEST_VALID_MSGID)

System observed on:
Ubuntu

Additional context
The important concept is nowhere does the documentation say that the CFE_SB_INVALID_MSG_ID constant must be greater than the CFE_PLATFORM_SB_HIGHEST_VALID_MSGID. In fact, the latter may not even exist in all implementations.

• If the intent was to reject an invalid MsgId value, the proper function to use is CFE_SB_IsValidMsgId, and the API documentation should state that CFE_MSG_BAD_ARGUMENT will be returned in response to an invalid MsgId (it does not currently say this).
• However, in general the MSG module is just supposed to be a getter/setter, not a validator, in its role. So in that sense, validating the MsgId is superfluous here, and the check against "highest" MsgId should be removed.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.