Fix #274, send packet when transaction resets #302

havencarlson · 2022-08-03T01:35:51Z

Checklist (Please check before submitting)

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution
Fix #274, when a transaction resets, an information packet is sent

Testing performed
Unit testing

Expected behavior changes
Sending a packet of information on a transaction when it is reset.

System(s) tested on

OS: Ubuntu 18.04

Contributor Info - All information REQUIRED for consideration of pull request
Haven Carlson - NASA

fsw/src/cf_cfdp.c

+
+    if (PktBuf != NULL)
+    {
+        CFE_MSG_Init(&PktBuf->eot.tlm_header.Msg, CFE_SB_ValueToMsgId(CF_EOT_TLM_MID), sizeof(*PktBuf));


fsw/src/cf_cfdp.c

+        ** Timestamp and send eod of transaction telemetry
+        */
+        CFE_SB_TimeStampMsg(&PktBuf->eot.tlm_header.Msg);
+        CFE_SB_TransmitBuffer(&PktBuf->SBBuf, true);


fsw/src/cf_cfdp.c

+ * See description in cf_cfdp.h for argument/return detail
+ *
+ *-----------------------------------------------------------------*/
+void CF_CFDP_SendEotPkt(CF_Transaction_t *t)


fsw/src/cf_msg.h

skliper

Should add a requirement and update design slides (and or coordinate w/ update in progress if @acudmore is working them). Requirement could be done as a follow-on issue/pr.

fsw/src/cf_msg.h

unit-test/cf_cfdp_tests.c

fsw/src/cf_msg.h

+    CF_TxnFilenames_t         fnames;     /**< \brief file names associated with this transaction */
+    CF_TxnState_t             state;      /**< \brief Transaction state */
+    CF_CFDP_ConditionCode_t   cc;         /**< \brief final condition code of operation */
+    CF_EntityId_t             src_eid;    /**< \brief the source eid of the transaction */


fsw/src/cf_msg.h

+ * that compiles with the alignment constraints of a CFE_SB_Buffer_t
+ */
+typedef union
+{


skliper

Looks like there were whitespace changes to the requirements. Preference is to use output exactly as provided from JIRA to make tracking changes more manageable. Maybe your editor added them? Also may want to add a requirement as to when the packet gets generated. Testers will need to add verification of this packet on transaction termination.

dzbaker · 2022-08-03T16:29:18Z

CCB 3 Aug 2022: Approved pending addition of NULL test, verifying no implicit padding, and review.

fsw/src/cf_cfdp.c

+    {
+        CFE_MSG_Init(&PktBuf->eot.tlm_header.Msg, CFE_SB_ValueToMsgId(CF_EOT_TLM_MID), sizeof(*PktBuf));
+
+        PktBuf->eot.channel    = t->chan_num;


fsw/src/cf_msg.h

+    CF_TxnState_t             state;      /**< \brief Transaction state */
+    CF_CFDP_ConditionCode_t   cc;         /**< \brief final condition code of operation */
+    CF_EntityId_t             src_eid;    /**< \brief the source eid of the transaction */
+    CF_EntityId_t             peer_eid;   /**< \brief peer_eid is always the "other guy", same src_eid for RX */


fsw/src/cf_msg.h

+{
+    CFE_MSG_TelemetryHeader_t tlm_header; /**< \brief Telemetry header */
+    CF_TransactionSeq_t       seq_num;    /**< \brief transaction identifier, stays constant for entire transfer */
+    uint32                    channel;    /**< \brief Channel number */


fsw/src/cf_msg.h

+typedef struct CF_EotPacket
+{
+    CFE_MSG_TelemetryHeader_t tlm_header; /**< \brief Telemetry header */
+    CF_TransactionSeq_t       seq_num;    /**< \brief transaction identifier, stays constant for entire transfer */


skliper

Minor comments, nothing required to change but possible suggestions if you want.

skliper · 2022-08-05T17:02:38Z

docs/cf_FunctionalRequirements.csv

+1. Transaction identifier
+2. Channel number
+3. Direction of transaction
+4. Filenames associated with transaction
+5. Transaction state
+6. Condition code
+7. Source EID of transaction
+8. Peer EID of transaction


Fine in this commit if this was already done in JIRA but in the future I wouldn't update a requirement just to change the order of parameters in a structure. Our requirements shouldn't dictate design/implementation down to that level. The goal is to keep requirements stable (minimize required document updates and all the associated analysis for a "requirements change")

skliper · 2022-08-05T17:05:43Z

fsw/src/cf_msg.h

    uint32                    fsize;      /**< \brief File size */
-    CF_Crc_t                  crc;        /**< \brief CRC state object */
+    CF_TxnFilenames_t         fnames;     /**< \brief file names associated with this transaction */


Usually strings are at the end? OK as is if there are no gaps... but just surprised to see it before a uint32.

skliper · 2022-08-05T17:06:55Z

unit-test/cf_cfdp_tests.c

+    /* setup for a call to CFE_SB_AllocateMessageBuffer() */
+    PktBufPtr = NULL;
+    UT_SetDataBuffer(UT_KEY(CFE_SB_AllocateMessageBuffer), &PktBufPtr, sizeof(PktBufPtr), true);


IIRC if you don't pass in a buffer it'll return NULL anyways, so you probably don't need this.

…it test

fsw/src/cf_msg.h

+    CF_CFDP_ConditionCode_t   cc;         /**< \brief final condition code of operation */
+    CF_EntityId_t             src_eid;    /**< \brief the source eid of the transaction */
+    CF_EntityId_t             peer_eid;   /**< \brief peer_eid is always the "other guy", same src_eid for RX */
+    uint32                    fsize;      /**< \brief File size */


fsw/src/cf_msg.h

+    CF_EntityId_t             src_eid;    /**< \brief the source eid of the transaction */
+    CF_EntityId_t             peer_eid;   /**< \brief peer_eid is always the "other guy", same src_eid for RX */
+    uint32                    fsize;      /**< \brief File size */
+    uint32                    crc_result; /**< \brief CRC result */


skliper · 2022-08-05T19:20:58Z

@dzbaker @havencarlson - heads up it looks like I missed the whitespace changes that snuck into the requirements .cvs. I recommend an export from JIRA again and put directly in w/o whitespace changes so the next person doesn't get surprised by the number of changes.

Fix #308, Revert requirements whitespace changes from #302

havencarlson requested review from a user and skliper August 3, 2022 01:35

github-advanced-security bot found potential problems Aug 3, 2022

View reviewed changes

skliper reviewed Aug 3, 2022

View reviewed changes

fsw/src/cf_msg.h Outdated Show resolved Hide resolved

fsw/src/cf_msg.h Outdated Show resolved Hide resolved

unit-test/cf_cfdp_tests.c Outdated Show resolved Hide resolved

havencarlson force-pushed the fix#274-eot_pkt branch from 28922e3 to b3637b2 Compare August 3, 2022 13:38

github-advanced-security bot found potential problems Aug 3, 2022

View reviewed changes

havencarlson force-pushed the fix#274-eot_pkt branch from b3637b2 to 577ed63 Compare August 3, 2022 15:11

havencarlson requested a review from skliper August 3, 2022 15:12

skliper requested changes Aug 3, 2022

View reviewed changes

havencarlson force-pushed the fix#274-eot_pkt branch 3 times, most recently from 009c1ac to 11a111d Compare August 3, 2022 15:51

havencarlson requested a review from skliper August 3, 2022 15:52

dzbaker added the CCB:Approved label Aug 3, 2022

havencarlson force-pushed the fix#274-eot_pkt branch from 11a111d to 3bdcc57 Compare August 4, 2022 14:17

alanc-gsfc and others added 2 commits August 4, 2022 10:19

Fix nasa#274, add end of transmission (eot) tlm packet

fe010a1

Fix nasa#274, send end of transaction packet when transaction is reset

0a3c218

havencarlson force-pushed the fix#274-eot_pkt branch from 3bdcc57 to 7c218d3 Compare August 5, 2022 13:59

github-advanced-security bot found potential problems Aug 5, 2022

View reviewed changes

havencarlson force-pushed the fix#274-eot_pkt branch 2 times, most recently from 6f1c629 to 02f86c7 Compare August 5, 2022 15:17

github-advanced-security bot found potential problems Aug 5, 2022

View reviewed changes

havencarlson force-pushed the fix#274-eot_pkt branch from 02f86c7 to be6df75 Compare August 5, 2022 16:52

skliper reviewed Aug 5, 2022

View reviewed changes

skliper approved these changes Aug 5, 2022

View reviewed changes

Fix nasa#274, rearrange fields in EoT packet and add null case for un…

954ad2b

…it test

havencarlson force-pushed the fix#274-eot_pkt branch from be6df75 to 954ad2b Compare August 5, 2022 17:49

github-advanced-security bot found potential problems Aug 5, 2022

View reviewed changes

dzbaker merged commit d623fa9 into nasa:main Aug 5, 2022

skliper mentioned this pull request Aug 8, 2022

Revert requirements custom whitespace changes #308

Closed

3 tasks

skliper added a commit to skliper/CF that referenced this pull request Aug 8, 2022

Fix nasa#308, Revert requirements whitespace changes from nasa#302

ebf5e58

dzbaker added a commit that referenced this pull request Aug 10, 2022

Merge pull request #309 from skliper/fix308-req_whitespace

4f00b05

Fix #308, Revert requirements whitespace changes from #302

skliper added this to the Draco milestone Aug 11, 2022

dmknutsen added the draco-rc3 label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #274, send packet when transaction resets #302

Fix #274, send packet when transaction resets #302

havencarlson commented Aug 3, 2022

skliper left a comment

skliper left a comment

dzbaker commented Aug 3, 2022

skliper left a comment

skliper Aug 5, 2022

skliper Aug 5, 2022

skliper Aug 5, 2022

skliper commented Aug 5, 2022

Fix #274, send packet when transaction resets #302

Fix #274, send packet when transaction resets #302

Conversation

havencarlson commented Aug 3, 2022

skliper left a comment

Choose a reason for hiding this comment

skliper left a comment

Choose a reason for hiding this comment

dzbaker commented Aug 3, 2022

skliper left a comment

Choose a reason for hiding this comment

skliper Aug 5, 2022

Choose a reason for hiding this comment

skliper Aug 5, 2022

Choose a reason for hiding this comment

skliper Aug 5, 2022

Choose a reason for hiding this comment

skliper commented Aug 5, 2022