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

CF should use larger cf_entity_id_t type as default #62

Closed
jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #159
Closed

CF should use larger cf_entity_id_t type as default #62

jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #159
Milestone

Comments

@jphickey
Copy link
Contributor

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1799] CF should use larger cf_entity_id_t type as default
Originally submitted by: Hickey, Joseph P. (GSFC-582.0)[VANTAGE SYSTEMS INC] on Wed Nov 17 10:10:01 2021

Original Description:
In the default configuration, the cf_entity_id_t is defined as a uint8. While this is permissible per the spec, it means none of the "variable size" features implemented in the code will ever be used, because the size is always 1.

In particular the "CF_GetMemcpySize" function will only ever be called with a size of 1, which basically skips the loops in here.

Although the test do call it with a bigger sizes (200!!?!) it does not seem to actually check the intended purpose of this routine, and the tests only appear to be included when ENDIAN==_EL.

@jphickey
Copy link
Contributor Author

I think this one is worth changing the default entity ID size from 8 to 32 bits.

There are a couple reasons:

  • This not just affects what CF sends, but also what it is capable of receiving.
  • 8 bit addressing scheme is pretty limiting ... probably real world deployments are going to need a bigger address space
  • Since the entity IDs also appear in some commands, changing from 8->32 is not as trivial as just updating a platform config and rebuilding, it will change the binary format of some commands due to the larger size.

Because it is not simply a platform config, but also affects commands - this is why it's better to err on the side of using the larger size. If the user only really needs 8 bits, of addressing, but the config is set to 32 bits, everything should still work, it only costs a bit of memory (and a fairly trivial amount, really). And users who care about that can do the work to reduce it back to 8.. But if the user needs >8 bits this can cause some pretty confusing issues, particularly on the receive side if the sender happens to encode the entity IDs in >8 bits and CF rejects it.

jphickey added a commit to jphickey/CF that referenced this issue Jan 10, 2022
Entity IDs should be larger by default for real-world applicability, as this also
restricts what CFDP can receive, not just what it sends.  Note CFDP will only use
the number of bytes required to express the value, so values less than 256 will
still only use 1 byte, regardless of this config.  This just allows use of larger
values.
jphickey added a commit to jphickey/CF that referenced this issue Jan 10, 2022
Entity IDs should be larger by default for real-world applicability, as this also
restricts what CFDP can receive, not just what it sends.  Note CFDP will only use
the number of bytes required to express the value, so values less than 256 will
still only use 1 byte, regardless of this config.  This just allows use of larger
values.
astrogeco added a commit that referenced this issue Jan 12, 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