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

SB subscription reporting request messages out of family #523

Closed
skliper opened this issue Feb 20, 2020 · 9 comments · Fixed by #647 or #692
Closed

SB subscription reporting request messages out of family #523

skliper opened this issue Feb 20, 2020 · 9 comments · Fixed by #647 or #692
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Feb 20, 2020

Is your feature request related to a problem? Please describe.
CFE_SB_EnableSubReportingCmd, CFE_SB_DisableSubReportingCmd, CFE_SB_SendPrevSubsCmd are processed like commands but don't increment the command counter.

Typical pattern is for non-ground, inter-app messages to have separate message IDs from ground commands.

Describe the solution you'd like
Make consistent with standard pattern

Describe alternatives you've considered
None

Additional context
See Hk message processing, or the message processing in Time services that don't increment command counter.

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 6.8.0 milestone Feb 25, 2020
@skliper
Copy link
Contributor Author

skliper commented Apr 8, 2020

@CDKnightNASA @tngo67 Does SBN or SBNg use these? Since they are from another app, preference would be to not use the ground command MID. Can we coordinate a change?

@jwilmot
Copy link

jwilmot commented Apr 21, 2020

As I recall these were for SBN to use when interfacing with SB. They are not ground commands. SBN is designed to be (as best we could) a plug-in just like any other apps. We wanted to avoid call backs or other compile/link time hooks. That said, the internal MID are in the same name space as "regular" commands so need to be allocated and managed but not in the ground system database. They should not increment the command counter +/- as those are for operators, but should send an event if an error occurs.

@skliper
Copy link
Contributor Author

skliper commented Apr 21, 2020

I think you are saying they should be a separate MID from the ground commands? Want them to use the same MID (CFE_SB_SUBSCRIPTION_RPT_CONTROL_MID or whatever), or each use their own?

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 21, 2020
@CDKnightNASA
Copy link
Contributor

CDKnightNASA commented Apr 21, 2020

As I recall these were for SBN to use when interfacing with SB. They are not ground commands. SBN is designed to be (as best we could) a plug-in just like any other apps. We wanted to avoid call backs or other compile/link time hooks. That said, the internal MID are in the same name space as "regular" commands so need to be allocated and managed but not in the ground system database. They should not increment the command counter +/- as those are for operators, but should send an event if an error occurs.

Nope, SBN does not use this API. SBN listens for events that are generated by subscription activity:

    Status = CFE_SB_SubscribeLocal(CFE_SB_ALLSUBS_TLM_MID, SBN.SubPipe,
...
    Status = CFE_SB_SubscribeLocal(CFE_SB_ONESUB_TLM_MID, SBN.SubPipe,

@skliper
Copy link
Contributor Author

skliper commented Apr 21, 2020

So who/what sends the referenced commands?

@CDKnightNASA
Copy link
Contributor

CDKnightNASA commented Apr 21, 2020

So who/what sends the referenced commands?

Sorry, looking through the code, SBN does send the relevant commands. I was looking for direct calls to the API.

sbn_sub.c:

void SBN_SendSubsRequests(void)
{
    CFE_SB_CmdHdr_t     SBCmdMsg;

    /* Turn on SB subscription reporting */
    CFE_SB_InitMsg(&SBCmdMsg, CFE_SB_CMD_MID, sizeof(CFE_SB_CmdHdr_t),
        TRUE);
    CFE_SB_SetCmdCode((CFE_SB_MsgPtr_t) &SBCmdMsg,
        CFE_SB_ENABLE_SUB_REPORTING_CC);
    CFE_SB_SendMsg((CFE_SB_MsgPtr_t) &SBCmdMsg);

    /* Request a list of previous subscriptions from SB */
    CFE_SB_SetCmdCode((CFE_SB_MsgPtr_t) &SBCmdMsg, CFE_SB_SEND_PREV_SUBS_CC);
    CFE_SB_SendMsg((CFE_SB_MsgPtr_t) &SBCmdMsg);
}/* end SBN_SendSubsRequests */

@jwilmot
Copy link

jwilmot commented Apr 22, 2020

The required SBN behavior is to requests subscriptions when it is done initializing the subnetwork and discovered peers. This can happen any time after local system init or later due to fault management restarting SBN. This decouples SBN from the local system. The internal commands CFE_SB_EnableSubReportingCmd, CFE_SB_DisableSubReportingCmd, CFE_SB_SendPrevSubsCmd support this. Like other internal commands, the counters should not increment but error events should be sent. Sorry I am being dense, but it's still not clear what problem this ticket is trying to solve.

@skliper
Copy link
Contributor Author

skliper commented Apr 22, 2020

Discussed w/ @jwilmot and got concurrence with the following approach.

Inter-app messages/MIDs:
CFE_SB_SEND_HK_MID - For scheduler to request HK message
CFE_SB_SUBSCRIPTION_RPT_CONTROL_MID - Use for these three commands

Ground command MID:
CFE_SB_CMD_MID - ground commands only

So it'll require a 1 line change in SBN, but provides separation between the inter-app and ground commands (preferred model).

skliper added a commit to skliper/cFE that referenced this issue Apr 28, 2020
skliper added a commit to skliper/cFS that referenced this issue Apr 28, 2020
@skliper skliper self-assigned this Apr 28, 2020
@astrogeco astrogeco added CCB-20200429 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels May 5, 2020
astrogeco added a commit that referenced this issue May 8, 2020
Fix #523, SB Subscription report control on separate MID
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue May 11, 2020
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue May 11, 2020
@juliejohn015

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants