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

arch: Migrate to new logging subsys #9361

Conversation

Olivier-ProGlove
Copy link
Contributor

Migrate from SYS_LOG to LOG logging mechanism.

Signed-off-by: Olivier Martin olivier.martin@proglove.de

@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9361   +/-   ##
=======================================
  Coverage   52.16%   52.16%           
=======================================
  Files         212      212           
  Lines       25894    25894           
  Branches     5563     5563           
=======================================
  Hits        13507    13507           
  Misses      10129    10129           
  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 3832378...6820576. Read the comment docs.

@Olivier-ProGlove
Copy link
Contributor Author

Olivier-ProGlove commented Aug 9, 2018

I have been trying to duplicate the build issue locally with success: rm -Rf * && cmake -DBOARD=frdm_k64f tests/net/socket/getaddrinfo/ && make

I guess I am missing something... Any idea?

@carlescufi carlescufi requested a review from nordic-krch August 9, 2018 19:43
#include <logging/sys_log.h>

#define LOG_MODULE_NAME arm_core_mpu
#define SYS_LOG_LEVEL SYS_LOG_LEVEL_DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

this entry has no impact on the logger configuration. If you want to overwrite default configuration you need to use
#define LOG_LEVEL LOG_LEVEL_DEBUG
Ideally, kconfig option should be added (see the comment #8816 (comment))

@@ -491,7 +495,7 @@ static int nxp_mpu_init(struct device *arg)
return 0;
}

#if defined(CONFIG_SYS_LOG)
#if defined(CONFIG_LOG)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is no longer needed. New logger is ready to be used from the start (before any module/driver initialization).

#include <linker/linker-defs.h>

#define LOG_MODULE_NAME arm_mpu
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if there is point to create to independent instances (arm_mpu and arm_core_mpu). If they are always working together then it can be just arm_mpu (see http://docs.zephyrproject.org/subsystems/logging/logger.html#usage for details how to declare a multi-file module).

Copy link
Member

Choose a reason for hiding this comment

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

@nordic-krch arm_mpu, or nxp_mpu are the MPU drivers, while arm_core_mpu is the high-level memory protection module that can work with both drivers

#include <misc/__assert.h>
#include <linker/linker-defs.h>

#define LOG_MODULE_NAME nxp_mpu
#define SYS_LOG_LEVEL SYS_LOG_LEVEL_DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

already mentioned. has no impact on the logger.

@ioannisg ioannisg added the area: ARM ARM (32-bit) Architecture label Aug 10, 2018
@ioannisg
Copy link
Member

@pizi-nordic FYI

@pizi-nordic pizi-nordic self-requested a review August 13, 2018 07:41
@Olivier-ProGlove
Copy link
Contributor Author

Olivier-ProGlove commented Aug 14, 2018

@nordic-krch @ioannisg Comments addressed

arch/arm/Kconfig Outdated
prompt "ARM Architecture logging level"
default ARCH_ARM_LOG_LEVEL_DBG

config ARCH_ARM_LOG_LEVEL_OFF
Copy link
Member

Choose a reason for hiding this comment

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

Why do these options need to be specific to ARM and not generic, i.e. for any ARCH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change done in new PR

Migrate from SYS_LOG to LOG logging mechanism.

Signed-off-by: Olivier Martin <olivier.martin@proglove.de>
@ioannisg
Copy link
Member

@Olivier-ProGlove this is now much better, IMO, as there will be common K-config structure for logging inside the component. @pizi-nordic could you please, re-review?
Adding @nashif, @andrewboie , as well, since this PR touches, now, arch/Kconfig

@ioannisg ioannisg requested review from nashif and andrewboie August 14, 2018 11:53
@Olivier-ProGlove Olivier-ProGlove changed the title arch: arm: Migrate to new logging subsys arch: Migrate to new logging subsys Aug 14, 2018
@ioannisg
Copy link
Member

Looks good to me. @nashif could you please do a sanity review, as well?

@ioannisg
Copy link
Member

ioannisg commented Sep 7, 2018

How could we proceed with this? @Olivier-ProGlove could you re-base this PR and resolve conflicts? @nordic-krch could you, then, re-review?

@Olivier-ProGlove
Copy link
Contributor Author

I am waiting for:

And then I will update all my new logging subystem pull-requests?

Does it sound a reasonable plan?

@nashif
Copy link
Member

nashif commented Oct 8, 2018

addressed in #10029

@nashif nashif closed this Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Logging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants