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

Remove undefined error defines in cfe_error.h #552

Closed
skliper opened this issue Mar 11, 2020 · 5 comments · Fixed by #582
Closed

Remove undefined error defines in cfe_error.h #552

skliper opened this issue Mar 11, 2020 · 5 comments · Fixed by #582
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Mar 11, 2020

Is your feature request related to a problem? Please describe.

#define CFE_OS_ERROR_TASK_ID (OS_ERROR_TASK_ID)
/**
**
**
*/
#define CFE_OS_SEM_UNAVAILABLE (OS_SEM_UNAVAILABLE)

neither of these exist in OSAL

Describe the solution you'd like
Delete

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 6.8.0 milestone Mar 11, 2020
@skliper skliper changed the title Remove undefined CFE_OS_ERROR_TASK_ID in cfe_error.h Remove undefined error defines in cfe_error.h Mar 11, 2020
@jphickey
Copy link
Contributor

Actually, I would propose removing all of the CFE_OS_ error codes. They only occur in one place in the unit test and that place is wrong (should be using the OSAL-provided constant).

There should be no need for these duplicate names for the same error codes. It would be a different story if they were providing some abstraction, but they are not- they are only providing a different name for the same thing.

@skliper
Copy link
Contributor Author

skliper commented Mar 12, 2020

Yeah, I thought this topic had been touched on recently but I didn't find an issue (maybe in comments somewhere?) I'd be interested to hear from @acudmore.

@jphickey
Copy link
Contributor

It came up in the review of a recent change (issue #308/pull #452) where it was initially mixing the OS_ERR_ codes (direct from osal) and these CFE_OS_ synonyms. That changeset was subsequently modified to use only the OSAL error codes.

Logically speaking - If calling an OSAL function directly, it should be comparing to the OSAL error codes directly too.

The gray area comes in when CFE API calls in turn call the OSAL API, and return the OSAL error directly. For true abstraction this is a bad paradigm and shouldn't even be done (it should explicitly translate the code). But for efficiency the CFE error codes are simply chosen to not overlap/alias OSAL codes to make this practice OK, and it is understood that a return code from a CFE API might be either an OSAL error code or a CFE error code. Since both sets agree on the general pattern that negative is error and non-negative is success, its fine.

At any rate, cfe_error.h internally also includes osapi.h so if an application does a #include <cfe_error.h> then this will provide the OSAL error codes too.

So long as we aren't explicitly translating OSAL error codes to CFE error codes, we do not need to redefine the OSAL error constants in CFE either (and should not) .

@skliper
Copy link
Contributor Author

skliper commented Mar 12, 2020

Bigger issue probably worth 2 steps - deprecate then remove, the undefined referenced here could just be removed (they'd likely already be broken).

@skliper
Copy link
Contributor Author

skliper commented Apr 2, 2020

See #580 to cover deprecation of the entire set.

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

Successfully merging a pull request may close this issue.

2 participants