Re: RFC[2/2] Common logging infrastructure and API


Tomasz Bursztyka
 

Hi Genaro,

The idea is nice, at least it will prevent to declare debug macros
everywhere.

However, split the patch:
- first provide a patch with the logging.h and it's related Kconfig file
- and only then whatever usage of it in further patch.

I have small comments about logging.h and Kconfig:

diff --git a/include/logging.h b/include/logging.h
new file mode 100644
index 0000000..d778da9
--- /dev/null
+++ b/include/logging.h
@@ -0,0 +1,166 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/** @file logging.h
+ * @brief Logging macros.
+ */
+#ifndef __LOGGING_H
+#define __LOGGING_H
+
+#if !defined LOG_THIS_MODULE
+#error DO NOT include this header without defining LOG_THIS_MODULE
+#endif
I wouldn't use LOG_THIS_MODULE also, rather something like LOG_ENABLE
(Ok taste issue here, but a short name is nicer imo)
And I do want to be able to include logging.h even if don't define it.

So instead of the above just do:

#ifdef LOG_ENABLE

and add a relevant #endif below.

Then I could use it like:

#ifdef CONFIG_SPI_DEBUG
#define LOG_ENABLE 1
#endif
#include <logging.h>

It avoids thes superfluous lines
#else
#define LOG_ENABLE 0

+
+#if defined CONFIG_USE_COMPILE_LOG && (1 == LOG_THIS_MODULE)
+
+#define IS_LOGGING_ACTIVE 1
+#define THREAD_FN_CALL sys_thread_self_get()
+
+/* decide print func */
+#if defined(CONFIG_STDOUT_CONSOLE)
+#include <stdio.h>
+#define LOG_FN printf
+#else
+#include <misc/printk.h>
+#define LOG_FN printk
+#endif /* CONFIG_STDOUT_CONSOLE */
+
+/* Should use color? */
+#if defined(CONFIG_USE_LOG_COLOR)
+#define LOG_COLOR_OFF "\x1B[0m"
+#define LOG_COLOR_RED "\x1B[0;31m"
+#define LOG_COLOR_YELLOW "\x1B[0;33m"
+#else
+#define LOG_COLOR_OFF ""
+#define LOG_COLOR_RED ""
+#define LOG_COLOR_YELLOW ""
+#endif /* CONFIG_BLUETOOTH_DEBUG_COLOR */
+
+/* Should use log lv labels? */
+#if defined(CONFIG_USE_LOG_LV_TAGS)
+#define LOG_LV_ERR "ERR "
+#define LOG_LV_WRN "WRN "
+#define LOG_LV_INF "INF "
+#define LOG_LV_DBG "DBG "
+#else
+#define LOG_LV_ERR ""
+#define LOG_LV_WRN ""
+#define LOG_LV_INF ""
+#define LOG_LV_DBG ""
+#endif /* CONFIG_USE_LOG_LV_TAGS */
+
+/* Log domain name */
+#if defined LOG_DMN
+#define LOG_DMN_TAG LOG_DMN ":\t"
+#else
+#define LOG_DMN_TAG ""
+#endif /* LOG_DMN */
+
+/* Should thread be printed on log? */
+#if defined CONFIG_PRINT_THREAD_LOG
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t%p: %s" /* label func thread */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, THREAD_FN_CALL, log_color, ##__VA_ARGS__,
color_off)
+#else
+#define LOG_LAYOUT LOG_DMN_TAG "%s%s\t: %s" /* label func */
+#define LOG_FN_CALL(log_lv, log_color, log_format, color_off, ...)
\
+ LOG_FN(LOG_LAYOUT log_format "%s\n",
\
+ log_lv, __func__, log_color, ##__VA_ARGS__, color_off)
+#endif /* CONFIG_PRINT_THREAD_LOG */
+
+#define LOG_NO_COLOR(log_lv, log_format, ...)
\
+ LOG_FN_CALL(log_lv, "", log_format, "", ##__VA_ARGS__)
+#define LOG_COLOR(log_lv, log_color, log_format, ...)
\
+ LOG_FN_CALL(log_lv, log_color, log_format, LOG_COLOR_OFF,
\
+ ##__VA_ARGS__)
+
+/**
+ * @def INF
+ *
+ * @brief Writes information to log
+ *
+ * @details Lowest logging lv, these messages are not disabled unless
whole
+ * logging gets disabled.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define INF(...) LOG_NO_COLOR(LOG_LV_INF, ##__VA_ARGS__)
+
+#if defined(CONFIG_LOG_LV_ERR) || defined(CONFIG_LOG_LV_WRN) \
+ || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def ERR
+ *
+ * @brief Writes Errors to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_ERR
or higiher,
+ * it's meant to report fatal errors that can't be recovered from.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define ERR(...) LOG_COLOR(LOG_LV_ERR, LOG_COLOR_RED, ##__VA_ARGS__)
+#else
+#define ERR(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_WRN) || defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def WRN
+ *
+ * @brief Writes warnings to log
+ *
+ * @details available if CONFIG_USE_COMPILE_LOG is CONFIG_LOG_LV_WRN
or higiher,
+ * it's meant to print unexpedted situations that are not necesarily
an error.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define WRN(...) LOG_COLOR(LOG_LV_WRN, LOG_COLOR_YELLOW,
##__VA_ARGS__)
+#else
+#define WRN(...) { ; }
+#endif
+
+#if defined(CONFIG_LOG_LV_DBG)
+/**
+ * @def DBG
+ *
+ * @brief Writes debugging information to log
+ *
+ * @details highest logging level, available if CONFIG_USE_COMPILE_LOG
is
+ * CONFIG_LOG_LV_DBG, it's meant to print developer information.
+ *
+ * @param A kprint valid format string and optinally, kprint
formateable
+ * arguments.
+ */
+#define DBG(...) LOG_NO_COLOR(LOG_LV_DBG, ##__VA_ARGS__)
+#else
+#define DBG(...) { ; }
+#endif
+
+#else
+#define IS_LOGGING_ACTIVE 0
+/* create dummy macros */
+#define DBG(...) { ; }
+#define ERR(...) { ; }
+#define WRN(...) { ; }
+#define INF(...) { ; }
+
+#endif /*USE_COMPILE_LOG*/
+
+#endif /* __LOGGING_H */
diff --git a/misc/debug/Kconfig b/misc/debug/Kconfig
index 51a3d3f..716a361 100644
--- a/misc/debug/Kconfig
+++ b/misc/debug/Kconfig
@@ -16,6 +16,72 @@
# limitations under the License.
#

+config USE_COMPILE_LOG
Better something like LOG_COMPILE_TIME
Thus it will ease grep on CONFIG_LOG_* for instance.

+ bool
+ prompt "Enable Logging"
Just do bool "Enable Logging"
No need of the prompt. You can apply that at all options below.

+ default n
+ help
+ Global switch for logging.
+
+config USE_LOG_LV_TAGS
LOG_USE_LV_TAGS

+ bool
+ prompt "Prepend level tags to logs"
+ depends on USE_COMPILE_LOG
+ default y
+ help
+ Log lines will beging with one of INF, ERR, WRN or DBG,
depending on the
+ macro used at C code.
+
+config USE_LOG_COLOR
LOG_USE_COLOR

+ bool
+ prompt "Use colored logs"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Use color in the logs. This requires an ANSI capable
terminal.
+
+config PRINT_THREAD_LOG
LOG_PRINT_THREAD

+ bool
+ prompt "Include thread on log messages"
+ depends on USE_COMPILE_LOG
+ default n
+ help
+ Each log line will include the pointer of the thread that
printed it
+
+choice
+depends on USE_COMPILE_LOG
+prompt "Log level"
+
+config LOG_LV_INF
+ bool
+ prompt "Info"
+ help
+ Print just unfiltered logging.
+
+config LOG_LV_ERR
+ bool
+ prompt "Error"
+ help
+ Do not filter printing of ERR logging macro, it is meant to
report fatal
+ errors.
+
+config LOG_LV_WRN
+ bool
+ prompt "Warning"
+ help
+ Do not filter printing of WRN and above logging macros, WRN
marcro is
+ meant to print messages generated under unexpedted
circumstances, not
+ necesarily errors.
+
+config LOG_LV_DBG
+ bool
+ prompt "Debug"
+ help
+ Enable printing of all logging macros, DBG is meant to print
developer
+ information.
+
+endchoice
+
In the end we only have to deal with CONFIG_LOG_* prefixed options.

Tomasz

Join devel@lists.zephyrproject.org to automatically receive all group messages.