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

SB: Remove PipeName from CFE_SB_PipeD_t #288

Closed
skliper opened this issue Sep 30, 2019 · 11 comments
Closed

SB: Remove PipeName from CFE_SB_PipeD_t #288

skliper opened this issue Sep 30, 2019 · 11 comments
Assignees

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

PipeName in CFE_SB_PipeD_t is redundant with the OSAL queue name and should be removed.

@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 257. Created by cdknight on 2018-10-23T18:35:33, last modified: 2019-03-26T13:10:28

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-10-23 22:15:23:

See also ticket #287

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-10-24 12:49:09:

change committed [changeset:a8d1216]

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-11-13 13:32:15:

Two suggestions from the Joe at the CCB meeting: in SendMessageFull there are multiple buffers created in multiple if blocks. Move that up.

Also instead of passing a char* buffer into GetPipeName, change it to take a typedef struct that wraps the OSAL struct to remove the double-copy.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-11-14 15:40:07:

Updated code at [changeset:c0a9d80]

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-02-25 16:16:53:

Updates pending CCB review, expected test and merge disposition.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-26 13:10:28:

Note this may impact get pipe id by name #210

@skliper skliper assigned CDKnightNASA and unassigned skliper Sep 30, 2019
@CDKnightNASA
Copy link
Contributor

Something I'm thinking about...OS_GetQueueInfo copies the struct to a buffer, and GetPipeName will copy the name to a second buffer.

I'm sure the answer is NO but we trade efficiency for safety when we do all of this buffer copying. GetQueueInfo could just return the pointer to the memory of the queue's info and GetPipeName could return the pointer to the string from that queue info. Just thought as this is flight software, we could significantly reduce the memory stack use by not copying data around as much as we do.

@jphickey
Copy link
Contributor

jphickey commented Nov 8, 2019

I do agree this is a lot of unnecessary string copying action here.

I would be generally in favor of using the OS_queue_prop_t object directly. But you'd have to instantiate that object at the parent function rather than a 20 character name buffer. The overall stack usage would be less, though, and it would avoid unnecessary string copies.

@CDKnightNASA
Copy link
Contributor

In discussing GetPipeIdByName, the CCB agreed to make GetPipeName a public API. As there's interactions between GetPipeIdByName and GetPipeName, I merged the branch for this ticket into the branch for ticket #210 (pull req #404 ). I am closing this ticket and suggesting we review/approve/merge the other ticket/branch. I just pushed changes to support a public GetPipeName.

@skliper
Copy link
Contributor Author

skliper commented Dec 30, 2019

Marking as duplicate (of #210) for ticket cleanup/management

@skliper skliper added duplicate and removed bug labels Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants