esp32/mphalport: Add _FUNCTION_, _LINE_, _FILE_ info to check_esp_err(). #10888

IhorNehrutsa · 2023-02-28T21:32:44Z

This PR can be useful for ESP32 developers.

Now check_esp_err() raises an exception without a location in the source code.

Traceback (most recent call last):
  File "<stdin>", line 8, in <module>
OSError: (-258, 'ESP_ERR_INVALID_ARG')

This PR adds debugging information to the exception and will be useful during code development.
Info about FUNCTION, LINE, FILE may be added.

During the development change the

micropython/ports/esp32/mpconfigport.h

Line 50 in 3637252

#define MICROPY_ERROR_REPORTING (MICROPY_ERROR_REPORTING_NORMAL)

to the
#define MICROPY_ERROR_REPORTING (MICROPY_ERROR_REPORTING_DETAILED)

REPL output may look like

Exception from function 'configure_channel' at line 233 in file './machine_pwm.c'
Traceback (most recent call last):
  File "<stdin>", line 8, in <module>
OSError: (-258, 'ESP_ERR_INVALID_ARG')

or in a different way.
Discussion is welcome.

Edited:
If MICROPY_ERROR_REPORTING is equal to the MICROPY_ERROR_REPORTING_NORMAL during final compilation
then the size of the binary code is not increased.

If MICROPY_ERROR_REPORTING is equal to the MICROPY_ERROR_REPORTING_DETAILED during development
then FUNCTION, LINE, FILE information is added to the output.

Edited:
With last commit output like:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: -258, 0x0102, 'ESP_ERR_INVALID_ARG' in function 'set_duty_u16' at line 342 in file './machine_pwm.c'

Edited:
With last commit output like:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: (-258, "0x0102 ESP_ERR_INVALID_ARG in function 'set_duty_u16' at line 342 in file './machine_pwm.c'")

IhorNehrutsa · 2023-07-18T05:35:28Z

@robert-hh
May you review this PR?
Thanks

robert-hh · 2023-07-21T07:20:03Z

I see that it makes life easier to tell, where an error occurred. It increase the code size. For a GENERIC build it is 2080 bytes, or 0.12%. This seems not much, especially in relation, but Damien refused to include the Pin.cpu table just because of an even smaller code size increase.

IhorNehrutsa · 2023-07-21T07:52:16Z

Thanks @robert-hh
Latest commit reduce added code size.
PR adds only one print line:

mp_printf(MP_PYTHON_PRINTER, "Exception in function '%s' at line %d in file '%s'", func, line, file);

I think that this is a small fee for convenience in development and informativeness when using.
Output like:

Exception from function 'configure_channel' at line 233 in file './machine_pwm.c'
Traceback (most recent call last):
  File "<stdin>", line 8, in <module>
OSError: (-258, 'ESP_ERR_INVALID_ARG')

Could you draw @dpgeorge attention to this PR?
Thanks a lot.

robert-hh · 2023-07-22T11:51:15Z

The code size increase dropped from 2080 bytes to 1792 bytes. At ESP32 ports there usually a lot of flash space available, and RTOS seems to use lot of it. The additional information is useful, but most of the time needed during development. So maybe you could use a DEBUG type switch to enable it.

IhorNehrutsa · 2023-07-24T06:43:55Z

@dpgeorge
May add to the https://github.com/micropython/micropython/milestone/4 ?

IhorNehrutsa · 2023-09-04T08:06:27Z

@andrewleech
Could you please review this PR?
Thanks.

P.S. I hope I haven't been too pushy lately.

andrewleech · 2023-09-04T09:34:17Z

@andrewleech
Could you please review this PR?
Thanks.

P.S. I hope I haven't been too pushy lately.

Yeah PR requests really aren't standard practice around here... but if you'd like to review some of my PR's I'd be happy to return the favour ;-)

That being said I rarely go near esp hardware so don't know that much about the port.

andrewleech · 2023-09-04T09:42:48Z

In this case, adding this kind of debug information takes a lot of flash space which only really provides benefit to C developers, not the general micropython users.

ports/esp32/mphalport.h

IhorNehrutsa · 2023-09-04T09:46:37Z

From #12346 (comment)

However another thing that helps community immensely is for the people who submit pr's to also code review other pr's and assist in resolving issues, this can seriously reduce the load on the core team! Some of the people on this thread do a huge amount of this already... I and others could do more.

Yeah PR requests really aren't standard practice around here... but if you'd like to review some of my PR's I'd be happy to return the favour ;-)

I think we have an agreement.
Yes, You can request me.

IhorNehrutsa · 2023-09-07T06:08:50Z

@robert-hh @andrewleech

If MICROPY_ERROR_REPORTING is equal to the MICROPY_ERROR_REPORTING_NORMAL during final compilation
then the size of the binary code is not increased.

If MICROPY_ERROR_REPORTING is equal to the MICROPY_ERROR_REPORTING_DETAILED during development
then FUNCTION, LINE, FILE information is added to the output.

Thanks

IhorNehrutsa · 2023-09-08T05:39:50Z

@jimmo @DvdGiessen @UnexpectedMaker @projectgus
Hi all.
I saw that you have a lot of esp32 PRs accepted.
Could you be interested in this PR?
Sorry if I'm being too pushy.
Thank you very much.

projectgus

This looks potentially very useful to me, thanks @IhorNehrutsa . I only have one major concern, see the inline comment.

ports/esp32/mphalport.c

ports/esp32/mphalport.h

projectgus

LGTM with the change to include the information in the OSError itself. Thanks @IhorNehrutsa!

projectgus · 2023-09-13T05:15:01Z

@IhorNehrutsa The CI commit message check is failing on the follow-up commit because the Sign-off has to be at the end. Could you squash these to one commit, please?

I appreciate the Co-Author credit but I'm happy for the commit to just be from you, you did all the work. :)

IhorNehrutsa · 2023-09-13T05:42:01Z

The CI commit message check is failing on the follow-up commit because the Sign-off has to be at the end.

I use GitHub Desktop. Sometimes it behaves strangely.

Thanks @projectgus

dpgeorge · 2023-09-14T02:35:35Z

The raised OSError should have as its first argument an integer error code. The second argument is optional and can be a descriptive string.

So this PR needs to be changed so that the function/line/file info is added to the string that is the second argument. You'll need to keep the nlr_raise(mp_obj_exception_make_new(&mp_type_OSError, 2, 0, args)); part, but make sure args[1] points to a more comprehensive string with the function/line/file.

For example the output should be something like:

OSError: (-258, "ESP_ERR_INVALID_ARG in function 'set_duty_u16' at line 342 in file './machine_pwm.c'")

IhorNehrutsa · 2023-09-14T09:08:47Z

@dpgeorge
With the last commit, I get abnormal output

Exception in function 'set_duty_u16' at line 342 in file './machine_pwm.c'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: (-258, 'ESP_ERR_INVALID_ARG', "0x0102, in function 'set_duty_u1|*\x08@?\x00@0\n\x06\x00\x19\t\x00\n\x00?\x00\x00\x00\x00#\n\x06\x00 \n\x06\x00\x00\x00?\x01\x00\x00\x00")

It looks like the bug inside the snprintf()

dpgeorge · 2023-09-14T13:57:21Z

ports/esp32/mphalport.c

+        char err_msg[64];
+        esp_err_to_name_r(code, err_msg, sizeof(err_msg));
+        char str[256];
+        snprintf(str, sizeof(str), MP_ERROR_TEXT("0x%04X, in function '%s' at line %d in file '%s'"), code, func, line, file);


snprintf won't work with MP_ERROR_TEXT. Just remove the MP_ERROR_TEXT macro.

dpgeorge · 2023-09-14T13:57:45Z

ports/esp32/mphalport.c

+            o_str2->len = strlen((char *)o_str2->data);
+            o_str2->hash = qstr_compute_hash(o_str2->data, o_str2->len);
+            // raise
+            mp_obj_t args[3] = { MP_OBJ_NEW_SMALL_INT(pcode), MP_OBJ_FROM_PTR(o_str), MP_OBJ_FROM_PTR(o_str2)};


I suggest to just have two arguments, the pcode and then an error message string.

IhorNehrutsa · 2023-09-14T15:22:15Z

With the last commit, it looks as expected

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: (-258, "0x0102 ESP_ERR_INVALID_ARG in function 'set_duty_u16' at line 342 in file './machine_pwm.c'")

Thanks @dpgeorge

dpgeorge · 2023-09-15T01:30:02Z

ports/esp32/mphalport.c

+        esp_err_to_name_r(code, err_msg, sizeof(err_msg));
+        char str[512]; // 256 bytes size leeds to abnormal output
+        snprintf(str, sizeof(str), "0x%04X %s in function '%s' at line %d in file '%s'", code, err_msg, func, line, file);
+        o_str->data = (const byte *)str;


This will not work because the str object is taking a pointer to the C stack, and the data on the C stack will not be valid once this function returns. Instead, the string data must be dynamically allocated here. Probably easiest to use vstr_printf for that.

ports/esp32/mphalport.c

dpgeorge · 2023-09-15T06:33:47Z

ports/esp32/mphalport.c

+        char err_msg[64];
+        esp_err_to_name_r(code, err_msg, sizeof(err_msg));
+        vstr_t vstr;
+        vstr_init(&vstr, 256);


256 is quite large. I suggest something smaller like 80.

dpgeorge · 2023-09-15T06:34:37Z

ports/esp32/mphalport.c

+        vstr_t vstr;
+        vstr_init(&vstr, 256);
+        vstr_printf(&vstr, "0x%04X %s in function '%s' at line %d in file '%s'", code, err_msg, func, line, file);
+        o_str->data = (const byte *)vstr.buf;


You need to use vstr_null_terminated to access the buffer and make sure it's null terminated.

Currently, check_esp_err() raises an exception without a location in the source code, eg: Traceback (most recent call last): File "<stdin>", line 8, in <module> OSError: (-258, 'ESP_ERR_INVALID_ARG') This commit allows additional error reporting (function, line and file) to be enabled via detailed exceptions. Change the error reporting config to #define MICROPY_ERROR_REPORTING (MICROPY_ERROR_REPORTING_DETAILED) and then exception messages from IDF errors look like: Traceback (most recent call last): File "<stdin>", line 1, in <module> OSError: (-258, "0x0102 ESP_ERR_INVALID_ARG in function 'set_duty_u16' at line 342 in file './machine_pwm.c'") Signed-off-by: Ihor Nehrutsa <IhorNehrutsa@gmail.com>

dpgeorge · 2023-09-18T01:51:51Z

Thanks for updating based on the feedback. This is now merged.

IhorNehrutsa · 2023-09-18T05:38:14Z

Thanks @robert-hh @andrewleech @projectgus @dpgeorge

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch from feaf5f5 to f97c02b Compare March 9, 2023 14:05

dpgeorge added the port-esp32 label Apr 11, 2023

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch 3 times, most recently from df39831 to 3257d02 Compare July 24, 2023 06:42

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch 2 times, most recently from 83d5c05 to ca2ec2f Compare September 4, 2023 07:52

andrewleech reviewed Sep 4, 2023

View reviewed changes

ports/esp32/mphalport.h Show resolved Hide resolved

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch 5 times, most recently from 7864cd5 to 4295f24 Compare September 7, 2023 05:42

IhorNehrutsa mentioned this pull request Sep 7, 2023

esp32/MCPWM: Add motor control MCPWM driver. #12345

Draft

projectgus requested changes Sep 12, 2023

View reviewed changes

ports/esp32/mphalport.c Outdated Show resolved Hide resolved

ports/esp32/mphalport.h Show resolved Hide resolved

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch from e000297 to d25d168 Compare September 12, 2023 12:20

IhorNehrutsa requested a review from projectgus September 12, 2023 12:45

projectgus approved these changes Sep 13, 2023

View reviewed changes

IhorNehrutsa force-pushed the check_esp_err_FUNC_LINE_FILE branch from d25d168 to 008d306 Compare September 13, 2023 05:31

dpgeorge reviewed Sep 14, 2023

View reviewed changes

dpgeorge reviewed Sep 15, 2023

View reviewed changes

IhorNehrutsa requested a review from dpgeorge September 15, 2023 05:42

dpgeorge reviewed Sep 15, 2023

View reviewed changes

IhorNehrutsa requested a review from dpgeorge September 15, 2023 07:40

dpgeorge force-pushed the check_esp_err_FUNC_LINE_FILE branch from 177a4cb to 00930b2 Compare September 18, 2023 01:29

dpgeorge merged commit 00930b2 into micropython:master Sep 18, 2023

IhorNehrutsa deleted the check_esp_err_FUNC_LINE_FILE branch September 18, 2023 05:38

IhorNehrutsa mentioned this pull request Sep 21, 2023

esp32: Use check_esp_err() for get error message. #8830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp32/mphalport: Add _FUNCTION_, _LINE_, _FILE_ info to check_esp_err(). #10888

esp32/mphalport: Add _FUNCTION_, _LINE_, _FILE_ info to check_esp_err(). #10888

IhorNehrutsa commented Feb 28, 2023 •

edited

Loading

IhorNehrutsa commented Jul 18, 2023

robert-hh commented Jul 21, 2023

IhorNehrutsa commented Jul 21, 2023 •

edited

Loading

robert-hh commented Jul 22, 2023

IhorNehrutsa commented Jul 24, 2023

IhorNehrutsa commented Sep 4, 2023 •

edited

Loading

andrewleech commented Sep 4, 2023

andrewleech commented Sep 4, 2023

IhorNehrutsa commented Sep 4, 2023

IhorNehrutsa commented Sep 7, 2023 •

edited

Loading

IhorNehrutsa commented Sep 8, 2023

projectgus left a comment

projectgus left a comment

projectgus commented Sep 13, 2023

IhorNehrutsa commented Sep 13, 2023

dpgeorge commented Sep 14, 2023

IhorNehrutsa commented Sep 14, 2023

dpgeorge Sep 14, 2023

dpgeorge Sep 14, 2023

IhorNehrutsa commented Sep 14, 2023

dpgeorge Sep 15, 2023

dpgeorge Sep 15, 2023

dpgeorge Sep 15, 2023

dpgeorge commented Sep 18, 2023

IhorNehrutsa commented Sep 18, 2023

esp32/mphalport: Add _FUNCTION_, _LINE_, _FILE_ info to check_esp_err(). #10888

esp32/mphalport: Add _FUNCTION_, _LINE_, _FILE_ info to check_esp_err(). #10888

Conversation

IhorNehrutsa commented Feb 28, 2023 • edited Loading

IhorNehrutsa commented Jul 18, 2023

robert-hh commented Jul 21, 2023

IhorNehrutsa commented Jul 21, 2023 • edited Loading

robert-hh commented Jul 22, 2023

IhorNehrutsa commented Jul 24, 2023

IhorNehrutsa commented Sep 4, 2023 • edited Loading

andrewleech commented Sep 4, 2023

andrewleech commented Sep 4, 2023

IhorNehrutsa commented Sep 4, 2023

IhorNehrutsa commented Sep 7, 2023 • edited Loading

IhorNehrutsa commented Sep 8, 2023

projectgus left a comment

Choose a reason for hiding this comment

projectgus left a comment

Choose a reason for hiding this comment

projectgus commented Sep 13, 2023

IhorNehrutsa commented Sep 13, 2023

dpgeorge commented Sep 14, 2023

IhorNehrutsa commented Sep 14, 2023

dpgeorge Sep 14, 2023

Choose a reason for hiding this comment

dpgeorge Sep 14, 2023

Choose a reason for hiding this comment

IhorNehrutsa commented Sep 14, 2023

dpgeorge Sep 15, 2023

Choose a reason for hiding this comment

dpgeorge Sep 15, 2023

Choose a reason for hiding this comment

dpgeorge Sep 15, 2023

Choose a reason for hiding this comment

dpgeorge commented Sep 18, 2023

IhorNehrutsa commented Sep 18, 2023

IhorNehrutsa commented Feb 28, 2023 •

edited

Loading

IhorNehrutsa commented Jul 21, 2023 •

edited

Loading

IhorNehrutsa commented Sep 4, 2023 •

edited

Loading

IhorNehrutsa commented Sep 7, 2023 •

edited

Loading