sys/log: make log_write nonstatic for log_color #18403
Conversation
sys/log/Makefile.include
Outdated
| BASELIBS += log.module | ||
| SRC += log_color.c |
There was a problem hiding this comment.
Can we use the default mechanism of SUBMODULES here and not some hand-spun solution, please? Edit: see https://doc.riot-os.org/creating-modules.html#pseudomodules
| PSEUDOMODULES += log | ||
| PSEUDOMODULES += log_printfnoformat | ||
| PSEUDOMODULES += log_color | ||
| PSEUDOMODULES += log_% |
There was a problem hiding this comment.
Why reduce specificity?
| PSEUDOMODULES += log_% | |
| PSEUDOMODULES += log_printfnoformat | |
| PSEUDOMODULES += log_color |
There was a problem hiding this comment.
It seemed overspecific (i had to remove PSEUDOMODULES += log since there are now variants that have build results)
so i made it less specific (as it is for other pseudo modules)
There was a problem hiding this comment.
if log is a pseudo module the build result isn't linked
There was a problem hiding this comment.
So how does a user find out what log_... pseudo-modules there are if you remove the only bit of documentation?
There was a problem hiding this comment.
It seemed overspecific (i had to remove
PSEUDOMODULES += logsince there are now variants that have build results)
Sorry my mistake. I realized that, but for got to remove the line. Fixed in the suggestion.
There was a problem hiding this comment.
and there seem are a lot on % in it (e.g.: prng_%)
And I dislike them as much as what you are trying to do here. I was and always will be against wildcards to define pseudomodules. I don't see any reason for them except lazyness. Let's be good scouts and not make the situation worse than it already is, just because there is precedence for bad behavior.
As for documentation: This file is actually rendered by Doxygen: https://doc.riot-os.org/group__pseudomodules.html...
There was a problem hiding this comment.
There was a problem hiding this comment.
If you can explain to me, why the change to wildcards is so important I am inclined to remove my change request regarding this. Because I still don't really see a valid reason.
Why do they use a different formatting than the rest of the listed pseudo-modules there?
There was a problem hiding this comment.
Why do they use a different formatting than the rest of the listed pseudo-modules there?
While most pseudomodules aren't even listed the one that are use the defgroup title in entirely different way than the rest of the documentation
I think: the title of a common defgroup for a (pseudo)module should be as i just wrote the one for log_... <modulename>: <title> this would also help the list in https://doc.riot-os.org/group__sys.html
eg.: from the syslist i know the is a "Lightweight Morse encoder" - what is its module to use?
changing this to "umorse: Lightweight Morse encoder" would help a lot
atm for many sys-modules the title is missused as a brief:
CongURE and fido2 are nice exemptions
There was a problem hiding this comment.
There is #7094. <modulename>: <title> would be one way to fix this. However, IMHO this should be done all in one go.
| # for each $(BASE_MODULE)_<name> in USEMODULE, add $(BASE_MODULE)_<name>.c to SRC | ||
| # unless in SUBMODULES_NO_SRC | ||
| SRC += $(wildcard \ | ||
| $(filter-out $(SUBMODULES_NO_SRC),\ | ||
| $(patsubst %,%.c,\ | ||
| $(filter $(BASE_MODULE)_%,$(USEMODULE))))) |
There was a problem hiding this comment.
previously log_color.c was not accepted (it wanted to have it named color.c) this adds log_color.c to the accepted names.
There was a problem hiding this comment.
Why not keep the old naming pattern in line with the documentation and name the file color.c? It is new anyways.
There was a problem hiding this comment.
the documentation of the submodule seemed to me as if log_color.c should be an accepted name and the only reason this did not work was that it was not done the way it was intended.
my personal reason: is that my editor names the tabs with the file name (no path) and i like to find the files again and color is very generic
There was a problem hiding this comment.
Our Makefile's contain already enough complexity to give me lots of nightmares. I totally agree with @miri64 that we should just use the submodule naming pattern as intended, rather than increasing the complexity of the build system even further to introduce an alternative naming pattern.
Also, adding a second naming pattern and introducing inconsistency is pretty confusing.
There was a problem hiding this comment.
should these log modules even be pseudo modules?
why not have sys/log_color and sys/log_puts? - they even got different tests this would simplify the whole thing a lot
There was a problem hiding this comment.
why not have sys/log_color and sys/log_puts? - they even got different tests this would simplify the whole thing a lot
There are already two ways of defining submodules.. either the SUBMODULES := 1 method with PSEUDOMODULES or the build system way of having subdirectories (see e.g. gnrc). Please let's not introduce a third. If you want the log_color module be a non-pseudomodule, do it with subdirectories.
287e299 to
6ffff1f
Compare
|
I prefer #18425 for its simplicity |
Contribution description
If log_color is used: this moves log_write from its header (being implemtented static to its own compile unit making it used accross multiple compile units)
Testing procedure
run tests/log_*
Issues/PRs references
#18379 i was asked by @miri64 to check if it increases the rom size (it didn't) while checking for that I found that log_write seems to not be inlined -> using one for all will decrease the size (if more than on compilation unit is using log (and log_color is used)).