fix: error not reported from getStringTrackedDeviceProperty

[risa2000]

In the original code, when the call to IVRSystem::GetStringTrackedDeviceProperty failed with error (e.g. "TrackedProp_UnknownProperty") but the returned buffer size was 0, the wrapper returned empty string, instead of signalling the error.

The fix first tests for "buffer too small" condition and only after dealing with that does the regular error code check.

[cmbruns]

@risa2000 Thank you for the pull request. I'll probably need to adjust the translator code rather than take this pull request directly; probably around here somewhere: https://github.com/cmbruns/pyopenvr/blob/master/src/translate/model.py#L99

Can you possibly please share a brief test program that would demonstrate the problem fixed by this pull request? To help guide me in making a more general fix.

[risa2000]

If you have original Vive you can use following code:

import openvr as ovr

vrsys = ovr.init(ovr.VRApplication_Other)
#  1009 : Prop_ConnectedWirelessDongle_String = [error: TrackedProp_UnknownProperty]
prop = vrsys.getStringTrackedDeviceProperty(0, ovr.Prop_ConnectedWirelessDongle_String)
print(repr(prop))

The unpatched version will return empty string.

D:\Work\python\openvr>py test_str_exc.py
''

The patched version will (correctly) raise the exception:

D:\Work\python\openvr>py test_str_exc.py
Traceback (most recent call last):
  File "test_str_exc.py", line 5, in <module>
    prop = vrsys.getStringTrackedDeviceProperty(0, ovr.Prop_ConnectedWirelessDongle_String)
  File "C:\Program Files\Python37\lib\site-packages\openvr\__init__.py", line 3036, in getStringTrackedDeviceProperty
    openvr.error_code.TrackedPropertyError.check_error_value(error.value)
  File "C:\Program Files\Python37\lib\site-packages\openvr\error_code\__init__.py", line 23, in check_error_value
    raise error_class(error_value, message)
openvr.error_code.TrackedProp_UnknownProperty

On the original Vive the other properties, which are reported as Unknown by OpenVR but incorrectly supplied with empty string value by the wrapper are:

1009 : Prop_ConnectedWirelessDongle_String = [error: TrackedProp_UnknownProperty]
1031 : Prop_DriverVersion_String = [error: TrackedProp_UnknownProperty]
1036 : Prop_RegisteredDeviceType_String = [error: TrackedProp_UnknownProperty]
1042 : Prop_AdditionalDeviceSettingsPath_String = [error: TrackedProp_UnknownProperty]
1045 : Prop_AdditionalSystemReportData_String = [error: TrackedProp_UnknownProperty]
1046 : Prop_CompositeFirmwareVersion_String = [error: TrackedProp_UnknownProperty]
2048 : Prop_DriverProvidedChaperonePath_String = [error: TrackedProp_UnknownProperty]
2090 : Prop_DashboardLayoutPathName_String = [error: TrackedProp_UnknownProperty]

I have confirmed the behavior by running the same query directly over C API.

[risa2000]

I guess you can "reapply" the patch to the translator as it is. The idea does not change. The important point is to not check the returned buffer size before checking the returned error code. When the lower layer returns an error (which is different from "Buffer Too Small") then this error takes precedence over the returned buffer size as it does not matter anymore.

[cmbruns]

There might be some other <Foo>Error_BufferTooSmall cases to be handled in the general case.

[risa2000]

If the code comes from the same template, it is possible. I have been however only working extensively with the properties and there only with the properties for "normal" devices, i.e. the headset, lighthouses, controllers, and there I noticed the problem only on String properties.

But I noticed that the wrapper does not implement the Array properties at all, so it might be potential candidate for the false positives too as it has basically the same semantics.

[cmbruns]

I'm planning to do something like this instead:

...
bufferSize = fn(deviceIndex, prop, None, 0, byref(error))
try:
    openvr.error_code.TrackedPropertyError.check_error_value(error.value)
except openvr.error_code.BufferTooSmallError:
    pass
if bufferSize == 0:
    return ''
value = ctypes.create_string_buffer(bufferSize)
fn(deviceIndex, prop, value, bufferSize, byref(error))
openvr.error_code.TrackedPropertyError.check_error_value(error.value)
return bytes(value.value).decode('utf-8')

where BufferTooSmallError will be a new additional base class for all the FooError_BufferTooSmall exceptions

[risa2000]

I see what you mean - an universal way to cover all the bases. Just one question though: Is the test:

if bufferSize == 0:
    return ''

Still necessary? You call the original function with 0-sized buffer, so if the returned buffsize is zero too, it should not return an error "buffer too small" in the first place. However since it returned this error, I guess the buffsize must be greater than zero (the original value).

There is one detail though. The corresponding C function IVRSystem::GetStringTrackedDeviceProperty returns as the buffsize the number of chars in the string + the terminating zero. So technically, returning the buffsize == 0 does not mean "the empty string" (this one would require 1 sized buffer), but an invalid property, as the doc and the header indicate. I would however prefer checking the error code (which, for some reason the doc and the header omit).