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.

Sign up for GitHub

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

Jump to bottom

core: mbox: initial commit #4919

Merged
merged 2 commits into from
Jul 18, 2016
Merged

core: mbox: initial commit #4919

merged 2 commits into from
Jul 18, 2016

Conversation

kaspar030
Copy link
Contributor

Alternative to #4552.

differences:

  • a quarter of the code size
  • more than two times faster

WIP:

  • documentation missing

(timeout functions missing) postponed

@kaspar030 kaspar030 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 28, 2016
@kaspar030 kaspar030 mentioned this pull request Feb 28, 2016
Closed
@miri64
Copy link
Member

miri64 commented Feb 28, 2016

Not tested, but at least code-wise it looks simpler then my implementation. However: I would prefer putting the non-mbox related changes to core into a separate commit.

@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 28, 2016
@OlegHahm
Copy link
Member

This is based on #4557, right?

@kaspar030
Copy link
Contributor Author

This is based on #4557, right?

Ah, yes...

@miri64 miri64 mentioned this pull request Feb 29, 2016
Merged
OlegHahm
OlegHahm reviewed Feb 29, 2016
View reviewed changes
core/include/mbox.h
};

void mbox_put(mbox_t *mbox, msg_t *msg);
int mbox_tryput(mbox_t *mbox, msg_t *msg);
Copy link
Member

Choose a reason for hiding this comment

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

For API consistency: mbox_try_put()

@miri64
Copy link
Member

miri64 commented Mar 6, 2016

I really would like to have this soonish (though lwIP is already winning time-wise despite my crappy-mbox API :P)

@miri64 miri64 mentioned this pull request Mar 8, 2016
Closed
@kaspar030
Copy link
Contributor Author

@authmillenon Sorry, I found a flaw in the logic. Will have to fix that first.

@miri64
Copy link
Member

miri64 commented Mar 14, 2016

Kk. Thesis evaluation is done anyways. Mentioned this PR as an optimization possibility, but used my mbox implementation for the actual comparison.

@miri64 miri64 added this to the Release 2016.07 milestone Mar 23, 2016
@miri64 miri64 mentioned this pull request Mar 31, 2016
Closed
@miri64
Copy link
Member

miri64 commented Mar 31, 2016

Fixes #4342

@miri64
Copy link
Member

miri64 commented Mar 31, 2016

(please rebase)

@kaspar030 kaspar030 removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 10, 2016
@kaspar030
Copy link
Contributor Author

  • rebased

I un-marked WIP as it should be usable now. Let's add timeout versions later.

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Apr 10, 2016
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 10, 2016
@kaspar030
Copy link
Contributor Author

As this is very unintrusive, we could think about merging for the release, marking it as experimental?

@miri64 miri64 assigned Kijewski, miri64 and OlegHahm and unassigned Kijewski and OlegHahm Jun 6, 2016
@miri64 miri64 mentioned this pull request Jun 7, 2016
Merged
miri64
miri64 reviewed Jun 7, 2016
View reviewed changes
core/include/thread.h
#if defined(MODULE_CORE_MSG) || defined(MODULE_CORE_THREAD_FLAGS)
void *wait_data; /**< used by msg and thread flags */
#if defined(MODULE_CORE_MSG) || defined(MODULE_CORE_THREAD_FLAGS) \
|| defined(MODULE_MBOX)
Copy link
Member

Choose a reason for hiding this comment

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

How does that module gets defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nowhere. fixed.

@miri64 miri64 mentioned this pull request Jun 7, 2016
Merged
miri64
miri64 reviewed Jun 10, 2016
View reviewed changes
core/include/mbox.h
#endif

/** Static initializer for mbox objects */
#define MBOX_INIT(queue, queue_size) {{0}, {0}, CIB_INIT(queue_size), queue, NULL}
Copy link
Member

Choose a reason for hiding this comment

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

Also needs a dynamic initializer e.g.:

static inline void mbox_init(mbox_t *mbox, msg_t queue, unsigned int queue_size)
{
    mbox_t m = MBOX_INIT(queue, queue_size);
    *mbox = m;
}

so you can (re-)initialize the variable at other points than variable definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@kaspar030
Copy link
Contributor Author

  • addressed comments
  • rebased

@emmanuelsearch
Copy link
Member

@miri64 ACK still stands?

miri64
miri64 reviewed Jul 14, 2016
View reviewed changes
core/include/mbox.h

#ifndef MODULE_CORE_MBOX
#error mbox depends on the module core_mbox!
#endif
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is superfluous. The linker will yield an error anyway.

@miri64
Copy link
Member

miri64 commented Jul 14, 2016

@miri64 ACK still stands?

Apart from comment above, yes :-)

@kaspar030
Copy link
Contributor Author

  • addressed & squashed

@emmanuelsearch
Copy link
Member

Looks good from what I can see.
It does not break anything, since it's new feature.
So nothing speaks against an ACK.
Hence: ACK

@miri64
Copy link
Member

miri64 commented Jul 14, 2016

AVR and MSP430 are butthurt about something.

@miri64
Copy link
Member

miri64 commented Jul 18, 2016

Ping?

@kaspar030
Copy link
Contributor Author

Hm, that seems to be a compiler problem. My arch avr-gcc (5.3.0) compiles the code just fine, the one used by murdock doesn't.

@kaspar030
Copy link
Contributor Author

  • rebased

@kaspar030
Copy link
Contributor Author

Turns out there was an excess initializer. Makes me wonder why the other compilers didn't choke on that.

  • amended a fix

@kaspar030
Copy link
Contributor Author

two ACKs -> go

@kaspar030 kaspar030 merged commit 66710ef into RIOT-OS:master Jul 18, 2016
@kaspar030 kaspar030 deleted the mbox branch July 18, 2016 15:56
@miri64 miri64 mentioned this pull request Feb 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Kijewski Kijewski

@miri64 miri64

@OlegHahm OlegHahm

Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kaspar030 @miri64 @OlegHahm @emmanuelsearch @Kijewski