feat: Add capacitive touch controls for Tidbyt Gen 2#130
feat: Add capacitive touch controls for Tidbyt Gen 2#130tavdog merged 2 commits intotronbyt:mainfrom
Conversation
Implements touch gestures on GPIO33 (TOUCH_PAD_NUM8): - TAP: Skip to next app - DOUBLE TAP: Reserved for future use - HOLD (2s): Toggle display on/off Features: - Adaptive baseline tracking to handle display EMI drift - Late tap detection to prevent accidental skips during double-tap - Fast warmup period (5s) for quicker calibration on boot - Debug logging (can be disabled via TOUCH_DEBUG_ENABLED) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @wurtz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces capacitive touch control functionality to the Tidbyt Gen 2, allowing users to interact with the device through simple gestures. It integrates a new touch control module that handles gesture detection, adaptive baseline tracking for environmental stability, and provides actions such as navigating applications and managing display power. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces capacitive touch controls for Tidbyt Gen 2, adding gestures for skipping apps and toggling the display. The implementation uses an adaptive baseline for touch detection, which is a solid approach.
My review has identified a few key areas for improvement:
- The touch polling in the HTTP mode is not responsive and needs to be reworked to match the responsiveness of the websocket mode.
- There's a bug in the display power-off logic that causes the user's brightness setting to be lost.
- The new
touch_controlmodule contains some unused code and inconsistent comments that should be cleaned up for better maintainability.
Details and suggestions are provided in the specific comments.
| // Check for touch events | ||
| touch_event_t touch_event = touch_control_check(); | ||
| if (touch_event != TOUCH_EVENT_NONE) { | ||
| handle_touch_event(touch_event); | ||
| } |
There was a problem hiding this comment.
The current implementation only checks for touch events once per image cycle in the HTTP loop. This will lead to an unresponsive UI, as the loop can be blocked for app_dwell_secs (plus fetch time). To fix this, touch events should be polled frequently, similar to the change made in the websocket loop. Consider refactoring the main HTTP loop to replace long blocking waits with shorter, repeated delays that include a touch check.
| isAnimating = 1; | ||
| } else { | ||
| ESP_LOGI(TAG, "HOLD - Display OFF"); | ||
| saved_brightness = 30; |
There was a problem hiding this comment.
When turning the display off, the current brightness level is not saved. Instead, saved_brightness is hardcoded to 30. This means any brightness level set by the user will be lost when the display is toggled off and on again. The current brightness should be read and stored in saved_brightness before setting the display brightness to 0. This may require a new function like display_get_brightness().
| typedef struct { | ||
| uint16_t threshold; | ||
| uint32_t debounce_ms; | ||
| bool initialized; | ||
| uint16_t baseline; | ||
| float adaptive_baseline; // Floating point for smooth adaptation | ||
| uint32_t last_baseline_update; | ||
| uint32_t init_time; // Time of initialization for warmup period | ||
| touch_fsm_state_t state; | ||
| uint32_t touch_start_time; | ||
| uint32_t release_time; | ||
| uint32_t last_event_time; | ||
| bool is_late_tap; // True if current touch was too late for double-tap (will be swallowed) | ||
| } touch_state_t; |
There was a problem hiding this comment.
The threshold and debounce_ms fields in the touch_state_t struct are initialized but never used in the touch detection logic. The detection relies on other constants like TOUCH_DROP_THRESHOLD. Consequently, the public functions touch_control_set_threshold, touch_control_get_threshold, and touch_control_set_debounce are operating on unused variables, making them dead code. This is misleading and can cause confusion for future maintenance. Please remove these unused fields and their corresponding functions from both the .c and .h files.
|
|
||
| // Touch control state | ||
| static bool display_power_on = true; | ||
| static uint8_t saved_brightness = 30; |
There was a problem hiding this comment.
The magic number 30 is used to initialize saved_brightness. To improve maintainability and consistency, please use the DISPLAY_DEFAULT_BRIGHTNESS constant, which is defined in display.h and already included.
static uint8_t saved_brightness = DISPLAY_DEFAULT_BRIGHTNESS;| * | ||
| * Gestures: | ||
| * - Single tap: Next app | ||
| * - Double tap: Cycle brightness (10% -> 25% -> 50% -> 75%) |
There was a problem hiding this comment.
The comment here states that a double tap is for cycling brightness, but the implementation in main.c has this action as 'reserved for future use'. The PR description also says it's reserved. Please update this comment and the corresponding one in touch_control.h to match the actual implementation and avoid confusion.
* - Double tap: (Reserved for future use)
|
We'll need to ifdef more precisecly so that all devices except Gen2 will not implement the button press code. |
- Add #ifdef CONFIG_BOARD_TIDBYT_GEN2 guards around all touch code - Conditionally compile touch_control.c only for Gen2 in CMakeLists - Non-Gen2 devices use original 5s delay instead of touch polling - Fix display_power_on sync when server sends brightness command Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Implements touch gestures on GPIO33 (TOUCH_PAD_NUM8):
Features: