Conversation
fatihak
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is a really nice implementation overall and very close to how I was thinking about adding button support, especially with the settings flow and GPIO handling.
A few items of feedback:
- Plugin button API: I don’t think this is really necessary. For most cases (for example, “next” image in an album or library), a refresh already achieves the same result. Exposing button handling inside plugins will likely lead to requests for plugin-specific button settings which adds complexity when defining a separate plugin instance with different settings is the expected use case.
- Button actions: I think the most useful/requested behavior will be mapping buttons to show a specific plugin. For example, button A shows a calendar, button B shows a clock, etc. This doesn’t need to be added as part of this PR since it should be straightforward to layer on later with the existing structure.
- Added plugins: I’m guessing the added plugins were to demonstrate the button actions/testing but they can be removed. You can add them as third plugin plugins which were recently added: https://github.com/fatihak/InkyPi/wiki
- Concurrent button actions: Right now, pressing a button multiple times with some delay can trigger multiple actions before the first one finishes. I think we should prevent new button actions from executing while another is in progress. Otherwise this can lead to race conditions especially with resource-heavy plugins like refreshing weather or calendar plugins at the same time.
| status = "enabled" if self._auto_cycle_enabled else "disabled" | ||
| logger.info(f"Auto-cycle {status}") | ||
|
|
||
| def _show_info(self): |
There was a problem hiding this comment.
This action doesn't update the screen for me, likely bc it instantiates a new DisplayManager instance. I dont think we should be instantiating a new display manager here but instead pass it into the button manager in inkypi.py
|
|
||
| return take_screenshot_html(rendered_html, dimensions) | ||
|
|
||
| def on_button_press(self, button_id, press_type, device_config) -> bool: |
There was a problem hiding this comment.
I don't think this feature is really necessary for custom button actions on plugins. The "next" image for an album/ library can already be achieved by refreshing that plugin. People will likely ask to switch between settings for a specific plugin, which will add additional complexity to plugins when it should just be defined as a separate plugin instance with a different setting.
| } | ||
| } | ||
|
|
||
| function startStatusPolling() { |
There was a problem hiding this comment.
The status polling only starts when using the buttons in the home page. I think we can run this all the time so it works with regular refreshes and hardware button actions as well.
7e1e687 to
fc55288
Compare
|
@fatihak Thanks for the detailed feedback! I've addressed almost all points in the latest commits. Re: Plugin button API - I respectfully disagree here. The API solves a problem that refresh alone can't handle. Note: I've also added concurrent action protection at both client and server levels to prevent race conditions when buttons are pressed rapidly. |
|
@v-murygin is absolutely right here. If you expect people to contribute by building plugins, you need APIs, regs, hooks, and callbacks for them to interact with as much as possible. Of course, it adds complexity, but it does so in a controlled way. |
This PR adds hardware button support for e-ink displays with GPIO controls.
Features
🔘 Hardware Button Support
Default button mapping:
Settings Improvements
Plugin Button API
Plugins can implement
on_button_press()to handle button events:Photo Frame Plugin
Test plugin demonstrating button navigation (B/C for prev/next photo).
Technical Details
New Module:
src/buttons/abstract_button_handler.py- Base class and enumsbutton_manager.py- Event routing and action executiongpio_button_handler.py- GPIO implementation using gpiozeromock_button_handler.py- Development/testing stubDependencies
gpiozero>=2.0.1lgpio>=0.2.2.0📸 UI Screenshots