Fix #943, #924 and #502, improve resource ID management #946

jphickey · 2020-10-13T20:29:03Z

Describe the contribution

Fixes #924
Fixes #943
Fixes #502

Provides a consistent Name-ID translation API for all resource types. Enforces consistent argument/name validation on all resource types, and also enforces name uniqueness where relevant.

Also includes an enhancement to use the full 16 bits of resource ID space, which avoids repeating ID values that have already been used. This significantly decreases the likelihood that a previously deleted ID will alias a newly allocated/valid ID.

Testing performed
Build and sanity test CFE
Ensure all unit tests pass
ALSO - run a continuous loop of "Restart" commands, to continually restart the SAMPLE_APP. Confirmed that after each restart, the app is assigned a new id, different than the old ID, and never aliases any other valid IDs.

Expected behavior changes

Complete/consistent API to translate between names and IDs, for all resource types which have a name
IDs are not re-issued after deletion, helping ensure safety as previously deleted IDs will not validate.

System(s) tested on
Ubuntu 20.04

Additional context
These two fixes are combined into a single PR as they have dependencies (would be a merge conflict if pulled separately)

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

Implement consistent conversion/lookup functions to translate between names and IDs for all resource types. Also reorganize all internal resource ID functions into a separate source file for better organization. Clean up documentation and references.

Use the entire resource ID space (16 bits) and do not immediately recycle ID values until the full space is used. Implement a generic routine to find the next available ID, and modify every resource ID allocation to use this routine. Where necessary, this also corrects for differences in argument checking and duplicate checking between the various resource types. All resource allocation procedures now have consistent argument checking, and will reject duplicate name creation for any resources that are also associated with names.

jphickey · 2020-10-14T14:43:04Z

Also adding a commit that fixes #502.
Again due to dependencies this has to be serialized, can't be a separate PR or merge conflicts will result.

There is no need to store the main task name a second time in the app record, as it is already stored in the task record.

skliper · 2020-10-14T17:03:42Z

CCB: 2020-10-14 - Approved, pending @acudmore review.

fsw/cfe-core/src/es/cfe_es_resource.c

ghost

Looks good. The only thought I had was perhaps the resource ID functions could be generalized.. The Get ID functions repeat for each object type in ES, but I don't know if it's worth another layer of ID abstraction at this point. (Generic ES ID -> Library ID, etc ). Regardless, it's better than it was before, and solves problems associated with deleting and re-creating a resource. I also like the error code improvements.

astrogeco · 2020-10-19T20:08:16Z

@jphickey can we resolve Alan's comments or do you need to make some tweaks?

Add a NULL check to CFE_ES_ResourceID_ToIndex_Internal(). Although this function is internal, it does check arguments on behalf of the public APIs which call this.

jphickey · 2020-10-19T21:15:28Z

@jphickey can we resolve Alan's comments or do you need to make some tweaks?

@astrogeco The update in commit 4356693 should be all we need.

jphickey · 2020-10-19T21:23:28Z

Looks good. The only thought I had was perhaps the resource ID functions could be generalized.. The Get ID functions repeat for each object type in ES, but I don't know if it's worth another layer of ID abstraction at this point.

Yes, I concur there is some repetition here. I had considered splitting the structures similar to OSAL - where the name/ID info is in one part (common to all) and the type-specific info is in a separate entry.

But - This would have been an even bigger change to the codebase, and the truth is that most of the time the user intends to operate on a specific resource type. So even if we did give e.g. a "generic" name lookup API, the user would probably have to follow it up with a type check in many cases. So I decided it probably wasn't worth it.

ghost · 2020-10-20T13:32:23Z

Looks good. The only thought I had was perhaps the resource ID functions could be generalized.. The Get ID functions repeat for each object type in ES, but I don't know if it's worth another layer of ID abstraction at this point.

Yes, I concur there is some repetition here. I had considered splitting the structures similar to OSAL - where the name/ID info is in one part (common to all) and the type-specific info is in a separate entry.

But - This would have been an even bigger change to the codebase, and the truth is that most of the time the user intends to operate on a specific resource type. So even if we did give e.g. a "generic" name lookup API, the user would probably have to follow it up with a type check in many cases. So I decided it probably wasn't worth it.

Another thought I had, but probably for a future round of updates.. your comments mentioned taking advantage of the OSAL resource ID structures while they are compatible, but no guarantee that it will stay that way. Maybe on a future update, the OSAL could provide a resource ID API to create its own resource IDs and allow cFE services such as ES to create and use Resource IDs, unifying the functions around creating, lookup, etc.
But I'm happy to leave that for now, this is good to me.

jphickey · 2020-10-20T15:44:18Z

Yeah, I think the correct answer longer-term is to decouple the OSAL task ID from the CFE task ID. That way each subsystem can manage their own concept of an "ID" independently. It's easy to do on the CFE side - just have to translate the ID, which is already being done as a placeholder/passthru.

It is only a pass through because code expects to be able to take the output of e.g. CFE_ES_CreateChildTask() and pass that to OSAL functions, and likewise take output of e.g. OS_TaskGetId() and pass that to CFE functions. So to support this in a backward-compatible manner we are keeping all IDs as uint32 and making this work as it historically has.

The reason I added wrapper functions like CFE_ES_GetTaskID() is to start laying the groundwork to breaking this assumption. Once this is established then CFE can allocate its own IDs independently of the OSAL IDs and they don't need to worry about keeping the bit fields compatible.

For backward compatibility - remove logic that actually made thes different numerical values. This should be put back in at some point.

jphickey · 2020-10-20T15:50:39Z

Update - writing the comment above made me realize that a bit of debug test logic was left in the final commit.

I had been specifically testing the case were OSAL task IDs and CFE task IDs were not numerically equivalent in order to confirm that all the abstractions were working (which they are) but this was still left in the final commit.

See Commit 0790e11 which puts this back to its backward-compatible form. Please include this in the IC.

astrogeco · 2020-10-20T15:54:05Z

Update - writing the comment above made me realize that a bit of debug test logic was left in the final commit.

I had been specifically testing the case were OSAL task IDs and CFE task IDs were not numerically equivalent in order to confirm that all the abstractions were working (which they are) but this was still left in the final commit.

See Commit 0790e11 which puts this back to its backward-compatible form. Please include this in the IC.

One last check to make sure. Can I merge this now?

* Add nasa/PSP#207 * Add nasa/cFE#946 * Update CFE to fix startup issue * Add nasa/osal#626 Co-authored-by: Joseph Hickey <joseph.p.hickey@nasa.gov>

thnkslprpt · 2023-05-06T01:16:37Z

fsw/cfe-core/src/es/cfe_es_cds.c

        }

-        if (IsNewOffset || IsNewEntry)


jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Oct 13, 2020

jphickey force-pushed the fix-924-unique-id branch from adb942b to 5d8f279 Compare October 13, 2020 20:52

jphickey added 2 commits October 13, 2020 17:00

jphickey force-pushed the fix-924-unique-id branch from 5d8f279 to 787f3a8 Compare October 13, 2020 21:01

astrogeco added the CCB-20201014 label Oct 14, 2020

jphickey changed the title ~~Fix #943 and Fix #924, improve resource ID management~~ Fix #943, #924 and #502, improve resource ID management Oct 14, 2020

jphickey linked an issue Oct 14, 2020 that may be closed by this pull request

CFE ES unnecessarily keeping multiple copies of task/app names #502

Closed

CDKnightNASA approved these changes Oct 14, 2020

View reviewed changes

Fix nasa#502, remove extra copy of task name

56cd634

There is no need to store the main task name a second time in the app record, as it is already stored in the task record.

jphickey force-pushed the fix-924-unique-id branch from 9a31c26 to 56cd634 Compare October 14, 2020 15:27

skliper requested a review from a user October 14, 2020 16:23

astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Oct 15, 2020

ghost reviewed Oct 19, 2020

View reviewed changes

fsw/cfe-core/src/es/cfe_es_resource.c Show resolved Hide resolved

ghost reviewed Oct 19, 2020

View reviewed changes

fsw/cfe-core/src/es/cfe_es_resource.c Show resolved Hide resolved

ghost reviewed Oct 19, 2020

View reviewed changes

fsw/cfe-core/src/es/cfe_es_resource.c Show resolved Hide resolved

ghost approved these changes Oct 19, 2020

View reviewed changes

astrogeco added the IC-20201020 label Oct 19, 2020

astrogeco changed the base branch from main to integration-candidate October 19, 2020 20:07

Update nasa#943, add NULL check for Idx parameter

4356693

Add a NULL check to CFE_ES_ResourceID_ToIndex_Internal(). Although this function is internal, it does check arguments on behalf of the public APIs which call this.

Update nasa#943, Make CFE and OSAL task ID equivalent

0790e11

For backward compatibility - remove logic that actually made thes different numerical values. This should be put back in at some point.

astrogeco merged commit c51ba03 into nasa:integration-candidate Oct 21, 2020

astrogeco added a commit to nasa/cFS that referenced this pull request Oct 21, 2020

Add nasa/cFE#946

d6f3197

astrogeco mentioned this pull request Oct 21, 2020

Integration Candidate: 2020-10-20 #959

Merged

astrogeco added a commit to nasa/cFS that referenced this pull request Oct 21, 2020

Integration Candidate: 2020-10-20 (#152)

3a76845

* Add nasa/PSP#207 * Add nasa/cFE#946 * Update CFE to fix startup issue * Add nasa/osal#626 Co-authored-by: Joseph Hickey <joseph.p.hickey@nasa.gov>

jphickey deleted the fix-924-unique-id branch December 3, 2020 17:53

skliper added this to the 7.0.0 milestone Sep 24, 2021

thnkslprpt reviewed May 6, 2023

View reviewed changes

fsw/cfe-core/src/es/cfe_es_cds.c

}

if (IsNewOffset || IsNewEntry)

Copy link

Contributor

thnkslprpt May 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link #1929

This was referenced May 6, 2023

ES missing branch in CFE_ES_RegisterCDSEx, overwrite success check doesn't make sense #1929

Open

Fix #1929, Remove redundant status check in CFE_ES_RegisterCDSEx() #2329

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #943, #924 and #502, improve resource ID management #946

Fix #943, #924 and #502, improve resource ID management #946

jphickey commented Oct 13, 2020 •

edited by skliper

Loading

jphickey commented Oct 14, 2020

skliper commented Oct 14, 2020 •

edited by astrogeco

Loading

ghost left a comment

astrogeco commented Oct 19, 2020

jphickey commented Oct 19, 2020

jphickey commented Oct 19, 2020

ghost commented Oct 20, 2020

jphickey commented Oct 20, 2020

jphickey commented Oct 20, 2020

astrogeco commented Oct 20, 2020

thnkslprpt May 6, 2023

Fix #943, #924 and #502, improve resource ID management #946

Fix #943, #924 and #502, improve resource ID management #946

Conversation

jphickey commented Oct 13, 2020 • edited by skliper Loading

jphickey commented Oct 14, 2020

skliper commented Oct 14, 2020 • edited by astrogeco Loading

ghost left a comment

Choose a reason for hiding this comment

astrogeco commented Oct 19, 2020

jphickey commented Oct 19, 2020

jphickey commented Oct 19, 2020

ghost commented Oct 20, 2020

jphickey commented Oct 20, 2020

jphickey commented Oct 20, 2020

astrogeco commented Oct 20, 2020

thnkslprpt May 6, 2023

Choose a reason for hiding this comment

jphickey commented Oct 13, 2020 •

edited by skliper

Loading

skliper commented Oct 14, 2020 •

edited by astrogeco

Loading