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.

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

Odd union for ack/nak counters #113

Closed
jphickey opened this issue Dec 10, 2021 · 1 comment · Fixed by #115
Closed

Odd union for ack/nak counters #113

jphickey opened this issue Dec 10, 2021 · 1 comment · Fixed by #115
Milestone

Comments

@jphickey
Copy link
Contributor

Union is declared here:

CF/fsw/src/cf_cfdp.h

Lines 137 to 141 in a894069

typedef union CF_RxTxCounters
{
unsigned ack;
uint8 nak;
} CF_RxTxCounters_t;

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:

  • unsigned is not a known/guaranteed range type, and the rollover point of this value is not guaranteed. FSW discourages use of types which do not have fixed range unless for valid reasons (e.g. interfacing with library code that uses this type)
  • uint8 is simply the 8 LSBs of the value, there is no need to unionize in order to be able to count both modulo 256 as well as a longer type

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
Copy link
Contributor Author

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).

jphickey added a commit to jphickey/CF that referenced this issue Dec 10, 2021
Using a union for the ack/nak counts is somewhat dangerous because
undefined behavior will occur if accessed improperly.  In this case
there is no need to have separate representations of the counter,
they are both limited to the "unit8" range, so use a uint8.
jphickey added a commit to jphickey/CF that referenced this issue Dec 16, 2021
The use of random numbers in a couple test cases was causing an
unpredictable flow through the code.  This replaces with fixed
values so get consistent flows.

Specifically, the "acknak_count" should always be less than the
"ack_limit".  The UT use of random numbers allowed it to be
greater.  However, if it is greater than the limit, it should
still follow the limit/error path, not the successful path.
astrogeco added a commit that referenced this issue Dec 17, 2021
@skliper skliper added this to the Draco milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants