Add Stream Deck Plus rotary encoder support#143
Add Stream Deck Plus rotary encoder support#143yalec wants to merge 1 commit intoenertial:masterfrom
Conversation
Add Stream Deck Plus rotary encoder support Features: - Rotary encoder rotation with separate CW/CCW increment values - Encoder press support with fixed value - LCD display using setFeedback API with B2 layout - Value-to-text mapping system with dynamic UI - Automatic gauge/indicator based on min/max/current values - Real-time settings update without restart Technical changes: - Add dialRotate, dialDown, dialUp event handlers - Create encoder_prop_inspector.html for configuration - Implement getCurrentDisplayValue() with mapping support - Update manifest.json with two encoder actions - Upgrade Visual Studio toolset to v145 (VS 2025) - Update README with encoder features documentation - Add CONTRIBUTING.md for fork information
|
Thanks for putting up a PR! I'll try to review it soon, but may not get to it until after Christmas. |
charlestytler
left a comment
charlestytler
left a comment
There was a problem hiding this comment.
This is fantastic! Thanks for working out the Streamdeck rotary details and also following my existing codebase style.
I don't have a rotary device so can't test it fully, but it looks reasonable. I've requested some changes in mostly just organization to better align with how different button types are separated. And sorry for the code review delay, it took me a while to re-orient myself in the codebase after not looking at it for several years.
One final note, since you're the first person other than myself to dig into the codebase in 5 years, let me know if some of the naming was confusing. In my re-review I feel "StreamdeckContext" which refers to the context instantiated for each button/rotary is not too obvious, but StreamdeckButtonContext doesn't really work if rotary encoders are now included.
I'll also look into the github actions that seem to have gone stale so C++ tests and linters can run.
| mConnectionManager->LogMessage("[DialPress] Simulator NOT connected"); | ||
| } | ||
| } else { | ||
| mConnectionManager->LogMessage("[DialPress] Context NOT found in visible contexts"); |
There was a problem hiding this comment.
Leaving a reminder - can these LogMessages be removed before landing?
| } | ||
|
|
||
| // Update encoder display with current increment value if this is an encoder action | ||
| if (send_action_) { |
There was a problem hiding this comment.
It would be helpful if the logic in here could be contained to its own encoder_display_monitor_ (or similar name) entity along with other items in the ExportMonitors/ directory. Then it can also be easily tested outside of this updateContextState() function.
| */ | ||
| void handleEncoderPress(SimulatorInterface *simulator_interface, | ||
| ESDConnectionManager *mConnectionManager, | ||
| const json &inPayload); |
There was a problem hiding this comment.
Since the context for Streamdeck encoders seem to not share events with the Button press/release increment actions, I think it would be cleaner to create a new EncoderAction.h with these four functions.
This new action can still use an IncrementMonitor internally to help track state.
| button_action_from_uuid_["com.ctytler.dcs.increment.dial.two-state"] = ButtonAction::INCREMENT; | ||
| button_action_from_uuid_["com.ctytler.dcs.increment.textdial.two-state"] = ButtonAction::INCREMENT; | ||
| button_action_from_uuid_["com.ctytler.dcs.encoder.rotary"] = ButtonAction::INCREMENT; | ||
| button_action_from_uuid_["com.ctytler.dcs.encoder.rotary.text"] = ButtonAction::INCREMENT; |
There was a problem hiding this comment.
Creating a new ButtonAction::ENCODER enum to map to a new EncoderAction will help avoid dealing with the dynamic casts I believe below, as the factory will return a unique pointer to an EncoderAction.
| int ticks) | ||
| { | ||
| // Cast to IncrementAction to access encoder-specific methods | ||
| auto *increment_action = dynamic_cast<IncrementAction *>(send_action_.get()); |
There was a problem hiding this comment.
I'm not sure if this dynamic cast should be needed, as the SendActionFactory should be constructing the correct Action type of send_action_. Maybe there was an issue you were running into.
| std::string current_title_ = ""; // Stored title of the context. | ||
| std::string last_encoder_display_value_ = ""; // Last value displayed on encoder LCD. | ||
| std::string last_encoder_image_path_ = ""; // Last image path set for encoder background. | ||
| json settings_; // Stored settings for this context. |
There was a problem hiding this comment.
Would prefer to avoid keeping this raw json settings as stored state. Having an encoder monitor would allow it to access the json settings from updateContextSettings(json settings) and save just what it needs.
|
@yalec I've landed #145 which fixed the broken github actions so it would be good to merge master on this. I'm also working on a PR to migrate from Visual Studio management of build files to Cmake which will hopefully make things easier going forward. But let me know if you intend to edit this anytime soon and I can hold off landing it to save you some rebasing. Let me know if you have any questions... or if you don't have time to continue modifying this PR for a while. Thanks. |
2 participants
Add Stream Deck Plus rotary encoder support
Features:
Technical changes: