-
Notifications
You must be signed in to change notification settings - Fork 53
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # src/bridge/bridge-unix-sockets.cpp
# Conflicts: # .github/workflows/c-cpp.yml
# Conflicts: # .github/workflows/c-cpp.yml
There was a problem hiding this 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.
Tracker reconnection/readding broke after reverting Originally on each reconnection there was a new device instance created with these flags set to |
IO is now handled by libuv. Pose requests now happen at 400-500hz to minimize latency, decoupled from
RunFrame
.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.