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

martirius · 2021-02-21T13:15:41Z

With this PR user can select to use the original ESP-IDF log instead of the redefined one.

User can also redefine the log function as per Logging Library so he can for example redirect logs to a file.

To enable this change just add -DUSE_ESP32_LOG to build flags.
User can also change the default TAG (that now is ES32) to whatever it wants adding '-DTAG="tag_value"' to build flags

…uino HAL

me-no-dev · 2021-02-22T17:42:14Z

cores/esp32/esp32-hal-log.h

+#ifdef USE_ESP32_LOG
+
+#ifndef TAG
+#define TAG "ESP32"


using TAG like "arduino" makes more sense here

No problem , i will change it for sure :)

me-no-dev · 2021-02-22T18:05:09Z

cores/esp32/esp32-hal-log.h

@@ -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


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

You are right, let me change it

me-no-dev · 2021-02-22T18:09:20Z

cores/esp32/esp32-hal-log.c

+#define __MY_LOG__
+#include "stdio.h"
+#include "esp32-hal-log.h"
+#ifdef USE_ESP32_LOG


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.

me-no-dev · 2021-02-22T18:14:25Z

cores/esp32/esp32-hal-log.c

+        int len = vsnprintf(log_buffer, sizeof(log_buffer), format, va_args);
+        if (len > 0)
+        {
+            switch (level)


this switch can be replaced by the following line:

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

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.

by the way, the following should also work:

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

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

me-no-dev · 2021-02-22T18:16:12Z

cores/esp32/esp32-hal-log.c

+#include "stdio.h"
+#include "esp32-hal-log.h"
+#ifdef USE_ESP32_LOG
+    void log_to_esp(esp_log_level_t level, const char *format, ...)


please pass also the TAG as argument:

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

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)

martirius · 2021-02-22T18:50:37Z

@me-no-dev I have noticed a strange behaviour; if i enable the esp-idf logs and lower the define of CORE_DEBUG_LEVEL to a value under DEBUG (4), the code doesn't compile anymore and this message is shown:
.platformio/packages/framework-arduinoespressif32/libraries/WiFi/src/WiFiSTA.cpp:738:25: error: 'sc_status_strings' was not declared in this scope.
However if I don't enable my defined log_* and use the arduino log, the code compile.

I checked that line that raises the error but i cannot understand why it compilessince it's calling an undefined variable

me-no-dev · 2021-02-22T19:10:17Z

Because you also need to make the defines based on the log level selected:

#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_ERROR
#define log_e(format, ...) do {log_to_esp(ESP_LOG_ERROR, TAG, format, ##__VA_ARGS__);}while(0)
#define isr_log_e(format, ...) do {ets_printf(LOG_FORMAT(E, format), esp_log_timestamp(), tag, ##__VA_ARGS__);}while(0)
#else
#define log_e(format, ...)
#define isr_log_e(format, ...)
#endif

Changed TAG definition to "arduino" Removed switch in log_to_esp and used ESP_LOG_LEVEL_LOCAL Added missing isr_log_* definitions FIxed conditional definitions of log MACROs

jfbuggen · 2021-04-15T15:55:16Z

Many thanks for developing the link with the ESP IDF log.

I have been looking at the code (as I had a buggy implementation of my own vprintf to install via esp_log_set_vprintf...) and was wondering why it is necessary to create the log_to_esp function in esp32-hal-log.c, instead if directly referring to ESP_LOG_LEVEL_LOCAL (which ultimately calls esp_log_write) in the definition of log_x macros in esp32-hal-log.h like this:

#define log_n(format, ...) ESP_LOG_LEVEL_LOCAL(ESP_LOG_ERROR, TAG, format, ##VA_ARGS)

I made some tests and it looks to work very fine like that.

It spares a function call, the 512b buffer of log_to_esp (esp_log_write in log.c is more prudent with a 64b buffer and malloc if bigger size is necessary) and a call to vsnprintf (esp_log_write in log.c already calls it twice).

martirius · 2021-04-15T16:52:11Z

Hi @jfbuggen, thanks for appreciating.
I'm honest, i don't remember why i took this decision, however you are free to made a PR to with your proposed solution :)

lbernstone · 2021-04-15T18:18:55Z

If your method makes it easier to redirect with esp_log_set_vprintf, then by all means submit it.

jfbuggen · 2021-04-18T14:31:53Z

Thank you @martirius. I will have to learn how to do that on GitHub but I will have a try.

@lbernstone it will not change much in terms of functionality, esp_log_set_vprintf works fine already with the current code, it brings just a small performance improvement.

…than using intermediate function log_to_esp As indicated in espressif#4845 (comment) it is more efficient to call directly the ESP LOG macros. This spares a function call, a 512b buffer and a call to vsnprintf. No change in functionality.

…than using intermediate function log_to_esp (#5081) As indicated in #4845 (comment) it is more efficient to call directly the ESP LOG macros. This spares a function call, a 512b buffer and a call to vsnprintf. No change in functionality.

cyberman54 · 2022-02-26T18:51:27Z

Even after this merge esp32-idf logging is still not usable, because of lots of conflicts with TAG macro in libraries, e.g.

packages/framework-arduinoespressif32/libraries/WiFiClientSecure/src/esp_crt_bundle.c:25:20: note: in expansion of macro 'TAG'
 static const char *TAG = "esp-x509-crt-bundle";
                    ^~~

/cc @coratron @lbernstone @martirius

zekageri · 2022-04-21T17:05:06Z

Any news on this?

VojtechBartoska · 2022-04-22T07:00:04Z

Hello @zekageri and @cyberman54, if this implementation doesn't work properly, please consider opening a new issue with behavior description. Thanks.

zekageri · 2022-04-22T07:09:24Z

Thank you for your response. I will test it soon.

Mattia Pini added 2 commits February 21, 2021 12:50

Added possibility to use ESP32-IDF log insted of redefined one by Ard…

9767e5e

…uino HAL

Fixed CI

3e60f6c

martirius mentioned this pull request Feb 22, 2021

ESP LOG redirect to own function bug. #4346

Closed

me-no-dev reviewed Feb 22, 2021

View reviewed changes

Renamed USE_ESP32_LOG to USE_ESP_IDF_LOG

b49f056

Changed TAG definition to "arduino" Removed switch in log_to_esp and used ESP_LOG_LEVEL_LOCAL Added missing isr_log_* definitions FIxed conditional definitions of log MACROs

me-no-dev merged commit b8dab5e into espressif:master Feb 22, 2021

jfbuggen mentioned this pull request Apr 19, 2021

In esp32-hal-log, direct calls to ESP_LOG_x macros is more efficient … #5081

Merged

coratron mentioned this pull request Dec 1, 2021

System logs not working as expected ( BUG + broken esp_log_set_vprintf ) #5954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

martirius commented Feb 21, 2021

me-no-dev Feb 22, 2021

martirius Feb 22, 2021

me-no-dev Feb 22, 2021

martirius Feb 22, 2021

me-no-dev Feb 22, 2021

martirius Feb 22, 2021

me-no-dev Feb 22, 2021

martirius Feb 22, 2021 •

edited

me-no-dev Feb 22, 2021

martirius Feb 22, 2021

me-no-dev Feb 22, 2021

me-no-dev Feb 22, 2021

martirius Feb 22, 2021

martirius commented Feb 22, 2021

me-no-dev commented Feb 22, 2021

jfbuggen commented Apr 15, 2021

martirius commented Apr 15, 2021

lbernstone commented Apr 15, 2021

jfbuggen commented Apr 18, 2021

cyberman54 commented Feb 26, 2022

zekageri commented Apr 21, 2022

VojtechBartoska commented Apr 22, 2022

zekageri commented Apr 22, 2022

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

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

Conversation

martirius commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martirius Feb 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martirius commented Feb 22, 2021

me-no-dev commented Feb 22, 2021

jfbuggen commented Apr 15, 2021

martirius commented Apr 15, 2021

lbernstone commented Apr 15, 2021

jfbuggen commented Apr 18, 2021

cyberman54 commented Feb 26, 2022

zekageri commented Apr 21, 2022

VojtechBartoska commented Apr 22, 2022

zekageri commented Apr 22, 2022

martirius Feb 22, 2021 •

edited