Skip to content

Conversation

@Olivier-ProGlove
Copy link
Contributor

Migrate from SYS_LOG to LOG logging mechanism.

Signed-off-by: Olivier Martin [email protected]

@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9367      +/-   ##
==========================================
+ Coverage   52.19%   54.38%   +2.19%     
==========================================
  Files         215      216       +1     
  Lines       26010    29924    +3914     
  Branches     5594     6836    +1242     
==========================================
+ Hits        13576    16275    +2699     
- Misses      10175    11134     +959     
- Partials     2259     2515     +256
Impacted Files Coverage Δ
lib/posix/clock.c 78.57% <0%> (-21.43%) ⬇️
lib/cmsis_rtos_v1/cmsis_kernel.c 91.66% <0%> (-8.34%) ⬇️
subsys/net/ip/tcp_internal.h 48.27% <0%> (-1.73%) ⬇️
include/misc/stack.h 90.9% <0%> (-1.4%) ⬇️
lib/cmsis_rtos_v1/cmsis_thread.c 72.44% <0%> (-1.09%) ⬇️
lib/mempool/mempool.c 95.07% <0%> (-0.55%) ⬇️
subsys/net/ip/rpl.c 13.69% <0%> (-0.5%) ⬇️
include/kernel.h 98.43% <0%> (-0.1%) ⬇️
include/net/socket.h 100% <0%> (ø) ⬆️
include/posix/time.h 100% <0%> (ø) ⬆️
... and 43 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 a5f7e33...ce25563. Read the comment docs.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

LGTM.
I would rename commit title prefix to
drivers: watchdog: nrf:
(nrf: is used for Nordic SOC-related patches, AFAICS)

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened similar PRs for stm32 I2C (#9385) and usb (#9384) drivers. I had to change this line to make it work with the new LOG subsys. Did it work without changing this line?

@anangl
Copy link
Member

anangl commented Aug 14, 2018

@ioannisg The commit title is okay as it is. This is a shim using the nrfx driver underneath, so it is perfectly fine to use the "nrfx:" prefix. Such prefix would be inappropriate for a commit changing something related to Nordic SoCs but not using the nrfx driver package.

@Olivier-ProGlove Olivier-ProGlove force-pushed the upstream-logging-watchdog branch from 2ab8896 to 231f6e1 Compare August 14, 2018 08:19
@Olivier-ProGlove
Copy link
Contributor Author

@ioannisg @anangl I have just renamed nrfx into nrf to make everyone happy.

But I have also replaced #define SYS_LOG_LEVEL by #define LOG_LEVEL as noticed by @ydamigos

@ydamigos
Copy link
Contributor

@Olivier-ProGlove CONFIG_SYS_LOG_WDT_LEVEL depends on SYS_LOG. In order to work with the new LOG subsys you need to edit Kconfig file and change it to:
depends on SYS_LOG || LOG

@Olivier-ProGlove Olivier-ProGlove force-pushed the upstream-logging-watchdog branch from 231f6e1 to a7c4b25 Compare August 14, 2018 12:16
@Olivier-ProGlove Olivier-ProGlove changed the title drivers: watchdog: nrfx: Migrate to new logging subsys drivers: watchdog: Migrate to new logging subsys Aug 14, 2018
@Olivier-ProGlove
Copy link
Contributor Author

@ydamigos I have updated the pull-request to migrate all watchdog drivers to LOG

Migrate from `SYS_LOG` to `LOG` logging mechanism.

Signed-off-by: Olivier Martin <[email protected]>
@Olivier-ProGlove Olivier-ProGlove force-pushed the upstream-logging-watchdog branch from a7c4b25 to ce25563 Compare August 17, 2018 16:07
@Olivier-ProGlove
Copy link
Contributor Author

I have rebased the pull-request to fix the conflict after this pull-request has been merged 8cf8db3

@nashif nashif added this to the v1.14.0 milestone Sep 6, 2018
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Can you fix this for the change in LOG_MODULE_REGISTER to take the name as a param, and remove LOG_MODULE_NAME

#define LOG_MODULE_NAME wdt_nrfx
#define LOG_LEVEL CONFIG_LOG_WDT_LEVEL
#include <logging/log.h>
LOG_MODULE_REGISTER();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix this for the change in LOG_MODULE_REGISTER to take the name as a param, and remove LOG_MODULE_NAME

@nashif
Copy link
Member

nashif commented Sep 17, 2018

cherrypicked in #10029

@Olivier-ProGlove
Copy link
Contributor Author

@nashif That's a better cherry-pick compared to #9429 (comment) 👍

@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants