sys/log: add module for colorized logging by aabadie · Pull Request #11573 · RIOT-OS/RIOT

aabadie · 2019-05-24T11:46:23Z

Contribution description

This PR adds a custom logging module for adding colors to displayed messages using ANSI escape codes. It can be useful to distinguish the type of messages displayed (ERROR, DEBUG, INFO, etc).
Colors are customizable using CFLAGS.

I had to patch the pyterm script to process the reset sequence correctly when the log message is not terminated by a newline. I haven't found a better solution so far.

Testing procedure

$ make  BOARD=<your favorite board> -C tests/log_color flash test
$ RIOT_TERMINAL=socat make BOARD=<your favorite board> -C tests/log_color flash term

Issues/PRs references

~~Based on #11572~~

miri64

Accept for having another example for a log module implementation, I really have to ask "why?". Especially since you want to change hello-world for that (if there would be a separate application to demo/test colorful logs I would care less about the "why?" ;-)).

miri64 · 2019-05-24T14:49:50Z

sys/log/log_color/log_module.h

@@ -0,0 +1,110 @@
+/*
+ * Copyright (C) 2015 Kaspar Schleiser <kaspar@schleiser.de>


You might want to update this copyright ;-)

aabadie · 2019-05-24T14:58:07Z

if there would be a separate application to demo/test colorful logs I would care less about the "why?" ;-)

The updated hello-world is the WIP part of this PR. Having a dedicated log demo/tests is what I want to achieve. I just PR'ed this a bit fast :)

kaspar030 · 2019-05-26T18:42:46Z

LGTM, and it is nicely contained. Works like a charm. I'll give my ACK once the dependency is in. Although I'm more with the "why?" guys... :)

miri64 · 2019-05-26T19:30:23Z

LGTM, and it is nicely contained. Works like a charm. I'll give my ACK once the dependency is in. Although I'm more with the "why?" guys... :)

Though I'm also in the "why?" camp, I can accept the answer "because we can", as long as it is not requiring large overhauls of the code base (which this change does not).

miri64 · 2019-05-26T19:31:37Z

tests/log_printfnoformat/Makefile

+TEST_ON_CI_WHITELIST += all
+
+# Enable debug log level
+CFLAGS += -DLOG_LEVEL=4


Idea for future "feature": make LOG_LEVEL configurable via global variable similar to DEVELHELP.

Hm, what would be the advantage over just setting the define?

Ease of use.

make LOG_LEVEL configurable via global variable similar to DEVELHELP

I like the idea :)

aabadie · 2019-05-27T09:05:07Z

I really have to ask "why?"

I think it's a cool/nice feature. But this is not the main argument. In general, having colored logs is helpful to distinguish between the level of messages. With the default logging in RIOT, all log messages looks the same, unless the developer add an extra prefix.

jcarrano · 2019-05-27T13:24:21Z

sys/log/log_color/log_module.h

+
+#ifdef MODULE_NEWLIB
+    /* no fflush on msp430 */
+    fflush(stdout);


Why are you flushing?

To force the reset escape code to be printed, otherwise, when one exits the terminal (pyterm or socat or miniterm.py) the last color code still applies and this messes up the host terminal.

And since fflush is not available msp430, this feature is not well supported on this family of platform.

Given that the terminal or the riot app can terminate at any moment (and that there is no flush in MSP430), one cannot really count on the RIOT app to reset the terminal state. The robust solution is to have "make term" or pyterm handle this by resetting after exit.

Except that make term or pyterm are not the only ways to connect to the stdout of a board. If I use miniterm.py /dev/ttyACM0 115200 or minicom or whatever, the terminal is not reset when exiting the tool. I think it's safer to let the call to LOG_XX do this.

jcarrano · 2019-05-27T13:33:22Z

I think it is important to note that the terms we are using are not very accurate. This (as well as the "noformat") are logging formatters, though currently they implement the logging "sink" too.

aabadie · 2019-08-27T14:44:19Z

Just in case, this is no longer WIP nor waiting for another PR.

aabadie · 2019-09-09T18:28:00Z

I'll give my ACK once the dependency is in

@kaspar030, this PR no longer relies on any other PR. I just rebased it on latest master.

kaspar030 · 2019-09-09T18:31:07Z

tests/log_color/Makefile

+
+USEMODULE += log_color
+
+TEST_ON_CI_WHITELIST += all


@cladmi this is default now, right?

Should be since #11680. @aabadie please remove this line, you can directly squash.

Apparently yes

makefiles/murdock.inc.mk:TEST_ON_CI_WHITELIST ?= all

I'll remove this line then.

kaspar030

ACK.

(tested on native and nrf52dk)

aabadie · 2019-09-09T19:01:53Z

Thanks for reviewing and ACKing @kaspar030. It's all green, let's merge!

aabadie requested a review from kaspar030 May 24, 2019 11:46

aabadie added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 24, 2019

miri64 reviewed May 24, 2019

View reviewed changes

aabadie force-pushed the pr/sys/log_color branch 7 times, most recently from 857b779 to cfd65b0 Compare May 26, 2019 09:36

aabadie added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels May 26, 2019

miri64 reviewed May 26, 2019

View reviewed changes

aabadie mentioned this pull request May 27, 2019

boards/native: fix overriden INCLUDES leading to build failure #11572

Merged

jcarrano reviewed May 27, 2019

View reviewed changes

aabadie mentioned this pull request May 28, 2019

make: add LOG_LEVEL to overridable variables #11592

Merged

aabadie removed State: waiting for other PR State: The PR requires another PR to be merged first Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Aug 6, 2019

aabadie force-pushed the pr/sys/log_color branch from cfd65b0 to 841d298 Compare August 6, 2019 19:56

aabadie added 2 commits September 9, 2019 20:27

sys/log: add colored logging module

04d3397

dist/tools/pyterm: fix handling of reset ANSI escape code

9ea3b2e

aabadie force-pushed the pr/sys/log_color branch from 841d298 to 3905290 Compare September 9, 2019 18:27

kaspar030 reviewed Sep 9, 2019

View reviewed changes

tests/log_color: add test application

f6ba779

aabadie force-pushed the pr/sys/log_color branch from 3905290 to f6ba779 Compare September 9, 2019 18:37

kaspar030 approved these changes Sep 9, 2019

View reviewed changes

aabadie merged commit 60229d1 into RIOT-OS:master Sep 9, 2019

aabadie deleted the pr/sys/log_color branch September 9, 2019 19:02

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

gschorcht mentioned this pull request Nov 21, 2019

cpu/esp32: improvements and cleanup of log_module #12044

Merged

		@@ -0,0 +1,110 @@
		/*
		* Copyright (C) 2015 Kaspar Schleiser <kaspar@schleiser.de>


		USEMODULE += log_color

		TEST_ON_CI_WHITELIST += all

Conversation

aabadie commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 24, 2019

Uh oh!

kaspar030 commented May 26, 2019

Uh oh!

miri64 commented May 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano commented May 27, 2019

Uh oh!

aabadie commented Aug 27, 2019

Uh oh!

aabadie commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aabadie commented May 24, 2019 •

edited

Loading

aabadie May 27, 2019 •

edited

Loading