CertainLach / VivePro2-Linux-Driver

SteamVR driver for VivePro2 on Linux
GNU General Public License v2.0
71 stars 9 forks source link

Power management doesn't work #36

Open digetx opened 3 months ago

digetx commented 3 months ago

Two problems:

  1. Rust drops stations that are added to self.stations, apparently something is wrong with ownership here: https://github.com/CertainLach/VivePro2-Linux-Driver/blob/8982075490c242bfb291cf997c07ade37357e558/bin/driver-proxy/src/driver/server_tracked_provider.rs#L87

the stations vec has 2 elements, but self.stations has 0 after .extend is invoked. I'm not fluent in Rust, please help fixing it.

  1. The vivepro2 doesn't have basestationPowerManagement setting in steamvr.vrsettings, the steamvr has it https://github.com/CertainLach/VivePro2-Linux-Driver/blob/8982075490c242bfb291cf997c07ade37357e558/bin/driver-proxy/src/driver/server_tracked_provider.rs#L28
   "steamvr" : {
      "basestationPowerManagement" : 1
   },
   "vivepro2" : {
      "brightness" : 130,
      "noiseCancel" : false,
      "resolution" : 4
   }

i.e. the fix is:

- const POWER_MANAGEMENT: Setting<i32> = setting!("vivepro2", "basestationPowerManagement");
+ const POWER_MANAGEMENT: Setting<i32> = setting!("steamvr", "basestationPowerManagement");

Maybe this setting depends on a steamvr version? I'm using today's latest beta v2.6.2

CertainLach commented 3 months ago

the stations vec has 2 elements, but self.stations has 0 after .extend is invoked. I'm not fluent in Rust, please help fixing it.

Are you sure it has zero elements after extend? a.extend(b) just moves elements from b to a

The vivepro2 doesn't have basestationPowerManagement setting in steamvr.vrsettings, the steamvr has it

You can just define it, settings file only has settings that were already set. Whole vivepro2 section is defined by this driver

digetx commented 3 months ago

Are you sure it has zero elements after extend? a.extend(b) just moves elements from b to a

I applied this:

diff --git a/bin/driver-proxy/src/driver/server_tracked_provider.rs b/bin/driver-proxy/src/driver/server_tracked_provider.rs
index c793cd8..36992c0 100644
--- a/bin/driver-proxy/src/driver/server_tracked_provider.rs
+++ b/bin/driver-proxy/src/driver/server_tracked_provider.rs
@@ -83,8 +83,11 @@ impl IServerTrackedDeviceProvider for ServerTrackedProvider {
                        StationControl::new(manager.clone(), name.to_owned(), StationState::On)
                    })
                    .collect();
-               info!("enabled power management for {} stations", stations.len());
+               println!("enabled power management for {} stations", stations.len());
                self.stations.lock().expect("lock").extend(stations);
+
+               println!("actual enabled power management for {} stations",
+                        self.stations.lock().expect("lock").len());
            }
        };

and it gives me this:

enabled power management for 2 stations
actual enabled power management for 0 stations

This makes me think that there is a problem with ownership.

You can just define it, settings file only has settings that were already set. Whole vivepro2 section is defined by this driver

When I'm enabling station power management in the SteamVR UI, SteamVR adds new steamvr section with basestationPowerManagement setting to the steamvr.vrsettings file. The vivepro2 section doesn't have basestationPowerManagement, and thus, ServerTrackedProvider is never initialized because it wants setting from the vivepro2 section and by default standby_state = StationState::Unknown, if I'm not missing anything.

Could you please clarify what do you mean by "You can just define it"? What, where and why to define?

SteamVR wipes steamvr.vrsettings on next restart when I'm manually adding basestationPowerManagement to the vivepro2 section, if that's what you meant.

CertainLach commented 3 months ago

This makes me think that there is a problem with ownership.

No, that's something else, rust ownership system is compile-time only. Don't you have "disconnecting from base stations" message somewhere in between?.. Or maybe SteamVR thread safety guarantees are changed, and there is an UB now (this code is not particularly safe, because SteamVR api guarantees are not properly documented, and this driver has some expectations on how those methods are called).

When I'm enabling station power management in the SteamVR UI, SteamVR adds new steamvr section with basestationPowerManagement setting to the steamvr.vrsettings file. The vivepro2 section doesn't have basestationPowerManagement, and thus, ServerTrackedProvider is never initialized because it wants setting from the vivepro2 section and by default standby_state = StationState::Unknown, if I'm not missing anything.

steamvr.basestationPowerManagement is responsible for SteamVR own base station power management, which was recently added to SteamVR for linux, but doesn't work correctly for everyone. To prevent conflicts with it, my driver defines a different option: vivepro2.basestationPowerManagement, which controls this driver own power management system.

SteamVR wipes steamvr.vrsettings on next restart when I'm manually adding basestationPowerManagement to the vivepro2 section, if that's what you meant.

It only wipes it on syntax error, seems like you're adding it wrong.

digetx commented 3 months ago

No, that's something else, rust ownership system is compile-time only. Don't you have "disconnecting from base stations" message somewhere in between?.. Or maybe SteamVR thread safety guarantees are changed, and there is an UB now (this code is not particularly safe, because SteamVR api guarantees are not properly documented, and this driver has some expectations on how those methods are called).

Doesn't look related to threading. I replaced StationControl::new vec with a vec of station name Strings and getting same result of .extend not working, maybe it doesn't work well together with .collect?

It only wipes it on syntax error, seems like you're adding it wrong.

Right, there was a syntax error. Now stations are powering up when StationControl::new is invoked using vivepro2 section because initial new state=on, but then hitting this offending empty stations' vec.

CertainLach commented 3 months ago

vec with a vec of station name Strings and getting same result of .extend not working, maybe it doesn't work well together with .collect?

Rust has no such magic, collect collects iterator into a vec, and vec.extend extends vector by iterator, something nasty is happening here, either the system is being destroyed (which you can see by "disconnecting from base stations" message in logs if it happens), or there is an UB, which may be caused by this driver not using SteamVR threading correctly.

digetx commented 3 months ago

There is no "disconnecting from base stations". StationControl connects and waits for commands.

digetx commented 3 months ago

To make it more clear, "disconnecting from base stations" happens only on the SteamVR exit.