-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adds "Relay n (API failsafe only)" to button's actions #63
Conversation
WalkthroughThe pull request introduces enhancements to the ESPHome configuration for button actions and relay toggling. In the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (2)
The Wi-Fi configuration appears to be truncated. Please ensure the following are properly configured:
The addition of ✅ Verification successfulLet me gather more context about how the wifi_component is being used in the relay configuration. The The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for references to wifi_component in other yaml files
rg "wifi_component" --type yaml
# Specifically look for failsafe-related code using this component
ast-grep --pattern 'id: wifi_component'
Length of output: 331 Script: #!/bin/bash
# Get the context around the wifi_component usage in the relay file
rg -B 5 -A 5 "wifi_component" ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml
# Also check the complete wifi configuration section
rg -B 5 -A 5 "wifi_component" ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml
Length of output: 720 🪧 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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
208-243
: Consider refactoring the relay toggle logic for better maintainability.While the implementation is functionally correct, the code structure could be improved to reduce repetition and enhance maintainability.
Consider refactoring using a map or helper function:
- switch (button) { - case ${BUTTON_1_ID}: - select_state = sl_button_1_action->state; - toggle_relay = (select_state == "${BUTTON_1_ACTION_TEXT}" or - (api_disconnected and select_state == "${BUTTON_1_ACTION_FAILSAFE_TEXT}")); - if (toggle_relay) - sw_relay_1->toggle(); - break; - // ... similar cases for buttons 2-4 - } + const std::map<uint8_t, std::tuple<select::Select*, switch_::Switch*, const char*, const char*>> button_map = { + {${BUTTON_1_ID}, {sl_button_1_action, sw_relay_1, "${BUTTON_1_ACTION_TEXT}", "${BUTTON_1_ACTION_FAILSAFE_TEXT}"}}, + {${BUTTON_2_ID}, {sl_button_2_action, sw_relay_2, "${BUTTON_2_ACTION_TEXT}", "${BUTTON_2_ACTION_FAILSAFE_TEXT}"}}, + {${BUTTON_3_ID}, {sl_button_3_action, sw_relay_3, "${BUTTON_3_ACTION_TEXT}", "${BUTTON_3_ACTION_FAILSAFE_TEXT}"}}, + {${BUTTON_4_ID}, {sl_button_4_action, sw_relay_4, "${BUTTON_4_ACTION_TEXT}", "${BUTTON_4_ACTION_FAILSAFE_TEXT}"}} + }; + + if (auto it = button_map.find(button); it != button_map.end()) { + auto [action_select, relay_switch, action_text, failsafe_text] = it->second; + select_state = action_select->state; + toggle_relay = (select_state == action_text or + (api_disconnected and select_state == failsafe_text)); + if (toggle_relay) + relay_switch->toggle(); + }This refactoring:
- Reduces code duplication
- Makes it easier to maintain and modify button-relay mappings
- Centralizes the toggle logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml
(5 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml
(1 hunks)
🔇 Additional comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
22-25
: LGTM! Well-structured failsafe action definitions.
The new substitutions follow consistent naming patterns and clearly indicate their failsafe nature.
296-296
: LGTM! Consistent implementation of failsafe options.
The failsafe options are properly added to each button's action select component, maintaining consistency across all buttons.
Also applies to: 311-311, 321-321, 331-331
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
194-205
: LGTM! Robust API connection state handling.
The implementation properly checks the API connection state and provides appropriate logging levels for different states.
I liked the suggestion, but I'm not gonna implement this now as this triggers a warning in ESPHome compiler:
|
Trying to detect a disconnection earlier.
Solves #23
Summary by CodeRabbit