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 #943, #924 and #502, improve resource ID management #946

Merged
merged 5 commits into from
Oct 21, 2020

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Oct 13, 2020

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.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Oct 13, 2020
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 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
Copy link
Contributor Author

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.

@jphickey jphickey linked an issue Oct 14, 2020 that may be closed by this pull request
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
Copy link
Contributor

skliper commented Oct 14, 2020

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

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

@ghost ghost left a comment

Choose a reason for hiding this comment

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

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 astrogeco changed the base branch from main to integration-candidate October 19, 2020 20:07
@astrogeco
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

ghost commented Oct 20, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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?

@astrogeco 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
astrogeco added a commit to nasa/cFS that referenced this pull request Oct 21, 2020
* Add nasa/PSP#207

* Add nasa/cFE#946

* Update CFE to fix startup issue

* Add nasa/osal#626

Co-authored-by: Joseph Hickey <[email protected]>
@jphickey jphickey deleted the fix-924-unique-id branch December 3, 2020 17:53
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
}

if (IsNewOffset || IsNewEntry)
Copy link
Contributor

Choose a reason for hiding this comment

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

Link #1929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants