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

Implement async IPC #38

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Implement async IPC #38

wants to merge 42 commits into from

Conversation

0forks
Copy link

@0forks 0forks commented Mar 22, 2023

IO is now handled by libuv. Pose requests now happen at 400-500hz to minimize latency, decoupled from RunFrame.

  • Added some basic tests;
  • Dependencies are now pinned using vcpkg submodule as recommended, readme and CI updated accordingly;
  • Fixed some code style inconsistencies.

Tested with ALVR, WMR on Windows combined with SlimeVR/SlimeVR-Server#629.
Reported by other testers that works on Linux.
Needs more testing on Linux.

@0forks 0forks marked this pull request as ready for review March 27, 2023 21:20
@ButterscotchV ButterscotchV added Type: Enhancement Adds or improves a feature Difficulty: Large Refactor Requires modifying large sections of the codebase Priority: Normal The default priority labels May 27, 2023
Copy link
Member

@kitlith kitlith left a comment

Choose a reason for hiding this comment

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

Sorry it's taken so long for me to review this. Overall, I think you've raised the code quality of the driver, even if I wish some of the style/formatting changes had been done in a separate PR to make this one easier to review. (However, I never spoke up previously to comment on or ask for that, so that's on me.) Almost every time I went "wait, that's weird" it ended up being original code that was reformatted, or a quirk of the original code that was faithfully reproduced, and I'm not going to argue with that.

None of my review comments block this PR. (but to be clear, I would appreciate some response.) They fall in a few categories:

  • Questions that I'd like an answer for, but require no action and I won't loose sleep over not knowing.
  • nits about byteswap that I'd prefer were addressed, but are ultimately unimportant. (it'll work either way.)
  • pointing out things that others might find interesting.

There are a couple limitations to what I've reviewed at this time:

  • I haven't viewed libuv documentation, so I'm basically going off of vibes, and taking it on faith since there are multiple reports of this code working.
  • Similarly, I noticed that you've started using atomics in some places to account for the newly multi-threaded nature of this code. I don't have the spoons to check if you've missed anything. (I haven't done any multithreaded programming in C++, and Rust will largely just fail to compile if you haven't justified yourself correctly.)
  • I've mostly glossed over the test code.

@0forks
Copy link
Author

0forks commented Mar 9, 2025

Tracker reconnection/readding broke after reverting pose = MakeDefaultPose(); 208d86e, now we explcitly set deviceIsConnected/poseIsValid flags when updating pose 279b469.

Originally on each reconnection there was a new device instance created with these flags set to true by default, now we're reusing the existing devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: Large Refactor Requires modifying large sections of the codebase Priority: Normal The default priority Type: Enhancement Adds or improves a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants