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

Instantiating globals in header files (FSW version) #94

Closed
jphickey opened this issue Dec 6, 2021 · 2 comments · Fixed by #182
Closed

Instantiating globals in header files (FSW version) #94

jphickey opened this issue Dec 6, 2021 · 2 comments · Fixed by #182
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Dec 6, 2021

The FSW has a macro called "DECLARE_FIELD" which creates a constant at global scope:

CF/fsw/src/cf_field.h

Lines 54 to 55 in 7b99b91

#define DECLARE_FIELD(NAME, NBITS, SHIFT) \
static const CF_FIELD_FIELD NAME = {.shift = (SHIFT), .mask = ((1 << NBITS) - 1)};

The constant is scoped as "static" so it doesn't create a linker error, but it still creates a separate instance of this global variable for each time the header is included.

Confirmed by checking cf.so and observing that each of these 8 fields occur in the binary file 8 times:

CF/fsw/src/cf_cfdp_pdu.h

Lines 73 to 80 in 7b99b91

DECLARE_FIELD(PDU_HDR_FLAGS_VERSION, 3, 5)
DECLARE_FIELD(PDU_HDR_FLAGS_TYPE, 1, 4)
DECLARE_FIELD(PDU_HDR_FLAGS_DIR, 1, 3)
DECLARE_FIELD(PDU_HDR_FLAGS_MODE, 1, 2)
DECLARE_FIELD(PDU_HDR_FLAGS_CRC, 1, 1)
DECLARE_FIELD(PDU_HDR_FLAGS_RESERVED, 1, 0)
DECLARE_FIELD(PDU_LENGTHS_ENTITY, 3, 4)
DECLARE_FIELD(PDU_LENGTHS_TRANSACTION_SEQUENCE, 3, 0)

@jphickey jphickey self-assigned this Dec 13, 2021
@jphickey jphickey added the bug label Dec 13, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Dec 13, 2021

Calling this a "bug" because space efficiency is one of the core requirements/expectations of this CF implementation, and this is not meeting that expectation. Plus, instantiating values from a header file is against most/all standards - it is not expected that simple inclusion of a header will cause extra memory to be allocated in the binary.

@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

This was partially addressed by #137, in that the DECLARE_FIELDS are only used in a single source file now, but it is not an ideal way to do it.

Better fix for this can be combined with #65, moving the field definitions into the codec code should achieve both.

jphickey added a commit to jphickey/CF that referenced this issue Jan 13, 2022
Nothing outside of the codec code should need to know about the bitfields
in the CFDP protocol definition.  Everything else uses logical values.

This moves all DECLARE_FIELD macros into cf_codec.c and removes cf_field.h.
astrogeco added a commit that referenced this issue Jan 18, 2022
@skliper skliper added this to the Draco milestone Mar 24, 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