-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
py/mpprint.h: Add do_printf() macro. #13400
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.
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
Conversation
Code size report:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13400 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 159 159
Lines 21088 21088
=======================================
Hits 20743 20743
Misses 345 345 ☔ View full report in Codecov by Sentry. |
9ef690f
to
fa7922a
Compare
a39489b
to
4d75c2a
Compare
@@ -37,9 +39,6 @@ | |||
|
|||
#if MICROPY_PY_BLUETOOTH | |||
|
|||
#define debug_printf(...) // mp_printf(&mp_plat_print, "mpbthciport.c: " __VA_ARGS__) | |||
#define error_printf(...) mp_printf(&mp_plat_print, "mpbthciport.c: " __VA_ARGS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These debug macros have lost the context of what file the log line is coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See do_print_line()
@IhorNehrutsa Could you please explain the reasoning behind this feature? My understanding is that the API in Suggest a good place to start is these two questions from last time:
Plus two new questions:
|
drivers/esp-hosted/esp_hosted_hal.h
Outdated
#ifndef ESP_HOSTED_DEBUG | ||
#define ESP_HOSTED_DEBUG (0) | ||
#endif | ||
|
||
#if ESP_HOSTED_DEBUG | ||
#define PROTOBUF_C_UNPACK_ERROR(...) error_printf(__VA_ARGS__); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block shouldn't be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
Those macros were actually just meant to be used internally by the esp-hosted driver as a debugging aid, they are also unrelated to ESP-IDF. It was just becoming hard to see useful error output from debugging, warnings etc.. and I wanted some colorful output :) It would be nice if they are standardized in MicroPython, and replace all internal debugging macros/helpers at some point, but they could probably use some work first. Re the context, it can be maintained by using |
Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>
4d75c2a
to
4bbb70e
Compare
Thanks for the contribution, but we don't need all this additional complexity in MicroPython. I know it's not adding any code size, but it adds lines to the source files and that adds to the complexity for someone reading and trying to understand the code. The printf-style debugging that we do have in MicroPython is already plenty, and should be done using local-to-the-file |
Move
do_printf()
fromdrivers/esp-hosted/esp_hosted_hal.h
topy/mpprint.h