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

Fix #6, remove implicit padding #24

Merged
merged 2 commits into from
May 4, 2022
Merged

Fix #6, remove implicit padding #24

merged 2 commits into from
May 4, 2022

Conversation

dmknutsen
Copy link
Contributor

@dmknutsen dmknutsen commented Apr 26, 2022

Describe the contribution

Fix #6

Updated cs_tbledefs.h to correct for implicit padding that gets added when running on a 64-bit system.
Also updated default tables provided (cs_apptbl.c, cs_eepromtbl.c, cs_memorytbl.c, and cs_tablestbl.c) to remove positional dependency on table elements.

Testing performed
Steps taken to test the contribution:

  1. Started cFS
  2. Enabled Tlm
  3. Dumped each structure
  4. Interpreted binary dump data and confirmed expected padding

System(s) tested on
Oracle VM VirtualBox
OS: ubuntu-20.10
Versions: cFS Versions: cfe v7.0.0-rc4+dev96, osal v6.0.0-rc4+dev66, psp v1.6.0-rc1+dev14.

Contributor Info - All information REQUIRED for consideration of pull request
Dan Knutsen
NASA Goddard

@astrogeco
Copy link
Contributor

@dmknutsen can you update the commit message to match the contributing standard?

@dmknutsen dmknutsen changed the title CS_Issue_6 Fix #6, remove implicit padding Apr 26, 2022
@astrogeco
Copy link
Contributor

CCB:2022-04-27 APPROVED

  • cpuaddress type will "change size". Use cfe_es_memaddress and cfe_es_offset types for tables and telemetry
    • open new issue to address and discuss with customers

@skliper
Copy link
Contributor

skliper commented Apr 28, 2022

I also suggest improving the commit message to provide a little more context. At minimum maybe something like Fix #6, Remove implicit padding in table definitions. Goal is for the reader to be able to skim through the commit messages and get a general sense of what the change was. LONG DESCRIPTION is also frequently helpful (could add context about how it was introduced with 64 bit support, etc), but I understand everyone is busy :)

Note if you are going with the ES definitions, I recommend considering resolving/addressing nasa/cFE#2093. At minimum make it easier to config for 64, but even better if the default would always provide valid address information.

@astrogeco
Copy link
Contributor

Fixed format and commit messages in my fork, I can force push those into this PR. See https://github.com/astrogeco/CS/tree/pr24

@skliper
Copy link
Contributor

skliper commented May 2, 2022

@astrogeco Could you add in the short description that it's specifically the table structures?

Correct structure definitions to minimize implicit padding added when
running on 64-bit systems.

Update default tables provided to remove positional dependency on table elements.
@astrogeco
Copy link
Contributor

Just force-pushed to @dmknutsen fork

@astrogeco astrogeco merged commit d076920 into nasa:main May 4, 2022
@skliper skliper added this to the Draco milestone Jul 11, 2022
skliper added a commit to skliper/CS that referenced this pull request Sep 9, 2022
skliper pushed a commit to skliper/CS that referenced this pull request Sep 9, 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 this pull request may close these issues.

CS: Implicit padding being added to definition and results tables
3 participants