Skip to content

logging: add template for log configuration #9933

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

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

nordic-krch
Copy link
Collaborator

Added Kconfig template file to be used when log configuration
in a module is added. Added example usage of kconfig template
in logger sample.

It is using new kconfig feature added in: #9797 (issue #9761)

Signed-off-by: Krzysztof Chruscinski [email protected]

@nordic-krch
Copy link
Collaborator Author

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Could this be split to two commits? The first one would add the template, and the second one would add the sample that shows how to use it.

@nordic-krch
Copy link
Collaborator Author

@jukkar done.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9933   +/-   ##
=======================================
  Coverage   52.51%   52.51%           
=======================================
  Files         213      213           
  Lines       26082    26082           
  Branches     5624     5624           
=======================================
  Hits        13696    13696           
  Misses      10135    10135           
  Partials     2251     2251

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 0329ec7...f40bac7. Read the comment docs.

Copy link
Member

@carlescufi carlescufi 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, Will add @ulfalizer to review too.

@jukkar
Copy link
Member

jukkar commented Sep 12, 2018

I have been porting this to networking logging code and was wondering if something like this would be possible (ping @ulfalizer) :

Kconfig:

module=FOOBAR
module-str=FOOBAR component
module-help=Enables FOOBAR debug messages.
source "Kconfig.log_level_template"

Kconfig.log_level_template:

choice
	prompt "Log level for $(module-str)"
	default $(module)_LOG_LEVEL_WRN
	help
	  $(module-help)

Current code outputs the help text just plain "$(module-help)" instead of expected help text.

menu "Application configuration"

module=SAMPLE_MODULE
module-str=Sample module
Copy link
Collaborator

@ulfalizer ulfalizer Sep 13, 2018

Choose a reason for hiding this comment

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

Would look less cramped with spaces around the =. Assignments work like in Make, so leading spaces are stripped automatically.

@ulfalizer
Copy link
Collaborator

ulfalizer commented Sep 13, 2018

@jukkar

Current code outputs the help text just plain "$(module-help)" instead of expected help text.

No macro expansion is done in help texts, so not currently possible. I wonder if it would break stuff in case help texts talk about Make variables, though that's unlikely in Zephyr.

Do you think making a reference to the prompt would be possible in the meantime, or making it generic ("enables some-feature for the subsystem" or the like)?

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

do we have documentation how this template can be used and will be used?

@nordic-krch
Copy link
Collaborator Author

good point. it is work updating the documentation.

@carlescufi
Copy link
Member

This is great. Just a minor comment, should we settle on an extension for Kconfig templates? I would perhaps suggest “.ktemplate”

@nordic-krch
Copy link
Collaborator Author

@carlescufi would you propose renaming file to log_config.ktemplate ?

@carlescufi
Copy link
Member

carlescufi commented Sep 14, 2018

@nordic-krch

@carlescufi would you propose renaming file to log_config.ktemplate ?

Actually looking at what we do today:
Kconfig.defconfig.nrf51822_QFAC
I think the consistent naming scheme would be:
Kconfig.template.logging

@nordic-krch
Copy link
Collaborator Author

@carlescufi maybe Kconfig.template.log_config since in future we might want to add another template (all must be in separate files) so Kconfig.template.logging is to wide. Don't you think?

@carlescufi
Copy link
Member

@nordic-krch

Sounds good to me.

Added Kconfig.log_template file with template for module log
configuration.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
Logger sample extended to use template for configuration of
sample module.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
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 change looks OK.

@nashif nashif merged commit 4add83c into zephyrproject-rtos:master Sep 14, 2018
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.

7 participants