-
Notifications
You must be signed in to change notification settings - Fork 202
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
Fix #797, refactor internal table/id management #859
Conversation
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? |
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
1994c51
to
7f0c08e
Compare
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 Externally the API uses the abstract AppID ( |
|
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:
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 ( Adding |
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. |
Copy, I think if it's all ES tasks then it's fine. |
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.