Skip to content

Commit

Permalink
Fix nasa#67, remove use of bitfields in CF
Browse files Browse the repository at this point in the history
Bit field behavior is platform-specific, bits are not
specified to be in any particular order. Furthermore,
unions of bitfields are likely undefined behavior.

This removes the bitfields and replaces with normal
fields.
  • Loading branch information
jphickey committed Dec 8, 2021
1 parent 7b99b91 commit 5cece50
Show file tree
Hide file tree
Showing 12 changed files with 300 additions and 308 deletions.
24 changes: 12 additions & 12 deletions fsw/src/cf_cfdp.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ typedef struct
void CF_CFDP_ArmAckTimer(transaction_t *t)
{
CF_Timer_InitRelSec(&t->ack_timer, CF_AppData.config_table->ack_timer_s);
t->flags.all.ack_timer_armed = 1;
t->flags.com.ack_timer_armed = 1;
}

/************************************************************************/
Expand All @@ -102,7 +102,7 @@ void CF_CFDP_ArmAckTimer(transaction_t *t)
*************************************************************************/
static inline cfdp_class_t CF_CFDP_GetClass(const transaction_t *ti)
{
CF_Assert(ti->flags.all.q_index != CF_Q_FREE);
CF_Assert(ti->flags.com.q_index != CF_Q_FREE);
return !!((ti->state == CFDP_S2) || (ti->state == CFDP_R2));
}

Expand All @@ -119,7 +119,7 @@ static inline cfdp_class_t CF_CFDP_GetClass(const transaction_t *ti)
*************************************************************************/
static inline int CF_CFDP_IsSender(transaction_t *ti)
{
CF_Assert(ti->flags.all.q_index != CF_Q_FREE);
CF_Assert(ti->flags.com.q_index != CF_Q_FREE);
/* the state could actually be CFDP_IDLE, which is still not a sender. This would
* be an unused transaction in the RX (CF_CFDP_ReceiveMessage) path. */
return !!((ti->state == CFDP_S1) || (ti->state == CFDP_S2));
Expand Down Expand Up @@ -278,7 +278,7 @@ static void CF_CFDP_FreeTransaction(transaction_t *t)
{
uint8 c = t->chan_num;
memset(t, 0, sizeof(*t));
t->flags.all.q_index = CF_Q_FREE;
t->flags.com.q_index = CF_Q_FREE;
t->fd = OS_OBJECT_ID_UNDEFINED;
t->chan_num = c;
t->state = CFDP_IDLE; /* NOTE: this is redundant as long as CFDP_IDLE == 0 */
Expand Down Expand Up @@ -391,7 +391,7 @@ pdu_header_t *CF_CFDP_MsgOutGet(const transaction_t *t, int silent)
goto error_out;
}

if (!CF_AppData.hk.channel_hk[t->chan_num].frozen && !t->flags.all.suspended)
if (!CF_AppData.hk.channel_hk[t->chan_num].frozen && !t->flags.com.suspended)
{
/* first, check if there's room in the pipe for the message we want to build */
if (!CF_AppData.engine.out.msg && ((CF_AppData.config_table->chan[t->chan_num].sem_name[0] &&
Expand Down Expand Up @@ -1444,7 +1444,7 @@ static void CF_CFDP_ReceiveMessage(channel_t *c)
t->state_data.r.r2.dc = FIN_INCOMPLETE;
t->state_data.r.r2.fs = FIN_DISCARDED;

CF_CList_InsertBack_Ex(c, (t->flags.all.q_index = CF_Q_RX), &t->cl_node);
CF_CList_InsertBack_Ex(c, (t->flags.com.q_index = CF_Q_RX), &t->cl_node);
CF_CFDP_DispatchRecv(t); /* will enter idle state */
}
}
Expand Down Expand Up @@ -1484,18 +1484,18 @@ static int CF_CFDP_CycleTx_(clist_node node, void *context)
transaction_t *t = container_of(node, transaction_t, cl_node);
int ret = 1; /* default option is exit traversal */

if (t->flags.all.suspended)
if (t->flags.com.suspended)
{
ret = 0; /* suspended, so move on to next */
goto err_out;
}

CF_Assert(t->flags.all.q_index == CF_Q_TXA); /* huh? */
CF_Assert(t->flags.com.q_index == CF_Q_TXA); /* huh? */

/* if no more messages, then c->cur will be set.
* If the transaction sent the last filedata pdu and eof, it will move itself
* off the active queue. Run until either of these occur. */
while (!args->c->cur && t->flags.all.q_index == CF_Q_TXA)
while (!args->c->cur && t->flags.com.q_index == CF_Q_TXA)
{
CFE_ES_PerfLogEntry(CF_PERF_ID_PDUSENT(t->chan_num));
CF_CFDP_DispatchTx(t);
Expand Down Expand Up @@ -1576,7 +1576,7 @@ static int CF_CFDP_DoTick(clist_node node, void *context)
{
/* found where we left off, so clear that and move on */
args->c->cur = NULL;
if (!t->flags.all.suspended)
if (!t->flags.com.suspended)
{
args->fn(t, &args->cont);
}
Expand Down Expand Up @@ -2203,9 +2203,9 @@ int CF_CFDP_CopyDataFromLv(uint8 buf[CF_FILENAME_MAX_LEN], const lv_t *src_lv)
void CF_CFDP_CancelTransaction(transaction_t *t)
{
void (*fns[2])(transaction_t * t) = {CF_CFDP_R_Cancel, CF_CFDP_S_Cancel};
if (!t->flags.all.canceled)
if (!t->flags.com.canceled)
{
t->flags.all.canceled = 1;
t->flags.com.canceled = 1;
t->history->cc = CC_CANCEL_REQUEST_RECEIVED;
fns[!!CF_CFDP_IsSender(t)](t);
}
Expand Down
60 changes: 26 additions & 34 deletions fsw/src/cf_cfdp.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,18 @@ typedef struct
uint16 num_ts; /* number of transactions -- 16 bit should be enough */
uint8 priority;
cf_entity_id_t dest_id;
unsigned busy : 1;
unsigned diropen : 1;
unsigned keep : 1;
unsigned counted : 1;

bool busy;
bool diropen;
bool keep;
bool counted;
} playback_t;

typedef struct
{
playback_t pb;
cf_timer_t interval_timer;
unsigned timer_set : 1;
bool timer_set;
} poll_t;

typedef union
Expand Down Expand Up @@ -176,47 +177,38 @@ typedef struct

typedef struct
{
unsigned q_index : 3; /* which Q is this in? */
unsigned ack_timer_armed : 1;
unsigned suspended : 1;
unsigned canceled : 1;
unsigned crc_calc : 1;
uint8 q_index; /* which Q is this in? */
bool ack_timer_armed;
bool suspended;
bool canceled;
bool crc_calc;
} flags_all_t;

typedef struct
{
unsigned q_index : 3; /* which Q is this in? */
unsigned ack_timer_armed : 1;
unsigned suspended : 1;
unsigned canceled : 1;
unsigned crc_calc : 1;

unsigned md_recv : 1; /* md received for r state */
unsigned eof_recv : 1;
unsigned send_nak : 1;
unsigned send_fin : 1;
unsigned send_ack : 1;
unsigned inactivity_fired : 1; /* used for r2 */
unsigned complete : 1; /* r2 */
unsigned fd_nak_sent : 1; /* latches that at least one nak has been sent for file data */
flags_all_t com;

bool md_recv; /* md received for r state */
bool eof_recv;
bool send_nak;
bool send_fin;
bool send_ack;
bool inactivity_fired; /* used for r2 */
bool complete; /* r2 */
bool fd_nak_sent; /* latches that at least one nak has been sent for file data */
} flags_rx_t;

typedef struct
{
unsigned q_index : 3; /* which Q is this in? */
unsigned ack_timer_armed : 1;
unsigned suspended : 1;
unsigned canceled : 1;
unsigned crc_calc : 1;

unsigned md_need_send : 1;
unsigned cmd_tx : 1; /* indicates transaction is commanded (ground) tx */
flags_all_t com;

bool md_need_send;
bool cmd_tx; /* indicates transaction is commanded (ground) tx */
} flags_tx_t;

typedef union
{
uint16 flags;
flags_all_t all;
flags_all_t com;
flags_rx_t rx;
flags_tx_t tx;
} state_flags_t;
Expand Down
8 changes: 4 additions & 4 deletions fsw/src/cf_cfdp_r.c
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ static inline void CF_CFDP_R1_Reset(transaction_t *t)
static void CF_CFDP_R2_Reset(transaction_t *t)
{
if ((t->state_data.r.sub_state == RECV_WAIT_FOR_FIN_ACK) || (t->state_data.r.r2.eof_cc != CC_NO_ERROR) ||
(t->history->cc != CC_NO_ERROR) || t->flags.rx.canceled)
(t->history->cc != CC_NO_ERROR) || t->flags.com.canceled)
{
CF_CFDP_R1_Reset(t); /* it's done */
}
Expand Down Expand Up @@ -758,7 +758,7 @@ static int CF_CFDP_R2_CalcCrcChunk(transaction_t *t)
CF_CFDP_R2_SetCc(t, CC_FILE_CHECKSUM_FAILURE);
}

t->flags.rx.crc_calc = 1;
t->flags.com.crc_calc = 1;

ret = 0;
}
Expand All @@ -783,7 +783,7 @@ static int CF_CFDP_R2_SubstateSendFin(transaction_t *t)
cfdp_send_ret_t sret;
int ret = -1;

if (t->history->cc == CC_NO_ERROR && !t->flags.rx.crc_calc)
if (t->history->cc == CC_NO_ERROR && !t->flags.com.crc_calc)
{
/* no error, and haven't checked crc -- so start checking it */
if (CF_CFDP_R2_CalcCrcChunk(t))
Expand Down Expand Up @@ -1126,7 +1126,7 @@ void CF_CFDP_R_Tick(transaction_t *t, int *cont /* unused */)
/* don't care about any other cases */
}

if (t->flags.rx.ack_timer_armed)
if (t->flags.com.ack_timer_armed)
{
if (CF_Timer_Expired(&t->ack_timer))
{
Expand Down
14 changes: 7 additions & 7 deletions fsw/src/cf_cfdp_s.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ static inline void CF_CFDP_S_Reset(transaction_t *t)
*************************************************************************/
static cfdp_send_ret_t CF_CFDP_S_SendEof(transaction_t *t)
{
if (!t->flags.tx.crc_calc)
if (!t->flags.com.crc_calc)
{
CF_CRC_Finalize(&t->crc);
t->flags.tx.crc_calc = 1;
t->flags.com.crc_calc = 1;
}
return CF_CFDP_SendEof(t);
}
Expand Down Expand Up @@ -106,8 +106,8 @@ static void CF_CFDP_S1_SubstateSendEof(transaction_t *t)
*************************************************************************/
static void CF_CFDP_S2_SubstateSendEof(transaction_t *t)
{
t->state_data.s.sub_state = SEND_WAIT_FOR_EOF_ACK;
t->flags.tx.ack_timer_armed = 1; /* will cause tick to see ack_timer as expired, and act */
t->state_data.s.sub_state = SEND_WAIT_FOR_EOF_ACK;
t->flags.com.ack_timer_armed = 1; /* will cause tick to see ack_timer as expired, and act */

/* no longer need to send file data PDU except in the case of NAK response */

Expand Down Expand Up @@ -596,8 +596,8 @@ static void CF_CFDP_S2_WaitForEofAck(transaction_t *t, const pdu_header_t *ph)
}
else
{
t->state_data.s.sub_state = SEND_WAIT_FOR_FIN;
t->flags.tx.ack_timer_armed = 0; /* just wait for fin now, nothing to re-send */
t->state_data.s.sub_state = SEND_WAIT_FOR_FIN;
t->flags.com.ack_timer_armed = 0; /* just wait for fin now, nothing to re-send */
}
}
else
Expand Down Expand Up @@ -781,7 +781,7 @@ void CF_CFDP_S_Tick(transaction_t *t, int *cont /* unused */)
{
CF_Timer_Tick(&t->inactivity_timer);

if (t->flags.tx.ack_timer_armed)
if (t->flags.com.ack_timer_armed)
{
if (CF_Timer_Expired(&t->ack_timer))
{
Expand Down
4 changes: 2 additions & 2 deletions fsw/src/cf_cmd.c
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,13 @@ static int CF_TsnChanAction(cf_cmd_transaction_t *cmd, const char *cmdstr, CF_Ts
static void CF_DoSuspRes_(transaction_t *t, susp_res_arg_t *context)
{
CF_Assert(t);
if (t->flags.all.suspended == context->action)
if (t->flags.com.suspended == context->action)
{
context->same = 1;
}
else
{
t->flags.all.suspended = context->action;
t->flags.com.suspended = context->action;
}
}

Expand Down
2 changes: 1 addition & 1 deletion fsw/src/cf_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ void CF_InsertSortPrio(transaction_t *t, cf_queue_index_t q)
{
CF_CList_InsertBack_Ex(c, q, &t->cl_node);
}
t->flags.all.q_index = q;
t->flags.com.q_index = q;
}

/************************************************************************/
Expand Down
16 changes: 8 additions & 8 deletions fsw/src/cf_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,20 @@
static inline void cf_dequeue_transaction(transaction_t *t)
{
CF_Assert(t && (t->chan_num < CF_NUM_CHANNELS));
CF_CList_Remove(&CF_AppData.engine.channels[t->chan_num].qs[t->flags.all.q_index], &t->cl_node);
CF_Assert(CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.all.q_index]); /* sanity check */
--CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.all.q_index];
CF_CList_Remove(&CF_AppData.engine.channels[t->chan_num].qs[t->flags.com.q_index], &t->cl_node);
CF_Assert(CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.com.q_index]); /* sanity check */
--CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.com.q_index];
}

static inline void cf_move_transaction(transaction_t *t, cf_queue_index_t q)
{
CF_Assert(t && (t->chan_num < CF_NUM_CHANNELS));
CF_CList_Remove(&CF_AppData.engine.channels[t->chan_num].qs[t->flags.all.q_index], &t->cl_node);
CF_Assert(CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.all.q_index]); /* sanity check */
--CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.all.q_index];
CF_CList_Remove(&CF_AppData.engine.channels[t->chan_num].qs[t->flags.com.q_index], &t->cl_node);
CF_Assert(CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.com.q_index]); /* sanity check */
--CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.com.q_index];
CF_CList_InsertBack(&CF_AppData.engine.channels[t->chan_num].qs[q], &t->cl_node);
t->flags.all.q_index = q;
++CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.all.q_index];
t->flags.com.q_index = q;
++CF_AppData.hk.channel_hk[t->chan_num].q_size[t->flags.com.q_index];
}

static inline void CF_CList_Remove_Ex(channel_t *c, cf_queue_index_t index, clist_node node)
Expand Down
Loading

0 comments on commit 5cece50

Please sign in to comment.