expose CFE_SB_IsValidMsgId() #263

skliper · 2019-09-30T19:14:27Z

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 · 2019-09-30T19:14:28Z

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

skliper · 2019-09-30T19:14:28Z

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 · 2019-09-30T19:14:29Z

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:

The EDS build tool chain may be the long term solution
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 · 2019-09-30T19:14:30Z

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 · 2019-09-30T19:14:30Z

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

Moved unfinished 6.6.1 issues to next minor release

skliper · 2019-11-20T16:34:22Z

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 · 2020-03-06T18:18:10Z

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

skliper · 2020-03-31T01:15:31Z

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 · 2020-03-31T12:53:05Z

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 · 2020-03-31T13:06:38Z

Agreed, separate issue.

skliper · 2020-04-10T12:27:26Z

@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 · 2020-04-10T12:40:48Z

Yes, I will submit something today

jphickey · 2020-04-10T14:30:31Z

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 · 2020-04-10T14:33:01Z

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

CDKnightNASA · 2020-04-10T16:06:18Z

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.

Make CFE core apps consistent in their use of the CFE_SB_MsgId type and with the CFE SB API. This employs the CFE SB API whenever any of the following needs to happen: - Use of a CFE_SB_MsgId_t value within a printf (event, syslog, etc). - Initialization of a CFE_SB_MsgId_t from an integer value - Comparison of two CFE_SB_MsgId_t values - Checking if a CFE_SB_MsgId_t value is within the valid set Notably, this replaces the switch/case constructs used in message dispatch with a nested if/else based on CFE_SB_MsgId_Equal(), as the switch statement can only be used with an integer. A few new macros are introduced, mainly because the inline functions that already existed for this purpose cannot be used where evaluation must be done at compile time (e.g. constants, struct initialization). These are initially just typecasts, but could become more interesting in future revisions.

jphickey · 2020-04-10T16:48:36Z

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

Make CFE core apps consistent in their use of the CFE_SB_MsgId type and with the CFE SB API. This employs the CFE SB API whenever any of the following needs to happen: - Use of a CFE_SB_MsgId_t value within a printf (event, syslog, etc). - Initialization of a CFE_SB_MsgId_t from an integer value - Comparison of two CFE_SB_MsgId_t values - Checking if a CFE_SB_MsgId_t value is within the valid set Notably, this replaces the switch/case constructs used in message dispatch with a nested if/else based on CFE_SB_MsgId_Equal(), as the switch statement can only be used with an integer. A few new macros are introduced, mainly because the inline functions that already existed for this purpose cannot be used where evaluation must be done at compile time (e.g. constants, struct initialization). These are initially just typecasts, but could become more interesting in future revisions.

skliper · 2020-04-10T17:07:42Z

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 · 2020-04-10T17:10:33Z

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?

Make CFE core apps consistent in their use of the CFE_SB_MsgId type and with the CFE SB API. This employs the CFE SB API whenever any of the following needs to happen: - Use of a CFE_SB_MsgId_t value within a printf (event, syslog, etc). - Initialization of a CFE_SB_MsgId_t from an integer value - Comparison of two CFE_SB_MsgId_t values - Checking if a CFE_SB_MsgId_t value is within the valid set Notably, this replaces the switch/case constructs used in message dispatch with a nested if/else based on CFE_SB_MsgId_Equal(), as the switch statement can only be used with an integer. A few new macros are introduced, mainly because the inline functions that already existed for this purpose cannot be used where evaluation must be done at compile time (e.g. constants, struct initialization). These are initially just typecasts, but could become more interesting in future revisions.

jphickey · 2020-04-10T17:54:23Z

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.

Make CFE core apps consistent in their use of the CFE_SB_MsgId type and with the CFE SB API. This employs the CFE SB API whenever any of the following needs to happen: - Use of a CFE_SB_MsgId_t value within a printf (event, syslog, etc). - Initialization of a CFE_SB_MsgId_t from an integer value - Comparison of two CFE_SB_MsgId_t values - Checking if a CFE_SB_MsgId_t value is within the valid set A few new macros are introduced, mainly because the inline functions that already existed for this purpose cannot be used where evaluation must be done at compile time (e.g. constants, struct initialization). These are initially just typecasts, but could become more interesting in future revisions.

Manually merged to resolve conflicts. From branch jphickey/fix-263-msgid-api: Fix #245, Consistent use of MsgId type Fix #263, Expose CFE_SB_IsValidMsgId() API From branch dmknutsen/issue_240: Fixes #240, removes MESSAGE_FORMAT_IS_CCSDS ifdefs from code

skliper added this to the 6.7.1 milestone Sep 30, 2019

skliper self-assigned this Sep 30, 2019

skliper added the enhancement label Sep 30, 2019

skliper removed their assignment Sep 30, 2019

skliper modified the milestones: 6.7.1, 6.7.0, 6.8.0 Sep 30, 2019

skliper removed this from the 6.8.0 milestone Nov 4, 2019

jphickey mentioned this issue Feb 12, 2020

CFE_SB_GetLastSenderID will crash if if called before message sent on pipe #494

Closed

skliper mentioned this issue Mar 6, 2020

expose CFE_SB_GetPktType (and add stub) #543

Closed

This was referenced Apr 7, 2020

Add requirements to cover IsValidMsgId and GetPktType #587

Closed

Fix #245, Opaque CFE_SB_MsgId_t values #592

Closed

skliper added this to the 6.8.0 milestone Apr 10, 2020

jphickey mentioned this issue Apr 10, 2020

Fix #263, expose IsValidMsgID and partially address #245 Consistent use of MsgId types #602

Merged

jphickey mentioned this issue Apr 15, 2020

Fix #45, Refactor to implement command and utility functions in separate files nasa/sample_app#58

Merged

skliper assigned jphickey Apr 21, 2020

astrogeco mentioned this issue Apr 27, 2020

Integration Candidate: 2020-04-15 #635

Merged

astrogeco closed this as completed in 537f9b8 Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expose CFE_SB_IsValidMsgId() #263

expose CFE_SB_IsValidMsgId() #263

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Nov 20, 2019

skliper commented Mar 6, 2020

skliper commented Mar 31, 2020

jphickey commented Mar 31, 2020

skliper commented Mar 31, 2020

skliper commented Apr 10, 2020

jphickey commented Apr 10, 2020

jphickey commented Apr 10, 2020

skliper commented Apr 10, 2020

CDKnightNASA commented Apr 10, 2020

jphickey commented Apr 10, 2020

skliper commented Apr 10, 2020 •

edited

Loading

skliper commented Apr 10, 2020

jphickey commented Apr 10, 2020

expose CFE_SB_IsValidMsgId() #263

expose CFE_SB_IsValidMsgId() #263

Comments

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Sep 30, 2019

skliper commented Nov 20, 2019

skliper commented Mar 6, 2020

skliper commented Mar 31, 2020

jphickey commented Mar 31, 2020

skliper commented Mar 31, 2020

skliper commented Apr 10, 2020

jphickey commented Apr 10, 2020

jphickey commented Apr 10, 2020

skliper commented Apr 10, 2020

CDKnightNASA commented Apr 10, 2020

jphickey commented Apr 10, 2020

skliper commented Apr 10, 2020 • edited Loading

skliper commented Apr 10, 2020

jphickey commented Apr 10, 2020

skliper commented Apr 10, 2020 •

edited

Loading