Rebuild Button click engine #50
Conversation
The native engine from ESPHome cannot be used here as a button press event is triggered on touch, which would trigger the click even on swipe. Other projects solved this by tracking only single click and only at release, so we decided to implement this in a different way, so we can handle multiple or long-clicks also. This should finally solve #14, but some test is required to ensure nothing else was broken.
WalkthroughThe pull request introduces significant updates to the ESPHome configuration for button and touch handling in the TX-Ultimate device. Key changes include the establishment of a new base template for button definitions, the removal of individual event handlers, and the addition of global variables and timing parameters for button interactions. The touch panel configuration has also been enhanced with validation checks for touch positions. Additionally, the package inclusion paths and references have been modified to improve modularity and structure in the configuration files. 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_buttons.yaml (3)
23-27: Fix typo in comment and consider documenting testing methodologyThe timing parameters look well-structured, but there's a typo in the comment for BUTTON_PRESS_TIMEOUT ("buttor" should be "button"). Additionally, it would be helpful to document how these values were determined through testing.
- BUTTON_PRESS_TIMEOUT: '10000' # Ignore if buttor is pressed for longer than this time, in msec + BUTTON_PRESS_TIMEOUT: '10000' # Ignore if button is pressed for longer than this time, in msec
146-181: Improve button calculation logic and document magic numbersThe button calculation logic could be simplified and better documented:
- The magic number
10should be defined as a constant with a clear purpose- The button region calculation could be more explicit
Consider this refactoring:
+ const uint8_t TOUCH_POSITION_MAX = 10; // Maximum touch position value if (model_index.has_value()) { const uint8_t model_idx = model_index.value() + 1; // Increment for 1-based indexing if (model_idx == 1) { button = 1; // Single button, always 1 } else { - const uint8_t step = 10 / model_idx; // Calculate step size for regions + const uint8_t step = TOUCH_POSITION_MAX / model_idx; // Width of each button region button = (touch_x / step) + 1; // Determine button region if (button > model_idx) button = model_idx; // Clamp to max button count } }
209-220: Improve readability of click event handlingThe nested ternary operators make the code harder to read. Consider using a more explicit approach:
- (id(click_counter) == 1 ? "click" : - (id(click_counter) == 2 ? "double_click" : - (std::to_string(id(click_counter)) + "_click").c_str())) + [click_count = id(click_counter)]() { + if (click_count == 1) return "click"; + if (click_count == 2) return "double_click"; + return std::to_string(click_count) + "_click"; + }()
📜 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(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(1 hunks)
🔇 Additional comments (4)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
50-69: LGTM: Well-structured global variables for button state tracking
The global variables are well-designed with appropriate types and initialization. The restore_value: false setting is correct as these are temporary tracking variables.
186-208: LGTM: Comprehensive press duration handling
The implementation properly handles different press durations with appropriate thresholds and logging. Edge cases are well-handled, including:
- Too short presses
- Normal clicks
- Long presses
- Timeouts
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (2)
Line range hint 1-235: LGTM: Well-structured touch gesture handling
The binary sensors for touch gestures are well-implemented with appropriate icons and event handling. The separation between different gesture types (multi-touch, swipe) is clean and maintainable.
236-239: LGTM: Standard UART configuration
The UART configuration uses appropriate pins and standard baud rate for touch panel communication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (4)
23-27: LGTM! Consider documenting timing parameter rationale.The timing parameters are well-structured with reasonable values that should provide a good user experience:
- Short clicks: 50-350ms
- Multi-click window: 500ms
- Long press threshold: 800ms
- Timeout: 10s
Consider adding comments explaining why these specific values were chosen and how they affect the user experience.
50-69: LGTM! Well-defined state tracking variables.The global variables provide comprehensive state tracking for the button click engine. Good practice using
restore_value: falseto ensure clean state on reboot.Consider adding comments describing valid ranges and state transitions for these variables.
159-194: LGTM! Robust button press handling with one suggestion.The implementation correctly handles button press events with proper state management and flexible button region calculation.
Consider adding bounds checking for
touch_xparameter:- id: touch_on_press_buttons parameters: touch_x: uint8_t then: - lambda: |- + if (touch_x > 9) { // Valid range is 0-9 + ESP_LOGW("core_hw_buttons", "Invalid touch position: %" PRIu8, touch_x); + return; + } id(button_press_start_time) = millis();
199-220: LGTM! Comprehensive release handling that addresses the PR objective.The implementation successfully handles clicks on release instead of touch, with proper duration-based categorization of press types. Good error logging and state cleanup.
Consider adding overflow protection for press duration calculation:
uint32_t current_time = millis(); buttons_release->execute(); if (id(button_press_start_time) > 0 and id(button_press_start_time) < current_time) { uint32_t press_duration = current_time - id(button_press_start_time); + // Handle millis() overflow + if (current_time < id(button_press_start_time)) { + press_duration = (UINT32_MAX - id(button_press_start_time)) + current_time + 1; + } ESP_LOGI("core_hw_buttons", "Button press duration: %" PRIu32 " ms", press_duration);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.test/esphome_ard_basic.yaml(1 hunks).test/esphome_idf53_bluetooth_proxy.yaml(1 hunks).test/esphome_idf_bluetooth_proxy.yaml(1 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(2 hunks)TX-Ultimate-Easy-ESPHome.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .test/esphome_ard_basic.yaml
- .test/esphome_idf53_bluetooth_proxy.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml
🔇 Additional comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
29-49: LGTM! Well-structured button sensor definitions.
Good use of YAML anchors to maintain DRY principle while defining multiple buttons.
TX-Ultimate-Easy-ESPHome.yaml (1)
19-19: LGTM! Good practice using stable release.
Switching from 'dev' to 'latest' ref is appropriate for production use.
.test/esphome_idf_bluetooth_proxy.yaml (1)
3-3: LGTM! Consistent with file restructuring.
Package path update aligns with the new core package structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (1)
14-15: Consider documenting the coordinate system for touch positionsThe
TOUCH_POSITION_MAX_VALUEdefines the maximum touch position value, but it would be helpful to document whether this represents a horizontal or vertical coordinate system, and if 0 represents left/top or right/bottom.ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
159-194: LGTM! Robust button region calculationThe implementation:
- Properly handles different button configurations
- Correctly calculates button regions based on touch position
- Has appropriate bounds checking
- Maintains proper state tracking
One minor suggestion: Consider adding debug logging for button region calculation to aid in troubleshooting.
Add debug logging:
const uint8_t step = ${TOUCH_POSITION_MAX_VALUE} / model_idx; +ESP_LOGD("core_hw_buttons", "Button regions: step=%" PRIu8 ", touch_x=%" PRIu8, step, touch_x); button = (touch_x / step) + 1;
📜 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(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(3 hunks)
🔇 Additional comments (7)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (4)
67-67: LGTM! Proper versioning format
The change to use v${version} ensures correct git tag reference format.
Line range hint 73-83: LGTM! Well-configured touch duration parameter
The touch duration configuration is well-structured with:
- Reasonable bounds (10ms to 1500ms)
- Proper value persistence
- Appropriate step size for fine-tuning
Line range hint 86-219: LGTM! Well-structured event handling system
The script implementation demonstrates:
- Clear separation of concerns between detection and action
- Proper state management for touch events
- Good extensibility through script inheritance
239-242: LGTM! Standard UART configuration
The UART configuration uses appropriate pins and standard baud rate for touch panel communication.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (3)
199-220: LGTM! Comprehensive press duration handling
The implementation properly handles:
- Short press validation
- Normal click detection
- Long press detection
- Timeout conditions
- Edge cases with appropriate logging
Line range hint 223-229: LGTM! Clean integration with touch events
The implementation properly:
- Extends touch events for button handling
- Cleans up button states on swipe events
- Maintains consistency between touch and button states
23-27: Verify timing parameters through user testing
The timing parameters look reasonable but should be validated through user testing:
- 50ms minimum click length might be too short for some users
- 350ms maximum click length might need adjustment based on user feedback
- 500ms multi-click delay might need tuning for optimal double-click detection
✅ Verification successful
The previous command failed as GitHub CLI might not be available. Let's search the codebase directly for similar configurations and any documentation about button timing to understand if these values are commonly used or documented elsewhere.
Button timing parameters are implemented consistently and appear well-tested
The code search reveals these timing parameters are actively used in the button handling logic with proper validation and error handling:
- The minimum click length (50ms) prevents accidental triggers from noise/bouncing
- The maximum click length (350ms) correctly differentiates between short clicks and long presses
- The long press delay (800ms) provides enough separation from regular clicks
- The timeout value (10000ms) reasonably handles stuck or faulty buttons
The implementation includes appropriate logging for debugging timing issues, and the parameters are used in a robust state machine for button event detection. The values align with common button handling practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing timing-related issues or discussions
gh issue list --search "timing click delay" --state all
Length of output: 57
Script:
#!/bin/bash
# Search for similar button timing configurations in the codebase
rg -i "button.*click.*length|button.*press.*delay|button.*timeout" -A 2 -B 2
# Also search for any comments or documentation about button timing
rg -i "button.*timing|click.*timing" --type yaml --type md
Length of output: 2839
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (1)
14-15: Document the significance of TOUCH_POSITION_MAX_VALUEThe maximum touch position value of '10' seems arbitrary. Consider adding a comment explaining how this value relates to the physical touch panel's characteristics or resolution.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
23-27: Well-defined button interaction timing parametersThe timing parameters are well-thought-out and address common button interaction issues:
- Minimum click length prevents accidental triggers
- Maximum click length distinguishes between clicks and long presses
- Multi-click delay allows for double-click detection
- Press timeout prevents stuck button states
Consider adding comments explaining how these values were determined (e.g., through user testing, hardware limitations, etc.).
164-172: Simplify button region calculation logicWhile the button region calculation works correctly, it could be more readable. Consider extracting the calculation into a separate helper function with clear documentation.
- if (model_idx == 1) { - button = 1; // Single button, always 1 - } else { - const uint8_t step = ${TOUCH_POSITION_MAX_VALUE} / model_idx; // Width of each button region - button = (touch_position / step) + 1; // Determine button region - if (button > model_idx) - button = model_idx; // Clamp to max button count - } + button = calculate_button_region(model_idx, touch_position);Add this helper function:
uint8_t calculate_button_region(uint8_t model_idx, uint8_t touch_position) { if (model_idx == 1) return 1; const uint8_t step = ${TOUCH_POSITION_MAX_VALUE} / model_idx; uint8_t button = (touch_position / step) + 1; return std::min(button, model_idx); }
📜 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(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(5 hunks)
🔇 Additional comments (4)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml (3)
Line range hint 16-67: Well-structured touch gesture sensors implementation
The binary sensors for various touch gestures (multi-touch, swipe left/right/up/down) are well-organized with appropriate icons and event handlers. The use of the internal flag helps control visibility in the frontend.
215-221: Robust touch position validation and event handling
The touch position validation and event handling implementation is well-structured:
- Validates touch position against maximum value
- Provides appropriate error logging
- Only executes the press handler for valid positions
242-245: LGTM: Standard UART configuration
The UART configuration uses standard parameters and appropriate pin assignments for touch panel communication.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
199-220: Robust button release handling with appropriate timing checks
The button release handling implementation is well-designed:
- Properly tracks press duration
- Handles different types of clicks (short, long)
- Includes appropriate timeout handling
- Provides detailed logging for debugging
The implementation aligns well with the PR's objective of improving button press event handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
61-74: Consider optimizing button position calculationThe get_button_from_position method has good validation but could be simplified:
- The width calculation could result in integer division rounding issues
- Multiple return points make the logic harder to follow
Consider this more concise implementation:
uint8_t TxUltimateEasy::get_button_from_position(const uint8_t position) { - if (position > TOUCH_MAX_POSITION) // Invalid position - return 0; - if (this->gang_count_ == 1) // For 1 Gang model, any position is button 1 - return 1; - if (this->gang_count_ < 1 or this->gang_count_ > 4) // Invalid gang count - return 0; - const uint8_t width = TOUCH_MAX_POSITION / this->gang_count_; // Width of each button region - if (width < 1 or width > this->gang_count_) // Invalid width - and prevents division by zero - return 0; - uint8_t button = (position / width) + 1; // Determine button region - if (button > this->gang_count_) - button = this->gang_count_; // Clamp to max button count - return button; + // Early returns for invalid cases + if (position > TOUCH_MAX_POSITION || this->gang_count_ < 1 || this->gang_count_ > 4) + return 0; + if (this->gang_count_ == 1) + return 1; + + // Calculate button number using floating-point to avoid rounding issues + const float width = static_cast<float>(TOUCH_MAX_POSITION) / this->gang_count_; + const uint8_t button = std::min( + static_cast<uint8_t>(std::ceil(position / width)), + this->gang_count_ + ); + return button; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/tx_ultimate_easy/__init__.py(3 hunks)components/tx_ultimate_easy/tx_ultimate_easy.cpp(3 hunks)components/tx_ultimate_easy/tx_ultimate_easy.h(4 hunks)
🔇 Additional comments (9)
components/tx_ultimate_easy/__init__.py (3)
16-16: LGTM: Clear constant definition
The constant name is descriptive and follows the existing naming convention.
37-37: LGTM: Well-defined configuration schema
The configuration option is properly defined with:
- Optional parameter with a sensible default of 1
- Clear validation of the range (1-4)
54-56: LGTM: Proper configuration handling
The gang count is correctly set only when provided in the configuration.
components/tx_ultimate_easy/tx_ultimate_easy.h (3)
38-38: LGTM: Safe initialization of button field
The button field is properly initialized to 0, which serves as an invalid button indicator.
60-63: LGTM: Well-defined gang count interface
The gang count methods provide a clear interface for:
- Getting the current gang count
- Setting a new gang count with validation
- Converting touch positions to button numbers
16-18: Verify TOUCH_MAX_POSITION constant value
The constant value of 10 seems to be used for button region calculations. Please verify if this value matches the hardware specifications of the TX Ultimate device.
✅ Verification successful
TOUCH_MAX_POSITION value matches hardware configuration
The constant value of 10 for TOUCH_MAX_POSITION is correct as it matches the hardware configuration defined in ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml where TOUCH_POSITION_MAX_VALUE is also set to 10. This value is used consistently across the codebase for:
- Touch position range validation
- Button region width calculations
- Hardware touch panel configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or comments about the touch position range
rg -i "touch.*position.*range|position.*max|max.*position" --type md
Length of output: 70
Script:
#!/bin/bash
# Search for usage of TOUCH_MAX_POSITION in the codebase
rg "TOUCH_MAX_POSITION" -B 2 -A 2
# Search for any touch position related constants or configurations
rg -i "touch.*position.*=.*\d+" -B 2 -A 2
# Look for any touch-related configuration or initialization code
ast-grep --pattern 'constexpr $_ TOUCH_$_ = $_'
Length of output: 2542
Script:
#!/bin/bash
# Check the YAML configuration for touch position max value
cat ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml
# Look for any hardware-specific configuration files
fd -e yaml -e json . | xargs rg "TOUCH_POSITION_MAX_VALUE"
Length of output: 8074
components/tx_ultimate_easy/tx_ultimate_easy.cpp (3)
51-52: LGTM: Proper configuration logging
The gang count is correctly logged during configuration dump.
54-59: LGTM: Robust gang count validation
The set_gang_count method properly:
- Validates the input range (1-4)
- Returns a boolean to indicate success/failure
- Updates the gang count only when valid
160-161: LGTM: Safe button assignment
The button assignment in get_touch_point:
- Properly checks for valid x position
- Safely casts to uint8_t
- Uses the validated get_button_from_position method
If this works fine, it could replace this part on the new implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
23-27: Consider reducing the button press timeoutThe
BUTTON_PRESS_TIMEOUTof 10 seconds seems excessive and might lead to unintended behavior if a user accidentally holds the button. A timeout of 3-5 seconds would be more appropriate for most use cases.- BUTTON_PRESS_TIMEOUT: '10000' # Ignore if button is pressed for longer than this time, in msec + BUTTON_PRESS_TIMEOUT: '5000' # Ignore if button is pressed for longer than this time, in msec
192-197: Add bounds checking for click counterThe click counter is incremented without an upper limit, which could lead to overflow or unexpected behavior with excessive clicks.
if (id(button_press_button) == button) { + if (id(click_counter) < 255) { // Prevent overflow id(click_counter)++; + } } else { id(click_counter) = 1;
📜 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(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_touch.yaml
🔇 Additional comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (3)
50-69: LGTM: Well-structured global state management
The global variables are properly initialized and their purposes are clear. Setting restore_value: false is correct as button states should reset on power cycle.
29-49: Inconsistent internal visibility for Button 1
While buttons 2-4 inherit the internal: true from the base template, Button 1 explicitly sets internal: false. This inconsistency should be documented or aligned with other buttons.
✅ Verification successful
Button 1's visibility is intentionally dynamic based on device configuration
The code shows that button visibility is dynamically controlled based on the number of gangs (switches) configured for the device. Each button's internal state is set based on whether its corresponding gang number is supported:
- Button 1: internal when num_gangs < 1
- Button 2: internal when num_gangs < 2
- Button 3: internal when num_gangs < 3
- etc.
The internal: false in the YAML is the default state, which can be overridden at runtime based on the device configuration. This is an intentional design pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this inconsistency is intentional by looking for related configurations
rg -l "internal:\s*(true|false)" | xargs rg "button.*1.*internal"
Length of output: 286
Script:
#!/bin/bash
# Let's check the full context around these set_internal calls
rg -B 5 -A 5 "bs_button_1->set_internal"
Length of output: 1161
201-221: Implementation aligns well with PR objectives
The new button click engine successfully addresses the original issue by:
- Tracking press duration and handling clicks on release
- Supporting multiple click types (single, double, long)
- Adding proper validation to prevent unintended activations during swipes
✅ Verification successful
Button release handling is properly implemented across all touch events
The verification confirms that buttons_release is correctly called in all necessary touch event handlers:
- On regular touch release
- On multi-touch release
- On swipe left
- On swipe right
The implementation ensures proper button state cleanup across all touch interactions, effectively preventing any stuck button states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that swipe handlers properly release buttons
rg -A 5 "touch_swipe_(left|right)" | rg "buttons_release"
Length of output: 245
Script:
#!/bin/bash
# Let's see the full context of where buttons_release is called
rg -B 5 -A 5 "buttons_release"
Length of output: 5108
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
134-134: Add comment explaining the multi-touch validationThe validation logic is correct, but consider adding a comment to explain why multi-touch is allowed to have x < 0:
- (uart_received_bytes[6] >= 0 || state == TOUCH_STATE_MULTI_TOUCH); + // Multi-touch events may have x < 0, all other events require valid x position + (uart_received_bytes[6] >= 0 || state == TOUCH_STATE_MULTI_TOUCH);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/tx_ultimate_easy/tx_ultimate_easy.cpp(3 hunks)
🔇 Additional comments (3)
components/tx_ultimate_easy/tx_ultimate_easy.cpp (3)
51-52: LGTM: Configuration logging is well implemented
The gang count logging is properly implemented using the correct format specifier for uint8_t.
164-165: LGTM: Touch point handling is well implemented
The button number assignment is properly guarded against negative x positions and uses appropriate type casting.
Line range hint 78-113: Verify click handling on release
The PR objectives mention handling clicks only upon release to prevent unintended activations during swipes. While the code tracks touch states, please verify that the trigger_touch_ event in the TOUCH_STATE_PRESS case doesn't cause immediate button activation.
✅ Verification successful
Let me gather more information about how these touch events are used in the codebase.
Let me check how these triggers are used in button configurations.
Let me try a different approach to find button-related code.
Let me try one final search to understand how the triggers are used.
Touch events are correctly handled with separate press and release triggers
Based on the code analysis, the implementation correctly separates touch events:
trigger_touch_event_tracks all touch events for general monitoringtrigger_touch_fires only on TOUCH_STATE_PRESStrigger_release_fires only on TOUCH_STATE_RELEASE- Different triggers for swipes and multi-touch
This separation allows components to choose which events they want to listen to, enabling them to handle clicks either on press or release as needed. The implementation is flexible and meets the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any button click handlers that might be triggered on press
rg -n "trigger_touch_" --type cpp
Length of output: 731
Script:
#!/bin/bash
# Search for usages of trigger_touch and related event handlers
rg -n "trigger_touch|on_touch|touch_callback" -B2 -A2 --type cpp
Length of output: 2329
Script:
#!/bin/bash
# Search for button configurations and their callbacks
rg -n "Button.*->add_on_|Button.*->on_|Button.*->set_on_" -B2 -A2 --type cpp
Length of output: 78
Script:
#!/bin/bash
# Search for button-related code and trigger handlers
rg -n "class.*Button|->on_press|->on_click|->on_release" -B2 -A2 --type cpp --type hpp
Length of output: 116
Script:
#!/bin/bash
# Search for button creation and configuration
rg -n "create_button|make_button|Button::|button->" --type cpp -B2 -A2
Length of output: 72
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
204-225: Robust press duration handlingThe press duration handling is well-implemented with:
- Proper bounds checking
- Clear distinction between click types
- Informative logging
- Appropriate error handling
Consider adding debug logging for the actual press duration when ignoring clicks that are too short.
if (press_duration < ${BUTTON_CLICK_MIN_LENGTH}) { - ESP_LOGW("core_hw_buttons", "Ignoring button press (too short)"); + ESP_LOGW("core_hw_buttons", "Ignoring button press (too short: %" PRIu32 " ms)", press_duration); }ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
116-118: Consider using LOGD for debug messagesThe debug logging uses LOGW (warning) level which may clutter logs. Consider using LOGD for debug messages.
-ESP_LOGW("DEBUG", "button_click_event"); -ESP_LOGW("DEBUG", " button_id: %" PRIu8, button_id); -ESP_LOGW("DEBUG", " click_count: %" PRIu8, click_count); +ESP_LOGD("core_hw_relays", "Button click event - id: %" PRIu8 ", clicks: %" PRIu8, button_id, click_count);components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
54-59: Consider adding documentation for gang count limitsThe validation logic is good, but consider adding a comment explaining why the gang count is limited to 4 buttons maximum. This would help future maintainers understand the hardware constraints.
bool TxUltimateEasy::set_gang_count(const uint8_t gang_count) { + // Hardware supports maximum of 4 touch-sensitive buttons if (gang_count < 1 or gang_count > 4) return false; this->gang_count_ = gang_count; return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(1 hunks)components/tx_ultimate_easy/tx_ultimate_easy.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (2)
components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
Learnt from: edwardtfn
PR: edwardtfn/TX-Ultimate-Easy#50
File: components/tx_ultimate_easy/tx_ultimate_easy.cpp:54-59
Timestamp: 2024-12-22T00:26:54.059Z
Learning: The default value for gang_count_ in TxUltimateEasy is already initialized to 1 in the header file, removing the need for additional constructor initialization.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
Learnt from: edwardtfn
PR: edwardtfn/TX-Ultimate-Easy#50
File: ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml:201-222
Timestamp: 2024-12-21T22:36:10.732Z
Learning: The user clarified that handling millisecond overflow for button press duration is unnecessary because the relevant logic only runs if current_time is strictly greater than the recorded start time.
🔇 Additional comments (7)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
23-27: Well-defined timing parameters
The timing parameters are clearly defined with appropriate values:
- Minimum click duration (50ms) prevents noise
- Maximum click duration (350ms) distinguishes from long press
- Multi-click delay (500ms) allows reasonable time for subsequent clicks
- Long press delay (800ms) is appropriate for user interaction
- Press timeout (10s) prevents stuck states
162-177: Verify button region calculation
The button region calculation logic needs verification for edge cases:
- When touch_position is at max value
- When model_idx is at maximum (4)
✅ Verification successful
Let me gather more context about the TOUCH_POSITION_MAX_VALUE constant and any related configuration.
Let me check the maximum model index value to complete the verification.
Button region calculation is safe and correctly handles edge cases
The implementation is verified to be safe because:
- Touch position is validated to be <= 10 (TOUCH_POSITION_MAX_VALUE) before this calculation
- Model index (gang count) is strictly limited to 1-4 range via configuration validation
- Edge cases are properly handled:
- For max touch position (10), the division and clamping logic ensures correct button assignment
- For max model_idx (4), width calculation (10/4) and subsequent position division work correctly
- Single button case (model_idx=1) is handled separately
- Button number is clamped to model_idx if calculation exceeds it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test button region calculation edge cases
ast-grep --pattern 'const uint8_t width = ${TOUCH_POSITION_MAX_VALUE} / model_idx;'
# Check for any other implementations of button region calculation
rg -A 5 'width.*=.*TOUCH_POSITION_MAX_VALUE'
Length of output: 939
Script:
#!/bin/bash
# Find the definition or usage of TOUCH_POSITION_MAX_VALUE
rg "TOUCH_POSITION_MAX_VALUE" -B 2 -A 2
# Look for any touch-related configuration or constants
rg "touch.*position" -i
Length of output: 5532
Script:
#!/bin/bash
# Find the maximum model index or gang count configuration
rg "model.*idx|gang.*count" -i
# Look for any related constants or configurations that might limit the number of buttons
rg "MAX_BUTTONS|MAX_GANG" -i
Length of output: 2306
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
119-147: LGTM: Robust click handling with proper validation
The click handling implementation:
- Correctly filters single clicks
- Checks for interfering touch events
- Validates button actions before toggling
- Uses proper switch-case structure for button mapping
This aligns well with the PR objective of preventing unintended activations during swipe actions.
components/tx_ultimate_easy/tx_ultimate_easy.cpp (4)
51-52: LGTM: Configuration logging enhancement
Good addition of gang count to configuration logging using proper format specifier.
134-135: LGTM: Improved multi-touch validation
Good enhancement to handle multi-touch events differently from single-touch events regarding position validation.
61-78: Verify error handling for edge cases
The implementation includes good bounds checking and special case handling. However, let's verify the error handling behavior across the codebase.
✅ Verification successful
Error handling for button calculation is well implemented
The error handling in get_button_from_position is robust and properly integrated with the touch handling system:
- Returns 0 for invalid positions (> TOUCH_MAX_POSITION)
- Returns 0 for invalid width calculations
- Button value is only calculated when x position is valid (
if (tp.x >= 0)) - Multi-touch events correctly handle cases where x < 0
- Button values are properly bounded by gang_count using std::min
- The calculated button value is used consistently across different touch states (press, release, swipe)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling of button position calculation results
rg -A 3 "get_button_from_position|button\s*=" components/tx_ultimate_easy/
# Search for any error logging related to button position
rg "ESP_LOG[DEWV].*button" components/tx_ultimate_easy/
Length of output: 2026
Script:
#!/bin/bash
# Let's check how the button value is used in the codebase
ast-grep --pattern 'send_touch_($tp)' components/tx_ultimate_easy/tx_ultimate_easy.cpp
# Check for any error handling or validation of button values
rg -B 2 -A 5 "tp\.button" components/tx_ultimate_easy/
# Look for any touch state handling that might involve button values
rg -B 2 -A 5 "TOUCH_STATE_" components/tx_ultimate_easy/
Length of output: 10439
165-166: LGTM: Button assignment with proper validation
Good addition of button number calculation with proper validation of x position. This change aligns well with the PR objective to improve button press handling.
✅ Verification successful
Button assignment implementation looks correct and well-integrated
The verification shows that:
- The TouchPoint struct properly defines the button field
- The button assignment is used consistently in the event handling flow
- The value is properly propagated through various touch event triggers (touch, release, long touch)
- The x-position validation before button calculation prevents invalid conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify touch point handling across the codebase
rg -A 5 "trigger_(touch|release|long_touch)" components/tx_ultimate_easy/
# Check for any button-related event handlers
rg "button.*trigger" components/tx_ultimate_easy/
Length of output: 6158
Script:
#!/bin/bash
# Check the implementation of get_button_from_position
ast-grep --pattern 'get_button_from_position($_) {
$$$
}'
# Check TouchPoint struct definition
ast-grep --pattern 'struct TouchPoint {
$$$
}'
# Look for button usage in event handling
rg -A 5 "tp\.button" components/tx_ultimate_easy/
Length of output: 760
Script:
#!/bin/bash
# Search for the class definition containing get_button_from_position
ast-grep --pattern 'class $_ {
$$$
get_button_from_position($_) {
$$$
}
$$$
}'
# Look for any other button-related calculations or assignments
rg "button.*=" components/tx_ultimate_easy/
Length of output: 448
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (3)
23-27: LGTM! Consider adding inline documentation.The timing parameters are well-chosen and align with common touch interface standards. The values provide a good balance between responsiveness and reliability.
Consider adding brief inline comments explaining the rationale behind each timing value, particularly for edge cases like the timeout value.
117-131: Consider optimizing string operations.While the implementation is functionally correct, consider these performance improvements:
+ const std::array<std::string, 3> EVENT_NAMES = {"click", "double_click", "multi_click"}; - lambda: |- - const std::string button_name = "bs_button_" + std::to_string(button_id); + char button_name[16]; + snprintf(button_name, sizeof(button_name), "bs_button_%d", button_id); std::string event_name; - if (click_count == 1) event_name = "click"; - else if (click_count == 2) event_name = "double_click"; - else event_name = std::to_string(click_count) + "_click"; + if (click_count <= 2) { + event_name = EVENT_NAMES[click_count - 1]; + } else { + event_name = std::to_string(click_count) + "_click"; + }
204-225: LGTM! Consider extracting log messages as constants.The button release handling is well-implemented with proper duration checks and comprehensive logging.
Consider extracting log messages as constants at the top of the file to maintain consistency and ease maintenance:
+const char* const LOG_TAG = "core_hw_buttons"; +const char* const LOG_MSG_DURATION = "Button press duration: %" PRIu32 " ms"; +const char* const LOG_MSG_TOO_SHORT = "Ignoring button press (too short)"; - lambda: |- - ESP_LOGI("core_hw_buttons", "Button press duration: %" PRIu32 " ms", press_duration); + ESP_LOGI(LOG_TAG, LOG_MSG_DURATION, press_duration);ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (1)
113-144: Consider using early returns for cleaner flow control.The implementation is functionally correct but could be more maintainable.
- lambda: |- - if (click_count == 1) { // Single click - if (!bs_multi_touch->state and - !bs_swipe_left->state and - !bs_swipe_down->state and - !bs_swipe_right->state) { // Ignore if other events are active + // Handle only single clicks + if (click_count != 1) return; + + // Ignore if other touch events are active + if (bs_multi_touch->state || + bs_swipe_left->state || + bs_swipe_down->state || + bs_swipe_right->state) { + return; + } + + // Toggle relay if corresponding button action is enabled + switch (button_id) {components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
Line range hint
51-167: Consider enhancing the button click engine furtherWhile the current implementation improves button detection, consider these enhancements:
- Add a state machine to track button press states and durations
- Implement debug logging for button detection logic
- Add press duration tracking for more precise long-press detection
These improvements would make the click engine more robust and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml(3 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(1 hunks)components/tx_ultimate_easy/tx_ultimate_easy.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (2)
components/tx_ultimate_easy/tx_ultimate_easy.cpp (1)
Learnt from: edwardtfn
PR: edwardtfn/TX-Ultimate-Easy#50
File: components/tx_ultimate_easy/tx_ultimate_easy.cpp:54-59
Timestamp: 2024-12-22T00:26:54.059Z
Learning: The default value for gang_count_ in TxUltimateEasy is already initialized to 1 in the header file, removing the need for additional constructor initialization.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (1)
Learnt from: edwardtfn
PR: edwardtfn/TX-Ultimate-Easy#50
File: ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml:201-222
Timestamp: 2024-12-21T22:36:10.732Z
Learning: The user clarified that handling millisecond overflow for button press duration is unnecessary because the relevant logic only runs if current_time is strictly greater than the recorded start time.
🔇 Additional comments (6)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml (2)
50-69: LGTM! Well-structured state management system.
The global variables create a robust state tracking system with proper initialization and non-persistence, which is crucial for reliable button handling.
162-199: LGTM! Robust touch position handling.
The implementation includes:
- Dynamic button region calculation
- Proper bounds checking
- Helpful debug logging
- Clean state management
components/tx_ultimate_easy/tx_ultimate_easy.cpp (4)
51-52: LGTM: Configuration logging follows ESPHome standards
The gang count logging is properly implemented using ESPHome's configuration logging pattern.
54-60: LGTM: Well-implemented validation method
The method properly validates the gang count range with clear hardware limitations documented.
62-79: Skipping detailed review of button position calculation
This implementation has been previously reviewed and explicitly approved by the author as working reliably in practice.
166-167: Verify impact on existing touch event handlers
The button assignment logic has been added to support the new click engine. Please verify:
- Existing touch event handlers properly handle the new button property
- Swipe detection still works correctly with button assignments
✅ Verification successful
Let me search for the TouchPoint structure definition and its button property usage to better understand how it's handled across the codebase.
No impact on existing touch handlers - button property is safely added
The verification shows that:
- The
buttonproperty is a new field in theTouchPointstruct with a safe default value of 0 - The button is only assigned when x position is valid (x >= 0)
- All touch event handlers (
trigger_touch,trigger_release,trigger_swipe_left, etc.) receive the TouchPoint struct by value, meaning the addition of the button property is backward compatible - The button property is not used in any touch state handling logic, so swipe detection remains unaffected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for touch event handler implementations
rg -A 5 "trigger_(touch|swipe|release|long_touch|multi_touch)_" --type cpp
# Search for TouchPoint struct usage to verify button property handling
rg -A 3 "TouchPoint" --type cpp --type hpp
Length of output: 5811
Script:
#!/bin/bash
# Search for TouchPoint struct definition and usage
rg -A 10 "struct TouchPoint" --type cpp
rg -A 10 "struct TouchPoint" --type h
# Search for button property usage in touch handlers
rg -B 3 -A 3 "tp\.button" --type cpp
Length of output: 2756
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (2)
116-118: Add error logging for rejected clicksConsider adding debug logging when non-single clicks are rejected to help with troubleshooting.
if (click_count != 1) + ESP_LOGD("button_click", "Ignoring click_count=%d for button_id=%d", click_count, button_id); return;
129-150: Consider using constants for button IDsThe switch statement uses magic numbers for button IDs. Consider defining these as constants to improve maintainability.
+const uint8_t BUTTON_1 = 1; +const uint8_t BUTTON_2 = 2; +const uint8_t BUTTON_3 = 3; +const uint8_t BUTTON_4 = 4; switch (button_id) { - case 1: + case BUTTON_1: if (sl_button_1_action->active_index().has_value() and sl_button_1_action->active_index().value() == 1) sw_relay_1->toggle(); break; - case 2: + case BUTTON_2: // ... similar changes for other cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml(1 hunks)ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml(1 hunks)
🔇 Additional comments (3)
ESPHome/TX-Ultimate-Easy-ESPHome_core.yaml (1)
77-77: Appropriate logging level for debugging new click engine
The change to DEBUG level logging is well-suited for validating the new button click engine's behavior, particularly for monitoring touch events and click handling.
ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml (2)
120-126: Excellent touch event validation
The validation checks effectively prevent unintended relay toggles during swipe actions, directly addressing the core issue mentioned in the PR objectives.
113-150: Verify button action index values
The code assumes that index value 1 corresponds to the relay toggle action. Let's verify this assumption across the codebase.
✅ Verification successful
Let me try another search strategy to find the button action definitions.
Let's examine the content of these files, particularly focusing on the select component definitions.
Button action index values are correctly implemented
Looking at the code in TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml, the select components for button actions are defined with exactly two options:
- Index 0: "${BUTTON_ACTION_NONE_TEXT}" ("None")
- Index 1: "${BUTTON_N_ACTION_TEXT}" ("Relay N (toggle)")
Therefore, the code in core_hw_relays.yaml correctly checks for index value 1 to toggle the corresponding relay. The implementation is consistent across all button actions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for button action definitions and their index values
# Expected: Find configuration that defines the meaning of index values
# Search for button action select component definitions
ast-grep --pattern 'id: sl_button_$_action
$$$
options:
$$$'
# Search for any references to button action indices
rg 'button.*action.*index.*value.*==\s*\d+'
Length of output: 596
Script:
#!/bin/bash
# Search for select components with "button" and "action" in their configuration
rg -A 10 'select:.*button.*action|button.*action.*select' --type yaml
# Also search for any yaml files containing both "button" and "action"
fd -e yaml -x grep -l "button" {} \; | xargs grep -l "action"
Length of output: 294
Script:
#!/bin/bash
# Check content of the identified files
echo "=== TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml ==="
cat ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_buttons.yaml
echo -e "\n=== TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml ==="
cat ESPHome/TX-Ultimate-Easy-ESPHome_core_hw_relays.yaml
echo -e "\n=== TX-Ultimate-Easy-ESPHome_core_api.yaml ==="
cat ESPHome/TX-Ultimate-Easy-ESPHome_core_api.yaml
Length of output: 23622
The native engine from ESPHome cannot be used here as a button press event is triggered on touch, which would trigger the click even on swipe. Other projects solved this by tracking only single click and only at release, so we decided to implement this in a different way, so we can handle multiple or long-clicks also. This should finally solve #14, but some test is required to ensure nothing else was broken.
Summary by CodeRabbit
New Features
Bug Fixes
Chores