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

Clean up ES task/app registration #1215

Closed
jphickey opened this issue Mar 10, 2021 · 4 comments · Fixed by #1250 or #1258
Closed

Clean up ES task/app registration #1215

jphickey opened this issue Mar 10, 2021 · 4 comments · Fixed by #1250 or #1258
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In nasa/osal#853 it is proposed that OS_TaskRegister() be finally deprecated/removed.

But this function is invoked by two places in CFE ES that follow a similar pattern: CFE_ES_RegisterApp() and CFE_ES_RegisterChildTask().

Describe the solution you'd like
At a minimum, calls to OS_TaskRegister() must be removed to allow the function to be deprecated.

Furthermore there is already a task startup wrapper in ES that can be used to call/handle setting up environment (CFE_PSP_SetDefaultExceptionEnvironment()) meaning that these two functions themselves can also be deprecated - basically using the same design pattern as OSAL uses such that we don't need to burden apps with calling this extra function. This design is simpler and less error prone.

Additional context
See nasa/osal#853

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

Ping @skliper , @ejtimmon , @acudmore , @jwilmot ....

This is the only ref to OS_TaskRegister() that I found but interestingly we have basically the same situation in CFE ES - a couple functions that have effectively become no-ops - or at least unnecessary - but remain in the API for compatibility reasons.

However unlike OS_TaskRegister it looks like these ARE invoked by many CFS apps - so there are definite backward compatibility issues to removing/deprecating it.

This makes me think we should just leave it alone - is it that bad to have a couple no-op functions laying around for backward compatibility?

@skliper
Copy link
Contributor

skliper commented Mar 10, 2021

I find it more confusing to have an API that's half utilized, possibly partly documented, doesn't actually do anything, not really clear what the plan is for the future (will we leave it forever?), etc. I'd rather a quick/hard break than the uncertainty the noops cause. Unless we can fully utilize/document and very clearly state to keep these calls in (for future development or whatever), as it it's a bug if they are missing.

Basically half way is worse (in my view) than hard yes or hard no.

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 16, 2021
@skliper
Copy link
Contributor

skliper commented Mar 17, 2021

CCB - if we are going to do it, do it now. No hard objects, lets remove.

@skliper skliper added this to the 7.0.0 milestone Mar 17, 2021
@skliper skliper added CCB:2021-03-17 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Mar 17, 2021
@skliper
Copy link
Contributor

skliper commented Mar 18, 2021

Also noted on community-email list the comment below is misleading. As long as we are removing then no additional action needed. If for some reason this stays, should be updated.

** Cannot create a syslog entry here because it requires the task to

ping @johnphamngc

jphickey added a commit to jphickey/cFE that referenced this issue Mar 23, 2021
Explicit task registration is no longer necessary, since all
required actions can be done before invoking the entry point.

This moves the invocation of CFE_PSP_AttachExceptions() from
the registration function to the pre-entry function, this was
the only remaining action in task registration.

All references to task registration in code, docs, and tests
are removed.
jphickey added a commit to jphickey/cFE that referenced this issue Mar 23, 2021
Explicit task registration is no longer necessary, since all
required actions can be done before invoking the entry point.

This moves the invocation of CFE_PSP_AttachExceptions() from
the registration function to the pre-entry function, this was
the only remaining action in task registration.

All references to task registration in code, docs, and tests
are removed.
astrogeco added a commit that referenced this issue Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants