-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add Detect a Swipe section to LVGL Cookbook #4614
base: current
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request adds a new section to the Changes
Sequence DiagramsequenceDiagram
participant User
participant Touchscreen
participant BinarySensor
participant Script
User->>Touchscreen: Perform swipe gesture
Touchscreen->>BinarySensor: Detect touch within defined area
BinarySensor->>Touchscreen: Validate swipe coordinates
alt Swipe Left
Touchscreen->>Script: Trigger swipe_left script
else Swipe Right
Touchscreen->>Script: Trigger swipe_right script
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Nitpick comments (2)
cookbook/lvgl.rst (2)
2279-2285
: Add error handling for page navigation.The scripts should handle cases where there are no more pages to navigate to.
Consider adding conditions:
script: - id: swipe_left then: + - if: + condition: lvgl.has_next_page + then: - lvgl.page.next + else: + - logger.log: "No next page available" - id: swipe_right then: + - if: + condition: lvgl.has_previous_page + then: - lvgl.page.previous + else: + - logger.log: "No previous page available"
2234-2237
: Documentation could be more detailed.The introduction should include more details about:
- Supported swipe directions (currently only horizontal is implemented)
- Minimum/maximum swipe distances
- Touch state definitions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cookbook/lvgl.rst
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
cookbook/lvgl.rst (1)
Pattern **
: - Do not generate or add any sequence diagrams
🔇 Additional comments (1)
cookbook/lvgl.rst (1)
2240-2248
: Consider adding validation for coordinate boundaries.The binary sensor configuration defines a swipe detection area but doesn't validate if the coordinates exceed screen dimensions.
- lambda: |- | ||
for (auto touch: touches) { | ||
ESP_LOGI("touch_on_update", "state=%d x=%d, y=%d, x_prev=%d, y_prev=%d, x_org=%d, y_org=%d", touch.state, touch.x, touch.y,touch.x_prev,touch.y_prev,touch.x_org,touch.y_org); | ||
if ((touch.state == 6) && (id(swipe_area).state)) { | ||
if ((touch.x_org - touch.x_prev) > 100) { | ||
ESP_LOGI("touch_on_update", "Swipe detected: RIGHT to LEFT"); | ||
id(swipe_left).execute(); | ||
} | ||
if ((touch.x_prev - touch.x_org) > 100) { | ||
ESP_LOGI("touch_on_update", "Swipe detected: LEFT to RIGHT"); | ||
id(swipe_right).execute(); | ||
} | ||
} | ||
} | ||
|
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.
🛠️ Refactor suggestion
Enhance swipe detection logic and error handling.
The swipe detection implementation has several areas for improvement:
- Magic number
6
for touch state should be defined as a named constant - The swipe distance threshold of
100
pixels should be configurable - Missing error handling for touch coordinate validation
Consider this improved implementation:
- lambda: |-
+ // Define touch states as constants
+ static const uint8_t TOUCH_STATE_RELEASED = 6;
+ static const int16_t SWIPE_THRESHOLD = 100; // pixels
+
for (auto touch: touches) {
ESP_LOGI("touch_on_update", "state=%d x=%d, y=%d, x_prev=%d, y_prev=%d, x_org=%d, y_org=%d", touch.state, touch.x, touch.y,touch.x_prev,touch.y_prev,touch.x_org,touch.y_org);
- if ((touch.state == 6) && (id(swipe_area).state)) {
+ // Validate coordinates are within bounds
+ if (!touch.is_valid()) {
+ ESP_LOGW("touch_on_update", "Invalid touch coordinates");
+ continue;
+ }
+
+ if ((touch.state == TOUCH_STATE_RELEASED) && (id(swipe_area).state)) {
if ((touch.x_org - touch.x_prev) > 100) {
ESP_LOGI("touch_on_update", "Swipe detected: RIGHT to LEFT");
id(swipe_left).execute();
}
if ((touch.x_prev - touch.x_org) > 100) {
ESP_LOGI("touch_on_update", "Swipe detected: LEFT to RIGHT");
id(swipe_right).execute();
}
}
}
Committable suggestion skipped: line range outside the PR's diff.
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Description:
Related issue (if applicable): fixes
Pull request in esphome with YAML changes (if applicable): esphome/esphome#
Checklist:
I am merging into
next
because this is new documentation that has a matching pull-request in esphome as linked above.or
I am merging into
current
because this is a fix, change and/or adjustment in the current documentation and is not for a new component or feature.Link added in
/index.rst
when creating new documents for new components or cookbook.