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

log: make name param explicit #9450

Merged
merged 1 commit into from
Sep 11, 2018
Merged

Conversation

galak
Copy link
Collaborator

@galak galak commented Aug 15, 2018

Rather than having some implied name for the logging name, explicitly
pass it in the macros LOG_MODULE_REGISTER & LOG_MODULE_DECLARE.

Signed-off-by: Kumar Gala kumar.gala@linaro.org

@galak
Copy link
Collaborator Author

galak commented Aug 15, 2018

@pizi-nordic any idea how to handle getting the name dynamically to fix / remove use of LOG_MODULE_NAME in LOG_CURRENT_MODULE_ID & LOG_CURRENT_DYNAMIC_DATA_ADDR

@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #9450 into master will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9450      +/-   ##
==========================================
+ Coverage   52.31%   52.51%   +0.19%     
==========================================
  Files         212      213       +1     
  Lines       25937    26082     +145     
  Branches     5589     5624      +35     
==========================================
+ Hits        13569    13696     +127     
- Misses      10122    10135      +13     
- Partials     2246     2251       +5
Impacted Files Coverage Δ
include/logging/log_core.h 100% <ø> (ø) ⬆️
lib/posix/clock.c 72.22% <0%> (-27.78%) ⬇️
lib/cmsis_rtos_v1/cmsis_kernel.c 80% <0%> (-20%) ⬇️
subsys/net/lib/app/server.c 41.41% <0%> (-11.12%) ⬇️
subsys/net/ip/net_stats.h 55.83% <0%> (-10.01%) ⬇️
subsys/random/rand32_xoroshiro128.c 70.37% <0%> (-4.63%) ⬇️
subsys/net/lib/http/http.c 23.58% <0%> (-3.78%) ⬇️
subsys/net/lib/app/net_app.c 41.07% <0%> (-2.81%) ⬇️
kernel/init.c 72.58% <0%> (-2.42%) ⬇️
subsys/net/ip/icmpv4.c 33.08% <0%> (-2.21%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4f8b7...e54956c. Read the comment docs.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

doc changes look OK

@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Aug 17, 2018
@pfalcon pfalcon requested a review from nordic-krch August 20, 2018 11:01
@pizi-nordic
Copy link
Collaborator

pizi-nordic commented Aug 20, 2018

@galak: The logger does not need LOG_MODULE_NAME. What you really need is a pointer to static and dynamic logger data.

What about the following solution:

  1. In the _LOG_MODULE_REGISTER(_name, _level) and _LOG_RUNTIME_MODULE_DECLARE(_name) add the following:
static inline const struct log_source_const_data *__log_current_const_data_get(void)
{
    return & LOG_ITEM_CONST_DATA(_name);
}

static inline struct log_source_dynamic_data *__log_current_dynamic_data_get(void)
{
    return &LOG_ITEM_DYNAMIC_DATA(_name);
}
  1. Instead of using &LOG_ITEM_DYNAMIC_DATA(LOG_MODULE_NAME) and &LOG_ITEM_CONST_DATA(LOG_MODULE_NAME) use the __log_current_dynamic_data_get() and __log_current_const_data_get().

The compiler should optimize out these functions and you will get something local to current compilation unit.

@nordic-krch: Could you please express your opinion on this proposal when you will be back in the office :)

@nordic-krch
Copy link
Contributor

@galak, @pizi-nordic proposal looks good imo.

@galak
Copy link
Collaborator Author

galak commented Sep 5, 2018

@pizi-nordic do you want to go head and make the change you suggested?

@nordic-krch
Copy link
Contributor

@galak, @pizi-nordic is on vacation. I've applied his suggestion by modifying last commit on my fork (https://github.com/nordic-krch/zephyr/commits/logging). Not sure how to proceed with that?

@galak
Copy link
Collaborator Author

galak commented Sep 6, 2018

@galak, @pizi-nordic is on vacation. I've applied his suggestion by modifying last commit on my fork (https://github.com/nordic-krch/zephyr/commits/logging). Not sure how to proceed with that?

Thanks, I pulled your changes into my tree and made a few updates.

@galak galak changed the title [DNM]log: make name param explicit log: make name param explicit Sep 6, 2018
@galak galak removed the DNM This PR should not be merged (Do Not Merge) label Sep 6, 2018
@galak galak mentioned this pull request Sep 6, 2018
@nashif nashif added this to the v1.14.0 milestone Sep 6, 2018
@mbolivar
Copy link
Contributor

mbolivar commented Sep 6, 2018

Is it possible to use logger after this commit with module_foo from a static inline routine in module_foo.h, as well as with module_bar from module_bar.c, where module_bar.c includes module_foo.h?

I am guessing the previous way 'ought' to work, possibly with some warnings for redefining the macro, but I'm less sure about the effect this PR would have on that. @galak, can you clarify?

@mbolivar
Copy link
Contributor

mbolivar commented Sep 6, 2018

Is it possible to use logger after this commit with module_foo from a static inline routine in module_foo.h, as well as with module_bar from module_bar.c, where module_bar.c includes module_foo.h?

I'm thinking of situations like this. What should the behavior be in general (with or without implicit names)?

/******************************************************************************
 * in module_foo.h:
 */

#define LOG_MODULE_NAME foo
#include <logging/log.h>
LOG_MODULE_REGISTER();

static inline void module_foo_func(void)
{
	LOG_INF("hello from foo");
}


/******************************************************************************
 * in module_bar.c:
 */

#include <module_foo.h>
#define LOG_MODULE_NAME bar
#include <logging/log.h>
LOG_MODULE_REGISTER();

void module_bar_func(void)
{
	module_foo_func();
	LOG_INF("hello from bar");
}

@nordic-krch
Copy link
Contributor

@mbolivar it's not only after this commit but in general using log in head it risky. I haven't yet into that but probably we would need to preceed every #define LOG_MODULE_NAME with #undef LOG_MODULE_NAME and use LOG_MODULE_DECLARE() in header and LOG_MODULE_REGISTER in C file.

Copy link
Contributor

@nordic-krch nordic-krch left a comment

Choose a reason for hiding this comment

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

lgtm. Not sure why shippable complains about line length. It usually accepts my 'signed-off-by'.

edit: missing dash probably: Signed-off by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

Rather than having some implied name for the logging name, explicitly
pass it in the macros LOG_MODULE_REGISTER & LOG_MODULE_DECLARE.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@mbolivar
Copy link
Contributor

I haven't yet into that but probably we would need to preceed every #define LOG_MODULE_NAME with #undef LOG_MODULE_NAME and use LOG_MODULE_DECLARE() in header and LOG_MODULE_REGISTER in C file.

Is that type of approach compatible with the changes @galak is suggesting? Naively it doesn't seem so, since the idea is to delete LOG_MODULE_NAME usage.

@galak galak merged commit 4fede8d into zephyrproject-rtos:master Sep 11, 2018
@galak
Copy link
Collaborator Author

galak commented Sep 11, 2018

Is that type of approach compatible with the changes @galak is suggesting? Naively it doesn't seem so, since the idea is to delete LOG_MODULE_NAME usage.

Yeah, we should assume magic #defines being set, thus me merging this.

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.

7 participants