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.

Sign up for GitHub

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

Jump to bottom

Fix #224, support dynamic pdu packets #295

Merged
merged 3 commits into from Aug 3, 2022
Merged

Fix #224, support dynamic pdu packets #295

merged 3 commits into from Aug 3, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 29, 2022 •
edited by ghost
Loading

Checklist (Please check before submitting)

Describe the contribution
Fix #224
In order to support CF peer to peer transactions, this change will allow incoming PDU packets to be either command or telemetry packets. This is handled in the cf_cfdp_sbintf.c file that handles PDUs wrapped in CCSDS packets.
This change required renaming structures:

CF_PduSendMsg_t to CF_PduTlmMsg_t
CF_PduRecvMsg_t to CF_PduCmdMsg_t

This rename makes it less confusing if CF receives a PDU wrapped in a TLM packet, for example. In this case, it would not make sense to receive a "Send" type packet.

In addition to renaming these structures, the typedefs were moved to cf_cfdp_sbint.h as a step toward decoupling the PDU send and receive from the software bus.

By allowing incoming PDU packets to be wrapped in command or telemetry CCSDS packets, CF can receive PDUs from the ground wrapped in a command packet, or PDUs from a CF peer wrapped in a telemetry packet.

Testing performed

  1. Built and ran (updated) unit tests.
  2. Set up a CF peer-to-peer configuration and verified a type 1 TX transaction works from one CF peer to another. Tested that the CF peer can receive the PDUs as telemetry or command packets.

Expected behavior changes

  • No impact to normal behavior, but does enable direct peer to peer PDU transactions CF1(TLM PDU) -> CF2(TLM PDU)

System(s) tested on

  • PC Virtual Machine
  • OS: Ubuntu 20.04
  • Versions: cFS main (7/28/2022)

Contributor Info - All information REQUIRED for consideration of pull request
Alan Cudmore / NASA GSFC / Code 582.0

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 29, 2022
View reviewed changes
fsw/src/cf_cfdp_sbintf.c
ph = &CF_AppData.engine.in.rx_pdudata;
CFE_ES_PerfLogEntry(CF_PERF_ID_PDURCVD(chan_num));
CFE_MSG_GetSize(&bufptr->Msg, &msg_size);
CF_CFDP_DecodeStart(&CF_AppData.engine.in.decode, bufptr, ph, offsetof(CF_PduRecvMsg_t, ph), msg_size);
CFE_MSG_GetType(&bufptr->Msg, &msg_type);

Check warning

Code scanning / CodeQL-coding-standard

Unchecked return value

The return value of non-void function [CFE_MSG_GetType](1) is not checked.
@skliper
Copy link
Contributor

skliper commented Jul 29, 2022

Can you rebase to get rid of that merge commit?

@skliper
Copy link
Contributor

skliper commented Aug 1, 2022

Add - Fix #224 in the text of the PR to auto-link to the issue (so it will close when this PR is merged)

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 1, 2022
View reviewed changes
fsw/src/cf_cfdp_sbintf.c
CF_Transaction_t *t; /* initialized below */
uint32 count = 0;
int32 status;
const int chan_num = (c - CF_AppData.engine.channels);

Check warning

Code scanning / CodeQL-coding-standard

Unchecked function argument

This use of parameter c has not been checked.
fsw/src/cf_cfdp_sbintf.c
CFE_SB_Buffer_t * bufptr;
CFE_MSG_Size_t msg_size;
CF_Transaction_t *t; /* initialized below */
uint32 count = 0;

Check notice

Code scanning / CodeQL-coding-standard

Use of basic integral type

count uses the basic integral type unsigned int rather than a typedef with size and signedness.
fsw/src/cf_cfdp_sbintf.c
CFE_MSG_Size_t msg_size;
CF_Transaction_t *t; /* initialized below */
uint32 count = 0;
int32 status;

Check notice

Code scanning / CodeQL-coding-standard

Use of basic integral type

status uses the basic integral type signed int rather than a typedef with size and signedness.
fsw/src/cf_cfdp_sbintf.c
CF_Transaction_t *t; /* initialized below */
uint32 count = 0;
int32 status;
const int chan_num = (c - CF_AppData.engine.channels);

Check notice

Code scanning / CodeQL-coding-standard

Use of basic integral type

chan_num uses the basic integral type int rather than a typedef with size and signedness.
fsw/src/cf_cfdp_sbintf.c
int32 status;
const int chan_num = (c - CF_AppData.engine.channels);
CFE_SB_Buffer_t * bufptr;
CFE_MSG_Size_t msg_size;

Check notice

Code scanning / CodeQL-coding-standard

Use of basic integral type

msg_size uses the basic integral type unsigned long rather than a typedef with size and signedness.
@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 3, 2022
@skliper skliper added this to the Draco milestone Aug 3, 2022
@dzbaker dzbaker added CCB:Approved and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 3, 2022
@dzbaker
Copy link
Contributor

dzbaker commented Aug 3, 2022

CCB 3 Aug 2022: Approved pending documentation.

@ghost
Copy link
Author

ghost commented Aug 3, 2022

Information about the CF peer to peer test conducted:
Using a cFS bundle build with

  • CF
  • PSP "iodriver" and "unsock_intf" Modules to allow serial data transfer from one instance of cFS to another on Linux
  • SGW App - uses the PSP modules to send packets back and forth between cFS instances with packet translation (cmd-> tlm, etc)

Normally, the SGW app reads PDUs wrapped in telemetry packets from the software bus, sends them to the peer cFS system, and sends the packet data on the SB using a new message ID. This enables the following unmodified CF peer to peer transactions:

  • CPU1 CF -> TLM PDU --> CPU1 SGW --> PSP modules --> CPU2 SGW --> CMD PDU --> CPU2 CF
  • TLM PDU (0x080a) --> SGW translation --> CMD PDU (0x190a)
  • CPU2 CF is configured to receive PDUs on message ID 0x190a

With this pull request applied to allow CF to receive PDUs wrapped in TLM packets, I changed the SGW configuration to wrap the PDU data using the same packet type/message ID:

  • TLM PDU (0x080a) --> SGW translation (essentially a no-op) --> TLM PDU (0x080a)
  • CPU2 CF is configured to receive PDUs on message ID 0x080a

The peer to peer transaction works as expected.

@dzbaker dzbaker merged commit 0442109 into nasa:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CCB:Approved draco-rc4
Projects
None yet
Milestone
Draco
Development

Successfully merging this pull request may close these issues.

Dynamically choose TLM or CMD message type for PDU
3 participants
@skliper @dzbaker @alanc-gsfc