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 #1342, rename "Zero" union member field #1369

Closed
wants to merge 1 commit into from

Conversation

jphickey
Copy link
Contributor

Describe the contribution
This field is just some generic bits. It is used when the CDS is cleared by zero-filling this data block and writing it to CDS repeatedly in a loop.

Renaming it to "GenericDataBlock" should be clearer as to its intent.

Fixes #1342

Testing performed
Build and sanity check CFE
Confirm all unit tests pass

Expected behavior changes
None

System(s) tested on
Ubuntu 20.04

Additional context
Just renames internal field to avoid confusion about the name Zero.

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

This field is just some generic bits. It is used when
the CDS is cleared by zero-filling this data block and
writing it to CDS repeatedly in a loop.

Renaming it to "GenericDataBlock" should be clearer as
to its intent.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 16, 2021
@skliper
Copy link
Contributor

skliper commented Apr 20, 2021

I still don't really follow as to why a clear wouldn't make more sense implemented by the PSP, or why it's sized as it is from the cFE level. Seems like the technique used to clear should be based on the hardware properties (page size, etc). Maybe we should convert this to a future work issue to do a more in-depth refactor? Clearing in arbitrary uint32[4] chunks just doesn't make sense to me whatever the variable is called.

@jphickey
Copy link
Contributor Author

Yes, I completely agree that if the CDS is implemented by e.g. some flash memory sectors or something like that, writing zeros is a pretty terrible way to clear it. In fact the current design/assumptions of CDS might exclude certain classes of nonvolatile memory tech from being used to implement it.

The objective of this PR was not to improve the design, just to clarify why that union field was named "zero". This union field is just so ES can fill it with zeros, and this way it can at least write 16 bytes of zeros at a time instead of just 4 or 1. This is the same way that CDS has always been cleared in previous versions of CFE.

I totally agree that the CDS access interface needs improvement, but it shouldn't be done in a bugfix context, someone should really take a look at the various types of physical memory devices that might be used to implement it, and design a proper abstraction API to better support them. At a minimum, it likely needs a mechanism to align the CDS blocks to match the erase blocks of the underlying mem tech. Right now it seems to assume random read/write access is possible, which is not always the case.

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB CFS-40 labels Apr 21, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Apr 21, 2021

CCB:2021-04-21 CONVERT FUTURE WORK

  • Is it worth fixing this because it is "so broken:?
  • This highlights that we need to move this to PSP
  • One reason to keep this is because it forces developers to think about how the memory is architected

@astrogeco astrogeco removed the CCB:Approved Indicates code review and approval by community CCB label Apr 21, 2021
@skliper
Copy link
Contributor

skliper commented Apr 21, 2021

See #1404

@skliper skliper added wontfix and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 21, 2021
@skliper skliper closed this Apr 21, 2021
@jphickey jphickey deleted the fix-1342-zero-field branch June 8, 2021 18:34
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.

Clarify use of Zero[4] in CFE_ES_CDS_AccessCacheData_t
3 participants