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

API for querying libraries (like CFE ES QueryApp) #28

Closed
skliper opened this issue Sep 16, 2019 · 18 comments · Fixed by #960 or #975
Closed

API for querying libraries (like CFE ES QueryApp) #28

skliper opened this issue Sep 16, 2019 · 18 comments · Fixed by #960 or #975
Assignees
Labels
cFS-Caelum enhancement Priority: Mission Feature or bug related to stakeholder needs
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 16, 2019

Is your feature request related to a problem? Please describe.
Unable to verify library image integrity (CS cannot scrub library code space)

Describe the solution you'd like
Implement API for querying library information, such that an app could scrub

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman/NASA-GSFC
External request from cfs-community email list on 9/3/2019

@skliper
Copy link
Contributor Author

skliper commented Jul 30, 2020

@jphickey could you look into this one? Mission priority for 7.0.

@jphickey
Copy link
Contributor

Sure, I'll take a look.

@jphickey jphickey self-assigned this Jul 30, 2020
@jphickey
Copy link
Contributor

After some initial evaluation it looks like this shouldn't be too hard to add - internally we just need to implement more generic variants of CFE_ES_GetAppIDByName and CFE_ES_GetAppInfoInternal functions that they can work on libraries in addition to apps.

For the TLM interface (output) I would propose to reuse the same command and packet format as the QUERY_ONE command currently uses. I think it can be done with a few simple overrides/extensions.

  • We already must have unique names between apps and libs because OSAL enforces this. So since the QUERY_ONE command accepts a name, it should be fairly simple to have it also check the Lib table for a name match in addition to the app table. This avoids having to make a new command for querying a lib and all the baggage associated with that.
  • Right now the "Type" field in the returned TLM packet only defines value for CORE or EXTERNAL. We should make it CORE (same), EXTERNAL_APP (the same meaning as current "external"), or EXTERNAL_LIB (new).
  • For EXTERNAL_LIB TLM structures, the following fields do not apply, and will be returned zeroed out: StackSize, ExceptionAction, Prioirt, MainTaskId, ExecutionCounter, MainTaskName, NumberOfChildTasks. Other fields (addresses, sizes, etc) would be valid. The "entry point" would be the lib init function. By not inventing a new TLM structure and maintaining the same format the existing ground system code should be able to display it with only minor updates (i.e. to recognize the type and maybe hide/gray out the fields that don't apply to libs).

@CDKnightNASA
Copy link
Contributor

@jphickey would it be worthwhile to adopt the OSAL "object id" model for this?

@jphickey
Copy link
Contributor

@jphickey would it be worthwhile to adopt the OSAL "object id" model for this?

By "object id model" - Do you mean the property that they are unique/independent number spaces so that the ID values cannot be confused/interchanged with eachother?

If so, then yes - it is definitely worthwhile to adopt the model, because in general it is a big improvement in code safety.

BUT - probably not under this ticket. It can be implemented independently. The change I'm proposing here would be fully backward compatible - but the change ID values would not be, in the event that any app used an ES AppId value as an array index. So while I think it's totally worthwhile and a big benefit, it needs its own separate buy-in and agreement from app developers as it is likely to break stuff.

@CDKnightNASA
Copy link
Contributor

@jphickey would it be worthwhile to adopt the OSAL "object id" model for this?

By "object id model" - Do you mean the property that they are unique/independent number spaces so that the ID values cannot be confused/interchanged with eachother?

Yes, and this also would set up the "framework" for other ID's in CFE.

BUT - probably not under this ticket. It can be implemented independently. The change I'm proposing here would be fully backward compatible - but the change ID values would not be, in the event that any app used an ES AppId value as an array index. So while I think it's totally worthwhile and a big benefit, it needs its own separate buy-in and agreement from app developers as it is likely to break stuff.

Agreed.

@jphickey
Copy link
Contributor

Update - spent some time exploring implementation on this -- and the net effect the was that the "LibTable" and "AppTable" start to look very similar, differing really in only the presence of TaskInfo data for the app main task and the control requests for exit/reload/etc.

I'm somewhat tempted to combine these tables into one, to reduce duplication of logic. It was also have the advantage such that AppIDs and LibIDs cannot get mixed up (they are the same).

On the downside it means the table will have to be sized for (CFE_ES_MAX_APPLICATIONS + CFE_ES_MAX_LIBARIES) -- which in turn meaning that you could get an AppID >= CFE_ES_MAX_APPLICATIONS. So it does break the notion of using as an array index. In which case it would make more sense to do what @CDKnightNASA suggested above.

Anyone with other thoughts/opinions on this matter? Can I revisit the definition of AppID or should we strive to keep it as backward compatible as possible, making the internal logic duplicated and/or more complicated to achieve that?

@skliper
Copy link
Contributor Author

skliper commented Aug 3, 2020

Ping @ejtimmon @acudmore @jwilmot. I don't have strong opinions other than a dislike of duplicated/complicated logic and I feel like if our concept is ID's are supposed to be opaque then eventually we are going to break the use of them as indexes, so why not now? I don't want to break a significant amount of code (maybe @ejtimmon and/or @tngo67 could survey apps?), but if they are looping through AppIDs like indexes they should use the foreach APIs instead anyways.

@jphickey
Copy link
Contributor

jphickey commented Aug 3, 2020

I'm leaning in the same direction.....

I looked through the cFS open source apps (CS, DS, FM, HK, HS, LC, MD, MM, SBN, SC, SCH) and I don't see any usage of AppID as an array index. For cases where it gets an AppID, they seem to just store the value and use it to invoke another function (e.g. CFE_ES_GetAppInfo, CFE_ES_RestartApp, CFE_EVS_SendEventWithAppID, etc) so that's fine.

Based on this it looks to me like changing the range of AppID values from [0, CFE_ES_MAX_APPLICATIONS) to something more abstract (not zero based) shouldn't be too bad at least WRT open source cFS apps.

At the same time we can also introduce a typedef for CFE_ES_AppID_t -- initially as uint32 for backward compatibility, but eventually I'd like to use a different (non-integer) type, for enhanced type safety here.

@jphickey
Copy link
Contributor

jphickey commented Aug 3, 2020

This has the distinct advantage that all of the existing APIs/command for dealing with apps (query one, query all, telemetry formats, etc) can also work on libraries with only perhaps minor updates - the only real difference being that there is no "main task" on a library - but this seems easy enough to deal with, as it can be zeroed out without changing any data formats.

By extension, if we do this, it may mean that commands like "ReloadApp" and "DeleteApp" might actually work on libs too - although we just had a CCB discussion about this and decided not to do it because of the high risk - if it can be done as part of the existing app commands maybe just leave it up to operator discretion as to whether they use it or not.

@tngo67
Copy link

tngo67 commented Aug 3, 2020

Ping @ejtimmon @acudmore @jwilmot. I don't have strong opinions other than a dislike of duplicated/complicated logic and I feel like if our concept is ID's are supposed to be opaque then eventually we are going to break the use of them as indexes, so why not now? I don't want to break a significant amount of code (maybe @ejtimmon and/or @tngo67 could survey apps?), but if they are looping through AppIDs like indexes they should use the foreach APIs instead anyways.

Just check with my team, we are using the API to get the AppID. So we are okay with the proposed changes for now.

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 5, 2020
@skliper
Copy link
Contributor Author

skliper commented Aug 5, 2020

Let's discuss at CCB to confirm everyone is OK with proposed solution.

@ejtimmon
Copy link
Contributor

ejtimmon commented Aug 5, 2020

Ping @ejtimmon @acudmore @jwilmot. I don't have strong opinions other than a dislike of duplicated/complicated logic and I feel like if our concept is ID's are supposed to be opaque then eventually we are going to break the use of them as indexes, so why not now? I don't want to break a significant amount of code (maybe @ejtimmon and/or @tngo67 could survey apps?), but if they are looping through AppIDs like indexes they should use the foreach APIs instead anyways.

I checked the internal versions of the open source GSFC apps as well as some other internal apps - I don't see any places where this change would cause a problem.

@astrogeco
Copy link
Contributor

CCB 2020-08-05 See notes

@jphickey
Copy link
Contributor

jphickey commented Aug 6, 2020

Another (minor?) issue I've come across is that some of the code that gets tripped up from reorganizing the internal tables is actually deprecated, e.g. CFE_ES_ListTasks().

Now that the 6.8.0 RC1 has been officially branched, can we go ahead and finally remove the deprecated items from the main branch? It would save me from updating those functions....

@skliper
Copy link
Contributor Author

skliper commented Aug 7, 2020

can we go ahead and finally remove...

Yes!

@jphickey
Copy link
Contributor

Submitted related issue #797 regarding internal ES table management.

Unfortunately ES is currently inconsistent in how it manages internal tables. The more general problem of using an abstract ID and making the internal ES table management more consistent I thought was worthy of a separate review and discussion. Once we have a consistent means/patterns to deal with "id" values and resource tracking in ES, then it shouldn't be too hard to make the application management commands apply to libraries too.

@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 12, 2020
jphickey added a commit to jphickey/cFE that referenced this issue Oct 19, 2020
Allows the existing "CFE_ES_AppInfo_t" structure to be extended to
libraries as well as applications by introducing a new value (3)
for the Type field.

Allows Libraries to be queried via API calls similar to App API.

Also extends the Query All/Query One commands to operate on
Libraries or Applications.
@jphickey
Copy link
Contributor

Status update:

Looking to get a PR for this (possibly) submitted for the 2020-10-21 CCB. Proposed/draft changeset can be found in e43ef28 - will need rebase once next "main" branch is finalized, but otherwise should be OK.

What this does:

  • Reuses/Extends the CFE_ES_AppInfo_t to also apply to libraries, by introducing a new/different value for the Type field (3).
  • When used for Libraries, Information related to tasks and task IDs is omitted (zero).
  • Adds a runtime API called CFE_ES_GetLibInfo() which correlates with CFE_ES_GetAppInfo() but operates on libraries instead of apps.
  • Adds an API called CFE_ES_GetModuleInfo() which operates on either app IDs or lib IDs. This eases conversion of existing code to operate on either type (just change the call).
  • Updates the Query One and Query All commands to work on Apps and Libs.

Is there any other command/API that needs to work on both items which is not addressed per above? I think this is it, but please advise if anything is missing.

Thoughts - @skliper @ejtimmon @tngo67 @jwilmot or @acudmore ?? Thanks!

jphickey added a commit to jphickey/cFE that referenced this issue Oct 21, 2020
Allows the existing "CFE_ES_AppInfo_t" structure to be extended to
libraries as well as applications by introducing a new value (3)
for the Type field.

Allows Libraries to be queried via API calls similar to App API.

Also extends the Query All/Query One commands to operate on
Libraries or Applications.
@jphickey jphickey linked a pull request Oct 21, 2020 that will close this issue
jphickey added a commit to jphickey/cFE that referenced this issue Oct 26, 2020
Allows the existing "CFE_ES_AppInfo_t" structure to be extended to
libraries as well as applications by introducing a new value (3)
for the Type field.

Allows Libraries to be queried via API calls similar to App API.

Also extends the Query All/Query One commands to operate on
Libraries or Applications.
astrogeco added a commit that referenced this issue Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cFS-Caelum enhancement Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants