-
Notifications
You must be signed in to change notification settings - Fork 2.1k
core: make messaging optional #3851
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
Conversation
Interesting |
@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. |
Sorry, this got lost somehow. Could you rebase, @kaspar030? |
b1bae9f
to
f08ff73
Compare
|
@@ -1,4 +1,7 @@ | |||
APPLICATION = sizeof_tcb | |||
include ../Makefile.tests_common | |||
|
|||
# optionally disable messaging | |||
#DISABLE_MODULE += msg |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
dc1c004
to
89e3727
Compare
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? |
I would prefer creating a new directory. Other then that |
(especially since compilers will complain about the empty compile unit). |
((I saw enough of this type of "modulization" in lwip and emb6 that I know I don't want to have that in RIOT ;-))) |
why?
|
I gave already a bunch of reasons but here are more:
Compared to that:
|
Martine, how is it possible that we end up having completely opposite opinions on so many matters? ;)
Subjective. I don't agree. At least it is a lot less ugly than a subdirectory.
this is not the first module to do this.
Meaning - what?
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". One-file modules in their own directory always seemed like a deficiency of the build system anyways. |
Hm, difficult thing. On the one hand, having a subdirectory is kinda ugly, particular since it would be the only subfolder in |
Well if #4919 is imported and also added as a directory |
But fine. If I'm the only one that diggs subdirectories I'm fine with #ifdef blocks |
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 |
Needs a rebase. |
89e3727
to
27ee8e4
Compare
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 so 1st ACK |
Yeah. |
d0439c5
to
cb3c991
Compare
|
cb3c991
to
b36b802
Compare
Would someone give a second ACK? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core_msg
b36b802
to
58a12e5
Compare
One minor doc thingy. Otherwise, fine... ACK |
Thx! addressed & amended. |
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
).