ryankurte / rust-streamdeck

libusb based driver for Elgato StreamDeck devices
Mozilla Public License 2.0
59 stars 24 forks source link

Implemented Input events, SD+ support (partial) #33

Open BIightning opened 1 month ago

BIightning commented 1 month ago

Hi @ryankurte , I implemented some new features in my fork

Input events

image

As you can see, I added a new get-input command that returns an input event enum on press. It's obviously not as lightweight as the current get-buttons command, but much nicer to work with in my opinion.

Streamdeck Plus Support

Buttons, Dials (Press and turn) and Touch (short, long, drag) input events work as you can see in the image above.

The only thing that doesn't work right now is writing images to the touchscreen. I took the python library as a reference, but everytime I try writing to the touchscreen, it shows garbage and crashes shortly after. Not sure why, but I normally don't program "low level" stuff like this. Could use help on that :) Also the main reason why I didn't open a pull request yet.

Probe

Just a simple function that returns all connected Streamdecks. image It's a bit pointless with the current cli, as you need to provide a pid to run the probe command.

The branch a bit messy right now, as I created several near identical functions for the touchscreen image logic but I'd really like to collaborate and bring this to the main repo. What do you think?

ryankurte commented 1 month ago

hey @BIightning these seem like great improvements!

if you'd like to contribute my preference would be separate PRs for different functions / units of work, it's a little more hassle from your side but makes reviewing and thinking about things easier for me. some feedback:

  1. the probe function looks useful, i'd be tempted to rework the CLI so the filters are optional arguments and it defaults to the first probed device matching whatever filters are provided. some weird shit can happen with some OS' when trying to create and destroy HidApi instances and re-connect to devices, but there's no easy fix so probably leave it till it causes a problem.

  2. input events seem like a good improvement, maybe the InputManager could wrap a StreamDeck instance to provide the more ergonomic API? that way there's a sorta separation between the high-level and low-level interfaces.

  3. Reverse-engineering low-level stuff can be rough, for USB it's often useful to dump the endpoint writes/reads from a working example to compare against / add as tests, either by injecting print statements if you have access to the library, or wireshark / hardware USB capture if you don't...

From a quick look at your code it seems like you're missing the padding on the last maybe partial chunk? also in rust you can replace a bunch of the slicing things up logic with .chunks(N) (though you still have to keep track of bytes written to identify the last chunk).

BIightning commented 1 month ago

Alright, sounds good to me, I'll split it up into 4 pull requests then?

1) Ok, I'll add the probe fn & command. If you'd like to adjust the cli, you/we can do that later?

2) You are right, the input manager wrapping a deck instance makes more sense, I'll change that.

3) I actually apply the padding on the last chunk, as I re-initialize the buf vec with zeroes here. I logged it and it looks fine. I'll investigate the problem further. slice.chunks(n) looks nice, good suggestion.

BIightning commented 1 month ago

So, for the get-buttons method with SD Plus:

The second byte that the SD sends determines what input type is used: Buttons(0), Touchscreen(2) or Dial(3).

If we receive one of the latter input types, we get incorrect button input data because the data is written to some of the same indeces used for buttons.

We can easily catch it, but what should we do here? Return a new error variant (maybe UnsupportedInput) or return an zero filled vec like it is implemented now?

An error would immediatly stop the cli with option --continuous right now.

EDIT: Erroring out seems the most logical imo, will implement that instead