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

Dynamically choose TLM or CMD message type for PDU #224

Closed
3 tasks done
jphickey opened this issue Apr 7, 2022 · 5 comments · Fixed by #295
Closed
3 tasks done

Dynamically choose TLM or CMD message type for PDU #224

jphickey opened this issue Apr 7, 2022 · 5 comments · Fixed by #295
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Apr 7, 2022

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the CF README.md file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
Currently, the CF application uses the software bus for PDU transfer. (This is an issue in itself, see #130).

However, in the meantime, when using SB it must make the PDU messages look like the CMD/TLM messages that SB typically deals with. CF currently assumes/requires that all ingress/rx direction PDUs will have a CMD header, and all egress/tx direction PDUs will have a TLM header.

This assumption is a barrier to configuration of having two instances of CF potentially send files to each other - in such a configuration, there must be an intermediate entity to forward the "TLM"-style egress PDU messages and convert them to "CMD"-style ingress PDU messages on the other node, in both directions.

Describe the solution you'd like
To potentially make this configuration a bit easier, CF could dynamically choose which type of message header to use based on the configured MsgID values. That is, if the user has configured CF to output on a MsgId which (per the MSG module) is identified as a CMD type, it should use a CMD header when assembling those egress PDUs. Likewise, if the user has configured CF to input on a MsgID which is identified as a TLM type, then CF should strip a TLM header from that message.

Describe alternatives you've considered
Currently if CF-CF transfer is required, an intermediate transfer agent must fulfill this role to forward the PDU to the other entity and also convert to the other type (TLM->CMD) for ingest by CF.

Additional context
While this would address one pain point of this type of configuration, it still leaves two major ones:

  • SB pipes have inherent buffering limits
  • SB pipes cannot provide backpressure to sender (they just hit MsgLim and drop)

As a result the backpressure still needs to be implemented separately (via a sync sem) to avoid overflowing pipes. I'd still rather fix #130 to address all the issues inherent with using SB as a bulk data transfer mechanism - it isn't designed to be that.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@skliper skliper added this to the Draco milestone Jun 14, 2022
@skliper skliper changed the title Dynamically chose TLM or CMD message type for PDU Dynamically choose TLM or CMD message type for PDU Jul 11, 2022
@havencarlson havencarlson removed their assignment Jul 14, 2022
@dmknutsen dmknutsen assigned ghost and havencarlson Jul 19, 2022
@ghost
Copy link

ghost commented Jul 21, 2022

Some notes/implementation ideas based on an initial review of the code:
For sending and receiving PDUs, the software bus specific code and typedefs are in:

  • cf_cfdp_sbint.c
  • cf_cfdp_sbint.h
  • cf_cfdp_types.h - This file has the following relevant types
    -- CF_PduSendMsg_t
    -- CF_PduRecvMsg_t
    -- CF_Input_t
    -- CF_Output_t

CF_PduSendMsg_t has CFE_MSG_TelemetryHeader_t and
CF_PduRecvMsg_t has CFE_MSG_CommandHeader_t
The two types appear to only be used in cf_cfdp_sbint.c, so it might be possible to:

  1. Move CF_PduSendMsg_t to cf_cfdp_sbint.h
  2. Move CF_PduRecvMsg_t to cf_cfdp_sbint.h
  3. Rename CF_PduSendMsg_t to CF_PduTelemetryMsg_t
  4. Rename CF_PduRecvMsg_t to CF_PduCommandMsg_t

By renaming the Send and Recv types, either structure could be used for either PDU sends or Receives, depending on the type specified in the message ID for that channel.

After that, then the logic in cf_cfdp_sbint.c could be modified to look at the packet type based on the message ID, then select which type to use for the buffer offset.

Questions:

  • If a PDU is supposed to be sent in a software bus command packet, does a command code need to be defined? I would assume it does, but given that we want to abstract the PDU send from the transport, that should be in the sbint module rather than with the rest of the command codes in CF.
  • Is it necessary to allow for both send and receive code to handle CMD and TLM packet types? If we only change the Send PDU code, then a peer CF app would receive command PDUs as expected. If we only change the Receive PDU code to dynamically detect and account for either a CMD or TLM packet type, then it could handle PDU packets from the ground or a Peer. Seems like we only need to change send or receive, but not both.

Finally, for #130 , other than cf_cfdp_sbint.c and .h, the global CF app structures (engine sub-structure) has software bus message buffer pointers defined. If we wanted to work on removing the dependency on using the software bus for PDU transport, we might need to move those items into the cf_cfdp_sbint.c module. If these message pointers need to be global so multiple functions can access the message buffer, maybe generic get/set functions can be added to the PDU transport module. I think that would take us closer to having the transport encapsulated in one module.

@skliper
Copy link
Contributor

skliper commented Jul 21, 2022

... does a command code need to be defined?

No. Command code is ignored since the MID for PDU's needs to be unique (CC of 0 is fine).

... necessary to allow for both ...

Good point. If it was just always tlm it'd be fine (or always cmd). I'm not sure we get to pick that though since MID's are configuration defined? Could check them at startup and reject if it doesn't match expected type though.

I like the ideas for keeping the SB message buffer pointers within cf_cfdp_sbint so it can more easily be replaced. If those function calls could be made generic (if they aren't already) then could use source selection and replace with cf_cfdp_qint or whatever other interface is desired. Although I say this without looking very closely at the code :) There's also an open issue on those being in the global data to begin with, see #91.

@skliper
Copy link
Contributor

skliper commented Jul 21, 2022

Thinking about this more it seems like a great idea to just make everything tlm. Or cmd. Dealer's choice! Eliminate the extra complexity of selecting since there's no requirement. It's been cmd on one side and tlm on the other forever so peers would have needed to handle both, so unless someone comes up with a compelling reason not to it sounds good to me. We should confirm w/ stakeholders this is ok.

@ghost
Copy link

ghost commented Jul 21, 2022

I think wrapping all PDUs in telemetry packets is my preference. They are time stamped in the secondary header, so that could provide some extra diagnostic info vs the command code which is useless in this case.

@ghost
Copy link

ghost commented Jul 25, 2022

Latest thoughts: Requiring ground systems to send PDUs to CF will require changes to each ground system that supports CF/CFDP. Likewise, sending all PDU packets as commands may also break existing flight and ground software, as we may not have any current use cases of sending command packets to the ground.

I think the least disruptive change is to have CF dynamically handle incoming PDUs as either CMD or TLM packets. This way, CF can receive PDUs from the ground wrapped in a command packet, or it could receive PDUs from a peer wrapped in a telemetry packet.

@ghost ghost mentioned this issue Aug 2, 2022
2 tasks
dzbaker added a commit that referenced this issue Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants