expose CFE_SB_IsValidMsgId()

[skliper]

CFE apps would benefit from a publicly-available IsValidMsgId() function (or perhaps an expanded "is valid message" function, that would check all header fields?)

For example, SCH has a function to validate its message table entries and has its own logic for determining whether a message ID is valid or not.

This would particularly facilitate CCSDS_VER_2 transitions.

[skliper]

Imported from trac issue 232. Created by cdknight on 2018-01-16T18:26:34, last modified: 2019-07-03T12:48:08

[skliper]

Trac comment by jphickey on 2018-05-14 14:35:14:

Upvote this for next cFE release.

Particularly with the recent addition of extended CCSDS header, it is not necessarily a trivial check to determine if a MsgId value is valid or not. Apps should not be making assumptions about this value, and should only treat it as an opaque value.

Note that #245, if implemented, will turn any comparison test of MsgId values into a compiler error (a good thing). This would necessitate having a function to call in its place.

[skliper]

Trac comment by dcmccom3 on 2018-11-15 09:19:31:

In addition to the run-time functional change, we should create a compile-time solution. Multiple apps use an undefined MID macro for default table values. I looked at three apps(DS, HK, and SCH) and they each define a local undefined MID macro. They're also inconsistent in their definitions. DS & HK use 0 and SCH uses 0x1897.

Some preliminary thoughts:

1. The EDS build tool chain may be the long term solution
2. We need to check whether an app's code makes assumptions about the local definition. For example, simply checking whether a MID is great than the undefined value because the undefined value is assumed to be zero.

[skliper]

Trac comment by jphickey on 2018-11-15 09:38:37:

Can you elaborate on "compile-time solution"? There are two possibilities:

• A value to use as a placeholder/initializer for msgids within data structures
• A method to check if a given msgid is valid.

We also need to be sure apps differentiate between a "MsgId" and "TopicId". Many places that are checking MIDs would be better served by checking a topic ID instead and I think this will solve a lot of the issue.

Per our previous CCB nomenclature discussions (around the time of the CCSDS v2 header review), a topic ID is defined to be consistent across all CPUs in a mission (so i.e. CFE_ES is always assigned topic ID=1) whereas the MsgId is unique to a given instance of the app (so MsgId of CFE_ES on CPU1 is different than MsgId of CFE_ES on CPU2).

In other words, TopicId is a "normailzed MsgId".

The problem with MsgId is that there isn't one specific value we can isolate at compile time and say its always invalid, because it depends on framing type. TopicId is better here, as we //can// say that e.g. TopicId=0 is always reserved/invalid and this is independent of the framing type.

Historically we lacked this differentiation and apps were simply checking MsgId because its the value they got from the frame, when they probably should have been checking TopicId.

[skliper]

Trac comment by jhageman on 2019-07-03 12:48:08:

Moved unfinished 6.6.1 issues to next minor release

[skliper]

There is a related email chain requesting CFE_SB_GetPktType be exposed as an API, with responses of cmd, tlm, or invalid. This would support proposed SCH message table update to create the right type of packets using the MsgId.

[skliper]

Worth a note exposing as an API also needs a requirement and stub.

[skliper]

I'm concerned at this point MsgId to hdr and hdr to MsgId is more appropriately defined by the project, and you may need more information to go from MsgId to hdr. I think it will be challenging to generalize due to the extension and variety of use cases, other than saying there needs to be conversion APIs and it needs to produce a unique MsgId for each SB route., and fill in the rest of the header based on a MsgId? Or maybe these tables shouldn't use MsgId to try to fully define the header and we would benefit from separating the SB routing concept from generating headers?

[jphickey]

Yes, I agree that the conversion from a packet content to CFE_SB_MsgId_t value and vice versa needs to be isolated into code which the user may modify if they so choose. This is precisely what the EDS modifications developed by the GHAPS project did. There is a separate library for that handles the "translation" jobs of getting/setting a CFE_SB_MsgId_t from a packet payload in memory, as well as translating a CFE_SB_MsgId_t to a TopicId value.

However, all that being good - it is still separate from this issue, where we still need to expose this API regardless. This is intended to just test if a given CFE_SB_MsgId_t value is OK and it needs to be based on how the type is defined - we need to get away from apps directly doing an integer comparison on this value, it violates the opaqueness/abstraction that it is supposed to have.

[skliper]

Agreed, separate issue.

[skliper]

@jphickey Do you have time to split the fix out from #592 and submit a pull request covering this issue this round? @ejtimmon is pending on this to resolve some final app tickets.

[jphickey]

Yes, I will submit something today

[jphickey]

Clarification question -- what was the verdict on the application of API functions like:

if (CFE_SB_MsgId_Equal(MessageID, CFE_ES_SEND_HK_MID))

in place of the switch() statement?

Ideally I'd like to get that, plus the proper application of CFE_SB_MsgIdToValue/MsgIdFromValue macros into the build ASAP. (and of course exposing the IsValidMsgId routine too).

[skliper]

Yup, those changes are all good to go this round.

[CDKnightNASA]

Quibble, given that this will have long-term ripple effects, could we call it "CFE_SB_MsgId_Is()" instead of "Equal"? Also removes the expectation that the MsgId is a scalar value.

[jphickey]

I don't think the term Equal implies a scalar.... there is precedent for this too (e.g. see pthread_equal).

[skliper]

or could just if (CFE_SB_MsgIdToValue(MsgId)==value). IsEqual in my mind is for checking things of the same type, as in seeing if two MsgId's are equal. I don't have strong feelings with any of these suggestions, just adding another suggestion.

EDIT - typo

[skliper]

Or just relate the handler directly with the MsgId and skip the value all together. Somewhat related to suggestion to go to tables, and be able to register/unregister command handlers?

[jphickey]

Or just relate the handler directly with the MsgId and skip the value all together. Somewhat related to suggestion to go to tables, and be able to register/unregister command handlers?

Yes - the intent should be to get away from application code assuming there is some single integer "value" behind the MsgId in the first place. In C++ we could override the == operator, but in C we cannot so hence why we need a named equality test function.

But as the value is included in a number of syslog and event messages as an integer, the integer representation will probably need to exist for some time.

Message Dispatching should be done using a dispatch table that is simply registered in SB, so code doesn't need to replicate this in every app. Right now there is a lot of code that is similar between apps but all a little different.

After start up, the run loop would just be something like:

    while (CFE_ES_RunLoop(&SAMPLE_AppData.RunStatus))
    {
        /* Pend on receipt of command packet */
        status = CFE_SB_RcvMsg(&SAMPLE_AppData.MsgPtr,
                               SAMPLE_AppData.CommandPipe,
                               SAMPLE_APP_POLL_TIMEOUT);

        if (status == CFE_SUCCESS)
        {
            CFE_SB_DispatchMessage(SAMPLE_AppData.MsgPtr, &SAMPLE_CMD_DISPATCH_TABLE);
        }
    }

The SAMPLE_CMD_DISPATCH_TABLE could be auto-populated by the build system based on the commands that are enabled/disabled in the user config. Once we have each command handler in a separate compilation unit, this is easy.

At any rate, once this is in possible, the entire duplication of effort in e.g. SAMPLE_ProcessCommandPacket, SAMPLE_ProcessGroundCommand that one sees in every app can go away, and the MsgId equality test would probably be very rarely used by the apps at that point.