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

CFE_ES_RegisterCDSEx: clear block if new allocation is needed but not new block size #1337

Closed
skliper opened this issue Apr 13, 2021 · 1 comment · Fixed by #1376 or #1406
Closed
Assignees
Labels
CFS-40 docs This change only affects documentation. enhancement
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Apr 13, 2021

Is your feature request related to a problem? Please describe.
May need to clear block if a new allocation is needed without a changed block size

if (Status == CFE_SUCCESS && RegRecPtr->BlockOffset != 0 && NewBlockSize != RegRecPtr->BlockSize)

Describe the solution you'd like
Analyze, clear block if needed

Describe alternatives you've considered
If not needed, document why for future reference

Additional context
Code review

Requester Info
Jacob Hageman - NASA/GSFC

@jphickey
Copy link
Contributor

jphickey commented Apr 19, 2021

Looked into this code, its one of those areas where the original design appears to have evolved in a certain way, and so the "classic" behavior from old CFE versions was preserved.

I went back to the requirements to see what it says, and only these three:

cES1315 Upon receipt of a Request, the cFE shall reserve the Request specified amount of memory in the Critical Data Store for the cFE Application using the Request specified name.
cES1315.1 If a Critical Data Store exists for the Request specified name but has a different size than what is specified in the Request, the cFE shall remove the existing Critical Data Store and create a new one using the Request specified name and size.
cES1315.2 If a Critical Data Store exists for the Request specified name but the Data Integrity value is invalid, the cFE shall remove the existing Critical Data Store and create a new one using the Request specified name and size.

But none of these requirements say anything about "pre-clearing" the CDS block, and historically the code has not done this in either case (i.e. for new allocations or size changes). I can't say for sure why not, but most likely for these reasons:

  • CDS access might be slow (e.g. flash/eeprom memory, possibly indirectly accessed through an of intermediate serial link) so zeroing it out could take a fair bit of time.
  • CDS memory might have a limited number of write cycles and zeroing it out might reduce its lifespan
  • CDS blocks are protected by a CRC-32. So in the event that a user attempted to read a CDS that had never been written to, it would fail the CRC check and the user would get CFE_ES_CDS_BLOCK_CRC_ERR. Notably this is dependent on the size of the block too, so if the block size were to change, even if it had a previous content with a good CRC, the CRC check would likely fail for the new/different block size.

My recommendation is to leave this as is, for the reasons above. Changing behavior is likely to have unintended side effects.

jphickey added a commit to jphickey/cFE that referenced this issue Apr 19, 2021
Note in the documentation for this function that it is the responsibility
of the calling app to clear/fill the CDS block any time a new block is
allocated.
@astrogeco astrogeco added the docs This change only affects documentation. label Apr 22, 2021
astrogeco added a commit that referenced this issue Apr 22, 2021
Fix #1337, add docs to CFE_ES_RegisterCDS() regarding clearing
zanzaben pushed a commit to zanzaben/cFE that referenced this issue Apr 22, 2021
Note in the documentation for this function that it is the responsibility
of the calling app to clear/fill the CDS block any time a new block is
allocated.
zanzaben pushed a commit to zanzaben/cFE that referenced this issue Apr 22, 2021
Fix nasa#1337, add docs to CFE_ES_RegisterCDS() regarding clearing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CFS-40 docs This change only affects documentation. enhancement
Projects
None yet
3 participants