Skip to content

RTOS : Add Message Pipe #5160

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

Closed
wants to merge 5 commits into from
Closed

RTOS : Add Message Pipe #5160

wants to merge 5 commits into from

Conversation

YarivCol
Copy link
Contributor

Description

Message Pipe is FileHandle abstraction over rtos Mail.
It will be provide the user with ability to build more generic ITC or test code that use FileHandles

#5158 should be merged before this one.

@YarivCol YarivCol changed the title Message pipe impl RTOS : Add Message Pipe Sep 21, 2017
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 21, 2017

cc @bulislaw @pan- @c1728p9

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting your previous PR into two distinct PRs.

@c1728p9
Copy link
Contributor

c1728p9 commented Sep 21, 2017

This PR is mixing cmsis-os abstraction with mbed specific FileHandle code. Because of this it seems out of place in the rtos directory. It may be more appropriate to put this in features somewhere. What do you think?

CC @sg- @geky

@YarivCol
Copy link
Contributor Author

@c1728p9 Every class in rtos folder depends on NonCopyable from mbed platform,I think its same case here too.

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've approved the wrong PR, comments will follow.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it would fit platform directory more, I would like to keep rtos directory with RTX primitives or thin utilities.
Also can you work on docs? Especially the overall description -> if I don't know anything and I read the class description I should understand what I can do with this class.
We would also like to have some unit tests for this PR.

Copy link
Contributor

@sg- sg- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo:

  • links to API docs
  • unit tests
    I get that it blurs RTOS and platform but Given this depends on RTOS, how does it not fit in the RTOS directory (if you remove RTOS is still works but otherwise you cant)? This would be the first RTOS tied change that I'm aware of platform directory.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2017

@YarivCol Can you please respond to comments?

@YarivCol
Copy link
Contributor Author

YarivCol commented Oct 5, 2017

Yes, I will modify the docs and add unit test.
Working on it :) .

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 10, 2017

Yes, I will modify the docs and add unit test.

Let us know once done

@theotherjimmy
Copy link
Contributor

@AnotherButler New API.

@mbed-ci
Copy link

mbed-ci commented Oct 11, 2017

Build : FAILURE

Build number : 85
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5160/

@adbridge
Copy link
Contributor

@YarivCol Any update on this ?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 16, 2017

Please reopen once it is up to date with the changes requested above are answered.

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

Successfully merging this pull request may close these issues.

9 participants