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 #61, remove dependence on ENDIAN macro for checksum #160

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Removes the checksum "optimization" for big endian, as it is unlikely to be useful and creates endianness dependencies.

Fixes #61

Testing performed
Build and sanity check CF

Expected behavior changes
None on x86/little endian (removes code that was not used)
PPC/big endian will use the same code as x86, might be a bit slower but its safer and more testable (because its the same code)

System(s) tested on
Ubuntu 21.10

Additional context
The big endian optimization is probably not that useful because it requires that the HW can do misaligned 32-bit reads. AFAIK most embedded CPUs do not have this capability, and if they can, it might not be all that much faster than reading it byte-by-byte. So even though it looks like a 32-bit read in C code (and therefore 4x fewer loop iterations) it probably doesn't have much of a payoff, if one could find a hardware with this capability (not sure how it was originally tested). In the end I don't think its worth trying to test and maintain this "optimization".

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

Removes the "optimization" for big endian, unlikely to be useful.
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree 100% this is a critical improvement.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jan 11, 2022
@astrogeco astrogeco merged commit 0b5ce38 into nasa:main Jan 12, 2022
@jphickey jphickey deleted the fix-61-endian branch January 12, 2022 16:52
@astrogeco astrogeco added CCB:Approved and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Jan 19, 2022
@astrogeco
Copy link
Contributor

astrogeco commented Jan 19, 2022

CCB:2022-01-19 APPROVED

@skliper skliper added this to the Draco milestone Mar 25, 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.

CF should not depend on ENDIAN macro (_EL/_EB)
3 participants