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 #797, refactor internal table/id management #859

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Sep 2, 2020

Describe the contribution
Introduce wrapper/accessor functions to look up table entries by ID for ES & EVS subsystems.

Do not use AppID as a table index.

Note - This does not change existing external APIs and AppIDs are still zero-based uint32. This only changes the internal
structures to remove use of ID as an array index, and to use a lookup function to locate the table entry from an ID. All entry access is then performed via the table entry pointer, rather than as an array index.

This provides the groundwork for abstract IDs without actually changing anything fundamental about resource IDs.

Fixes #797

Testing performed
Build and sanity check CFE - start up apps and send commands, confirm normal operation
Run all unit tests.

Expected behavior changes
No impact to behavior - internal refactoring only.
API additions to support more abstract resource IDs.

System(s) tested on
Ubuntu 20.04

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

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Sep 2, 2020
@astrogeco
Copy link
Contributor

Would it be easier to review this by splitting it up into a couple of separate commits?

@jphickey
Copy link
Contributor Author

jphickey commented Sep 2, 2020

Would it be easier to review this by splitting it up into a couple of separate commits?

I could probably split into a separate commit for each subsystem (ES, EVS, SB, etc) if that helps?

@astrogeco
Copy link
Contributor

That might be helpful, I like atomized commits as much as possible. The subsystem approach makes sense

Introduce wrapper/accessor functions to look up table
entries by ID for Executive Services subsystem.

__Do not use AppID as a table index__.

Note - This does not change existing external APIs and AppIDs
are still zero-based uint32.  This only changes the internal
structures to remove use of ID as an array index, and to use a lookup
function to locate the table entry from an ID.  All entry access
is then performed via the table entry pointer, rather than
as an array index.

This provides the groundwork for abstract IDs without actually
changing anything fundamental about resource IDs.
Update the EVS subsystem to follow the ES pattern for
internal table management.

Do not use AppID directly as a table index.  Instead,
use a separate lookup routine to get a pointer to the
entry, then access the entry via the pointer.

Also introduce inline helper functions to get/set
status of entry (free/not free), etc.
Minor fixup for use of IDs when logging
Change SB to get its task ID from ES rather than getting
it directly from OSAL.  Update syslog calls to use the
ES-supplied conversion to integer rather than direct cast.
Update TBL to use the new ES-supplied ID manipulations.  Update
all syslog calls to convert to integer using ES wrapper.
Update the TIME subsystem to use the new ES-supplied ID
abstractions.  Do not use AppID directly as array index
when registering sync callbacks, use the ES-supplied
conversion to array index before accessing local table.

Also update logging to use ES-supplied conversion
@jphickey
Copy link
Contributor Author

jphickey commented Sep 2, 2020

OK - separate commits for each subsystem. ES is still the biggest one as its where the bulk of changes lie.

Note that most changes are mechanical -- that is, changing something like CFE_ES_Global.AppTable[AppID].something into AppRecPtr->something instead. The idea here is that all this internal stuff needs to be cleaned up first which shouldn't change anything other than make it consistently apply the lookup function to get/change internal data.

Externally the API uses the abstract AppID (uint32) but internally (after validation of said AppID) we can pass around a pointer to the entry instead, avoiding repeat lookups and clean up the code substantially.

@astrogeco astrogeco added CCB:Splinter Item needs a focused splinter meeting CCB-20200902 unit-test and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 2, 2020
@astrogeco
Copy link
Contributor

CCB 2020-09-02

  • Expand comments to note the justification of why AppID does not need to be re-validated internally in ES
  • Note substantial changes to Unit Tests
  • New function CFE_ES_GetTaskID() usage versus OS_TaskGetId() warrants more discussion due to naming and scoping. Need to find a new name for task.

@astrogeco
Copy link
Contributor

astrogeco commented Sep 2, 2020

@jphickey should we convert this to draft while working on the rename of CFE_ES_GetTaskID?

I suppose we can do that, but I hope this doesn't become a long drawn-out thing .... The more I think about it since the CCB the more I think that changing the name from "task" would only add confusion since we have many existing (public) APIs that refer to "task" in their name:

  • CFE_ES_GetTaskInfo
  • CFE_ES_CreateChildTask
  • CFE_ES_RegisterChildTask
  • CFE_ES_ExitChildTask
  • CFE_ES_DeleteChildTask
  • CFE_ES_IncrementTaskCounter

Changing the name of all of these would be a huge backward compatibility issue. And these are all referring to ES-scope tasks (because they are in CFE ES) and they are largely wrappers around OSAL with extra ES sauce on top.

The fact that apps went direct to OSAL for the one missing ES task routine (OS_TaskGetID()) really just seems more odd when you think about.

Adding CFE_ES_GetTaskID() that mirrors the existing CFE_ES_GetAppID() just completes the ES task API. All task IDs at this level (and beyond) are ES tasks.

@jphickey
Copy link
Contributor Author

jphickey commented Sep 2, 2020

In regards to the original concern about differentiating OSAL tasks from ES tasks -- the more I think about this, I don't think we really need to do this via a different name, because these two should really never be mixed. The fact that they are mixed now is actually the problem that this PR is fixing.

Moving forward -- all tasks at the CFE level and above should be ES tasks, and all management of those tasks should happen though ES APIs. There really shouldn't be any areas where OSAL task IDs are intermixed with ES task IDs. They should all be ES task IDs - EXCEPT, of course, in OSAL itself, and the ES APIs that actually call OSAL.

@yammajamma yammajamma changed the base branch from main to integration-candidate September 2, 2020 19:05
@yammajamma yammajamma added CCB:Approved Indicates code review and approval by community CCB IC-20200902 labels Sep 2, 2020
@skliper
Copy link
Contributor

skliper commented Sep 2, 2020

Copy, I think if it's all ES tasks then it's fine.

@astrogeco astrogeco merged commit a40ce45 into nasa:integration-candidate Sep 4, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 4, 2020
@astrogeco astrogeco removed the CCB:Splinter Item needs a focused splinter meeting label Sep 9, 2020
@jphickey jphickey deleted the fix-797-internal-tbl-mgmt branch September 29, 2020 21:52
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB unit-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve resource management and internal consistency in ES
4 participants