jphickey
commented
1 year ago Looking into this issue today, but there is a bit of a deeper design problem here, related to the "EOT" telemetry message.
The problem is that the condition code represented by history->cc is one of the set defined per the CFDP protocol, that is on table 5-5 of CCSDS book 727. This is a very limited list and does not really represent all things that could go wrong with a transaction.
While in some cases there is a code that simply needs to be set (e.g. INACTIVITY_DETECTED in the case of having the inactivity timer expire), transactions are also reset in situations where protocol errors occurred (e.g. invalid values in fields, wrong or unhandleable PDU types for the transaction type) or unsupported/incompatible features were requested (e.g. large file). The CFDP CC table doesn't have suitable values for these possibilities.
Note that condition codes only appear in the CFDP protocol when sending the FIN/ACK/EOF PDUs, which only occur after successful protocol interaction to get to that point. That means the cc value in memory here is pretty meaningless until it gets to at least this point in the file transfer process. For transactions that are canceled for other reasons before getting to the end of file, the value is not relevant. The CFDP protocol does not accommodate this possibility because CC only appears in this limited scope of PDUs. So for the other conditions it is "don't care' because there is no CFDP-defined PDU in which a CC will be sent sent.
The problem only occurs with the the "EOT" telemetry packet because it sent for every transaction that ends and this value is included in the TLM, even when the CFDP protocol had not yet set it to something relevant. Basically, EOT is an extension of CFDP, but the condition codes where not extended accordingly.
Possible resolution would be to define a separate "transaction status" which can represent the full set of conditions for which CFDP might reset a transaction. This would be a superset of the condition codes defined in CFDP. Because a transaction status is a superset, it can be translated to a CFDP CC, but not vice versa. Thus it can still serve the same purpose for generating the FIN/ACK/EOF PDU, but also express more failure conditions that are not part of that set.
Checklist (Please check before submitting)
Describe the bug A call to CF_CFDP_SendEotPkt() was added to the CF_CFDP_ResetTransaction() function to give feedback on successful file transmission. However, It is sent every time the transaction is discarded regardless of the cause. I believe it was the intent that the t->history->cc could then be used to determine if it was successful.
However, it is unclear if t->history->cc is being correctly set on on every possible condition that calls CF_CFDP_ResetTransaction(). Particularly with the CFDP send control loop, it appears that there are cases where the the sending transaction is reset without setting t->history->cc which is used by CF_CFDP_SendEotPkt(). i.e. The CF_EotPacket telemetry would indicate success when it in fact did not complete successfully.
Before CF_CFDP_SendEotPkt() was added to CF_CFDP_ResetTransaction(), it did not matter if t->history->cc was set before calling CF_CFDP_ResetTransaction() since it wasn't used in the function before freeing the transaction.
To Reproduce Steps to reproduce the behavior: example -Let CFPD Send t->inactivity_timer timeout before completing the transaction.
Expected behavior t->history->cc must be set correctly for all possible cases before calling CF_CFDP_ResetTransaction(). Canceling a transaction or an error condition that leads to resetting the transaction must set t->history->cc to a value other than CF_CFDP_ConditionCode_NO_ERROR.
Unit tests for function that have an error condition that leads to resetting the transaction should verify that t->history->cc is also set to an error condition.
Code snips void CF_CFDP_SendEotPkt(CF_Transaction_t *t) { ... PktBuf->eot.cc = t->history->cc; ... }
System observed on:
Additional context Add any other context about the problem here.
Reporter Info Nathan Lynch JSC-ER611