New PR for #19(Watchdog terminates the driver when debugging longer than 10s) #33

DarkNebula0 · 2023-01-19T13:40:17Z

Fix for SteamVR quits when debugging SlimeVR server for more than 10 seconds

Rebased with new changes
Wrapped linux socket client

funnbot · 2023-01-26T21:27:02Z

I thought this is what the --keepalive flag when launching vrserver is for.

Also, I'm not positive on the windows implementation for the bridge, but the linux one was written to be non-blocking, so wrapping it in a thread is fine, but doesn't do much.

On the windows side, isn't there an option to set a timeout instead of blocking to wait for messages?
Using a thread is better, I'm just curious.

DarkNebula0 · 2023-01-26T21:42:35Z

I thought this is what the --keepalive flag when launching vrserver is for.

Also, I'm not positive on the windows implementation for the bridge, but the linux one was written to be non-blocking, so wrapping it in a thread is fine, but doesn't do much.

On the windows side, isn't there an option to set a timeout instead of blocking to wait for messages? Using a thread is better, I'm just curious.

As far as I know there is no flag, you could use SetCommTimeouts or change the pipes to Overlapped(Event based), but you would have to do that in SlimeServer as well.

When I wrote it there was no linux implementation implemented. But it's been a few months :)

https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setcommtimeouts

kitlith

really, these are nitpicks. I have some other nitpicks that were not introduced as part of this PR, so I'm not opposed to merging as-is and cleaning it up later as I build on top of this to work on #34.

kitlith · 2023-02-13T23:23:05Z

src/bridge/bridge.hpp

+            BRIDGE_ERROR = 2
+        };
+
+        explicit Bridge(VRDriver *i_pDriver);


why a pointer over a reference?

Because I pass the class pointer from VRDriver -> src/VRDriver.cpp line 9

You can still use a reference if you deref the pointer before passing it, like the old code was doing when it was passing a reference to the bridge every time it did something.

kitlith · 2023-02-13T23:25:51Z

src/VRDriver.hpp

@@ -52,6 +54,7 @@ namespace SlimeVRDriver {
        vr::HmdVector3_t GetPosition(vr::HmdMatrix34_t &matrix);

        bool sentHmdAddMessage = false;
+        Bridge *m_pBridge;


Currently, we do not use (systems) hungarian notation in the driver. To be fair though, this is somewhat of a nitpick.

Suggested change

Bridge *m_pBridge;

Bridge *bridge;

should've included the suggestion in the original comment.

I don't want to make suggestions for every occurrence, because basically every single variable/parameter that you added does not follow the style of the rest of the driver, and I don't want to spam the PR.

kitlith · 2023-02-13T23:27:37Z

src/bridge/bridge-unix-sockets.cpp

+    this->stop();
+}
+
+void SlimeVRDriver::Bridge::run() {


This appears to consist entirely of platform independent code, but is duplicated between windows pipes and unix sockets.

kitlith · 2023-02-13T23:39:03Z

src/bridge/bridge-unix-sockets.cpp

+    messages::ProtobufMessage oMsg;
+
+    while (!this->m_bStop) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(3));


Ideally, we'd wait for the OS to notify us that there's a message incoming, or we have a message in the queue that needs to be sent instead of sleeping for <arbitrary amount of time>.

The sleep is a common practice to allow the cpu to switch the context to avoid high utilization.

In my eyes it would be the best to remove the pipes completely and send the data over TCP e.g. with Asio. Then you have only one implementation that works for windows and linux.

I'm talking about using select or WaitForMultipleObjects instead of sleeping, meaning the OS can wake us up as soon as either there's an incoming message, or another thread sends a notification that there's an outgoing message, and we don't need to periodically wake-up to poll the outgoing queue.

This would still be applicable to a TCP based implementation, unless you had the other thread just send the message directly. (But that optimization would also apply to this implementation, so eh)

This doesn't need to be done right now, so I'll resolve the conversation after adding a TODO.

Suggested change

std::this_thread::sleep_for(std::chrono::milliseconds(3));

// TODO: use select, or a timeout, or just send the messages from another thread.

std::this_thread::sleep_for(std::chrono::milliseconds(3));

src/bridge/bridge.hpp

kitlith · 2023-02-20T21:47:53Z

src/bridge/bridge-windows-pipes.cpp

+    messages::ProtobufMessage oMsg;
+
+    while (!this->m_bStop) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(3));


Suggested change

std::this_thread::sleep_for(std::chrono::milliseconds(3));

// TODO: use WaitForMultipleObjects, or a timeout, or just send the messages from another thread.

std::this_thread::sleep_for(std::chrono::milliseconds(3));

rebase for old pr(SlimeVR#19) and wrap linux socket impl

63a9a0e

Eirenliel approved these changes Jan 27, 2023

View reviewed changes

Eirenliel requested a review from kitlith January 27, 2023 12:10

kitlith mentioned this pull request Feb 13, 2023

HMD Position -> SlimeVR Server -> Tracker Position is likely always >= 1 frame behind #34

Open

kitlith reviewed Feb 13, 2023

View reviewed changes

src/bridge/bridge.hpp Show resolved Hide resolved

kitlith reviewed Feb 20, 2023

View reviewed changes

ButterscotchV added Type: Bug Something isn't working Status: Help Wanted Needs some help to make progress Priority: Normal The default priority labels May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New PR for #19(Watchdog terminates the driver when debugging longer than 10s) #33

New PR for #19(Watchdog terminates the driver when debugging longer than 10s) #33

DarkNebula0 commented Jan 19, 2023

funnbot commented Jan 26, 2023 •

edited

Loading

DarkNebula0 commented Jan 26, 2023

kitlith left a comment

kitlith Feb 13, 2023

DarkNebula0 Feb 14, 2023

kitlith Feb 20, 2023

kitlith Feb 13, 2023

kitlith Feb 20, 2023

kitlith Feb 13, 2023

kitlith Feb 13, 2023

DarkNebula0 Feb 14, 2023

kitlith Feb 20, 2023

kitlith Feb 20, 2023

kitlith Feb 20, 2023

	std::this_thread::sleep_for(std::chrono::milliseconds(3));
	// TODO: use select, or a timeout, or just send the messages from another thread.
	std::this_thread::sleep_for(std::chrono::milliseconds(3));

	std::this_thread::sleep_for(std::chrono::milliseconds(3));
	// TODO: use WaitForMultipleObjects, or a timeout, or just send the messages from another thread.
	std::this_thread::sleep_for(std::chrono::milliseconds(3));

New PR for #19(Watchdog terminates the driver when debugging longer than 10s) #33

Are you sure you want to change the base?

New PR for #19(Watchdog terminates the driver when debugging longer than 10s) #33

Conversation

DarkNebula0 commented Jan 19, 2023

funnbot commented Jan 26, 2023 • edited Loading

DarkNebula0 commented Jan 26, 2023

kitlith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funnbot commented Jan 26, 2023 •

edited

Loading