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

OSAL headers define a shared memory API that is not actually implemented anywhere #18

Closed
jphickey opened this issue Sep 16, 2019 · 6 comments · Fixed by #386 or #408
Closed

OSAL headers define a shared memory API that is not actually implemented anywhere #18

jphickey opened this issue Sep 16, 2019 · 6 comments · Fixed by #386 or #408
Assignees
Labels
Milestone

Comments

@jphickey
Copy link
Contributor

Describe the bug
The osapi-os-core.h header defines several prototypes e.g. OS_ShMemCreate(). However, these functions are not implemented anywhere, and are not used by CFE nor CFS apps.

To Reproduce
N/A - this is unused code.

Expected behavior
The headers should not include these prototypes if they are not implemented and there is no plan or requirement to implement them.

Code snips

/*
** Shared memory API 
*/
int32 OS_ShMemInit          (void);
int32 OS_ShMemCreate        (uint32 *Id, uint32 NBytes, const char* SegName);
int32 OS_ShMemSemTake       (uint32 Id);
int32 OS_ShMemSemGive       (uint32 Id);
int32 OS_ShMemAttach        (cpuaddr * Address, uint32 Id);
int32 OS_ShMemGetIdByName   (uint32 *ShMemId, const char *SegName );

System observed on:
Ubuntu 18.04 (64-bit)

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@skliper
Copy link
Contributor

skliper commented Sep 16, 2019

I'd assume these are here to define the API, similar to the memory/port/EEPROM prototypes that don't really do the unique work expected for custom hardware. Likely still useful if this is specifying what the prototypes would need to be, and the framework supported OSALs could define the functions as not implemented.

Maybe @acudmore or @jwilmot could provide more context, but I'd be inclined to leave them in if the intent is to define the API (knowing not all OSALs implement every API).

@jphickey
Copy link
Contributor Author

I would argue that if this would fit better as a PSP module akin to the memory/port/EEPROM code. It can then be modularized and selectively included in the PSP for platforms that may utilize shared memory.

However, it doesn't make much sense at this layer of OSAL, which is supposed to be for common, abstract services that all RTOS implementations should provide. Shared memory is a concept that only applies to targets with virtualized memory spaces.

I would recommend for removal of the APIs from this file, and they can be re-introduced with the PSP when an use-case is identified. The use case is quite important, as this is the only way to determine whether the proposed API will be sufficient to cover it.

@ghost
Copy link

ghost commented Sep 17, 2019

I went back as far as OSAL 3.3, I have older archives on a backup disk that I wanted to look up.
But do not think we ever had an implementation for the shared memory API, and I cannot currently think of a reason to keep it here.
It might have had roots in an attempt to work in a memory protected architecture, where each cFE subsystem (SB, etc ) could have exposed global/shared memory as a shared segment to the operating system. On a single address space OS, it would have been a no-op.

@skliper
Copy link
Contributor

skliper commented Sep 18, 2019

Sounds good, I'm in concurrence with removing these from the OSAL API and that it would be more appropriate at PSP level if it becomes a requirement.

@skliper skliper added this to the 5.0.0 milestone Sep 19, 2019
@skliper skliper modified the milestones: 5.0.0, 5.1.0 Oct 15, 2019
@skliper skliper added the good first issue Good for newcomers label Jan 2, 2020
skliper added a commit to skliper/osal that referenced this issue Mar 15, 2020
Document/comment changes only
Partially addresses nasa#255, nasa#12, nasa#18, nasa#366
skliper added a commit to skliper/osal that referenced this issue Mar 15, 2020
Document/comment changes only
Partially addresses nasa#255, nasa#12, nasa#18, nasa#366
@skliper
Copy link
Contributor

skliper commented Mar 23, 2020

Note also no functional tests... (no issue as long as we get these removed).

@skliper skliper added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Mar 25, 2020
@skliper skliper self-assigned this Mar 25, 2020
skliper added a commit to skliper/osal that referenced this issue Mar 25, 2020
@astrogeco
Copy link
Contributor

CCB 2020-03-25: No objections

@astrogeco astrogeco added CCB - 20200325 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Mar 25, 2020
skliper added a commit to skliper/osal that referenced this issue Mar 27, 2020
astrogeco added a commit that referenced this issue Apr 6, 2020
@astrogeco astrogeco linked a pull request Apr 9, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants