Skip to content
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

fix(CompositeDevice): Ensure proper handling of chord events in a way steam can understand them. #32

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

pastaq
Copy link
Contributor

@pastaq pastaq commented Apr 1, 2024

  • Add default configuration for steam that covers 99% of handhelds.

@pastaq pastaq marked this pull request as ready for review April 1, 2024 05:47
Copy link
Contributor

@ShadowApex ShadowApex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue I see with the current implementation is that the sleep calls that happen when performing these chords will halt the processing of all events for the CompositeDevice while its sleeping.

To avoid this, I propose we actually spawn a tokio task that will sleep for the desired amount of time, then send a Command using the sender side of the CompositeDevice channel to tell the device it should process the event after the sleep has finished. Since the sleep is happening in its own task, it will not block the CompositeDevice from processing other events.

e.g.

pub enum Command {
    ...
    WriteEvent(NativeEvent),
...
...
tokio::spawn(async move {
    tokio::time::sleep(Duration::from_millis(80)).await;
    if let Err(e) = tx.send(Command::WriteEvent(event)) {
        log::error!("Failed to send chord event command: {:?}", e);
    }
});
...

Then in the run loop, we can process the command sent through the channel after the delay:

match cmd {
    ...
    Command::WriteEvent(event) => {
        if let Err(e) = self.write_event(event).await {
            log::error!("Failed to write event: {:?}", e);
        }
    }
...

.iter()
.position(|n| n.clone().as_capability() == event.as_capability());
if idx != Some(events.len() - 1) {
sleep(Duration::from_millis(80));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using thread::sleep, we should be using tokio::time::sleep instead whenever we need to sleep in an async function. Using thread::sleep is something we should only use for synchronous code to prevent holding up the async runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this, the tokio sleeps don't affect the thread we need to sleep so everything breaks.

@pastaq pastaq force-pushed the pastaq/chord_fix branch from de80c16 to fbac03c Compare April 1, 2024 20:39
@pastaq
Copy link
Contributor Author

pastaq commented Apr 1, 2024

The main issue I see with the current implementation is that the sleep calls that happen when performing these chords will halt the processing of all events for the CompositeDevice while its sleeping.

To avoid this, I propose we actually spawn a tokio task that will sleep for the desired amount of time, then send a Command using the sender side of the CompositeDevice channel to tell the device it should process the event after the sleep has finished. Since the sleep is happening in its own task, it will not block the CompositeDevice from processing other events.

e.g.

pub enum Command {
    ...
    WriteEvent(NativeEvent),
...
...
tokio::spawn(async move {
    tokio::time::sleep(Duration::from_millis(80)).await;
    if let Err(e) = tx.send(Command::WriteEvent(event)) {
        log::error!("Failed to send chord event command: {:?}", e);
    }
});
...

Then in the run loop, we can process the command sent through the channel after the delay:

match cmd {
    ...
    Command::WriteEvent(event) => {
        if let Err(e) = self.write_event(event).await {
            log::error!("Failed to write event: {:?}", e);
        }
    }
...

We can't do this if we want to support devices that use on-release instead of momentary action for their buttons. Take for example the ROG Ally, arguably the second most popular handheld after the Steam Deck. The CC and AC buttons do not emit events when pressed, rather they release events only on release. Approximately 1ms after the "down" events fire the "up" events fire. If we use a tokio to delay before the event is transmitted then the "up" event will arrive 79ms before the "down" event. We could also add an 80 ms delay before any "up" chords but that still only provides 1ms for steam to recognize the chord and use the events, leading to multiple event pass-throughs.

@pastaq pastaq force-pushed the pastaq/chord_fix branch from fbac03c to 2bfb521 Compare April 1, 2024 22:13
@pastaq pastaq force-pushed the pastaq/chord_fix branch from 2bfb521 to 6d47bf0 Compare April 1, 2024 22:16
Copy link
Contributor

@ShadowApex ShadowApex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@ShadowApex ShadowApex merged commit 620ec53 into main Apr 2, 2024
2 checks passed
@ShadowApex ShadowApex deleted the pastaq/chord_fix branch April 2, 2024 00:19
Copy link

github-actions bot commented Apr 2, 2024

🎉 This PR is included in version 0.10.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants