Skip to content

Added possibility to use ESP32-IDF log insted of redefined one #4845

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Added possibility to use ESP32-IDF log insted of redefined one #4845

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9767e5e
Added possibility to use ESP32-IDF log insted of redefined one by Ard…
Feb 21, 2021
3e60f6c
Fixed CI
Feb 21, 2021
b49f056
Renamed USE_ESP32_LOG to USE_ESP_IDF_LOG
Feb 22, 2021
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
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ set(CORE_SRCS
cores/esp32/esp32-hal-dac.c
cores/esp32/esp32-hal-gpio.c
cores/esp32/esp32-hal-i2c.c
cores/esp32/esp32-hal-log.c
cores/esp32/esp32-hal-ledc.c
cores/esp32/esp32-hal-matrix.c
cores/esp32/esp32-hal-misc.c
Expand Down
42 changes: 42 additions & 0 deletions cores/esp32/esp32-hal-log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#ifndef __MY_LOG__
#define __MY_LOG__
#include "stdio.h"
#include "esp32-hal-log.h"
#ifdef USE_ESP32_LOG
Copy link
Member

Choose a reason for hiding this comment

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

you can remove the #ifdef switch. Function will not be included in the final binary if not called within the application and will at the same time allow USE_ESP32_LOG to be defined on per file basis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! :)

void log_to_esp(esp_log_level_t level, const char *format, ...)
Copy link
Member

Choose a reason for hiding this comment

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

please pass also the TAG as argument:

void log_to_esp(esp_log_level_t level, const char *tag, const char *format, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here i wanted to replicate the actual behavior of the arduino library, so the tag is fixed.
But I understand it's better to put it as a parameter ( and passing it fixed when calling MACROs)

{
va_list va_args;
va_start(va_args, format);

char log_buffer[512];
int len = vsnprintf(log_buffer, sizeof(log_buffer), format, va_args);
if (len > 0)
{
switch (level)
Copy link
Member

Choose a reason for hiding this comment

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

this switch can be replaced by the following line:

ESP_LOG_LEVEL_LOCAL(level, tag, "%s", log_buffer);

Copy link
Contributor Author

@martirius martirius Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It was my decision to doesn't call directly ESP_LOG_LEVEL_LOCAL, instead using the provided MACROs.
Bu i agree that the switch is resolved in this way.

Copy link
Member

Choose a reason for hiding this comment

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

by the way, the following should also work:

ESP_LOG_LEVEL_LOCAL(level, tag, (const char *)log_buffer);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in this way the compiler says :
expression preceding parentheses of apparent call must have (pointer-to-) function typeC/C++(109)

Copy link
Member

Choose a reason for hiding this comment

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

ok :)

{
case ESP_LOG_ERROR:
ESP_LOGE(TAG, "%s", log_buffer);
break;
case ESP_LOG_DEBUG:
ESP_LOGD(TAG, "%s", log_buffer);
break;
case ESP_LOG_WARN:
ESP_LOGW(TAG, "%s", log_buffer);
break;
case ESP_LOG_INFO:
ESP_LOGI(TAG, "%s", log_buffer);
break;
case ESP_LOG_VERBOSE:
ESP_LOGV(TAG, "%s", log_buffer);
break;
case ESP_LOG_NONE:
//do nothing
break;
}
}

va_end(va_args);
}
#endif
#endif

25 changes: 23 additions & 2 deletions cores/esp32/esp32-hal-log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2015-2021 Espressif Systems (Shanghai) PTE LTD
// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -11,7 +11,6 @@
// 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.

#ifndef __ARDUHAL_LOG_H__
#define __ARDUHAL_LOG_H__

Expand All @@ -37,6 +36,9 @@ extern "C"
#define ARDUHAL_LOG_LEVEL CONFIG_ARDUHAL_LOG_DEFAULT_LEVEL
#else
#define ARDUHAL_LOG_LEVEL CORE_DEBUG_LEVEL
#ifdef USE_ESP32_LOG
Copy link
Member

Choose a reason for hiding this comment

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

I think that renaming the name to USE_ESP_IDF_LOG would describe a bit better what the switch does :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, let me change it

#define LOG_LOCAL_LEVEL CORE_DEBUG_LEVEL
#endif
#endif

#ifndef CONFIG_ARDUHAL_LOG_COLORS
Expand Down Expand Up @@ -72,12 +74,15 @@ extern "C"
#define ARDUHAL_LOG_RESET_COLOR
#endif



const char * pathToFileName(const char * path);
int log_printf(const char *fmt, ...);

#define ARDUHAL_SHORT_LOG_FORMAT(letter, format) ARDUHAL_LOG_COLOR_ ## letter format ARDUHAL_LOG_RESET_COLOR "\r\n"
#define ARDUHAL_LOG_FORMAT(letter, format) ARDUHAL_LOG_COLOR_ ## letter "[" #letter "][%s:%u] %s(): " format ARDUHAL_LOG_RESET_COLOR "\r\n", pathToFileName(__FILE__), __LINE__, __FUNCTION__

#ifndef USE_ESP32_LOG
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_VERBOSE
#define log_v(format, ...) log_printf(ARDUHAL_LOG_FORMAT(V, format), ##__VA_ARGS__)
#define isr_log_v(format, ...) ets_printf(ARDUHAL_LOG_FORMAT(V, format), ##__VA_ARGS__)
Expand Down Expand Up @@ -125,9 +130,24 @@ int log_printf(const char *fmt, ...);
#define log_n(format, ...)
#define isr_log_n(format, ...)
#endif
#endif

#include "esp_log.h"

#ifdef USE_ESP32_LOG

#ifndef TAG
#define TAG "ESP32"
Copy link
Member

Choose a reason for hiding this comment

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

using TAG like "arduino" makes more sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem , i will change it for sure :)

#endif
void log_to_esp(esp_log_level_t level, const char* format, ...);

#define log_e(format, ...) do {log_to_esp(ESP_LOG_ERROR, format, ##__VA_ARGS__);}while(0)
#define log_w(format, ...) do {log_to_esp(ESP_LOG_WARN, format, ##__VA_ARGS__);}while(0)
#define log_d(format, ...) do {log_to_esp(ESP_LOG_DEBUG, format, ##__VA_ARGS__);}while(0)
#define log_i(format, ...) do {log_to_esp(ESP_LOG_INFO, format, ##__VA_ARGS__);}while(0)
#define log_v(format, ...) do {log_to_esp(ESP_LOG_VERBOSE, format, ##__VA_ARGS__);}while(0)
//#define log_n(format, ...) myLog(ESP_LOG_NONE, format, ##__VA_ARGS__)
#else
#ifdef CONFIG_ARDUHAL_ESP_LOG
#undef ESP_LOGE
#undef ESP_LOGW
Expand All @@ -151,6 +171,7 @@ int log_printf(const char *fmt, ...);
#define ESP_EARLY_LOGD(tag, ...) isr_log_d(__VA_ARGS__)
#define ESP_EARLY_LOGV(tag, ...) isr_log_v(__VA_ARGS__)
#endif
#endif

#ifdef __cplusplus
}
Expand Down