-
Notifications
You must be signed in to change notification settings - Fork 2
fix(log): update CXX log trace guard #14
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
976e40e to
814b3f3
Compare
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.
Pull Request Overview
This PR refactors the logging subsystem to use a C++ RAII-based trace guard, centralizes file-name extraction in C++ code, and updates related logging macros across backends.
- Replaces C-style trace guard macros with a
log_trace_guardclass leveragingstd::source_location. - Renames and consolidates backend macros (STD and ESP) to
_IMPL_FUNCand_IMPLvariants. - Removes the old C implementation of
esp_utils_log_extract_file_nameand adds a C++ version plus aparseFunctionNamehelper.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test_apps/main/test_on_cpp.cpp | Adds a LogTestClass and exercises the new C++ trace guard. |
| src/log/impl/esp_utils_log_impl_std.h | Refactors STD backend macros (ESP_UTILS_LOG*_IMPL[_FUNC]). |
| src/log/impl/esp_utils_log_impl_esp.h | Refactors ESP backend macros to match the STD pattern. |
| src/log/impl/esp_utils_log_impl.cpp | Introduces C++ esp_utils_log_extract_file_name and parseFunctionName. |
| src/log/impl/esp_utils_log_impl.c | Removes the old C version of esp_utils_log_extract_file_name. |
| src/log/esp_utils_log.hpp | Defines log_trace_guard class and new trace guard macros. |
| src/log/esp_utils_log.h | Updates ESP_UTILS_LOG_LEVEL to use _IMPL macros. |
| src/esp_utils_versions.h | Bumps patch version to 0.2.3. |
| library.properties | Updates library version to 0.2.3. |
| idf_component.yml | Updates component version to "0.2.3". |
| CHANGELOG.md | Adds v0.2.3 entry for the C++ trace guard update. |
Comments suppressed due to low confidence (4)
src/log/impl/esp_utils_log_impl.cpp:33
- The new
parseFunctionNamefunction contains non-trivial parsing logic; adding unit tests to cover various signature formats (with/without namespaces and templates) would help prevent regressions.
std::string parseFunctionName(const char *func_name)
src/log/esp_utils_log.hpp:53
- [nitpick] The
log_trace_guardtemplate and its constructors aren’t documented. Consider adding Doxygen comments explaining theTAGparameter and overload behaviors.
template <FixedString TAG>
test_apps/main/test_on_cpp.cpp:15
- [nitpick] The test helper class
LogTestClasshas a generic name. Renaming it to reflect its purpose (e.g.,TraceGuardTester) could improve readability in larger test suites.
class LogTestClass {
| #define ESP_UTILS_LOGD_IMPL_FUNC(TAG, format, ...) printf("[D][%s]" format "\n", TAG, ##__VA_ARGS__) | ||
| #define ESP_UTILS_LOGI_IMPL_FUNC(TAG, format, ...) printf("[I][%s]" format "\n", TAG, ##__VA_ARGS__) | ||
| #define ESP_UTILS_LOGW_IMPL_FUNC(TAG, format, ...) printf("[W][%s]" format "\n", TAG, ##__VA_ARGS__) | ||
| #define ESP_UTILS_LOGE_IMPL_FUNC(TAG, format, ...) printf("[E][%s]" format "\n", TAG, ##__VA_ARGS__) | ||
|
|
||
| #define ESP_UTILS_LOGD_IMPL(TAG, format, ...) ESP_UTILS_LOGD_IMPL_FUNC(TAG, "[%s:%04d](%s): " format, \ | ||
| esp_utils_log_extract_file_name(__FILE__), __LINE__, __func__, ##__VA_ARGS__) | ||
| #define ESP_UTILS_IMPL_LOGI(TAG, format, ...) printf("[I][" TAG "][%s:%04d](%s): " format "\n", \ | ||
| #define ESP_UTILS_LOGI_IMPL(TAG, format, ...) ESP_UTILS_LOGI_IMPL_FUNC(TAG, "[%s:%04d](%s): " format, \ | ||
| esp_utils_log_extract_file_name(__FILE__), __LINE__, __func__, ##__VA_ARGS__) | ||
| #define ESP_UTILS_IMPL_LOGW(TAG, format, ...) printf("[W][" TAG "][%s:%04d](%s): " format "\n", \ | ||
| #define ESP_UTILS_LOGW_IMPL(TAG, format, ...) ESP_UTILS_LOGW_IMPL_FUNC(TAG, "[%s:%04d](%s): " format, \ | ||
| esp_utils_log_extract_file_name(__FILE__), __LINE__, __func__, ##__VA_ARGS__) | ||
| #define ESP_UTILS_IMPL_LOGE(TAG, format, ...) printf("[E][" TAG "][%s:%04d](%s): " format "\n", \ | ||
| #define ESP_UTILS_LOGE_IMPL(TAG, format, ...) ESP_UTILS_LOGE_IMPL_FUNC(TAG, "[%s:%04d](%s): " format, \ |
Copilot
AI
Jul 3, 2025
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.
[nitpick] The pair of macros _IMPL_FUNC and _IMPL can be confusing to maintain. Adding a brief comment or renaming to clarify their distinct roles (format-wrapping vs. tagged invocation) may aid future contributors.
Closes #13 Aims at resolving esp-arduino-libs/ESP32_Display_Panel#216
55d8bb6 to
322451f
Compare
Bugfixes: