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 #9368

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Conversation

galak
Copy link
Collaborator

@galak galak commented Aug 9, 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 galak added the RFC Request For Comments: want input from the community label Aug 9, 2018
@galak
Copy link
Collaborator Author

galak commented Aug 9, 2018

Before we starting using the logging subsystem everywhere, wanted to see about this change (needs some updates to the docs if the change is agreeable).

@galak galak added the DNM This PR should not be merged (Do Not Merge) label Aug 9, 2018
@galak galak changed the title [RFC]log: make name param explicit [DNM][RFC]log: make name param explicit Aug 9, 2018
@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #9368 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9368   +/-   ##
=======================================
  Coverage   52.17%   52.17%           
=======================================
  Files         212      212           
  Lines       25892    25892           
  Branches     5561     5561           
=======================================
  Hits        13508    13508           
  Misses      10126    10126           
  Partials     2258     2258

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 8cf8db3...e8e7c25. Read the comment docs.

@carlescufi carlescufi requested a review from nordic-krch August 9, 2018 19:47
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.

I like the idea and it looks good. Only documentation must be then update

@nordic-krch
Copy link
Contributor

cc: @pizi-nordic @jarz-nordic
It's my last day before 3 weeks vacation. Piotr or Jakub can follow in my stead.

@pizi-nordic pizi-nordic self-requested a review August 10, 2018 06:32
Copy link
Collaborator

@pizi-nordic pizi-nordic left a comment

Choose a reason for hiding this comment

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

Looks good for me. The only thing which might be potentially missing is the define/API for fetching current module name (similar to LOG_CURRENT_MODULE_ID()).

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 galak requested a review from dbkinder as a code owner August 15, 2018 13:03
@galak galak changed the title [DNM][RFC]log: make name param explicit log: make name param explicit Aug 15, 2018
@galak galak removed DNM This PR should not be merged (Do Not Merge) RFC Request For Comments: want input from the community labels Aug 15, 2018
@galak
Copy link
Collaborator Author

galak commented Aug 15, 2018

I updated the docs, figure we can address the addition of some new API to fetch the name as a separate PR. Wanted to have this go in before all the other changes to convert a bunch of subsystems to use logging.

@galak galak merged commit 2cb17a0 into zephyrproject-rtos:master Aug 15, 2018
@galak galak deleted the logging branch August 15, 2018 14:41
@galak
Copy link
Collaborator Author

galak commented Aug 15, 2018

I thought this was sufficient since it passed CI, but it breaks.

Re-opened as PR #9450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants