nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
79 stars 45 forks source link

Odd union for ack/nak counters #113

Closed jphickey closed 2 years ago

jphickey commented 2 years ago

Union is declared here: https://github.com/nasa/CF/blob/a894069439316ef89ad7751f3a03036930158a07/fsw/src/cf_cfdp.h#L137-L141

This counter is then used in the TxS2/RxS2 state data structures.

It should not be necessary to create a union between the unsigned and uint8 types for two reasons:

In short, declaring a union like this has no benefit at all, but only introduces the possibility of accessing it wrong and getting undefined behavior. Only downsides, no upside.

Recommendation is to replace with a normal uint32 counter.

jphickey commented 2 years ago

Upon further inspection, it looks like both the ack and nak states are limited by ack_limit and nak_limit, respectively, in the configuration table. Both limits are uint8 types. So the counter can be uint8, not uint32 (and since these are in the transaction structure, of which there are many instances, this saves a bit of memory).