8000 Logger Recursion Guard per Task on ESP32 by bdraco · Pull Request #8765 · esphome/esphome · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

Logger Recursion Guard per Task on ESP32 #8765

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 61 commits into from
May 15, 2025
Merged

Logger Recursion Guard per Task on ESP32 #8765

merged 61 commits into from
May 15, 2025

Conversation

bdraco
Copy link
Member
@bdraco bdraco commented May 13, 2025 •
edited
Loading

What does this implement/fix?

The previous implementation used a global recursion guard, which caused log messages to be blocked in multi-task environments. To address this, we now leverage pthread thread-local storage (TLS) to track recursion state per task.

Key Changes

  • Main Task: Uses a dedicated boolean flag for minimal overhead.
  • Other Tasks: Utilize pthread TLS with a dynamically created key via pthread_key_create.

Design Highlights

  • Completely thread-safe--no locks, mutexes, or maps required.
  • Automatic state cleanup on task termination, handled by pthread TLS.
  • Improved reliability with significantly fewer dropped logs.
  • Backward compatible across all supported platforms.
  • Tested successfully with both IDF (4.4.x and 5.1.x) and Arduino.

Implementation Notes

  • Dynamic TLS Allocation: The implementation uses pthread_key_create to dynamically allocate a TLS key.
  • No Fixed Key Limitations: ESP-IDF's pthread implementation uses a linked list approach for TLS, allowing an unlimited number of keys (limited only by heap memory).
  • Efficient Per-Task State: This approach enables per-task state tracking without the need for locks, mutexes, or maps.
  • FreeRTOS TLS Slots Not Used: This implementation does not rely on the fixed FreeRTOS task local storage slots (CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS).

Limitations & Considerations

  • libretiny Compatibility: While libretiny shares similar recursion challenges, its limited multi-task capabilities (no Bluetooth) and constrained API make the impact less critical. Addressing this would require a different approach tailored to its specific limitations.

Testing

external_components:
  - source: github://pr#8765
    components: [ logger ]
    refresh: 0s

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

Pull request in esphome-docs with documentation (if applicable):

  • esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP32 IDF 5.1 and 4.4
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@bdraco
Rework logger recursion guard to be per task on FreeRTOS platforms
f9eca7b 
fixes #esphome/issues#5102

Also fixes a missing recursion_guard_ for ESP8266
@bdraco bdraco changed the title Rework logger recursion guard to be per task on FreeRTOS platforms Rework logger recursion guard to be per task on ESP32 May 13, 2025
@bdraco

This comment was marked as outdated.

Sign in to view
@bdraco bdraco changed the title Rework logger recursion guard to be per task on ESP32 Logger Recursion Guard per Task May 13, 2025
bdraco
bdraco commented May 13, 2025
View reviewed changes
@bdraco bdraco marked this pull request as ready for review May 13, 2025 04:57
@bdraco bdraco requested a review from a team as a code owner May 13, 2025 04:57
@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (logger) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

bdraco
bdraco commented May 13, 2025
View reviewed changes
@bdraco bdraco requested a review from Copilot May 13, 2025 04:58
Copilot
Copilot AI reviewed May 13, 2025
View reviewed changes
Copy link
Contributor
@Copilot Copilot AI left a 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 implements a task-specific recursion guard for the ESP32 logger component to avoid log message loss in multi-task environments. Key changes include:

  • Replacing the global recursion guard with separate guards: a dedicated boolean for the main task and pthread TLS for other tasks.
  • Updating logging functions in logger.cpp to use the new guard mechanism.
  • Configuring ESP-IDF to ensure sufficient thread local storage pointers via init.py.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
esphome/components/logger/logger.h Replaces the atomic recursion guard with a task-specific implementation using pthread TLS
esphome/components/logger/logger.cpp Updates the log functions to check, set, and reset the new recursion guard appropriately
esphome/components/logger/init.py Adds configuration to ensure enough thread local storage pointers for the new guard
Comments suppressed due to low confidence (1)

esphome/components/logger/logger.h:271

  • [nitpick] Verify that using a hard-coded TLS key value of 1 does not conflict with other platform keys; consider adding a compile-time check or extended documentation to enforce this constraint.
static const pthread_key_t LOG_RECURSION_KEY = (pthread_key_t) 1;

bdraco
bdraco commented May 15, 2025
View reviewed changes
bdraco
bdraco commented May 15, 2025
View reviewed changes
@jesserockz jesserockz added this to the 2025.5.0b3 milestone May 15, 2025
@jesserockz jesserockz merged commit 0b77cb1 into dev May 15, 2025
28 checks passed
@jesserockz jesserockz deleted the logger_recursion_guard_per_task branch May 15, 2025 09:36
@bdraco
Copy link
Member Author
bdraco commented May 15, 2025

Thanks for the review, and thanks for catching the TLS issue

jesserockz pushed a commit that referenced this pull request May 18, 2025
@bdraco @jesserockz
Logger Recursion Guard per Task on ESP32 (#8765)
41f860c
@jesserockz jesserockz mentioned this pull request May 18, 2025
Merged
@jesserockz jesserockz mentioned this pull request May 21, 2025
Merged
sa-crespo pushed a commit to sa-crespo/esphome that referenced this pull request May 26, 2025
@bdraco @sa-crespo
Logger Recursion Guard per Task on ESP32 (esphome#8765)
df65d70
sa-crespo pushed a commit to sa-crespo/esphome that referenced this pull request May 26, 2025
@bdraco @sa-crespo
Logger Recursion Guard per Task on ESP32 (esphome#8765)
4a48465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@jesserockz jesserockz jesserockz approved these changes

Assignees
No one assigned
Labels
cherry-picked core integration: logger
Projects
None yet
Milestone
2025.5.0b3
Development

Successfully merging this pull request may close these issues.

Logger is not thread safe
2 participants
@bdraco @jesserockz
0