esp/modesp.c: Add osdebug() and oslog() #112

ak15199 · 2017-06-15T02:47:15Z

Code lineage:

osdebug() is based loosely on the version in esp8266, but there didn't
seem to be an obvious way of choosing a particular UART. The basic
behavior is the same, though: provide None, and logging is disabled;
provide an integer and logging is restored to the default level.

To build on that, and because the IDF provides more functionality, a
second parameter has now been implemented which allows the active log
level to be set:

esp.osdebug(uart[, level])

The module has a corresponding set of LOG_ values to set this
accordingly. An additional level esp.LOG_CRITICAL, which maps to
esp.LOG_ERROR has been included in order to provide better
symetry with standard Python logging frameworks.

In order to test out correct behavior, it was straightforward to add a
second function that calls the IDF to perform a logging operation:

esp.oslog(level, tag, message)

Because our intent to write anything in Python that can be in Python,
the thought is that format and parameters will be transformed to a
simple string in Python, to them be passed here for rendering.

Code lineage: osdebug() is based loosely on the version in esp8266, but there didn't seem to be an obvious way of choosing a particular UART. The basic behavior is the same, though: provide None, and logging is disabled; provide an integer and logging is restored to the default level. To build on that, and because the IDF provides more functionality, a second parameter has now been implemented which allows the active log level to be set: esp.osdebug(uart[, level]) The module has a corresponding set of LOG_ values to set this accordingly. An additional level esp.LOG_CRITICAL, which maps to esp.LOG_ERROR has been included in order to provide better symetry with standard Python logging frameworks. In order to test out correct behavior, it was straightforward to add a second function that calls the IDF to perform a logging operation: esp.oslog(level, tag, message) Because our intent to write anything in Python that can be in Python, the thought is that format and parameters will be transformed to a simple string in Python, to them be passed here for rendering.

ak15199 · 2017-06-15T03:27:32Z

I wanted to be able to switch off logging to the console, and was disappointed to see esp.osdebug() missing. I mean, esp32 is an evolving platform, so it's totally fair, but I still wanted it.

There are a couple of other things that I want to work on, but this one seemed like relatively low hanging fruit that I could use to learn the platform. Hopefully I haven't made too many rookie mistakes along the way, and that I've observed style guidelines and pull request convention, but let me know if I need to do anything differently in the future.

dpgeorge · 2017-06-15T08:19:41Z

esp32/modesp.c

+
+    return mp_const_none;
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(esp_osdebug_obj, 1, 2, esp_osdebug);


Wouldn't it be better to have this function just take 1 arg, esp.osdebug(level), same as esp8266, and just have level=None mean disable, and any other value is an integer that is passed as the second arg to esp_log_level_set?

In the case of the ESP32 it doesn't really allow you to redirect errors to different UARTs (?) so I'd say it's fine to repurpose the single argument to this function to be the log level.

Maybe? You're the maintainer, so get the final say.

The reasoning for the additional parameter is twofold: that repurposing the existing argument would break backwards compatibility for anyone migrating code from ESP8266 in a minor but unpredictable way; the other is that although I can't see how to do it yet, the ESP32 does have two UARTs, and the hardware allows developers to change pin assignments for how UARTs are routed... it may be that at some point this would become possible.

But would appreciate your thoughts.

Ok, points taken. But I think the implementation should be adjusted so that passing None as the first arg always disables logging, even if there are 2 args passed (eg esp.osdebug(None, 123)).

dpgeorge · 2017-06-15T08:28:38Z

esp32/modesp.c

+
+    return mp_const_none;
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_3(esp_oslog_obj, esp_oslog);


This is a neat function, but is it useful? What was your use case for it?

In Python you can just use "print" to write logging info, or use the logging module (which is already ported to MicroPython).

You're absolutely right... there are other ways of delivering text. I think that use cases for print and logging are different though. I like the way that (regular) Python at least allows for different types of endpoints. If the endpoint is the console, then it occurred to me (after I'd added the thing for testing) that it might be less jarring to present log output in a manner consistent with the OS (in terms of tags, colors, and so on).

(so basically a shim between the python logging module and the OS)

Again, though, I'll leave you to decide.

dpgeorge · 2017-06-15T08:30:17Z

esp32/modesp.c

+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_DEBUG), MP_OBJ_NEW_SMALL_INT((mp_uint_t)ESP_LOG_DEBUG)},
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_VERBOSE), MP_OBJ_NEW_SMALL_INT((mp_uint_t)ESP_LOG_VERBOSE)},
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_DEFAULT), MP_OBJ_NEW_SMALL_INT((mp_uint_t)LOG_LOCAL_LEVEL)},
+#endif


No need to have this protected by MODESP_INCLUDE_CONSTANTS.

That did seem a bit weird. Thanks for the pointer.

dpgeorge · 2017-06-15T08:30:27Z

esp32/modesp.c

+
+#if MODESP_INCLUDE_CONSTANTS
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_NONE), MP_OBJ_NEW_SMALL_INT((mp_uint_t)ESP_LOG_NONE)},
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_CRITICAL), MP_OBJ_NEW_SMALL_INT((mp_uint_t)ESP_LOG_ERROR)},


Should this be ESP_LOG_CRITICAL?

The levels are defined as an enum and typedef in esp-idf/components/log/include/esp_log.h, and there isn't a ESP_LOG_CRITICAL. I wanted to provide some equivalence for the Python abstraction, though: the absence of the level in the base OS, shouldn't be something that your casual developer needs to worry about.

dpgeorge · 2017-06-15T08:30:45Z

esp32/modesp.c

@@ -101,6 +146,17 @@ STATIC const mp_rom_map_elem_t esp_module_globals_table[] = {

    { MP_ROM_QSTR(MP_QSTR_neopixel_write), MP_ROM_PTR(&esp_neopixel_write_obj) },
    { MP_ROM_QSTR(MP_QSTR_dht_readinto), MP_ROM_PTR(&dht_readinto_obj) },
+
+#if MODESP_INCLUDE_CONSTANTS
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LOG_NONE), MP_OBJ_NEW_SMALL_INT((mp_uint_t)ESP_LOG_NONE)},


You should use MP_ROM_QSTR and MP_ROM_INT for the macro wrappers.

Nice! Thank you!

dpgeorge · 2017-06-15T08:35:14Z

I reviewed the code before reading the above comments, sorry!

An additional level esp.LOG_CRITICAL, which maps to
esp.LOG_ERROR has been included in order to provide better
symetry with standard Python logging frameworks.

IMO this is not necessary. The less constants in the esp module the better.

Because our intent to write anything in Python that can be in Python,
the thought is that format and parameters will be transformed to a
simple string in Python, to them be passed here for rendering.

With that philosophy (which is a good philosophy) one should use the Python logging module, not have something extra like oslog.

dpgeorge · 2017-06-15T08:36:23Z

I wanted to be able to switch off logging to the console, and was disappointed to see esp.osdebug() missing. I mean, esp32 is an evolving platform, so it's totally fair, but I still wanted it.

Yes, I agree, the info messages can get annoying (not to mention mess up scripts/tests that don't expect them). So the addition of esp.osdebug is certainly welcome, thanks!

ak15199 · 2017-06-17T20:44:52Z

I just commented on your original comments before I saw your latest messages. Unless I hear otherwise, I'll remove oslog from the pull request. Thanks for your attention, Damien!

dpgeorge · 2017-11-29T04:27:38Z

This PR was reworked a little to make esp.osdebug(None) always disable the log even if a second arg is given, and then merged in 84035f0

Thanks @ak15199 for the contribution.

dpgeorge reviewed Jun 15, 2017

View reviewed changes

esp32/modesp.c updates for logging pull request per dpgeorge

e614378

dpgeorge mentioned this pull request Oct 4, 2017

Telnet to esp32 #191

Closed

dpgeorge closed this Nov 29, 2017

dpgeorge mentioned this pull request Nov 29, 2017

Hard time to upload files with ampy and related sw #165

Closed

nickzoic mentioned this pull request May 24, 2018

ports/esp32: silence ESP-IDF log messages when in raw REPL mode micropython/micropython#3817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp/modesp.c: Add osdebug() and oslog() #112

esp/modesp.c: Add osdebug() and oslog() #112

ak15199 commented Jun 15, 2017

ak15199 commented Jun 15, 2017

dpgeorge Jun 15, 2017

dpgeorge Jun 15, 2017

ak15199 Jun 17, 2017

dpgeorge Jun 19, 2017

dpgeorge Jun 15, 2017

dpgeorge Jun 15, 2017

ak15199 Jun 17, 2017 •

edited

Loading

dpgeorge Jun 15, 2017

ak15199 Jun 17, 2017

dpgeorge Jun 15, 2017

ak15199 Jun 17, 2017

dpgeorge Jun 15, 2017

ak15199 Jun 17, 2017

dpgeorge commented Jun 15, 2017

dpgeorge commented Jun 15, 2017

ak15199 commented Jun 17, 2017

dpgeorge commented Nov 29, 2017

esp/modesp.c: Add osdebug() and oslog() #112

esp/modesp.c: Add osdebug() and oslog() #112

Conversation

ak15199 commented Jun 15, 2017

ak15199 commented Jun 15, 2017

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

ak15199 Jun 17, 2017 • edited Loading

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

dpgeorge commented Jun 15, 2017

dpgeorge commented Jun 15, 2017

ak15199 commented Jun 17, 2017

dpgeorge commented Nov 29, 2017

ak15199 Jun 17, 2017 •

edited

Loading