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

core: make messaging optional #3851

Merged
merged 1 commit into from
Mar 30, 2016

Conversation

kaspar030
Copy link
Contributor

In order to enhance RIOT's modularity, make messaging optional.

When not used but compiled in, messaging uses a whopping 24b of code on 32bit platforms, but doubles the size of each tcb (16b -> 40b).

Not sure the savings are worth the effort, but who knows...

(Added msg to DEFAULT_MODULES, so this will not affect anything but those brave souls who compile RIOT with DISABLE_MODULE += msg).

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: core Area: RIOT kernel. Handle PRs marked with this with care! Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Sep 15, 2015
@kaspar030 kaspar030 added this to the Release NEXT MAJOR milestone Sep 15, 2015
@OlegHahm
Copy link
Member

Interesting

@IldarValiev
Copy link
Contributor

@kaspar030, may be it will be better to add new flag DISABLE_MESSAGING instead of MODULE_MSG - it makes more sense.

I think is important PR, because not in every case we need messaging, especially if RIOT controls simple sensor node.

@kaspar030 kaspar030 removed this from the Release 2015.12 milestone Dec 7, 2015
@OlegHahm OlegHahm modified the milestone: Release 2016.03 Dec 8, 2015
@OlegHahm
Copy link
Member

Sorry, this got lost somehow. Could you rebase, @kaspar030?

@kaspar030
Copy link
Contributor Author

  • rebased

@@ -1,4 +1,7 @@
APPLICATION = sizeof_tcb
include ../Makefile.tests_common

# optionally disable messaging
#DISABLE_MODULE += msg
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to comment this in here?

Maybe you could add this snippet also to dist/Makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it make sense to comment this in here?

hm?

Copy link
Member

Choose a reason for hiding this comment

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

I meant: why not indeed disabling the msg module here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Nope, IMHO sizeof_tcb should show the tcb size in the general case. disabling messaging seems more on the special side.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, sorry, confused with the minimal example.

@OlegHahm
Copy link
Member

I'm not sure, I see a concrete use case for that right now, but increasing modularity and making more features optional seems always a good idea if the overhead is manageable - which is the case for this change.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 26, 2016
@kaspar030
Copy link
Contributor Author

I've re-done the PR, it felt pointless to create a whole directory for msg, now I'm using ifdef within msg.c.

I've moved thread_print_msg_queue() to msg.c and renamed it to msg_queue_print(). @authmillenon, do you approve?

@kaspar030 kaspar030 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 26, 2016
@miri64
Copy link
Member

miri64 commented Feb 26, 2016

I would prefer creating a new directory. Other then that msg_queue_print() is okay. :-)

@miri64
Copy link
Member

miri64 commented Feb 26, 2016

(especially since compilers will complain about the empty compile unit).

@miri64
Copy link
Member

miri64 commented Feb 26, 2016

((I saw enough of this type of "modulization" in lwip and emb6 that I know I don't want to have that in RIOT ;-)))

@kaspar030
Copy link
Contributor Author

I would prefer creating a new directory.

why?

(especially since compilers will complain about the empty compile unit).
AFAIK, not if there's a single valid source line (e.g., headers).

@miri64
Copy link
Member

miri64 commented Feb 27, 2016

I gave already a bunch of reasons but here are more:

  • it's ugly
  • it gives a bad example and will enable future contributors to provide modules the same way
  • it's hard to assign binary data to modules (not in this example, but in lwIP - which uses only this kind of moduling - it definitely is a problem)

Compared to that:

  • moving it to a directory isn't hard ;-)

@kaspar030
Copy link
Contributor Author

Martine, how is it possible that we end up having completely opposite opinions on so many matters? ;)

I gave already a bunch of reasons but here are more:
No you didn't. You gave one, which I think is invalid.

it's ugly

Subjective. I don't agree. At least it is a lot less ugly than a subdirectory.

it gives a bad example and will enable future contributors to provide modules the same way

this is not the first module to do this.

it's hard to assign binary data to modules (not in this example, but in lwIP - which uses only this kind of moduling - it definitely is a problem)

Meaning - what?

Compared to that:

moving it to a directory isn't hard ;-)

It was in a subdirectory before the last commit(s). That looked ugly and completely unnecessary.

Mind you - the whole idea of disabling messaging is more an excercise of "because we can".
If it adds a pseudomodule and an #ifdef'd code block, fine. If it adds another directory (and thus becomes the only core .c file not in /core), it feels too intrusive to me for what it can do.

One-file modules in their own directory always seemed like a deficiency of the build system anyways.

@OlegHahm
Copy link
Member

Hm, difficult thing. On the one hand, having a subdirectory is kinda ugly, particular since it would be the only subfolder in core. On the other hand, this ifdef bracket around basically a whole file is kinda ugly, too. But finally, we cannot do this without some ifdefs anyway, so I'm willing to accept the current solution.

@miri64
Copy link
Member

miri64 commented Feb 29, 2016

Well if #4919 is imported and also added as a directory msg wouldn't be the only directory in core ;-). Also: I like directories they are not ugly :P.

@miri64
Copy link
Member

miri64 commented Feb 29, 2016

But fine. If I'm the only one that diggs subdirectories I'm fine with #ifdef blocks

@miri64
Copy link
Member

miri64 commented Feb 29, 2016

this is not the first module to do this.

Which other pseudo-module does this? I see only dependency-collecting modules, modules that are only header files, common API modules, and "pseudo-submodules" (modules that add additional functionality to an existing module) in Makefile.pseudomodules. There is no module that is exactly like a normal module, but is a pseudo-module because the complete code for is in an #ifdef.

@OlegHahm
Copy link
Member

Needs a rebase.

@haukepetersen
Copy link
Contributor

I like the PR and I agree with @kaspar030, that I don't really like putting it into a separate folder. The only thing I think could make sense, is to rename the module to core_msg, to make clear that this module is part of core. @authmillenon can you live with this?

so 1st ACK

@miri64
Copy link
Member

miri64 commented Mar 29, 2016

Yeah.

@kaspar030 kaspar030 added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 30, 2016
@kaspar030
Copy link
Contributor Author

@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 State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 30, 2016
@kaspar030
Copy link
Contributor Author

Would someone give a second ACK?

@kaspar030
Copy link
Contributor Author

green! :)

# If your application is very simple and doesn't use modules that use
# messaging, it can be disabled to save some memory:

#DISABLE_MODULE += msg
Copy link
Member

Choose a reason for hiding this comment

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

core_msg

@miri64
Copy link
Member

miri64 commented Mar 30, 2016

One minor doc thingy. Otherwise, fine... ACK

@kaspar030
Copy link
Contributor Author

Thx! addressed & amended.

@OlegHahm OlegHahm merged commit c09190b into RIOT-OS:master Mar 30, 2016
@kaspar030 kaspar030 deleted the make_messaging_optional branch March 30, 2016 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants