Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions include/logging/log_ctrl.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ extern "C" {

typedef u32_t (*timestamp_get_t)(void);

/** @brief Function system initialization of the logger.
*
* Function is called during start up to allow logging before user can
* explicitly initialize the logger.
*/
void log_core_init(void);

/**
* @brief Function for initializing logger core.
* @brief Function for user initialization of the logger.
*
* @return 0 on success or error.
*/
int log_init(void);
void log_init(void);

/**
* @brief Function for providing timestamp function.
Expand Down Expand Up @@ -138,7 +144,7 @@ void log_backend_enable(struct log_backend const *const backend,
*/
void log_backend_disable(struct log_backend const *const backend);

#if defined(CONFIG_LOG) && CONFIG_LOG
#if CONFIG_LOG
#define LOG_INIT() log_init()
#define LOG_PANIC() log_panic()
#define LOG_PROCESS() log_process(false)
Expand Down
3 changes: 3 additions & 0 deletions include/logging/log_msg.h
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ union log_msg_chunk {

extern struct k_mem_slab log_msg_pool;

/** @brief Function for initialization of the log message pool. */
void log_msg_pool_init(void);

/** @brief Function for indicating that message is in use.
*
* @details Message can be used (read) by multiple users. Internal reference
Expand Down
5 changes: 5 additions & 0 deletions kernel/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <kernel_internal.h>
#include <kswap.h>
#include <entropy.h>
#include <logging/log_ctrl.h>

/* kernel build timestamp items */
#define BUILD_TIMESTAMP "BUILD: " __DATE__ " " __TIME__
Expand Down Expand Up @@ -459,6 +460,10 @@ FUNC_NORETURN void _Cstart(void)

_IntLibInit();

if (IS_ENABLED(CONFIG_LOG)) {
log_core_init();
}

/* perform any architecture-specific initialization */
kernel_arch_init();

Expand Down
10 changes: 4 additions & 6 deletions samples/subsys/logging/logger/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ static void performance_showcase(void)
u32_t window = 2;

do {
LOG_INF("performance test - log message %d", cnt++);
LOG_INF("performance test - log message %d", cnt);
Copy link
Contributor

Choose a reason for hiding this comment

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

As @andrewboie proposed here: #8828 (comment), we could use static inline functions in order to not change behaviour when logger is enabled and disabled.

@nordic-krch; Have you tried this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macros gives me possibility to handle variable number of arguments without using va_list. but even when inline functions would be used I need to handle the case when some variables that macro/function access disappears when logger is disabled for given module.

Copy link
Contributor

Choose a reason for hiding this comment

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

if the log is disabled you can just make empty variadic inline functions.
I really would rather see this with inline functions at some point in the layers so that variables defined specifically for use in a log point don't generate "defined but not used" warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ensured, if you look at macro __LOG() (log_core.h line 169) you can see that it always evaluates to if else clause. Condition in if clause evaluates to false on compile time if log is disabled for given module and log code is not included in the binary but you will never get 'defined but not used'.

cnt++;
current_timestamp = timestamp_get();
} while (current_timestamp < (start_timestamp + window));

Expand All @@ -191,12 +192,9 @@ static void external_log_system_showcase(void)

void main(void)
{
int err = LOG_INIT();

if (err == 0) {
err = log_set_timestamp_func(timestamp_get, timestamp_freq());
}
LOG_INIT();

int err = log_set_timestamp_func(timestamp_get, timestamp_freq());
(void)err;

module_logging_showcase();
Expand Down
58 changes: 39 additions & 19 deletions subsys/logging/log_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <logging/log_output.h>
#include <misc/printk.h>
#include <assert.h>
#include <atomic.h>
#include <stdio.h>

#ifndef CONFIG_LOG_PRINTK_MAX_STRING_LENGTH
Expand All @@ -23,10 +24,16 @@ LOG_BACKEND_UART_DEFINE(log_backend_uart);
#endif

static struct log_list_t list;
static atomic_t initialized;
static bool panic_mode;
static bool initialized;

static timestamp_get_t timestamp_func;
static u32_t dummy_timestamp(void);
static timestamp_get_t timestamp_func = dummy_timestamp;

static u32_t dummy_timestamp(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is timestamp now mandatory? If not, why we have to have default function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is mandatory. and actually there are 2 default functions. Dummy_timestamp is used between log_core_init() and log_init(). Then timestamp which is using k_cycle_get_32 is used and it can be later on replaced by a user function. k_cycle_get_32 cannot be used from the start because it may not be configured/available then.

{
return 0;
}

static inline void msg_finalize(struct log_msg *msg,
struct log_msg_ids src_level)
Expand Down Expand Up @@ -130,10 +137,6 @@ int log_printk(const char *fmt, va_list ap)
struct log_msg *msg;
int length;

if (!initialized) {
log_init();
}

length = vsnprintf(formatted_str,
sizeof(formatted_str), fmt, ap);

Expand Down Expand Up @@ -173,25 +176,43 @@ static u32_t timestamp_get(void)
return k_cycle_get_32();
}

int log_init(void)
void log_core_init(void)
{
log_msg_pool_init();
log_list_init(&list);

/* No backends attached so far but set default level as a filter for
* any source of logging in the system. When backends are attached later
* logs will be filtered out during processing.
*/
if (IS_ENABLED(CONFIG_LOG_RUNTIME_FILTERING)) {
for (int i = 0; i < log_sources_count(); i++) {
u32_t *filters = log_dynamic_filters_get(i);

LOG_FILTER_SLOT_SET(filters,
LOG_FILTER_AGGR_SLOT_IDX,
CONFIG_LOG_DEFAULT_LEVEL);
}
}
}

void log_init(void)
Copy link
Member

Choose a reason for hiding this comment

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

This func can be called multiple times now. Will it cause any issues if done so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added protection

{
assert(log_backend_count_get() < LOG_FILTERS_NUM_OF_SLOTS);
int i;

if (atomic_inc(&initialized)) {
return;
}

/* Set default timestamp. */
timestamp_func = timestamp_get;
log_output_timestamp_freq_set(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC);

if (!initialized) {
log_list_init(&list);

/* Assign ids to backends. */
for (int i = 0; i < log_backend_count_get(); i++) {
log_backend_id_set(log_backend_get(i),
i + LOG_FILTER_FIRST_BACKEND_SLOT_IDX);
}

panic_mode = false;
initialized = true;
/* Assign ids to backends. */
for (i = 0; i < log_backend_count_get(); i++) {
log_backend_id_set(log_backend_get(i),
i + LOG_FILTER_FIRST_BACKEND_SLOT_IDX);
}

#ifdef CONFIG_LOG_BACKEND_UART
Expand All @@ -200,7 +221,6 @@ int log_init(void)
NULL,
CONFIG_LOG_DEFAULT_LEVEL);
#endif
return 0;
}

int log_set_timestamp_func(timestamp_get_t timestamp_getter, u32_t freq)
Expand Down
9 changes: 8 additions & 1 deletion subsys/logging/log_msg.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@
#define MSG_SIZE sizeof(union log_msg_chunk)
#define NUM_OF_MSGS (CONFIG_LOG_BUFFER_SIZE / MSG_SIZE)

K_MEM_SLAB_DEFINE(log_msg_pool, MSG_SIZE, NUM_OF_MSGS, sizeof(u32_t));
struct k_mem_slab log_msg_pool;
static u8_t __noinit __aligned(sizeof(u32_t))
log_msg_pool_buf[CONFIG_LOG_BUFFER_SIZE];

void log_msg_pool_init(void)
{
k_mem_slab_init(&log_msg_pool, log_msg_pool_buf, MSG_SIZE, NUM_OF_MSGS);
}

void log_msg_get(struct log_msg *msg)
{
Expand Down
5 changes: 2 additions & 3 deletions tests/subsys/logging/log_core/src/log_core_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,8 @@ static void log_setup(bool backend2_enable)
{
stamp = 0;

zassert_equal(0,
log_init(),
"Expects successful initialization.");
log_init();

zassert_equal(0, log_set_timestamp_func(timestamp_get, 0),
"Expects successful timestamp function setting.");

Expand Down