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

Function pointers should be typedef'ed #111

Closed
jphickey opened this issue Dec 9, 2021 · 0 comments · Fixed by #135
Closed

Function pointers should be typedef'ed #111

jphickey opened this issue Dec 9, 2021 · 0 comments · Fixed by #135
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Dec 9, 2021

CF uses function pointers to implement its state machines, but generally do not use a "typedef" for this, they are mostly declared inline. For example:

CF/fsw/src/cf_cfdp.c

Lines 153 to 154 in a894069

static void (*const state_fns[])(CF_Transaction_t *) = {CF_CFDP_RecvIdle, CF_CFDP_R1_Recv, CF_CFDP_S1_Recv,
CF_CFDP_R2_Recv, CF_CFDP_S2_Recv, CF_CFDP_RecvDrop};

static void (*const state_fns[])(CF_Transaction_t *) = {NULL, NULL, CF_CFDP_S1_Tx, NULL, CF_CFDP_S2_Tx, NULL};

Not only is this hard to read, it does not facilitate or encourage any sort of uniformity/consistency in the dynamically-called functions. No doubt this is likely a contributor to the fact that some functions take a pointer to the PDU header and some do not (see #90, #91).

Recommendation to fix:

  • Determine a common set of arguments that all "state handler" functions are likely to need - from initial inspection, this is probably a pointer to the transaction structure, a pointer to the current PDU header, and a generic/opaque argument for any additional data (this may or may not be needed/used now, but future proofs the calling conventions in case state-specific data becomes needed)
  • Declare a global-scope function pointer typedef that conforms to that spec (accepting the standard set of args)
  • Convert all "dispatcher" code to use that typedef.

This will not only make the code more readable (function pointer syntax in C is particularly messy) but also encourage more uniformity on the arguments and patterns of state handler functions. It will likely help solve the fact that some functions read their packet data from a global, while others read it from a passed-in pointer (and mixed within the same processing cycle!).

jphickey added a commit to jphickey/CF that referenced this issue Dec 17, 2021
Standardize the dynamic handler functions to two basic types,
one that accepts a PDU (recv) and one that does not (send).

Also create several dispatch table types, one based on
file directive code, one based on Tx sub state, and one based
on Rx sub state.

Change the dispatcher functions to use these common types
and create new dispatcher functions where there was not
a separate function already (this makes the pattern consistent).

Make all "receive" helper functions accept a pointer to the
recieved PDU and actually use that pointer to read the data.  This
substantially reduces reliance on the global and fixes some
cases where a pointer was actually passed into a function, but
ignored.  This takes a significant step toward removing the
global entirely, but does not do so yet.
astrogeco added a commit that referenced this issue Dec 17, 2021
Fix #111, make dispatch tables and functions consistent
@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