New event handler #56
Conversation
This rebuild the engine to send events to Home Assistant
WalkthroughThe pull request introduces substantial modifications to the ESPHome configuration for the TX Ultimate Easy project, enhancing API functionality, event handling, and button/touch interactions. Key changes include the addition of a new API section, a new script for sending boot events, and updates to button handling logic to support various actions. The configuration files are refined for clarity and maintainability, with comprehensive documentation added for the event system, facilitating better integration with Home Assistant. Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (1)
90-101: Consider adding error handling to the lambda expressionsWhile the event script implementation is good, consider adding error handling for the lambda expressions to handle potential null states:
- homeassistant.event: event: esphome.tx_ultimate_easy data: - device_name: !lambda return tx_device_name->state.c_str(); + device_name: !lambda |- + if (tx_device_name->has_state()) { + return tx_device_name->state.c_str(); + } + return "unknown"; firmware: ${version} domain: boot type: !lambda return type.c_str();docs/events.md (2)
50-50: Fix typo in documentationThere's a typo in the word "refeer" which should be "refer".
-For more details, please refeer to our **[Events docs](docs/events.md)**. +For more details, please refer to our **[Events docs](docs/events.md)**.
19-32: Consider adding version information to example payloadsThe example payloads are clear and well-structured. Consider adding a note about payload versioning or compatibility to help with future updates.
Add a comment above the example:
# Event payload structure (v1) # Compatible with firmware versions 2024.12.1 and laterREADME.md (1)
141-144: Consider adding response time expectationsThe support section is well-structured. Consider adding expected response times for different support channels to help manage user expectations.
Add response time expectations:
- **Bug Reports & Feature Requests**: Use [GitHub Issues](https://github.com/edwardtfn/TX-Ultimate-Easy/issues) - for all bug reports and feature requests + for all bug reports and feature requests (typical response time: 2-3 business days) - **Community Chat**: Join our [Discord Server](https://discord.gg/Db6WJWzWuf) - for discussions and community interaction + for discussions and community interaction (real-time community support)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
183-193: Optimize click event delay timing.The fixed delay of
${BUTTON_MULTI_CLICK_DELAY}before processing clicks could lead to a noticeable lag in the UI response, especially for single clicks which are the most common use case.Consider implementing an adaptive delay:
- delay: - milliseconds: ${BUTTON_MULTI_CLICK_DELAY} + milliseconds: !lambda |- + // Reduce delay for single clicks + return (click_count == 1) ? + ${BUTTON_MULTI_CLICK_DELAY}/2 : + ${BUTTON_MULTI_CLICK_DELAY};ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
Line range hint
192-207: Add validation for relay state changes.While the relay toggle logic works correctly, it would benefit from additional validation to prevent unintended state changes.
Consider adding state validation:
switch (button) { case ${BUTTON_1_ID}: - if (sl_button_1_action->state != "${RELAY_MODE_TEXT_NOT_USED}") + if (sl_button_1_action->state != "${RELAY_MODE_TEXT_NOT_USED}" && + sw_relay_1->state.has_value()) sw_relay_1->toggle(); break;ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (1)
366-427: Enhance event data structure.The Home Assistant event data structure could be improved for better consistency and maintainability.
Consider standardizing the event data structure:
- homeassistant.event: event: esphome.tx_ultimate_easy data: device_name: !lambda return tx_device_name->state.c_str(); firmware: ${version} domain: touch type: swipe - action: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "right" : "up"; - swipe-direction: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "right" : "up"; + action: !lambda |- + const auto direction = sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" + ? "right" : "up"; + return { + "type": "swipe", + "direction": direction, + "model_format": sl_tx_model_format->state + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_api.yaml(0 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(1 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(6 hunks)README.md(2 hunks)docs/events.md(1 hunks)
💤 Files with no reviewable changes (1)
- ESPHome/TX-Ultimate-Easy-ESPHome_core_api.yaml
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
32-32: Expected: 80; Actual: 105
Line length
(MD013, line-length)
46-46: Expected: 80; Actual: 93
Line length
(MD013, line-length)
🔇 Additional comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (2)
38-42: LGTM: API configuration looks good!
The API configuration properly sets up the server and handles client connections by publishing device information.
107-108: Verify the event handling in boot scripts
The boot scripts are correctly integrated with the new event system. However, let's verify that all boot events are properly handled.
Also applies to: 118-119
✅ Verification successful
Boot event handling is properly implemented
The verification shows a well-structured boot event system:
- Boot events are properly handled through
api_send_ha_event_bootwith both 'start' and 'done' states - The boot sequence is correctly integrated with Home Assistant events (
esphome.tx_ultimate_easy) - Boot initialization is properly prioritized (priority 700) and linked to the boot sequence script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other boot event handlers that might need updating
rg -l "send_event_to_ha"
Length of output: 24
Script:
#!/bin/bash
# Search for event-related patterns in yaml files
rg -t yaml "event:|send.*event|api.*event" -A 3
# Also search for boot-related configurations
rg -t yaml "boot:|on_boot:|boot_.*event" -A 3
Length of output: 3898
README.md (1)
32-33: LGTM: Documentation improvements enhance clarity
The restructured sections provide better readability and clearer organization of information. The added reference to the events documentation is particularly helpful.
Also applies to: 47-50
🧰 Tools
🪛 Markdownlint (0.37.0)
32-32: Expected: 80; Actual: 105
Line length
(MD013, line-length)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
173-177:⚠️ Potential issueConsider potential race conditions in button state management.
The global state variables are still reset without proper synchronization, which could lead to race conditions if multiple button events occur in quick succession.
Consider implementing atomic operations or a queue-based approach for state management:
- id(button_press_button) = 0; - id(button_press_position) = 0; - id(button_press_start_time) = 0; - id(click_counter) = 0; + auto current_button = id(button_press_button); + auto current_position = id(button_press_position); + id(button_press_button) = 0; + id(button_press_position) = 0; + id(button_press_start_time) = 0; + id(click_counter) = 0; + if (current_button > 0) { + process_pending_events(current_button, current_position); + }
🧹 Nitpick comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
183-193: Consider improving click event handling.While the implementation is functional, consider these improvements:
- The fixed delay might affect responsiveness for rapid button presses
- The event name mapping could be more maintainable
Consider this refactor:
- lambda: |- - std::string event_name; - if (click_count == 1) event_name = "click"; - else if (click_count == 2) event_name = "double_click"; - else event_name = "multiple_click"; + static const std::map<uint8_t, const char*> EVENT_NAMES = { + {1, "click"}, + {2, "double_click"} + }; + const char* event_name = EVENT_NAMES.count(click_count) ? + EVENT_NAMES.at(click_count) : "multiple_click"; button_action->execute(button, event_name, click_count);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml(3 hunks)
🔇 Additional comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
115-124: LGTM: Well-structured event publishing!
The event structure provides comprehensive metadata including device name, firmware version, domain, and action details, which will be valuable for Home Assistant integration.
267-267: LGTM: Proper long press event handling.
The implementation correctly triggers the button action for long press events.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (1)
394-428: Refactor duplicate swipe direction logic.The swipe direction logic is duplicated in both
actionandswipe-directionfields. Consider extracting this into a reusable function.on_swipe_left: - homeassistant.event: event: esphome.tx_ultimate_easy data: device_name: !lambda return tx_device_name->state.c_str(); firmware: ${version} domain: touch type: swipe - action: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "left" : "down"; - swipe-direction: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "left" : "down"; + action: !lambda return get_swipe_direction("left", "down"); + swipe-direction: !lambda return get_swipe_direction("left", "down"); position: !lambda return touch.x; on_swipe_right: - homeassistant.event: event: esphome.tx_ultimate_easy data: device_name: !lambda return tx_device_name->state.c_str(); firmware: ${version} domain: touch type: swipe - action: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "right" : "up"; - swipe-direction: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "right" : "up"; + action: !lambda return get_swipe_direction("right", "up"); + swipe-direction: !lambda return get_swipe_direction("right", "up"); position: !lambda return touch.x;Add this helper function at the top of the file:
# Add to substitutions section get_swipe_direction: |- std::string get_swipe_direction(const std::string& us_dir, const std::string& eu_dir) { return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? us_dir : eu_dir; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml(4 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(6 hunks)
🔇 Additional comments (4)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (2)
38-42: LGTM! API configuration looks good.
The API configuration is well-structured with proper client connection handling.
90-101: LGTM! Event script is well-designed.
The script follows best practices:
- Uses queued mode to prevent event loss
- Includes standardized event data structure
- Properly handles dynamic device information
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (2)
18-147: Reduce code duplication in button event sensors.
The binary sensors for button events contain significant code duplication, despite using the template.
Line range hint 232-290: LGTM! Well-structured initialization scripts.
The initialization scripts are well-organized with:
- Clear separation of format and gang initialization
- Comprehensive state management
- Proper condition checks before initialization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (2)
25-146: Consider further reducing code duplication in button events.While the template helps, there's still significant duplication in the event handlers. Consider using YAML anchors for the
on_pressactions.# Define a template for button press actions - &button_press_actions then: - delay: !lambda return nr_touch_duration->state; - binary_sensor.template.publish: id: BUTTON_ID # To be replaced state: OFF # Use the template (example for one button) - id: bs_button_1_click_event name: Button 1 - Click event internal: false <<: *bs_button_event_template on_press: <<: *button_press_actions then: - binary_sensor.template.publish: id: bs_button_1_click_event state: OFF
394-424: Consider removing redundant swipe direction field.The event data contains duplicate information with both
actionandswipe-directionfields containing the same value.- homeassistant.event: event: esphome.tx_ultimate_easy data: device_name: !lambda return tx_device_name->state.c_str(); firmware: ${version} domain: touch type: swipe action: !lambda |- return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "left" : "down"; - swipe-direction: !lambda |- - return sl_tx_model_format->state == "${TX_MODEL_FORMAT_US_TEXT}" ? "left" : "down"; position: !lambda return touch.x;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml(4 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(6 hunks)
🔇 Additional comments (5)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (3)
38-42: LGTM! Well-structured API configuration.
The API configuration follows best practices by publishing device information whenever a client connects.
90-101: LGTM! Well-designed event handling script.
The script follows a consistent event structure and properly handles all parameters. The event payload includes essential device information.
163-165: 🛠️ Refactor suggestion
Add validation for gang count.
The lambda function directly sets the gang count without validation. Consider adding range checks.
on_value:
then:
- - lambda: tx_ultimate->set_gang_count(static_cast<uint8_t>(i)+1);
+ - lambda: |-
+ const uint8_t gang_count = static_cast<uint8_t>(i) + 1;
+ if (gang_count >= 1 && gang_count <= 4) {
+ tx_ultimate->set_gang_count(gang_count);
+ } else {
+ ESP_LOGE("core", "Invalid gang count: %u", gang_count);
+ }Likely invalid or redundant comment.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (2)
18-24: LGTM! Well-structured button event template.
The template effectively reduces code duplication and follows YAML best practices.
Line range hint 237-290: LGTM! Comprehensive touch initialization implementation.
The initialization scripts properly handle both format and gang configuration with appropriate state management and error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
32-50: Documentation aligns well with architectural changes.The README updates effectively document the architectural changes for Home Assistant event integration. The separation of concerns between state monitoring and event handling is well explained, providing users with a clear understanding of how to leverage the new event system.
🧰 Tools
🪛 Markdownlint (0.37.0)
32-32: Expected: 80; Actual: 105
Line length(MD013, line-length)
46-46: Expected: 80; Actual: 93
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
32-32: Expected: 80; Actual: 105
Line length
(MD013, line-length)
46-46: Expected: 80; Actual: 93
Line length
(MD013, line-length)
🔇 Additional comments (3)
README.md (3)
32-33: LGTM! Improved readability.
The line break after "allowing you to:" improves the document's readability and structure.
🧰 Tools
🪛 Markdownlint (0.37.0)
32-32: Expected: 80; Actual: 105
Line length
(MD013, line-length)
141-144: LGTM! Clear support channel organization.
The improved formatting clearly distinguishes between bug reports/feature requests and community chat channels.
47-50: Verify the events documentation link.
The changes effectively explain the event system. Let's ensure the referenced documentation exists and is accessible.
✅ Verification successful
Documentation link is valid and comprehensive
The events documentation exists at docs/events.md and contains detailed information about:
- Event system overview and integration with Home Assistant
- Event structure and payload format
- Supported event types (Boot, Touch, Swipe, Multi-Touch)
- Implementation examples for automations
- Real-time event monitoring instructions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the events documentation file
# and check its content for completeness
# Check if the events documentation file exists
if [ -f "docs/events.md" ]; then
echo "Events documentation found"
# Check if it contains essential sections
rg -i "event|automation|home assistant" "docs/events.md"
else
echo "Warning: Events documentation file not found at docs/events.md"
fi
Length of output: 2177
|
Seems like with the recent changes the time it takes from a button release to a switch toggle state has increased noticeably. Even the logs show that operations are taking too long, perhaps some tuning is required. Also, can it be possible to implement 'on press' logic for the buttons? I know gestures wouldn't work but I imagine a lot of logic could be skipped this way increasing response time greatly |
|
The delay is due to the logic for waiting for a second (and consecutive) clicks to be able to detect a I've later added a substitution for that, so probably reducing If you don't mind playing a bit with those substitutions and share here the values that fits your better, that would be awesome. 😉 |
|
Should these substitutions work from my main yaml file? I suspect not because I added those lines and modified the values but the default values are still in effect substitutions:
name: <my-device-name>
friendly_name: <"my friendly name">
api_key: !secret api_key
ota_password: !secret ota_password
BUTTON_CLICK_MIN_LENGTH: '30' # The minimum duration the click should last, in msec
BUTTON_CLICK_MAX_LENGTH: '250' # The maximum duration the click should last, in msec
BUTTON_MULTI_CLICK_DELAY: '150' # The time to wait for another click, in msec
BUTTON_PRESS_TIMEOUT: '10000' # Ignore if button is pressed for longer than this time, in msec
BUTTON_LONG_PRESS_DELAY: '800' # The time to wait to consider a long press, in msec... |
|
Yes, it should work from your file. |
|
This is my yaml config file: |
|
Please try this: substitutions:
name: <my-device-name>
friendly_name: <"my friendly name">
api_key: !secret api_key
ota_password: !secret ota_password
BUTTON_CLICK_MIN_LENGTH: '30' # The minimum duration the click should last, in msec
BUTTON_CLICK_MAX_LENGTH: '250' # The maximum duration the click should last, in msec
BUTTON_MULTI_CLICK_DELAY: '150' # The time to wait for another click, in msec
BUTTON_PRESS_TIMEOUT: '10000' # Ignore if button is pressed for longer than this time, in msec
BUTTON_LONG_PRESS_DELAY: '800' # The time to wait to consider a long press, in msec
wifi:
ssid: !secret wifi_ssid
password: !secret wifi_password
#append encryption
api:
encryption:
key: "${api_key}"
#append password
ota:
#- platform: esphome
password: "${ota_password}"
packages:
remote_package:
url: https://github.com/edwardtfn/TX-Ultimate-Easy
ref: main
refresh: 30s
files:
- ESPHome/TX-Ultimate-Easy-ESPHome.yaml
# - ESPHome/TX-Ultimate-Easy-ESPHome_addon_ble_proxy.yaml # Adds experimental BLE proxy support (limited) |
|
Hi, |
This rebuild the engine to send events to Home Assistant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores