Fix relay on display as light mode #55
Conversation
WalkthroughThe pull request modifies the ESPHome configuration files for hardware relays, LED lights, and button interactions. Key changes include the introduction of a consolidated event handler for button clicks, updates to light configurations to accurately reflect relay states, and enhancements to the readability of relay mode selections and switch configurations through the use of substitution variables and clearer naming conventions. Additionally, new light entities are added, and improvements are made to error logging and button action handling. 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: 0
🧹 Nitpick comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (3)
27-45: LGTM! Good state synchronization between lights and relaysThe handlers ensure that light states accurately reflect relay states, preventing inconsistencies.
Consider reducing code duplication in light configurations
The four light configurations are nearly identical, differing only in their IDs and associated relay numbers.
Consider using ESPHome's substitutions and templating to reduce duplication:
substitutions: # Add these to your existing substitutions LIGHT_TEMPLATE: &light_template platform: binary internal: true on_turn_on: then: - if: condition: - switch.is_off: sw_relay_$num then: - switch.turn_on: sw_relay_$num on_turn_off: then: - if: condition: - switch.is_on: sw_relay_$num then: - switch.turn_off: sw_relay_$num light: - id: light_output_1 name: Light output 1 output: output_relay_1 <<: *light_template substitutions: num: "1" # Repeat for lights 2-4 with corresponding numbersAlso applies to: 49-64, 68-83, 87-102
176-209: LGTM! Well-structured event handler with proper validationThe event handler properly validates click counts and touch events before processing, with clear control flow.
Consider using substitutions for button IDs in switch cases
The switch statement uses button IDs directly in case statements.
Consider defining button IDs as substitutions for better maintainability:
substitutions: BUTTON_1_CASE: "${BUTTON_1_ID}" BUTTON_2_CASE: "${BUTTON_2_ID}" BUTTON_3_CASE: "${BUTTON_3_ID}" BUTTON_4_CASE: "${BUTTON_4_ID}"
315-405: LGTM! Good state management in switch configurationsThe switches properly manage state synchronization with lights and include appropriate restore behavior.
Consider reducing code duplication in switch configurations
Similar to the light configurations, the switch configurations are nearly identical.
Consider using YAML anchors and substitutions:
switch: - &switch_base platform: output restore_mode: RESTORE_DEFAULT_OFF internal: true on_turn_on: then: - script.execute: show_relay_status - if: condition: - light.is_off: light_output_$num then: - light.turn_on: light_output_$num on_turn_off: then: - script.execute: show_relay_status - if: condition: - light.is_on: light_output_$num then: - light.turn_off: light_output_$num - id: sw_relay_1 name: Relay 1 output: output_relay_1 <<: *switch_base substitutions: num: "1" # Repeat for switches 2-4 with corresponding numbers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(5 hunks)
🔇 Additional comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (2)
14-17: LGTM! Good use of substitutions for relay modes
The substitutions provide clear, reusable text constants for relay modes, improving maintainability.
Line range hint 288-311: LGTM! Well-structured select configurations
Good use of YAML anchors and substitutions to maintain consistency across relay mode selections. The naming clearly indicates these are display modes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml (2)
Line range hint
798-836: Document index conversion in logging messagesThe logging messages now display 1-based indices (light_index+1) while the internal logic uses 0-based indices. Consider adding a comment to explain this conversion to prevent confusion in future maintenance.
ESP_LOGE("core_hw_leds", "Invalid set with %s light for Relay %" PRIu8, - light_mode_group_1.c_str(), light_index+1); + light_mode_group_1.c_str(), light_index+1); // Convert to 1-based index for user-friendly logs
Line range hint
844-922: Consider consolidating common template propertiesThe EU and US relay light mode templates share many properties. Consider extracting common properties into a base template to reduce duplication and improve maintainability.
+ # Base relay light mode template + - &relay_light_mode_base + platform: template + optimistic: true + restore_value: true + internal: true + entity_category: config + disabled_by_default: false + icon: mdi:dots-square # EU relay light mode template - &relay_light_mode_eu id: sl_relay_1_light_mode_eu name: Relay 1 light mode - platform: template + <<: *relay_light_mode_base options: - "${LIGHT_MODE_DISABLED_TEXT}" - "${LIGHT_MODE_BOTTOM_TEXT}" - "${LIGHT_MODE_TOP_TEXT}" - "${LIGHT_MODE_BOTH_TEXT}" initial_option: "${LIGHT_MODE_BOTTOM_TEXT}" - optimistic: true - restore_value: true - internal: true - entity_category: config - disabled_by_default: false - icon: mdi:dots-square # US relay light mode template similar changes...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml(4 hunks)
🔇 Additional comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_leds.yaml (1)
Line range hint 428-460: LGTM: New light entities for US model
The new light entities (light_28 to light_31) are correctly implemented and follow the established pattern, properly marked as internal for the US model.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
29-34: LGTM! Consider adding documentation.The introduction of button ID constants improves maintainability and reduces magic numbers in the code. The naming convention is clear and consistent.
Consider adding a brief comment explaining the purpose and usage of these constants, for example:
# Button ID constants used for event handling and state management # These IDs correspond to the physical button positions on the device BUTTON_1_ID: '1' BUTTON_2_ID: '2' BUTTON_3_ID: '3' BUTTON_4_ID: '4'
Line range hint
141-169: Consider improving button handling robustness.The button handling implementation has several areas that could benefit from additional safeguards:
Multi-click Detection:
- The delay between clicks (
BUTTON_MULTI_CLICK_DELAY: '250') might need adjustment based on user testing- Consider adding a maximum click count to prevent overflow
Touch Position Calculation:
- Add bounds checking for
touch_positioninput- Consider logging invalid positions for debugging
- Document the position calculation formula
Timing Constants:
- Consider making timing constants configurable via Home Assistant
- Add validation for timing constant relationships (e.g.,
BUTTON_CLICK_MAX_LENGTHshould be less thanBUTTON_LONG_PRESS_DELAY)Would you like me to provide specific code examples for any of these improvements?
Also applies to: 249-266
📜 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(1 hunks)
🔇 Additional comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
29-34: Verify integration with relay control system.
Since this PR aims to fix relay display in light mode, we should verify that the button ID constants are correctly integrated with the relay control system.
✅ Verification successful
Button ID constants are correctly integrated with relay control system
The verification shows that:
- The button IDs (1-4) are properly used in the relay control system through the
button_click_eventscript - Each button correctly maps to its corresponding relay (BUTTON_1_ID -> sw_relay_1, etc.)
- The relay control system properly handles both switch and light modes as part of the fix
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relay-related configurations and verify button ID usage
echo "Searching for relay configurations..."
rg -l "relay" . --type yaml
echo "Checking button-to-relay mappings..."
rg "BUTTON_.*_ACTION_TEXT.*[Rr]elay" . --type yaml
echo "Verifying event handler implementations..."
rg "button_click_event.*[Rr]elay" . --type yaml
Length of output: 942
Script:
#!/bin/bash
# Let's check the actual button-relay interaction implementation
echo "Checking button event handlers and relay interactions..."
rg -A 5 "on_click:" . --type yaml
echo "Checking relay component definitions..."
cat ./ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml
echo "Checking if button IDs are used in relay control..."
rg "id: ['\"]?[1-4]['\"]?" . --type yaml
Length of output: 16949
Solves #28
Summary by CodeRabbit
New Features
Improvements