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 #894, add resource ID type #896

Merged

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Add a new typedef CFE_ES_ResourceID_t that can replace uint32 for all ID storage and manipulation. Initially this is just an alias to uint32 for backward compatibility.

Convert all APIs that accept an ID to use the new typedef.

This also reserves the value "0" as an undefined ID, and gives a separate base value for each resource type. Therefore even though the type is still fundamentally a uint32, the different resource IDs can still be distinguished.

Fixes #894

Testing performed
Build and run, sanity test CFE, ensure apps and libs are loaded OK
Spot-check various commands that operate on apps/appids by sending using cmdUtil. Confirmed commands that write to data files (e.g. QUERY_ALL) has the correct appIDs in the file (e.g. 0x02110000 instead of 0x00000000 for CFE_EVS).
Confirmed that commands that operate on app name (start/stop/restart etc) still work fine.

Expected behavior changes
IDs are no longer zero based.
This should not matter so long as the ID is treated as opaque, and only manipulated via the ES APIs. This matters if an app is using the ID as a table/array index. All instances of this usage in CFE itself has already been fixed, and initial review of CFS apps didn't show any concerns.

Otherwise, no change to behavior as long as IDs are treated as opaque and only stored/used per API.

System(s) tested on
Ubuntu 20.04

Additional context
An optional add-on can make the ID type into a struct so it triggers a compiler error if/when used as an integer. This can aid in updating apps that might still use uint32.

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

@jphickey jphickey force-pushed the fix-894-resourceid-typedef branch from a0a5819 to 7132452 Compare September 22, 2020 17:42
@jphickey
Copy link
Contributor Author

Update commit 7132452 just corrects a doxygen warning.

@jphickey jphickey requested a review from skliper September 22, 2020 17:43
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Sep 22, 2020
Add a new typedef "CFE_ES_ResourceID_t" that can
replace uint32 for all ID storage and manipulation.
Initially this is just an alias to uint32 for
backward compatibility.

Convert all APIs that accept an ID to use the new
typedef.

This also reserves the value "0" as an undefined ID,
and gives a separate base value for each resource type.
Therefore even though the type is still uint32, the
different resource IDs can still be distingushed.
@jphickey jphickey force-pushed the fix-894-resourceid-typedef branch from 7132452 to 96d82c5 Compare September 22, 2020 17:54
@jphickey
Copy link
Contributor Author

Update commit 96d82c5 fixes a cppcheck warning....

@CDKnightNASA
Copy link
Contributor

Personally I'd rather see more specific types, CFE_ES_AppID_t, CFE_ES_TaskID_t, etc. Could do that later, after this monumental change set is applied...

@astrogeco
Copy link
Contributor

CCB 2020-09-23 APPROVED

@jphickey
Copy link
Contributor Author

Personally I'd rather see more specific types, CFE_ES_AppID_t, CFE_ES_TaskID_t, etc. Could do that later, after this monumental change set is applied...

Right - by design this is also entirely possible if we want to do this. The API has separate category-specific functions so its easy to update these to have a separate ID type per category. But you get much of the same effect by using different bases, it just happens at runtime rather than compile time. And it wouldn't be possible to trigger compile time checking as long as we are still using uint32 ... until we transition to a type safe wrapper, there is no benefit to different types.

Still if we wanted to at least define separate types now, it could be a good idea.

@skliper
Copy link
Contributor

skliper commented Sep 23, 2020

Still if we wanted to at least define separate types now, it could be a good idea.

I like separate types, a step further in the right direction.

@jphickey
Copy link
Contributor Author

Still if we wanted to at least define separate types now, it could be a good idea.

I like separate types, a step further in the right direction.

No objection there -- do you want me to submit a separate PR for that or add a new commit on top of this one? My suggestion is a separate PR, and I can have it ready for next week CCB.

@CDKnightNASA
Copy link
Contributor

Personally I'd rather see more specific types, CFE_ES_AppID_t, CFE_ES_TaskID_t, etc. Could do that later, after this monumental change set is applied...

Right - by design this is also entirely possible if we want to do this. The API has separate category-specific functions so its easy to update these to have a separate ID type per category. But you get much of the same effect by using different bases, it just happens at runtime rather than compile time. And it wouldn't be possible to trigger compile time checking as long as we are still using uint32 ... until we transition to a type safe wrapper, there is no benefit to different types.

Still if we wanted to at least define separate types now, it could be a good idea.

Actually was thinking:

typedef uint32 CFE_ES_ResourceID_t;
typedef CFE_ES_ResourceID_t CFE_ES_AppID_t;
...

so you could switch all to a struct with one line change...And gives more explicit semantics that the "subtypes" are related.

@jphickey
Copy link
Contributor Author

FYI - submitted issue #913 to make the separate type per category enhancement. I can have this for next CCB but need to get this into the baseline first, so I don't want to hold this one up.

@yammajamma yammajamma added IC-20200929 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 29, 2020
@yammajamma yammajamma changed the base branch from main to integration-candidate September 29, 2020 15:07
@yammajamma yammajamma merged commit 82f1425 into nasa:integration-candidate Sep 29, 2020
@jphickey jphickey deleted the fix-894-resourceid-typedef 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add typedef and nonzero base for resource identifiers
5 participants